Hibernate's @Version annotation to enable Optimistic Locking is ignored

[manoelcampos]

Describe the bug The Hibernate´s @Version annotation is having no effect when using Panache.

Expected behavior An OptimisticLockException must be thrown to avoid possible data override when two users try to update the same entity at the same time.

Actual behavior When using Hibernate's @Version annotation to enable Optimistic Locking, if two users change the same Entity at the same time, no OptimisticLockException is thrown.

To Reproduce

To make it easy to reproduce the issue, a repository was created here. It provides two minimal applications to show the issue. The applications use an in-memory H2 database, requiring no extra configurations.

Environment:

• Quarkus version: 1.2.0.Final

[gsmet]

/cc @Sanne @FroMage any idea?

[emmanuelbernard]

Can you try one thing @manoelcampos

Add https://github.com/manoelcampos/hibernate-concurrency/blob/master/2-database-concurrency-panache/src/main/java/com/manoelcampos/server/model/Cliente.java#L23

existente.version = cliente.version;

to make sure we get the client side version copied so we detect the conflict.

Also replace https://github.com/manoelcampos/hibernate-concurrency/blob/master/2-database-concurrency-panache/src/main/java/com/manoelcampos/server/model/Cliente.java#L27 existente.persist() with existente.flush() and see what's happening.

You must do flush() so that the operation is effectively sent to the database. persist on an existing entity is a noop.

In practice, your v1 is doing a merge of a client side copy and thus the copy is automatic.

BTW @FroMage , there is no merge equivalent so the copy is manual. Should we have a reattach operation of sort?

Also, you should have a @Transactional here https://github.com/manoelcampos/hibernate-concurrency/blob/master/2-database-concurrency-panache/src/main/java/com/manoelcampos/server/rest/ClienteResource.java#L37 to ensure that a flush and an actual update of the database happen. But that change won't fix your optimistic locking.

[manoelcampos]

Hello @emmanuelbernard In fact, I removed the field from theupdate() method just to try different things and forgot to put it back. Including the field is the way it's supposed to work, but it doesn't. I just updated the source code including it.

I added 2 animated gifs to the README to show the issue.

Replacing persist() by flush() or persistAndFlush() doesn't have any effect with the locking. The @Version long versao field is incremented, but the data from the previous user is overwritten.

About the transaction, as it's just an example, I made the entire ClienteResource class transactional. I moved the @Transactional directly to the update method and it doesn't help anyway as you said.

I made sure to stop and restart Quarkus after such changes.

[emmanuelbernard]

@Sanne @gsmet I am a bit lost but it seems that

Foo fooWithChangesAndOldVersion = ...;
Foo foo = em.find(Foo.class, 1L);
foo.title = fooWithChangesAndOldVersion.title;
foo.version = fooWithChangesAndOldVersion.version;
em.flush();

Does not raise an exception because the version number overridden is ignored in the update SQL comparison. The comparison is there, just the version value loaded into foo is used which defeats the optimistic versioning mechanism. I have to admit I am blanking whether that's expected behavior or not. In JPA we don't have that problem as merge does an explicit checking.

Thoughts? Is that expected Hibernate behavior, a hibernate bug? Or some unexpected interaction between Panache and Hibernate?

[manoelcampos]

@emmanuelbernard I've just found the issue. Panache doesn't expose the EntityManager.update() method, so we have to write that extra code to merge an existing entity:

public static boolean update(Foo foo) {
        Foo existing = Foo.findById(foo.id);
        if(existing != null){
            existing.name = foo.name;
            existing.version = foo.version;
            existing.persistAndFlush();
            return true;
        }

        return false;
    }

I've just added a merge() method to JpaOperations and PanacheEntityBase classes and it works as expected. This way, I can update an entity by just calling:

    public static boolean update(Foo foo) {
        Foo updated = foo.merge();
        updated.flush();
        return true;
    }

