Eliminate unnecessary merge() if entity is non-versioned

[quaff]

Fix GH-3401

[mp911de]

Can you please provide additional integration tests for the cases of optimistic locking failures and removal of an entity that contains modifications?

[quaff]

Can you please provide additional integration tests for the cases of optimistic locking failures and removal of an entity that contains modifications?

It's nothing related to optimistic locking but InvalidDataAccessApiUsage Removing a detached instance, and it's covered by existing tests, so I don't think additional integration tests is required here.

[schauder]

The PR as it stands right now, is not acceptable. It will not throw an OptimisticLockingException in the case of concurrent modification.

It is not about avoiding optimistic locking exceptions.
These exceptions are an important feature!

[quaff]

The PR as it stands right now, is not acceptable. It will not throw an OptimisticLockingException in the case of concurrent modification.

It is not about avoiding optimistic locking exceptions. These exceptions are an important feature!

I get your point now, that's not covered by existing tests, I will try.

[quaff]

I've updated this PR, I suggest not to squash commits, at least the first commit should be kept as it is. @schauder

[schauder]

Thanks for the additional test. There is still one scenario to investigate though.

What happens in the old and new version of the code when the entity (without optimistic locking) to be deleted contains changes, i.e. it is dirty.

The previous version might actually flush those changes, potentially triggering life cycle events, triggers and validations.
We need to make sure the new version behaves in the same way.

[quaff]

The previous version might actually flush those changes, potentially triggering life cycle events, triggers and validations. We need to make sure the new version behaves in the same way.

It's weird that flush changes before deletion, IMO we should eliminate those side effects for versioned entity too, by comparing versions between entity and existing.

[schauder]

I'm very much against changing anything about the observed behaviour in this.
While I agree that a lot of it is weird, there are going to be people relying on it.
And I'm not going to fight them in order to get some minor optimization out of a method that is not often used anyway, since people tend to not delete data from their database outside tests.

[quaff]

I'm very much against changing anything about the observed behaviour in this. While I agree that a lot of it is weird, there are going to be people relying on it. And I'm not going to fight them in order to get some minor optimization out of a method that is not often used anyway, since people tend to not delete data from their database outside tests.

OK, should I update commits to only keep the first one?

[schauder]

OK, should I update commits to only keep the first one?

I'm not sure I understand the intention with this one.
We need either full support by integration tests, or we'll have to decline this PR.

I honestly aren't sure how exactly the old and the proposed new version of the code behave.
That's why I'm insisting on the integration tests. Please let me know if you are going to try to provide such tests, otherwise I'll close this PR.

There is no blame in failing to optimise this method. At least I hope so because I tried myself before and failed miserably :o)

[quaff]

I don't understand integration tests that you said, the commit ac3f819 added test to guard current behavior that deleting stale detached entity should cause optimistic lock failure.

[schauder]

But it does not test what happens, when you delete a managed dirty entity.

[quaff]

But it does not test what happens, when you delete a managed dirty entity.

JPA will guarantee it raise OptimisticLockException at flush stage, I added test to cover that, please review 7abb5e7

[schauder]

In this scenario the interesting case is not for versioned entities, but for unversioned.

There is a risk that they get flushed to the database triggering life cycle events, database triggers and database constraints, while the new version won't

[quaff]

There is a risk that they get flushed to the database triggering life cycle events, database triggers and database constraints, while the new version won't

You mentioned it already, I agree that but I think the risk is tiny.
since it's applicative for both current and new version, should I split commit 7abb5e7 up to another dedicated PR?

[schauder]

No, we need a test mitigating that risk.