Fix missing attributes in serialization

[tms-91]

Environment

Liquibase Version: 4.7.1+

Liquibase Integration & Version: CLI and Java API

Liquibase Extension(s) & Version: none

Database Vendor & Version: H2 2.1 and MSSQL

Operating System Type & Version: Windows 10

Pull Request Type

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

Description

The ChangeSet attributes "runInTransaction", "runOrder" and "ignore" were never serialized since they were missing in the serializableFields list.

Steps To Reproduce

1. Have a ChangeLog containing a ChangeSet with "runInTransaction", "runOrder" and "ignore" attributes with non default values.
2. Let Liquibase read the Changelog.
3. Export the ChangeLog with the for example the XMLChangeLogSerializer

Actual Behavior

After exporting the ChangeLog again the attributes "runInTransaction", "runOrder" and "ignore" are missing on the ChangeSet.

Expected/Desired Behavior

The ChangeSet retaining the attributes "runInTransaction", "runOrder" and "ignore" after export.

Screenshots (if appropriate)

Additional Context

Fast Track PR Acceptance Checklist:

• Build is successful and all new and existing tests pass
• Added Unit Test(s)
  extended already existing Tests
• Added Integration Test(s)
• Added Test Harness Test(s)
• Documentation Updated

Need Help?

Come chat with us on our discord channel

[nvoxland]

Code Review and test results:

Things to be aware of

• Changes make sense and have good tests added for them
• Liquibase code doesn't use the new behavior, only external callers

Things to worry about

• Nothing

[github-actions]

Unit Test Results

  4 548 files  ±0    4 548 suites  ±0   33m 9s ⏱️ - 4m 18s
  4 508 tests ±0    4 294 ✔️ +4     214 💤  - 4  0 ❌ ±0 
53 376 runs  ±0  48 368 ✔️ +4  5 008 💤  - 4  0 ❌ ±0 

Results for commit 5e3ae9a. ± Comparison against base commit 687baea.

♻️ This comment has been updated with latest results.

[XDelphiGrl]

Hello, @tms-91! Thanks for the PR! Is the reason we do not have a test for the formatted SQL changelog because there is no serializer for the "raw" SQL format?

@nvoxland, hi - Why wouldn't this code be used by the CLI? I am able to add runOrder, ignore and runInTransaction on an XML changeset and successfully validate and update using the changelog. Is it because the CLI does not use XMLChangeLogSerializer to export the changelog?

[wwillard7800]

Looks good.

[nvoxland]

@nvoxland, hi - Why wouldn't this code be used by the CLI? I am able to add runOrder, ignore and runInTransaction on an XML changeset and successfully validate and update using the changelog. Is it because the CLI does not use XMLChangeLogSerializer to export the changelog?

@XDelphiGrl Liquibase uses XMLChangeLogSerializer to generate the XML from constructed changelogs, but we don't set runOrder, ignore, or runInTransaction to the changesets we generate. So there isn't a way from our code (currently) for them to get set.

[XDelphiGrl]

UPDATE

• No need to do any manual testing, @FBurguer or @ecarneiro01; see comments from contributor for the explanation as to why my concerns here do not apply to this PR. Moving to ready to merge & no further action is required of Community QA. Thanks!!

---

(start of original comment)
@nvoxland, Liquibase update-sql and 'update' do use runOrder and ignore in an XML changelog. If you use this changelog:

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


	<changeSet author="liquibase-user" id="runLast" labels="runOrderWorksQuestionMark" runOrder="last">
        <createTable tableName="testtable1">
            <column name="supplier_id" type="numeric(4)"/>
            <column name="supplier_name" type="VARCHAR(50)"/>
        </createTable>
    </changeSet>

	<changeSet author="liquibase-user" id="runFirst" labels="runOrderWorksQuestionMark" runOrder="first">
        <createTable tableName="testtable2">
            <column name="supplier_id" type="numeric(4)"/>
            <column name="supplier_name" type="VARCHAR(50)"/>
        </createTable>
    </changeSet>
	
	<changeSet author="liquibase-user" id="ignoreMe" labels="runOrderWorksQuestionMark" ignore="true">
        <createTable tableName="testtable2">
            <column name="supplier_id" type="numeric(4)"/>
            <column name="supplier_name" type="VARCHAR(50)"/>
        </createTable>
    </changeSet>
</databaseChangeLog>

Then update-sql will show that the second changeset runs first (as set in runOrder) and the first changeset runs last (again, in runOrder). The third changeset is entirely ignored.

When you update, the changes run in the order shown by update-sql:

Running Changeset: runOrder-ignore-changelog.xml::runFirst::liquibase-user
Running Changeset: runOrder-ignore-changelog.xml::runLast::liquibase-user
Liquibase command 'update' was executed successfully.

I don't know how to validate runInTransaction but it could also be the case that the CLI knows/uses that, too.

I'm testing with a feature branch that is (relatively) up-to-date with liquibase/liquibase master. I believe this test should be executed against this PR's build to ensure that there are no regressions (at least for runOrder and ignore). I'm going to remove the autocandidate tag from this ticket.

CC @FBurguer @ecarneiro01 @kataggart

[tms-91]

Hello, @tms-91! Thanks for the PR! Is the reason we do not have a test for the formatted SQL changelog because there is no serializer for the "raw" SQL format?

Hello @XDelphiGrl ! Thanks for reviewing this PR.

From a quick glance at the code it doesn't seem to me that the FormattedSqlChangeLogSerializer uses the getSerializableFields() method of ChangeSet, so there should be no changes in behaviour here. I might have overlooked something though.

There should also be no changes to the parsing (reading of changelogs) since ChangeSet does not extend AbstractLiquibaseSerializable and implements its own load and serialize (stub in case of serialize) methods. The load method of ChangeSet doesnt use the getSerializableFields method either and is unchanged.

An example use case that would be affected (and fixed) by this PR is the conversion between different Changelog file formats by using Liquibase as a library (not cli):

1. let Liquibase read the changelog file and create the Java objects.
2. use the XML/YAML/JSON-Serializers to export the Java objects to the output changelog file

In this case, without this PR, if a changeset had "runInTransaction", "runOrder", or "ignore" set to non-default values, they would be missing in the generated output changeset and treated as if they had default values, which would alter the semantics of the changeset.

[XDelphiGrl]

Hello, @tms-91! Thank you for the excellent explanations in response to my questions. Based on your answers, I will move this ticket to Ready to Merge & you should see your fix in the next release.

It makes my day to hear from someone in the Community - thank you for that!

[XDelphiGrl]

• Changes impact consumers of Liquibase as a library.
  • No impact to the CLI or other integrations.
• Tests added for each of the serializable changelog formats (XML, JSON, YAML).
  • Formatted SQL changelogs are not impacted by this fix.
• @tms-91 is an excellent collaborator. As a QA, I love Devs who take the time to explain the code to me.

APPROVED

Passing Functional Tests
Passing Test Harness Execution

[nvoxland]

Agreed, thank you @tms-91!