migrate mapping & messages table names are truncated, can lead to incorrect mapping lookups

I guess we could throw an exception.

Yes, more information is needed I was mostly curious to know what tests which fail, which I should have mentioned. There are two challenges here, 1) decide on the message and 2) changing the ids on existing migrations needs to have this fixed first, #3039240: Create a way to declare a plugin as deprecated.
For the first, since we have to explain about bundles and extra characters such as '__', I wonder if the best thing is to point to a doc page with examples. For the later this, needs to be postponed.

And for fun I made a patch anyway, this time including the migration_id.

I'm thinking this needs to check for length as well as duplicate tables names. For example, when bundle names are added to they table name, they are the uniquely defining part of the string but they are at the end of the string.

or length as well as duplicate tables names

Indeed!

For example, when bundle names are added to they table name, they are the uniquely defining part of the string but they are at the end of the string.

Exactly — if we don't guarantee uniqueness, then we risk wrongly counting things. For example, multiple derived migration plugin definitions can have their source plugins returning a lower count than what the ID Map plugin will return, because all of the derived migration plugin definitions end up reading the same mapping table… 😨🤯

(Ask me how I know.)

Interesting, I didn't realize *.PATCH would disable testing!

Updated test expectations.

Glad to see progress here. I don't recall anything that would cause the map table name to vary between 9.0 and 9.1. And I just tested locally on 9.0 and MigrateLookupTest::testInvalidIdLookup passed. Retesting because, well, why not?

Not retesting. I see the failure is with SqlLite.

Queued a 9.0.x test with MariaDB. #20 sounds like SQLite is known to not pass migration tests?

Sorry, no I meant I wasn't testing with SqlLite.

