From b6fef268c1e4bd1a5ef0ceb505a29740b13e88bb Mon Sep 17 00:00:00 2001 From: Michael Bromley Date: Fri, 28 Aug 2020 15:28:50 +0200 Subject: [PATCH 1/5] fix: Unnecessary migrations for unsigned numeric types In MariaDB, unsigned numeric types (``int`) without an explicit `width` property set would generate migrations on every run. This is due to an error in setting the default width for unsigned types. Fixes #2943 --- src/driver/mysql/MysqlQueryRunner.ts | 2 +- src/query-runner/BaseQueryRunner.ts | 14 ++++++++++--- test/github-issues/2943/entity/Test.ts | 22 +++++++++++++++++++ test/github-issues/2943/issue-2943.ts | 29 ++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 4 deletions(-) create mode 100644 test/github-issues/2943/entity/Test.ts create mode 100644 test/github-issues/2943/issue-2943.ts diff --git a/src/driver/mysql/MysqlQueryRunner.ts b/src/driver/mysql/MysqlQueryRunner.ts index 0a2b439f2d..970412e037 100644 --- a/src/driver/mysql/MysqlQueryRunner.ts +++ b/src/driver/mysql/MysqlQueryRunner.ts @@ -1296,6 +1296,7 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner { tableColumn.name = dbColumn["COLUMN_NAME"]; tableColumn.type = dbColumn["DATA_TYPE"].toLowerCase(); + tableColumn.unsigned = tableColumn.zerofill ? true : dbColumn["COLUMN_TYPE"].indexOf("unsigned") !== -1; if (this.driver.withWidthColumnTypes.indexOf(tableColumn.type as ColumnType) !== -1) { const width = dbColumn["COLUMN_TYPE"].substring(dbColumn["COLUMN_TYPE"].indexOf("(") + 1, dbColumn["COLUMN_TYPE"].indexOf(")")); tableColumn.width = width && !this.isDefaultColumnWidth(table, tableColumn, parseInt(width)) ? parseInt(width) : undefined; @@ -1333,7 +1334,6 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner { return this.driver.buildTableName(dbPrimaryKey["TABLE_NAME"], undefined, dbPrimaryKey["TABLE_SCHEMA"]) === tableFullName && dbPrimaryKey["COLUMN_NAME"] === tableColumn.name; }); tableColumn.zerofill = dbColumn["COLUMN_TYPE"].indexOf("zerofill") !== -1; - tableColumn.unsigned = tableColumn.zerofill ? true : dbColumn["COLUMN_TYPE"].indexOf("unsigned") !== -1; tableColumn.isGenerated = dbColumn["EXTRA"].indexOf("auto_increment") !== -1; if (tableColumn.isGenerated) tableColumn.generationStrategy = "increment"; diff --git a/src/query-runner/BaseQueryRunner.ts b/src/query-runner/BaseQueryRunner.ts index fefb2e05a2..5af73d5ea8 100644 --- a/src/query-runner/BaseQueryRunner.ts +++ b/src/query-runner/BaseQueryRunner.ts @@ -324,10 +324,18 @@ export abstract class BaseQueryRunner { return false; } - if (this.connection.driver.dataTypeDefaults + const defaultWidthForType = this.connection.driver.dataTypeDefaults && this.connection.driver.dataTypeDefaults[column.type] - && this.connection.driver.dataTypeDefaults[column.type].width) { - return this.connection.driver.dataTypeDefaults[column.type].width === width; + && this.connection.driver.dataTypeDefaults[column.type].width; + + if (defaultWidthForType) { + // the default widths of these numeric types are 1 less when the column is unsigned. + const typesWithReducedUnsignedDefault = ["int", "tinyint", "smallint", "mediumint"]; + if (column.unsigned && -1 < typesWithReducedUnsignedDefault.indexOf(column.type)) { + return (defaultWidthForType - 1) === width; + } else { + return defaultWidthForType === width; + } } return false; diff --git a/test/github-issues/2943/entity/Test.ts b/test/github-issues/2943/entity/Test.ts new file mode 100644 index 0000000000..9055aa93b1 --- /dev/null +++ b/test/github-issues/2943/entity/Test.ts @@ -0,0 +1,22 @@ +import { Column, Entity, PrimaryColumn } from "../../../../src"; + +@Entity() +export class Test { + @PrimaryColumn() + id: number; + + @Column({ type: 'int', unsigned: true}) + uInt: number; + + @Column({ type: 'tinyint', unsigned: true}) + uTinyInt: number; + + @Column({ type: 'smallint', unsigned: true}) + uSmallInt: number; + + @Column({ type: 'mediumint', unsigned: true}) + uMediumInt: number; + + @Column({ type: 'bigint', unsigned: true}) + uBigInt: number; +} diff --git a/test/github-issues/2943/issue-2943.ts b/test/github-issues/2943/issue-2943.ts new file mode 100644 index 0000000000..6e0d4a1a8a --- /dev/null +++ b/test/github-issues/2943/issue-2943.ts @@ -0,0 +1,29 @@ +import "reflect-metadata"; +import { expect } from "chai"; +import { Connection } from "../../../src"; +import { closeTestingConnections, createTestingConnections, reloadTestingDatabases } from "../../utils/test-utils"; +import { Test } from "./entity/Test"; + +describe.only("github issues > #2943 Inappropriate migration generated", () => { + + let connections: Connection[]; + + before(async () => { + connections = await createTestingConnections({ + enabledDrivers: ['mariadb', 'mysql'], + entities: [Test], + schemaCreate: true, + dropSchema: true + }); + }); + beforeEach(() => reloadTestingDatabases(connections)); + after(() => closeTestingConnections(connections)); + + it("should not create migrations for unsigned numeric types with no specified width", () => + Promise.all(connections.map(async (connection) => { + const sqlInMemory = await connection.driver.createSchemaBuilder().log(); + + expect(sqlInMemory.upQueries).to.eql([]); + expect(sqlInMemory.downQueries).to.eql([]); + }))); +}); From 414a3faada18a7574e7dfba98006304cb2df0d36 Mon Sep 17 00:00:00 2001 From: Michael Bromley Date: Fri, 28 Aug 2020 21:48:32 +0200 Subject: [PATCH 2/5] test: Enable all tests --- test/github-issues/2943/issue-2943.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/github-issues/2943/issue-2943.ts b/test/github-issues/2943/issue-2943.ts index 6e0d4a1a8a..5fee6e7754 100644 --- a/test/github-issues/2943/issue-2943.ts +++ b/test/github-issues/2943/issue-2943.ts @@ -4,7 +4,7 @@ import { Connection } from "../../../src"; import { closeTestingConnections, createTestingConnections, reloadTestingDatabases } from "../../utils/test-utils"; import { Test } from "./entity/Test"; -describe.only("github issues > #2943 Inappropriate migration generated", () => { +describe("github issues > #2943 Inappropriate migration generated", () => { let connections: Connection[]; From 4382d2b8f39171b34caef8a42ebaa694ab9ab62a Mon Sep 17 00:00:00 2001 From: Michael Bromley Date: Fri, 4 Sep 2020 11:01:30 +0200 Subject: [PATCH 3/5] refactor: Move isDefaultColumnWidth() method out of BaseQueryRunner See https://github.com/typeorm/typeorm/pull/6632#pullrequestreview-480932808 for discussion as to why. --- .../AuroraDataApiQueryRunner.ts | 22 +++++++++++++ src/driver/mysql/MysqlQueryRunner.ts | 31 +++++++++++++++++++ src/query-runner/BaseQueryRunner.ts | 29 ----------------- 3 files changed, 53 insertions(+), 29 deletions(-) diff --git a/src/driver/aurora-data-api/AuroraDataApiQueryRunner.ts b/src/driver/aurora-data-api/AuroraDataApiQueryRunner.ts index a9df895eba..14d3d68bab 100644 --- a/src/driver/aurora-data-api/AuroraDataApiQueryRunner.ts +++ b/src/driver/aurora-data-api/AuroraDataApiQueryRunner.ts @@ -1623,4 +1623,26 @@ export class AuroraDataApiQueryRunner extends BaseQueryRunner implements QueryRu return c; } + /** + * Checks if column display width is by default. + */ + protected isDefaultColumnWidth(table: Table, column: TableColumn, width: number): boolean { + // if table have metadata, we check if length is specified in column metadata + if (this.connection.hasMetadata(table.name)) { + const metadata = this.connection.getMetadata(table.name); + const columnMetadata = metadata.findColumnWithDatabaseName(column.name); + if (columnMetadata && columnMetadata.width) + return false; + } + + const defaultWidthForType = this.connection.driver.dataTypeDefaults + && this.connection.driver.dataTypeDefaults[column.type] + && this.connection.driver.dataTypeDefaults[column.type].width; + + if (defaultWidthForType) { + return defaultWidthForType === width; + } + return false; + } + } diff --git a/src/driver/mysql/MysqlQueryRunner.ts b/src/driver/mysql/MysqlQueryRunner.ts index 970412e037..408df7ff4e 100644 --- a/src/driver/mysql/MysqlQueryRunner.ts +++ b/src/driver/mysql/MysqlQueryRunner.ts @@ -1694,4 +1694,35 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner { return result[0]["version"]; } + /** + * Checks if column display width is by default. + */ + protected isDefaultColumnWidth(table: Table, column: TableColumn, width: number): boolean { + // if table have metadata, we check if length is specified in column metadata + if (this.connection.hasMetadata(table.name)) { + const metadata = this.connection.getMetadata(table.name); + const columnMetadata = metadata.findColumnWithDatabaseName(column.name); + if (columnMetadata && columnMetadata.width) + return false; + } + + const defaultWidthForType = this.connection.driver.dataTypeDefaults + && this.connection.driver.dataTypeDefaults[column.type] + && this.connection.driver.dataTypeDefaults[column.type].width; + + if (defaultWidthForType) { + // In MariaDB, the default widths of certain numeric types are 1 less than + // the usual defaults when the column is unsigned. + const isMariaDb = this.driver.options.type === "mariadb"; + const typesWithReducedUnsignedDefault = ["int", "tinyint", "smallint", "mediumint"]; + if (isMariaDb && column.unsigned && -1 < typesWithReducedUnsignedDefault.indexOf(column.type)) { + return (defaultWidthForType - 1) === width; + } else { + return defaultWidthForType === width; + } + } + + return false; + } + } diff --git a/src/query-runner/BaseQueryRunner.ts b/src/query-runner/BaseQueryRunner.ts index 5af73d5ea8..7ab8ebf1fe 100644 --- a/src/query-runner/BaseQueryRunner.ts +++ b/src/query-runner/BaseQueryRunner.ts @@ -312,35 +312,6 @@ export abstract class BaseQueryRunner { return false; } - /** - * Checks if column display width is by default. Used only for MySQL. - */ - protected isDefaultColumnWidth(table: Table, column: TableColumn, width: number): boolean { - // if table have metadata, we check if length is specified in column metadata - if (this.connection.hasMetadata(table.name)) { - const metadata = this.connection.getMetadata(table.name); - const columnMetadata = metadata.findColumnWithDatabaseName(column.name); - if (columnMetadata && columnMetadata.width) - return false; - } - - const defaultWidthForType = this.connection.driver.dataTypeDefaults - && this.connection.driver.dataTypeDefaults[column.type] - && this.connection.driver.dataTypeDefaults[column.type].width; - - if (defaultWidthForType) { - // the default widths of these numeric types are 1 less when the column is unsigned. - const typesWithReducedUnsignedDefault = ["int", "tinyint", "smallint", "mediumint"]; - if (column.unsigned && -1 < typesWithReducedUnsignedDefault.indexOf(column.type)) { - return (defaultWidthForType - 1) === width; - } else { - return defaultWidthForType === width; - } - } - - return false; - } - /** * Checks if column precision is by default. */ From 8a0fb4de14d7237a5371b888fc9c15f70cce5110 Mon Sep 17 00:00:00 2001 From: Michael Bromley Date: Wed, 9 Sep 2020 11:30:11 +0200 Subject: [PATCH 4/5] fix: Correct unsigned int behaviour for MySQL 5.7 --- src/driver/mysql/MysqlQueryRunner.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/driver/mysql/MysqlQueryRunner.ts b/src/driver/mysql/MysqlQueryRunner.ts index 408df7ff4e..6d039ec0df 100644 --- a/src/driver/mysql/MysqlQueryRunner.ts +++ b/src/driver/mysql/MysqlQueryRunner.ts @@ -1711,11 +1711,10 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner { && this.connection.driver.dataTypeDefaults[column.type].width; if (defaultWidthForType) { - // In MariaDB, the default widths of certain numeric types are 1 less than + // In MariaDB & MySQL 5.7, the default widths of certain numeric types are 1 less than // the usual defaults when the column is unsigned. - const isMariaDb = this.driver.options.type === "mariadb"; const typesWithReducedUnsignedDefault = ["int", "tinyint", "smallint", "mediumint"]; - if (isMariaDb && column.unsigned && -1 < typesWithReducedUnsignedDefault.indexOf(column.type)) { + if (column.unsigned && -1 < typesWithReducedUnsignedDefault.indexOf(column.type)) { return (defaultWidthForType - 1) === width; } else { return defaultWidthForType === width; From 71fb2869fb7e37f396e0a6ab7cb8246f2f4815fe Mon Sep 17 00:00:00 2001 From: Michael Bromley Date: Wed, 9 Sep 2020 20:24:34 +0200 Subject: [PATCH 5/5] fix: Correct position of zerofill check Plus stylistic change based on code review --- src/driver/mysql/MysqlQueryRunner.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/driver/mysql/MysqlQueryRunner.ts b/src/driver/mysql/MysqlQueryRunner.ts index 6d039ec0df..4eff8332fa 100644 --- a/src/driver/mysql/MysqlQueryRunner.ts +++ b/src/driver/mysql/MysqlQueryRunner.ts @@ -1296,6 +1296,7 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner { tableColumn.name = dbColumn["COLUMN_NAME"]; tableColumn.type = dbColumn["DATA_TYPE"].toLowerCase(); + tableColumn.zerofill = dbColumn["COLUMN_TYPE"].indexOf("zerofill") !== -1; tableColumn.unsigned = tableColumn.zerofill ? true : dbColumn["COLUMN_TYPE"].indexOf("unsigned") !== -1; if (this.driver.withWidthColumnTypes.indexOf(tableColumn.type as ColumnType) !== -1) { const width = dbColumn["COLUMN_TYPE"].substring(dbColumn["COLUMN_TYPE"].indexOf("(") + 1, dbColumn["COLUMN_TYPE"].indexOf(")")); @@ -1333,7 +1334,6 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner { tableColumn.isPrimary = dbPrimaryKeys.some(dbPrimaryKey => { return this.driver.buildTableName(dbPrimaryKey["TABLE_NAME"], undefined, dbPrimaryKey["TABLE_SCHEMA"]) === tableFullName && dbPrimaryKey["COLUMN_NAME"] === tableColumn.name; }); - tableColumn.zerofill = dbColumn["COLUMN_TYPE"].indexOf("zerofill") !== -1; tableColumn.isGenerated = dbColumn["EXTRA"].indexOf("auto_increment") !== -1; if (tableColumn.isGenerated) tableColumn.generationStrategy = "increment"; @@ -1714,7 +1714,8 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner { // In MariaDB & MySQL 5.7, the default widths of certain numeric types are 1 less than // the usual defaults when the column is unsigned. const typesWithReducedUnsignedDefault = ["int", "tinyint", "smallint", "mediumint"]; - if (column.unsigned && -1 < typesWithReducedUnsignedDefault.indexOf(column.type)) { + const needsAdjustment = typesWithReducedUnsignedDefault.indexOf(column.type) !== -1; + if (column.unsigned && needsAdjustment) { return (defaultWidthForType - 1) === width; } else { return defaultWidthForType === width;