fix(postgres): fix having non-PK serial column next to a non-serial PK

Closes #3350

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);
       }
     };
 

packages/knex/src/schema/SchemaGenerator.ts

@@ -336,7 +336,7 @@ export class SchemaGenerator extends AbstractSchemaGenerator<AbstractSqlDriver>
 
       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<AbstractSqlDriver>
         }
 
         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<AbstractSqlDriver>
     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<AbstractSqlDriver>
     return builder;
   }
 
-  private configureColumn<T>(column: Column, col: Knex.ColumnBuilder, changedProperties?: Set<string>) {
-    return this.helper.configureColumn(column, col, this.knex, changedProperties);
-  }
-
   private createForeignKeys(table: Knex.CreateTableBuilder, tableDef: DatabaseTable, schema?: string): void {
     if (!this.helper.supportsSchemaConstraints()) {
       return;

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\\"));
+
+"
+`;

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