improve exception messages and logging related to tx management

along with some minor aesthetic code cleanups

Author: gavinking

hibernate-core/src/main/java/org/hibernate/Transaction.java

@@ -69,9 +69,13 @@ public interface Transaction extends EntityTransaction {
 
 	/**
 	 * Attempt to mark the underlying transaction for rollback only.
+	 * <p>
+	 * Unlike {@link #setRollbackOnly()}, which is specified by JPA
+	 * to throw when the transaction is inactive, this operation may
+	 * be called on an inactive transaction, in which case it has no
+	 * effect.
+	 *
+	 * @see #setRollbackOnly()
 	 */
-	default void markRollbackOnly() {
-		setRollbackOnly();
-	}
-
+	void markRollbackOnly();
 }

hibernate-core/src/main/java/org/hibernate/engine/transaction/internal/TransactionImpl.java

@@ -12,7 +12,6 @@
 import org.hibernate.engine.transaction.spi.TransactionImplementor;
 import org.hibernate.internal.AbstractSharedSessionContract;
 import org.hibernate.internal.CoreLogging;
-import org.hibernate.jpa.spi.JpaCompliance;
 import org.hibernate.resource.transaction.spi.TransactionCoordinator;
 import org.hibernate.resource.transaction.spi.TransactionStatus;
 
