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

Integrator not compatible with Hibernate ORM 5.0.1 #96

Closed
IvanRF opened this issue Sep 24, 2015 · 85 comments
Closed

Integrator not compatible with Hibernate ORM 5.0.1 #96

IvanRF opened this issue Sep 24, 2015 · 85 comments
Milestone

Comments

@IvanRF
Copy link

IvanRF commented Sep 24, 2015

The current Liquibase Hibernate Extension (liquibase-hibernate4) is not compatible with Hibernate ORM 5.0.1.

Here is the error report: AbstractMethodError in SessionFactoryImpl

Steve Ebersole said:

That line is attempting to call Integrators. I would assume you have Integrators not intended for use with Hibernate 5.0. That contract did change in 5.0 as discussed in the migration guide.

Also mentioned in Liquibase + Hibernate ORM 5.0

Here is Hibernate 5.0 Migration Guide

@jdubois
Copy link

jdubois commented Oct 13, 2015

Hibernate 5.x support would be great! @nvoxland any idea when work will start on supporting it?

@lluiscanals
Copy link

+1 to Hibernate 5.x support!

@howie
Copy link

howie commented Feb 5, 2016

+1

7 similar comments
@rivergod
Copy link

+1

@zyro23
Copy link

zyro23 commented Feb 18, 2016

+1

@andrejpetras
Copy link

+1

@charliecooper45
Copy link

+1

@siyb
Copy link

siyb commented Apr 7, 2016

+1

@vrudikov
Copy link

vrudikov commented Apr 8, 2016

+1

@kflorian
Copy link

+1

@jean-merelis
Copy link

I started a lab repository based on liquibase-hibernate to support Hibernate 5.x.
https://github.com/jean-merelis/liquibase-hibernate5

@lluiscanals
Copy link

@jean-merelis 👍 🎉

@jdubois
Copy link

jdubois commented Jun 6, 2016

@jean-merelis if you need help, I'm sure the JHipster community would be ready to code/test/debug! Just ping me on https://twitter.com/java_hipster

@cbornet
Copy link

cbornet commented Jul 20, 2016

+1

11 similar comments
@pascalgrimaud
Copy link

+1

@deepu105
Copy link

+1

@alfredorueda
Copy link

+1

@sdoxsee
Copy link

sdoxsee commented Jul 20, 2016

+1

@matlock08
Copy link

+1

@marhan
Copy link

marhan commented Jul 20, 2016

+1

@7eben
Copy link

7eben commented Jul 20, 2016

+1

@sakhtar1979
Copy link

+1

@cdavid15
Copy link

+1

@cicoub13
Copy link

+1

@mptardy
Copy link

mptardy commented Jul 21, 2016

+1

@eddiejaoude
Copy link

+10 😁

@charlvictor
Copy link

+1

1 similar comment
@Ari651
Copy link

Ari651 commented Jul 22, 2016

+1

@henri-tremblay
Copy link
Contributor

I still have one thing to check and I'll be good to go. Hopefully this
week-end

On 13 October 2016 at 15:51, Nathan Voxland notifications@github.com
wrote:

Good to hear. There were a couple other issues reported and/or fixed that
I have pushed to master. Could you pull the new changes and double-check
that everything looks good. When you say that it is, I'll release the new
version.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#96 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABKRM5EvHbU1MgaKzJRapQUiys0GIAXjks5qzou-gaJpZM4GDZOz
.

@henri-tremblay
Copy link
Contributor

Some bad news that need investigation. All this was tested on MySQL.

  • It seems Liquibase fault but between 3.4.2 and 3.5.3, the checksum calculation has changed. Annoying when migrating because you get a checksum error
  • Liquibase-hibernate5 will add the hibernate_sequence in the change log on MySQL when doing the liquibase:diff
  • Using a naming strategy doesn't seem to work (I'm passing the implicit and physical strategy in the URL). I don't get FK_gsfdgsdfs, instead I get FKgsfdgsdfs

I can provide a demo project featuring these issues is needed.

@nvoxland
Copy link
Contributor

If you have an example you could attach, that would be great. It would help me see what the problems are.

@henri-tremblay
Copy link
Contributor

Will do that this afternoon

On 18 October 2016 at 12:25, Nathan Voxland notifications@github.com
wrote:

If you have an example you could attach, that would be great. It would
help me see what the problems are.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#96 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABKRM8ca80-mch009LQJsQfQt4mMZ34eks5q1PL8gaJpZM4GDZOz
.

@henri-tremblay
Copy link
Contributor

Try with this please: https://github.com/henri-tremblay/finaldemo. We can chat if needed.

@nvoxland
Copy link
Contributor

