Split calculate-checksum parameter changeSetIdentifier into changeSetPath / changesetId / changeSetAuthor #4463

JulienMa94 · 2023-07-04T08:04:58Z

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)

TypeBug
APIBreakingChanges
newContributors

Description

As described in #3814 the calculate-checksum command was not able to parse arguments which contains :: characters. So the user is not able to use this command in conjuction with changeset Identifiers containing ::.
As suggested in the issue I have split the parameter changeSetIdentifier into three seperate parameters

changeSetPath
changeSetId
changeSetAuthor

Herefor I made changes to the CalculateChecksumCommandStep and introduced the parameters mentioned above. I had to adjust the calculate-checksum API in Main and the Liquibase class.

So the user has to adjust his use of the calculate-checksum command with the new parameters like so.
liquibase calculate-checksum --changeSetPath="changelog.xml" --changeSetId="1::createTable" --changeSetAuthor="Me"

Fixes #3814

Things to be aware of

No Tests were written / adjusted until now
New Contributor

Things to worry about

Additional Context

Manual Test

Run config

Result

liquibase-standard/src/main/java/liquibase/integration/commandline/Main.java

liquibase-standard/src/main/java/liquibase/Liquibase.java

MalloD12 · 2023-07-21T20:46:49Z

liquibase-standard/src/main/java/liquibase/command/core/CalculateChecksumCommandStep.java

        final String changeLogFile = commandScope.getArgumentValue(CHANGELOG_FILE_ARG).replace('\\', '/');
        final Database database = (Database) commandScope.getDependency(Database.class);

-        List<String> parts = validateAndExtractParts(changeSetIdentifier, changeLogFile);
-        Scope.getCurrentScope().getLog(getClass()).info(String.format("Calculating checksum for changeset %s", changeSetIdentifier));
+        Scope.getCurrentScope().getLog(getClass()).info(String.format("Calculating checksum for changeset %s", changeSetId));


I think in this log message we should also include the other two arguments as well (path and author) since the three together compose the unique id of a changeset. Otherwise, there could be cases of having the same id multiple times in a changeset and this message can add a bit of confusion around.

Added both arguments

MalloD12 · 2023-07-21T20:47:01Z

