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

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

About this issue

  • Original URL
  • State: open
  • Created 4 years ago
  • Reactions: 3
  • Comments: 48 (22 by maintainers)

Commits related to this issue

Most upvoted comments

I mean in lots of cases doing this check in your program is better than having Hibernate do it for you: instead of getting a StaleObjectStateException telling you your session is hosed and your transaction should in principle be rolled back, you get to go back to your user and say “hey, your annoying colleague was also changing this shit, and here’s his version and here’s your version and what do you want to do about it?” That’s much more robust.

You guys are scaring the hell out of me with this discussion. After 5 years of considering all kinds of possible ways of handling re-merging of disassociated state, we eventually decided that the only reasonable way to do it that didn’t suffer from pathological corner cases was the merge() operation.

Please please please don’t invent something new here in Panache because it will be broken I can promise you.

Just expose merge() because it’s carefully designed to not be broken.

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();
    }

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.

If you use REST Data Panache, you need to create that interface for each entity you want to apply the version lock. Some other workarounds were discussed here.

By the way, if you use the REST Data Panache extension, the lock works out-of-the-box by just creating an empty interface (which the implementation is automatically provided by the lib):

public interface CustomerResource extends PanacheEntityResource<Customer, Long> {
}

@liebig Sure, of course. The update statement will contain a version check. You only need to do an in-memory check to handle the case that it was updated before the find() was executed. Any updates between the find() and the flush() are of course detected by Hibernate using the returned updated row count.

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).

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.

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