Hibernate envers collections initialized on call to getMyCollection

[fabrii]

Describe the bug

Hibernate.isInitialized(proxy) function is not working properly when the entity is returned by envers.

This bug is related to https://github.com/quarkusio/quarkus/issues/17291

Expected behavior

Return true when the object is initialized. Return false when the object is not initialized.

Actual behavior

Returns true always.

How to Reproduce?

Reproducer: https://github.com/fabrii/quarkus-playground/tree/hibernate-envers-isinitialized

Output of uname -a or ver

Linux fabricio-desktop 5.4.0-73-generic #82-Ubuntu SMP Wed Apr 14 17:39:42 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Output of java -version

openjdk version "11.0.11" 2021-04-20 OpenJDK Runtime Environment (build 11.0.11+9-Ubuntu-0ubuntu2.20.04) OpenJDK 64-Bit Server VM (build 11.0.11+9-Ubuntu-0ubuntu2.20.04, mixed mode, sharing)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.5.1.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.6.3 Maven home: /usr/share/maven Java version: 11.0.11, vendor: Ubuntu, runtime: /usr/lib/jvm/java-11-openjdk-amd64 Default locale: es_ES, platform encoding: UTF-8 OS name: "linux", version: "5.4.0-73-generic", arch: "amd64", family: "unix"

Additional information

No response

[quarkus-bot[bot]]

/cc @Sanne, @gsmet, @yrodiere

[gsmet]

/cc @Naros

[Naros]

Hi @gsmet, I'll try to take a look at this as soon as I can. My plate is pretty full this week but I'll do my best to try and look at it perhaps on Friday during DoL.

[fabrii]

Hi!

I was looking the solution for the hibernate normal entities (not envers) in this link: https://github.com/hibernate/hibernate-orm/pull/3994/files

The code used for skipping the snapshot is:

if ( value instanceof PersistentCollection && !( (PersistentCollection) value ).wasInitialized() ) {
	// Cannot take a snapshot of an un-initialized collection.
	return;
}

For retrieving my Envers items I use the following code:

AuditReader reader = AuditReaderFactory.get(em);
        List<Object[]> resultados = reader.createQuery().forRevisionsOfEntity(clase, false, true)
                .add(AuditEntity.id().eq(id))
                .addOrder(AuditEntity.revisionNumber().desc())
                .setMaxResults(maxResults)
                .setFirstResult(first)
                .getResultList();
        List<RevHistorico> respuesta = new ArrayList<>();
        for (Object[] r : resultados) {
            RevHistorico rh = new RevHistorico();
            rh.setObjPk(id);
            rh.setObj(r[0]);
            rh.setRevEntity((RevEntity) r[1]);
            rh.setRevType((RevisionType) r[2]);
            respuesta.add(rh);
        }

        //code to check the collection class
        for (RevHistorico r : respuesta) {
            if (r.getObj() instanceof SgAmbito) {
                SgAmbito amb = (SgAmbito) r.getObj();
                if (amb.getSistemas() instanceof Collection) {
                    LOGGER.log(Level.SEVERE, "IS Collection");
                }
                if (amb.getSistemas() instanceof PersistentCollection) {
                    LOGGER.log(Level.SEVERE, "IS PersistentCollection");
                }
            }
        }

amb.getSistemas() is a List. When running this code, I can see the "IS Collection" log, but I don't see the IS PersistentCollection log.

Envers collections are not PersistentCollection ?

[fabrii]

