fix(postgres): do not ignore custom PK constraint names #2931

jbl428 · 2022-03-19T10:47:24Z

resolve #2930

codecov-commenter · 2022-03-19T10:59:53Z

Codecov Report

Merging #2931 (9dca2ba) into master (16b677c) will decrease coverage by 0.01%.
The diff coverage is 93.75%.

@@             Coverage Diff             @@
##            master    #2931      +/-   ##
===========================================
- Coverage   100.00%   99.98%   -0.02%     
===========================================
  Files          193      193              
  Lines        11783    11800      +17     
  Branches      2721     2723       +2     
===========================================
+ Hits         11783    11798      +15     
- Misses           0        2       +2

Impacted Files	Coverage Δ
packages/core/src/platforms/Platform.ts	`99.03% <0.00%> (-0.97%)`	⬇️
packages/knex/src/schema/SchemaGenerator.ts	`100.00% <100.00%> (ø)`
packages/mariadb/src/MariaDbPlatform.ts	`100.00% <100.00%> (ø)`
packages/mysql/src/MySqlPlatform.ts	`100.00% <100.00%> (ø)`
packages/postgresql/src/PostgreSqlPlatform.ts	`100.00% <100.00%> (ø)`
packages/sqlite/src/SqlitePlatform.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16b677c...9dca2ba. Read the comment docs.

packages/core/src/utils/Configuration.ts

packages/knex/src/schema/SchemaGenerator.ts

B4nan · 2022-03-22T07:29:17Z

packages/core/src/platforms/Platform.ts

+  getDefaultPrimaryName(tableName: string, columns: string[]): string {
+    return this.namingStrategy.indexName(tableName, columns, 'primary');
+  }


this seems like a wrong default to me, how can you then compare the default implementation with the one from naming strategy, if both ways use the naming strategy?

edit: i see it gets overridden, so not a big deal, but it looks confusing. note that method will need to be unit tested to keep full coverage

B4nan

i would say this can be done in much more readable way, now it seems quite confusing to me, please give it another try, or i can take over if you want :]

B4nan · 2022-03-22T07:42:39Z

packages/postgresql/src/PostgreSqlPlatform.ts

+  getDefaultPrimaryName(tableName: string, columns: string[]): string {
+    const indexName = `${tableName}_pkey`;
+    if (indexName.length > 64) {
+      return `${indexName.substr(0, 57 - 'primary'.length)}_${Utils.hash(indexName).substr(0, 5)}_primary`;
    }


i dont think we need to care about this for default primary index name, it will be problematic only if the table name will be too long, which will on its own cause other problems, and i am also quite sure the default way of handling this would produce something else than our custom md5 hash in the pk name

B4nan · 2022-03-22T07:44:48Z

packages/mariadb/src/MariaDbPlatform.ts


    /* istanbul ignore next */
    if (indexName.length > 64) {
-      indexName = `${indexName.substr(0, 57 - type.length)}_${Utils.hash(indexName).substr(0, 5)}_${type}`;
+      return `${indexName.substr(0, 57 - type.length)}_${Utils.hash(indexName).substr(0, 5)}_${type}`;


i dont mind the early return, but the way it was written before made all the branches covered, your changes will result in lower code coverage and i do care about that. if you want to use early return, add more tests that will cover the following branch

B4nan · 2022-03-22T07:51:39Z

packages/knex/src/schema/SchemaGenerator.ts

@@ -488,14 +488,22 @@ export class SchemaGenerator extends AbstractSchemaGenerator<AbstractSqlDriver>
    });
  }

