Skip to content

Commit

Permalink
fix: redundant migration with decimal default (#6879)
Browse files Browse the repository at this point in the history
MySQL/MariaDB decimal column is represented like '0.0'.
MysqlDriver.normalizeDefault didn't consider trailing zeros.
User can bypass this by passing default as string, but we
can't guide the zeros by type, so we should convert user input.

undefined scale was compared with defined scale. Skip this comparison.

Close #6140
Close #5407
  • Loading branch information
jeiea committed Oct 11, 2020
1 parent 3e8ddca commit 6ff67f7
Show file tree
Hide file tree
Showing 7 changed files with 82 additions and 9 deletions.
4 changes: 2 additions & 2 deletions src/driver/cockroachdb/CockroachDriver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ export class CockroachDriver implements Driver {
const arrayCast = columnMetadata.isArray ? `::${columnMetadata.type}[]` : "";

if (typeof defaultValue === "number") {
return "" + defaultValue;
return `(${defaultValue})`;

} else if (typeof defaultValue === "boolean") {
return defaultValue === true ? "true" : "false";
Expand Down Expand Up @@ -618,7 +618,7 @@ export class CockroachDriver implements Driver {
|| tableColumn.type !== this.normalizeType(columnMetadata)
|| tableColumn.length !== columnMetadata.length
|| tableColumn.precision !== columnMetadata.precision
|| tableColumn.scale !== columnMetadata.scale
|| (columnMetadata.scale !== undefined && tableColumn.scale !== columnMetadata.scale)
// || tableColumn.comment !== columnMetadata.comment // todo
|| (!tableColumn.isGenerated && this.lowerDefaultValueIfNecessary(this.normalizeDefault(columnMetadata)) !== tableColumn.default) // we included check for generated here, because generated columns already can have default values
|| tableColumn.isPrimary !== columnMetadata.isPrimary
Expand Down
1 change: 1 addition & 0 deletions src/driver/cockroachdb/CockroachQueryRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1492,6 +1492,7 @@ export class CockroachQueryRunner extends BaseQueryRunner implements QueryRunner

} else {
tableColumn.default = dbColumn["column_default"].replace(/:::.*/, "");
tableColumn.default = tableColumn.default.replace(/^(-?[\d\.]+)$/, "($1)");
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/driver/mysql/MysqlDriver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ export class MysqlDriver implements Driver {
}

if (typeof defaultValue === "number") {
return "" + defaultValue;
return `'${defaultValue.toFixed(columnMetadata.scale)}'`;

} else if (typeof defaultValue === "boolean") {
return defaultValue === true ? "1" : "0";
Expand Down Expand Up @@ -770,8 +770,8 @@ export class MysqlDriver implements Driver {
|| tableColumn.type !== this.normalizeType(columnMetadata)
|| tableColumn.length !== columnMetadataLength
|| tableColumn.width !== columnMetadata.width
|| tableColumn.precision !== columnMetadata.precision
|| tableColumn.scale !== columnMetadata.scale
|| (columnMetadata.precision !== undefined && tableColumn.precision !== columnMetadata.precision)
|| (columnMetadata.scale !== undefined && tableColumn.scale !== columnMetadata.scale)
|| tableColumn.zerofill !== columnMetadata.zerofill
|| tableColumn.unsigned !== columnMetadata.unsigned
|| tableColumn.asExpression !== columnMetadata.asExpression
Expand Down
8 changes: 4 additions & 4 deletions src/driver/postgres/PostgresDriver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ export class PostgresDriver implements Driver {
return metadata.columns.filter(column => this.spatialTypes.indexOf(column.type) >= 0).length > 0;
});
const hasLtreeColumns = this.connection.entityMetadatas.some(metadata => {
return metadata.columns.filter(column => column.type === 'ltree').length > 0;
return metadata.columns.filter(column => column.type === "ltree").length > 0;
});
const hasExclusionConstraints = this.connection.entityMetadatas.some(metadata => {
return metadata.exclusions.length > 0;
Expand Down Expand Up @@ -498,7 +498,7 @@ export class PostgresDriver implements Driver {
return `(${value.join(",")})`;

} else if (columnMetadata.type === "ltree") {
return value.split(".").filter(Boolean).join('.').replace(/[\s]+/g, "_");
return value.split(".").filter(Boolean).join(".").replace(/[\s]+/g, "_");
} else if (
(
columnMetadata.type === "enum"
Expand Down Expand Up @@ -734,7 +734,7 @@ export class PostgresDriver implements Driver {
}

if (typeof defaultValue === "number") {
return "" + defaultValue;
return `'${defaultValue}'`;

} else if (typeof defaultValue === "boolean") {
return defaultValue === true ? "true" : "false";
Expand Down Expand Up @@ -874,7 +874,7 @@ export class PostgresDriver implements Driver {
|| tableColumn.type !== this.normalizeType(columnMetadata)
|| tableColumn.length !== columnMetadata.length
|| tableColumn.precision !== columnMetadata.precision
|| tableColumn.scale !== columnMetadata.scale
|| (columnMetadata.scale !== undefined && tableColumn.scale !== columnMetadata.scale)
// || tableColumn.comment !== columnMetadata.comment // todo
|| (!tableColumn.isGenerated && this.lowerDefaultValueIfNecessary(this.normalizeDefault(columnMetadata)) !== tableColumn.default) // we included check for generated here, because generated columns already can have default values
|| tableColumn.isPrimary !== columnMetadata.isPrimary
Expand Down
1 change: 1 addition & 0 deletions src/driver/postgres/PostgresQueryRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1589,6 +1589,7 @@ export class PostgresQueryRunner extends BaseQueryRunner implements QueryRunner
tableColumn.generationStrategy = "uuid";
} else {
tableColumn.default = dbColumn["column_default"].replace(/::.*/, "");
tableColumn.default = tableColumn.default.replace(/^(-?\d+)$/, "'$1'");
}
}

Expand Down
41 changes: 41 additions & 0 deletions test/github-issues/5407/entity/User.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import {PrimaryColumn, Column} from "../../../../src";
import {Entity} from "../../../../src/decorator/entity/Entity";

@Entity()
export class User {

@PrimaryColumn()
id: number;

@Column("decimal", { default: -0, precision: 3, scale: 1 })
decimalWithDefault: number;

@Column("decimal", { default: 100, precision: 3 })
noScale: number;

@Column("decimal", { default: 10, precision: 3, scale: 0 })
zeroScale: number;

@Column("decimal", { default: 9999999999 })
maxDefault: number;

@Column("decimal", { default: -9999999999 })
minDefault: number;

@Column("int", { default: -100 })
intDefault: number;

@Column("float", { default: 3.5 })
floatDefault: number;

@Column({ default: "New user" })
stringDefault: string;

// ER_PARSE_ERROR
// @Column("decimal", { default: 0, precision: 8, scale: -4 })
// negativeScale: number;

// ER_INVALID_DEFAULT
// @Column("decimal", { default: -12345.67890, precision: 8, scale: 4 })
// defaultOverflow: number;
}
30 changes: 30 additions & 0 deletions test/github-issues/5407/issue-5407.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import "reflect-metadata";
import {Connection} from "../../../src";
import {createTestingConnections, closeTestingConnections} from "../../utils/test-utils";
import {User} from "./entity/User";

describe("github issues > #5407 Wrong migration created because of default column value format", () => {
let connections: Connection[];
before(async () => connections = await createTestingConnections({
migrations: [],
enabledDrivers: ["mysql", "mariadb", "postgres", "better-sqlite3", "cockroachdb", "sqlite"],
schemaCreate: false,
dropSchema: true,
entities: [User],
}));
after(() => closeTestingConnections(connections));

it("can recognize model changes", () => Promise.all(connections.map(async connection => {
const sqlInMemory = await connection.driver.createSchemaBuilder().log();
sqlInMemory.upQueries.length.should.be.greaterThan(0);
sqlInMemory.downQueries.length.should.be.greaterThan(0);
})));

it("does not generate when no model changes", () => Promise.all(connections.map(async connection => {
await connection.driver.createSchemaBuilder().build();

const sqlInMemory = await connection.driver.createSchemaBuilder().log();
sqlInMemory.upQueries.length.should.be.equal(0);
sqlInMemory.downQueries.length.should.be.equal(0);
})));
});

0 comments on commit 6ff67f7

Please sign in to comment.