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
fix: CompositeClassLoader does not implement getResource(String) #927
Conversation
Any updates on this? |
Hi, any updates? |
could you merge this fix please? |
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.
Looks pretty reasonable. Not sure if there is any way to test this.
@SteveDonie I have built latest 3.8.x with this patch and it resolved the bug (similar to liquibase/liquibase-hibernate#170) I experienced with my setup of spring boot 2.2.1 JPA-s with liquibase-hibernate 3.8 |
any updates/estimation on when this will be merged? |
Datical, the company that is currently maintaining Liquibase, has recently committed more resources to the project. We are currently reviewing all open pull requests and will be making decisions on them in the near future. Thanks for your patience. |
…ked-hash-map changing HashMap to LinkedHashMap for deterministic iterations
liquibase-hibernate5.version = 3.6 not updated to 3.8 due to an error on running `./mvnw compile liquibase:diff` that get this error: "Error setting up or running Liquibase: javax.persistence.PersistenceException: Unable to resolve persistence unit root URL: class path resource [] cannot be resolved to URL because it does not exist" and seems related to these issue: liquibase/liquibase-hibernate#222 and possible PR: liquibase/liquibase#927
i test it on my project, i get the 3.8.X branch, remplace all the class in this commit, the diff goal of liquibase works ! i can make a diff on my project. i see that you have make some change in the master branch for the 4.0.X version. Can you create a fix for this problem ? |
We are currently working this PR through our internal processes. Does anyone have repro steps/environment description for how to reproduce this? |
I can try to extract a MRE (minimum reproducible example) from our codebase tomorrow or early next week. |
Use jhipster and upgrade liquibase to latest. With its default configuration (or update the pom.xml where needed) you will get to the fun part soon as you do mvn liquibase:diff |
I have made a MRE and it's available here : https://github.com/mselerin/liquibase-PR-927 You just need to clone it and run The branch "liquibase-3.6.x" works fine : it first create a The branch "liquibase-3.8.x" fails, unless I use the code inside this PR. (And btw, I've just merge the branch with the upstream just to keep it up-to-date) |
@mselerin Thanks so much for the MRE. I have been fighting with node and npm to get a version that would even install jhipster. |
Just so you can have another example if needed.
Note you need to have an LTS version of Node installed on your computer to use jhipster.
At this point you can select all the defaults except the database you have to chose both on development and production to be mysql (sql anyway) This should complete with this message: At this point you open pom.xml <hibernate.version>5.4.10.Final</hibernate.version>
<liquibase.version>3.8.6</liquibase.version>
<liquibase-hibernate5.version>3.8</liquibase-hibernate5.version>
<hibernate-core.version>5.4.10.Final</hibernate-core.version> Then you navigate to line 621 and change your database information. Edit: i think it's safe to say that you can skip everything else and try mvn liquibase:diffYou also need to navigate to src\main\resources\config open application-dev.yml There's one more step that you need to follow since the newest version of jhipster introduced a bug with tslint In the .eslintrc.json located on the root folder "rules": {
"@typescript-eslint/no-unnecessary-type-assertion": 0
} As soon as you do that fire up the app with After that we need to introduce a change to the database so we create a simple entity Write to a file the following jdl code:
and then import it with
you overwrite everything and then run
There's no configuration that will fix this. You can configure it in ways that will give you different other problems though 😋 |
Also i think there's no need to import anything at all since it won't run liquibase at all. So you can just skip everything after the changes to the pom.xml and just run the liquibase:diff |
The change does include an automatic check of Thread.currentThread().getContextClassLoader() in the CompositeResourceAccessor which is technically not correct. In theory, this class should just be aggregating the resource accessors it was given and not also checking other places. However, the code as-is is a bit messy around when and how these resource accessors get created and who is in charge of setting up "normal" settings like that. We are working on a cleanup of the ResourceAccessor classes in general in master I'll make sure that even if we pull that line out of the corresponding class in master, we include the contextClassLoader correctly. I added a unit test in a local DAT-4145 branch and will get this merged into 3.8.x through there. |
@mselerin Thank you so much for your patience as we work on new processes. This is now merged into the 3.8.x branch and assuming we don't find any issues in final testing will be included in the 3.8.7 release coming out in the next couple of days. |
I see that the liquibase maven plugins 3.8.7 version is published, tested and approuved |
Fix for liquibase/liquibase-hibernate#170
CompositeClassLoader
does not implement the getResource() and getResources() methods : they always return null / empty collection.This raise an exception inside
DefaultPersistenceUnitManager.determineDefaultPersistenceUnitRootUrl()
with the liquibase-hibernate plugin.