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: Unnecessary migrations for unsigned numeric types #6632

Merged
merged 5 commits into from Sep 10, 2020
Merged

fix: Unnecessary migrations for unsigned numeric types #6632

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b6fef26
fix: Unnecessary migrations for unsigned numeric types
michaelbromley Aug 28, 2020
414a3fa
test: Enable all tests
michaelbromley Aug 28, 2020
4382d2b
refactor: Move isDefaultColumnWidth() method out of BaseQueryRunner
michaelbromley Sep 4, 2020
8a0fb4d
fix: Correct unsigned int behaviour for MySQL 5.7
michaelbromley Sep 9, 2020
71fb286
fix: Correct position of zerofill check
michaelbromley Sep 9, 2020
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
2 changes: 1 addition & 1 deletion src/driver/mysql/MysqlQueryRunner.ts
View file
Open in desktop
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

tableColumn.zerofill is set below, why did you move it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. The value of tableColumn.unsigned is used inside the isDefaultColumnWidth() method, so it needs to be set prior to checking the width, which is why I moved it up. I missed the dependency on the zerofill value. I'll move the zerofill assignment above the unsigned assignment.

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;
Expand Down Expand Up @@ -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";
Expand Down
14 changes: 11 additions & 3 deletions src/query-runner/BaseQueryRunner.ts
View file
Open in desktop
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

is this statement true for all databases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, actually only MariaDB. The method this code lives in (isDefaultColumnWidth) is only called for the MySQL driver, as stated in the comment above it, and it turns out that the method is actually only hit when using MariaDB specifically, because this line always results in width = undefined for MySQL due to it's representation of dbColumn["COLUMN_TYPE"] not including the width part.

Copy link
Member

Choose a reason for hiding this comment

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

Having this check for all drivers is confusing. We need to move this logic to a driver-specific method, e.g. driver.compareColumnWidths or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I agree. I can do this refractor as an addition to this PR.

Ps if you are planning to make a release soon let me know so I can do the changes in time.

Copy link
Member

Choose a reason for hiding this comment

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

yeah its already quite a while I didn't make a release, so yeah, expect a release soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I just looked into this more. The isDefaultColumnWidth method is actually used in 2 QueryRunners: MysqlQueryRunner & AuroraQueryRunner. I guess that's why it was put on the base class.

I don't see any tests for Aurora in CI, so I've no way to be sure that my changes are valid for Aurora.

I question the value of putting it in the Driver interface because as far as I can tell it is an (almost useless) feature of MySQL only. Here's a good explanation of what it actually does.

Here's another suggestion:

  1. Move the isDefaultColumnWidth method from the Base class to both MysqlQueryRunner and AuroraQueryRunner.
  2. Leave the AuroraQueryRunner version as it was before the fix, since I do not know whether it breaks anything there.
  3. In the MysqlQueryRunner include the changes from this PR, with an additional check of the existing isMariaDb boolean to further make sure (and make explicit) that the fix only applies to MariaDB.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, go ahead

const typesWithReducedUnsignedDefault = ["int", "tinyint", "smallint", "mediumint"];
if (column.unsigned && -1 < typesWithReducedUnsignedDefault.indexOf(column.type)) {
return (defaultWidthForType - 1) === width;
} else {
return defaultWidthForType === width;
}
}

return false;
Expand Down
22 changes: 22 additions & 0 deletions test/github-issues/2943/entity/Test.ts
View file
Open in desktop
@@ -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;
}
29 changes: 29 additions & 0 deletions test/github-issues/2943/issue-2943.ts
View file
Open in desktop
@@ -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("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([]);
})));
});