[DATAJPA-1535] Simple Repository delete of unexisting entity generates insert + delete SQL Created: 30/Apr/19  Updated: 05/Aug/19  Resolved: 21/May/19

Status: Closed
Project: Spring Data JPA
Component/s: Core
Affects Version/s: 2.1.6 (Lovelace SR6)
Fix Version/s: 2.2 RC1 (Moore), 2.1.9 (Lovelace SR9), 1.11.23 (Ingalls SR23)

Type: Bug Priority: Minor
Reporter: gpeel Assignee: Jens Schauder
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified
Environment:

windows, maven, intellij


Issue Links:
Duplicate
duplicates DATAJPA-1324 delete method CrudRepository when @Ve... Closed
Last updater: Mark Paluch
Pull Request URL: https://github.com/spring-projects/spring-data-jpa/pull/378
Sprint: Moore RC1

 Description   

With a simple Repository extending JpaRepository<>  a delete of an Entity not existing in the DB will generate 2 SQL request : an insert = a delete.

This seems dangerous, why create an Entity to delete it afterward?

It you have constraints your insert could fail, and the error would really be unclear ...

JPA behavior for remove() : you have to give a managed Entity, so you never have that case.

 

 

@Entity
public class Info {

 @GeneratedValue(strategy = GenerationType.AUTO)
 @Id
 private Long id;

 @Version
 private int version;

 private String message;
 // ....
}
 

 

public interface InfoRepository extends JpaRepository<Info, Long> {}

 

@RunWith(SpringRunner.class)
@SpringBootTest(classes = BusinessConfig.class)
public class InfoRepositoryTest {

 private @Autowired InfoRepository infoRepository;

 @Test
 public void remove_notExistEntity_Ko() {
 Info info = new Info("a new message");
 infoRepository.delete(info);
 }

}

Logs  for    infoRepository.delete(info);

DEBUG org.hibernate.SQL - insert into info (message, version, id) values (?, ?, ?)
TRACE o.h.type.descriptor.sql.BasicBinder - binding parameter [1] as [VARCHAR] - [a new message]
TRACE o.h.type.descriptor.sql.BasicBinder - binding parameter [2] as [INTEGER] - [0]
TRACE o.h.type.descriptor.sql.BasicBinder - binding parameter [3] as [BIGINT] - [1]
DEBUG org.hibernate.SQL - delete from info where id=? and version=?
TRACE o.h.type.descriptor.sql.BasicBinder - binding parameter [1] as [BIGINT] - [1]
TRACE o.h.type.descriptor.sql.BasicBinder - binding parameter [2] as [INTEGER] - [0]

 



 Comments   
Comment by Jens Schauder [ 02/May/19 ]

DATAJPA-1324 was closed, probably because we didn't get feedback on it (which should have resulted in a differnt resolution).

Comment by gpeel [ 02/May/19 ]

The repository impl code seems strange to me :

@Transactional
public void delete(T entity)

{ Assert.notNull(entity, "The entity must not be null!"); this.em.remove(this.em.contains(entity) ? entity : this.em.merge(entity)); }

 

// T.class is more difficult to extract from the environment but I would do something like ...

@Transactional
public void delete(T entity)

