-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
base: master
Are you sure you want to change the base?
Conversation
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 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, |
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). |
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, |
Let me poke around |
f2e94aa
to
d32a3e9
Compare
d32a3e9
to
94950ca
Compare
@MalloD12, I updated the PR. Now it has two commits:
Hope this helps with that property implementation. |
@@ -1071,10 +1071,6 @@ public static String normalizePath(String filePath) { | |||
return null; | |||
} | |||
|
|||
if (filePath.startsWith("classpath:")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave this code as it is for now.
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
||
|
||
public class UrlDrivenResourceAccessorTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please move this test class to a separate file?
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 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, |
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. |
94950ca
to
75d0b45
Compare
Just updated the PR:
|
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 creatingDatabaseChangeLog
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