I ran the Hibernate.isInitialized test (on envers entities) in the 5.6, 5.2 and 5.0 branches. It is not working in any of those. Envers uses a ListProxy class instead of PersistentCollection, so it is ignored by the "isInitialized" method. The ListProxy class does not have any method to check if it is initialized or not. This comment is a follow up to this discussion: (https://github.com/quarkusio/quarkus/discussions/27657)

Update: There is a checkInit method on CollectionProxy here, MapProxy, etc. There is also a "delegate" object. Can we assume that if delegate == null, then initialized is false?. If this is true, we can implement a LazyInitializable interface as suggested by @yrodiere.

I am not quite sure how to handle Hibernate.isInitialized on *ToOne attributes yet.

[fabrii]

With *ToOne attributes is another story:

Given:

@Entity
@Audited
public class MultipleEntity {

	@Id
	@GeneratedValue(strategy = GenerationType.IDENTITY)
	@Column(name = "ID", length = 10)
	private Long id;

        @ManyToOne
        private MultipleRefEntity refEntity;

}

and this test:

EntityManager em = getEntityManager();

AuditReader reader = AuditReaderFactory.get(em);
List<MultipleEntity> response = reader.createQuery().forEntitiesAtRevision(MultipleEntity.class, 1)
                .add(AuditEntity.id().eq(id))
                .getResultList();
if (response != null && response.size() > 0) {
      MultipleEntity ret = response.get(0);
     assertEquals(Hibernate.isInitialized(ret.getRefEntity()), false);
}

When running the test in hibernate-orm 5.6 branch, it works correctly. When doing ret.getRefEntity().getClass().getCanonicalName(), I am getting org.hibernate.envers.test.entities.collection.MultipleRefEntity$HibernateProxy$sP2hnRhO

When running the test in the last release of Quarkus, it does not work. When doing ret.getRefEntity().getClass().getCanonicalName(), I am getting mypackage.MultipleRefEntity

To sum up, I think there are two different issues:

1. With Envers collections we need to handle the isInitialized check at hibernate-orm level.
2. With Envers *ToOne attributes, I believe there is a bug in Quarkus.

[yrodiere]

When running the test in hibernate-orm 5.6 branch, it works correctly. When doing ret.getRefEntity().getClass().getCanonicalName(), I am getting org.hibernate.envers.test.entities.collection.MultipleRefEntity$HibernateProxy$sP2hnRhO

~Where are you getting that exactly? I don't see you calling toString in this test.~ Ok, I should grab a coffee before I write something. I understand now.

Also, I'm a bit surprised that you expect the association to be lazy, since it's a @ManyToOne, and those are eager by default... ? (See javax.persistence.ManyToOne#fetch)

Note I'm not saying there's no bug; just trying to clarify the expectations and what you're seing.

With Envers *ToOne attributes, I believe there is a bug in Quarkus.

Careful about that, it might still be (and often is) a bug in the Bytecode Enhancement feature of Hibernate ORM. Quarkus enables that by default, while Hibernate ORM disables it by default.

[yrodiere]

When running the test in hibernate-orm 5.6 branch, it works correctly. When doing ret.getRefEntity().getClass().getCanonicalName(), I am getting org.hibernate.envers.test.entities.collection.MultipleRefEntity$HibernateProxy$sP2hnRhO

When running the test in the last release of Quarkus, it does not work. When doing ret.getRefEntity().getClass().getCanonicalName(), I am getting mypackage.MultipleRefEntity

This difference is actually meaningless, since Quarkus relies on bytecode enhancement for proxies, so you're unlikely to ever see a HibernateProxy class in Quarkus (except in very specific cases). An instance of mypackage.MultipleRefEntity could very well be an uninitialized proxy.

[Sanne]

Right, be careful about expecing a particular class to be used to model a relation. Tests should cover semantics, implementation details aren't guaranteed to be stable and would depend on various factors - such as the relation being nullable, type of enhancement being applied, etc..

[fabrii]

Thanks for the feedback.

Is there a way to run the hibernate-orm tests with Bytecode Enhancement enabled?

Also, I'm a bit surprised that you expect the association to be lazy, since it's a @ManyToOne, and those are eager by default... ? (See javax.persistence.ManyToOne#fetch)

I know that *ToOne associations are eager in JPA, but that was not the case with Envers. As you can see in my hibernate-orm test, Hibernate.isInitialized returned false for a ManyToOne association, and the object had the HibernateProxy class.

See adamw message in https://developer.jboss.org/message/628530

I don't know if there is a specific reason behind this. The first thing that occurs to me is that it may be because loading the history of an entity in a given revision with many relationships may not be very performant. Maybe there is some other technical reason that I don't know about.

[yrodiere]

Is there a way to run the hibernate-orm tests with Bytecode Enhancement enabled?

All of them? Not that I know. There are dedicated tests though, those that use @RunWith(BytecodeEnhancerRunner.class).

[yrodiere]

Also, I'm a bit surprised that you expect the association to be lazy, since it's a @manytoone, and those are eager by default... ? (See javax.persistence.ManyToOne#fetch)

I know that *ToOne associations are eager in JPA, but that was not the case with Envers. As you can see in my hibernate-orm test, Hibernate.isInitialized returned false for a ManyToOne association, and the object had the HibernateProxy class.

See adamw message in https://developer.jboss.org/message/628530

I don't know if there is a specific reason behind this. The first thing that occurs to me is that it may be because loading the history of an entity in a given revision with many relationships may not be very performant. Maybe there is some technical reason that I don't know about.

Interesting. If that's on purpose, this could indeed be an unintended regression when bytecode enhancement is enabled.

[Sanne]

Adam only said "all relations are lazy", but I don't think that implied _ToOne relations as well as that's not technically possible unless they are marked as non-nullable. We can't convert a lazy proxy to "null" on initialization if we discover the _one element it's pointing to didn't actually exist.

[yrodiere]

We can't convert a lazy proxy to "null" on initialization if we discover the _one element it's pointing to didn't actually exist.

Right. Though that's only a problem for:

• @OneToOne or @ManyToOne using an association table; that generally means a legacy application...
• Some exotic configurations without foreign key constraints or where a non-null foreign key may mean "no association"; that's even more legacy...
• The non-owning side of a @OneToOne when optional = true; that can happen even in recent applications and more importantly optional = true is the default.

In any other case, we would in theory be able to set the association to null upon initializing the properties of the entity holding the association, because the foreign key would be in the entity table and a simple null-check would tell us if there's an association or not. That feature probably even exists somewhere, though I won't bother finding out where.

Now, the question remains: when possible, should these associations be lazy or not? And unless the documentation says something about it, I'm afraid it's anyone's guess...

[fabrii]

Is there a way to run the hibernate-orm tests with Bytecode Enhancement enabled?

All of them? Not that I know. There are dedicated tests though, those that use @RunWith(BytecodeEnhancerRunner.class).

Good! If you think it might help, I can start by coding Hibernate.isInitialized tests with and without Bytecode Enhancement in Envers at hibernate-orm (5.6 or main branch ?)

Adam only said "all relations are lazy", but I don't think that implied _ToOne relations as well as that's not technically possible unless they are marked as non-nullable. We can't convert a lazy proxy to "null" on initialization if we discover the _one element it's pointing to didn't actually exist.

The implementation of this Envers feature is in this two classes:

• https://github.com/hibernate/hibernate-orm/blob/main/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/ToOneIdMapper.java
• https://github.com/hibernate/hibernate-orm/blob/main/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/ToOneEntityLoader.java

[yrodiere]

Good! If you think it might help, I can start by coding Hibernate.isInitialized tests with and without Bytecode Enhancement in Envers at hibernate-orm (5.6 or main branch ?)

For the collection problem? Sure it would help, thanks. Better submit a PR to the main branch; backporting to 5.6 is another story.

By the way, for the collections, if the fix is just about adding an interface and extending it in both PersistentCollection and envers' collections, writing the tests is probably 90% of the work, so you can probably submit the fix along with the tests :)

Don't forget to raise the issue through JIRA before sending a PR, though: https://hibernate.atlassian.net/

[fabrii]

Collections issue ready:

JIRA: https://hibernate.atlassian.net/browse/HHH-15522 PR: https://github.com/hibernate/hibernate-orm/pull/5302

Is this going to be backported to 5.6?

[yrodiere]

Thanks.

Is this going to be backported to 5.6?

We'll have to check how likely this is to cause regressions, but probably, yes.

[fabrii]

Hi! Just to point out that the fix related to Hibernate.isInitialized for envers collections was released on Hibernate 5.6.12, and it was updated on Quarkus 2.13.1.Final.

I don't know what the decision will be regarding *ToOne relations. Should we keep this issue or open a new one?

[yrodiere]

Thanks for the heads-up, yes we should close this ticket.

Regarding making *ToOne associations lazy, as we mentioned earlier, that's only possible in very specific cases (optional = false, ...).

If that's really a problem for you, and these *ToOne associations were lazy at some point, or they were at least explicitly documented as being lazy, or if them not being lazy is making Envers unusable, then I'd recommend filing an issue on the Hibernate ORM project. Though I'll be honest: I don't think this will be considered a priority by the ORM team (they're very busy) and you might have to submit a PR...

Anyway, at least the collection part is fixed. Thanks a lot for that!