liquibase-standard/src/main/java/liquibase/command/core/CalculateChecksumCommandStep.java

        if (changeSet == null) {
-            throw new LiquibaseException(new IllegalArgumentException("No such changeSet: " + changeSetIdentifier));
+            throw new LiquibaseException(new IllegalArgumentException("No such changeSet: " + changeSetId));


Added both arguments here aswell

MalloD12 · 2023-07-21T21:04:43Z

liquibase-standard/src/main/java/liquibase/integration/commandline/Main.java

@@ -1513,7 +1501,11 @@ protected void doMigration() throws Exception {
                liquibase.clearCheckSums();
                return;
            } else if (COMMANDS.CALCULATE_CHECKSUM.equalsIgnoreCase(command)) {
-                liquibase.calculateCheckSum(commandParams.iterator().next());
+                liquibase.calculateCheckSum(


I think here we should better perform the calculateCheckSum call by the command scope as below to avoid keep using the deprecated method and just leave the Liquibase class methods for those who use liquibase as a library:

CommandResults commandResults = new CommandScope("calculateChecksum") .addArgumentValue(DbUrlConnectionCommandStep.DATABASE_ARG, database) .addArgumentValue(CalculateChecksumCommandStep.CHANGESET_PATH_ARG, changeSetPath) .addArgumentValue(CalculateChecksumCommandStep.CHANGESET_ID_ARG, changeSetId) .addArgumentValue(CalculateChecksumCommandStep.CHANGESET_AUTHOR_ARG, changeSetAuthor) .addArgumentValue(CalculateChecksumCommandStep.CHANGELOG_FILE_ARG, this.changeLogFile) .execute(); return commandResults.getResult(CalculateChecksumCommandStep.CHECKSUM_RESULT);

Added the command scope call

MalloD12 · 2023-07-21T21:08:23Z

liquibase-cli/src/main/java/liquibase/integration/commandline/LiquibaseCommandLine.java

@@ -104,7 +104,7 @@ private void cleanup() {

    public LiquibaseCommandLine() {
        this.legacyPositionalArguments = new HashMap<>();
-        this.legacyPositionalArguments.put("calculatechecksum", CalculateChecksumCommandStep.CHANGESET_IDENTIFIER_ARG.getName());


I think we should return this one to its previous state as well.

Reverted to changeset identifier

MalloD12 · 2023-07-21T21:09:32Z

Hi @JulienMa94,

Thank you for submitting this PR for us, I have left you a few comments. Please have a look at them and please let me know if you have any questions.

Thanks,
Daniel.

JulienMa94 · 2023-08-02T10:35:18Z

Hi @MalloD12,

thx for your review. I will look into your suggested changes as soon I can find some freetime.

Greetings,
Julien

JulienMa94 · 2023-08-15T08:57:49Z

Hi @MalloD12,

I had some time to look into your suggestions and tried to refine the code to your suggestions.

Unfortunately I have some problems with my local project setup and could not test my changes until now.
At the moment I receive a NPE inside the Scope class at line 427

liquibase/liquibase-standard/src/main/java/liquibase/Scope.java

Lines 426 to 431 in e885b06

    
           public MdcObject addMdcValue(String key, String value, boolean removeWhenScopeExits) { 
        
               MdcObject mdcObject = getMdcManager().put(key, value); 
        
               removeMdcObjectWhenScopeExits(removeWhenScopeExits, mdcObject); 
        
               return mdcObject; 
        
           }

I tried to debug this problem but I could not resolve it and I am a bit clueless if it is a problem related to my code changes or to my local run configuration.

Greetings
Julien

MalloD12 · 2023-08-16T14:01:19Z

liquibase-standard/src/main/java/liquibase/command/core/CalculateChecksumCommandStep.java

-                .description("Changeset ID identifier of form filepath::id::author").build();
+                                    .description("The root changelog file").build();
+
+        CHANGESET_IDENTIFIER_ARG = builder.argument("changeSetIdentifier", String.class)


Hi @JulienMa94, I have been reviewing this PR yesterday and talked about it with the team and we have a few more things we might need to change. One of these is right now we are specifying the four arguments (CHANGESET_IDENTIFIER_ARG, CHANGESET_PATH_ARG, CHANGESET_ID_ARG and CHANGESET_AUTHOR_ARG) all defined as required. And I think this could be a bit ambiguous. What one of my team members suggest is we should make the four of them optional and then add a bit of logic in the run() method to control which of these arguments we will use.

Doing the mentioned update will require us to update some of our command definition tests, plus in order to avoid confusion for other users I think we can add some examples like the ones below to command help. We can do this by calling commandDefinition.setHelpFooter() in the adjustCommandDefinition() method.

liquibase calculateCheckSum --changeSetIdentifier myFile::myId::myAuthor

and

liquibase calculatCheckSum --changesetId myId --changesetAuthor myAuthor --changesetPath myPath

If you need any help I'm here to help/assist you as you need, or I can even help doing these changes if you don't have any time constraints.

Thanks,
Daniel.

MalloD12 · 2023-08-16T14:06:01Z

liquibase-standard/src/main/java/liquibase/command/core/CalculateChecksumCommandStep.java

+
+        CHANGESET_IDENTIFIER_ARG = builder.argument("changeSetIdentifier", String.class)
+                                    .required()
+                                    .description("Changeset ID identifier of form filepath::id::author")


A minor thing. Can we use ChangeSet instead of Changeset argument descriptions? In this particular case, I would also remove the ID part as well.

MalloD12 · 2023-08-16T14:14:02Z

Hi @MalloD12,

I had some time to look into your suggestions and tried to refine the code to your suggestions.

Unfortunately I have some problems with my local project setup and could not test my changes until now. At the moment I receive a NPE inside the Scope class at line 427

liquibase/liquibase-standard/src/main/java/liquibase/Scope.java

Lines 426 to 431 in e885b06

public MdcObject addMdcValue(String key, String value, boolean removeWhenScopeExits) {

MdcObject mdcObject = getMdcManager().put(key, value);

removeMdcObjectWhenScopeExits(removeWhenScopeExits, mdcObject);

return mdcObject;

}

I tried to debug this problem but I could not resolve it and I am a bit clueless if it is a problem related to my code changes or to my local run configuration.

Greetings Julien

I haven't faced this particular issue previously, but a team member told me he did and it got fixed for him by rebuilding the project.

Thanks,
Daniel.

JulienMa94 · 2023-08-16T15:12:50Z

Will try to rebuild it 👍🏽

JulienMa94 · 2023-08-18T08:38:42Z

Hi @MalloD12,

I have implemented your suggestions and made the following parameters optional

changeSetIdentifier
changeSetPath
changeSetId
changeSetAuthor

Inside the run method there is a case distinction if a single parameter (changeSetIdentifier) and multiple parameters (changeSetPath, changeSetid, changeSetAuthor) are passed by the user.
The changeSetIdentifier follows the orginial pattern myFile::myId::myAuthor.
I have added a help text to inform the user that there are two options for the Calculate Checksum command.

Manual Tests:

Single parameter config:

Single parameter result:

Multiple parameters config:

Multiple parameters result:

I have not found any test to adjust. Let me know if there are any adjustments to the code or any test which have to be made.

Greetings,
Julien.

JulienMa94 · 2023-08-18T08:39:53Z

Hi @MalloD12,
I had some time to look into your suggestions and tried to refine the code to your suggestions.
Unfortunately I have some problems with my local project setup and could not test my changes until now. At the moment I receive a NPE inside the Scope class at line 427

liquibase/liquibase-standard/src/main/java/liquibase/Scope.java

Lines 426 to 431 in e885b06

public MdcObject addMdcValue(String key, String value, boolean removeWhenScopeExits) {

MdcObject mdcObject = getMdcManager().put(key, value);

removeMdcObjectWhenScopeExits(removeWhenScopeExits, mdcObject);

return mdcObject;

}

I tried to debug this problem but I could not resolve it and I am a bit clueless if it is a problem related to my code changes or to my local run configuration.
Greetings Julien

I haven't faced this particular issue previously, but a team member told me he did and it got fixed for him by rebuilding the project.

Thanks, Daniel.

Rebuilding did the trick, thx for the hint 👍🏼

liquibase-standard/src/main/java/liquibase/command/core/CalculateChecksumCommandStep.java

MalloD12 · 2023-08-18T22:28:34Z

Hi @JulienMa94,

Thank you for addressing the discussed changes. I reviewed them today and they look good to me. I have made a few minor text updates, plus fixed some command definition tests (as well as added one happy path test by making use of the three new arguments).

We are having a build issue when trying to publish artifacts generated from a contributor branch and the team is already working on it. Before approving this PR I need to hold and wait until that issue is resolved, since once the build is successful some other tests should be triggered. I hope this gets fixed, so we can move forward with this one next week.

Again, thank you for all the effort you have been doing.

Thanks,
Daniel.

JulienMa94 · 2023-08-21T06:23:28Z

Hi @JulienMa94,

Thank you for addressing the discussed changes. I reviewed them today and they look good to me. I have made a few minor text updates, plus fixed some command definition tests (as well as added one happy path test by making use of the three new arguments).

We are having a build issue when trying to publish artifacts generated from a contributor branch and the team is already working on it. Before approving this PR I need to hold and wait until that issue is resolved, since once the build is successful some other tests should be triggered. I hope this gets fixed, so we can move forward with this one next week.

Again, thank you for all the effort you have been doing.

Thanks, Daniel.

Hi @MalloD12,

no problem I will be waiting and hoping that the issues will get fixed soon.
Thank you aswell for the constructive review and the work you did!

Have a nice week,
Julien

JulienMa94 · 2023-09-08T13:13:33Z

Hi @MalloD12 and @rberezen,

I refined the error message.
If the user provides insufficent arguments e.g missing id & path the following message will be printed:

Error encountered while parsing the command line. If --changeset-identifier is not provided than --changeset-id, --changeset-author and --changeset-path must be specified. Missing argument:  '--changeset-author', '--changeset-path'.

Greetings
Julien

MalloD12

Approved.

Thank you, @JulienMa94 for taking care of this PR and addressing all changes we have been requesting you. I very appreciate your efforts.

Review and testing results:

Code changes look good to me. Happy path tests have been added. Which I think complementary with some extra manual test and maybe functional test should be good enough.

Build and most of the tests checks have been successfully executed, except for a MySQL job on the functional tests which will be addressed soon separately (for more details see #4863).

NOTE: PD ticket (internal ticket) to reflect this changes on our docs has been created.

Things to be aware of:

None

Things to worry about:

None

filipelautert · 2023-09-25T16:34:50Z

liquibase-standard/src/main/java/liquibase/command/core/CalculateChecksumCommandStep.java

+
+        final boolean isChangeSetIdentifierPassed = changeSetIdentifier != null;
+
+        validateIdentifierParameters(commandScope, changeSetIdentifier);


@JulienMa94 I extracted the parameters validation to a new method so it's easier to spot this change.

…tPath / changesetId / changeSetAuthor

…o calculdate checksum command

- Minor text updates changing changeset to changeSet.

…nt ones. - Update calculate checksum definition test.

JulienMa94 requested a review from filipelautert as a code owner July 4, 2023 08:04

github-advanced-security bot found potential problems Jul 4, 2023

View reviewed changes

liquibase-standard/src/main/java/liquibase/integration/commandline/Main.java Fixed Show fixed Hide fixed

kevin-atx added TypeEnhancement needs_guidance labels Jul 5, 2023

MalloD12 self-assigned this Jul 21, 2023

MalloD12 self-requested a review July 21, 2023 13:28

MalloD12 reviewed Jul 21, 2023

View reviewed changes

liquibase-standard/src/main/java/liquibase/Liquibase.java Show resolved Hide resolved

MalloD12 reviewed Jul 21, 2023

View reviewed changes

MalloD12 added the sprint2023-56 label Jul 21, 2023

MalloD12 reviewed Aug 16, 2023

View reviewed changes

MalloD12 added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed needs_guidance labels Aug 18, 2023

github-advanced-security bot found potential problems Aug 18, 2023

View reviewed changes

liquibase-standard/src/main/java/liquibase/command/core/CalculateChecksumCommandStep.java Fixed Show fixed Hide fixed

MalloD12 changed the title ~~Fixes #3814 Split calculate-checksum parameter changeSetIdentifier into changeSetPath / changesetId / changeSetAuthor~~ Split calculate-checksum parameter changeSetIdentifier into changeSetPath / changesetId / changeSetAuthor Aug 18, 2023

MalloD12 added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Aug 18, 2023

MalloD12 removed the SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions label Aug 21, 2023

MalloD12 added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Sep 8, 2023

MalloD12 approved these changes Sep 8, 2023

View reviewed changes

filipelautert requested review from rberezen and suryaaki2 September 19, 2023 12:17

suryaaki2 approved these changes Sep 20, 2023

View reviewed changes

rberezen approved these changes Sep 20, 2023

View reviewed changes

filipelautert approved these changes Sep 25, 2023

View reviewed changes

filipelautert force-pushed the ISSUE-3814 branch from 3dd4b95 to 0278603 Compare September 25, 2023 16:35

filipelautert added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Sep 25, 2023

kevin-atx added DocNeeded BreakingChange labels Oct 2, 2023

JulienMa94 and others added 10 commits October 9, 2023 13:03

Spread calculate-checksum parameter changeSetIdentifier into changeSe…

6a36a95

…tPath / changesetId / changeSetAuthor

Refined code according to pr suggestions

ad834cf

ISSUE-3814: Added possibility to pass single or multiple parameters t…

4bd1bc4

…o calculdate checksum command

ISSUE-3814: Fixed some grammar

9dcfe8a

- Fixed calcula-checksum command test.

740a0fc

- Minor text updates changing changeset to changeSet.

CodeQL Change: updated variable from Boolean to boolean type.

8fe94d6

- Calculate checksum arguments renamed to keep consitency with existe…

c3f9622

…nt ones. - Update calculate checksum definition test.

ISSUE-3814: Added suggested error messages

9aef2f6

ISSUE-3814: Refined error message

a058ff3

Validation refactoring

97017c5

filipelautert force-pushed the ISSUE-3814 branch from 0278603 to 97017c5 Compare October 9, 2023 16:03

filipelautert added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Oct 9, 2023

filipelautert merged commit b91ae06 into liquibase:master Oct 9, 2023
15 of 18 checks passed

filipelautert added this to the 1NEXT milestone Oct 9, 2023

joschi mentioned this pull request Nov 15, 2023

Regression in Liquibase 4.25.0: Liquibase#calculateCheckSum() always fails #5225

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split calculate-checksum parameter changeSetIdentifier into changeSetPath / changesetId / changeSetAuthor #4463

Split calculate-checksum parameter changeSetIdentifier into changeSetPath / changesetId / changeSetAuthor #4463

JulienMa94 commented Jul 4, 2023 •

edited by MalloD12

MalloD12 Jul 21, 2023

JulienMa94 Aug 15, 2023

MalloD12 Jul 21, 2023

JulienMa94 Aug 15, 2023

MalloD12 Jul 21, 2023

JulienMa94 Aug 15, 2023

MalloD12 Jul 21, 2023

JulienMa94 Aug 15, 2023

MalloD12 commented Jul 21, 2023

JulienMa94 commented Aug 2, 2023 •

edited

JulienMa94 commented Aug 15, 2023

MalloD12 Aug 16, 2023

MalloD12 Aug 16, 2023

MalloD12 commented Aug 16, 2023

JulienMa94 commented Aug 16, 2023

JulienMa94 commented Aug 18, 2023 •

edited

JulienMa94 commented Aug 18, 2023

MalloD12 commented Aug 18, 2023

JulienMa94 commented Aug 21, 2023

JulienMa94 commented Sep 8, 2023

MalloD12 left a comment

filipelautert Sep 25, 2023


		final boolean isChangeSetIdentifierPassed = changeSetIdentifier != null;

		validateIdentifierParameters(commandScope, changeSetIdentifier);

Split calculate-checksum parameter changeSetIdentifier into changeSetPath / changesetId / changeSetAuthor #4463

Split calculate-checksum parameter changeSetIdentifier into changeSetPath / changesetId / changeSetAuthor #4463

Conversation

JulienMa94 commented Jul 4, 2023 • edited by MalloD12

Impact

Description

Things to be aware of

Things to worry about

Additional Context

Manual Test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MalloD12 commented Jul 21, 2023

JulienMa94 commented Aug 2, 2023 • edited

JulienMa94 commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MalloD12 commented Aug 16, 2023

JulienMa94 commented Aug 16, 2023

JulienMa94 commented Aug 18, 2023 • edited

JulienMa94 commented Aug 18, 2023

MalloD12 commented Aug 18, 2023

JulienMa94 commented Aug 21, 2023

JulienMa94 commented Sep 8, 2023

MalloD12 left a comment

Choose a reason for hiding this comment

Review and testing results:

Things to be aware of:

Things to worry about:

Choose a reason for hiding this comment

JulienMa94 commented Jul 4, 2023 •

edited by MalloD12

JulienMa94 commented Aug 2, 2023 •

edited

JulienMa94 commented Aug 18, 2023 •

edited