From 522269e9a93f71af593f1d94039df5ade4f93617 Mon Sep 17 00:00:00 2001 From: Jan Schatteman Date: Mon, 31 Oct 2022 22:15:41 +0100 Subject: [PATCH] HHH-1661 throw when merge() applied to a definitely-removed instance group effort by @jrenaat, @beikov, and myself Signed-off-by: Jan Schatteman Signed-off-by: Gavin King --- .../boot/model/internal/PropertyBinder.java | 2 - .../engine/internal/UnsavedValueFactory.java | 18 +- .../internal/DefaultMergeEventListener.java | 25 +- ...hedCascadedCollectionInEmbeddableTest.java | 24 +- .../StaleObjectMergeTest.java | 228 +++++++++++++++++ .../StaleVersionedObjectMergeTest.java | 233 ++++++++++++++++++ .../orm/test/jpa/ops/MergeNewTest.java | 13 +- .../org/hibernate/orm/test/ops/MergeTest.java | 8 +- migration-guide.adoc | 13 + 9 files changed, 541 insertions(+), 23 deletions(-) create mode 100644 hibernate-core/src/test/java/org/hibernate/orm/test/exceptionhandling/StaleObjectMergeTest.java create mode 100644 hibernate-core/src/test/java/org/hibernate/orm/test/exceptionhandling/StaleVersionedObjectMergeTest.java diff --git a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/PropertyBinder.java b/hibernate-core/src/main/java/org/hibernate/boot/model/internal/PropertyBinder.java index 16a42d49be..99a7fe4447 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/model/internal/PropertyBinder.java +++ b/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(); diff --git a/hibernate-core/src/main/java/org/hibernate/engine/internal/UnsavedValueFactory.java b/hibernate-core/src/main/java/org/hibernate/engine/internal/UnsavedValueFactory.java index 6460cbe7ed..db95f99fdb 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/internal/UnsavedValueFactory.java +++ b/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() { } } diff --git a/hibernate-core/src/main/java/org/hibernate/event/internal/DefaultMergeEventListener.java b/hibernate-core/src/main/java/org/hibernate/event/internal/DefaultMergeEventListener.java index ff655326c6..ff05ca73e5 100644 --- a/hibernate-core/src/main/java/org/hibernate/event/internal/DefaultMergeEventListener.java +++ b/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 - // really persistent - entityIsTransient( event, clonedIdentifier, copyCache ); + // with an assigned id and no version was detached, + // when it was really transient (or deleted) + 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! diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/merge/MergeDetachedCascadedCollectionInEmbeddableTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/merge/MergeDetachedCascadedCollectionInEmbeddableTest.java index 8c738f9267..849e8606f3 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/bytecode/enhancement/merge/MergeDetachedCascadedCollectionInEmbeddableTest.java +++ b/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 -> { - heading.name = "updated"; - Heading headingMerged = (Heading) session.merge( heading ); - assertNotSame( heading, headingMerged ); - assertNotSame( heading.grouping, headingMerged.grouping ); - assertNotSame( heading.grouping.things, headingMerged.grouping.things ); - } ); + try { + scope.inTransaction(session -> { + heading.name = "updated"; + Heading headingMerged = session.merge(heading); + assertNotSame(heading, headingMerged); + 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") diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/exceptionhandling/StaleObjectMergeTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/exceptionhandling/StaleObjectMergeTest.java new file mode 100644 index 0000000000..1f173b5ceb --- /dev/null +++ b/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; + } + } +} diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/exceptionhandling/StaleVersionedObjectMergeTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/exceptionhandling/StaleVersionedObjectMergeTest.java new file mode 100644 index 0000000000..26d0f3f7a6 --- /dev/null +++ b/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; + } + } +} diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ops/MergeNewTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ops/MergeNewTest.java index d0b851ff35..e3899aedea 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ops/MergeNewTest.java +++ b/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 ); - entityManager.flush(); + try { + entityManager.merge( load ); + entityManager.flush(); + } + catch (OptimisticLockException e) { + //expected since object can be inferred detached + assertTrue( e.getCause() instanceof StaleObjectStateException ); + } } ); } diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/ops/MergeTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/ops/MergeTest.java index 6222c4d6d5..e0156cb4ce 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/ops/MergeTest.java +++ b/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 ) + ); } ); diff --git a/migration-guide.adoc b/migration-guide.adoc index 7bad67b268..45d3d8e207 100644 --- a/migration-guide.adoc +++ b/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.