From 7abb5e78a9e860fc6e1780d8b1b97b2bb21e6452 Mon Sep 17 00:00:00 2001 From: Yanming Zhou Date: Tue, 19 Mar 2024 15:56:20 +0800 Subject: [PATCH 1/3] Test deleting dirty detached and managed versioned entity --- .../data/jpa/domain/sample/VersionedUser.java | 11 ++++ .../support/JpaRepositoryTests.java | 61 +++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/domain/sample/VersionedUser.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/domain/sample/VersionedUser.java index 5e5927e1e4..a8651b3189 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/domain/sample/VersionedUser.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/domain/sample/VersionedUser.java @@ -22,6 +22,7 @@ /** * @author Oliver Gierke + * @author Yanming Zhou */ @Entity public class VersionedUser { @@ -33,6 +34,8 @@ public class VersionedUser { @Version private Long version; + private String name; + public Long getId() { return id; } @@ -48,4 +51,12 @@ public Long getVersion() { public void setVersion(Long version) { this.version = version; } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } } diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/JpaRepositoryTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/JpaRepositoryTests.java index 27daa255a8..cf4878d6fa 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/JpaRepositoryTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/JpaRepositoryTests.java @@ -18,22 +18,30 @@ import static org.assertj.core.api.Assertions.*; import jakarta.persistence.EntityManager; +import jakarta.persistence.OptimisticLockException; import jakarta.persistence.PersistenceContext; import java.util.Arrays; import java.util.Iterator; import java.util.List; +import java.util.Map; + +import javax.sql.DataSource; import org.jetbrains.annotations.NotNull; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.data.jpa.domain.sample.PersistableWithIdClass; import org.springframework.data.jpa.domain.sample.PersistableWithIdClassPK; import org.springframework.data.jpa.domain.sample.SampleEntity; import org.springframework.data.jpa.domain.sample.SampleEntityPK; +import org.springframework.data.jpa.domain.sample.VersionedUser; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.data.repository.CrudRepository; +import org.springframework.jdbc.core.namedparam.NamedParameterJdbcOperations; +import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit.jupiter.SpringExtension; import org.springframework.transaction.annotation.Transactional; @@ -46,21 +54,28 @@ * @author Jens Schauder * @author Greg Turnquist * @author Krzysztof Krason + * @author Yanming Zhou */ @ExtendWith(SpringExtension.class) @ContextConfiguration({ "classpath:infrastructure.xml" }) @Transactional class JpaRepositoryTests { + @Autowired DataSource dataSource; + @PersistenceContext EntityManager em; private JpaRepository repository; private CrudRepository idClassRepository; + private JpaRepository versionedUserRepository; + private NamedParameterJdbcOperations jdbcOperations; @BeforeEach void setUp() { repository = new JpaRepositoryFactory(em).getRepository(SampleEntityRepository.class); idClassRepository = new JpaRepositoryFactory(em).getRepository(SampleWithIdClassRepository.class); + versionedUserRepository = new JpaRepositoryFactory(em).getRepository(VersionedUserRepository.class); + jdbcOperations = new NamedParameterJdbcTemplate(dataSource); } @Test @@ -162,6 +177,48 @@ public Iterator iterator() { assertThat(repository.findAll()).containsExactly(two); } + @Test + void deleteDirtyDetachedVersionedEntityShouldRaiseOptimisticLockException() { + + VersionedUser entity = new VersionedUser(); + entity.setName("name"); + versionedUserRepository.save(entity); + versionedUserRepository.flush(); + em.detach(entity); + + versionedUserRepository.findById(entity.getId()).ifPresent(u -> { + u.setName("new name"); + versionedUserRepository.flush(); + }); + + assertThatExceptionOfType(OptimisticLockException.class).isThrownBy(() -> { + versionedUserRepository.delete(entity); + versionedUserRepository.flush(); + }); + + jdbcOperations.update("delete from VersionedUser", Map.of()); + } + + @Test + void deleteDirtyManagedVersionedEntityShouldRaiseOptimisticLockException() { + + VersionedUser entity = new VersionedUser(); + entity.setName("name"); + versionedUserRepository.save(entity); + versionedUserRepository.flush(); + + + assertThat(jdbcOperations.update("update VersionedUser set version=version+1 where id=:id", + Map.of("id", entity.getId()))).isEqualTo(1); + + assertThatExceptionOfType(OptimisticLockException.class).isThrownBy(() -> { + versionedUserRepository.delete(entity); + versionedUserRepository.flush(); + }); + + jdbcOperations.update("delete from VersionedUser", Map.of()); + } + private interface SampleEntityRepository extends JpaRepository { } @@ -170,4 +227,8 @@ private interface SampleWithIdClassRepository extends CrudRepository { } + + private interface VersionedUserRepository extends JpaRepository { + + } } From d334baaa73bff6c1a3f582835f065ad3ed2df760 Mon Sep 17 00:00:00 2001 From: Yanming Zhou Date: Tue, 19 Mar 2024 15:07:55 +0800 Subject: [PATCH 2/3] Introduce JpaEntityInformation.getVersionAttribute() --- .../data/jpa/repository/support/JpaEntityInformation.java | 7 +++++++ .../repository/support/JpaMetamodelEntityInformation.java | 8 +++++++- .../support/JpaEntityInformationSupportUnitTests.java | 7 +++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/JpaEntityInformation.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/JpaEntityInformation.java index 096bd77d21..d38332ac57 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/JpaEntityInformation.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/JpaEntityInformation.java @@ -19,6 +19,7 @@ import java.util.Collection; import java.util.Map; +import java.util.Optional; import org.springframework.data.jpa.repository.query.JpaEntityMetadata; import org.springframework.data.repository.core.EntityInformation; @@ -30,6 +31,7 @@ * @author Oliver Gierke * @author Thomas Darimont * @author Mark Paluch + * @author Yanming Zhou */ public interface JpaEntityInformation extends EntityInformation, JpaEntityMetadata { @@ -39,6 +41,11 @@ public interface JpaEntityInformation extends EntityInformation, J @Nullable SingularAttribute getIdAttribute(); + /** + * Returns the version attribute of the entity. + */ + Optional> getVersionAttribute(); + /** * Returns the required identifier type. * diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/JpaMetamodelEntityInformation.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/JpaMetamodelEntityInformation.java index 9979fc773b..a508a45456 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/JpaMetamodelEntityInformation.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/JpaMetamodelEntityInformation.java @@ -47,7 +47,7 @@ /** * Implementation of {@link org.springframework.data.repository.core.EntityInformation} that uses JPA {@link Metamodel} - * to find the domain class' id field. + * to find the domain class' id and version field. * * @author Oliver Gierke * @author Thomas Darimont @@ -55,6 +55,7 @@ * @author Mark Paluch * @author Jens Schauder * @author Greg Turnquist + * @author Yanming Zhou */ public class JpaMetamodelEntityInformation extends JpaEntityInformationSupport { @@ -191,6 +192,11 @@ public Class getIdType() { return idMetadata.getSimpleIdAttribute(); } + @Override + public Optional> getVersionAttribute() { + return versionAttribute; + } + @Override public boolean hasCompositeId() { return !idMetadata.hasSimpleId(); diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/JpaEntityInformationSupportUnitTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/JpaEntityInformationSupportUnitTests.java index e66e624b12..df7522a65e 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/JpaEntityInformationSupportUnitTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/JpaEntityInformationSupportUnitTests.java @@ -29,6 +29,7 @@ import java.util.Collection; import java.util.Collections; import java.util.Map; +import java.util.Optional; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -42,6 +43,7 @@ * * @author Oliver Gierke * @author Jens Schauder + * @author Yanming Zhou */ @ExtendWith(MockitoExtension.class) @MockitoSettings(strictness = Strictness.LENIENT) @@ -88,6 +90,11 @@ static class DummyJpaEntityInformation extends JpaEntityInformationSuppor return null; } + @Override + public Optional> getVersionAttribute() { + return Optional.empty(); + } + @Override public ID getId(T entity) { return null; From 3a17e8033b182012395707b3bd500d1b43018053 Mon Sep 17 00:00:00 2001 From: Yanming Zhou Date: Tue, 19 Mar 2024 10:03:59 +0800 Subject: [PATCH 3/3] Eliminate unnecessary merge() if entity is non-versioned Fix GH-3401 --- .../support/SimpleJpaRepository.java | 8 ++++++- .../support/JpaRepositoryTests.java | 21 ++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java index 31abc19187..67e437917c 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java @@ -195,7 +195,13 @@ public void delete(T entity) { return; } - entityManager.remove(entityManager.contains(entity) ? entity : entityManager.merge(entity)); + if (entityInformation.getVersionAttribute().isPresent()) { + // call merge() to raise ObjectOptimisticLockingFailureException if entity is stale + entityManager.remove(entityManager.contains(entity) ? entity : entityManager.merge(entity)); + } + else { + entityManager.remove(existing); + } } @Override diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/JpaRepositoryTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/JpaRepositoryTests.java index cf4878d6fa..a67167833c 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/JpaRepositoryTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/JpaRepositoryTests.java @@ -16,6 +16,9 @@ package org.springframework.data.jpa.repository.support; import static org.assertj.core.api.Assertions.*; +import static org.mockito.BDDMockito.then; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.reset; import jakarta.persistence.EntityManager; import jakarta.persistence.OptimisticLockException; @@ -32,6 +35,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mockito; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.data.jpa.domain.sample.PersistableWithIdClass; import org.springframework.data.jpa.domain.sample.PersistableWithIdClassPK; @@ -65,6 +69,8 @@ class JpaRepositoryTests { @PersistenceContext EntityManager em; + private EntityManager spiedEntityManager; + private JpaRepository repository; private CrudRepository idClassRepository; private JpaRepository versionedUserRepository; @@ -72,7 +78,8 @@ class JpaRepositoryTests { @BeforeEach void setUp() { - repository = new JpaRepositoryFactory(em).getRepository(SampleEntityRepository.class); + spiedEntityManager = Mockito.spy(em); + repository = new JpaRepositoryFactory(spiedEntityManager).getRepository(SampleEntityRepository.class); idClassRepository = new JpaRepositoryFactory(em).getRepository(SampleWithIdClassRepository.class); versionedUserRepository = new JpaRepositoryFactory(em).getRepository(VersionedUserRepository.class); jdbcOperations = new NamedParameterJdbcTemplate(dataSource); @@ -219,6 +226,18 @@ void deleteDirtyManagedVersionedEntityShouldRaiseOptimisticLockException() { jdbcOperations.update("delete from VersionedUser", Map.of()); } + @Test //GH-3401 + void deleteNonVersionedEntityShouldNotInvokeMerge() { + SampleEntity entity = new SampleEntity("one", "eins"); + repository.save(entity); + repository.flush(); + em.detach(entity); + + reset(spiedEntityManager); + repository.delete(entity); + then(spiedEntityManager).should(never()).merge(entity); + } + private interface SampleEntityRepository extends JpaRepository { }