Thanks, that's an impressively nice setup for reproducing the issue. I'll take a look a bit during the upcoming day, but will look more tonight.

@henri-tremblay
Copy link
Contributor

Did you have a look? To prevent loosing time looking at it myself if you
already figured it out.

On 19 October 2016 at 10:05, Nathan Voxland notifications@github.com
wrote:

Thanks, that's an impressively nice setup for reproducing the issue. I'll
take a look a bit during the upcoming day, but will look more tonight.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#96 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABKRMxNYzCmxXRZoKvIz0LLbQM-Qh8LTks5q1iO9gaJpZM4GDZOz
.

@nvoxland
Copy link
Contributor

Sorry I didn't get to it yesterday like I was hoping. Looking now, though.

The checksum issue is related to a fix that went into 3.5.0 that adjusted the checksum logic to no longer include blank lines in csv files used by loadData, to make it more resilient. Unfortunately, that means that anyone that happens to have blank lines in their CSV files will get a validation error because they are now different.

Your csv files have a trailing newline and so are effected.

While I have a checksum version flag that can disable checking during an upgrade, it running without checksum validation can cause unexpected behavior and the version flag is global so making a more seamless upgrade for some would make a less seamless upgrade for others. I ended up leaving the validation error for people with a newline in their csv files in this case.

The work-around is to either run something like "update databasechangelog set md5sum=null where description like "%loadData%" or add a any tag to the problem changeset.

Is the csv files you have with the newline something everyone using jhipster will run into? Or is it relatively unique to you?

@cbornet
Copy link

cbornet commented Oct 21, 2016

Having changes in changelogs when upgrading Jhipster is quite common and you generally need to revert them anyway if you do it for an already deployed app. So I don't think this is a real issue.

@cbornet
Copy link

cbornet commented Oct 21, 2016

Oh, I just understood that it's on the checksum calculation so that's an harder issue. Forget my stupid comment...

@cbornet
Copy link

cbornet commented Oct 21, 2016

The csv files are common to any JHipster app btw

@nvoxland
Copy link
Contributor

Is there a jhipster-level "we are upgrading to a new version" flag where we could update the databasechangelog table to clear out potentially problematic changesets using the update clause I listed above?

@nvoxland
Copy link
Contributor

I pushed a fix for the sequence showing up in the mysql changelog.

The different-format foreign key (no _ in the name) seems to be a change in how the naming strategies are working in hibernate 4.x vs. 5.x. See http://stackoverflow.com/questions/37062675/hibernate-5-1-x-naming-strategy-backward-compatible-with-hibernate-4-x

The test example is using: hibernate.implicit_naming_strategy=org.springframework.boot.orm.jpa.hibernate.SpringImplicitNamingStrategy

hibernate.physical_naming_strategy=org.springframework.boot.orm.jpa.hibernate.SpringPhysicalNamingStrategy

which as far as I can tell are being used correctly.

@henri-tremblay
Copy link
Contributor

Thanks. I'll test the sequence. I'll check for the naming. About the checksum, yes, this is standard Jhipster. But it will also happen to anyone using a csv then. And having a final endline is quite standard.

So I don't know what to think about it. We will still have to upgrade liquibase but I think a fix accepting the final endline would make everyone's life easier.

@henri-tremblay
Copy link
Contributor

henri-tremblay commented Oct 21, 2016

1- I confirm that if there is no EOF endline in the CSV, the checksum is correct

