Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from Mar 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad8b719
chore: fix wrong driver import
jbl428 Mar 21, 2022
9dca2ba
fix(core): use custom PK name when it is different from the default
jbl428 Mar 21, 2022
6a5c604
refactor: try to simplify the code a bit
B4nan Mar 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/better-sqlite/src/BetterSqlitePlatform.ts
View file
Open in desktop
Expand Up @@ -118,12 +118,16 @@ export class BetterSqlitePlatform extends AbstractSqlPlatform {

getIndexName(tableName: string, columns: string[], type: 'index' | 'unique' | 'foreign' | 'primary' | 'sequence'): string {
if (type === 'primary') {
return 'primary';
return this.getDefaultPrimaryName(tableName, columns);
}

return super.getIndexName(tableName, columns, type);
}

getDefaultPrimaryName(tableName: string, columns: string[]): string {
return 'primary';
}

supportsDownMigrations(): boolean {
return false;
}
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/platforms/Platform.ts
View file
Open in desktop
Expand Up @@ -363,6 +363,10 @@ export abstract class Platform {
return this.namingStrategy.indexName(tableName, columns, type);
}

getDefaultPrimaryName(tableName: string, columns: string[]): string {
return this.namingStrategy.indexName(tableName, columns, 'primary');
}
Comment on lines +367 to +369
Copy link
Member

@B4nan B4nan Mar 22, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


supportsCustomPrimaryKeyNames(): boolean {
return false;
}
Expand Down
26 changes: 17 additions & 9 deletions packages/knex/src/schema/SchemaGenerator.ts
View file
Open in desktop
@@ -1,4 +1,4 @@
import type { Knex } from 'knex';
import type { Knex } from 'knex';
import type { Dictionary, EntityMetadata } from '@mikro-orm/core';
import { AbstractSchemaGenerator } from '@mikro-orm/core';
import type { Check, Column, ForeignKey, Index, SchemaDifference, TableDifference } from '../typings';
Expand Down Expand Up @@ -239,7 +239,7 @@ export class SchemaGenerator extends AbstractSchemaGenerator<AbstractSqlDriver>
schema = schemaName ?? schema ?? this.config.get('schema');

/* istanbul ignore next */
if (schema && schemaName ==='*') {
if (schema && schemaName === '*') {
return `${schema}.${referencedTableName.replace(/^\*\./, '')}`;
}

Expand Down Expand Up @@ -377,17 +377,17 @@ export class SchemaGenerator extends AbstractSchemaGenerator<AbstractSqlDriver>
}

for (const index of Object.values(diff.addedIndexes)) {
this.createIndex(table, index);
this.createIndex(table, index, diff.fromTable);
}

for (const index of Object.values(diff.changedIndexes)) {
this.createIndex(table, index, true);
this.createIndex(table, index, diff.fromTable, true, true);
}

for (const [oldIndexName, index] of Object.entries(diff.renamedIndexes)) {
if (index.unique) {
this.dropIndex(table, index, oldIndexName);
this.createIndex(table, index);
this.createIndex(table, index, diff.fromTable);
} else {
this.helper.pushTableQuery(table, this.helper.getRenameIndexSQL(diff.name, index, oldIndexName));
}
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

for (const check of tableDef.getChecks()) {
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jbl428 jbl428 Mar 22, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@B4nan B4nan Mar 22, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

if (index.primary && !createPrimary) {
return;
}

if (index.primary) {
const keyName = this.platform.supportsCustomPrimaryKeyNames() ? index.keyName : undefined;
table.primary(index.columnNames, keyName);
const defaultName = this.platform.getDefaultPrimaryName(tableDef.name, index.columnNames);

if (this.platform.supportsCustomPrimaryKeyNames() && (useExplicitPKName || index.keyName !== defaultName)) {
table.primary(index.columnNames, { constraintName: index.keyName });
return;
}

if (index.composite || !tableDef.getColumns().some(c => c.autoincrement)) {
table.primary(index.columnNames);
}
} else if (index.unique) {
table.unique(index.columnNames, { indexName: index.keyName });
} else if (index.expression) {
Expand Down
10 changes: 7 additions & 3 deletions packages/mariadb/src/MariaDbPlatform.ts
View file
Open in desktop
Expand Up @@ -52,17 +52,21 @@ export class MariaDbPlatform extends AbstractSqlPlatform {
*/
getIndexName(tableName: string, columns: string[], type: 'index' | 'unique' | 'foreign' | 'primary' | 'sequence'): string {
if (type === 'primary') {
return 'PRIMARY'; // https://dev.mysql.com/doc/refman/8.0/en/create-table.html#create-table-indexes-keys
return this.getDefaultPrimaryName(tableName, columns);
}

let indexName = super.getIndexName(tableName, columns, type);
const indexName = super.getIndexName(tableName, columns, type);

/* 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}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return indexName;
}

getDefaultPrimaryName(tableName: string, columns: string[]): string {
return 'PRIMARY'; // https://dev.mysql.com/doc/refman/8.0/en/create-table.html#create-table-indexes-keys
}

}
10 changes: 7 additions & 3 deletions packages/mysql/src/MySqlPlatform.ts
View file
Open in desktop
Expand Up @@ -51,15 +51,19 @@ export class MySqlPlatform extends AbstractSqlPlatform {
*/
getIndexName(tableName: string, columns: string[], type: 'index' | 'unique' | 'foreign' | 'primary' | 'sequence'): string {
if (type === 'primary') {
return 'PRIMARY'; // https://dev.mysql.com/doc/refman/8.0/en/create-table.html#create-table-indexes-keys
return this.getDefaultPrimaryName(tableName, columns);
}

let indexName = super.getIndexName(tableName, columns, type);
const indexName = super.getIndexName(tableName, columns, type);
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}`;
}

return indexName;
}

getDefaultPrimaryName(tableName: string, columns: string[]): string {
return 'PRIMARY'; // https://dev.mysql.com/doc/refman/8.0/en/create-table.html#create-table-indexes-keys
}

}
13 changes: 11 additions & 2 deletions packages/postgresql/src/PostgreSqlPlatform.ts
View file
Open in desktop
Expand Up @@ -213,9 +213,18 @@ export class PostgreSqlPlatform extends AbstractSqlPlatform {
* cannot go past 64 character length for identifiers in MySQL
*/
getIndexName(tableName: string, columns: string[], type: 'index' | 'unique' | 'foreign' | 'primary' | 'sequence'): string {
let indexName = super.getIndexName(tableName, columns, type);
const indexName = super.getIndexName(tableName, columns, type);
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}`;
}

return indexName;
}

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`;
}
Comment on lines +224 to 228
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return indexName;
Expand Down
6 changes: 5 additions & 1 deletion packages/sqlite/src/SqlitePlatform.ts
View file
Open in desktop
Expand Up @@ -118,12 +118,16 @@ export class SqlitePlatform extends AbstractSqlPlatform {

getIndexName(tableName: string, columns: string[], type: 'index' | 'unique' | 'foreign' | 'primary' | 'sequence'): string {
if (type === 'primary') {
return 'primary';
return this.getDefaultPrimaryName(tableName, columns);
}

return super.getIndexName(tableName, columns, type);
}

getDefaultPrimaryName(tableName: string, columns: string[]): string {
return 'primary';
}

supportsDownMigrations(): boolean {
return false;
}
Expand Down
107 changes: 107 additions & 0 deletions tests/issues/GH2930.test.ts
View file
Open in desktop
@@ -0,0 +1,107 @@
import {
Entity,
PrimaryKey,
Property,
MikroORM,
UnderscoreNamingStrategy,
} from '@mikro-orm/core';
import type { PostgreSqlDriver } from '@mikro-orm/postgresql';
import type { MySqlDriver } from '@mikro-orm/mysql';

@Entity()
class A {

@PrimaryKey()
id!: number;

@Property()
prop?: string;

}

describe('GH issue 2930', () => {

describe('postgresql (PK override)', () => {
let orm: MikroORM<PostgreSqlDriver>;

beforeAll(async () => {
orm = await MikroORM.init({
entities: [A],
dbName: 'mikro_orm_test_gh2930',
type: 'postgresql',
namingStrategy: class extends UnderscoreNamingStrategy {

indexName(tableName: string, columns: string[], type: 'primary' | 'foreign' | 'unique' | 'index' | 'sequence' | 'check'): string {
if (type === 'primary') {
return `pk_${tableName}_${columns.join('_')}`;
}
return super.indexName(tableName, columns, type);
}

},
});
});

afterAll(() => orm.close(true));

test(`should not ignore custom pk name`, async () => {
const sql = await orm.getSchemaGenerator().getCreateSchemaSQL();
expect(sql).toMatchSnapshot();
});
});

describe('postgresql (PK not override)', () => {
let orm: MikroORM<PostgreSqlDriver>;

beforeAll(async () => {
orm = await MikroORM.init({
entities: [A],
dbName: 'mikro_orm_test_gh2930',
type: 'postgresql',
namingStrategy: class extends UnderscoreNamingStrategy {

indexName(tableName: string, columns: string[], type: 'primary' | 'foreign' | 'unique' | 'index' | 'sequence' | 'check'): string {
return super.indexName(tableName, columns, type);
}

},
});
});

afterAll(() => orm.close(true));

test(`should not generate a sql naming PK`, async () => {
const sql = await orm.getSchemaGenerator().getCreateSchemaSQL();
expect(sql).toMatchSnapshot();
});
});

describe('mysql', () => {
let orm: MikroORM<MySqlDriver>;

beforeAll(async () => {
orm = await MikroORM.init({
entities: [A],
dbName: 'mikro_orm_test_gh2930',
type: 'mysql',
namingStrategy: class extends UnderscoreNamingStrategy {

indexName(tableName: string, columns: string[], type: 'primary' | 'foreign' | 'unique' | 'index' | 'sequence' | 'check'): string {
if (type === 'primary') {
return `pk_${tableName}_${columns.join('_')}`;
}
return super.indexName(tableName, columns, type);
}

},
});
});

afterAll(() => orm.close(true));

test(`should ignore custom pk name`, async () => {
const sql = await orm.getSchemaGenerator().getCreateSchemaSQL();
expect(sql).toMatchSnapshot();
});
});
});
4 changes: 2 additions & 2 deletions tests/issues/GH472.test.ts
View file
Open in desktop
@@ -1,5 +1,5 @@
import { Entity, PrimaryKey, Property, MikroORM, EntityCaseNamingStrategy } from '@mikro-orm/core';
import type { SqliteDriver } from '@mikro-orm/sqlite';
import type { PostgreSqlDriver } from '@mikro-orm/postgresql';

@Entity()
class A {
Expand All @@ -14,7 +14,7 @@ class A {

describe('GH issue 472', () => {

let orm: MikroORM<SqliteDriver>;
let orm: MikroORM<PostgreSqlDriver>;

beforeAll(async () => {
orm = await MikroORM.init({
Expand Down
32 changes: 32 additions & 0 deletions tests/issues/__snapshots__/GH2930.test.ts.snap
View file
Open in desktop
@@ -0,0 +1,32 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`GH issue 2930 mysql should ignore custom pk name 1`] = `
"set names utf8mb4;
set foreign_key_checks = 0;

create table \`a\` (\`id\` int unsigned not null auto_increment primary key, \`prop\` varchar(255) not null) default character set utf8mb4 engine = InnoDB;

set foreign_key_checks = 1;
"
`;

exports[`GH issue 2930 postgresql (PK not override) should not generate a sql naming PK 1`] = `
"set names 'utf8';
set session_replication_role = 'replica';

create table \\"a\\" (\\"id\\" serial primary key, \\"prop\\" varchar(255) not null);

set session_replication_role = 'origin';
"
`;

exports[`GH issue 2930 postgresql (PK override) should not ignore custom pk name 1`] = `
"set names 'utf8';
set session_replication_role = 'replica';

create table \\"a\\" (\\"id\\" serial, \\"prop\\" varchar(255) not null);
alter table \\"a\\" add constraint \\"pk_a_id\\" primary key (\\"id\\");

set session_replication_role = 'origin';
"
`;