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

HHH-19203 Unexpected association fetch triggered by Bean Validation #9804

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,17 @@ public class BeanValidationEventListener
BeanValidationEventListener.class.getName()
);

private HibernateTraversableResolver traversableResolver;
private Validator validator;
private GroupsPerOperation groupsPerOperation;
private final HibernateTraversableResolver traversableResolver;
private final Validator validator;
private final GroupsPerOperation groupsPerOperation;

public BeanValidationEventListener(
ValidatorFactory factory, Map<String, Object> settings, ClassLoaderService classLoaderService) {
traversableResolver = new HibernateTraversableResolver();
ValidatorFactory factory,
Map<String, Object> settings,
ClassLoaderService classLoaderService,
SessionFactoryImplementor sessionFactory) {

traversableResolver = new HibernateTraversableResolver( sessionFactory.getPersistenceUnitUtil() );
validator = factory.usingContext()
.traversableResolver( traversableResolver )
.getValidator();
Expand All @@ -80,7 +84,6 @@ public boolean onPreInsert(PreInsertEvent event) {
validate(
event.getEntity(),
event.getPersister(),
event.getFactory(),
GroupsPerOperation.Operation.INSERT
);
return false;
Expand All @@ -90,7 +93,6 @@ public boolean onPreUpdate(PreUpdateEvent event) {
validate(
event.getEntity(),
event.getPersister(),
event.getFactory(),
GroupsPerOperation.Operation.UPDATE
);
return false;
Expand All @@ -100,7 +102,6 @@ public boolean onPreDelete(PreDeleteEvent event) {
validate(
event.getEntity(),
event.getPersister(),
event.getFactory(),
GroupsPerOperation.Operation.DELETE
);
return false;
Expand All @@ -111,7 +112,6 @@ public boolean onPreUpsert(PreUpsertEvent event) {
validate(
event.getEntity(),
event.getPersister(),
event.getFactory(),
GroupsPerOperation.Operation.UPSERT
);
return false;
Expand All @@ -123,15 +123,13 @@ public void onPreUpdateCollection(PreCollectionUpdateEvent event) {
validate(
entity,
event.getSession().getEntityPersister( event.getAffectedOwnerEntityName(), entity ),
event.getFactory(),
GroupsPerOperation.Operation.UPDATE
);
}

private <T> void validate(
T object,
EntityPersister persister,
SessionFactoryImplementor sessionFactory,
GroupsPerOperation.Operation operation) {
if ( object == null || persister.getRepresentationStrategy().getMode() != RepresentationMode.POJO ) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.hibernate.type.EntityType;
import org.hibernate.type.Type;

import jakarta.persistence.PersistenceUnitUtil;
import jakarta.validation.Path;
import jakarta.validation.TraversableResolver;

Expand All @@ -32,6 +33,11 @@
*/
public class HibernateTraversableResolver implements TraversableResolver {
private final Map<Class<?>, Set<String>> associationsPerEntityClass = new HashMap<>();
private final PersistenceUnitUtil persistenceUnitUtil;

public HibernateTraversableResolver(PersistenceUnitUtil persistenceUnitUtil) {
this.persistenceUnitUtil = persistenceUnitUtil;
}

public void addPersister(EntityPersister persister, SessionFactoryImplementor factory) {
Class<?> javaTypeClass = persister.getEntityMappingType().getMappedJavaType().getJavaTypeClass();
Expand Down Expand Up @@ -91,7 +97,7 @@ public boolean isReachable(Object traversableObject,
ElementType elementType) {
//lazy, don't load
return Hibernate.isInitialized( traversableObject )
&& Hibernate.isPropertyInitialized( traversableObject, traversableProperty.getName() );
&& persistenceUnitUtil.isLoaded( traversableObject, traversableProperty.getName() );
Comment on lines -94 to +100
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hibernate.isPropertyInitialized() is documented to be a synonym for PersistenceUtil.isLoaded(), so this should in principle not have fixed anything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, the issue is around uninitialized collections. So what we're trying to do here is add an additional check equivalent to Hibernate.isInitialized(collection). Apparently there's an undocumented difference between Hibernate.isPropertyInitialized() and PersistenceUtil.isLoaded() for that collections. At minimum we should document that properly/.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we should maybe also change the documentation, because it's not a synonym.

The PersistenceUnitUtil implementation is aware of the SessionFactory and can hence also check laziness of and initialize managed objects that are not enhanced, whereas the Hibernate class can only deal with bytecode enhanced classes.

This is mostly due to the fact that checking if an attribute is actually initialized, we'd have to be able to access the value of the attribute, which we can't without the SessionFactory, because we don't have a PropertyAccess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, with all bytecode enhancement settings currently deprecated, I don't see any uses for Hibernate.isInitialized anymore. I think at least a documentation update is in order, and would go as far as deprecating the method. I don't think it's possible to reimplement Hibernate.isPropertyInitialized to align PersistenceUnitUtil.isLoaded without significant side effects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hibernate does not require the use of the bytecode enhancer, and most people don't use it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry I meant Hibernate.isPropertyInitialized not Hibernate.isInitialized. Without bytecode enhancement, the only time it returns unintialized (false) is when the passed in Object entity itself is a HibernateProxy returned by emf.getReferenceById, which is completely covered by Hibernate.isInitialized.

}

public boolean isCascadable(Object traversableObject,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,12 @@ private static void setupListener(ValidatorFactory validatorFactory, SessionFact
final ClassLoaderService classLoaderService = serviceRegistry.requireService( ClassLoaderService.class );
final ConfigurationService cfgService = serviceRegistry.requireService( ConfigurationService.class );
final BeanValidationEventListener listener =
new BeanValidationEventListener( validatorFactory, cfgService.getSettings(), classLoaderService );
new BeanValidationEventListener(
validatorFactory,
cfgService.getSettings(),
classLoaderService,
sessionFactory
);
final EventListenerRegistry listenerRegistry = serviceRegistry.requireService( EventListenerRegistry.class );
listenerRegistry.addDuplicationStrategy( DuplicationStrategyImpl.INSTANCE );
listenerRegistry.appendListeners( EventType.PRE_INSERT, listener );
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
/*
* SPDX-License-Identifier: LGPL-2.1-or-later
* Copyright Red Hat Inc. and Hibernate Authors
*/
package org.hibernate.orm.test.annotations.beanvalidation;

import java.util.List;

import org.hibernate.cfg.AvailableSettings;

import org.hibernate.testing.jdbc.SQLStatementInspector;
import org.hibernate.testing.orm.junit.DomainModel;
import org.hibernate.testing.orm.junit.JiraKey;
import org.hibernate.testing.orm.junit.ServiceRegistry;
import org.hibernate.testing.orm.junit.SessionFactory;
import org.hibernate.testing.orm.junit.SessionFactoryScope;
import org.hibernate.testing.orm.junit.Setting;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Test;

import jakarta.persistence.CascadeType;
import jakarta.persistence.CollectionTable;
import jakarta.persistence.ElementCollection;
import jakarta.persistence.Entity;
import jakarta.persistence.Id;
import jakarta.persistence.OneToMany;
import jakarta.validation.constraints.Size;

@DomainModel(annotatedClasses = {
LazyPropertiesFetchTest.Association.class,
LazyPropertiesFetchTest.MutableEntity.class
})
@ServiceRegistry(settings = @Setting(name = AvailableSettings.JAKARTA_VALIDATION_MODE, value = "CALLBACK"))
@SessionFactory(useCollectingStatementInspector = true)
@JiraKey("HHH-19203")
class LazyPropertiesFetchTest {

@AfterAll
static void cleanup(SessionFactoryScope scope) {
scope.dropData();
}

@Test
void testLazyCollectionNotFetched(SessionFactoryScope scope) {
scope.inTransaction( session -> {
MutableEntity mutableEntity = new MutableEntity();
mutableEntity.id = 1L;
mutableEntity.lazyCollection = List.of( 1, 2 );
session.persist( mutableEntity );
} );

SQLStatementInspector inspector = scope.getCollectingStatementInspector();
inspector.clear();

scope.inTransaction( session -> {
MutableEntity fetched = session.find( MutableEntity.class, 1L );
inspector.assertExecutedCount( 1 );
fetched.mutableField = 1;
} );

inspector.assertExecutedCount( 2 );
}

@Test
void testLazyCollectionFetchDoesntDependOnEachOther(SessionFactoryScope scope) {
scope.inTransaction( session -> {
MutableEntity mutableEntity = new MutableEntity();
mutableEntity.id = 2L;
mutableEntity.lazyCollection = List.of( 1, 2 );

Association asso = new Association();
asso.id = 1L;
asso.lazyCollection = List.of( 2, 3 );

mutableEntity.lazyAssociation = List.of( asso );

session.persist( mutableEntity );
} );

SQLStatementInspector inspector = scope.getCollectingStatementInspector();
inspector.clear();

scope.inTransaction( session -> {
MutableEntity fetched = session.find( MutableEntity.class, 2L );
inspector.assertExecutedCount( 1 );

Association asso = fetched.lazyAssociation.get( 0 );
inspector.assertExecutedCount( 2 );

asso.mutableField = 5;
} );
inspector.assertExecutedCount( 3 );
}

@Entity(name = "MutableEntity")
static class MutableEntity {
@Id
private Long id;

private int mutableField = 0;

@Size(max = 10)
@ElementCollection
@CollectionTable(name = "LazyPropertiesCollection1")
private List<Integer> lazyCollection;

@OneToMany(cascade = CascadeType.PERSIST)
private List<Association> lazyAssociation;
}

@Entity(name = "Association")
static class Association {
@Id
private Long id;

private int mutableField = 0;

@Size(max = 10)
@ElementCollection
@CollectionTable(name = "LazyPropertiesCollection2")
private List<Integer> lazyCollection;
}
}