4.x: Tests fail when upgrading to Hibernate 6.6.23.Final #10441

[jbescos]

Description

Resolves #10441

Hibernate 6.6 cannot attach a removed entity:
https://docs.jboss.org/hibernate/orm/6.6/migration-guide/migration-guide.html#merge-versioned-deleted

[ljnelson]

I want to be careful here; I think it should be possible to merge a detached entity according to the spec.

Specifically, the spec says:

If X is a detached entity, the state of X is copied onto a pre-existing managed entity instance X' of the same identity or a new managed copy X' of X is created.

Then, recently, Hibernate says:

Previously, merging a detached entity resulted in a SQL insert whenever there was no matching row in the database (for example, if the object had been deleted in another transaction). This behavior was unexpected and violated the rules of optimistic locking.

Indeed, merging a detached entity X when there is no "pre-existing managed entity instance X'" should result in an INSERT when everything is said and done since that's how the persistence context works in all other cases. I'm not sure why Hibernate asserts that this is unexpected.

[jbescos]

I want to be careful here; I think it should be possible to merge a detached entity according to the spec.

Specifically, the spec says:

If X is a detached entity, the state of X is copied onto a pre-existing managed entity instance X' of the same identity or a new managed copy X' of X is created.

Then, recently, Hibernate says:

Previously, merging a detached entity resulted in a SQL insert whenever there was no matching row in the database (for example, if the object had been deleted in another transaction). This behavior was unexpected and violated the rules of optimistic locking.

Indeed, merging a detached entity X when there is no "pre-existing managed entity instance X'" should result in an INSERT when everything is said and done since that's how the persistence context works in all other cases. I'm not sure why Hibernate asserts that this is unexpected.

Thanks @ljnelson , does it make sense that you open them an issue to discuss this?.

[jbescos]

Native image fails consistently:
tests/native-image-mp-2-windows

Caused by: org.apache.maven.plugin.MojoExecutionException: Execution of C:\hostedtoolcache\windows\graalvm-jdk-21.0.3_windows-x64_bin\21.0.3\x64\graalvm-jdk-21.0.3+7.1\bin\native-image.cmd @target\tmp\native-image-5266420025302078153.args io.helidon.tests.integration.packaging.mp2.Mp2Main returned non-zero result
    at org.graalvm.buildtools.maven.AbstractNativeImageMojo.buildImage (AbstractNativeImageMojo.java:429)
    at org.graalvm.buildtools.maven.NativeCompileNoForkMojo.execute (NativeCompileNoForkMojo.java:104)

[jbescos]

It is passing now

[Sanne]

@jbescos such metadata should be unnecessary if you use the Hibernate provided GraalVM module?
-https://github.com/hibernate/hibernate-orm/blame/main/hibernate-graalvm/src/main/java/org/hibernate/graalvm/internal/StaticClassLists.java#L68

I created that module to simplify such integrations by other frameworks, please let us know if it's not useful.

[jbescos]

In this file we tell GraalVM what classes should be available for reflection at runtime. I added that because GraalVM told me to do so.

We don't have org.hibernate.orm:hibernate-graalvm as dependency, but I have added it and the test fails, with and without this configuration. It compiles well, but in runtime I am having some ClassNotFoundExceptions.

[Sanne]

In this file we tell GraalVM what classes should be available for reflection at runtime. I added that because GraalVM told me to do so.

I understand, but it makes more sense to have the rules which are specific to a library to be maintained by the library - otherwise you'll need to make changes for each new Hibernate release.
So for that reason I had created the hibernate-graalvm module.

We don't have org.hibernate.orm:hibernate-graalvm as dependency, but I have added it and the test fails, with and without this configuration.

Such features need to be activated

[Sanne]

My concern would be that this is going to fail at runtime. Just hiding / postponing problems.

[jbescos]

We use static enhancement, so bytebuddy related things should not be executed. This args allows me to replace the static fields that are triggering reflection.

[Sanne]

I understand - but the compiler is usually right, it's running a formal proof. So if it's including it, this would suggest that there are some corner cases in which this code is necessary.

[jbescos]

Adding --trace-class-initialization only shows the stack traces I printed here. It starts with ByteBuddyEnhancementContext and ByteBuddyProxyHelper. We cannot know more about why are that static fields loaded.

