sync might be too strict during id comparison; rolls back everything

As mentioned by @byrond, this seems to have been fixed in 6.0.x by this commit in 🐛 sync option doesnt work with track_changes Fixed .

Version 6.0.1 does seem to fix the issue as long as the ids' "type" property is set correctly.

In my case migrate_tools was rolling back all my items because the source ID was an integer, and I had defined:

  ids:
    source_id:
      type: string
      max_length: 100

Fixing my configuration to type "integer" fixed everything:

  ids:
    source_id:
      type: integer

I overlooked it in the first place, so I'm wondering if this subtlety is documented somewhere?

Hi, sorry for my bad english.

This issue is still valid.

I have many d7 to d9 migrations suffering from this because, for example, d7_taxonomy_term_entity_translation states that entity_id will be integer, but the data coming from the db returns it as a string.

The approach used in patch #4/#17 might be incorrect because, if the id is an md5, the conversion might assume it's a numeric value when it doesn't contain letters and the strict in_array check will fail.

Attached a "workaround" that enforce the type of $source_id_values as stated by $source->getIds()

sorry, there is no point in checking repeatedly...

new patch attached

Even with patch #21 I get some unwanted rollbacks when i run a migration group with the --group=... option.

The problem is due to the way the data is stored in the state variable. We need to add the migration id as the key of the array.

Attached a proposed patch and interdiff with #21

The last submitted patch, 22: sync-id-hash-3104268-22.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

I had a situation with a unit tests that failed because it executed the same migration twice in the same page request. It wasn't enough for migrate_tools_sync state to be reset in the constructor.

I've created a Merge Request that adds the following changes to the patch in 22 (interdiff attached):

1. Reset migrate_tools_sync at the point of scanning through source rows to collect the IDs. This fixes my unit test
2. Factor that bit of code to a new method for readability.

I've also raised this as a Merge Request, rather than as a patch, since that is the workflow in place on this ticket originally. I raised a new one because things seem to have moved along quite a lot since the original was raised.

Tests fail, but the first error I'm seeing looks unrelated to my change. Is there a problem with the tests at the moment?

Unfortunately the MR from #26 cannot be applied to the latest version of the module.

While testing this behavior, I realized that it makes many, many write queries on the key_value table. This in turn results in huge MySQL bin logs which makes the server run out of memory at some point. Is anyone else facing this issue? Is it safe to say that there is an efficiency/memory optimization issue with the --sync option?

I'm afraid that the change in #26 to move the set into the point of scanning might have lead to #28. Can we revert that change please?

I updated the MR with a merge from 6.0.x and created a separate issue https://www.drupal.org/project/migrate_tools/issues/3396130 🐛 Switch GitLab CI configuration to the template developed by the DA Needs review for the pipeline failure

Thank you, tested the patch from MR !40. It seems to have fixed the rolling back everything situation for a migration that uses track_changes. Though the migration is much slower now, quite a difference. Track changes works pretty fast when not used in combination with sync. I'm not sure if this can be improved or not.

@bbu23 Could you be facing the issue referenced in https://www.drupal.org/project/migrate_tools/issues/3378047 🐛 drush migratation with --sync has suboptimal performance (v6) Active ?

More generally, I have the feeling that this thread should take into consideration the other thread I posted above, because the way migrate_tools_migrate_prepare_row currently works gets exponentially slower with time and should be fixed first, in my opinion.

@pacproduct Yes, thank you, that could be it! One of my migrations took hours with sync option on.

So what is your suggestion? Should I try the patch from the referenced issue that has a dedicated new table to sync or do I need to combine both patches?
Also, is your comment #19 from current issue still valid? I feel that it's a bit strange to have to manually specify a type for each migration. Most of the migrations have the ID coming in as string, that means modifications everywhere. Thanks

@bbu23 I believe my comment in #19 is still valid, but the best is to try it to be sure.

I feel like the global tendency in programming is to favor strict typing more and more as it tends to reduce issues and bugs.
Thus personally, I'm kind of okay with the idea of configuring what is the type of data you're expecting from the source.
Although maybe `migrate_tools` could warn you when source IDs type do not match the configured type so you know something is dubious, instead of converting it silently which causes it to rollback everything later...

