Proposed fix for #5442 - command "unexpectedChangesets" always reports at least 1 unexpected changeset with initial tag in database

[Tylorjg]

Impact

• Bug fix (non-breaking change which fixes expected existing functionality)
• Enhancement/New feature (adds functionality without impacting existing logic)
• Breaking change (fix or feature that would cause existing functionality to change)

Description

Issue #5442 outlines an odd behavior where the unexpected-changesets command shows the tag changeset as unexpected. If using the tag command itself, a changeset is not added to the changelog, but an internal Liquibase changeset will be added to the DBCL table. This would cause the unexpected-changesets to always show the internal Liquibase changeset as unexpected. Side note, the generate-changelog does not create the changeset in a changelog if it exists in the DBCL, so there isn't a way to make the tag expected.

This fix adds a function that is called first to determine if the file path is the liquibase-internal changeset. If it is, it removes it from the unexpected list.

Fixes #5442

Things to be aware of

I couldn't find a way to capture the file path for the ran changeset in the RanChangesSet.java file, so I did the same call that occurs in the isSameAs function (occurs in the isSamePath function technically), so it could be refactored to be more efficient.

Things to worry about

There isn't any additional tests associated with the change. I have found that there is a file unexpectedChangesets.test.groovy, but was unsure how to get the tag command to execute first. I would be happy to write test cases/modify the existing ones.

Additional Context

N/A

[MalloD12]

Hi @Tylorjg,

Thank you for adding submitting this PR to fix #5442. I was thinking maybe we can only update isSameAs() by also checking whether filepath is equals to "liquibase-internal" . Something like this:

public boolean isSameAs(ChangeSet changeSet) {
        return isSamePath("liquibase-internal") || (
                this.getId().equalsIgnoreCase(changeSet.getId())
                && this.getAuthor().equalsIgnoreCase(changeSet.getAuthor())
                && isSamePath(changeSet.getFilePath()));
    }

Then we don't need to create a new method nor update ExpectedChangesVisitor class.

Regarding tests, I think it would be nice if we can add some integration test here by creating a new class (UnexpectedChangesetsCommandStepTest), for adding this test you can also use CommandUtil test class where you have a runTag() and probably you can also add a runUnexpectedChangeset() to validate the given issue.

Please let me know if you have any question/concerns.

Thanks,
Daniel.

[MalloD12]

Approved.

Thanks @Tylorjg for submitting this PR and addressing my review comment. I have manually tested this change, plus also adding a basic integration test to validate your fix against MySQL in this case, but it could against any DB.

Build and test checks have been successfully executed (there are some functional test failing, but that doesn't seem to be related with this change.

[rberezen]

@Tylorjg @filipelautert @MalloD12
The fix introduces a regression. So, if you execute the following commands:

1. liquibase tag tag1
2. liquibase update
3. liquibase history

you will observe that it appears as though nothing was deployed; the databasechangelog will be empty, but the deployed objects will exist in the database.

[MalloD12]

@Tylorjg would you mind looking at the regression issue @rberezen mentioned above?

Thanks,
Daniel.

[MalloD12]

Code changes look good to me, thank you for taking care of this regression issue @Tylorjg.

@rberezen would you mind trying again when you have a chance?

Thanks,
Daniel.

[rberezen]

Thank you, @Tylorjg, for your contribution!
@MalloD12, I've manually tested and verified the test results - everything looks good!