Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not strip "classpath:" when normalizing the path #5894

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

andrus
Copy link

@andrus andrus commented May 8, 2024

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

.. preserving it is the only way to resolve relative paths with a custom ResourceAccessor
@MalloD12 MalloD12 requested a deployment to external May 21, 2024 19:32 — with GitHub Actions Abandoned
@MalloD12
Copy link
Contributor

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.

@MalloD12 MalloD12 self-requested a review May 28, 2024 18:48
@MalloD12 MalloD12 self-assigned this May 28, 2024
@andrus
Copy link
Author

andrus commented May 28, 2024

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
Copy link
Contributor

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
Copy link
Author

andrus commented May 28, 2024

Let me poke around

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Development
Development

Successfully merging this pull request may close these issues.

Path normalization breaks classpath-relative paths with a custom ResourceAccessor
4 participants