do not strip "classpath:" when normalizing the path

[andrus]

Impact

This may or may not be a breaking change.

Description

This PR addresses the issue #5878 . It ensures that path "normalization" never strips the classpath: prefix when creating DatabaseChangeLog for child resources. The solution is submitted per @tati-qalified advise.

Things to be aware of

The altered method is static and is widely used across Liquibase. This may or may not be a breaking change.

Things to worry about

I don't fully understand the consequences of NOT stripping classpath: prefix when resolving relative paths for other implementations of ResourceAccessor. Quite possibly some of them will need to be updated as well. But it seems logical: in a hierarchy of resources, child resource resolution should follow the same algorithm as the root resource.

Additional Context

See #5878

[MalloD12]

Hi @andrus,

How are you doing? We very much appreciate you trying to fix #5878 and create this PR for it, but we can't merge it as it is. This PR is breaking some tests, but besides that, I have been discussing it with the team and we think this is a breaking change and we don't want to change the current behavior.

I think what we can do is define a new global configuration (you can have a look at src/main/java/liquibase/GlobalConfiguration.java to see some existent examples) and depending on the value set for this configuration then we can decide to strip/don't strip classpath: from the path. I think this way we would prevent breaking somebody else code.

If you have any questions/concerns about it please let us know and me or someone else from the team would be keen to help with you. Last thing, if you are keen to do address these modifications could you please add some unit tests to test the code changes made.

Thanks, Daniel.

[andrus]

Hi @MalloD12 , I suspected we can't just change the policy globally. The suggested idea of doing it conditionally via GlobalConfiguration property should work. Not sure how hard it will be to write a test on the LB site, but I will definitely be happy to test it on Bootique side (and I already have unit tests there).

[MalloD12]

You can try adding the tests also in Liquibase repo and I can help with it, or you can share the scenarios you would like to add and I can assist you on adding them.

Thanks, Daniel.

[andrus]

Let me poke around

[andrus]

@MalloD12, I updated the PR. Now it has two commits:

• c0d203b91310c610c9b5c880995fe4c72fc2d5a5 - a unit test that shows how relative paths do not work for a certain type of dynamic ResourceAccessors
• 94950cad34921f62bf1d5940b6f9e635e1148e76 - my original "fix" that will be replaced with GlobalConfiguration property as you suggested.

Hope this helps with that property implementation.

[MalloD12]

Hi @andrus,

I've been looking at this PR and it's still in a not mergeable state. By removing:

if (filePath.startsWith("classpath:")) {
    filePath = filePath.substring("classpath:".length());
}

will change and break the current behavior which is what we are trying to avoid at the moment. I think it is a reasonable approach to create a new resource accessor class as you are doing and handling there the decision to according the global configuration we were discussing previously decide whether we want to remove or not the classpath: prefix from the path when normalizing.

The global configuration I was talking about would be something like:

INCLUDE_CLASSPATH_PREFIX = builder.define("includeClasspathPrefix", Boolean.class)
                .setDescription("If false Liquibase will remove 'classpath:' prefix when normalizing a resource path. By default, path will keep the classpath prefix")
                .setDefaultValue(true)
                .build();

Thanks, Daniel.

[andrus]

Yes, this is what I said in my comment. The second commit in the PR is for information purposes so to speak. Anyways, I will remove it shortly to avoid any confusion.

[andrus]

Just updated the PR:

• Removed the changes from DatabaseChangeLog
• Moved the test ResourceAccessor into a top-level class as requested in the previous review.

[MalloD12]

Hey @andrus,

Are you keen to add the global configuration and make the necessary changes to the new resource accessor class you have already implemented?

Thanks, Daniel.

[andrus]

I can give it a shot. But unfortunately the check can't be done inside a ResourceAccessor (otherwise I wouldn't have a need to patch Liquibase). The check will have to go in the DatabaseChangeLog.normalizePath(..) method, and depending on the config value, it would either strip the classpath: prefix or not. Is that ok?

[andrus]

Hi @MalloD12 , I just updated the PR to conditionally use preserveClasspathPrefixInNormalizedPaths GlobalConfiguration property, with default being the current behavior. Also extended the tests to verify multiple levels of nesting and check the outcome with different values of the above property.

Please let me know what you guys think. Looking forward to inclusion of this in one of the upcoming LB releases, as using LB 3.x has become untenable for the Bootique community. 😅

[andrus]

Hi @MalloD12 , wonder if you had a chance to take a look at this?

[MalloD12]

Hi @andrus,

Thank you for all your updates on this PR. I'll start looking at it tomorrow.

Thanks, Daniel.

[andrus]

@MalloD12 thanks for the review and suggestions! Looks like we need one more approval Who can help with that?

[MalloD12]

Hey @andrus, yeap someone else from the team needs to review this as well. Another Dev and QA team will review it, we are releasing a patch this week, so I would think if everything is ok for the rest of the team this change will be part of our next release (4.30.0).

Thanks, Daniel.

[filipelautert]

@MalloD12 could you create a documentation ticket for this PR?

[MalloD12]

Hi @andrus,

Could you please do us the favor of updating this PR description with the addition and usage of the newly introduced flag, so our docs team can update our docs accordingly?

Thanks, Daniel.

[MalloD12]

PD-5137 has been created for docs update.

Thanks, Daniel.

[andrus]

@MalloD12

Could you please do us the favor of updating this PR description with the addition and usage of the newly introduced flag, so our docs team can update our docs accordingly?

Just added a paragraph to the top description.

[andrus]

Hi folks. Do we need any extra approvals to get this merged?

[filipelautert]

Hi folks. Do we need any extra approvals to get this merged?

@andrus nope, all fine! I'm getting it merged. Thanks for the PR!