Eliminate unnecessary merge() if entity is non-versioned

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<T, ID> extends EntityInformation<T, ID>, JpaEntityMetadata<T> {
 
@@ -39,6 +41,11 @@ public interface JpaEntityInformation<T, ID> extends EntityInformation<T, ID>, J
 	@Nullable
 	SingularAttribute<? super T, ?> getIdAttribute();
 
+	/**
+	 * Returns the version attribute of the entity.
+	 */
+	Optional<SingularAttribute<? super T, ?>> getVersionAttribute();
+
 	/**
 	 * Returns the required identifier type.
 	 *

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/JpaMetamodelEntityInformation.java

@@ -47,14 +47,15 @@
 
 /**
  * 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
  * @author Christoph Strobl
  * @author Mark Paluch
  * @author Jens Schauder
  * @author Greg Turnquist
+ * @author Yanming Zhou
  */
 public class JpaMetamodelEntityInformation<T, ID> extends JpaEntityInformationSupport<T, ID> {
 
@@ -191,6 +192,11 @@ public Class<ID> getIdType() {
 		return idMetadata.getSimpleIdAttribute();
 	}
 
+	@Override
+	public Optional<SingularAttribute<? super T, ?>> getVersionAttribute() {
+		return versionAttribute;
+	}
+
 	@Override
 	public boolean hasCompositeId() {
 		return !idMetadata.hasSimpleId();

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

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;
+	}
 }

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<T, ID> extends JpaEntityInformationSuppor
 			return null;
 		}
 
+		@Override
+		public Optional<SingularAttribute<? super T, ?>> getVersionAttribute() {
+			return Optional.empty();
+		}
+
 		@Override
 		public ID getId(T entity) {
 			return null;

spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/JpaRepositoryTests.java

@@ -16,24 +16,36 @@
 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;
 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.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;
 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 +58,31 @@
  * @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 EntityManager spiedEntityManager;
+
 	private JpaRepository<SampleEntity, SampleEntityPK> repository;
 	private CrudRepository<PersistableWithIdClass, PersistableWithIdClassPK> idClassRepository;
+	private JpaRepository<VersionedUser, Long> versionedUserRepository;
+	private NamedParameterJdbcOperations jdbcOperations;
 
 	@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);
 	}
 
 	@Test
@@ -162,6 +184,60 @@ public Iterator<SampleEntityPK> 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());
+	}
+
+	@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<SampleEntity, SampleEntityPK> {
 
 	}
@@ -170,4 +246,8 @@ private interface SampleWithIdClassRepository
 			extends CrudRepository<PersistableWithIdClass, PersistableWithIdClassPK> {
 
 	}
+
+	private interface VersionedUserRepository extends JpaRepository<VersionedUser, Long> {
+
+	}
 }