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

[oxahex]

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?