Prevent silent failures when two "databaseChangeLog" tags are present

[danielthegray]

Environment

Liquibase Version: all

Liquibase Integration & Version: all

Liquibase Extension(s) & Version: all

Database Vendor & Version: all

Operating System Type & Version: all

Pull Request Type

• [ ] Bug fix (non-breaking change which fixes an issue.)
• [x] Enhancement/New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Description

If someone makes a mistake and copy/pastes a databaseChangeLog block in, and makes the file have two tags, the SnakeYAML parser silently covers up the first instance with the second one, which will cause the first block of changes to be silently omitted.

This can be very problematic and cause hard-to-debug errors, which is why I have added a small FilterInputStream interceptor to check the stream as it is fed into the YAML parser, for a duplicate tag. Doing it with the FilterInputStream like this prevents the efficiency losses of opening the file twice.

Steps To Reproduce

You can look at the added test; essentially, just create a changelog file which has two "databaseChangeLog" tags.

Actual Behavior

It will silently to the wrong thing (i.e. skip over the first group of changesets).

Expected/Desired Behavior

An error should be thrown to highlight the fact that there are two databaseChangeLog tags/keys.

Fast Track PR Acceptance Checklist:

• [x] Build is successful and all new and existing tests pass
• [x] Added Unit Test(s)
• [ ] Added Integration Test(s)
• [ ] Added Test Harness Test(s)
• [ ] Documentation Updated

Need Help?

Come chat with us on our discord channel

[molivasdat]

Hi @danielthegray Thanks for the improvement in the use of Liquibase. It is much appreciated.

Here's what happens next: A member of the Liquibase team will take a look at your contribution and may suggest:

• Changes based on guidelines for PR submission
• Additional tests (unit and/or integration) and/or Test Harness Tests
• Provide other feedback, like requesting changes.

The PR will be prioritized according to our internal development and testing capacity.

We’ll let you know when it’s ready to move to the next step or if any changes are needed.

[asomov]

@danielthegray SnakeYAML has a property to prohibit the same keys

[daniel-gray-jemmic]

@danielthegray SnakeYAML has a property to prohibit the same keys

Thank you @asomov for the great tip, it greatly simplifies the code! I've pushed a new revision of the branch with conflicts resolved, but the "FOSSA License Compliance and Security Check" is failing. Does something need to be added to the branch code?

[filipelautert]

@daniel-gray-jemmic currently fossa is failing for contributed PR's, don't worry about it.

[MalloD12]

Hi @daniel-gray-jemmic,

I just wanted to let you know today I commented out the line you added on the YamlParser class, plus ignored the test you also added in this PR because this change was breaking the case of having a duplicate sql: property. I created the issue #5963 in order to fine a way to keep using your code change, but without breaking some other case like the one mentioned in that issue.

If you have any ideas or would like to work on it, please feel free to assign it to yourself or ask any questions/concerns you might have.

Thank you and apologies for this, Daniel.

[danielthegray]

Hi, I understand. The first commit I had on this issue was ONLY detecting the duplicate databaseChangeLog, and I changed it to the duplicate flag because of a suggestion in a comment on this thread. Should we try to revert to the previous version instead?

[filipelautert]

Hi, I understand. The first commit I had on this issue was ONLY detecting the duplicate databaseChangeLog, and I changed it to the duplicate flag because of a suggestion in a comment on this thread. Should we try to revert to the previous version instead?

@danielthegray I think this would be a better approach, thanks!