From d49d13c9db9d4374558217ee7af12a605313db5c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Ad=C3=A1mek?= <banan23@gmail.com>
Date: Thu, 28 Dec 2023 23:03:45 +0100
Subject: [PATCH] fix(postgres): respect column length in down migrations

Closes #5048
---
 .../knex/src/schema/SqlSchemaGenerator.ts     |   1 +
 .../postgresql/src/PostgreSqlSchemaHelper.ts  |  14 +-
 .../changing-pk-type.postgres.test.ts.snap    |   2 +-
 .../length-diffing.mysql.test.ts.snap         |  27 +++-
 .../length-diffing.postgres.test.ts.snap      |  42 +++++
 .../length-diffing.mysql.test.ts              |  32 +++-
 .../length-diffing.postgres.test.ts           | 148 ++++++++++++++++++
 7 files changed, 251 insertions(+), 15 deletions(-)
 create mode 100644 tests/features/schema-generator/__snapshots__/length-diffing.postgres.test.ts.snap
 create mode 100644 tests/features/schema-generator/length-diffing.postgres.test.ts

diff --git a/packages/knex/src/schema/SqlSchemaGenerator.ts b/packages/knex/src/schema/SqlSchemaGenerator.ts
index 9a97ce06a2f5..921390b3a2ab 100644
--- a/packages/knex/src/schema/SqlSchemaGenerator.ts
+++ b/packages/knex/src/schema/SqlSchemaGenerator.ts
@@ -177,6 +177,7 @@ export class SqlSchemaGenerator extends AbstractSchemaGenerator<AbstractSqlDrive
     if (!options.fromSchema) {
       await this.ensureDatabase();
     }
+
     const { fromSchema, toSchema } = await this.prepareSchemaForComparison(options);
     const comparator = new SchemaComparator(this.platform);
     const diffUp = comparator.compare(fromSchema, toSchema);