I don't know why the EntityManager.merge() isn't exposed in Panache. I just added it in my fork at https://github.com/manoelcampos/quarkus/tree/hibernate_optimistic_locking I can send a PR if you think it's OK.

[emmanuelbernard]

I don't think it's ok no, at least I'm not sure. I need to discuss with @FroMage and @Sanne on this one.

[FroMage]

Given:

Foo fooWithChangesAndOldVersion = ...;
Foo foo = em.find(Foo.class, 1L);
foo.title = fooWithChangesAndOldVersion.title;
foo.version = fooWithChangesAndOldVersion.version;
em.flush();

It appears that foo.version = fooWithChangesAndOldVersion.version differs in behaviour from what the equivalent merge would do.

My guess is that setting the version manually causes some dirty state to be set while merge setting the version does not. We have to look at the code to make sure.

As to why we didn't add merge is that we figured that detached entities are evil and the source of too many bugs and WTFs. Now, admittedly this issue is a WTF that would be fixed by adding merge but we did not expect that ;)

[Sanne]

It appears that foo.version = fooWithChangesAndOldVersion.version differs in behaviour from what the equivalent merge would do.

That's spot on: the Hibernate ORM documentation specifically prohibits messing with the version attribute - it's going to be ignored. I wonder if it shouldn't throw an exception.

I suspect you'll need to expose the merge() operation; but I agree that it would be nice to find an alternative.

[FroMage]

I can probably make it throw if you attempt to set the version to a different version if the entity is managed.

Is it merge that throws, or is it on flush?

[manoelcampos]

@FroMage yeah, it's really strange. If I can help anyway, just let me know. I'll be glad to. @Sanne, to avoid exposing merge(), the persist() method could be changed as below, but that will change the behavior of persist() and may cause undesired surprises for existing Quarkus users:

public static void persist(Object entity) {
    EntityManager em = getEntityManager();
    if(isPersistent(entity))
          merge(entity);
    else persist(em, entity);
}

[Sanne]

If persist just behaves like merge, I'd prefer it to be called merge to not confuse people.

BTW are you sure that would work? I expect isPersistent would always return false for a detached entity.

[Sanne]

I can probably make it throw if you attempt to set the version to a different version if the entity is managed.

Is it merge that throws, or is it on flush?

I guess ideally I'd throw directly on attempting to write on the field.

But while adding such extra checks might avoid some confusion, it doesn't solve the main problem: expose a way to attache detached entities, or alternatively give people an explicit way to deserialize an entity including its version state.

[FroMage]

I guess ideally I'd throw directly on attempting to write on the field.

That should be easy

But while adding such extra checks might avoid some confusion, it doesn't solve the main problem: expose a way to attache detached entities, or alternatively give people an explicit way to deserialize an entity including its version state.

But this is not the problem here. This is an orthogonal issue which we are investigating as part of the resteasy/panache problem space in another issue.

[manoelcampos]

BTW are you sure that would work? I expect isPersistent would always return false for a detached entity.

@Sanne, in fact, isPersistent() will return false for detached entities. What I usually do in my applications is just:

public static void persist(Object entity) {
    EntityManager em = getEntityManager();
    if(entity.id != null && entity.id > 0)
          merge(entity);
    else persist(em, entity);
}

[manoelcampos]

Is it merge that throws, or is it on flush?

@FroMage, merge is the one to throw the OptimisticLockException, as can be seen in the stacktrace below:

