When migrating content entries, migration version returned is erroneous if one of the migration entry was created manually

[yvesgurcan]

How to reproduce the bug

• Init migrations on new environments called environment1 and environment2 using content storage as a method to track migrations. You can use a source environment to duplicate from for both of the environments if you want.
• Create a migration and apply it to environment1.
• Manually create a migration content entry in environment2 using the Contentful UI.
• Run npx migrations content --verbose --diff --source-environment-id environment1 --dest-environment-id environment2
• You should see an error like this one: Different migration states detected. environment1 (1682093148544) !== environment2 (2)

Explanation of the issue

While the ID of the migration in environment1 is accurate (1682093148544), the ID of the migration in environment2 isn't (2) and is the outcome of this line of code:

contentful-migrations/lib/backend.js

Line 297 in bfdca95

return (items || []).map((item) => parseInt(item.sys.id, 10));

parseInt(item.sys.id, 10) converts the ID of the migration to a number in order to compare them across environments. However, if the migration entry was created manually, the ID is not a number but a string generated automatically by Contentful, like this one: 2txYqBZB1wAHVjtHmvzbcA. It seems like parseInt in this particular case only keeps the 2 and this is what the CLI interprets as the ID of the manually-created migration when comparing migrations between the environments.

Suggestions on how to solve the issue

1. Add logic to avoid using parseInt on IDs that are not numbers. Consequently, Math.max should not occur when comparing IDs that are not both numbers since it would result in NaN:
   

   contentful-migrations/lib/backend.js

   Line 347 in bfdca95

   return Math.max(...versions);

   
   There is probably more to this since a string ID is not a timestamp and won't really compare with number IDs. The upside of this approach is that the ID of the migration could still surface in the error displayed from the migration content command (for example: Different migration states detected. environment1 (1682093148544) !== environment2 (2txYqBZB1wAHVjtHmvzbcA)), which would hint at the fact that there are IDs that were manually created. The downside is that manually-created migrations might be considered as legitimate as a result of this change, which might not be a good idea.
2. Warn the user of the CLI that a manually-created entry was found and stop the content-migration process.
3. It might be possible to use a blend of both approaches, where the ID of the migration is displayed even if the entry was created manually but a warning is also shown to the user to tell them that they shouldn't do that.

[yvesgurcan]

@bezoerb @tharders Do you have thoughts about this bug?

[bezoerb]

Hey @yvesgurcan,
thanks for looking into this.
It is intended that the content migrations only work if the content model is identical in both environments. Thats why i check if the same migrations ran on both environments. The migrations content type was intended more as a kind of private list and ideally no entries should be added within the contentful backend, as these always refer to an associated migration file.

But I realize that the current implementation is not super robust ;)

Maybe we could do something like this:

1. The version field should be of type number (this is required to stay compatible with the managed by tag strategy)
2. One could add a help text that says that the entries should not be created manually
3. After fetching the migration entries, we could filter out all without a corresponding migration file (and show a warning/note)