diff --git a/packages/postgresql/src/PostgreSqlSchemaHelper.ts b/packages/postgresql/src/PostgreSqlSchemaHelper.ts
index 9aeda2861bf5..5972bb9d29e1 100644
--- a/packages/postgresql/src/PostgreSqlSchemaHelper.ts
+++ b/packages/postgresql/src/PostgreSqlSchemaHelper.ts
@@ -134,9 +134,21 @@ export class PostgreSqlSchemaHelper extends SchemaHelper {
       const increments = (col.column_default?.includes('nextval') || col.is_identity === 'YES') && connection.getPlatform().isNumericColumn(mappedType);
       const key = this.getTableKey(col);
       ret[key] ??= [];
+      let type = col.data_type.toLowerCase() === 'array'
+        ? col.udt_name.replace(/^_(.*)$/, '$1[]')
+        : col.udt_name;
+
+      if (col.length != null && !type.endsWith(`(${col.length})`)) {
+        type += `(${col.length})`;
+      }
+
+      if (type === 'numeric' && col.numeric_precision != null && col.numeric_scale != null) {
+        type += `(${col.numeric_precision},${col.numeric_scale})`;
+      }
+
       const column: Column = {
         name: col.column_name,
-        type: col.data_type.toLowerCase() === 'array' ? col.udt_name.replace(/^_(.*)$/, '$1[]') : col.udt_name,
+        type,
         mappedType,
         length: col.length,
         precision: col.numeric_precision,
diff --git a/tests/features/schema-generator/__snapshots__/changing-pk-type.postgres.test.ts.snap b/tests/features/schema-generator/__snapshots__/changing-pk-type.postgres.test.ts.snap
index 8690a2347ca5..40a668aadac0 100644
--- a/tests/features/schema-generator/__snapshots__/changing-pk-type.postgres.test.ts.snap
+++ b/tests/features/schema-generator/__snapshots__/changing-pk-type.postgres.test.ts.snap
@@ -13,7 +13,7 @@ exports[`changing PK column type [postgres] (GH 1480) changing PK type: 0. creat
 
 exports[`changing PK column type [postgres] (GH 1480) changing PK type: 1. change PK type from text to int 1`] = `
 {
-  "down": "alter table "user" alter column "id" type varchar using ("id"::varchar);
+  "down": "alter table "user" alter column "id" type varchar(255) using ("id"::varchar(255));
 
 ",
   "up": "alter table "user" alter column "id" type int using ("id"::int);
diff --git a/tests/features/schema-generator/__snapshots__/length-diffing.mysql.test.ts.snap b/tests/features/schema-generator/__snapshots__/length-diffing.mysql.test.ts.snap
index 10422de94325..3d76387c6194 100644
--- a/tests/features/schema-generator/__snapshots__/length-diffing.mysql.test.ts.snap
+++ b/tests/features/schema-generator/__snapshots__/length-diffing.mysql.test.ts.snap
@@ -1,19 +1,34 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
 exports[`length diffing in mysql schema generator updates column types when length changes (varchar, decimal, ...) 1`] = `
-"alter table \`book\` modify \`id\` bigint unsigned not null auto_increment, modify \`name\` varchar(100) not null, modify \`length\` int unsigned not null, modify \`price\` numeric(16,0) not null;
+{
+  "down": "alter table \`book\` modify \`id\` int unsigned not null auto_increment, modify \`name\` varchar(255) not null, modify \`length\` int not null, modify \`price\` decimal(10,0) not null, modify \`created_at\` datetime(2) not null;
 
-"
+",
+  "up": "alter table \`book\` modify \`id\` bigint unsigned not null auto_increment, modify \`name\` varchar(100) not null, modify \`length\` int unsigned not null, modify \`price\` numeric(16,0) not null, modify \`created_at\` datetime(3) not null;
+
+",
+}
 `;
 
 exports[`length diffing in mysql schema generator updates column types when length changes (varchar, decimal, ...) 2`] = `
-"alter table \`book\` modify \`name\` varchar(150) not null, modify \`price\` numeric(16,4) not null;
+{
+  "down": "alter table \`book\` modify \`name\` varchar(100) not null, modify \`price\` decimal(16,0) not null;
+
+",
+  "up": "alter table \`book\` modify \`name\` varchar(150) not null, modify \`price\` numeric(16,4) not null;
 
-"
+",
+}
 `;
 
 exports[`length diffing in mysql schema generator updates column types when length changes (varchar, decimal, ...) 3`] = `
-"alter table \`book\` modify \`name\` varchar(100) not null;
+{
+  "down": "alter table \`book\` modify \`name\` varchar(150) not null;
+
+",
+  "up": "alter table \`book\` modify \`name\` varchar(100) not null;
 
-"
+",
+}
 `;
diff --git a/tests/features/schema-generator/__snapshots__/length-diffing.postgres.test.ts.snap b/tests/features/schema-generator/__snapshots__/length-diffing.postgres.test.ts.snap
new file mode 100644
index 000000000000..261484d13956
--- /dev/null
+++ b/tests/features/schema-generator/__snapshots__/length-diffing.postgres.test.ts.snap
@@ -0,0 +1,42 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`length diffing in postgres schema orm.schema updates column types when length changes (varchar, decimal, ...) 1`] = `
+{
+  "down": "alter table "book" alter column "id" type int4 using ("id"::int4);
+alter table "book" alter column "name" type varchar(255) using ("name"::varchar(255));
+alter table "book" alter column "price" type numeric(10,0) using ("price"::numeric(10,0));
+alter table "book" alter column "created_at" type timestamptz(2) using ("created_at"::timestamptz(2));
+
+",
+  "up": "alter table "book" alter column "id" type bigint using ("id"::bigint);
+alter table "book" alter column "name" type varchar(100) using ("name"::varchar(100));
+alter table "book" alter column "price" type numeric(16,0) using ("price"::numeric(16,0));
+alter table "book" alter column "created_at" type timestamptz(3) using ("created_at"::timestamptz(3));
+
+",
+}
+`;
+
+exports[`length diffing in postgres schema orm.schema updates column types when length changes (varchar, decimal, ...) 2`] = `
+{
+  "down": "alter table "book" alter column "name" type varchar(100) using ("name"::varchar(100));
+alter table "book" alter column "price" type numeric(16,0) using ("price"::numeric(16,0));
+
+",
+  "up": "alter table "book" alter column "name" type varchar(150) using ("name"::varchar(150));
+alter table "book" alter column "price" type numeric(16,4) using ("price"::numeric(16,4));
+
+",
+}
+`;
+
+exports[`length diffing in postgres schema orm.schema updates column types when length changes (varchar, decimal, ...) 3`] = `
+{
+  "down": "alter table "book" alter column "name" type varchar(150) using ("name"::varchar(150));
+
+",
+  "up": "alter table "book" alter column "name" type varchar(100) using ("name"::varchar(100));
+
+",
+}
+`;
diff --git a/tests/features/schema-generator/length-diffing.mysql.test.ts b/tests/features/schema-generator/length-diffing.mysql.test.ts
index 7b0233af44dc..ae98afbe76c1 100644
--- a/tests/features/schema-generator/length-diffing.mysql.test.ts
+++ b/tests/features/schema-generator/length-diffing.mysql.test.ts
@@ -16,6 +16,9 @@ export class Book0 {
   @Property({ type: t.decimal })
   price!: string;
 
+  @Property({ length: 2 })
+  createdAt!: Date;
+
 }
 
 @Entity({ tableName: 'book' })
@@ -33,6 +36,9 @@ export class Book1 {
   @Property({ type: t.decimal, precision: 16 })
   price!: string;
 
+  @Property({ length: 3 })
+  createdAt!: Date;
+
 }
 
 @Entity({ tableName: 'book' })
@@ -50,6 +56,9 @@ export class Book2 {
   @Property({ type: t.decimal, precision: 16, scale: 4 })
   price!: number;
 
+  @Property({ length: 3 })
+  createdAt!: Date;
+
 }
 
 @Entity({ tableName: 'book' })
@@ -67,6 +76,9 @@ export class Book3 {
   @Property({ columnType: 'decimal(16,4)' })
   price!: number;
 
+  @Property({ length: 3 })
+  createdAt!: Date;
+
 }
 
 @Entity({ tableName: 'book' })
@@ -84,6 +96,9 @@ export class Book4 {
   @Property({ columnType: 'decimal(16,4)' })
   price!: number;
 
+  @Property({ length: 3 })
+  createdAt!: Date;
+
 }
 
 describe('length diffing in mysql', () => {
@@ -106,26 +121,29 @@ describe('length diffing in mysql', () => {
   test('schema generator updates column types when length changes (varchar, decimal, ...)', async () => {
     orm.getMetadata().reset('Book0');
     await orm.discoverEntity(Book1);
-    const diff1 = await orm.schema.getUpdateSchemaSQL({ wrap: false });
+    const diff1 = await orm.schema.getUpdateSchemaMigrationSQL({ wrap: false });
     expect(diff1).toMatchSnapshot();
-    await orm.schema.execute(diff1);
+    await orm.schema.execute(diff1.up);
 
     orm.getMetadata().reset('Book1');
     await orm.discoverEntity(Book2);
-    const diff2 = await orm.schema.getUpdateSchemaSQL({ wrap: false });
+    const diff2 = await orm.schema.getUpdateSchemaMigrationSQL({ wrap: false });
     expect(diff2).toMatchSnapshot();
-    await orm.schema.execute(diff2);
+    await orm.schema.execute(diff2.up);
 
     orm.getMetadata().reset('Book2');
     await orm.discoverEntity(Book3);
-    const diff3 = await orm.schema.getUpdateSchemaSQL({ wrap: false });
+    const diff3 = await orm.schema.getUpdateSchemaMigrationSQL({ wrap: false });
     expect(diff3).toMatchSnapshot();
-    await orm.schema.execute(diff3);
+    await orm.schema.execute(diff3.up);
 
     orm.getMetadata().reset('Book3');
     await orm.discoverEntity(Book4);
 
-    await expect(orm.schema.getUpdateSchemaSQL({ wrap: false })).resolves.toBe('');
+    await expect(orm.schema.getUpdateSchemaMigrationSQL({ wrap: false })).resolves.toEqual({
+      down: '',
+      up: '',
+    });
   });
 
 });
diff --git a/tests/features/schema-generator/length-diffing.postgres.test.ts b/tests/features/schema-generator/length-diffing.postgres.test.ts
new file mode 100644
index 000000000000..f0bac7e8c3bb
--- /dev/null
+++ b/tests/features/schema-generator/length-diffing.postgres.test.ts
@@ -0,0 +1,148 @@
+import { Entity, PrimaryKey, Property, t } from '@mikro-orm/core';
+import { MikroORM } from '@mikro-orm/postgresql';
+
+@Entity({ tableName: 'book' })
+export class Book0 {
+
+  @PrimaryKey()
+  id!: number;
+
+  @Property()
+  name!: string;
+
+  @Property()
+  length!: number;
+
+  @Property({ type: t.decimal })
+  price!: string;
+
+  @Property({ length: 2 })
+  createdAt!: Date;
+
+}
+
+@Entity({ tableName: 'book' })
+export class Book1 {
+
+  @PrimaryKey({ type: t.bigint })
+  id!: string;
+
+  @Property({ length: 100 })
+  name!: string;
+
+  @Property({ unsigned: true })
+  length!: number;
+
+  @Property({ type: t.decimal, precision: 16 })
+  price!: string;
+
+  @Property({ length: 3 })
+  createdAt!: Date;
+
+}
+
+@Entity({ tableName: 'book' })
+export class Book2 {
+
+  @PrimaryKey({ type: t.bigint })
+  id!: string;
+
+  @Property({ length: 150 })
+  name!: string;
+
+  @Property({ unsigned: true })
+  length!: number;
+
+  @Property({ type: t.decimal, precision: 16, scale: 4 })
+  price!: number;
+
+  @Property({ length: 3 })
+  createdAt!: Date;
+
+}
+
+@Entity({ tableName: 'book' })
+export class Book3 {
+
+  @PrimaryKey({ type: t.bigint })
+  id!: string;
+
+  @Property({ length: 100 })
+  name!: string;
+
+  @Property({ unsigned: true })
+  length!: number;
+
+  @Property({ columnType: 'decimal(16,4)' })
+  price!: number;
+
+  @Property({ length: 3 })
+  createdAt!: Date;
+
+}
+
+@Entity({ tableName: 'book' })
+export class Book4 {
+
+  @PrimaryKey({ type: t.bigint })
+  id!: string;
+
+  @Property({ columnType: 'varchar(100)' })
+  name!: string;
+
+  @Property({ unsigned: true })
+  length!: number;
+
+  @Property({ columnType: 'decimal(16,4)' })
+  price!: number;
+
+  @Property({ length: 3 })
+  createdAt!: Date;
+
+}
+
+describe('length diffing in postgres', () => {
+
+  let orm: MikroORM;
+
+  beforeAll(async () => {
+    orm = await MikroORM.init({
+      entities: [Book0],
+      dbName: `mikro_orm_test_length_diffing`,
+    });
+    await orm.schema.ensureDatabase();
+    await orm.schema.execute('drop table if exists book');
+    await orm.schema.createSchema();
+  });
+
+  afterAll(() => orm.close(true));
+
+  test('schema orm.schema updates column types when length changes (varchar, decimal, ...)', async () => {
+    orm.getMetadata().reset('Book0');
+    await orm.discoverEntity(Book1);
+    const diff1 = await orm.schema.getUpdateSchemaMigrationSQL({ wrap: false });
+    expect(diff1).toMatchSnapshot();
+    await orm.schema.execute(diff1.up);
+
+    orm.getMetadata().reset('Book1');
+    await orm.discoverEntity(Book2);
+    const diff2 = await orm.schema.getUpdateSchemaMigrationSQL({ wrap: false });
+    expect(diff2).toMatchSnapshot();
+    await orm.schema.execute(diff2.up);
+
+    orm.getMetadata().reset('Book2');
+    await orm.discoverEntity(Book3);
+    const diff3 = await orm.schema.getUpdateSchemaMigrationSQL({ wrap: false });
+    expect(diff3).toMatchSnapshot();
+    await orm.schema.execute(diff3.up);
+
+    orm.getMetadata().reset('Book3');
+    await orm.discoverEntity(Book4);
+
+    await expect(orm.schema.getUpdateSchemaMigrationSQL({ wrap: false })).resolves.toEqual({
+      down: '',
+      up: '',
+    });
+  });
+
+});