javax.persistence.OptimisticLockException: Row was updated or deleted by another transaction (or unsaved-value mapping was incorrect) : [com.manoelcampos.server.model.Cliente#1]
        at org.hibernate.internal.ExceptionConverterImpl.wrapStaleStateException(ExceptionConverterImpl.java:223)
        at org.hibernate.internal.ExceptionConverterImpl.convert(ExceptionConverterImpl.java:93)
        at org.hibernate.internal.ExceptionConverterImpl.convert(ExceptionConverterImpl.java:181)
        at org.hibernate.internal.ExceptionConverterImpl.convert(ExceptionConverterImpl.java:188)
        at org.hibernate.internal.SessionImpl.fireMerge(SessionImpl.java:786)
        at org.hibernate.internal.SessionImpl.merge(SessionImpl.java:762)
        at io.quarkus.hibernate.orm.runtime.entitymanager.TransactionScopedEntityManager.merge(TransactionScopedEntityManager.java:124)
        at io.quarkus.hibernate.orm.runtime.entitymanager.ForwardingEntityManager.merge(ForwardingEntityManager.java:32)
        at com.manoelcampos.server.dao.JpaDAO.save(JpaDAO.java:31)

[emmanuelbernard]

Let me take a step back.

PanacheEntity objects are where operations live. JPA merge() by default, will return a copy of a detached entity. And the use case we want to fix is to be able to copy part or the entirety of the state from a version coming from the web back into the database.

Option A

So we can expose panacheEntity.merge() knowing that the merge semantic will not be the same as JPAs (no copy here). Also I wonder what PanacheRepository should do.

Option A1

We can expose a panacheEntity.reattach() to clarify that the semantic is different.

PS what do we do semantic wise if there is already an existing entity in the EM context and we want to "pseudo-merge" or reattach (option A or A1).

None do solve the problem of partial updates.

@Entity 
public class Foo extends PanacheEntity {
    @Version public long version;
    public String title;
    public String author;
}

@PUT
public updateTitle(Foo foo) {
    Foo existing = Foo.findById(foo.id);
    existing.title = foo.title;
    existing.version = foo.version;
    // not author!
    existing.merge(); // is that how we want to do it? 
}

This is a different semantic from Hibernate ORM (version protection) and I'd like to know if we are fine with all that.

[emmanuelbernard]

Option B is to not have full state merge and force a manual copy.

Foo existing = Foo.findById(foo.id);
existing.title = foo.title;
existing.version = foo.version;
existing.author = foo.author;
existing.merge(); // or update() maybe
// or have flush() check it and do some entity tracking above / or in Hibernate??

[Sanne]

But this is not the problem here. This is an orthogonal issue which we are investigating as part of the resteasy/panache problem space in another issue.

@FroMage I'm not following - what is this this that you're referring to? Please quote when commenting. Your previous comment I can probably make it throw if you attempt to set the version to a different version seemed to refer to my suggestion that ORM should not really let people write on the version field, since it ignores it. I guess a "WARN: you've messed with field x, I'm going to ignore it" could also work. Either way, "this is not the problem here" is exactly what I said: it doesn't solve the main problem

@manoelcampos :

in fact, isPersistent() will return false for detached entities. What I usually do in my [..]

Yes I understand, but while that code might work for you, it isn't safe for other use cases; for example when using explicitly assigned identifiers. We need a bullet-proof solution to be able to rely it in framework code.

So let's not overload the meaning of persist(), this needs to be an explicit user choice.

@emmanuelbernard Option B is defeating the point of optimistic version control :) In Manoel's example, the webpage would present data from entity version 1, but then you load entity version 2 and copy changes to it. You need the "original" existing entity at version 1.

Option A1

+1 Seems to be the most sensible choice.

We need to avoid the case of an existing attached instance with the same ID: maybe just throw an exception? I think I'd use the exception as I'd consider it an unexpected use case.

[Sanne]

hum actually throwing an exception might not be straight forward as there's grey areas when it comes to associations: what if some of the objects in the model already exist in the session, and others don't? Is it ok to fail only after having loaded some of these?

[emmanuelbernard]

To clarify, in option B we would have a different behavior of Hibernate (or of the panache layer): i.e. let the version be overridden and checked before.

Option C

To avoid the attach problem I think I would prefer the original merge contract

class PanacheEntity
    // should be called mergeState?
    public static PanacheEntity merge(PanacheEntity panacheEntity) {
        return merge(panacheEntity);
    }
}

The reason is that Hibernate had reattach semantic in the past but we moved away from it. I remember @gavinking saying that the saveOrUpdate semantics were dodgy and that might have been the reasons.

