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.

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

Handle the fact that FK names are not always unique. #2228

Merged
merged 4 commits into from Dec 21, 2021

Conversation

nvoxland
Copy link
Contributor

@nvoxland nvoxland commented Nov 22, 2021

Description

Update ForeignKeyComparator logic to work like PrimaryKeyComparitor which already assumes primary key names are only unique within the table name.

Fixes #2227


Dev Handoff Notes (Internal Use)

Links

Testing

Dev Verification

With the change, I get

Missing Foreign Key(s): 
     fk_test_fk(table2[id] -> table1[id])

Test Requirements (Liquibase Internal QA)

You need two Postgres instances to test this fix. Postgres version does not matter; I reproed using a PG 12 and a PG 9.

Setup:

On one PG instance, execute the following SQL:

create table table1 (id int primary key);
create table table2 (id int primary key);
alter table table1 add constraint fk_test_fk FOREIGN KEY (id) references table1(id);
alter table table2 add constraint fk_test_fk FOREIGN KEY (id) references table1(id);

On the second PG instance, execute the following SQL:

create table table1 (id int primary key);
create table table2 (id int primary key);
alter table table1 add constraint fk_test_fk FOREIGN KEY (id) references table1(id);

The difference is the missing foreign key on table2 on the second instance.

Manual Tests

Verify liquibase diff shows a unexpected foreign key fk_test_fk from table2(id) to table1(id).

  • In liquibase.properties, specify url as the first database and referenceUrl as the second database.
  • liquibase diff --schemas public

Verify liquibase diffChangeLog has an ALTER TABLE to drop a foreign key fk_test_fk from table2(id) to table1(id).

  • In liquibase.properties, specify url as the first database and referenceUrl as the second database.
  • liquibase diff-changelog --changelog-file gcl_dropFk.postgres.sql --schemas public
    • The SQL output to the changelog is:
    • ALTER TABLE table2 DROP FOREIGN KEY fk_test_fk;

Verify reversing the order of the diff shows an missing foreign key fk_test_fk from table2(id) to table1(id).

  • In liquibase.properties, specify url as the second database and referenceUrl as the first database.
  • liquibase diff --schemas public

Verify reversing the order of databases during diff-changelog generates a ALTER TABLE to add a foreign key fk_test_fk from table2(id) to table1(id).

  • In liquibase.properties, specify url as the second database and referenceUrl as the first database.
  • liquibase diff-changelog --changelog-file gcl_addFk.postgres.sql --schemas public
    • The SQL output to the changelog is:
    • alter table table2 add constraint fk_test_fk FOREIGN KEY (id) references table1(id);

Verify liquibase update using changelog gcl_dropFK.postgres.sql correctly drops the foreign key from the first database.

(Note: I added this test case because I have a suspicion that if the SQL to drop the FK will throw an error on update because there are two “fk_test_fk” foreign keys on table1. If there is no error during update itself, verify that the correct/expected foreign key is dropped.)

  • In liquibase.properties, specify url as the first database. (In this test referenceUrl is not used).
  • liquibase update --changelog-file gcl_dropFk.postgres.sql
    • Update is successful.
    • Validation directly on the database confirms the foreign key on table1 from table2 is dropped.

Automated Tests

  • None. There are sufficient integration/dev tests for testing foreign keys.

┆Issue is synchronized with this Jira Bug by Unito

@nvoxland nvoxland added this to To Do in Conditioning++ via automation Nov 22, 2021
@nvoxland nvoxland moved this from To Do to Code Review in Conditioning++ Nov 22, 2021
@nvoxland nvoxland moved this from Code Review to In Development in Conditioning++ Nov 23, 2021
@nvoxland nvoxland moved this from In Development to Code Review in Conditioning++ Nov 24, 2021
@suryaaki2 suryaaki2 self-requested a review December 2, 2021 13:22
@suryaaki2 suryaaki2 moved this from Code Review to Ready for Handoff (In JIRA) in Conditioning++ Dec 2, 2021
@nvoxland nvoxland added this to the v4.5.0 milestone Dec 7, 2021
@nvoxland nvoxland closed this Dec 7, 2021
Conditioning++ automation moved this from Ready for Handoff (In JIRA) to Done Dec 7, 2021
@nvoxland nvoxland reopened this Dec 7, 2021
Conditioning++ automation moved this from Done to To Do Dec 7, 2021
@nvoxland nvoxland moved this from To Do to Ready for Handoff (In JIRA) in Conditioning++ Dec 7, 2021
@nvoxland nvoxland removed this from the v4.5.0 milestone Dec 8, 2021
@nvoxland nvoxland merged commit f62273e into master Dec 21, 2021
Conditioning++ automation moved this from Ready for Handoff (In JIRA) to Done Dec 21, 2021
@nvoxland nvoxland deleted the fix-non-unique-fk-name branch December 21, 2021 22:16
@nvoxland nvoxland removed this from Done in Conditioning++ Jan 6, 2022
@nvoxland nvoxland added this to the v4.7.0 milestone Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diff: Foreign keys in different tables but with matching names are seen as the same objects
3 participants