+++ b/core/modules/migrate/tests/src/Kernel/MigrateLookupTest.php
@@ -72,7 +72,7 @@ public function testSingleLookup() {
-    $this->expectExceptionMessage("Extra unknown items for map migrate_map_sample_lookup_migration in source IDs: array (\n  'invalid_id' => 25,\n)");
+    $this->expectExceptionMessage("Extra unknown items for map migrate_map_sample_lookup_migra11fefc51977839762138 in source IDs: array (\n  'invalid_id' => 25,\n)");

This assumes a table prefix of 12 characters. By default on testbot, mysql and postgre, prefixes are 'test12345678' (well random 8 numbers), but on sqllite they add a period 'test12345678.' making them 13 characters, and dropping the last 'a' in migrate_map_sample_lookup_migra11fefc51977839762138.

I'm not certain that the test should rely on any specific prefix length, I think that is technically configurable within phpunit settings.

This issue is a Major bug, and from the comments it looks as though it is pretty close to being resolved. If I understand correctly, we just need to update the tests so that they do not rely on a specific prefix length, and then they should pass for SQLite.

This modifies the test to handle the different prefix length used by SQLite.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -171,9 +171,9 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
     $prefix_length = strlen($this->database->tablePrefix());
     $this->mapTableName = 'migrate_map_' . mb_strtolower($machine_name);
-    $this->mapTableName = mb_substr($this->mapTableName, 0, 63 - $prefix_length);
+    $this->mapTableName = mb_substr($this->mapTableName, 0, 43 - $prefix_length) . substr(md5($machine_name), 0, 20);
     $this->messageTableName = 'migrate_message_' . mb_strtolower($machine_name);
-    $this->messageTableName = mb_substr($this->messageTableName, 0, 63 - $prefix_length);
+    $this->messageTableName = mb_substr($this->messageTableName, 0, 43 - $prefix_length) . substr(md5($machine_name), 0, 20);

+++ b/core/modules/migrate/tests/src/Kernel/MigrateLookupTest.php
@@ -72,8 +72,13 @@ public function testSingleLookup() {
+    $table_name = 'migrate_map_sample_lookup_migra11fefc51977839762138';
+    // The table prefix length used by SQLite is one character longer so adjust
+    // the table name.
+    if (\Drupal::database()->driver() === 'sqlite') {
+      $table_name = 'migrate_map_sample_lookup_migr11fefc51977839762138';
+    }
+    $this->expectExceptionMessage("Extra unknown items for map $table_name in source IDs: array (\n  'invalid_id' => 25,\n)");

I think we need to do more. It's a balancing act, and I hate to have the test just redo the string concatation in the method, as it kills some of what we are testing for, but the test can't assume the prefix length based on the assumed prefix that testbot uses. In this case, we do need to retrieve the prefix length from \Drupal::database->tablePrefix() in the test and calculate what the table name should be, I think.

I can't decided which method I prefer. In this patch the test re-calculates the table name.

I like this better.

Fyi I just released a contrib ID map plugin that copies this solution with some small difference:

1. That plugin adds the hash only when the table names would be truncated. (And i think that this behaviour provides better DX.)
2. There are three tests: one for a short plugin ID, one for a longer one, and one for a very long one with a DERIVATE_SEPARATOR

The code is here: Smart SQL ID Map.

The postgres failure in #27 looks real.

The test didn't account for the new table name, which is not specific to Postgres, The test was committed in #3098282: [backport] SQL error if migration has too many ID fields on Sep 27. The mysql test was run on 18 Sep, sqlite on 19 Sep whereas postgres was run on 30 sep, after the new test was added.

This gets the correct table name for the test.

Silly me, I uploaded the old patch not the new one. This should be better.

Looks like this fixes Postgres, back to RTBC, thanks @catch for the um... catch.

Uploading the patch from #33 so that testing is against 9.2.x

The original IS includes adding documentation explaining the format of the table names. That documentation doesn't already exist, that I can find, so maybe add it to the property description in Sql.php?

The IS has this postponed on #3039240: Create a way to declare a plugin as deprecated which I think still stands because, for example, changing the table names will cause existing migration lookups to fail.

Setting to NR for the above.

I've changed my mind, this doesn't need to be postponed on #3039240: Create a way to declare a plugin as deprecated. What is needs is a way to handle the changing of migrate tables in a BC friendly way. Setting NW for that piece of work and updated the IS.

Here is a first attempt at the update hook and test.

This will update the table names for all discovered migrations and it assumes that if there is map table there is a message table.

Ignore that patch.

This is the same idea, it will update migrate table names for discovered migrations.

They have the same problem in the views module.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -171,9 +171,9 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
-    $this->mapTableName = mb_substr($this->mapTableName, 0, 63 - $prefix_length);
+    $this->mapTableName = mb_substr($this->mapTableName, 0, 43 - $prefix_length) . substr(md5($machine_name), 0, 20);
     $this->messageTableName = 'migrate_message_' . mb_strtolower($machine_name);
-    $this->messageTableName = mb_substr($this->messageTableName, 0, 63 - $prefix_length);
+    $this->messageTableName = mb_substr($this->messageTableName, 0, 43 - $prefix_length) . substr(md5($machine_name), 0, 20);

I was just chatting with @huzooka about this issue and the https://www.drupal.org/project/smart_sql_idmap contrib module he released to allow e.g. the https://www.drupal.org/project/media_migration to work today (i.e. without this issue having been solved).

He pointed out that it'd be far better for the Developer Experience if we'd:

1. put an underscore before the MD5 hash
2. not add an MD5 hash if the table name is short

WDYT, @quietone & @mikelutz?

EDIT: LOL I just realized that @huzooka already wrote exactly this at #29 😂🙈

@Wim Leers, thanks, I had forgotten about #29.

Yes, I think it is a good idea. It would be much better for the developer if the table names matched the migration id. This patch will only change the table name if the table name was truncated. That means less changes to tests, which is a good thing. I've add testing MigrateSqlIdMapTest and reworked the entire Update test as well.

I am not sure how useful the interdiff will be because it is bigger than the patch now.

@quietone, and you also add an underscore before the hash :)

Thanks!!!

I had problems running the update.

Here's what I did before the update. I ran a couple migrations with very long names that would get truncated to the same table names. The migration ids were:

• horizontal_horizontal_horizontal_horizontal_horizontal_posts
• horizontal_horizontal_horizontal_horizontal_horizontal_images

These both end up pointing at the same migrate tables:

• migrate_map_horizontal_horizontal_horizontal_horizontal_horizon
• migrate_message_horizontal_horizontal_horizontal_horizontal_hor

It turns out in my case the shared table doesn't cause any problems because posts and images never have the same id.

When I run migrate:status, the status is indeed confused because of the shared tables.

 ------------------- --------------------- -------- ------- ---------- 
  Group               Migration ID          Status   Total   Imported   Unprocessed    
 ------------------- --------------------- -------- ------- ---------- 
Default (default)   horizontal_horizont   Idle     1116    1491       -381        
                      al_horizontal_horiz                                                                  
                      ontal_horizontal_im                                                                  
                      ages                                                                                 
  
Default (default)   horizontal_horizont   Idle     381     1491       -1116       
                      al_horizontal_horiz                                                                  
                      ontal_horizontal_po                                                                  
                      sts  

Then I applied the patch in #44 and ran updb. I received the following output:

>  [notice] Update started: migrate_update_9201
>  [error]  Cannot rename 'migrate_map_horizontal_horizontal_horizontal_horizontal_horizon' to 'migrate_map_horizontal_horizontal_horizontal__6bea8c6829a77901b': table 'migrate_map_horizontal_horizontal_horizontal_horizontal_horizon' doesn't exist. 
>  [error]  Update failed: migrate_update_9201 
 [error]  Update aborted by: migrate_update_9201 
 [error]  Finished performing updates. 

The shared migrate map table got updated to migrate_map_horizontal_horizontal_horizontal__fbe14d486cb1d9888. But then the update hook tried to update the table name a second time for the second migration that used that (shared) table. By that point migrate_map_horizontal_horizontal_horizontal_horizontal_horizon had a different name.

This situation with shared table names (described somewhat in #15) is ugly, but I don't think the update should fail. I think the best thing may be to duplicate the shared table in this situation. Otherwise we guarantee data loss. The migrate:status output would still be confusing, but I don't think there's any way we can unscramble that egg.

Nice find! 😱

Also: that is the most absurd migration ID (or plug-in ID) I’ve ever seen 🤣👏

@danflanagan8, thanks for the thorough testing!

Part of me wonders if it is necessary to handle the case discovered by the testing in #47. While it certainly can happen, I think the developer would discover the problem of a map and/or message table being used by two or more migrations. It seems to me that just looking at the data in a 'shared' map table a developer would see that source / destination pairing were a jumble. Or course, that may be harder to see if the source/destination are both just an ID.

If this does need to be done then a single table needs to be copied to more than one new tables. I do not see any 'copy' method in the Database API. So, would this need a unique solution for each database type?

Also, a question. This patch gets a list of current map tables by finding tables with a name like 'migrate_map%'. Or should this get the map tables names from each migration ($migration->getIdMap()->mapTableName) and thus cover custom id_map plugins?

@quietone, it's certainly a reasonable question as to how far we should go to handle the case in #47 and #15. I was thinking at the very least, we should make sure the update doesn't fail by doing a check with tableExists():

if ($legacy_table_names[0] != $id_map->mapTableName()) {
  if ($this->database->schema()->tableExists($legacy_table_names[0])) {
    $this->database->schema()
        ->renameTable($legacy_table_names[0], $id_map->mapTableName());
  }
  else {
    // Print some message saying maybe data loss has occurred due to truncated table names and point to a helpful url?
  } 
}

Would that be good enough?

I do not see any 'copy' method in the Database API. So, would this need a unique solution for each database type?

It looks like the API used to have a copy function, but it was removed mostly due to it not supporting PostrgeSQL: #2061879: Remove Schema::copyTable

Multiple times in that issue there are comments like "this could be added back." (5, 11) It's pretty far out of my wheelhouse to try to manage something like that though.

Re-rolled patch for 9.3.x

Tried to fix custom failures of patch #52.

Triggering bots to check the custom commands failures.

@yogeshmpawar, to check the custom command failure just run commit-code-check locally. Have a look at the instructions for running the coding standard checks locally. It also has the advantage of saving resources, including money, for the Drupal association. There are real costs for running the tests.

@karishmaamin, I do not understand why you made a reroll the patch, it still applies to 9.3.x and the issue is not tagged for a reroll. If you read the latest comments there is a question asked in #49 which needs to be resolved before work continues.

Settings to NW for #47

Thanks @quietone - for the information. seems like link posted in your comment gives me 404. I will check this custom command failures on local first onwords.
I got the correct link - https://www.drupal.org/docs/develop/development-tools/running-core-devel...

I was thinking about this one again and thought I throw out an idea. The blocker here is that copying a db table is ridiculously painful. I wonder if there's a way we could handle this where we avoid needing an update hook. What I mean is this:

Could we somehow continue supporting the legacy table names for existing migration maps, but use the new naming scheme for new migration maps?

In general, I think that the one-off pain of an update is far better than the ongoing pain of supporting 2 systems in parallel.

Hear hear.

#2919158: Add the MigrationPluginManager to Drupal\migrate\Plugin\migrate\id_map\Sql broke this patch.

2.5 years later, we're still using #16, so rebasing that, since it's less likely to fail to apply, unlike #53.

I was thinking about #47, where two migrations are sharing a map table name. While it is rare, it is possible. If a site is in such a situation then changing the tables names could break their migrations. In fact, it would if they are using migration_lookup. A migration using a 'shared' table would start using a new table and thus not have access to the older data. If this was detected we could copy the data to the two new tables and everything should be fine.

But, maybe a simpler way is to not modify the table names when it is detected that 'shared' tables are in use. When 'shared' tables are detected a flag could be set, in key/value, to indicate that the older method for computing table name should be used. And if that flag is not set then the new method is used.

Setting to NR for comment on this idea.

BAck to NW since there is still work to do and no comments for a month

@mikelutz, before work continues I think we should have direction on how to handle #47. I proposed an idea in #65.

@quietone on #65:
I think I'd like to have two copies of the same table more than having to maintain that legacy way of handling migrate tables.
If we decided to follow the flag way, one day we would certainly like to deprecate that and prepare a new upgrade path that would be the same as the one we would code today, isn't it?

Rerolled #53 on HEAD in a MR.
Fixed phpcs and phpstan issues.
Updated the upgrade path to clone tables instead of renaming them. (I wish I could use a Merge query for this but... #3486134: Merge queries should allow setting the conditionTable)

In other situations like this (cache IDs in the database backend, maybe field/bundle names), we hash the end of the ID if the entire thing is too long. It's not great for readability but it doesn't require an update path.

@catch #71: This is exactly what this issue is about. The original issue was caused by the fact the tables names were only truncated. Now we hash the end but we still have to handle the existing values, hence the upgrade path.

I am worried about how disruptive this change could be. Since both @catch and @daffie have commented on this issue, and neither seems concerned about that, I guess it is OK. But I am adding the tag for a change record (NW for that) and I am adding a release-notes snippet to the issue summary.

Rerolled MR.
Patch attached for composer.

Edited view summary, completed most of the empty fields.

#3200743: Decouple identifier management from database Connection, introduce an IdentifierHandler class now has a MR in NR that addresses this (for the time being for table identifiers) on the Database API level.

Appears current MR has test failures and merge conflicts.

I went to just fixed the test but then did more.

• The tables for a migration are treated as a pair, that is, if the message table name needs to be hashed then both are.
• If more that one migration shares the same table name(s), then the new name is hashed and the data copied to all the new tables.
• If only one migration has a map/message pair that needs to be hashed, then the tables are renamed.
• If the table names do not need to be hashed, the table names are left as is.
• Tests improved by adding tables to cover the case when more than one migrate shares that same table name.
• The test tables have data to ensure the copy works.

This should mean that it is only in the case of more than one migration having the same tables names that tables will be copied. I think I left it copying all the duplicates instead of doing a rename for one migration and copying for the others.

With migrate_drupal being deprecated that impact the solution?

MR rerolled on main
Patch attached for composer

This one has multiple migrate maintainers wonder if it still needs architectural review?

Unfortunately this one does need a rebase now.