pluginpal / strapi-plugin-config-sync

:recycle: CLI & GUI for continuous migration of config data across environments
https://www.pluginpal.io/plugin/config-sync
MIT License
251 stars 36 forks source link

Non-deterministic insert order if importOnBoostrap runs on an empty database #120

Closed karlkeefer closed 7 months ago

karlkeefer commented 7 months ago

Firstly thanks for building this plugin. It does critical work for multi-env deployments 🙇

Bug report

If config-sync has records of roles from a different env, they can be inserted before users-permissions does its inserts, sometimes resulting in incorrect IDs for roles. This could also cause problems if more than one role was added at a time when migrating that config across environments.

Describe the bug

In the users-permissions initialization inserts, roles are inserted in a specific order. The config-sync imports don't specify an order.

Steps to reproduce the behavior

We ran into this in our integration tests, and it happened really haphazardly.

Initializing strapi with...

Expected behavior

I'd expect roles export to track ids to insert them in the correct order if they are missing. So far it's the only config table that has presented an ordering problem for us.

Additional context

I initially thought this was a users-permissions issue. More context there.

We've worked around this by setting importOnBootsrap only true in deployment environments, and are manually running config-sync imports in our tests after the default db setup happens.

boazpoolman commented 7 months ago

Hi @karlkeefer,

Thanks for all the context you've given in the issue.

I understand that the roles could be inserted in different orders. Though I'm not quite sure what that matters. Is there a specific issue you run into when the auto-increment id's of the roles don't match up?

The plugin itself uses the role type as it's unique identifier, completely neglecting it's id. Maby you could do the same to prevent the issue?

karlkeefer commented 7 months ago

Yeah... we could make our fixture data inserter fetch the roles before assigning them to test users. It's very convenient to just use known-in-advance role IDs though.

It seems plausible to me that other plugins might lean on role IDs? I don't know a concrete case of that.

Ultimately it took me hours to figure out what was happening, so I thought it worth creating this issue, but I get this is a pretty niche fix, also with some functional workarounds.

Still, it strikes me as counter to expectation for the role IDs to be different after sync.

boazpoolman commented 7 months ago

Honestly, anybody using the auto-increment id to identify a role implemented a bad practice imo.

Somebody could’ve (accidentally) removed the role, and re-created it, causing the id to change, meaning their code wouldn’t work anymore.

The only known-in-advance role id you have is it’s type field. Even Strapi themselves use the type field to identify the roles. See https://github.com/search?q=repo%3Astrapi%2Fstrapi%20type%3A%20%27public%27&type=code

Ultimately the only proper way to fix this is to use some kind of uuid to identify the role (or any config entry in the db). Though luckily Strapi v5 will offer that feature, and you can be sure that I’ll be migrating this plugin to move along with that change.

karlkeefer commented 7 months ago

Cheers - thanks for your time and consideration!