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

Upgrade to hibernate-6.4.4 #359

Merged
merged 9 commits into from
Sep 5, 2024
Merged

Upgrade to hibernate-6.4.4 #359

merged 9 commits into from
Sep 5, 2024

Conversation

cies
Copy link
Member

@cies cies commented Mar 25, 2024

Closes #358

This is merely a small PoC showing that it RePlay can compile and pass tests with Hibernate 6.4.

There are loooooads of deprecation warnings and I've ignored them on purpose. Most were already present in with Hibernate 5.6.

The main thing I changed is that we cannot get a connection from a Hibernate session in 6.4, the new way to use the connection of a specific session is with lambdas as explained here. Since I found no way to get the conneciton, I also removed the getConnection() method. This will be missed in our codebase in 4 places in which I can do the same trick with the lambdas. This makes is an API breaking change (if we were to uphold semver it'd need a big version number bump).

While this branch builds and the tests pass (more about failing test in conversation below), I'm not sure it is correct. I've not tested in our codebase (under production load). This branch/PR is for us to discuss, and look deeper into: if we're final on the how to implement this, I'm willing to test it on our staging env and after some time on with our production load.

@cies cies changed the title Make it build, and pass th test Upgrade to hibernate-6.4.4: make it build and pass the tests Mar 25, 2024
@cies
Copy link
Member Author

cies commented Mar 27, 2024

Hibernate 5.6 has a upperbound on op JDK 18 (19 or 20 worked well for us, but 21 did not -- then I reverted back to JDK17 to be sure), Hibernate 6.4 officially only works on JDK 11, 17 or 21 (recent LTSes; probably the in-betweens work as well).

@cies cies changed the title Upgrade to hibernate-6.4.4: make it build and pass the tests Upgrade to hibernate-6.4.4: make it build and pass the non-ui tests Mar 27, 2024
@cies
Copy link
Member Author

cies commented Mar 28, 2024

So the ui-tests fail on liquibaseapp's PetsRegistrationSpec and PetsReportSpec. All other ui-tests tests pass.

The PetsRegistrationSpec fails because (as I understand it now) the re-hydration of the Pet hibernate entity fails (and subsequently the validation of that entity fails).

So far I tracked the problem down to play.data.binding.Binder.internalBindBean where the binding of values to the entity's properties does not work.

In GenericModel line:

ParamNode beanNode = StringUtils.isEmpty(name) ? rootParamNode : rootParamNode.getChild(name, true);

the name is "arg0" while it should have been "pet".

But... This only happened when running from IntelliJ! From the command line the error was different. So I've concluded that somehow the -parameters compile flag is not set when running the ui-tests from IntelliJ.


The error I got from the command line was :

    12:50:53,383 ERROR [play.server.netty3.PlayHandler] ~ Internal Server Error (500) for POST /pet/registration (SQLGrammarException)
    org.hibernate.exception.SQLGrammarException: could not prepare statement [Sequence "PET_SEQ" not found; SQL statement:
    select next value for Pet_SEQ [90036-224]] [select next value for Pet_SEQ]
...
        at play.db.jpa.JPABase._save(JPABase.java:32)
        at play.db.jpa.GenericModel.save(GenericModel.java:241)
        at model.PetRepository.register(PetRepository.java:21)
        at controllers.PetRegistration.register(PetRegistration.java:32)

I'm still looking for way to fix this. So far setting @GeneratedValue(strategy = GenerationType.IDENTITY) on the id field in play.db.jpa.Model did not work and neither did setting MODE=LEGACY in the jdbc url in conf/application.conf.

I've been going about changing the liquibase files in replay-tests/liquibase-app/app/database, while I'm not familiar with Liquibase. So far I've only manage to change the error from "no PET_SEQ sequence" to "PET_SEQ sequence already exists" and back. I give up. Somehow the combination Hibernate 6.4 - Liquibase (we do not use) - H2 (we do not use) does not work with the same code as Hibernate 5.6 when it comes to sequences. Should not be too hard to fix, but so far I've not been able to.

@cies
Copy link
Member Author

cies commented May 22, 2024

Update:

  • Indeed the -parameters flag was missing on my end (properly described in the README -- so my bad)
  • I've renamed the hibernate_sequence to Pet_SEQ in the Liquibade migrations as that is what Hibernate 6+ expects for H2 dbs
  • The increment defaults to 50 (dont ask me why) and since the id field is part of the Model class that I cannot override in the app (or please tell me how) I cannot change it to 1 (what Hibernate defaults to). So I have set the increment to 50 and now the tests pass.

Like I said: we do not use H2 or Liquibase so I'm not too invested in figuring out all the nitty-gritties of that combination of tools.

It'd be great if we can move to Hibernate 6.4 as we like to stay current on the libraries.

@cies cies changed the title Upgrade to hibernate-6.4.4: make it build and pass the non-ui tests Upgrade to hibernate-6.4.4 May 22, 2024
@cies

This comment was marked as outdated.

@cies cies changed the title Upgrade to hibernate-6.4.4 Upgrade to hibernate-6.4.4 [dont merge] May 23, 2024
Signed-off-by: Cies <cies-AT@stager-DOT.nl>
@xabolcs
Copy link
Collaborator

xabolcs commented May 23, 2024

[dont merge]

You can always mark your PR as draft!

@cies cies marked this pull request as draft May 23, 2024 12:47
@cies cies changed the title Upgrade to hibernate-6.4.4 [dont merge] Upgrade to hibernate-6.4.4 May 23, 2024
@cies cies changed the title Upgrade to hibernate-6.4.4 Upgrade to hibernate-6.4.4 [release by itself in 2.5.1 or 2.6.0] May 31, 2024
@cies cies marked this pull request as ready for review May 31, 2024 17:27
@cies
Copy link
Member Author

cies commented Jun 3, 2024

I have successfully used this branch with our codebase. It's currently tested, but I expect no further problems.

To get it to work we had to implement jpa.Model in our codebase. It works with this branch, but it is a little ugle (as jpa.Model get referenced in the RePlay code. I have a separate issue removing all these references and deprecating jpa.Model in favour of everyone using RePlay having to implement their own: #394

The reason we had to implement our own is that Hibernate 6+ changed the way that @GenerationType.AUTO works for MySQL, which is what jpa.Model uses (since it is the @GeneratedValue annotation's default) for the id column. Before Hibernate 6 it was GenerationType.IDENTITY for MySQL, since Hibernate 6 it is GenerationType.TABLE. By implementing our own we could stick to the old behaviour.

@xabolcs
Copy link
Collaborator

xabolcs commented Jun 3, 2024

... I have a separate issue removing all these references and deprecating jpa.Model in favour of everyone using RePlay having to implement their own ...

How about doing it the other way? Exactly as you did in your codebase:

Before Hibernate 6 it was GenerationType.IDENTITY for MySQL, since Hibernate 6 it is GenerationType.TABLE. By implementing our own we could stick to the old behaviour.

Just implement the old behaviour for RePlay Framework, and document in the README if one would like to use the new behavior of Hibernate 6, then copy that jpa.Model and customize for theirself.

This framework isn't supposed for new projects, instead for an intermediate step for leaving Play! 1 Framework.
I think it makes sense to keep the old behavior.

@cies
Copy link
Member Author

cies commented Aug 8, 2024

@xabolcs Hibernate changed the default behaviour and I cannot change it back from RePlay. To be able to change it back you need to override the default behaviour (with annotations) which is why I advocate for dropping jpa.Model so we do not push people towards the defaults, based on which they can get into a difficult situation.

There are more reasons to deprecate jpa.Model, see the PR #394 's issue: #393

@cies cies changed the title Upgrade to hibernate-6.4.4 [release by itself in 2.5.1 or 2.6.0] Upgrade to hibernate-6.4.4 Aug 14, 2024
@cies
Copy link
Member Author

cies commented Sep 3, 2024

We've tested this and are now prod-testing this. I'll likely run this in production next week (2024w37). Once landed on production I'll make a new release of RePlay that includes this branch.

@cies cies merged commit 80d725e into main Sep 5, 2024
3 checks passed
@asolntsev asolntsev added this to the 2.6.0 milestone Sep 5, 2024
@asolntsev asolntsev deleted the upgrade-to-hibernate-6.4 branch September 5, 2024 20:11
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.

Upgrade to Hibernate 6.4
3 participants