-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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: Fix unnecessary migrations for timestamps with default values of CURRENT_TIMESTAMP and precision #7229
Conversation
…recision - Returned the database specific equivalent of CURRENT_TIMESTAMP in normalizeDefault() - Pulled out individual conditions for easier debugging. - Intentionally used '!=' instead of '!==' for the precision comparison, so that undefined and null compare as equal.
…d normalizeDefault() that were useful for debugging.
Actually, I think I got one of the tests wrong ... and likely some production code as a result ... I'll take a look at it soon. |
const lengthCond = tableColumn.length !== columnMetadata.length; | ||
|
||
// Intentionally using '!=' instead of '!==' so that undefined and null compare as equal. | ||
const precisionCond = tableColumn.precision != columnMetadata.precision; |
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.
not sure I really like !=
approach instead of explicit check for null or undefined, because !=
forces me to ask questions:
- what if tableColumn.precision is
0
and columnMetadata.precision isundefined
ornull
? What behaviour should I expect? - why the hell precision can be both
null
orundefined
? In what situations it can benull
and in what situations it can beundefined
? Feels like it should be only one of them when the value isn't defined? Maybe normalization can be applied somewhere else before this check?
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.
same questions can arise in other people heads, and instead of writing the comments maybe it's better to replace this check with explicit one?
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.
If we could bump the TypeScript version a nice check here would be
const precisionCond = tableColumn.precision != columnMetadata.precision; | |
const precisionCond = (tableColumn.precision ?? null) !== (columnMetadata.precision ?? null); |
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 is now possible on latest TypeScript 😀
@@ -483,14 +483,21 @@ export class CockroachDriver implements Driver { | |||
const defaultValue = columnMetadata.default; | |||
const arrayCast = columnMetadata.isArray ? `::${columnMetadata.type}[]` : ""; | |||
|
|||
// console.log(`normalizeDefault() : columnMetadata.propertyName = ${columnMetadata.propertyName}`); | |||
// console.log(`normalizeDefault() : defaultValue = ${defaultValue}`); |
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 probably remove all of the commented out logging lines.
Description of change
After upgrading
typeorm
from0.2.12
to0.2.29
, I started seeingCOMMENT ON COLUMN "tableName"."columnName" IS NULL
statements in both theup
anddown
migrations for columns that had default values but no changes.I saw that issue #3991 looked similar, so I fixed it. I have a separate fix for the comment issue that I will send in another pull request.
Details:
Fixes #3991
Pull-Request Checklist
master
branchnpm run lint
passes with this changenpm run test
passes with this changeFixes #0000