From 6c589b0c534b8fa5994363f5ed718b9be8840e8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ad=C3=A1mek?= Date: Mon, 1 Aug 2022 01:12:31 +0200 Subject: [PATCH] fix(postgres): fix having non-PK serial column next to a non-serial PK Closes #3350 --- packages/knex/src/schema/SchemaComparator.ts | 6 ++-- packages/knex/src/schema/SchemaGenerator.ts | 12 +++---- .../serial-property.postgres.test.ts.snap | 14 ++++++++ .../serial-property.postgres.test.ts | 35 +++++++++++++++++-- 4 files changed, 54 insertions(+), 13 deletions(-) diff --git a/packages/knex/src/schema/SchemaComparator.ts b/packages/knex/src/schema/SchemaComparator.ts index 9a9973bddce8..eaed060b8faf 100644 --- a/packages/knex/src/schema/SchemaComparator.ts +++ b/packages/knex/src/schema/SchemaComparator.ts @@ -1,6 +1,6 @@ import { inspect } from 'util'; import type { Dictionary, EntityProperty } from '@mikro-orm/core'; -import { BooleanType, DateTimeType } from '@mikro-orm/core'; +import { BooleanType, DateTimeType, Utils } from '@mikro-orm/core'; import type { Check, Column, ForeignKey, Index, SchemaDifference, TableDifference } from '../typings'; import type { DatabaseSchema } from './DatabaseSchema'; import type { DatabaseTable } from './DatabaseTable'; @@ -402,7 +402,9 @@ export class SchemaComparator { const columnType2 = column2.mappedType.getColumnType(prop2, this.platform).toLowerCase(); const log = (msg: string, params: Dictionary) => { if (tableName) { - this.log(msg, params); + const copy = Utils.copy(params); + Utils.dropUndefinedProperties(copy); + this.log(msg, copy); } }; diff --git a/packages/knex/src/schema/SchemaGenerator.ts b/packages/knex/src/schema/SchemaGenerator.ts index e74293f00bf7..f435cff72788 100644 --- a/packages/knex/src/schema/SchemaGenerator.ts +++ b/packages/knex/src/schema/SchemaGenerator.ts @@ -336,7 +336,7 @@ export class SchemaGenerator extends AbstractSchemaGenerator for (const column of Object.values(diff.addedColumns)) { const col = this.helper.createTableColumn(table, column, diff.fromTable); - this.configureColumn(column, col); + this.helper.configureColumn(column, col, this.knex); const foreignKey = Object.values(diff.addedForeignKeys).find(fk => fk.columnNames.length === 1 && fk.columnNames[0] === column.name); if (foreignKey && this.options.createForeignKeyConstraints) { @@ -362,7 +362,7 @@ export class SchemaGenerator extends AbstractSchemaGenerator } const col = this.helper.createTableColumn(table, column, diff.fromTable, changedProperties).alter(); - this.configureColumn(column, col, changedProperties); + this.helper.configureColumn(column, col, this.knex, changedProperties); } for (const { column } of Object.values(diff.changedColumns).filter(diff => diff.changedProperties.has('autoincrement'))) { @@ -475,11 +475,11 @@ export class SchemaGenerator extends AbstractSchemaGenerator return this.createSchemaBuilder(tableDef.schema).createTable(tableDef.name, table => { tableDef.getColumns().forEach(column => { const col = this.helper.createTableColumn(table, column, tableDef); - this.configureColumn(column, col); + this.helper.configureColumn(column, col, this.knex); }); for (const index of tableDef.getIndexes()) { - const createPrimary = !tableDef.getColumns().some(c => c.autoincrement) || this.helper.hasNonDefaultPrimaryKeyName(tableDef); + const createPrimary = !tableDef.getColumns().some(c => c.autoincrement && c.primary) || this.helper.hasNonDefaultPrimaryKeyName(tableDef); this.createIndex(table, index, tableDef, createPrimary); } @@ -553,10 +553,6 @@ export class SchemaGenerator extends AbstractSchemaGenerator return builder; } - private configureColumn(column: Column, col: Knex.ColumnBuilder, changedProperties?: Set) { - return this.helper.configureColumn(column, col, this.knex, changedProperties); - } - private createForeignKeys(table: Knex.CreateTableBuilder, tableDef: DatabaseTable, schema?: string): void { if (!this.helper.supportsSchemaConstraints()) { return; diff --git a/tests/features/schema-generator/__snapshots__/serial-property.postgres.test.ts.snap b/tests/features/schema-generator/__snapshots__/serial-property.postgres.test.ts.snap index b3834b7dfb4b..381b2858d59c 100644 --- a/tests/features/schema-generator/__snapshots__/serial-property.postgres.test.ts.snap +++ b/tests/features/schema-generator/__snapshots__/serial-property.postgres.test.ts.snap @@ -27,3 +27,17 @@ exports[`schema generator works with non-pk autoincrement columns (serial) 4`] = " `; + +exports[`schema generator works with non-pk autoincrement columns (serial) 5`] = ` +"alter table \\"something\\" add column \\"_id\\" serial; +alter table \\"something\\" alter column \\"id\\" type varchar(255) using (\\"id\\"::varchar(255)); +alter table \\"something\\" alter column \\"id\\" drop default; + +" +`; + +exports[`schema generator works with non-pk autoincrement columns (serial) 6`] = ` +"create table \\"something\\" (\\"id\\" varchar(255) not null, \\"_id\\" serial, \\"foo\\" varchar(255) not null, constraint \\"something_pkey\\" primary key (\\"id\\")); + +" +`; diff --git a/tests/features/schema-generator/serial-property.postgres.test.ts b/tests/features/schema-generator/serial-property.postgres.test.ts index 01d494d034e6..3404ad14fb6f 100644 --- a/tests/features/schema-generator/serial-property.postgres.test.ts +++ b/tests/features/schema-generator/serial-property.postgres.test.ts @@ -65,6 +65,20 @@ export class Something4 { } +@Entity({ tableName: 'something' }) +export class Something5 { + + @PrimaryKey({ primary: true }) + id!: string; + + @Property({ autoincrement: true }) + _id!: number; + + @Property() + foo!: string; + +} + test('schema generator works with non-pk autoincrement columns (serial)', async () => { const orm = await MikroORM.init({ entities: [Something0], @@ -111,15 +125,30 @@ test('schema generator works with non-pk autoincrement columns (serial)', async await expect(generator.getUpdateSchemaSQL()).resolves.toBe(''); + orm.getMetadata().reset('Something4'); + await orm.discoverEntity(Something5); + const diff5 = await generator.getUpdateSchemaSQL(); + expect(diff5).toMatchSnapshot(); + await generator.execute(diff5); + + await expect(generator.getUpdateSchemaSQL()).resolves.toBe(''); + + const diff52 = await generator.getCreateSchemaSQL(); + await expect(diff52).toMatchSnapshot(); + await orm.close(true); - expect(mock.mock.calls).toHaveLength(6); + expect(mock.mock.calls).toHaveLength(10); expect(mock.mock.calls[0][0]).toMatch(`column public.something._id of type serial added`); - expect(mock.mock.calls[1][0]).toMatch(`'autoincrement' changed for column public.something._id { column1: { name: '_id', type: 'int4', mappedType: IntegerType {}, length: null, precision: 32, scale: 0, nullable: false, default: null, unsigned: true, autoincrement: true, comment: null, primary: false, unique: false, enumItems: [] }, column2: { name: '_id', type: 'int', mappedType: IntegerType {}, unsigned: false, autoincrement: false, primary: false, nullable: false, length: undefined, precision: undefined, scale: undefined, default: undefined, enumItems: undefined, comment: undefined, extra: undefined }}`); + expect(mock.mock.calls[1][0]).toMatch(`'autoincrement' changed for column public.something._id { column1: { name: '_id', type: 'int4', mappedType: IntegerType {}, length: null, precision: 32, scale: 0, nullable: false, default: null, unsigned: true, autoincrement: true, comment: null, primary: false, unique: false, enumItems: [] }, column2: { name: '_id', type: 'int', mappedType: IntegerType {}, unsigned: false, autoincrement: false, primary: false, nullable: false }}`); expect(mock.mock.calls[2][0]).toMatch(`column public.something._id changed { changedProperties: Set(1) { 'autoincrement' } }`); - expect(mock.mock.calls[3][0]).toMatch(`'autoincrement' changed for column public.something._id { column1: { name: '_id', type: 'int4', mappedType: IntegerType {}, length: null, precision: 32, scale: 0, nullable: false, default: null, unsigned: undefined, autoincrement: undefined, comment: null, primary: false, unique: false, enumItems: [] }, column2: { name: '_id', type: 'serial', mappedType: IntegerType {}, unsigned: true, autoincrement: true, primary: false, nullable: false, length: undefined, precision: undefined, scale: undefined, default: undefined, enumItems: undefined, comment: undefined, extra: undefined }}`); + expect(mock.mock.calls[3][0]).toMatch(`'autoincrement' changed for column public.something._id { column1: { name: '_id', type: 'int4', mappedType: IntegerType {}, length: null, precision: 32, scale: 0, nullable: false, default: null, comment: null, primary: false, unique: false, enumItems: [] }, column2: { name: '_id', type: 'serial', mappedType: IntegerType {}, unsigned: true, autoincrement: true, primary: false, nullable: false }}`); expect(mock.mock.calls[4][0]).toMatch(`column public.something._id changed { changedProperties: Set(1) { 'autoincrement' } }`); expect(mock.mock.calls[5][0]).toMatch(`column public.something._id removed`); + expect(mock.mock.calls[6][0]).toMatch(`column public.something._id of type serial added`); + expect(mock.mock.calls[7][0]).toMatch(`'type' changed for column public.something.id { columnType1: 'int', columnType2: 'varchar(255)' }`); + expect(mock.mock.calls[8][0]).toMatch(`'autoincrement' changed for column public.something.id { column1: { name: 'id', type: 'int4', mappedType: IntegerType {}, length: null, precision: 32, scale: 0, nullable: false, default: null, unsigned: true, autoincrement: true, comment: null, primary: true, unique: false, enumItems: [] }, column2: { name: 'id', type: 'varchar(255)', mappedType: StringType {}, unsigned: false, autoincrement: false, primary: false, nullable: false }}`); + expect(mock.mock.calls[9][0]).toMatch(`column public.something.id changed { changedProperties: Set(2) { 'type', 'autoincrement' } }`); }); test('create schema dump with serial property', async () => {