I have a very simple migration for migrating roles. The map table looks like this after running the migration:

+-----------+---------+--------------+---------------+
| sourceid1 | destid1 | needs_update | last_imported |
+-----------+---------+--------------+---------------+
|         3 |       3 |            0 |             0 |
|         4 |       5 |            0 |             0 |
|         5 |       4 |            0 |             0 |
+-----------+---------+--------------+---------------+

If I run a subsequent migration with --update, the map table then looks like this:

+-----------+---------+--------------+---------------+
| sourceid1 | destid1 | needs_update | last_imported |
+-----------+---------+--------------+---------------+
|         3 |       6 |            0 |             0 |
|         4 |       8 |            0 |             0 |
|         5 |       7 |            0 |             0 |
+-----------+---------+--------------+---------------+

I looked at the MigrateDestinationRole::import() code, and I think I see why. Looks like $entity->rid only gets set if $entity->rid is already set (which seems a little pointless, since if it's set and it's the wrong value, it throws an Exception).

    // Updating previously-migrated content?
    if (isset($row->migrate_map_destid1)) {
      if (isset($entity->rid)) {
        if ($entity->rid != $row->migrate_map_destid1) {
          throw new MigrateException(t("Incoming id !id and map destination id !destid don't match",
            array('!id' => $entity->rid, '!destid' => $row->migrate_map_destid1)));
        }
        else {
          $entity->rid = $row->migrate_map_destid1;
        }
      }
    }

I'm wondering why it couldn't be this:

    if (isset($row->migrate_map_destid1)) {
      $entity->rid = $row->migrate_map_destid1;
    }

Marking this as major, as it completely hosed my user migration, and added a bunch of cruft in the database.

CommentFileSizeAuthor
#3 migrate-1872446-duplicate-roles-3.patch1.86 KBmikeryan
#1 migrate-1872446-duplicate-roles-1.patch1.24 KBq0rban

Comments

q0rban’s picture

q0rban commented
StatusFileSize
new migrate-1872446-duplicate-roles-1.patch1.24 KB

Well, I tried to write a test for this, but it was failing silently, due to exceptions just getting logged to the message table, but not actually killing the test.

Here was what I added to user.test:

    $role_migration->prepareUpdate();
    $role_migration->processImport();
    $new_roles = $query->execute()->fetchAllKeyed();
    $this->verbose(print_r($new_roles, 1));
    global $_migrate_messages;
    $this->verbose(print_r($_migrate_messages, 1));
    $this->assertEqual($roles, $new_roles, t('Role migration did not create duplicates.'));

I used Migrate::setDisplayFunction() to store the messages in a global variable.

Array ( [0] => SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'Taster' for key 'name': INSERT INTO {role} (name, weight) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => Taster [:db_insert_placeholder_1] => 5 ) (/Users/jsansbury/Sites/drupal7/includes/common.inc:7036) [1] => SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'Vintner' for key 'name': INSERT INTO {role} (name, weight) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => Vintner [:db_insert_placeholder_1] => 5 ) (/Users/jsansbury/Sites/drupal7/includes/common.inc:7036) [2] => Processed 2 (2 created, 0 updated, 2 failed, 0 ignored) in 1.7 sec (73/min) - done with 'WineRole' )

The test above actually passes, because the integrity constraint makes it so that duplicates aren't possible. In my migration, the names were getting appended with a number, so I wasn't running into that, not sure how/why.

Anyway, I can confirm the bug exists in migrate itself, but don't know a way to easily write a test to prove it, without doing queries to the message table looking for these errors, which seems error prone itself. Is there a way to just get number failed or something?

I can confirm that this patch does fix the issue, though.

q0rban’s picture

q0rban commented
Status: Active » Needs review
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
StatusFileSize
new migrate-1872446-duplicate-roles-3.patch1.86 KB

I think it's just fine to check the message table, I've added a test to do that.

I'd like to keep the check on rid discrepancies - yes, you'd have to work hard to make that fail, but Murphy will someday bite someone. The base problem is that the assignment of $entity->rid is at the wrong nesting level.

I couldn't reproduce the duplicate role creation with WineRoleMigration, just the dupe keys on insert you saw in the test, so please give this a patch a try and make sure it addresses your scenario.

Thanks.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Fixed

That last patch is at least an improvement, so I've gone ahead and committed it. James, if it doesn't address your duplicate role problem, please reopen.

Thanks.

q0rban’s picture

q0rban commented

Yep, that fixed it! Thanks. :)

q0rban’s picture

q0rban commented

For the record, the reason I was getting new records instead of the integrity constraint violation is because dedupe() was being called on the name mapping:

    $this->addFieldMapping('name', 'name')
      ->dedupe('role', 'name');

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.