Skip to content

Commit

Permalink
fix: wrong migration generation when column default value is set to n…
Browse files Browse the repository at this point in the history 
…ull #6950 (#7356)

* fix: #6950;

* removed single driver in test;

* fixed issues with column `comment` change detection;
removed `schema` from test due to SqlServer error;

* fixed `default` normalization in oracle;

* commented debug logs;

* removed log;
  • Loading branch information
@AlexMesser
AlexMesser committed Feb 9, 2021
1 parent b9d7898 commit 5a3f9ff
Show file tree
Hide file tree
Showing 21 changed files with 103 additions and 84 deletions.
2 changes: 1 addition & 1 deletion src/driver/Driver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ export interface Driver {
/**
* Normalizes "default" value of the column.
*/
normalizeDefault(columnMetadata: ColumnMetadata): string;
normalizeDefault(columnMetadata: ColumnMetadata): string | undefined;

/**
* Normalizes "isUnique" value of the column.
Expand Down
7 changes: 2 additions & 5 deletions src/driver/aurora-data-api/AuroraDataApiDriver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,7 @@ export class AuroraDataApiDriver implements Driver {
/**
* Normalizes "default" value of the column.
*/
normalizeDefault(columnMetadata: ColumnMetadata): string {
normalizeDefault(columnMetadata: ColumnMetadata): string | undefined {
const defaultValue = columnMetadata.default;

if ((columnMetadata.type === "enum" || columnMetadata.type === "simple-enum") && defaultValue !== undefined) {
Expand All @@ -550,9 +550,6 @@ export class AuroraDataApiDriver implements Driver {
} else if (typeof defaultValue === "string") {
return `'${defaultValue}'`;

} else if (defaultValue === null) {
return `null`;

} else {
return defaultValue;
}
Expand Down Expand Up @@ -827,7 +824,7 @@ export class AuroraDataApiDriver implements Driver {
/**
* Checks if "DEFAULT" values in the column metadata and in the database are equal.
*/
protected compareDefaultValues(columnMetadataValue: string, databaseValue: string): boolean {
protected compareDefaultValues(columnMetadataValue: string | undefined, databaseValue: string | undefined): boolean {
if (typeof columnMetadataValue === "string" && typeof databaseValue === "string") {
// we need to cut out "'" because in mysql we can understand returned value is a string or a function
// as result compare cannot understand if default is really changed or not
Expand Down
9 changes: 3 additions & 6 deletions src/driver/cockroachdb/CockroachDriver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ export class CockroachDriver implements Driver {
/**
* Normalizes "default" value of the column.
*/
normalizeDefault(columnMetadata: ColumnMetadata): string {
normalizeDefault(columnMetadata: ColumnMetadata): string | undefined {
const defaultValue = columnMetadata.default;
const arrayCast = columnMetadata.isArray ? `::${columnMetadata.type}[]` : "";

Expand All @@ -495,10 +495,7 @@ export class CockroachDriver implements Driver {
} else if (typeof defaultValue === "string") {
return `'${defaultValue}'${arrayCast}`;

} else if (defaultValue === null) {
return `null`;

} else if (typeof defaultValue === "object") {
} else if (typeof defaultValue === "object" && defaultValue !== null) {
return `'${JSON.stringify(defaultValue)}'`;

} else {
Expand Down Expand Up @@ -619,7 +616,7 @@ export class CockroachDriver implements Driver {
|| tableColumn.length !== columnMetadata.length
|| tableColumn.precision !== columnMetadata.precision
|| (columnMetadata.scale !== undefined && tableColumn.scale !== columnMetadata.scale)
|| (tableColumn.comment || "") !== columnMetadata.comment
|| tableColumn.comment !== columnMetadata.comment
|| (!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
|| tableColumn.isNullable !== columnMetadata.isNullable
Expand Down
6 changes: 3 additions & 3 deletions src/driver/mongodb/MongoDriver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ export class MongoDriver implements Driver {
/**
* Normalizes "default" value of the column.
*/
normalizeDefault(columnMetadata: ColumnMetadata): string {
normalizeDefault(columnMetadata: ColumnMetadata): string | undefined {
throw new Error(`MongoDB is schema-less, not supported by this driver.`);
}

Expand Down Expand Up @@ -442,8 +442,8 @@ export class MongoDriver implements Driver {
const credentialsUrlPart = (options.username && options.password)
? `${options.username}:${options.password}@`
: "";
const portUrlPart = (schemaUrlPart === "mongodb+srv")
? ""
const portUrlPart = (schemaUrlPart === "mongodb+srv")
? ""
: `:${options.port || "27017"}`;

return `${schemaUrlPart}://${credentialsUrlPart}${options.host || "127.0.0.1"}${portUrlPart}/${options.database || ""}`;
Expand Down
8 changes: 4 additions & 4 deletions src/driver/mysql/MysqlDriver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,7 @@ export class MysqlDriver implements Driver {
/**
* Normalizes "default" value of the column.
*/
normalizeDefault(columnMetadata: ColumnMetadata): string {
normalizeDefault(columnMetadata: ColumnMetadata): string | undefined {
const defaultValue = columnMetadata.default;

if ((columnMetadata.type === "enum" || columnMetadata.type === "simple-enum") && defaultValue !== undefined) {
Expand All @@ -603,7 +603,7 @@ export class MysqlDriver implements Driver {
return `'${defaultValue}'`;

} else if (defaultValue === null) {
return `NULL`;
return undefined;

} else {
return defaultValue;
Expand Down Expand Up @@ -776,7 +776,7 @@ export class MysqlDriver implements Driver {
|| tableColumn.unsigned !== columnMetadata.unsigned
|| tableColumn.asExpression !== columnMetadata.asExpression
|| tableColumn.generatedType !== columnMetadata.generatedType
|| (tableColumn.comment || "") !== columnMetadata.comment
|| tableColumn.comment !== columnMetadata.comment
|| !this.compareDefaultValues(this.normalizeDefault(columnMetadata), tableColumn.default)
|| (tableColumn.enum && columnMetadata.enum && !OrmUtils.isArraysEqual(tableColumn.enum, columnMetadata.enum.map(val => val + "")))
|| tableColumn.onUpdate !== columnMetadata.onUpdate
Expand Down Expand Up @@ -919,7 +919,7 @@ export class MysqlDriver implements Driver {
/**
* Checks if "DEFAULT" values in the column metadata and in the database are equal.
*/
protected compareDefaultValues(columnMetadataValue: string, databaseValue: string): boolean {
protected compareDefaultValues(columnMetadataValue: string | undefined, databaseValue: string | undefined): boolean {
if (typeof columnMetadataValue === "string" && typeof databaseValue === "string") {
// we need to cut out "'" because in mysql we can understand returned value is a string or a function
// as result compare cannot understand if default is really changed or not
Expand Down
4 changes: 2 additions & 2 deletions src/driver/mysql/MysqlQueryRunner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1812,7 +1812,7 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner {
* Escapes a given comment so it's safe to include in a query.
*/
protected escapeComment(comment?: string) {
if (comment === undefined || comment.length === 0) {
if (!comment || comment.length === 0) {
return `''`;
}

Expand Down Expand Up @@ -1872,7 +1872,7 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner {
c += " PRIMARY KEY";
if (column.isGenerated && column.generationStrategy === "increment") // don't use skipPrimary here since updates can update already exist primary without auto inc.
c += " AUTO_INCREMENT";
if (column.comment !== undefined && column.comment.length > 0)
if (column.comment && column.comment.length > 0)
c += ` COMMENT ${this.escapeComment(column.comment)}`;
if (column.default !== undefined && column.default !== null)
c += ` DEFAULT ${column.default}`;
Expand Down
32 changes: 28 additions & 4 deletions src/driver/oracle/OracleDriver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ export class OracleDriver implements Driver {
/**
* Normalizes "default" value of the column.
*/
normalizeDefault(columnMetadata: ColumnMetadata): string {
normalizeDefault(columnMetadata: ColumnMetadata): string | undefined {
const defaultValue = columnMetadata.default;

if (typeof defaultValue === "number") {
Expand All @@ -479,6 +479,9 @@ export class OracleDriver implements Driver {
} else if (typeof defaultValue === "string") {
return `'${defaultValue}'`;

} else if (defaultValue === null) {
return undefined;

} else {
return defaultValue;
}
Expand Down Expand Up @@ -599,17 +602,38 @@ export class OracleDriver implements Driver {
if (!tableColumn)
return false; // we don't need new columns, we only need exist and changed

return tableColumn.name !== columnMetadata.databaseName
const isColumnChanged = tableColumn.name !== columnMetadata.databaseName
|| tableColumn.type !== this.normalizeType(columnMetadata)
|| tableColumn.length !== columnMetadata.length
|| tableColumn.precision !== columnMetadata.precision
|| tableColumn.scale !== columnMetadata.scale
// || tableColumn.comment !== columnMetadata.comment || // todo
|| this.normalizeDefault(columnMetadata) !== tableColumn.default
// || tableColumn.comment !== columnMetadata.comment
|| tableColumn.default !== this.normalizeDefault(columnMetadata)
|| tableColumn.isPrimary !== columnMetadata.isPrimary
|| tableColumn.isNullable !== columnMetadata.isNullable
|| tableColumn.isUnique !== this.normalizeIsUnique(columnMetadata)
|| (columnMetadata.generationStrategy !== "uuid" && tableColumn.isGenerated !== columnMetadata.isGenerated);

// DEBUG SECTION
// if (isColumnChanged) {
// console.log("table:", columnMetadata.entityMetadata.tableName);
// console.log("name:", tableColumn.name, columnMetadata.databaseName);
// console.log("type:", tableColumn.type, this.normalizeType(columnMetadata));
// console.log("length:", tableColumn.length, columnMetadata.length);
// console.log("precision:", tableColumn.precision, columnMetadata.precision);
// console.log("scale:", tableColumn.scale, columnMetadata.scale);
// console.log("comment:", tableColumn.comment, columnMetadata.comment);
// console.log("default:", tableColumn.default, this.normalizeDefault(columnMetadata));
// console.log("enum:", tableColumn.enum && columnMetadata.enum && !OrmUtils.isArraysEqual(tableColumn.enum, columnMetadata.enum.map(val => val + "")));
// console.log("onUpdate:", tableColumn.onUpdate, columnMetadata.onUpdate);
// console.log("isPrimary:", tableColumn.isPrimary, columnMetadata.isPrimary);
// console.log("isNullable:", tableColumn.isNullable, columnMetadata.isNullable);
// console.log("isUnique:", tableColumn.isUnique, this.normalizeIsUnique(columnMetadata));
// console.log("isGenerated:", tableColumn.isGenerated, columnMetadata.isGenerated);
// console.log("==========================================");
// }

return isColumnChanged
});
}

Expand Down
33 changes: 27 additions & 6 deletions src/driver/postgres/PostgresDriver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -747,10 +747,7 @@ export class PostgresDriver implements Driver {
} else if (typeof defaultValue === "string") {
return `'${defaultValue}'${arrayCast}`;

} else if (defaultValue === null) {
return `null`;

} else if (typeof defaultValue === "object") {
} else if (typeof defaultValue === "object" && defaultValue !== null) {
return `'${JSON.stringify(defaultValue)}'`;

} else {
Expand Down Expand Up @@ -872,12 +869,12 @@ export class PostgresDriver implements Driver {
if (!tableColumn)
return false; // we don't need new columns, we only need exist and changed

return tableColumn.name !== columnMetadata.databaseName
const isColumnChanged = tableColumn.name !== columnMetadata.databaseName
|| tableColumn.type !== this.normalizeType(columnMetadata)
|| tableColumn.length !== columnMetadata.length
|| tableColumn.precision !== columnMetadata.precision
|| (columnMetadata.scale !== undefined && tableColumn.scale !== columnMetadata.scale)
|| (tableColumn.comment || "") !== columnMetadata.comment
|| tableColumn.comment !== columnMetadata.comment
|| (!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
|| tableColumn.isNullable !== columnMetadata.isNullable
Expand All @@ -886,6 +883,29 @@ export class PostgresDriver implements Driver {
|| tableColumn.isGenerated !== columnMetadata.isGenerated
|| (tableColumn.spatialFeatureType || "").toLowerCase() !== (columnMetadata.spatialFeatureType || "").toLowerCase()
|| tableColumn.srid !== columnMetadata.srid;

// DEBUG SECTION
// if (isColumnChanged) {
// console.log("table:", columnMetadata.entityMetadata.tableName);
// console.log("name:", tableColumn.name, columnMetadata.databaseName);
// console.log("type:", tableColumn.type, this.normalizeType(columnMetadata));
// console.log("length:", tableColumn.length, columnMetadata.length);
// console.log("precision:", tableColumn.precision, columnMetadata.precision);
// console.log("scale:", tableColumn.scale, columnMetadata.scale);
// console.log("comment:", tableColumn.comment, columnMetadata.comment);
// console.log("enum:", tableColumn.enum && columnMetadata.enum && !OrmUtils.isArraysEqual(tableColumn.enum, columnMetadata.enum.map(val => val + "")));
// console.log("onUpdate:", tableColumn.onUpdate, columnMetadata.onUpdate);
// console.log("isPrimary:", tableColumn.isPrimary, columnMetadata.isPrimary);
// console.log("isNullable:", tableColumn.isNullable, columnMetadata.isNullable);
// console.log("isUnique:", tableColumn.isUnique, this.normalizeIsUnique(columnMetadata));
// console.log("isGenerated:", tableColumn.isGenerated, columnMetadata.isGenerated);
// console.log("isGenerated 2:", !tableColumn.isGenerated && this.lowerDefaultValueIfNecessary(this.normalizeDefault(columnMetadata)) !== tableColumn.default);
// console.log("spatialFeatureType:", (tableColumn.spatialFeatureType || "").toLowerCase(), (columnMetadata.spatialFeatureType || "").toLowerCase());
// console.log("srid", tableColumn.srid, columnMetadata.srid);
// console.log("==========================================");
// }

return isColumnChanged
});
}

Expand All @@ -898,6 +918,7 @@ export class PostgresDriver implements Driver {
return i % 2 === 1 ? v : v.toLowerCase();
}).join(`'`);
}

/**
* Returns true if driver supports RETURNING / OUTPUT statement.
*/
Expand Down
10 changes: 5 additions & 5 deletions src/driver/postgres/PostgresQueryRunner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1417,9 +1417,9 @@ export class PostgresQueryRunner extends BaseQueryRunner implements QueryRunner
return `("table_schema" = '${schema}' AND "table_name" = '${name}')`;
}).join(" OR ");
const tablesSql = `SELECT * FROM "information_schema"."tables" WHERE ` + tablesCondition;

/**
* Uses standard SQL information_schema.columns table and postgres-specific
* Uses standard SQL information_schema.columns table and postgres-specific
* pg_catalog.pg_attribute table to get column information.
* @see https://stackoverflow.com/a/19541865
*/
Expand Down Expand Up @@ -1647,7 +1647,7 @@ export class PostgresQueryRunner extends BaseQueryRunner implements QueryRunner
}
}

tableColumn.comment = dbColumn["description"] == null ? undefined : dbColumn["description"];
tableColumn.comment = dbColumn["description"] ? dbColumn["description"] : undefined;
if (dbColumn["character_set_name"])
tableColumn.charset = dbColumn["character_set_name"];
if (dbColumn["collation_name"])
Expand Down Expand Up @@ -2078,7 +2078,7 @@ export class PostgresQueryRunner extends BaseQueryRunner implements QueryRunner
let seqName = `${tableName}_${columnName}_seq`;
if (seqName.length > this.connection.driver.maxAliasLength!) // note doesn't yet handle corner cases where .length differs from number of UTF-8 bytes
seqName=`${tableName.substring(0,29)}_${columnName.substring(0,Math.max(29,63-tableName.length-5))}_seq`;

if (schema && schema !== currentSchema && !skipSchema) {
return disableEscape ? `${schema}.${seqName}` : `"${schema}"."${seqName}"`;
} else {
Expand Down Expand Up @@ -2130,7 +2130,7 @@ export class PostgresQueryRunner extends BaseQueryRunner implements QueryRunner
* Escapes a given comment so it's safe to include in a query.
*/
protected escapeComment(comment?: string) {
if (comment === undefined || comment.length === 0) {
if (!comment || comment.length === 0) {
return "NULL";
}

Expand Down
2 changes: 1 addition & 1 deletion src/driver/sap/SapDriver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ export class SapDriver implements Driver {
/**
* Normalizes "default" value of the column.
*/
normalizeDefault(columnMetadata: ColumnMetadata): string {
normalizeDefault(columnMetadata: ColumnMetadata): string | undefined {
const defaultValue = columnMetadata.default;

if (typeof defaultValue === "number") {
Expand Down
5 changes: 4 additions & 1 deletion src/driver/sqlite-abstract/AbstractSqliteDriver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ export abstract class AbstractSqliteDriver implements Driver {
/**
* Normalizes "default" value of the column.
*/
normalizeDefault(columnMetadata: ColumnMetadata): string {
normalizeDefault(columnMetadata: ColumnMetadata): string | undefined {
const defaultValue = columnMetadata.default;

if (typeof defaultValue === "number") {
Expand All @@ -463,6 +463,9 @@ export abstract class AbstractSqliteDriver implements Driver {
} else if (typeof defaultValue === "string") {
return `'${defaultValue}'`;

} else if (defaultValue === null) {
return undefined;

} else {
return defaultValue;
}
Expand Down
2 changes: 1 addition & 1 deletion src/driver/sqlserver/SqlServerDriver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ export class SqlServerDriver implements Driver {
/**
* Normalizes "default" value of the column.
*/
normalizeDefault(columnMetadata: ColumnMetadata): string {
normalizeDefault(columnMetadata: ColumnMetadata): string | undefined {
const defaultValue = columnMetadata.default;

if (typeof defaultValue === "number") {
Expand Down
2 changes: 1 addition & 1 deletion src/metadata/ColumnMetadata.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export class ColumnMetadata {
* Column comment.
* This feature is not supported by all databases.
*/
comment: string = "";
comment?: string;

/**
* Default database value.
Expand Down
2 changes: 1 addition & 1 deletion src/query-builder/InsertQueryBuilder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,7 @@ export class InsertQueryBuilder<Entity> extends QueryBuilder<Entity> {
// if value for this column was not provided then insert default value
} else if (value === undefined) {
if ((this.connection.driver instanceof OracleDriver && valueSets.length > 1) || this.connection.driver instanceof AbstractSqliteDriver || this.connection.driver instanceof SapDriver) { // unfortunately sqlite does not support DEFAULT expression in INSERT queries
if (column.default !== undefined) { // try to use default defined in the column
if (column.default !== undefined && column.default !== null) { // try to use default defined in the column
expression += this.connection.driver.normalizeDefault(column);
} else {
expression += "NULL"; // otherwise simply use NULL and pray if column is nullable
Expand Down
Loading

0 comments on commit 5a3f9ff

Please sign in to comment.