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: fixed all known enum issues #7419

Merged
merged 20 commits into from
Mar 5, 2021
Merged

fix: fixed all known enum issues #7419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
5fe13b1
fix #5371
AlexMesser Feb 24, 2021
734bd2b
fix #6471;
AlexMesser Feb 25, 2021
48f1740
fixed falling test;
AlexMesser Feb 25, 2021
3c5f946
added test for #7217
AlexMesser Feb 25, 2021
009fd39
fix #6047, #7283;
AlexMesser Feb 25, 2021
cc46ecd
fix #5871
AlexMesser Feb 25, 2021
6796563
added support for `enumName` in `joinColumns` (#5729)
AlexMesser Feb 25, 2021
4613f31
fix #5478
AlexMesser Feb 25, 2021
94ab29d
fixed falling test;
AlexMesser Feb 26, 2021
bcdc33d
added column `array` property change detection (#5882);
AlexMesser Feb 26, 2021
798763d
fix #5275
AlexMesser Feb 26, 2021
2c2def2
added validation for `enum` property (#2233)
AlexMesser Feb 26, 2021
a481012
fix #5648
AlexMesser Feb 26, 2021
be6faa2
improved missing "enum" or "enumName" properties validation;
AlexMesser Feb 26, 2021
db03ca2
fix #4897, #6376
AlexMesser Feb 26, 2021
b1e6b74
lint fix;
AlexMesser Feb 26, 2021
b1666ef
fixed falling tests;
AlexMesser Feb 27, 2021
ccfad1e
fixed falling tests;
AlexMesser Feb 27, 2021
621732f
removed .only
AlexMesser Feb 27, 2021
233b7ad
fix #6115
AlexMesser Mar 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/driver/aurora-data-api/AuroraDataApiDriver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,10 @@ export class AuroraDataApiDriver implements Driver {
normalizeDefault(columnMetadata: ColumnMetadata): string | undefined {
const defaultValue = columnMetadata.default;

if (defaultValue === null) {
return undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe be differentiating here between null and undefined? When the user sets the column default explicitly to null perhaps they would want the column generated to also explicitly specify DEFAULT NULL, even if that is probably unnecessary.

It is also already dealt with in places like this anyway:

typeorm/src/driver/mysql/MysqlQueryRunner.ts

Lines 1877 to 1878 in 2fa6231

if (column.default !== undefined && column.default !== null)
c += ` DEFAULT ${column.default}`;

Copy link
Collaborator Author

@AlexMesser AlexMesser Feb 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@nebkat thank you for review!

I've checked MySQL and Postgres databases and they are storing both null and undefined defaults like (NULL) value, therefore there is no difference on database level between null and undefined.

So, we can't differentiate these values during synchronization process that leads to unnecessary update queries.

Copy link
Contributor

Choose a reason for hiding this comment

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

This problem was also brought up somewhat in #7229 (comment) but now we have TypeScript 4.2 with support for the ?? operator. Maybe we could start using ?? null when comparing all the fields to avoid these situations?

Now that I think about it more it might actually make sense to have such a mapping in normalizeDefault but I think it should be if (defaultValue === undefined) return null rather than vice-versa. It's a small change but I think it might save some headaches in a few months when someone else is looking at it: it makes more sense that we are saying "if the column default wasn't defined, we default to null" rather than "if the column was explicitly set to null we change it back to not being defined (and then assume that elsewhere the code will treat undefined as null)".

}

if ((columnMetadata.type === "enum" || columnMetadata.type === "simple-enum") && defaultValue !== undefined) {
return `'${defaultValue}'`;
}
Expand Down
2 changes: 1 addition & 1 deletion src/driver/aurora-data-api/AuroraDataApiQueryRunner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1333,7 +1333,7 @@ export class AuroraDataApiQueryRunner extends BaseQueryRunner implements QueryRu

if (tableColumn.type === "enum" || tableColumn.type === "simple-enum") {
const colType = dbColumn["COLUMN_TYPE"];
const items = colType.substring(colType.indexOf("(") + 1, colType.indexOf(")")).split(",");
const items = colType.substring(colType.indexOf("(") + 1, colType.lastIndexOf(")")).split(",");
tableColumn.enum = (items as string[]).map(item => {
return item.substring(1, item.length - 1);
});
Expand Down
21 changes: 10 additions & 11 deletions src/driver/mysql/MysqlDriver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,15 +582,20 @@ export class MysqlDriver implements Driver {
normalizeDefault(columnMetadata: ColumnMetadata): string | undefined {
const defaultValue = columnMetadata.default;

if ((columnMetadata.type === "enum" || columnMetadata.type === "simple-enum") && defaultValue !== undefined) {
if (defaultValue === null) {
return undefined

} else if (
(columnMetadata.type === "enum"
|| columnMetadata.type === "simple-enum"
|| typeof defaultValue === "string")
&& defaultValue !== undefined) {
return `'${defaultValue}'`;
}

if ((columnMetadata.type === "set") && defaultValue !== undefined) {
} else if ((columnMetadata.type === "set") && defaultValue !== undefined) {
return `'${DateUtils.simpleArrayToString(defaultValue)}'`;
}

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

} else if (typeof defaultValue === "boolean") {
Expand All @@ -599,12 +604,6 @@ export class MysqlDriver implements Driver {
} else if (typeof defaultValue === "function") {
return defaultValue();

} else if (typeof defaultValue === "string") {
return `'${defaultValue}'`;

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

} else {
return defaultValue;
}
Expand Down
4 changes: 2 additions & 2 deletions src/driver/mysql/MysqlQueryRunner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1529,7 +1529,7 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner {

if (tableColumn.type === "enum" || tableColumn.type === "simple-enum" || tableColumn.type === "set") {
const colType = dbColumn["COLUMN_TYPE"];
const items = colType.substring(colType.indexOf("(") + 1, colType.indexOf(")")).split(",");
const items = colType.substring(colType.indexOf("(") + 1, colType.lastIndexOf(")")).split(",");
tableColumn.enum = (items as string[]).map(item => {
return item.substring(1, item.length - 1);
});
Expand Down Expand Up @@ -1861,7 +1861,7 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner {
const isMariaDb = this.driver.options.type === "mariadb";
if (isMariaDb && column.asExpression && (column.generatedType || "VIRTUAL") === "VIRTUAL") {
// do nothing - MariaDB does not support NULL/NOT NULL expressions for VIRTUAL columns
} else {
} else {
if (!column.isNullable)
c += " NOT NULL";
if (column.isNullable)
Expand Down
48 changes: 31 additions & 17 deletions src/driver/postgres/PostgresDriver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,21 +586,30 @@ export class PostgresDriver implements Driver {

} else if (columnMetadata.type === "enum" || columnMetadata.type === "simple-enum" ) {
if (columnMetadata.isArray) {
if (value === "{}") return [];

// manually convert enum array to array of values (pg does not support, see https://github.com/brianc/node-pg-types/issues/56)
value = value !== "{}" ? (value as string).substr(1, (value as string).length - 2).split(",") : [];
// convert to number if that exists in poosible enum options
value = (value as string).substr(1, (value as string).length - 2).split(",").map(val => {
// replace double quotes from the beginning and from the end
if (val.startsWith(`"`) && val.endsWith(`"`)) val = val.slice(1, -1);
// replace double escaped backslash to single escaped e.g. \\\\ -> \\
val = val.replace(/(\\\\)/g, "\\")
// replace escaped double quotes to non-escaped e.g. \"asd\" -> "asd"
return val.replace(/(\\")/g, '"')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not so good in regex and here can be more elegant solution

});

// convert to number if that exists in possible enum options
value = value.map((val: string) => {
return !isNaN(+val) && columnMetadata.enum!.indexOf(parseInt(val)) >= 0 ? parseInt(val) : val;
});
} else {
// convert to number if that exists in poosible enum options
// convert to number if that exists in possible enum options
value = !isNaN(+value) && columnMetadata.enum!.indexOf(parseInt(value)) >= 0 ? parseInt(value) : value;
}
}

if (columnMetadata.transformer)
value = ApplyValueTransformers.transformFrom(columnMetadata.transformer, value);

return value;
}

Expand Down Expand Up @@ -719,18 +728,22 @@ export class PostgresDriver implements Driver {
/**
* Normalizes "default" value of the column.
*/
normalizeDefault(columnMetadata: ColumnMetadata): string {
normalizeDefault(columnMetadata: ColumnMetadata): string | undefined {
const defaultValue = columnMetadata.default;
if (columnMetadata.isArray && Array.isArray(defaultValue)) {
return `'{${defaultValue.map((val: string) => `${val}`).join(",")}}'`;
}

if ((columnMetadata.type === "enum" || columnMetadata.type === "simple-enum")
&& defaultValue !== undefined) {
return `'${defaultValue}'`;
}
if (defaultValue === null) {
return undefined;

} else if (columnMetadata.isArray && Array.isArray(defaultValue)) {
return `'{${defaultValue.map((val: string) => `${val}`).join(",")}}'`;

if (typeof defaultValue === "number") {
} else if (
(columnMetadata.type === "enum"
|| columnMetadata.type === "simple-enum"
|| typeof defaultValue === "number"
|| typeof defaultValue === "string")
&& defaultValue !== undefined
) {
return `'${defaultValue}'`;

} else if (typeof defaultValue === "boolean") {
Expand All @@ -739,10 +752,7 @@ export class PostgresDriver implements Driver {
} else if (typeof defaultValue === "function") {
return defaultValue();

} else if (typeof defaultValue === "string") {
return `'${defaultValue}'`;

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

} else {
Expand Down Expand Up @@ -867,13 +877,15 @@ export class PostgresDriver implements Driver {
const isColumnChanged = tableColumn.name !== columnMetadata.databaseName
|| tableColumn.type !== this.normalizeType(columnMetadata)
|| tableColumn.length !== columnMetadata.length
|| tableColumn.isArray !== columnMetadata.isArray
|| tableColumn.precision !== columnMetadata.precision
|| (columnMetadata.scale !== undefined && tableColumn.scale !== columnMetadata.scale)
|| 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
|| tableColumn.isUnique !== this.normalizeIsUnique(columnMetadata)
|| tableColumn.enumName !== columnMetadata.enumName
|| (tableColumn.enum && columnMetadata.enum && !OrmUtils.isArraysEqual(tableColumn.enum, columnMetadata.enum.map(val => val + ""))) // enums in postgres are always strings
|| tableColumn.isGenerated !== columnMetadata.isGenerated
|| (tableColumn.spatialFeatureType || "").toLowerCase() !== (columnMetadata.spatialFeatureType || "").toLowerCase()
Expand All @@ -885,9 +897,11 @@ export class PostgresDriver implements Driver {
// console.log("name:", tableColumn.name, columnMetadata.databaseName);
// console.log("type:", tableColumn.type, this.normalizeType(columnMetadata));
// console.log("length:", tableColumn.length, columnMetadata.length);
// console.log("isArray:", tableColumn.isArray, columnMetadata.isArray);
// console.log("precision:", tableColumn.precision, columnMetadata.precision);
// console.log("scale:", tableColumn.scale, columnMetadata.scale);
// console.log("comment:", tableColumn.comment, columnMetadata.comment);
// console.log("enumName:", tableColumn.enumName, columnMetadata.enumName);
// 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);
Expand Down
Loading