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

Fix ForeignKeyComparator when foreign key table is not known #2565

Merged
merged 7 commits into from
May 19, 2022
Merged

Fix ForeignKeyComparator when foreign key table is not known #2565

merged 7 commits into from
May 19, 2022

Conversation

joserebelo
Copy link
Contributor

@joserebelo joserebelo commented Feb 21, 2022

Environment

Liquibase Version: 887e441 (master branch)

Liquibase Integration & Version: CLI

Database Vendor & Version: PostgreSQL 9.6.24

Operating System Type & Version: N/A

Pull Request Type

  • Bug fix (non-breaking change which fixes an issue.) - closes foreignKeyConstraintExists not working #2389
  • Enhancement/New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

The changes introduced in #2228 did not account for the case where the table in the ForeignKey is not null, but its name is (which is effectively the same). This seems to result in 2 different ForeignKeys being interpreted as the same object, when they in fact are not, which breaks the ForeignKeyConstraintExists when checking by foreignKeyName.

Steps To Reproduce

Setup a sample database:

CREATE TABLE table1 (
    id1 INT PRIMARY KEY
);

CREATE TABLE table2 (
    id1 INT,
    CONSTRAINT fk_1
        FOREIGN KEY (id1)
        REFERENCES table1 (id1)
);

Run the following changelog:

<?xml version="1.1" encoding="UTF-8" standalone="no"?>
<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
                   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                   xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.8.xsd"
                   logicalFilePath="test">

    <changeSet author="joserebelo" id="test-fk">
        <preConditions onFail="MARK_RAN">
            <not>
                <foreignKeyConstraintExists foreignKeyName="fk_1"/>
            </not>
        </preConditions>
        <addForeignKeyConstraint baseColumnNames="id1" baseTableName="table2" constraintName="fk_1"
                                 referencedColumnNames="id1" referencedTableName="table1" validate="true"/>
    </changeSet>
</databaseChangeLog>

Actual Behavior

Starting Liquibase at 22:05:25 (version [local build] #0 built at 2022-02-21 22:03+0000)
Running Changeset: test::test-fk::joserebelo
Unexpected error running Liquibase: Migration failed for change set test::test-fk::joserebelo:
     Reason: liquibase.exception.DatabaseException: ERROR: constraint "fk_1" for relation "table2" already exists [Failed SQL: (0) ALTER TABLE public.table2 ADD CONSTRAINT fk_1 FOREIGN KEY (id1) REFERENCES public.table1 (id1)]
For more information, please use the --logLevel flag

Expected/Desired Behavior

The foreign key should not be created, and the changeSet is set as MARK_RAN:

Starting Liquibase at 22:06:06 (version [local build] #0 built at 2022-02-21 22:05+0000)
Running Changeset: test::test-fk::joserebelo
Liquibase: Update has been successful.
postgres=# select * from public.databasechangelog;
-[ RECORD 1 ]-+----------------------------------------------------------------------------------------------
id            | test-fk
author        | joserebelo
filename      | test
dateexecuted  | 2022-02-21 22:06:07.268835
orderexecuted | 1
exectype      | MARK_RAN
md5sum        | 8:db5f9965f89a09f531746da886ef1fe4
description   | addForeignKeyConstraint baseTableName=table2, constraintName=fk_1, referencedTableName=table1
comments      |
tag           |
liquibase     | DEV
contexts      |
labels        |
deployment_id | 5481167201

Additional Context

Add any other context about the problem here.

Fast Track PR Acceptance Checklist:

Need Help?

Come chat with us on our discord channel

@nvoxland
Copy link
Contributor

I added an integration test for foreignKeyConstraintExists without a tableName

@nvoxland nvoxland changed the base branch from master to 1_9 April 18, 2022 21:49
@nvoxland nvoxland changed the base branch from 1_9 to master April 18, 2022 21:49
@bgrand-Lam
Copy link

Can you release that fix on next release ? I waiting that fix and I was thinking that apear in 4.10.0 ...

@github-actions
Copy link

github-actions bot commented May 18, 2022

Unit Test Results

  4 512 files  ±0    4 512 suites  ±0   34m 10s ⏱️ +22s
  4 419 tests ±0    4 205 ✔️ ±0     214 💤 ±0  0 ±0 
52 308 runs  ±0  47 300 ✔️ ±0  5 008 💤 ±0  0 ±0 

Results for commit cbdc9fd. ± Comparison against base commit 818022c.

♻️ This comment has been updated with latest results.

@nvoxland nvoxland changed the base branch from master to 1_9 May 18, 2022 19:29
@nvoxland nvoxland changed the base branch from 1_9 to master May 18, 2022 19:29
Copy link
Contributor

@XDelphiGrl XDelphiGrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Excellent bug find and description, @joserebelo!
  • Fix is localized and for a targeted issue with foreign keys.
    • Integration test added to check for regressions to this fix.
  • No additional testing required.

APPROVED

Passing Harness Tests
Passing Functional Tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

foreignKeyConstraintExists not working
6 participants