For partial update, then I think the user should findById and do the manual version comparison. I don't know how we could nicely help otherwise (except maybe in RESTEasy with Panache

[manoelcampos]

To avoid the attach problem I think I would prefer the original merge contract

@emmanuelbernard

I'm thinking that it would be great if merge() was a generic method such as below:

    public <T extends PanacheEntityBase> T merge() {
        return (T) JpaOperations.merge(this);
    }

That will enable to use it as:

    @PUT
    @Transactional
    public void update(Foo foo) {
        Foo updated = foo.merge(); //instead of: PanacheEntity updated = foo.merge();
        foo.flush();
    }

I think it's safe to cast the merge() return to T and therefore include a @SuppressWarnings("unchecked"). If we try to generify the JpaOperations method, we go back to the same warning again.

[fdai2003]

This will work for us to. It requires that the developer of the update method need to know if persist or merge must be used. But in this case it is clear to call merge, because the entity is detached and modified by a user.

[emmanuelbernard]

Right. I did not bother with the generic work. But your solution is better.

[FroMage]

OK, my intention got lost in translation. Here's what I'm proposing:

@Entity 
public class Foo extends PanacheEntity {
    @Version public long version;
    public String title;
    public String author;
}

// proposal
@PUT
public updateTitle(Foo foo) {
    Foo existing = Foo.findById(foo.id);
    existing.title = foo.title;
    // this throws if the versions differ if the entity is persistent
    existing.version = foo.version;
}

// this is if we add merge()
@PUT
public updateTitle(Foo foo) {
    // this would throw
    foo.merge();
}

What I'm saying is that we can make setVersion throw if the entity is attached and the version differs, which achieves the same semantics as merge, and does not force us to add merge for this use-case.

This way, we can discuss adding merge for other use-cases, but not version checking.

[from @Sanne] hum actually throwing an exception might not be straight forward as there's grey areas when it comes to associations: what if some of the objects in the model already exist in the session, and others don't? Is it ok to fail only after having loaded some of these?

Now, this is somewhat contingent on what merge does for associations. If it does version checking even for merged relations, then I doubt we can do that.

[emmanuelbernard]

OK, my intention got lost in translation. Here's what I'm proposing:

@Entity 
public class Foo extends PanacheEntity {
    @Version public long version;
    public String title;
    public String author;
}

// proposal
@PUT
public updateTitle(Foo foo) {
    Foo existing = Foo.findById(foo.id);
    existing.title = foo.title;
    // this throws if the versions differ if the entity is persistent
    existing.version = foo.version;
}

// this is if we add merge()
@PUT
public updateTitle(Foo foo) {
    // this would throw
    foo.merge();
}

What I'm saying is that we can make setVersion throw if the entity is attached and the version differs, which achieves the same semantics as merge, and does not force us to add merge for this use-case.

The problem with this model is that I'm sure someone will do something with intermediary version swaps and you will get an early exception before an effective merge or flush operation. I think I like merge a bit better because of that. Also closer to JPA.

[emmanuelbernard]

I had a conversation with @Sanne and conceptually, we feel that there is a missing operation in Hibernate ORM itself to do the kind of things you want to do - checking a detached entity state and do (partial or complete) update on an already attached entity. So I'd recommend you use the entityManager().merge() for now and we need to list this problem as one of the several to solve in Panache (and in this case Hibernate ORM more likely).

[manoelcampos]

Meanwhile, the code below is a workaround for those facing the same issue:

    public static void update(SomeEntity entity) {
        final EntityManager em = JpaOperations.getEntityManager();
        SomeEntity updated = em.merge(entity);
        updated.flush();
    }

[nicmarti]

Just out of curiosity: any update related to this merge issue? We're facing the same issue

[emmanuelbernard]

We have not yet had time to focus on it unfortunately.

[Sanne]

Sorry I got distracted. I'll try to have this resolved - at least to not ignore the annotation - for 1.7