[Solution found, needs review from Strapi] Multi-word Collection Types with many-to-many Relationships are skipped

MO-Lewis commented 1 year ago

Bug report

Required System information

Node.js version: v16.17.0
NPM version: v8.15.0
Source Strapi version: v3.5.0
Target Strapi version: v4.5.5
Source Database: MySQL
Target Database: MySQL
Operating system: Windows 10 64-bit
Which script are you running: v3-sql-v4-sql (using the MySQL .env files)

Describe the bug (TL;DR Version)

Please see here: https://github.com/strapi/migration-scripts/blob/main/v3-sql-v4-sql/migrate/helpers/relationHelpers.js#L49-L50

ManyToMany relationships in CollectionTypes with hyphens (e.g. Care-roles) are not migrated properly (screenshots in the full description below).

The following lines need to have the value of modelF and attributeF wrapped in snakeCase(), like so:

addRelation(
        {
          uid,
          model: collectionName,
          attribute: key,
          type: 'manyToMany',
          modelF: snakeCase(value.collection),
          attributeF: snakeCase(value.attribute),
        },
        relations
      );

Through testing on my side, wrapping just modelF seems to work. However, no better / worse behavior was observed when adding it to attribtueF, so ideally I'd need a second pair of eyes from the Strapi team to check for any consequences when adding it to the attributeF field.

Describe the bug (Full Version)

I have two CollectionTypes that interact with one another, articles and care-roles. They hold a many-to-many relationship, where many articles can hold many care-roles.

When I run the migration scripts, I seem to lose all relationships relating to the care-roles collection. I would expect to see something like this in the migration script console output, when run:

Migrating X items from articles_care_roles__care_roles_articles to articles_care_roles_links

However, no message appears. This means the migration script has skipped over it for some reason. I spent a few hours debugging the migration script in VSCode line-by-line, and found the cause of it.

As shown in this screenshot, this will mean that the migrations script is looking for a table called articles_care-roles__care-roles_articles (note the hyphens), which doesn't exist, and so is skipped over.

Whilst looking for potential fixes to this, I could see that the one-to-many addRelation() function uses snakeCase(). This sounded like something I needed for my many-to-many relationship. I added it like so and re-ran the script:

This seemed to fix the issue and all of my relationships migrated properly upon re-run! I ran this fix twice, once with just the modelF value snakecased, and once with both modelF and attributeF snakecased.

There appears to be no difference in behavior; both fixed the issue properly, which makes me wonder if I'm potentially causing any other issues under-the-hood with this.

It would be much appreciated if I could have this reviewed by someone on the Strapi team, as I've found the solution and would like this to be reviewed and a fix merged into the repo, but I've been trying to contact the Strapi team for ~3 weeks to raise this issue, to no avail.

Steps to reproduce the behavior

Create a Strapi V3 App.
Create two Collection Types called "Article" and "Care-role".
Form a many-to-many relationship between them.
Add some test data
Run CodeMods to upgrade it to V4.
Run the migration scripts on the DB to upgrade it to V4.
All relationships involving the Care-roles table should be skipped over, due to the reasons stated above.

Expected behavior

The Care-roles relationships should be migrated properly, outputting the following message in the console:

Migrating X items from articles_care_roles__care_roles_articles to articles_care_roles_links

Screenshots

Relevant screenshots included above, please see my messages in the Strapi discord for more screenshots:

Code snippets

Code samples AND pending solution posted above.

Additional context

This is pretty-much solved and I just need this solution reviewed and merged into the main repo by a member of the Strapi team. The only reason I haven't made this a PR is because I'm unsure whether attributeF requires the snake-case fix, so I'd like someone from the Strapi team to review the change to ensure no further issues happen with a result.

I'll be running this eventually on a real client's database, so having this fix officially merged in would be a real confidence-booster in running the script against their real database in a few weeks time.

cpaczek commented 1 year ago

@derrickmehaffy this user in discord asked if someone could give a sanity check on this issue to make sure they are not missing anything. I looked over it and it looks logical but I don't know enough about the v3 db layer.

MO-Lewis commented 1 year ago

Hi Cpaczek, thanks for chasing this up! The main question I have for Derrick (or any of the team) is:

Would the fix need to be:

modelF: snakeCase(value.collection),
attributeF: snakeCase(value.attribute),

or

modelF: snakeCase(value.collection),
attributeF: value.attribute,

Again, thanks for your time today! I hope this fix can go to main to help future users 😄

geeky-biz commented 1 year ago

I'm not from Strapi team but just letting know - for us, just changing the modelF: snakeCase(value.collection), resolved the issue of many-to-many relationships not getting migrated for collections with hyphen.

valentyn-dasburo commented 1 year ago

I have the same issue, but for me, both suggested fixes break the script with the following error:

Migrating 100 items from foo_bars_test_entries__test_entries_foo_bars to test_entries_foo_bars_links
foo_bars_test_entries__test_entries_foo_bars batch #1
WARNING - items of test_entries_foo_bars_links was filtered ["inv_test_entry_id","inv_foo_bar_id"]
WARNING - items of test_entries_foo_bars_links was filtered ["inv_test_entry_id","inv_foo_bar_id"]
WARNING - items of test_entries_foo_bars_links was filtered ["inv_test_entry_id","inv_foo_bar_id"]
(node:20292) UnhandledPromiseRejectionWarning: Error: The query is empty
    at Client_PG._query (v3-sql-v4-sql/node_modules/knex/lib/dialects/postgres/index.js:214:25)
    at executeQuery (v3-sql-v4-sql/node_modules/knex/lib/execution/internal/query-executioner.js:37:17)
    at Client_PG.query (v3-sql-v4-sql/node_modules/knex/lib/client.js:146:12)
    at Runner.query (v3-sql-v4-sql/node_modules/knex/lib/execution/runner.js:130:36)
    at ensureConnectionCallback (v3-sql-v4-sql/node_modules/knex/lib/execution/internal/ensure-connection-callback.js:13:17)
    at Runner.ensureConnection (v3-sql-v4-sql/node_modules/knex/lib/execution/runner.js:307:20)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at async Runner.run (v3-sql-v4-sql/node_modules/knex/lib/execution/runner.js:30:19)
    at async migrate (v3-sql-v4-sql/migrate/helpers/migrate.js:146:7)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:20292) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
(node:20292) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

I guess, it gets broken for a specific table/field name

strapi / migration-scripts