Or course, it still mean that if you had a endline and you migrate, you are doomed (meaning you need update databasechangelog set md5sum=null where description like "%loadData%)

Can't you calculate the checksum the old way when databasechagelogs.liquibase is on a old version?

@henri-tremblay
Copy link
Contributor

2- I also confirm that the hibernate_sequence is not re-created. So that's fixed

@henri-tremblay
Copy link
Contributor

3- Hibernate has changed its naming strategy. I filed an (issue)[https://hibernate.atlassian.net/browse/HHH-11193] to see if that was meant to be. Meanwhile, I have two choices. One is to leave it like that. The other is to add a custom naming strategy in JHipster. Anyhow, there is nothing you can do about it.

@henri-tremblay
Copy link
Contributor

3- Hibernate has changed its naming strategy. I filed an [issue])https://hibernate.atlassian.net/browse/HHH-11193) to see if that was meant to be. Meanwhile, I have two choices. One is to leave it like that. The other is to add a custom naming strategy in JHipster. Anyhow, there is nothing you can do about it.

@henri-tremblay
Copy link
Contributor

Bottomline:
1- A possible liquibase fix but not related to liquibase-hibernate
2- Fixed
3- An hibernate issue

I validate that everything is working perfectly. You can release whenever you want.

@nvoxland
Copy link
Contributor

The whole checksum handling logic is definitely part of my larger changes I'm working on for Liquibase 4 because it isn't flexible enough at this point.

I do have a version as part of the checksum that I can use to say "this old checksum was computed differently, so just ignore it", but it's a global number so it just makes all the changeSets ignored. The other problem is that when I go into "we can't trust the old checksum" mode, then it breaks the runOnChange logic, doesn't catch actual unexpected changed changeSets, and causes some other similar unexpected behavior for users. Because of that I have to be careful about not changing checksum logic if at all possible, but when we end up needing to we have to decide between making it transparent for people whose checksums changed vs. affecting everyone.

In this case, we ended up thinking that when we were adjusting the checksum logic to no longer care about whitespace, the selection of people with CSVs that have empty lines was small enough that it wasn't worth potentially causing problems for everyone else.

If JHipster does have a lot of users with CSV files with a trailing newline, it is probably worth figuring out something to make it more transparent vs. making them add a to problem changeSets and/or running the update statement.

I'm looking to see if I can see a good way to handle it in the liquibase code, but it will take a new liquibase release, not just a new liquibase-hibernate release.

@henri-tremblay
Copy link
Contributor

henri-tremblay commented Oct 22, 2016

I will remove the newlines at EOF for future JHipster versions. But in fact, pretty much everyone use a final newline from what I know. So I'm a bit surprised that nobody complained.

Anyway. When do you think you can release liquibase-hibernate?

@nvoxland
Copy link
Contributor

You don't have to worry about removing the EOF newlines since the liquibase change that caused the problem makes empty newlines now not a part of the checksum. So from now on it doesn't matter if there are newlines at the end or in the middle or anywhere else. Better and more consistent for everyone from now on.

I'll release the new liquibase-hibernate version today.

@nvoxland
Copy link
Contributor

It has now been released as liquibase-hibernate5 3.6

@IvanRF
Copy link
Author

IvanRF commented Oct 24, 2016

@nvoxland thank you very much Nathan! I'm glad you could fix this, great work!

@dpupaza
Copy link

dpupaza commented Oct 24, 2016

@nvoxland awesome work and thanks! Any idea when it will be available on Maven Central?

@deepu105
Copy link

In my opinion if adding/removing newlines to CSV can ease your problem a
bit I think we can do that for Jhipster and advice users who are upgrading
to do the same. Shouldn't be a huge issue.

Thanks & Regards,
Deepu

On Sat, Oct 22, 2016 at 2:55 AM, Nathan Voxland notifications@github.com
wrote:

The whole checksum handling logic is definitely part of my larger changes
I'm working on for Liquibase 4 because it isn't flexible enough at this
point.

I do have a version as part of the checksum that I can use to say "this
old checksum was computed differently, so just ignore it", but it's a
global number so it just makes all the changeSets ignored. The other
problem is that when I go into "we can't trust the old checksum" mode, then
it breaks the runOnChange logic, doesn't catch actual unexpected changed
changeSets, and causes some other similar unexpected behavior for users.
Because of that I have to be careful about not changing checksum logic if
at all possible, but when we end up needing to we have to decide between
making it transparent for people whose checksums changed vs. affecting
everyone.

In this case, we ended up thinking that when we were adjusting the
checksum logic to no longer care about whitespace, the selection of people
with CSVs that have empty lines was small enough that it wasn't worth
potentially causing problems for everyone else.

If JHipster does have a lot of users with CSV files with a trailing
newline, it is probably worth figuring out something to make it more
transparent vs. making them add a to problem changeSets and/or running the
update statement.

I'm looking to see if I can see a good way to handle it in the liquibase
code, but it will take a new liquibase release, not just a new
liquibase-hibernate release.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#96 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlFxsP1YpaHhncKoFCQS65aY1h5AfGks5q2S3CgaJpZM4GDZOz
.

@henri-tremblay
Copy link
Contributor

@deepu105 The thing is that for existing users, it will be too late. Because the current checksum was already calculated with spaces. So in fact @nvoxland is right, removing the spaces now won't help. It was "just in case" for later.

@nvoxland
Copy link
Contributor

It should be working it's way through maven central. I pushed it to sonatype and they mirror it from there.

@henri-tremblay
Copy link
Contributor

It is available. Since yesterday at least.

Le 26 oct. 2016 12:56 AM, "Nathan Voxland" notifications@github.com a
écrit :

It should be working it's way through maven central. I pushed it to
sonatype and they mirror it from there.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#96 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABKRM0NtBnrokMXiuhpDS-8OldH8I4biks5q3t19gaJpZM4GDZOz
.

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

No branches or pull requests