HHH-1661 throw when merge() applied to a definitely-removed instance

group effort by @jrenaat, @beikov, and myself

Signed-off-by: Jan Schatteman <jschatte@redhat.com>
Signed-off-by: Gavin King <gavin@hibernate.org>

hibernate-core/src/main/java/org/hibernate/boot/model/internal/PropertyBinder.java

@@ -983,8 +983,6 @@ public class PropertyBinder {
 			rootClass.setDeclaredVersion( property );
 		}
 
-		final SimpleValue simpleValue = (SimpleValue) property.getValue();
-		simpleValue.setNullValue( "undefined" );
 		rootClass.setOptimisticLockStyle( OptimisticLockStyle.VERSION );
 		if ( LOG.isTraceEnabled() ) {
 			final SimpleValue versionValue = (SimpleValue) rootClass.getVersion().getValue();

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

@@ -6,12 +6,13 @@
  */
 package org.hibernate.engine.internal;
 
-import java.lang.reflect.Constructor;
 import java.util.function.Supplier;
 
-import org.hibernate.InstantiationException;
 import org.hibernate.MappingException;
+import org.hibernate.boot.spi.MetadataBuildingContext;
+import org.hibernate.engine.jdbc.spi.JdbcServices;
 import org.hibernate.engine.spi.IdentifierValue;
+import org.hibernate.engine.spi.SharedSessionDelegatorBaseImpl;
 import org.hibernate.engine.spi.VersionValue;
 import org.hibernate.mapping.KeyValue;
 import org.hibernate.property.access.spi.Getter;
@@ -94,7 +95,8 @@ public class UnsavedValueFactory {
 
 				// if the version of a newly instantiated object is not the same
 				// as the version seed value, use that as the unsaved-value
-				final T seedValue = jtd.seed( length, precision, scale, null );
+				final T seedValue = jtd.seed( length, precision, scale,
+						mockSession( bootVersionMapping.getBuildingContext() ) );
 				return jtd.areEqual( seedValue, defaultValue )
 						? VersionValue.UNDEFINED
 						: new VersionValue( defaultValue );
@@ -119,6 +121,16 @@ public class UnsavedValueFactory {
 
 	}
 
+	private static SharedSessionDelegatorBaseImpl mockSession(MetadataBuildingContext context) {
+		return new SharedSessionDelegatorBaseImpl(null) {
+			@Override
+			public JdbcServices getJdbcServices() {
+				return context.getBootstrapContext().getServiceRegistry()
+						.requireService( JdbcServices.class );
+			}
+		};
+	}
+
 	private UnsavedValueFactory() {
 	}
 }

hibernate-core/src/main/java/org/hibernate/event/internal/DefaultMergeEventListener.java

@@ -405,17 +405,26 @@ public class DefaultMergeEventListener
 		final Object result = source.getLoadQueryInfluencers().fromInternalFetchProfile(
 				CascadingFetchProfile.MERGE,
 				() -> source.get( entityName, clonedIdentifier )
-
 		);
-		if ( result == null ) {
-			//TODO: we should throw an exception if we really *know* for sure
-			//      that this is a detached instance, rather than just assuming
-			//throw new StaleObjectStateException(entityName, id);
 
+		if ( result == null ) {
+			LOG.trace( "Detached instance not found in database" );
 			// we got here because we assumed that an instance
-			// with an assigned id was detached, when it was
+			// with an assigned id and no version was detached,
-			// really persistent
+			// when it was really transient (or deleted)
-			entityIsTransient( event, clonedIdentifier, copyCache );
+			final Boolean knownTransient = persister.isTransient( entity, source );
+			if ( knownTransient == Boolean.FALSE ) {
+				// we know for sure it's detached (generated id
+				// or a version property), and so the instance
+				// must have been deleted by another transaction
+				throw new StaleObjectStateException( entityName, id );
+			}
+			else {
+				// we know for sure it's transient, or we just
+				// don't have information (assigned id and no
+				// version property) so keep assuming transient
+				entityIsTransient( event, clonedIdentifier, copyCache );
+			}
 		}
 		else {
 			// before cascade!

hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/merge/MergeDetachedCascadedCollectionInEmbeddableTest.java

@@ -17,6 +17,7 @@ import jakarta.persistence.Id;
 import jakarta.persistence.JoinColumn;
 import jakarta.persistence.OneToMany;
 
+import jakarta.persistence.OptimisticLockException;
 import org.hibernate.testing.bytecode.enhancement.extension.BytecodeEnhanced;
 import org.hibernate.testing.orm.junit.DomainModel;
 import org.hibernate.testing.orm.junit.JiraKey;
@@ -27,6 +28,7 @@ import static org.hibernate.orm.test.bytecode.enhancement.merge.MergeDetachedCas
 import static org.hibernate.orm.test.bytecode.enhancement.merge.MergeDetachedCascadedCollectionInEmbeddableTest.Heading;
 import static org.hibernate.orm.test.bytecode.enhancement.merge.MergeDetachedCascadedCollectionInEmbeddableTest.Thing;
 import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.fail;
 
 import org.junit.jupiter.api.Test;
 
@@ -55,13 +57,21 @@ public class MergeDetachedCascadedCollectionInEmbeddableTest {
 			return entity;
 		} );
 
-		scope.inTransaction( session -> {
+		try {
-			heading.name = "updated";
+			scope.inTransaction(session -> {
-			Heading headingMerged = (Heading) session.merge( heading );
+				heading.name = "updated";
-			assertNotSame( heading, headingMerged );
+				Heading headingMerged = session.merge(heading);
-			assertNotSame( heading.grouping, headingMerged.grouping );
+				assertNotSame(heading, headingMerged);
-			assertNotSame( heading.grouping.things, headingMerged.grouping.things );
+				assertNotSame(heading.grouping, headingMerged.grouping);
-		} );
+				assertNotSame(heading.grouping.things, headingMerged.grouping.things);
+				fail();
+			});
+		}
+		catch (OptimisticLockException e) {
+			// expected since tx above was never committed
+			// so the entity had id generated but was never
+			// actually inserted in database
+		}
 	}
 
 	@Entity(name = "Heading")

hibernate-core/src/test/java/org/hibernate/orm/test/exceptionhandling/StaleObjectMergeTest.java

@@ -0,0 +1,228 @@
+/*
+ * Hibernate, Relational Persistence for Idiomatic Java
+ *
+ * License: GNU Lesser General Public License (LGPL), version 2.1 or later
+ * See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html
+ */
+package org.hibernate.orm.test.exceptionhandling;
+
+import java.sql.Timestamp;
+
+import org.hibernate.dialect.H2Dialect;
+
+import org.hibernate.testing.TestForIssue;
+import org.hibernate.testing.orm.junit.DomainModel;
+import org.hibernate.testing.orm.junit.RequiresDialect;
+import org.hibernate.testing.orm.junit.SessionFactory;
+import org.hibernate.testing.orm.junit.SessionFactoryScope;
+import org.junit.jupiter.api.Test;
+
+import jakarta.persistence.Column;
+import jakarta.persistence.Entity;
+import jakarta.persistence.GeneratedValue;
+import jakarta.persistence.Id;
+import jakarta.persistence.OptimisticLockException;
+import jakarta.persistence.Version;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+/**
+ * @author Jan Schatteman
+ */
+@TestForIssue( jiraKey = "HHH-1661")
+@RequiresDialect( H2Dialect.class )
+public class StaleObjectMergeTest {
+
+	@DomainModel(
+			annotatedClasses = { A.class }
+	)
+	@SessionFactory
+	@Test
+	public void testStaleNonVersionEntityMerged(SessionFactoryScope scope) {
+		A a = new A();
+		scope.inTransaction(
+				session -> session.persist( a )
+		);
+
+		scope.inTransaction(
+				session -> {
+					A aGet = session.get( A.class, a.getId() );
+					session.remove( aGet );
+				}
+		);
+
+		assertThrows(
+				OptimisticLockException.class,
+				() -> scope.inTransaction(
+						session -> session.merge( a )
+				)
+		);
+	}
+
+	@DomainModel(
+			annotatedClasses = { B.class, C.class }
+	)
+	@SessionFactory
+	@Test
+	public void testStalePrimitiveAndWrapperVersionEntityMerged(SessionFactoryScope scope) {
+		B b = new B();
+		// this is a workaround because the version is seeded to 0, so there's no way of differentiating
+		// a new instance from a detached one for primitive types
+		b.setVersion( 1 );
+		scope.inTransaction(
+				session -> session.persist( b )
+		);
+
+		scope.inTransaction(
+				session -> {
+					B bGet = session.get( B.class, b.getId() );
+					session.remove( bGet );
+				}
+		);
+
+		assertThrows(
+				OptimisticLockException.class,
+				() -> {
+					scope.inTransaction(
+							session -> {
+								session.merge( b );
+							}
+					);
+				}
+		);
+
+		C c = new C();
+		scope.inTransaction(
+				session -> session.persist( c )
+		);
+
+		scope.inTransaction(
+				session -> {
+					C cGet = session.get( C.class, c.getId() );
+					session.remove( cGet );
+				}
+		);
+
+		assertThrows(
+				OptimisticLockException.class,
+				() -> {
+					scope.inTransaction(
+							session -> {
+								session.merge( c );
+							}
+					);
+				}
+		);
+	}
+
+	@DomainModel(
+			annotatedClasses = { D.class }
+	)
+	@SessionFactory
+	@Test
+	public void testStaleTimestampVersionEntityMerged(SessionFactoryScope scope) {
+		D d = new D();
+		scope.inTransaction(
+				session -> session.persist( d )
+		);
+
+		scope.inTransaction(
+				session -> {
+					D dGet = session.get( D.class, d.getId() );
+					session.remove( dGet );
+				}
+		);
+
+		assertThrows(
+				OptimisticLockException.class,
+				() -> {
+					scope.inTransaction(
+							session -> {
+								session.merge( d );
+							}
+					);
+				}
+		);
+	}
+
+	@Entity(name = "A")
+	public static class A {
+		@Id
+		@GeneratedValue
+		private long id;
+
+		public Long getId() {
+			return id;
+		}
+
+		public void setId(Long id) {
+			this.id = id;
+		}
+	}
+
+	@Entity(name = "B")
+	public static class B {
+		@Id
+		@GeneratedValue
+		private long id;
+
+		@Version
+		@Column(name = "ver")
+		private int version;
+
+		public Long getId() {
+			return id;
+		}
+
+		public void setId(Long id) {
+			this.id = id;
+		}
+
+		public int getVersion() {
+			return version;
+		}
+
+		public void setVersion(int version) {
+			this.version = version;
+		}
+	}
+
+	@Entity(name = "C")
+	public static class C {
+		@Id
+		@GeneratedValue
+		private long id;
+
+		@Version
+		@Column(name = "ver")
+		private Integer version;
+
+		public Long getId() {
+			return id;
+		}
+
+		public void setId(Long id) {
+			this.id = id;
+		}
+	}
+
+	@Entity(name = "D")
+	public static class D {
+		@Id
+		@GeneratedValue
+		private long id;
+
+		@Version
+		@Column(name = "ver")
+		private Timestamp version;
+
+		public Long getId() {
+			return id;
+		}
+
+		public void setId(Long id) {
+			this.id = id;
+		}
+	}
+}

hibernate-core/src/test/java/org/hibernate/orm/test/exceptionhandling/StaleVersionedObjectMergeTest.java

@@ -0,0 +1,233 @@
+/*
+ * Hibernate, Relational Persistence for Idiomatic Java
+ *
+ * License: GNU Lesser General Public License (LGPL), version 2.1 or later
+ * See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html
+ */
+package org.hibernate.orm.test.exceptionhandling;
+
+import jakarta.persistence.Column;
+import jakarta.persistence.Entity;
+import jakarta.persistence.EntityExistsException;
+import jakarta.persistence.Id;
+import jakarta.persistence.OptimisticLockException;
+import jakarta.persistence.Version;
+import org.hibernate.dialect.H2Dialect;
+import org.hibernate.testing.TestForIssue;
+import org.hibernate.testing.orm.junit.DomainModel;
+import org.hibernate.testing.orm.junit.RequiresDialect;
+import org.hibernate.testing.orm.junit.SessionFactory;
+import org.hibernate.testing.orm.junit.SessionFactoryScope;
+import org.junit.jupiter.api.Test;
+
+import java.sql.Timestamp;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+/**
+ * @author Jan Schatteman
+ */
+@TestForIssue( jiraKey = "HHH-1661")
+@RequiresDialect( H2Dialect.class )
+public class StaleVersionedObjectMergeTest {
+
+	@DomainModel(
+			annotatedClasses = { A.class }
+	)
+	@SessionFactory
+	@Test
+	public void testStaleNonVersionEntityMerged(SessionFactoryScope scope) {
+		A a = new A();
+		a.id = 3;
+		scope.inTransaction(
+				session -> session.persist( a )
+		);
+
+		scope.inTransaction(
+				session -> {
+					A aGet = session.get( A.class, a.getId() );
+					session.remove( aGet );
+				}
+		);
+
+		scope.inTransaction(
+				session -> session.merge( a )
+		);
+	}
+
+	@DomainModel(
+			annotatedClasses = { B.class }
+	)
+	@SessionFactory
+	@Test
+	public void testStalePrimitiveVersionEntityMerged(SessionFactoryScope scope) {
+		B b = new B();
+		b.id = 3;
+		scope.inTransaction(
+				session -> session.persist( b )
+		);
+
+		scope.inTransaction(
+				session -> {
+					B aGet = session.get( B.class, b.getId() );
+					session.remove( aGet );
+				}
+		);
+
+		// we have no way to detect that the instance
+		// was removed so it is treated as new
+		scope.inTransaction(
+				session -> session.merge( b )
+		);
+	}
+
+	@DomainModel(
+			annotatedClasses = { C.class }
+	)
+	@SessionFactory
+	@Test
+	public void testStalePrimitiveAndWrapperVersionEntityMerged(SessionFactoryScope scope) {
+		C b = new C();
+		b.id = 5;
+		b.version = 1;
+		assertThrows(
+				EntityExistsException.class,
+				() -> {
+					scope.inTransaction(
+							session -> session.persist( b )
+					);
+				}
+		);
+
+		C c = new C();
+		c.id = 6;
+		scope.inTransaction(
+				session -> session.persist( c )
+		);
+
+		scope.inTransaction(
+				session -> {
+					C cGet = session.get( C.class, c.getId() );
+					session.remove( cGet );
+				}
+		);
+
+		assertThrows(
+				OptimisticLockException.class,
+				() -> {
+					scope.inTransaction(
+							session -> {
+								session.merge( c );
+							}
+					);
+				}
+		);
+	}
+
+	@DomainModel(
+			annotatedClasses = { D.class }
+	)
+	@SessionFactory
+	@Test
+	public void testStaleTimestampVersionEntityMerged(SessionFactoryScope scope) {
+		D d = new D();
+		d.id = 8;
+		scope.inTransaction(
+				session -> session.persist( d )
+		);
+
+		scope.inTransaction(
+				session -> {
+					D dGet = session.get( D.class, d.getId() );
+					session.remove( dGet );
+				}
+		);
+
+		assertThrows(
+				OptimisticLockException.class,
+				() -> {
+					scope.inTransaction(
+							session -> {
+								session.merge( d );
+							}
+					);
+				}
+		);
+	}
+
+	@Entity(name = "A")
+	public static class A {
+		@Id
+		private long id;
+
+		public Long getId() {
+			return id;
+		}
+
+		public void setId(Long id) {
+			this.id = id;
+		}
+	}
+
+	@Entity(name = "B")
+	public static class B {
+		@Id
+		private long id;
+
+		@Version
+		@Column(name = "ver")
+		private int version;
+
+		public Long getId() {
+			return id;
+		}
+
+		public void setId(Long id) {
+			this.id = id;
+		}
+
+		public int getVersion() {
+			return version;
+		}
+
+		public void setVersion(int version) {
+			this.version = version;
+		}
+	}
+
+	@Entity(name = "C")
+	public static class C {
+		@Id
+		private long id;
+
+		@Version
+		@Column(name = "ver")
+		private Integer version;
+
+		public Long getId() {
+			return id;
+		}
+
+		public void setId(Long id) {
+			this.id = id;
+		}
+	}
+
+	@Entity(name = "D")
+	public static class D {
+		@Id
+		private long id;
+
+		@Version
+		@Column(name = "ver")
+		private Timestamp version;
+
+		public Long getId() {
+			return id;
+		}
+
+		public void setId(Long id) {
+			this.id = id;
+		}
+	}
+}

hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ops/MergeNewTest.java

@@ -6,12 +6,15 @@
  */
 package org.hibernate.orm.test.jpa.ops;
 
+import jakarta.persistence.OptimisticLockException;
+import org.hibernate.StaleObjectStateException;
 import org.hibernate.testing.orm.junit.EntityManagerFactoryScope;
 import org.hibernate.testing.orm.junit.Jpa;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;
 
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 /**
  * @author Emmanuel Bernard
@@ -78,8 +81,14 @@ public class MergeNewTest {
 
 		scope.inTransaction(
 				entityManager -> {
-					entityManager.merge( load );
+					try {
-					entityManager.flush();
+						entityManager.merge( load );
+						entityManager.flush();
+					}
+					catch (OptimisticLockException e) {
+						//expected since object can be inferred detached
+						assertTrue( e.getCause() instanceof StaleObjectStateException );
+					}
 				}
 		);
 	}

hibernate-core/src/test/java/org/hibernate/orm/test/ops/MergeTest.java

@@ -9,6 +9,8 @@ package org.hibernate.orm.test.ops;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
+
+import jakarta.persistence.OptimisticLockException;
 import jakarta.persistence.PersistenceException;
 import jakarta.persistence.criteria.CriteriaBuilder;
 import jakarta.persistence.criteria.CriteriaQuery;
@@ -33,6 +35,7 @@ import static org.hibernate.testing.orm.junit.ExtraAssertions.assertTyping;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotSame;
 import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
@@ -769,7 +772,10 @@ public class MergeTest extends AbstractOperationTestCase {
 					session.clear();
 					jboss.setVers( 1 );
 					session.getTransaction().begin();
-					session.merge( jboss );
+					assertThrows(
+							OptimisticLockException.class,
+							() -> session.merge( jboss )
+					);
 				}
 		);
 

migration-guide.adoc

@@ -57,3 +57,16 @@ Support for `array_contains()` to accept an array as element argument is depreca
 To check if an array is a subset of another array, use the `array_includes()` function,
 or the new `INCLUDES` predicate i.e. `array INCLUDES subarray`.
 
+
+[[merge-versioned-deleted]]
+=== Merge versioned entity when row is deleted
+Previously, merging a detached entity resulted in a SQL `insert` whenever there was no matching row in the database (for example, if the object had been deleted in another transaction).
+This behavior was unexpected and violated the rules of optimistic locking.
+
+An `OptimisticLockException` is now thrown when it is possible to determine that an entity is definitely detached, but there is no matching row.
+For this determination to be possible, the entity must have either:
+
+- a generated `@Id` field, or
+- a non-primitive `@Version` field.
+
+For entities which have neither, it's impossible to distinguish a new instance from a deleted detached instance, and there is no change from the previous behavior.