From 5c89079f2e61d82f623b48da4623af022bc5daa8 Mon Sep 17 00:00:00 2001 From: Gavin King Date: Sun, 1 Sep 2024 19:47:48 +0200 Subject: [PATCH] consistently pass along session-level lock options previously, these were respected by a random subset of session methods Signed-off-by: Gavin King --- .../internal/DefaultLoadEventListener.java | 27 ++-- .../hibernate/internal/CoreMessageLogger.java | 6 + .../org/hibernate/internal/SessionImpl.java | 119 ++++++++++-------- 3 files changed, 83 insertions(+), 69 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/event/internal/DefaultLoadEventListener.java b/hibernate-core/src/main/java/org/hibernate/event/internal/DefaultLoadEventListener.java index 3f6227a27c..cfd84bccd7 100644 --- a/hibernate-core/src/main/java/org/hibernate/event/internal/DefaultLoadEventListener.java +++ b/hibernate-core/src/main/java/org/hibernate/event/internal/DefaultLoadEventListener.java @@ -106,7 +106,7 @@ public class DefaultLoadEventListener implements LoadEventListener { } else { //return a proxy if appropriate - Object result = event.getLockMode() == LockMode.NONE + final Object result = event.getLockMode() == LockMode.NONE ? proxyOrLoad( event, persister, keyToLoad, loadType ) : lockAndLoad( event, persister, keyToLoad, loadType ); event.setResult( result ); @@ -123,17 +123,16 @@ public class DefaultLoadEventListener implements LoadEventListener { // we may have the jpa requirement of allowing find-by-id where id is the "simple pk value" of a // dependent objects parent. This is part of its generally goofy derived identity "feature" final EntityIdentifierMapping idMapping = persister.getIdentifierMapping(); - if ( idMapping instanceof CompositeIdentifierMapping ) { - final CompositeIdentifierMapping compositeIdMapping = (CompositeIdentifierMapping) idMapping; + if ( idMapping instanceof CompositeIdentifierMapping compositeIdMapping ) { final EmbeddableMappingType partMappingType = compositeIdMapping.getPartMappingType(); if ( partMappingType.getNumberOfAttributeMappings() == 1 ) { final AttributeMapping singleIdAttribute = partMappingType.getAttributeMapping( 0 ); - if ( singleIdAttribute.getMappedType() instanceof EntityMappingType ) { - final EntityMappingType parentIdTargetMapping = (EntityMappingType) singleIdAttribute.getMappedType(); + if ( singleIdAttribute.getMappedType() instanceof EntityMappingType parentIdTargetMapping ) { final EntityIdentifierMapping parentIdTargetIdMapping = parentIdTargetMapping.getIdentifierMapping(); - final MappingType parentIdType = parentIdTargetIdMapping instanceof CompositeIdentifierMapping - ? ((CompositeIdentifierMapping) parentIdTargetIdMapping).getMappedIdEmbeddableTypeDescriptor() - : parentIdTargetIdMapping.getMappedType(); + final MappingType parentIdType = + parentIdTargetIdMapping instanceof CompositeIdentifierMapping compositeMapping + ? compositeMapping.getMappedIdEmbeddableTypeDescriptor() + : parentIdTargetIdMapping.getMappedType(); if ( parentIdType.getMappedJavaType().getJavaTypeClass().isInstance( event.getEntityId() ) ) { // yep that's what we have... loadByDerivedIdentitySimplePkValue( @@ -309,8 +308,8 @@ public class DefaultLoadEventListener implements LoadEventListener { // else { // if the entity defines a HibernateProxy factory, see if there is an // existing proxy associated with the PC - and if so, use it - final Object proxy; - if ( holder != null && ( proxy = holder.getProxy() ) != null ) { + final Object proxy = holder == null ? null : holder.getProxy(); + if ( proxy != null ) { LOG.trace( "Entity proxy found in session cache" ); if ( LOG.isDebugEnabled() && HibernateProxy.extractLazyInitializer( proxy ).isUnwrap() ) { LOG.debug( "Ignoring NO_PROXY to honor laziness" ); @@ -372,7 +371,7 @@ public class DefaultLoadEventListener implements LoadEventListener { if ( LOG.isTraceEnabled() ) { LOG.trace( "Entity proxy found in session cache" ); } - LazyInitializer li = HibernateProxy.extractLazyInitializer( proxy ); + final LazyInitializer li = HibernateProxy.extractLazyInitializer( proxy ); if ( li.isUnwrap() ) { return li.getImplementation(); } @@ -389,7 +388,7 @@ public class DefaultLoadEventListener implements LoadEventListener { } private Object proxyImplementation(LoadEvent event, EntityPersister persister, EntityKey keyToLoad, LoadType options) { - Object entity = load( event, persister, keyToLoad, options ); + final Object entity = load( event, persister, keyToLoad, options ); if ( entity != null ) { return entity; } @@ -415,7 +414,7 @@ public class DefaultLoadEventListener implements LoadEventListener { * @param persister The persister corresponding to the entity to be loaded * @param keyToLoad The key of the entity to be loaded * @param options The defined load options - * @param holder + * @param holder an {@link EntityHolder} for the key * * @return The created/existing proxy */ @@ -467,7 +466,7 @@ public class DefaultLoadEventListener implements LoadEventListener { * @return The loaded entity */ private Object lockAndLoad(LoadEvent event, EntityPersister persister, EntityKey keyToLoad, LoadType options) { - final SessionImplementor source = event.getSession();; + final SessionImplementor source = event.getSession(); final EntityDataAccess cache = persister.getCacheAccessStrategy(); final SoftLock lock; diff --git a/hibernate-core/src/main/java/org/hibernate/internal/CoreMessageLogger.java b/hibernate-core/src/main/java/org/hibernate/internal/CoreMessageLogger.java index 9929c6c027..b9a33a468c 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/CoreMessageLogger.java +++ b/hibernate-core/src/main/java/org/hibernate/internal/CoreMessageLogger.java @@ -17,6 +17,7 @@ import java.util.Set; import javax.naming.NamingException; import org.hibernate.HibernateException; +import org.hibernate.JDBCException; import org.hibernate.LockMode; import org.hibernate.cache.CacheException; import org.hibernate.dialect.Dialect; @@ -964,4 +965,9 @@ public interface CoreMessageLogger extends BasicLogger { @Message(value = "Failed to discover types for enhancement from class: %s", id = 516) void enhancementDiscoveryFailed(String className, @Cause Throwable cause); + + @LogMessage(level = DEBUG) + @Message(value = "JDBCException was thrown for a transaction marked for rollback. " + + " This is probably due to an operation failing fast due to the transaction being marked for rollback.") + void jdbcExceptionThrownWithTransactionRolledBack(@Cause JDBCException e); } diff --git a/hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java b/hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java index 9335a85f8b..b175fcaa05 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java @@ -309,14 +309,14 @@ public class SessionImpl } private LockOptions getLockOptionsForRead() { - return this.lockOptions == null ? fastSessionServices.defaultLockOptions : this.lockOptions; + return lockOptions == null ? fastSessionServices.defaultLockOptions : lockOptions; } private LockOptions getLockOptionsForWrite() { - if ( this.lockOptions == null ) { - this.lockOptions = new LockOptions(); + if ( lockOptions == null ) { + lockOptions = new LockOptions(); } - return this.lockOptions; + return lockOptions; } protected void applyQuerySettingsAndHints(SelectionQuery query) { @@ -647,7 +647,9 @@ public class SessionImpl @Override public void lock(Object object, LockMode lockMode) throws HibernateException { - fireLock( new LockEvent( object, lockMode, this ) ); + final LockOptions lockOptions = copySessionLockOptions(); + lockOptions.setLockMode( lockMode ); + fireLock( new LockEvent( object, lockOptions, this ) ); } private void fireLock(LockEvent event) { @@ -852,13 +854,7 @@ public class SessionImpl logRemoveOrphanBeforeUpdates( "before continuing", entityName, object ); } fireDelete( - new DeleteEvent( - entityName, - object, - isCascadeDeleteEnabled, - removingOrphanBeforeUpates, - this - ), + new DeleteEvent( entityName, object, isCascadeDeleteEnabled, removingOrphanBeforeUpates, this ), transientEntities ); if ( traceEnabled && removingOrphanBeforeUpates ) { @@ -1159,7 +1155,9 @@ public class SessionImpl @Override public void refresh(Object object, LockMode lockMode) throws HibernateException { - fireRefresh( new RefreshEvent( object, lockMode, this ) ); + final LockOptions lockOptions = copySessionLockOptions(); + lockOptions.setLockMode( lockMode ); + fireRefresh( new RefreshEvent( object, lockOptions, this ) ); } @Override @@ -2260,11 +2258,10 @@ public class SessionImpl public T find(Class entityClass, Object primaryKey, LockModeType lockModeType, Map properties) { checkOpen(); - LockOptions lockOptions = null; + final LockOptions lockOptions = lockModeType == null ? null : buildLockOptions( lockModeType, properties ); try { if ( lockModeType != null ) { checkTransactionNeededForLock( LockModeTypeHelper.getLockMode( lockModeType ) ); - lockOptions = buildLockOptions( lockModeType, properties ); } final EffectiveEntityGraph effectiveEntityGraph = loadQueryInfluencers.getEffectiveEntityGraph(); @@ -2276,33 +2273,34 @@ public class SessionImpl .with( lockOptions ) .load( primaryKey ); } - catch ( EntityNotFoundException enfe ) { + catch ( FetchNotFoundException e ) { // This may happen if the entity has an associations mapped with // @NotFound(action = NotFoundAction.EXCEPTION) and this associated // entity is not found - if ( enfe instanceof FetchNotFoundException ) { - throw enfe; - } + throw e; + } + catch ( EntityFilterException e ) { // This may happen if the entity has an associations which is // filtered by a FilterDef and this associated entity is not found - if ( enfe instanceof EntityFilterException ) { - throw enfe; - } - // DefaultLoadEventListener#returnNarrowedProxy() may throw ENFE (see HHH-7861 for details), - // which find() should not throw. Find() should return null if the entity was not found. - if ( log.isDebugEnabled() ) { - String entityName = entityClass != null ? entityClass.getName(): null; - String identifierValue = primaryKey != null ? primaryKey.toString() : null ; - log.ignoringEntityNotFound( entityName, identifierValue ); - } + throw e; + } + catch ( EntityNotFoundException e ) { + // We swallow other sorts of EntityNotFoundException and return null + // For example, DefaultLoadEventListener.proxyImplementation() throws + // EntityNotFoundException if there's an existing proxy in the session, + // but the underlying database row has been deleted (see HHH-7861) + logIgnoringEntityNotFound( entityClass, primaryKey ); return null; } catch ( ObjectDeletedException e ) { - //the spec is silent about people doing remove() find() on the same PC + // the spec is silent about people doing remove() find() on the same PC return null; } catch ( ObjectNotFoundException e ) { - //should not happen on the entity itself with get + // should not happen on the entity itself with get + // TODO: in fact this will occur instead of EntityNotFoundException + // when using StandardEntityNotFoundDelegate, so probably we + // should return null here, as we do above throw new IllegalArgumentException( e.getMessage(), e ); } catch ( MappingException | TypeMismatchException | ClassCastException e ) { @@ -2310,13 +2308,9 @@ public class SessionImpl } catch ( JDBCException e ) { if ( accessTransaction().isActive() && accessTransaction().getRollbackOnly() ) { - // Assume this is similar to the WildFly / IronJacamar "feature" described under HHH-12472. - // Just log the exception and return null. - if ( log.isDebugEnabled() ) { - log.debug( "JDBCException was thrown for a transaction marked for rollback; " + - "this is probably due to an operation failing fast due to the " + - "transaction marked for rollback.", e ); - } + // Assume situation HHH-12472 running on WildFly + // Just log the exception and return null + log.jdbcExceptionThrownWithTransactionRolledBack( e ); return null; } else { @@ -2332,11 +2326,20 @@ public class SessionImpl } } + private static void logIgnoringEntityNotFound(Class entityClass, Object primaryKey) { + if ( log.isDebugEnabled() ) { + log.ignoringEntityNotFound( + entityClass != null ? entityClass.getName(): null, + primaryKey != null ? primaryKey.toString() : null + ); + } + } + private IdentifierLoadAccessImpl loadAccessWithOptions(Class entityClass, FindOption[] options) { - final IdentifierLoadAccessImpl loadAccess = byId(entityClass); + final IdentifierLoadAccessImpl loadAccess = byId( entityClass ); CacheStoreMode storeMode = getCacheStoreMode(); CacheRetrieveMode retrieveMode = getCacheRetrieveMode(); - LockOptions lockOptions = new LockOptions(); + LockOptions lockOptions = copySessionLockOptions(); for ( FindOption option : options ) { if ( option instanceof CacheStoreMode cacheStoreMode ) { storeMode = cacheStoreMode; @@ -2481,8 +2484,9 @@ public class SessionImpl lock( entity, buildLockOptions( lockMode, options ) ); } - private static LockOptions buildLockOptions(LockModeType lockMode, LockOption[] options) { - final LockOptions lockOptions = new LockOptions( LockModeTypeHelper.getLockMode( lockMode ) ); + private LockOptions buildLockOptions(LockModeType lockModeType, LockOption[] options) { + final LockOptions lockOptions = copySessionLockOptions(); + lockOptions.setLockMode( LockModeTypeHelper.getLockMode( lockModeType ) ); for ( LockOption option : options ) { if ( option instanceof PessimisticLockScope lockScope ) { lockOptions.setLockScope( lockScope ); @@ -2494,6 +2498,23 @@ public class SessionImpl return lockOptions; } + private LockOptions buildLockOptions(LockModeType lockModeType, Map properties) { + final LockOptions lockOptions = copySessionLockOptions(); + lockOptions.setLockMode( LockModeTypeHelper.getLockMode( lockModeType ) ); + if ( properties != null ) { + applyPropertiesToLockOptions( properties, () -> lockOptions ); + } + return lockOptions; + } + + private LockOptions copySessionLockOptions() { + final LockOptions copiedLockOptions = new LockOptions(); + if ( lockOptions != null ) { + LockOptions.copy( lockOptions, copiedLockOptions ); + } + return copiedLockOptions; + } + @Override public void refresh(Object entity, LockModeType lockModeType) { refresh( entity, LockModeTypeHelper.getLockMode( lockModeType ) ); @@ -2526,7 +2547,7 @@ public class SessionImpl @Override public void refresh(Object entity, RefreshOption... options) { CacheStoreMode storeMode = getCacheStoreMode(); - LockOptions lockOptions = new LockOptions(); + LockOptions lockOptions = copySessionLockOptions(); for ( RefreshOption option : options ) { if ( option instanceof CacheStoreMode cacheStoreMode ) { storeMode = cacheStoreMode; @@ -2560,18 +2581,6 @@ public class SessionImpl } } - private LockOptions buildLockOptions(LockModeType lockModeType, Map properties) { - final LockOptions lockOptions = new LockOptions(); - if ( this.lockOptions != null ) { //otherwise the default LockOptions constructor is the same as DEFAULT_LOCK_OPTIONS - LockOptions.copy( this.lockOptions, lockOptions ); - } - lockOptions.setLockMode( LockModeTypeHelper.getLockMode( lockModeType ) ); - if ( properties != null ) { - applyPropertiesToLockOptions( properties, () -> lockOptions ); - } - return lockOptions; - } - @Override public void detach(Object entity) { checkOpen();