more work on uniform reporting of illegal arguments

Author: gavinking

hibernate-core/src/main/java/org/hibernate/engine/internal/StatefulPersistenceContext.java

@@ -1707,27 +1707,26 @@ private void clearNullProperties() {
 	@Override
 	public boolean isReadOnly(Object entityOrProxy) {
 		if ( entityOrProxy == null ) {
-			throw new IllegalArgumentException( "Null entity instance" );
+			throw new IllegalArgumentException( "Entity may not be null" );
 		}
-		boolean isReadOnly;
+
 		final LazyInitializer lazyInitializer = extractLazyInitializer( entityOrProxy );
 		if ( lazyInitializer != null ) {
-			isReadOnly = lazyInitializer.isReadOnly();
+			return lazyInitializer.isReadOnly();
 		}
 		else {
-			final EntityEntry ee =  getEntry( entityOrProxy );
+			final EntityEntry ee = getEntry( entityOrProxy );
 			if ( ee == null ) {
-				throw new IllegalArgumentException( "Instance is not associated with this persistence context" );
+				throw new IllegalArgumentException( "Given entity is not associated with the persistence context" );
 			}
-			isReadOnly = ee.isReadOnly();
+			return ee.isReadOnly();
 		}
-		return isReadOnly;
 	}
 
 	@Override
 	public void setReadOnly(Object object, boolean readOnly) {
 		if ( object == null ) {
-			throw new IllegalArgumentException( "Null entity instance" );
+			throw new IllegalArgumentException( "Entity may not be null" );
 		}
 		if ( isReadOnly( object ) == readOnly ) {
 			return;
@@ -1765,7 +1764,7 @@ private void setProxyReadOnly(LazyInitializer hibernateLazyInitializer, boolean
 	private void setEntityReadOnly(Object entity, boolean readOnly) {
 		final EntityEntry entry = getEntry( entity );
 		if ( entry == null ) {
-			throw new IllegalArgumentException( "Instance was not associated with this persistence context" );
+			throw new IllegalArgumentException( "Given entity is not associated with the persistence context" );
 		}
 		entry.setReadOnly( readOnly, entity );
 		hasNonReadOnlyEntities = hasNonReadOnlyEntities || ! readOnly;

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

@@ -549,16 +549,13 @@ public LockMode getCurrentLockMode(Object object) {
 			}
 		}
 
-		final EntityEntry e = persistenceContext.getEntry( object );
-		if ( e == null ) {
-			throw new IllegalArgumentException( "Given entity is not associated with the persistence context" );
-		}
-		else if ( e.getStatus().isDeletedOrGone() ) {
-			throw new ObjectDeletedException( "Given entity was removed", e.getId(),
-					e.getPersister().getEntityName() );
+		final EntityEntry entry = getEntityEntry( object );
+		if ( entry.getStatus().isDeletedOrGone() ) {
+			throw new ObjectDeletedException( "Given entity was removed", entry.getId(),
+					entry.getPersister().getEntityName() );
 		}
 		else {
-			return e.getLockMode();
+			return entry.getLockMode();
 		}
 	}
 
@@ -1555,17 +1552,11 @@ public Object getIdentifier(Object object) {
 		}
 		final LazyInitializer lazyInitializer = extractLazyInitializer( object );
 		if ( lazyInitializer != null ) {
-			if ( lazyInitializer.getSession() != this ) {
-				throw new IllegalArgumentException( "Given proxy is not associated with the persistence context" );
-			}
+			checkOwnsProxy( lazyInitializer );
 			return lazyInitializer.getInternalIdentifier();
 		}
 		else {
-			final EntityEntry entry = persistenceContext.getEntry( object );
-			if ( entry == null ) {
-				throw new IllegalArgumentException( "Given entity is not associated with the persistence context" );
-			}
-			return entry.getId();
+			return getEntityEntry( object ).getId();
 		}
 	}
 
@@ -1819,17 +1810,25 @@ public String getEntityName(Object object) {
 
 		final LazyInitializer lazyInitializer = extractLazyInitializer( object );
 		if ( lazyInitializer != null ) {
-			if ( !persistenceContext.containsProxy( object ) ) {
-				throw new IllegalArgumentException( "Given proxy is not associated with the persistence context" );
-			}
+			checkOwnsProxy( lazyInitializer );
 			object = lazyInitializer.getImplementation();
 		}
 
+		return getEntityEntry( object ).getPersister().getEntityName();
+	}
+
+	private void checkOwnsProxy(LazyInitializer lazyInitializer) {
+		if ( lazyInitializer.getSession() != this ) {
+			throw new IllegalArgumentException( "Given proxy is not associated with the persistence context" );
+		}
+	}
+
+	private EntityEntry getEntityEntry(Object object) {
 		final EntityEntry entry = persistenceContext.getEntry( object );
 		if ( entry == null ) {
 			throw new IllegalArgumentException( "Given entity is not associated with the persistence context" );
 		}
-		return entry.getPersister().getEntityName();
+		return entry;
 	}
 
 	@Override @SuppressWarnings("unchecked")
@@ -2788,7 +2787,7 @@ public LockModeType getLockMode(Object entity) {
 		if ( !contains( entity ) ) {
 			// convert() calls markForRollbackOnly()
 			throw getExceptionConverter()
-					.convert( new IllegalArgumentException( "Entity not associated with the persistence context" ) );
+					.convert( new IllegalArgumentException( "Given entity is not associated with the persistence context" ) );
 		}
 
 		return LockModeTypeHelper.getLockModeType( getCurrentLockMode( entity ) );

hibernate-core/src/test/java/org/hibernate/orm/test/nullargs/DetachedEntityManagerArgumentsTest.java

@@ -0,0 +1,44 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ * Copyright Red Hat Inc. and Hibernate Authors
+ */
+package org.hibernate.orm.test.nullargs;
+
+import jakarta.persistence.Entity;
+import jakarta.persistence.Id;
+import jakarta.persistence.LockModeType;
+import org.hibernate.testing.orm.junit.EntityManagerFactoryScope;
+import org.hibernate.testing.orm.junit.Jpa;
+import org.junit.jupiter.api.Test;
+
+import java.util.List;
+import java.util.function.Consumer;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+@Jpa(annotatedClasses = DetachedEntityManagerArgumentsTest.Thing.class)
+class DetachedEntityManagerArgumentsTest {
+	@Test void test(EntityManagerFactoryScope scope) {
+		scope.inTransaction( em -> {
+			var operations = List.<Consumer<Object>>of(
+					em::refresh,
+					e -> em.lock( e, LockModeType.NONE ),
+					em::getLockMode
+			);
+			Thing thing = new Thing();
+			thing.id = 5L;
+			operations.forEach( c -> {
+				try {
+					c.accept( thing );
+				}
+				catch ( IllegalArgumentException e ) {
+					assertTrue( e.getMessage().startsWith( "Given entity is not associated with the persistence context" ) );
+				}
+			} );
+		} );
+	}
+	@Entity static class Thing {
+		@Id
+		Long id;
+	}
+}

hibernate-core/src/test/java/org/hibernate/orm/test/nullargs/DetachedSessionArgumentsTest.java

@@ -0,0 +1,50 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ * Copyright Red Hat Inc. and Hibernate Authors
+ */
+package org.hibernate.orm.test.nullargs;
+
+import jakarta.persistence.Entity;
+import jakarta.persistence.Id;
+import jakarta.persistence.LockModeType;
+import org.hibernate.testing.orm.junit.DomainModel;
+import org.hibernate.testing.orm.junit.SessionFactory;
+import org.hibernate.testing.orm.junit.SessionFactoryScope;
+import org.junit.jupiter.api.Test;
+
+import java.util.List;
+import java.util.function.Consumer;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+@SessionFactory
+@DomainModel(annotatedClasses = DetachedSessionArgumentsTest.Thing.class)
+class DetachedSessionArgumentsTest {
+	@Test void test(SessionFactoryScope scope) {
+		scope.inTransaction( session -> {
+			var operations = List.<Consumer<Object>>of(
+					session::refresh,
+					e -> session.lock( e, LockModeType.NONE ),
+					session::getLockMode,
+					session::isReadOnly,
+					e -> session.setReadOnly( e, true ),
+					session::getIdentifier
+
+			);
+			Thing thing = new Thing();
+			thing.id = 5L;
+			operations.forEach( c -> {
+				try {
+					c.accept( thing );
+				}
+				catch ( IllegalArgumentException e ) {
+					assertTrue( e.getMessage().startsWith( "Given entity is not associated with the persistence context" ) );
+				}
+			} );
+		} );
+	}
+	@Entity static class Thing {
+		@Id
+		Long id;
+	}
+}

hibernate-core/src/test/java/org/hibernate/orm/test/nullargs/NullSessionArgumentsTest.java

@@ -33,7 +33,9 @@ class NullSessionArgumentsTest {
 					session::getLockMode,
 					session::getEntityName,
 					session::getIdentifier,
-					session::contains
+					session::contains,
+					session::isReadOnly,
+					e -> session.setReadOnly( e, true )
 			);
 			operations.forEach( c -> {
 						try {