{ Object managed = em.find(T.class, entity.getId()); // Of course T.class does not work, just showing the idea here. Assert.notNull(managed, "The entity must exist in the database to remove it!"); this.em.remove(managed); }
Comment by Jens Schauder [ 02/May/19 ]

I think that is basically what I did in the PR.

Comment by gpeel [ 02/May/19 ]

I tested your PR, it solves the exact case I reported, but is not enough ...

This following example will still produce 2 SQL orders : insert + delete

@Test
public void remove_notExistEntity2_Ko() {
 Info info = new Info("a new message");
 info.setId(44444L);
 infoRepository.delete(info);
}

 

The core pb is the same : ie merge() is done on an entity which does not exists in the DB.
merge() should not be used for that.
If we designed from scratch the repo.remove() the goal is to delete an Entity which exists in the DB.
So we would try to find it, if it is not found => do nothing.
If the entity is found hten em.deleted(theFoundManagedEntity)

To find the right Entity we have to find the right match for ID + VERSION.

 

Comment by Jens Schauder [ 02/May/19 ]

That would add a select statement for every delete. I don't think we want to have that.

If there is a chance that you have an entity that is not new based on version and id, i.e. Spring Datas internal logic, but also doesn't exist in the database the question becomes: what is the meaning of that status at all?

I'd say in this scenario it is the task of the client to check that the value to be deleted actually exists.

Comment by gpeel [ 02/May/19 ]

If the entity is not there on (id+version) match, it is a normal case of OptimistLock failure. => so I think that case should be covered correctly.
To limit the select,  a test on em.contains() could be made.

 

I feel that the repo.remove(anyPresumableEntity) method is a friendly method to replace em.delete(aManagedEntity).

So if repo.remove(anyPresumableEntity)  is friendly, it does the check for the user, preventing him to fetch the managed Entity himself.

I could do the findByIdVersion() myself and use the entityManager, but it means that in RestController for each CRUD  I should add some code for the delete.

Comment by gpeel [ 13/May/19 ]

do I have to open a new issue for the other problem ?
AT the moment the repository.delete() is not safe.

it should detect Optimist Lock failure, whatever the nature of it :

  • id == null (wrong user input ...)
  •  id not in DB
  • id in db but version wrong
Comment by Jens Schauder [ 13/May/19 ]

As mentioned before: I don't think we want the delete to perform a select statement every time.

So if you want that behavior, please do it on the client side.

Comment by gpeel [ 13/May/19 ]

ok if you really gained one select almost every time. but this is not the case.

As you know merge WILL generate a select anyway in all the cases I would like to detect.

So there will no performance lost, and better safety.

 

Here is the way I extended the repositoryImpl to solve this issue in my code (it' probably not perfect for all cases, but works for my way of definig version and id)

BTW repo.merge() suffers from the same kind of failsafe problems.

 


@Override
@Transactional
public void delete(T entity) {
 checkIfEntityExistWithIdAndVersionOk(entity);
 super.delete(entity);

}

@Override
@Transactional
public T merge(T entity) {
 ID id = (ID) entityInformation.getId(entity);
 if (id == null) {
 return persist(entity);
 } else {
 return update(entity);
 }
}

/**
 * Added
 */ 
@Override
@Transactional
public T update(T entity) {
 checkIfEntityExistWithIdAndVersionOk(entity);
 return em.merge(entity);
}


/**
 * Added
 */
@Override
@Transactional
public T persist(T entity) {
 em.persist(entity);
 return entity;
}

private void checkIfEntityExistWithIdAndVersionOk(T entity) {
 ID id = (ID) entityInformation.getId(entity);
 if (id == null) {
 throw new javax.persistence.EntityNotFoundException("Entity Not Found : id == null");
 }

 Optional<T> entityFoundOptional = super.findById(id);
 if (!entityFoundOptional.isPresent()) {
 throw new ObjectOptimisticLockingFailureException(getDomainClass(), id);
 }

 Object[] array2 = em.getEntityManagerFactory().getMetamodel().getEntities().toArray();

 EntityType<?> entityTypeNow = null;
 for (Object o : array2) {
 EntityType entityType = (EntityType) o;
 if (entityType.getJavaType().equals(entity.getClass())) {
 entityTypeNow = entityType;
 break;
 }
 }
 SingularAttribute<?, ?> singular = extractVersionAttribute(entityTypeNow);
 Object valueVersionIn = getValue(entity, singular);
 Object valueVersionDb = getValue(entityFoundOptional.get(), singular);

 // check if entity has @Version attribute
 // Object id =
 // entityManagerFactory.getPersistenceUnitUtil().getIdentifier(entity);

 if (!valueVersionIn.equals(valueVersionDb)) {
 throw new ObjectOptimisticLockingFailureException(getDomainClass(), id);
 }
}
Comment by Jens Schauder [ 17/May/19 ]

gpeel I think you are right. I updated the PR. Could you give that a try?

 

Comment by Oliver Drotbohm [ 05/Jul/19 ]

The changes made for this ticket is causing trouble in the following scenario:

@Transactional
void someTest() {
  Entity entity = repo.save(entity);
  repo.delete(entity);

  assertThat(repo.existsById(entity.getId())).isFalse();
}

Before the changes made, this code succeeded. After the change it does not anymore as the delete is not actually executed anymore.

Can the original poster elaborate on why one would call delete(…) for an instance that has not been persisted before at all? The previous behavior was assuming that the instance handed to the delete(…) method was managed but detached, e.g. an instance that has been detached due to some transactional boundary closed, some manipulations e.g. in a Spring MVC controller and a final call to delete(…) on the managed but detached instance. For that, the call to merge(…) was necessary to attach the instance to the persistence context again.

I am not sure we're doing ourselves a favor trying to be clever to avoid database interactions on top of all the batching that's already in place by the JPA provider. If we do we're now responsible for all the anomalies we create and can't argue "That's just the way JPA works."

I vote for rolling back this change as the code presented in the first place is totally non-idiomatic in the first place.

Comment by Jens Schauder [ 08/Jul/19 ]

One would get this kind of scenario when one deletes an entity via a web front end and clicks twice on delete, either because the front end is sluggish or because two people try the same thing.

Comment by Oliver Drotbohm [ 08/Jul/19 ]

I'm not sure I follow. Wouldn't you just issue a delete by id for that? Even if you load the entity first, it is only detached, not unmanaged.

Comment by gpeel [ 08/Jul/19 ]

With a web front, you can do both :

  • a delete by id and version (I personaly do not like the pure delete by id because you miss the OptimistLock contract)
  • a delete by entity, you receive from the web the full object.

Respecting the OptimistLock strategy really doesn't seem an option to me. 
I would think that Deleting or updating without a control on the version is a bad habit.

Comment by Oliver Drotbohm [ 08/Jul/19 ]

a delete by entity, you receive from the web the full object.

That doesn't make sense. Either you need to merge the data submitted first, which requires a merge anyway. Then you have a managed object again.
If you don't need the data merged, you can load the entity by id and have a detached instance as well.

To me, all of this sounds like an effect you get from not properly separating UI DTOs from entities. I don't think we should make compromises on the backend to cater that use case.

Comment by Jens Schauder [ 08/Jul/19 ]

Yes, if you want the record to be gone no matter what.

But if you want to make sure the user saw the last version before deleting it you'd would want to use optimistic locking.

As an example:
Imagine you review old issues in order to delete stale ones.
Also the deleting transaction writes a log about the event.

Then you want a delete to go through only when
a) it actually happens, i.e. the row wasn't deleted before by someone else
b) and it is still in the state that you saw when you decided that it should get deleted.

