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

fix: CompositeClassLoader does not implement getResource(String) #927

Merged
merged 2 commits into from Feb 21, 2020
Merged

fix: CompositeClassLoader does not implement getResource(String) #927

merged 2 commits into from Feb 21, 2020

Conversation

mselerin
Copy link
Contributor

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.

@ProIcons
Copy link

ProIcons commented Nov 8, 2019

Any updates on this?

@bibibiu2017
Copy link

Hi, any updates?

@materemias
Copy link

could you merge this fix please?

Copy link
Contributor

@SteveDonie SteveDonie left a 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.

@materemias
Copy link

materemias commented Nov 28, 2019

@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

@mohamedzaki90
Copy link

any updates/estimation on when this will be merged?

@SteveDonie
Copy link
Contributor

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.

bibibiu2017 referenced this pull request Dec 14, 2019
…ked-hash-map

changing HashMap to LinkedHashMap for deterministic iterations
jgribonvald added a commit to GIP-RECIA/esup-publisher that referenced this pull request Feb 10, 2020
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
@alexis-puska
Copy link

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 ?

@SteveDonie
Copy link
Contributor

We are currently working this PR through our internal processes. Does anyone have repro steps/environment description for how to reproduce this?

@timbru31
Copy link

I can try to extract a MRE (minimum reproducible example) from our codebase tomorrow or early next week.

@ProIcons
Copy link

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

@mselerin
Copy link
Contributor Author

mselerin commented Feb 13, 2020

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 mvn clean compile liquibase:update liquibase:diff to reproduce the problem.

The branch "liquibase-3.6.x" works fine : it first create a FooEntity table and then generate a changelog inside target/xxxxx_changelog.xml with 2 new columns.

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)

@SteveDonie
Copy link
Contributor

@mselerin Thanks so much for the MRE. I have been fighting with node and npm to get a version that would even install jhipster.
I'll try your method now.

@ProIcons
Copy link

ProIcons commented Feb 19, 2020

Just so you can have another example if needed.

>java -version
java version "1.8.0_231"
Java(TM) SE Runtime Environment (build 1.8.0_231-b11)
Java HotSpot(TM) 64-Bit Server VM (build 25.231-b11, mixed mode)
> javac -version
javac 1.8.0_212
>npm -version
6.4.1
>node --version
v10.15.3
>jhipster --version
INFO! Using JHipster version installed globally
6.7.1

Note you need to have an LTS version of Node installed on your computer to use jhipster.
To install jhipster use:

npm install -g generator-jhipster
C:\Users\Nikolas>mkdir exampleproject
C:\Users\Nikolas>cd exampleproject
C:\Users\Nikolas\exampleproject>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)

image

This should complete with this message:
INFO! Congratulations, JHipster execution is complete!

At this point you open pom.xml
And navigate to line: 52, 60, 61 and update versions as needed.
I'm testing this with the following versions:

<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:diff

You also need to navigate to src\main\resources\config open application-dev.yml
and change the database information there aswell.

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
Add the following rule:

"rules": {
    "@typescript-eslint/no-unnecessary-type-assertion": 0
}

As soon as you do that fire up the app with
./mvnw clean compile spring-boot:run
Spring will initialize the database with hibernate and liquibase. And as soon you do that close it.

After that we need to introduce a change to the database so we create a simple entity
with jdl

Write to a file the following jdl code:

entity Image {
    aspectRatio Integer required,
    filePath String required,
    height Integer required,
    width Integer required
}

and then import it with

jhipster import-jdl file --skip-db-changelog

you overwrite everything and then run

./mvnw liquibase:diff

[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  9.052 s
[INFO] Finished at: 2020-02-20T01:19:24+02:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.liquibase:liquibase-maven-plugin:3.8.6:diff (default-cli) on project exampleproject: 
[ERROR] Error setting up or running Liquibase:
[ERROR] javax.persistence.PersistenceException: Unable to resolve persistence unit root URL: class path resource [] cannot be resolved to URL because it does not exist

There's no configuration that will fix this. You can configure it in ways that will give you different other problems though 😋

@ProIcons
Copy link

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

nvoxland added a commit that referenced this pull request Feb 20, 2020
@nvoxland
Copy link
Contributor

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.

@SteveDonie SteveDonie merged commit 586eb88 into liquibase:3.8.x Feb 21, 2020
@SteveDonie
Copy link
Contributor

@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.

SteveDonie pushed a commit that referenced this pull request Feb 21, 2020
@alexis-puska
Copy link

I see that the liquibase maven plugins 3.8.7 version is published, tested and approuved

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

Successfully merging this pull request may close these issues.

None yet

10 participants