From c1d99ae77ed5b69b6955f075b777538ae1e3812d Mon Sep 17 00:00:00 2001
From: Jens Schauder <jschauder@pivotal.io>
Date: Thu, 2 May 2019 10:23:47 +0200
Subject: [PATCH] DATAJPA-1535 - When deleting an entity we no longer merge it
 if it doesn't exist in the database.

The merge happened to allow deleting of unmanaged entities.
As a side effect it triggered an insert for new (and thereby also unmanaged) entities.
By checking if the entity exists we avoid this now.

The check should NOT trigger an additional select since the select would have happened in the past anyway.

Original pull request: #378.
---
 .../support/SimpleJpaRepository.java          | 13 +++++++++
 ...lipseLinkNamespaceUserRepositoryTests.java |  7 +++++
 .../jpa/repository/UserRepositoryTests.java   | 15 ++++++++--
 .../support/SimpleJpaRepositoryUnitTests.java | 29 +++++++++++++++++++
 4 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java b/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java
index 708773fa7..e330f037c 100644
--- a/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java
+++ b/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java
@@ -168,6 +168,19 @@ public class SimpleJpaRepository<T, ID> implements JpaRepositoryImplementation<T
 	public void delete(T entity) {
 
 		Assert.notNull(entity, "The entity must not be null!");
+
+		Object id = entityInformation.getId(entity);
+		if (id == null) {
+			return;
+		}
+
+		T existing = em.find(entityInformation.getJavaType(), id);
+
+		// if the entity to be deleted doesn't exist, delete is a NOOP
+		if (existing == null) {
+			return;
+		}
+
 		em.remove(em.contains(entity) ? entity : em.merge(entity));
 	}
 
diff --git a/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java b/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java
index 7fb35b674..c77048f50 100644
--- a/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java
+++ b/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java
@@ -174,6 +174,13 @@ public class EclipseLinkNamespaceUserRepositoryTests extends NamespaceUserReposi
 		super.findByEmptyCollectionOfStrings();
 	}
 
+	@Override
+	@Test
+	@Ignore
+	public void savingUserThrowsAnException() {
+		super.savingUserThrowsAnException();
+	}
+
 	private void assumeNotEclipseLink2_7_2plus() {
 
 		Assume.assumeFalse("Empty collections seem to be broken in EclipseLink 2.7.2+",
diff --git a/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java b/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java
index d28602070..2b56b0605 100644
--- a/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java
+++ b/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java
@@ -48,12 +48,11 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.dao.DataAccessException;
+import org.springframework.dao.DataIntegrityViolationException;
 import org.springframework.dao.IncorrectResultSizeDataAccessException;
 import org.springframework.dao.InvalidDataAccessApiUsageException;
 import org.springframework.data.domain.Example;
 import org.springframework.data.domain.ExampleMatcher;
-import org.springframework.data.domain.ExampleMatcher.GenericPropertyMatcher;
-import org.springframework.data.domain.ExampleMatcher.StringMatcher;
 import org.springframework.data.domain.Page;
 import org.springframework.data.domain.PageImpl;
 import org.springframework.data.domain.PageRequest;
@@ -62,6 +61,7 @@ import org.springframework.data.domain.Slice;
 import org.springframework.data.domain.Sort;
 import org.springframework.data.domain.Sort.Direction;
 import org.springframework.data.domain.Sort.Order;
+import org.springframework.data.domain.ExampleMatcher.*;
 import org.springframework.data.jpa.domain.Specification;
 import org.springframework.data.jpa.domain.sample.Address;
 import org.springframework.data.jpa.domain.sample.Role;
@@ -2211,6 +2211,17 @@ public class UserRepositoryTests {
 		assertThat(repository.findByEmailNativeAddressJdbcStyleParameter("gierke@synyx.de")).isEqualTo(firstUser);
 	}
 
+	@Test() // DATAJPA-1535
+	public void savingUserThrowsAnException() {
+		// if this test fails this means deleteNewInstanceSucceedsByDoingNothing() might actually save the user without the test failing, which would be a bad thing.
+		assertThatThrownBy(() -> repository.save(new User())).isInstanceOf(DataIntegrityViolationException.class);
+	}
+
+	@Test // DATAJPA-1535
+	public void deleteNewInstanceSucceedsByDoingNothing() {
+		repository.delete(new User());
+	}
+
 	private Page<User> executeSpecWithSort(Sort sort) {
 
 		flushTestUsers();
diff --git a/src/test/java/org/springframework/data/jpa/repository/support/SimpleJpaRepositoryUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/support/SimpleJpaRepositoryUnitTests.java
index a300bd62d..32167b669 100644
--- a/src/test/java/org/springframework/data/jpa/repository/support/SimpleJpaRepositoryUnitTests.java
+++ b/src/test/java/org/springframework/data/jpa/repository/support/SimpleJpaRepositoryUnitTests.java
@@ -156,4 +156,33 @@ public class SimpleJpaRepositoryUnitTests {
 
 		verify(em).merge(attachedUser);
 	}
+
+	@Test // DATAJPA-1535
+	public void doNothingWhenNewInstanceGetsDeleted() {
+
+		User newUser = new User();
+		newUser.setId(null);
+
+		repo.delete(newUser);
+
+		verify(em, never()).find(any(Class.class), any(Object.class));
+		verify(em, never()).remove(newUser);
+		verify(em, never()).merge(newUser);
+	}
+
+	@Test // DATAJPA-1535
+	public void doNothingWhenNonExistantInstanceGetsDeleted() {
+
+		User newUser = new User();
+		newUser.setId(23);
+
+		when(information.isNew(newUser)).thenReturn(false);
+		when(em.find(User.class,23)).thenReturn(null);
+
+		repo.delete(newUser);
+
+		verify(em, never()).remove(newUser);
+		verify(em, never()).merge(newUser);
+	}
+
 }