-  private createIndex(table: Knex.CreateTableBuilder, index: Index, createPrimary = false) {
+  private createIndex(table: Knex.CreateTableBuilder, index: Index, tableDef: DatabaseTable, createPrimary = false, useExplicitPKName = false) {


i dont understand why you need useExplicitPKName parameter (it should always behave like that based on the NS/default pk name from platform), and generally i would expect this method not to change that much

i would very much like to keep the previous code structure as this is much less readable and i can imagine it will have some branches where it wont create the PK. there should be one call to table.primary() as it was, and it should be always fired if index.primary is true.

If PK columns is autoincrement and table.primary() is fired. knex generates sql which have explicit PK name.
(I can not 100% sure it's knex doing)

create table "new_table" ("id" serial, "enum_test" varchar(255) not null); alter table "new_table" add constraint "new_table_pkey" primary key ("id");

If table.primary() is not fired. It generates primary key keyword anyway.

create table "new_table" ("id" serial primary key, "enum_test" varchar(255) not null);

This is the why there are some branches table.primary() is not fired when index.primary is true.

I have one question.
Could we change existing behavior to have explicit PK name when NS not override and PK column is autoincrement?

If PK columns is autoincrement and table.primary() is fired

It should not be fired, that is the exact reason why we have the createPrimary parameter

Could we change existing behavior to have explicit PK name when PK column is autoincrement?

Nope, that would affect most of the users in a bad way I'd say. I want the exact opposite, have the names there only when needed, not always.

If table.primary() is not fired. It generates primary key keyword anyway.

That's because the column.primary() is called for those. As said above, we have the createPrimary parameter to distinguish.

edit: or maybe not, I need to check the code more deeper, it was working like that before but now I can't find any reference. but knex on its own will definitely not mark things as PK if we dont instruct it that way, so it needs to happen somewhere

edit2: it does work this way, i just confused myself because we dont call col.primary(), instead we call table.increments() for those, which implies col.primary()

B4nan · 2022-03-22T08:01:02Z

packages/knex/src/schema/SchemaGenerator.ts

@@ -466,7 +466,7 @@ export class SchemaGenerator extends AbstractSchemaGenerator<AbstractSqlDriver>
      });

      for (const index of tableDef.getIndexes()) {
-        this.createIndex(table, index, !tableDef.getColumns().some(c => c.autoincrement));
+        this.createIndex(table, index, tableDef, true);


i dont understand why the createPrimary is no longer driven by this, i believe it was still valid, and now you just hide it inside the method implementation, making it much more complex

To check if table.primary() should be called.

Have override for NS <- should be called

Not have override and table has some autoincrement <- should not be called

Not have override and table do not have autoincrement <- should be called

To handle the above branches in createIndex. I have changed the createPrimary code.

B4nan · 2022-03-22T08:24:37Z

Based on the code changes, it feels like you wanted to make all the existing tests pass. That is wrong, the snapshots are not 100% correct, this change should be reflected in them too. One example is that Label2 entity I mentioned above, the PK name it uses explicitly in current snapshots is the default and should not be explicitly stated. Maybe that is the reason why the implementation looks overcomplicated.

jbl428 · 2022-03-22T14:42:00Z

Maybe that is the reason why the implementation looks overcomplicated.

Yes, you are right. I have thought that this PR should not impact existing behavior and only add functionality to have custom PK name in postresql.

Now that we are allowed to change existing behavior. I think I can make more readable code.

B4nan · 2022-03-22T15:57:02Z

maybe i should finish this myself, i just checked the code more in depth and it is a bit more complicated, as the current code was doing some workaround to make things work the way they work, and those should be refactored as well, otherwise we cant make this cleaner.

will try to spend some time with it and report back to you

B4nan · 2022-03-23T09:37:35Z

Thanks a lot!

jbl428 force-pushed the postgre-custom-pk branch 2 times, most recently from 9e99430 to 8ef4752 Compare March 19, 2022 10:53

B4nan reviewed Mar 19, 2022

View reviewed changes

packages/core/src/utils/Configuration.ts Outdated Show resolved Hide resolved

packages/knex/src/schema/SchemaGenerator.ts Outdated Show resolved Hide resolved

jbl428 added 2 commits March 22, 2022 07:48

chore: fix wrong driver import

ad8b719

fix(core): use custom PK name when it is different from the default

9dca2ba

jbl428 force-pushed the postgre-custom-pk branch from 210db01 to 9dca2ba Compare March 21, 2022 22:48

B4nan requested changes Mar 22, 2022

View reviewed changes

refactor: try to simplify the code a bit

6a5c604

B4nan merged commit 24bf10e into mikro-orm:master Mar 23, 2022

jbl428 deleted the postgre-custom-pk branch March 23, 2022 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(postgres): do not ignore custom PK constraint names #2931

fix(postgres): do not ignore custom PK constraint names #2931

jbl428 commented Mar 19, 2022

codecov-commenter commented Mar 19, 2022 •

edited

B4nan Mar 22, 2022 •

edited

B4nan left a comment

B4nan Mar 22, 2022

B4nan Mar 22, 2022

B4nan Mar 22, 2022

jbl428 Mar 22, 2022

jbl428 Mar 22, 2022 •

edited

B4nan Mar 22, 2022 •

edited

B4nan Mar 22, 2022

jbl428 Mar 22, 2022

B4nan commented Mar 22, 2022

jbl428 commented Mar 22, 2022 •

edited

B4nan commented Mar 22, 2022

B4nan commented Mar 23, 2022

fix(postgres): do not ignore custom PK constraint names #2931

fix(postgres): do not ignore custom PK constraint names #2931

Conversation

jbl428 commented Mar 19, 2022

codecov-commenter commented Mar 19, 2022 • edited

Codecov Report

B4nan Mar 22, 2022 • edited

Choose a reason for hiding this comment

B4nan left a comment

Choose a reason for hiding this comment

B4nan Mar 22, 2022

Choose a reason for hiding this comment

B4nan Mar 22, 2022

Choose a reason for hiding this comment

B4nan Mar 22, 2022

Choose a reason for hiding this comment

jbl428 Mar 22, 2022

Choose a reason for hiding this comment

jbl428 Mar 22, 2022 • edited

Choose a reason for hiding this comment

B4nan Mar 22, 2022 • edited

Choose a reason for hiding this comment

B4nan Mar 22, 2022

Choose a reason for hiding this comment

jbl428 Mar 22, 2022

Choose a reason for hiding this comment

B4nan commented Mar 22, 2022

jbl428 commented Mar 22, 2022 • edited

B4nan commented Mar 22, 2022

B4nan commented Mar 23, 2022

codecov-commenter commented Mar 19, 2022 •

edited

B4nan Mar 22, 2022 •

edited

jbl428 Mar 22, 2022 •

edited

B4nan Mar 22, 2022 •

edited

jbl428 commented Mar 22, 2022 •

edited