Defensively drop constraints during migrations #480
Merged
Conversation
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
chris-kobrzak
requested review from
agnes512,
dhmlau,
emonddr and
jannyHou
as code owners
|
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
When `PostgreSQL.prototype.getDropForeignKeys` gets called with duplicated foreign keys, an invalid `ALTER TABLE` statement gets created. This is because the duplicates are mapped to multiple `DROP CONSTRAINT fk_key` SQL actions, e.g.: ```sql -- This will throw an error ALTER TABLE "public"."foo" DROP CONSTRAINT "fk_duplicatedKey", DROP CONSTRAINT "fk_duplicatedKey", DROP CONSTRAINT "fk_duplicatedKey", DROP CONSTRAINT "fk_otherKey" ``` As the `DROP CONSTRAINT` actions are run sequentially, when the first action gets executed by PostgreSQL, the next one (for the same, but already dropped key) will not cause an SQL error thanks to the added IF EXISTS check: ```sql -- This is valid SQL ALTER TABLE "public"."foo" DROP CONSTRAINT IF EXISTS "fk_duplicatedKey", DROP CONSTRAINT IF EXISTS "fk_duplicatedKey", DROP CONSTRAINT IF EXISTS "fk_duplicatedKey", DROP CONSTRAINT IF EXISTS "fk_otherKey" ``` Please note, this is really a hack and we still need to understand why the method in question gets called with duplicated foreign keys in certain conditions. Signed-off-by: Chris Kobrzak <chris.kobrzak@gmail.com>
dhmlau
approved these changes
Jul 13, 2021
Member
dhmlau
left a comment
dhmlau
left a comment
There was a problem hiding this comment.
@chris-kobrzak, thanks for the PR. LGTM.
Contributor
Author
|
Thanks Diana! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
When
PostgreSQL.prototype.getDropForeignKeysgets called with duplicated foreign keys, an invalidALTER TABLEstatement gets created. This is because the duplicates are mapped to multipleDROP CONSTRAINT fk_keySQL actions, e.g.:As the
DROP CONSTRAINTactions are run sequentially, when the first action gets executed by PostgreSQL, the next one (for the same, but already dropped key) will no longer cause an SQL error thanks to the added IF EXISTS check:Please note, this is really a hack that fixes
npm run migrateoperations I have experienced when working on my project. We still need to understand why thegetDropForeignKeysmethod gets called with duplicated foreign keys in certain conditions but I believe ensuring the generated SQL code is valid is still a step in the right direction.Checklist
npm testpasses on your machine (with the exception of 1-2 tests that keep timing out both on the upstream and my fork master branches)