quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.75k stars 2.67k forks source link

Panache#count() should be stateless #35978

Open Serkan80 opened 1 year ago

Serkan80 commented 1 year ago

Describe the bug

When calling the count method on the PanacheEntity, the call is made with dirty checking (~statefull).

This results in unnecessary and unexpected extra SQL calls. And also causes errors, because we have an auditlog which is triggered via a db trigger, because it receives an insert & update record at the same time, which causes not uniqueness error on the primary keys.

See below for an example.

Expected behavior

The count call should be stateless, not resulting in an extra update call.

Expected sql calls should be:

select count ....
insert into ...

Actual behavior

This results in the following queries:

select count ....
insert into ...
update entity set (all the fields) where id = ...

How to Reproduce?

    @Transactional
    public MyEntityDto save(MyEntityDto dto) {
        var entityExists = MyEntity.count("name = ?1", dto.name) > 0;

        if (entityExists) {
            throw new WebApplicationException("Entity(name=%s) already exists".formatted(dto.name), 400);
        }

        var entity = Mapper.mapToEntity(dto);
        entity.persist();
        Log.infof("Entity(name=%s) created", entity.name);

        return Mapper.mapToDto(entity);
    }

Output of uname -a or ver

windows 10

Output of java -version

JDK 17 Temurin

GraalVM version (if different from Java)

No response

Quarkus version or git rev

3.2.6.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Maven 3.8.x

Additional information

A quick fix is by replacing the count with find:

 var entityExists = MyEntity.find("name = ?1", dto.name)
                            .withHint(HINT_READONLY, true)
                            .count() > 0;

It would be nice if the count() method was by default stateless (= with HINT_READONLY = true)

quarkus-bot[bot] commented 1 year ago

/cc @FroMage (panache), @loicmathieu (panache)

FroMage commented 1 year ago

@yrodiere and @Sanne on #36009 imply this may be a Hibernate ORM bug. What DB are you using?

Sanne commented 1 year ago

I don't think we have enough information to assume it's an ORM bug. What makes you suspect this?

yrodiere commented 1 year ago

Because of what I said, I think:

First, why would a count query before a persist() call lead to an additional update after the persist() call... ? It looks a lot like a bug in ORM, doesn't it?

Though I do agree we need to have a closer look as there could be weird configuration or mapping at play. I wonder if the reproducer in the description truly is enough to trigger this? There might be getters with side effects at play... ?

geoand commented 14 hours ago

@yrodiere what's the status of this?

yrodiere commented 14 hours ago

@yrodiere what's the status of this?

Needs investigation on the root cause, from what I can see. It's in the backlog, someone will have a look when they have time.

mbladel commented 12 hours ago

FWIW, what I can see from the current implementations of io.quarkus.hibernate.orm.panache.common.runtime.AbstractJpaOperations#count is that a simple SELECT COUNT(*) FROM <entity_name> query is created, and this has no effect to the persistence context: no entity instance is loaded at all, so no "dirty checking" would be executed when flushing the session.

I suspect there might be additional application logic (unrelated to the simple count operation) that changes the state of the persisted entity.

yrodiere commented 12 hours ago

FWIW, what I can see from the current implementations of io.quarkus.hibernate.orm.panache.common.runtime.AbstractJpaOperations#count is that a simple SELECT COUNT(*) FROM <entity_name> query is created, and this has no effect to the persistence context: no entity instance is loaded at all, so no "dirty checking" would be executed when flushing the session.

Already done recently: https://github.com/quarkusio/quarkus/pull/41620

I suspect there might be additional application logic (unrelated to the simple count operation) that changes the state of the persisted entity.

Right. I suspect that reproducer is not enough, but someone would need to have a look.

mbladel commented 11 hours ago

Already done recently: #41620

I'm guessing you're referring to switching to the new Hibernate SelectionQuery#getResultCount() API, introduced with https://hibernate.atlassian.net/browse/HHH-16931. That PR addressed getting counts for existing Panache queries, true, but I was thinking of the count operation on Panache entities themselves.

That is handled in a different place (AbstractJpaOperations IIUC), and I think it would benefit from the new standardized API as well: Hibernate applies some clever tricks on the generated count query to ensure the count is correct and retrieved optimally. I've created https://github.com/quarkusio/quarkus/pull/44188 to address this.

Serkan80 commented 8 hours ago

I think this issue was solved by someone else by adding WITH_HINT to true in another issue. But I can’t find the issue anymore.

Anyways, the reason why the extra update statement was called, might have to do with:

@Transactional
public MyEntityDto save(MyEntityDto dto) {
   …

    return Mapper.mapToDto(entity);
}

This mapper is calling the getters of the entity to create and return a new dto, maybe that was causing it.

But as I stated in my first post, as soon as I replace Panache#count with Panache#find with hints set to true, the update statement doesn’t happen.