@@ -28,7 +27,7 @@ public class TransactionImpl implements TransactionImplementor {
 	private static final Logger LOG = CoreLogging.logger( TransactionImpl.class );
 
 	private final TransactionCoordinator transactionCoordinator;
-	private final JpaCompliance jpaCompliance;
+	private final boolean jpaCompliance;
 	private final AbstractSharedSessionContract session;
 
 	private TransactionDriver transactionDriverControl;
@@ -37,7 +36,9 @@ public TransactionImpl(
 			TransactionCoordinator transactionCoordinator,
 			AbstractSharedSessionContract session) {
 		this.transactionCoordinator = transactionCoordinator;
-		this.jpaCompliance = session.getFactory().getSessionFactoryOptions().getJpaCompliance();
+		this.jpaCompliance =
+				session.getFactory().getSessionFactoryOptions().getJpaCompliance()
+						.isJpaTransactionComplianceEnabled();
 		this.session = session;
 
 		if ( session.isOpen() && transactionCoordinator.isActive() ) {
@@ -47,11 +48,8 @@ public TransactionImpl(
 			LOG.debug( "TransactionImpl created on closed Session/EntityManager" );
 		}
 
-		if ( LOG.isDebugEnabled() ) {
-			LOG.debugf(
-					"On TransactionImpl creation, JpaCompliance#isJpaTransactionComplianceEnabled == %s",
-					jpaCompliance.isJpaTransactionComplianceEnabled()
-			);
+		if ( LOG.isDebugEnabled() && jpaCompliance ) {
+			LOG.debugf( "TransactionImpl created in JPA compliant mode" );
 		}
 	}
 
@@ -65,78 +63,66 @@ public void begin() {
 			transactionDriverControl = transactionCoordinator.getTransactionDriverControl();
 		}
 
-		// per-JPA
 		if ( isActive() ) {
-			if ( jpaCompliance.isJpaTransactionComplianceEnabled()
-					|| !transactionCoordinator.getTransactionCoordinatorBuilder().isJta() ) {
-				throw new IllegalStateException( "Transaction already active" );
+			if ( jpaCompliance ) {
+				throw new IllegalStateException( "Transaction already active (in JPA compliant mode)" );
 			}
-			else {
-				return;
+			else if ( !transactionCoordinator.getTransactionCoordinatorBuilder().isJta() ) {
+				throw new IllegalStateException( "Resource-local transaction already active" );
 			}
 		}
-
-		LOG.debug( "begin" );
-
-		this.transactionDriverControl.begin();
+		else {
+			LOG.debug( "begin transaction" );
+			transactionDriverControl.begin();
+		}
 	}
 
 	@Override
 	public void commit() {
+		// allow MARKED_ROLLBACK to propagate through to transactionDriverControl
+		// the boolean passed to isActive indicates whether MARKED_ROLLBACK should
+		// be considered active
 		if ( !isActive( true ) ) {
-			// allow MARKED_ROLLBACK to propagate through to transactionDriverControl
-			// the boolean passed to isActive indicates whether MARKED_ROLLBACK should be
-			// considered active
-			//
-			// essentially here we have a transaction that is not active and
-			// has not been marked for rollback only
+			// we have a transaction that is inactive and has not been marked for rollback only
 			throw new IllegalStateException( "Transaction not successfully started" );
 		}
-
-		LOG.debug( "committing" );
-
-		try {
-			internalGetTransactionDriverControl().commit();
-		}
-		catch (RuntimeException e) {
-			throw session.getExceptionConverter().convertCommitException( e );
+		else {
+			LOG.debug( "committing transaction" );
+			try {
+				internalGetTransactionDriverControl().commit();
+			}
+			catch (RuntimeException e) {
+				throw session.getExceptionConverter().convertCommitException( e );
+			}
 		}
 	}
 
 	public TransactionDriver internalGetTransactionDriverControl() {
 		// NOTE here to help be a more descriptive NullPointerException
-		if ( this.transactionDriverControl == null ) {
+		if ( transactionDriverControl == null ) {
 			throw new IllegalStateException( "Transaction was not properly begun/started" );
 		}
-		return this.transactionDriverControl;
+		else {
+			return transactionDriverControl;
+		}
 	}
 
 	@Override
 	public void rollback() {
-		if ( !isActive() ) {
-			if ( jpaCompliance.isJpaTransactionComplianceEnabled() ) {
-
-				throw new IllegalStateException(
-						"JPA compliance dictates throwing IllegalStateException when #rollback " +
-								"is called on non-active transaction"
-				);
-			}
+		if ( !isActive() && jpaCompliance ) {
+			throw new IllegalStateException( "rollback() called on inactive transaction (in JPA compliant mode)" );
 		}
 
-		TransactionStatus status = getStatus();
+		final TransactionStatus status = getStatus();
 		if ( status == TransactionStatus.ROLLED_BACK || status == TransactionStatus.NOT_ACTIVE ) {
-			// Allow rollback() calls on completed transactions, just no-op.
+			// allow rollback() on completed transaction as noop
 			LOG.debug( "rollback() called on an inactive transaction" );
-			return;
 		}
-
-		if ( !status.canRollback() ) {
-			throw new TransactionException( "Cannot rollback transaction in current status [" + status.name() + "]" );
+		else if ( !status.canRollback() ) {
+			throw new TransactionException( "Cannot roll back transaction in current status [" + status.name() + "]" );
 		}
-
-		LOG.debug( "rolling back" );
-
-		if ( status != TransactionStatus.FAILED_COMMIT || allowFailedCommitToPhysicallyRollback() ) {
+		else if ( status != TransactionStatus.FAILED_COMMIT || allowFailedCommitToPhysicallyRollback() ) {
+			LOG.debug( "rolling back transaction" );
 			internalGetTransactionDriverControl().rollback();
 		}
 	}
@@ -176,55 +162,50 @@ public TransactionStatus getStatus() {
 
 	@Override
 	public void registerSynchronization(Synchronization synchronization) throws HibernateException {
-		this.transactionCoordinator.getLocalSynchronizations().registerSynchronization( synchronization );
+		transactionCoordinator.getLocalSynchronizations().registerSynchronization( synchronization );
 	}
 
 	@Override
 	public void setTimeout(int seconds) {
-		this.transactionCoordinator.setTimeOut( seconds );
+		transactionCoordinator.setTimeOut( seconds );
 	}
 
 	@Override
 	public void setTimeout(@Nullable Integer seconds) {
-		this.transactionCoordinator.setTimeOut( seconds == null ? -1 : seconds );
+		transactionCoordinator.setTimeOut( seconds == null ? -1 : seconds );
 	}
 
 	@Override
 	public @Nullable Integer getTimeout() {
-		final int timeOut = this.transactionCoordinator.getTimeOut();
-		if ( timeOut == -1 ) {
-			return null;
-		}
-		return timeOut;
+		final int timeout = transactionCoordinator.getTimeOut();
+		return timeout == -1 ? null : timeout;
 	}
 
 	@Override
 	public void markRollbackOnly() {
-		// this is the Hibernate-specific API, whereas #setRollbackOnly is the
-		// JPA-defined API.  In our opinion it is much more user-friendly to
+		// this is the Hibernate-specific API, whereas setRollbackOnly is the
+		// JPA-defined API. In our opinion it is much more user-friendly to
 		// always allow user/integration to indicate that the transaction
 		// should not be allowed to commit.
 		//
-		// However.. should only "do something" on an active transaction
 		if ( isActive() ) {
 			internalGetTransactionDriverControl().markRollbackOnly();
 		}
+		// else noop for an inactive transaction
 	}
 
 	@Override
 	public void setRollbackOnly() {
 		if ( !isActive() ) {
-			// Since this is the JPA-defined one, we make sure the txn is active first
-			// so long as compliance (JpaCompliance) has not been defined to disable
-			// that check - making this active more like Hibernate's #markRollbackOnly
-			if ( jpaCompliance.isJpaTransactionComplianceEnabled() ) {
-				throw new IllegalStateException(
-						"JPA compliance dictates throwing IllegalStateException when #setRollbackOnly " +
-								"is called on non-active transaction"
-				);
+			if ( jpaCompliance ) {
+				// This is the JPA-defined version of this operation,
+				// so we must check that the transaction is active
+				throw new IllegalStateException( "setRollbackOnly() called on inactive transaction (in JPA compliant mode)" );
 			}
 			else {
-				LOG.debug( "#setRollbackOnly called on a not-active transaction" );
+				// JpaCompliance disables the check, so this method
+				// is equivalent our native markRollbackOnly()
+				LOG.debug( "setRollbackOnly() called on a inactive transaction" );
 			}
 		}
 		else {
@@ -234,16 +215,12 @@ public void setRollbackOnly() {
 
 	@Override
 	public boolean getRollbackOnly() {
-		if ( !isActive() ) {
-			if ( jpaCompliance.isJpaTransactionComplianceEnabled() ) {
-				throw new IllegalStateException(
-						"JPA compliance dictates throwing IllegalStateException when #getRollbackOnly " +
-								"is called on non-active transaction"
-				);
-			}
+		if ( jpaCompliance && !isActive() ) {
+			throw new IllegalStateException( "getRollbackOnly() called on inactive transaction (in JPA compliant mode)" );
+		}
+		else {
+			return getStatus() == TransactionStatus.MARKED_ROLLBACK;
 		}
-
-		return getStatus() == TransactionStatus.MARKED_ROLLBACK;
 	}
 
 	protected boolean allowFailedCommitToPhysicallyRollback() {

hibernate-core/src/main/java/org/hibernate/internal/ExceptionConverterImpl.java

@@ -63,7 +63,7 @@ public RuntimeException convertCommitException(RuntimeException exception) {
 			catch (Exception re) {
 				//swallow
 			}
-			return new RollbackException( "Error while committing the transaction",
+			return new RollbackException( "Error while committing the transaction [" + exception.getMessage() + "]",
 					exception instanceof HibernateException hibernateException
 							? convert( hibernateException )
 							: exception );

hibernate-core/src/main/java/org/hibernate/internal/FastSessionServices.java

@@ -358,18 +358,16 @@ CacheRetrieveMode getCacheRetrieveMode(Map<String, Object> properties) {
 
 	private static CacheRetrieveMode determineCacheRetrieveMode(Map<String, Object> settings) {
 		final CacheRetrieveMode cacheRetrieveMode = (CacheRetrieveMode) settings.get( JPA_SHARED_CACHE_RETRIEVE_MODE );
-		if ( cacheRetrieveMode == null ) {
-			return (CacheRetrieveMode) settings.get( JAKARTA_SHARED_CACHE_RETRIEVE_MODE );
-		}
-		return cacheRetrieveMode;
+		return cacheRetrieveMode == null
+				? (CacheRetrieveMode) settings.get( JAKARTA_SHARED_CACHE_RETRIEVE_MODE )
+				: cacheRetrieveMode;
 	}
 
 	private static CacheStoreMode determineCacheStoreMode(Map<String, Object> settings) {
 		final CacheStoreMode cacheStoreMode = (CacheStoreMode) settings.get( JPA_SHARED_CACHE_STORE_MODE );
-		if ( cacheStoreMode == null ) {
-			return ( CacheStoreMode ) settings.get( JAKARTA_SHARED_CACHE_STORE_MODE );
-		}
-		return cacheStoreMode;
+		return cacheStoreMode == null
+				? (CacheStoreMode) settings.get( JAKARTA_SHARED_CACHE_STORE_MODE )
+				: cacheStoreMode;
 	}
 
 	public JdbcValuesMappingProducerProvider getJdbcValuesMappingProducerProvider() {

hibernate-core/src/main/java/org/hibernate/jpa/internal/MutableJpaComplianceImpl.java

@@ -169,15 +169,15 @@ public void setLoadByIdCompliance(boolean enabled) {
 
 	@Override
 	public JpaCompliance immutableCopy() {
-		JpaComplianceImpl.JpaComplianceBuilder builder = new JpaComplianceImpl.JpaComplianceBuilder();
-		builder = builder.setProxyCompliance( proxyCompliance )
+		return new JpaComplianceImpl.JpaComplianceBuilder()
+				.setProxyCompliance( proxyCompliance )
 				.setOrderByMappingCompliance( orderByMappingCompliance )
 				.setGlobalGeneratorNameCompliance( generatorNameScopeCompliance )
 				.setQueryCompliance( queryCompliance )
 				.setTransactionCompliance( transactionCompliance )
 				.setClosedCompliance( closedCompliance )
 				.setCachingCompliance( cachingCompliance )
-				.setLoadByIdCompliance( loadByIdCompliance );
-		return builder.createJpaCompliance();
+				.setLoadByIdCompliance( loadByIdCompliance )
+				.createJpaCompliance();
 	}
 }