I'm curious about why SimpleJpaRepository checks twice whether the entity is in a persistent state when calling the delete method.

[oxahex]

trafficstars

I think if the entity was retrieved using the find() in the delete method of SimpleJpaRepository, it can be considered as already managed by the persistence context.

However, I'm curious if there is a need to check if the entity exists in the persistence context using the contains() method before executing the remove() method below.

@Override
	@Transactional
	@SuppressWarnings("unchecked")
	public void delete(T entity) {

		Assert.notNull(entity, "Entity must not be null");

		if (entityInformation.isNew(entity)) {
			return;
		}

		Class<?> type = ProxyUtils.getUserClass(entity);

		T existing = (T) entityManager.find(type, entityInformation.getId(entity));

		// if the entity to be deleted doesn't exist, delete is a NOOP
		if (existing == null) {
			return;
		}

		entityManager.remove(entityManager.contains(entity) ? entity : entityManager.merge(entity));
	}

If so, isn't it unnecessary to call contains()?

[schauder]

I think this somewhat convoluted process is necessary for cases where entity was modified, before the call to delete, and/or to make sure optimistic locking works as intended.

Consider this:

1. entity gets loaded, and detached.
2. the same row in the database gets modified in the database by some other process, updating the version attribute.
3. delete(entity) gets called.

In this case the delete should fail, but if we use entityManager.remove(entityManger.find(..)) instead, it will actually succeed.

On the other hand, if we don't check if it exists first, calling delete on a new entity will actually persist it, which probably isn't what was intended.

[quaff]

@schauder How about this improvement?

[quaff]

I think this somewhat convoluted process is necessary for cases where entity was modified, before the call to delete, and/or to make sure optimistic locking works as intended.

Consider this:

1. entity gets loaded, and detached.
2. the same row in the database gets modified in the database by some other process, updating the version attribute.
3. delete(entity) gets called.

In this case the delete should fail, but if we use `entityManager.remove(entityManger.find(..)) instead, it will actually succeed.

On the other hand, if we don't check if it exists first, calling delete on a new entity will actually persist it, which probably isn't what was intended.

it still cannot prevent throwing OptimisticLockingFailureException.

1. merge(entity) gets called.
2. the same row in the database gets modified in the database by some other process, updating the version attribute.
3. delete(entity) gets called will throw OptimisticLockingFailureException.

OptimisticLockingFailureException should be thrown for safety if optimistic lock failed.

[schauder]

OptimisticLockingFailureException should be thrown for safety if optimistic lock failed.

That's the point: If we don't use the entity passed into the method we might end up doing a delete which should fail with an optimistic locking exception.

[quaff]

That's the point: If we don't use the entity passed into the method we might end up doing a delete which should fail with an optimistic locking exception.

OptimisticLockingFailureException may be thrown no matter merge() is called or not.

[schauder]

If you want to use the positive result of the find call as an indication that the entity in question is managed, you'd have to use existing as an argument for remove which would not throw an optimistic locking exception in the scenario I out lined.

[quaff]

If you want to use the positive result of the find call as an indication that the entity in question is managed, you'd have to use existing as an argument for remove which would not throw an optimistic locking exception in the scenario I out lined.

Using existing instead of entity may throw OptimisticLockingFailureException also if the database row is updated between find() and remove().

[schauder]

Yes, but it won't throw an exception when the change happend before the find

[quaff]

So is it worthy to perform find() and merge() then remove() since the exception can not be avoided completely?

[quaff]

I find out that merge gets called to avoid InvalidDataAccessApiUsageException not OptimisticLockingFailureException, because there is no way to reattach a stale detached entity in JPA, see https://stackoverflow.com/a/4438358.

Here is failed tests:

[ERROR] Errors: 
[ERROR]   JavaConfigUserRepositoryTests>UserRepositoryTests.removeDetachedObject:629 » InvalidDataAccessApiUsage Removing a detached instance org.springframework.data.jpa.domain.sample.User#756
[ERROR]   NamespaceUserRepositoryTests>UserRepositoryTests.removeDetachedObject:629 » InvalidDataAccessApiUsage Removing a detached instance org.springframework.data.jpa.domain.sample.User#756
[ERROR]   UserRepositoryTests.removeDetachedObject:629 » InvalidDataAccessApiUsage Removing a detached instance org.springframework.data.jpa.domain.sample.User#757
[ERROR]   JpaRepositoryTests.testCrudOperationsForCompoundKeyEntity:76 » IllegalArgument Removing a detached instance org.springframework.data.jpa.domain.sample.SampleEntity#org.springframework.data.jpa.domain.sample.SampleEntityPK@5e1258

The fix is very simple, I've updated the PR.

[schauder]

Nobody said that merge gets called to avoid OptimisticLockingFailureException. The opposite is true. In the situation described in my first comment we must have an OptimisticLockingFailureException!

[oxahex]

Issue title revised for clarity as it seemed unclear and caused confusion regarding the topic I intended to inquire about.

I agree with your explanation. @schauder

Probably, your initial response regarding the necessity of the merge() method was in response to my inquiry. There seems to have been some confusion there. What I was curious about was why the contains() method is called again at the point of invoking the remove() method, even though the entity is already considered to be in a persistent state when the entityManager.find() method is called.

Now, let's consider another scenario: Threads A and B are running independently in separate transactions. Entity@v1 is already loaded into Thread A's persistence context, and Thread A is currently modifying it. At this point, Thread B attempts to delete the same entity.

1. Thread A: Loads and modifies the entity (Entity@v1 -> Entity@v2, stored in the persistence context).
2. Thread B: Attempts to delete the same entity by calling the delete() method.
3. Thread B: Inside the delete() method, entityManager.find() is used to retrieve the entity. Since Thread A's changes have not been committed, the entity's version in the database remains at v1. Therefore, existing is assigned Entity@v1 retrieved from the database.

In this case, although existing != null within the delete() method in Thread B, at the point where entityManager.contains() is called internally to check the persistent state, the entity has already been retrieved via the find() method into Thread B's persistence context or directly from the database.

Therefore, at the time of the remove() method call, the entity is indeed in a persistent state. If so, isn't it unnecessary to call contains()?

Is there a scenario where the return value of the contains() method is false even when the find() method has been called and existing != null?

[schauder]

Yes, when entity is in detached state. find will return a different instance and entity is still detached, i.e. contains(entity) will return false.

[oxahex]

@schauder I had thought that a detached entity transitions to a persistent state by the time find() is called based on the id field, but I realize now that this is incorrect. Therefore, just because the condition existing != null is satisfied, it doesn't mean that the entity is in a persistent state by the time contains() method is invoked. As you mentioned, it can return a different entity instance.

So, calling the contains() method ultimately serves as a kind of defensive strategy, doesn't it?