Revert optimizations made for existing entities in implementation of CrudRepository.save(…) [DATAJPA-1261]

[spring-projects-issues]

Jens Schauder opened DATAJPA-1261 and commented

Triggered by BATCH-2678

---

Issue Links:

• BATCH-2678 DATAJPA-931 breaks merging with RepositoryItemWriter

• DATAJPA-931 Optimize calls to SimpleJpaRepository.save(…) if the entity is still attached

Referenced from: pull request #249, and commits 6e5bbde, 68fcf6e, c47ff56, 476baa4

Backported to: 2.0.4 (Kay SR4)

[spring-projects-issues]

Christoph Dreis commented

Given that BATCH-2678 seems to be not related to the optimizations after all, could we revert the revert?

[spring-projects-issues]

Oliver Drotbohm commented

Unless we get further arguments for the original change, we won't. The reason is a s follows: the blog post used as inspiration describes one particular scenario in which the call to save is superfluous from a JPA point of view. However it raises a couple of questions:

1. If that's the right approach in general, why doesn't the EntityManager.merge(…) implementation do the check and drop the call? EntityManager.contains(…) is a pretty simplistic check to identify the scenario in which we think we can optimize, but unfortunately in some cases, it's an invalid one. Simply checking whether the aggregate root is present in the persistence context is just a too little information. The persistence provider however has much more insight into the state of the aggregate and could skip the operations if it can tell for sure that it wouldn't make a difference. Outside of the EntityManager, we simply can't.
2. Assume the following piece of code:

@Transactional
public someMethod(Long id, SomeChild child) {
  Parent parent = parents.findById(id);
  parent.add(child);
  parents.save(parent);
  // …
}

Assume child is a transient instance. With the "optimization" applied, the call to save would avoid the call to ….merge(…) as parent is present in the persistence context. Even if child uses auto-generated identifiers, this would mean that no identifiers for child would get generated on the call to ….save(…) and child would not even become a managed instance. That doesn't make any difference at all if the call to ….save(…) is the last operation in the transactional method, just as the sample in the blog post assumes. However, if it's not, the code following the save statement would use an unmanaged child and child would not carry an identifier yet.

I'd argue that that's totally unexpected. Even worse, there's now no way to actually enforce what had been the behavior before except calling JpaRepository.flush() which creates incentives to implement technology-specific interface. All of these downsides make me think that it's easier not to call ….save(…) in the first place or introduce a custom method that implements the alternative way

[spring-projects-issues]

Christoph Dreis commented

Fair enough. Thanks for the explanation :)