Everyone's situation isn't the same, and there might be cases where having a strict source ID typing wouldn't fit one's need. From the top of my mind, the only case that could be problematic that I can think of is if the source system were to return a mix of different types as the primary ID (a mix of strings and ints, for instance). In that particular case, having a "loose type" for source IDs could be useful.
Hard for me to say which approach is better...

-- --

With regards to your case: If setting properly the source ID types on your migration configurations solves your rollback problem without patching `migrate_tools`, it feels like a quick and healthy fix in my opinion. In which case, you may apply the patch from the referenced issue if it fixes your performance problem.
Otherwise, you may have to merge the 2 patches together. Not sure how tricky this is going to be. YMMV.

scott_euser → changed the visibility of the branch 3104268-sync-might-be to hidden.

Things have changed too much to safely just resolve conflicts. This will need a more in-depth review and re-application of the MR.

scott_euser → changed the visibility of the branch 3104268-sync-might-be3 to hidden.

scott_euser → changed the visibility of the branch 3104268-sync-id-too-strict to hidden.

1. Updated code to work with new code from 🐛 drush migratation with --sync has suboptimal performance (v6) Active
2. Added an example source to the test coverage
3. Added additional test coverage method to prove the issue and demonstrate the fix: test only fails.
4. Updated issue summary to standard template with details and clear steps to reproduce

Added a diff from merge request for ease of use as patch.

Thankyou @scott_euser for updating this.

I made a small mistake with the prev patch.
Goodmorning :D

Hiding patch, you should download the patch locally as per https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → This avoids confusion for maintainers & further development as to what to work on.

Thanks!

What would be useful though is RTBC'ing it if you have reviewed (+explaining what you did to test/review). Thanks!

Looks good and works!
Thanks!

Does not apply

nicxvan → changed the visibility of the branch 3104268-sync-id-too-strict2 to hidden.

nicxvan → changed the visibility of the branch 6.0.x to hidden.

Rebased on 6.0.x

Thanks for sorting out the conflict! Still working well! Phpcs errors are unrelated and due to changes in standards I believe, will need a separate issue to solve that, but not a blocker here.

Created here for phpcs 📌 PHPCS fixes for MigrateToolsCommands Active

Here's a patch that applies against 6.0.5 (does not include tests)

1. Added change record: https://www.drupal.org/node/3507073 →
2. Created follow-up to remove deprecation: https://www.drupal.org/project/migrate_tools/issues/3507077 📌 Remove deprecation trigger at MigrateTools::addToSyncSourceIds in 7.x Postponed

Okay tests for deprecation now passing

Tested it on a migration where I have bigint as source and string as destination since integer can't be the solution (SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'sourceid1' at row 1)

Before patch:

vendor/bin/drush mim my_migration --sync
 [notice] Rolled back 74 items - done with 'my_migration'
 [notice] Processed 74 items (74 created, 0 updated, 0 failed, 0 ignored) - done with 'my_migration'

After patch:

vendor/bin/drush mim my_migration --sync
 [notice] No item has been rolled back - done with 'my_migration'
 [notice] Processed 1 item (0 created, 0 updated, 0 failed, 0 ignored) - done with 'my_migration'

Seems good to me

woops sorry for the status change ; for me it's RTBC
I leave "Needs review" until few more people test it

I'm successfully using the patch from #63 on a sync from an external MySQL data source with no issues.

Hiding patch to avoid confusion.

I can't change to RTBC since I've been doing the work so next step is someone here in the community confirming they've reviewed and tested to change the status so we can get this back infront of the maintainers now that feedback is resolved + test coverage is in place.

Apologies; I had to read up more on Drupal etiquette about changing status - I have used the patch in #63 on large datasets without issue, many of them mixing string values that start with "0" which are impossible to handle correctly without it.

Sorry wasn't aimed at you necessarily, just the notification reminded me of this issue and wanted to comment what is needed to move forward :) Thanks for confirming RTBC in any case!

Hopefully I caught everyone who contributed to this issue. Thanks for all your work on it.

Thank you!