Optimistic lock does not work correctly when REST call is received

[Dennisber00]

Affects: Spring boot 2.4.1 and earlier
Affects: spring-boot-starter-data-jpa 2.4.1 and earlier
Affects: spring-boot-starter-web 2.4.1 and earlier

Description: I found out that optimistic lock does not work correctly in a case when a method, which can produce it, is called via REST controller.

As an example, the following method of a service must produce an optimistic lock exception every time it is called (let assume that a book entity is already created in the database):

1  public void produceOptimisticLock(Long id) {
2   Book found = bookRepository.findById(id)
3        .orElseThrow(() -> new RuntimeException("Oops"));
4    found.setName("optLock !");
5    found.setVersion(-777L);
6    save(found);
7  }

I. Because there is no @transaction annotation above the method and a read only transaction as well as hibernate session are created on line 2, then entity is loaded and got a DETACHED status. After that on line 6 a new transaction as well as hibernate session are created to merge "found" entity changes. Finally DefaultMergeEventListener (hibernate) produces a new optimistic lock exception, because the entity is processed as DETACHED (entityIsDetached method) and the version field was changed. It works correctly when it is called internally in an application.

II. But it does not work at all when it is called as part of REST request handling. Because OpenEntityManagerInViewFilter delegates OpenEntityManagerInViewInterceptor to create a new entity manager instance which creates one hibernate session to process entire request. In this case on line 2 the "found" entity is cached by StatefulPersistenceContext which leads to a problem on line 6, because the session is still alive/active StatefulPersistenceContext still contains "found" entity in L1 cache. In this case entity state is resolved as PERSISTENT in DefaultMergeEventListener.onMerge method. It means that "found" entity is processed as PERSISTENT in entityIsPersistent method of DefaultMergeEventListener. entityIsPersistent does not check if the version is changed. Because of it "found" entity is saved on line 6 without any optimistic lock exception.

I guess this method should work always identical and produces the optimistic lock exception. Am i right ?

I created a demo project to produce this problem:
https://github.com/Dennisber00/spring-opt-log-bug

Step to reproduce in the demo project

No optimistic lock exception:

1. Run the app
2. Send a rest call - http://localhost:8080/book/opt-lock/1
3. Open h2 console http://localhost:8088/h2-console/
4. Execute "select * from book"
5. Check that entity with ID - 1 was successfully updated (version was increased to 1)

Optimistic lock exception:

1. Open application.properties
2. Change do.magic.fire.opt.lock=false -> do.magic.fire.opt.lock=true property
3. Run the app
4. The app should be crashed with optimistic lock exception due to DoMagic event listener class which receives ApplivaitonReadyEvent and calls produceOptimisticLock method.

---

[Dennisber00]

did i create the issue incorrectly ? or in wrong place ?

[wwwrakesh1]

I have seen same issue

[fraf]

Hi,

Same issue here.
Even if I force a random "version" to my entity, when I do a merge call, the random version was completely ignored. It should compare the version of the entity to merge (specified in method argument) with the one existing in the database (the one relying into the hibernate session from the "get" method call).

@Dennisber00 : To bypass the issue, I wrapped the merge method of the entityManager like this :

public T merge(T entity) throws HeliosConcurrentModificationException {
        if (entity instanceof VersionableEntity) {
            entityManager.detach(entity);
            try {
                return entityManager.merge(entity);
            } catch (OptimisticLockException e) {
                throw new HeliosConcurrentModificationException("Concurrent update prevented ! entity : " + entity, e);
            }
        }
        return entityManager.merge(entity);
    }

I flagged versioned entities with VersionableEntity interface. If one is asking for merge, then I do a check of which kind of entity it is. In case of a versionable one, I force a detach of it before calling merge. Like this, version attribut is matched against the one recorded in the database. If OLE is thrown, I wrapped it into the application global Exception HeliosConcurrentModificationException, which extends RuntimeException and then causes the transaction rollback (default Spring Framework behavior).
This wrapped merge method works. But entity classes with encapsulation pattern should be processed carefully. Here an exemple :
Main entity :

@Getter
@Setter
@Entity
@Table(name = "produit")
@NoArgsConstructor
@AllArgsConstructor
public class Produit extends AbstractData implements IdNomCode, VersionableEntity {

    @Id
    @Column(name = "id")
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Long id;


    @ManyToOne(cascade = {CascadeType.MERGE, CascadeType.REMOVE, CascadeType.PERSIST, CascadeType.REFRESH})
    @JoinColumn(name = "id_produit_cru", foreignKey = @ForeignKey(name = "fk_produit_produit_cru"), referencedColumnName = "id")
    private ProduitCru produitCru;

@Version
    private int version;
....
}

