fix: Fix unnecessary migrations for timestamps with default values of CURRENT_TIMESTAMP and precision#7229
Closed
RohanTalip wants to merge 2 commits intotypeorm:masterfrom
Closed
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.
RohanTalip
changed the title
Contributor
Author
|
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. |
pleerock
reviewed
Jan 4, 2021
| const lengthCond = tableColumn.length !== columnMetadata.length; | ||
|
|
||
| // Intentionally using '!=' instead of '!==' so that undefined and null compare as equal. | ||
| const precisionCond = tableColumn.precision != columnMetadata.precision; |
Member
There was a problem hiding this comment.
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
0and columnMetadata.precision isundefinedornull? What behaviour should I expect? - why the hell precision can be both
nullorundefined? In what situations it can benulland 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?
Member
There was a problem hiding this comment.
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?
Contributor
There was a problem hiding this comment.
If we could bump the TypeScript version a nice check here would be
Suggested change
| const precisionCond = tableColumn.precision != columnMetadata.precision; | |
| const precisionCond = (tableColumn.precision ?? null) !== (columnMetadata.precision ?? null); |
Contributor
There was a problem hiding this comment.
This is now possible on latest TypeScript 😀
nebkat
reviewed
Jan 4, 2021
| const arrayCast = columnMetadata.isArray ? `::${columnMetadata.type}[]` : ""; | ||
|
|
||
| // console.log(`normalizeDefault() : columnMetadata.propertyName = ${columnMetadata.propertyName}`); | ||
| // console.log(`normalizeDefault() : defaultValue = ${defaultValue}`); |
Contributor
There was a problem hiding this comment.
Should probably remove all of the commented out logging lines.
6 tasks
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of change
After upgrading
typeormfrom0.2.12to0.2.29, I started seeingCOMMENT ON COLUMN "tableName"."columnName" IS NULLstatements in both theupanddownmigrations 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
masterbranchnpm run lintpasses with this changenpm run testpasses with this changeFixes #0000