Comment by Oliver Drotbohm [ 08/Jul/19 ]

I agree, that would still imply you need to obtain a managed instance, apply the version you receive by the client and then trying to delete that instance. Again: this is about a managed but detached instance.

From EntityManager.remove(…)'s Javadoc:

@throws IllegalArgumentException if the instance is not an entity or is a detached entity.

I.e. what's handed to the method has to be already managed or merged beforehand. That's exactly how the method was implemented before.

Comment by Oliver Drotbohm [ 09/Jul/19 ]

A bit more investigation revealed that to produce the problem I observed a couple of more aspects need to be considered:

  • my entity was using manual identifier assignments with a Persistable.isNew() implementation using a transient flag originally set to true but currently changed to false in an @PostPersist method. That method only get's called on actual flush, so that the entity still has isNew() returning true when returning from CrudRepository.save(…).
  • that in turn causes the new guard introduced in CrudRepository.delete(…) using the EntityInformation to result in an early return (as the instance is considered new) so that EntityManager.remove(…) is never called.
  • the subsequent call to existsById(…) then only flushes the persist operation and returns true unexpectedly.

A possible workaround is to change my implementation to rather use the @PrePersist callback to switch the flag as that gets called on invocation of EntityManager.persist(…). I guess we could argue that this is the right thing to do anyway. Further discussion with Jens Schauder scheduled for tomorrow.

Comment by Kristina Heckelmann [ 16/Jul/19 ]

The fix of this bug breaks the SimpleJpaRepository for mapped superclasses since the entity type is taken from the entityInformation of the repository in the delete method. I'm not sure if it is recommended to create a repository for mapped superclasses, probably not, but we had no problems so far with our use case. Is it possible to take the java type information from the entity itself instead?

Thank you!

Kristina

Comment by Oliver Drotbohm [ 16/Jul/19 ]

I've just pushed tweaks to the fix resorting to the actually given entity's type instead of the one obtained from the repository.

Generated at Sun Oct 20 19:35:52 UTC 2019 using Jira 7.13.8#713008-sha1:1606a5c1e7006e1ab135aac81f7a9566b2dbc3a6.