We already have a lot of GraalVM configuration related to Hibernate here and some other methods substitutions here. I was not part of this, but it seems this integration was not easy.

[jbescos]

Kindly reminder to review this

[ljnelson]

Next on my plate. Given @Sanne's comments and the change in tested behavior we may want to rethink how this all takes place.

[Tomas-Kraus]

I want to be careful here; I think it should be possible to merge a detached entity according to the spec.

Specifically, the spec says:

If X is a detached entity, the state of X is copied onto a pre-existing managed entity instance X' of the same identity or a new managed copy X' of X is created.

Then, recently, Hibernate says:

Previously, merging a detached entity resulted in a SQL insert whenever there was no matching row in the database (for example, if the object had been deleted in another transaction). This behavior was unexpected and violated the rules of optimistic locking.

Indeed, merging a detached entity X when there is no "pre-existing managed entity instance X'" should result in an INSERT when everything is said and done since that's how the persistence context works in all other cases. I'm not sure why Hibernate asserts that this is unexpected.

Well, it's not my bussiness now, but it's not the first time when I saw that Hibernate has issues to follow the spec. I have seen similar issues while merging an entity with Collection mapping in Hibernate 5 and 6.
I filed a bug for that more than 6 months ago and nobody touched it.

[ljnelson]

Hi @Tomas-Kraus, hope you're well. I will keep my opinions to myself. What I'm concerned about here specifically is that it seems that Hibernate does one thing and Eclipselink does another. There is a mechanism in our unit tests to account for this kind of divergence which happens in several other places if I remember right, where in the test you can say, well, if Eclipselink does this one thing and Hibernate does this other thing, pass the test, but at least in the test you can see that you had to account for the divergence.

[Tomas-Kraus]

Hi @ljnelson, I'm fine and my relations with Oracle were always good so I can still at least tell what I know.
So this is what I was reporting: https://hibernate.atlassian.net/browse/HHH-19007 with reference to the spec.
And here is very simple test case: https://github.com/Tomas-Kraus/hibernate-bug-merge
Whole thing is about different approach Hibernate and EclipseLink uses when processing data:

• EclipseLink always clones enities, but always keeps original user's classes
• Hibernate uses proxy, including that PersistentBag collection which causes troubles in my cases

Now back to this issue: In Helidon Data there are few tests skipped with Hibernate.

    @TestSuite.Suite(MySqlSuite.class)
    @Testcontainers(disabledWithoutDocker = true)
    public static class TestBasicRepository extends io.helidon.data.tests.common.TestBasicRepository {

        @Test
        @Override
        public void testSave() {
            LOGGER.log(Level.DEBUG, "Skipped testSave");
            assumeTrue(false);
        }

        @Test
        @Override
        public void testSaveAll() {
            LOGGER.log(Level.DEBUG, "Skipped testSaveAll");
            assumeTrue(false);
        }

This is exactly what you are talking about.

And another problem is caused by the previously mentioned merge on detached entity which, after being prosessed by Hibernate before, contains their PersistentBag as Collection. In this case, making clone of entity structure with Java default collections helps as workaround. And that's why my entity model in the tests, e.g. https://github.com/helidon-io/helidon/blob/main/data/tests/model/src/main/java/io/helidon/data/tests/model/Pokemon.java has clone methods.

Anyway, spec says that merge shall work as UPSERT, i.e. merge state (update) or insert a new record, so you should push on Gavin to fix this. And https://javadoc.io/static/jakarta.persistence/jakarta.persistence-api/3.2.0/jakarta.persistence/jakarta/persistence/EntityManager.html#merge(java.lang.Object) javadoc says the same.

Edit: And that's why I'm sad about Eclipselink, I have never seen issue like this there.

[ljnelson]

Interesting. I was not talking about Helidon Data tests, though, just the stock container-managed JPA tests we have. Again, I would have to go back to refresh my memory but there are a few cases around JTA transactions where Hibernate does one thing and Eclipselink does another. We run the same tests through both providers and there is some mechanism for ensuring that you can at least see that the discrepancy is happening.

I agree with you that the behavior for merge is very well-specified. I'm not yet fully convinced one way or the other whether Hibernate is doing the "wrong" thing now or not. Contracts are hard!