-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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.
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
fix: fixed all known enum issues #7419
Changes from 14 commits
5fe13b1
734bd2b
48f1740
3c5f946
009fd39
cc46ecd
6796563
4613f31
94ab29d
bcdc33d
798763d
2c2def2
a481012
be6faa2
db03ca2
b1e6b74
b1666ef
ccfad1e
621732f
233b7ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -586,14 +586,24 @@ 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 | ||||||||||||||||||||||||||
val = val.replace(/(^")|("$)/g, "") | ||||||||||||||||||||||||||
// 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, '"') | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be faster? Probably slightly easier to read. Safer for the start/end quotes because it won't fix a broken
Suggested change
|
||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// 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; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
@@ -719,18 +729,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") { | ||||||||||||||||||||||||||
|
@@ -739,10 +753,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 { | ||||||||||||||||||||||||||
|
@@ -867,13 +878,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() | ||||||||||||||||||||||||||
|
@@ -885,9 +898,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); | ||||||||||||||||||||||||||
|
There was a problem hiding this comment.
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
andundefined
? When the user sets the column default explicitly tonull
perhaps they would want the column generated to also explicitly specifyDEFAULT 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
There was a problem hiding this comment.
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
andundefined
defaults like(NULL)
value, therefore there is no difference on database level betweennull
andundefined
.So, we can't differentiate these values during synchronization process that leads to unnecessary update queries.
There was a problem hiding this comment.
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 beif (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 tonull
" rather than "if the column was explicitly set tonull
we change it back to not being defined (and then assume that elsewhere the code will treatundefined
asnull
)".