I'm curious about why SimpleJpaRepository checks twice whether the entity is in a persistent state when calling the delete method. about spring-data-jpa HOT 14 OPEN

@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?

from spring-data-jpa.

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.

from spring-data-jpa.

@schauder How about this improvement?

from spring-data-jpa.

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.

from spring-data-jpa.

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.

from spring-data-jpa.

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.

from spring-data-jpa.

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.

from spring-data-jpa.

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().

from spring-data-jpa.

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

from spring-data-jpa.

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

from spring-data-jpa.

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.

from spring-data-jpa.

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!

from spring-data-jpa.

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?

from spring-data-jpa.

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.

from spring-data-jpa.