vendure-ecommerce / vendure

The commerce platform with customization in its DNA.
https://www.vendure.io
Other
5.53k stars 979 forks source link

TypeORM foreign_key names #2542

Open HoseinGhanbari opened 9 months ago

HoseinGhanbari commented 9 months ago

Describe the bug

Hi,

When switching from dev to prod or vice versa, using yarn migration:generate auto-migration-file will almost all of the time produces MANY unnecessary sql statements like the following

ALTER TABLE `XYZ` DROP FOREIGN KEY `FK_000000000000000000000000000` ...
ALTER TABLE `XYZ` ADD CONSTRAINT `FK_111111111111111111111111111` ...

Actually these are just implicit FK names which are gets considered as unsynchronized changes that have no effect on the business/logics that are going to be executed. BUT They are really annoying as you are consistently getting the warning of Your database schema does not match your current configuration which is almost a false alarm.

Also ...

Who knows what's the meaning of PK_0669fe20e252eb692bf4d344975 ?

This is the related github issue on typeorm about using explicitly named FK https://github.com/typeorm/typeorm/issues/1355

To Reproduce

  1. MySQL version 8.0 is a must to be used as database.
  2. Generate and run migrations on remote server.
  3. Use a cloned version of your remote database as your local database.
  4. Generate and run migrations on local server.
  5. There will be lots of unnecessary sql statements trying to drop and then recreating many FK

Expected behavior It would be great if we can add support of an explicit naming strategy with no hashed values for (primaryKeyName/uniqueConstraintName/relationConstraintName/foreignKeyName/indexName) to Vendure's internal TypeORM as an option for our users so there will be no false alarm of unsynchronized database schema messages also actual database schema is going to use meaningful names.

Environment:

Additional context nothing required as an additional context

HoseinGhanbari commented 9 months ago

Now that I'm using a custom naming strategy (actually a fixed one based on table, column and field names), the issue still exists with another wired behavior which I think more related to MySQL itself.

Whenever you try generating migration files while the migrations dir is empty, there will be a new file created which is going to delete all FK first then recreate them with the same name but different Referential Actions for ON DELETE AND ON UPDATE.

Here is an example of what were genereted for a single FOREIGN KEY:

migration UP

ALTER TABLE `authentication_method` DROP FOREIGN KEY `FK_authentication_method_userId_user_id`;

ALTER TABLE `authentication_method` ADD CONSTRAINT `FK_authentication_method_userId_user_id` FOREIGN KEY (`userId`) REFERENCES `user`(`id`) ON DELETE NO ACTION ON UPDATE NO ACTION;

migration DOWN

ALTER TABLE `authentication_method` DROP FOREIGN KEY `FK_authentication_method_userId_user_id`;

ALTER TABLE `authentication_method` ADD CONSTRAINT `FK_authentication_method_userId_user_id` FOREIGN KEY (`userId`) REFERENCES `user`(`id`) ON DELETE RESTRICT ON UPDATE RESTRICT;

So it seems TypeORM wants NO ACTION but MySQL prefers RESTRICT :exploding_head:

HoseinGhanbari commented 9 months ago

Actually found some related issues on TypeORM's official repository, which were opened 2018 and still receiving comments in 2023.

Someone found the solution to use mariadb type for a mysql instance but it doesn't seem logical.

https://github.com/typeorm/typeorm/issues/1686 https://github.com/typeorm/typeorm/issues/5960

So as @michaelbromley mentioned before the DefaultNamingStrategy of TypeORM doing its job well by generating deterministic names for "identifiers" but the issue is more about MySQL and TypeORM's beahvior about Referential Actions.

HoseinGhanbari commented 9 months ago

using the DefaultNamingStrategy will produces the following statements for the sample FK mentioned above.

migration UP

ALTER TABLE `authentication_method` DROP FOREIGN KEY `FK_00cbe87bc0d4e36758d61bd31d6`;
ALTER TABLE `authentication_method` ADD CONSTRAINT `FK_00cbe87bc0d4e36758d61bd31d6` FOREIGN KEY (`userId`) REFERENCES `user`(`id`) ON DELETE NO ACTION ON UPDATE NO ACTION;

migration DOWN

ALTER TABLE `authentication_method` DROP FOREIGN KEY `FK_00cbe87bc0d4e36758d61bd31d6`;
ALTER TABLE `authentication_method` ADD CONSTRAINT `FK_00cbe87bc0d4e36758d61bd31d6` FOREIGN KEY (`userId`) REFERENCES `user`(`id`) ON DELETE RESTRICT ON UPDATE RESTRICT;