And the encapsulated one :

@Getter
@Setter
@Entity
@Table(name = "produit_cru", uniqueConstraints = {
        @UniqueConstraint(name = "uk_produit_cru_code", columnNames = {"code"}),
        @UniqueConstraint(name = "uk_produit_cru_nom", columnNames = {"nom"})})
@NoArgsConstructor
@AllArgsConstructor
public class ProduitCru extends AbstractData implements IdNomCode {

    @Id
    @Column(name = "id")
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Long id;

     @OneToMany(mappedBy = "produitCru")
    private Set<Produit> produitsMillesime = new HashSet<>();

...
}

Pay attention to the cascadeType in the Produit entity. The ProduitCru relation should not be cascaded with DETACH, otherwise, the merge fail ! "Attempting to persist a detached entity" message exception is thrown.
Like this it works. But all write operations done on a produitCru entity should be done only through Produit entity. That's the deal. Produit entity is the only one having the version attribute.

[snicoll]

@Dennisber00 this all looks like expected, setting the version manually and not having a transaction around that code should throw that exception. I am going to close this now and please use StackOverflow if you have more questions around JPA usage.

[Dennisber00]

@Dennisber00 this all looks like expected, setting the version manually and not having a transaction around that code should throw that exception. I am going to close this now and please use StackOverflow if you have more questions around JPA usage.

@snicoll Have you read my description carefully ? Of course, it is excepted to have en exception, but I showed a sample (even created a small project to show it) when NO EXCEPTION is thrown if it is executed behind a REST call. So it is a bug then.

Please, re-read this part of my description one more time:

"II. But it does not work at all when it is called as part of REST request handling. Because OpenEntityManagerInViewFilter delegates OpenEntityManagerInViewInterceptor to create a new entity manager instance which creates one hibernate session to process entire request. In this case on line 2 the "found" entity is cached by StatefulPersistenceContext which leads to a problem on line 6, because the session is still alive/active StatefulPersistenceContext still contains "found" entity in L1 cache. In this case entity state is resolved as PERSISTENT in DefaultMergeEventListener.onMerge method. It means that "found" entity is processed as PERSISTENT in entityIsPersistent method of DefaultMergeEventListener. entityIsPersistent does not check if the version is changed. Because of it "found" entity is saved on line 6 without any optimistic lock exception.

I guess this method should work always identical and produces the optimistic lock exception. Am i right ?"

I will create a new identical issue if this is not be re-opened. Because i believe it is the bug.

[snicoll]

I guess this method should work always identical and produces the optimistic lock exception. Am i right ?"

No. I've shared why I think that code is invalid already. If you read an entity outside of a transaction and attempt to save it the way you do, you're opening yourself to inconsistencies like this. You think this is a Spring issue but what you use from it simply delegates to JPA, that's why I mentioned to ask usage question on SO.

I will create a new identical issue if this is not be re-opened. Because i believe it is a bug.

This is a waste of both our time as it will be closed.

[Dennisber00]

No. I've shared why I think that code is invalid already. If you read an entity outside of a transaction and attempt to save it the way you do, you're opening yourself to inconsistencies like this. You think this is a Spring issue but what you use from it simply delegates to JPA, that's why I mentioned to ask usage question on SO.

@snicoll This is only a simplified sample (invalid as you wish) to demonstrate this problem. Of course, in this sample it is only JPA delegation. You are right here. Unfortunately, i am not allow to share an Enterprise project (which is definitely way more complex than this sample) to show a real example where this problem was faced.

No exception is thrown because of OSIV or OpenSessionInViewFilter creating a session per each REST request (spring logic and not related to JPA) + lack of transaction (human error). And OSIV is enabled by default. It means that in a real big project where spring/spring boot is used to simplify developers life ("easy to use" is one of spring slogan, right ?) if human factor is applied (or i would say when applied) and @transaction is lost somewhere in between (this situation is easy to imagine, unfortunately) a developer will face inconsistency in behavior (as you correctly mentioned) which is really hard to identify in a spot where exception must be thrown. Based on this i think it is still a spring issue, not JPA or something else.

[snicoll]

And OSIV is enabled by default.

Correct. There is a warning in the startup log of your sample to tell you how to disable it. We strongly encourage you to disable it if you don't need it. If you want more context as why OSIV is enabled by default, see spring-projects/spring-boot#7107