From d2dcb5e4a4fa0f7374e25fa852bafafd87583f13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20=C3=89pardaud?= Date: Wed, 5 Jun 2024 14:59:02 +0200 Subject: [PATCH] ORM/Panache: rely on ORM getResultCount This allows us to not care about which parameters are set on the count query, since `order by` parameters are ignored by ORM. Which is not the case if we write our own count query. Note that this is not yet available in HR, so we have to keep the count query logic around until that's available. Fixes #40962 --- .../runtime/CommonPanacheQueryImpl.java | 53 +++++++++---------- .../runtime/CustomCountPanacheQuery.java | 2 +- .../common/runtime/PanacheJpaUtil.java | 4 ++ .../io/quarkus/it/panache/Bug40962Entity.java | 11 ++++ .../io/quarkus/it/panache/TestEndpoint.java | 13 +++++ .../it/panache/PanacheFunctionalityTest.java | 5 ++ 6 files changed, 60 insertions(+), 28 deletions(-) create mode 100644 integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/Bug40962Entity.java diff --git a/extensions/panache/hibernate-orm-panache-common/runtime/src/main/java/io/quarkus/hibernate/orm/panache/common/runtime/CommonPanacheQueryImpl.java b/extensions/panache/hibernate-orm-panache-common/runtime/src/main/java/io/quarkus/hibernate/orm/panache/common/runtime/CommonPanacheQueryImpl.java index f9dd38ee25652..e47d6be8a9142 100644 --- a/extensions/panache/hibernate-orm-panache-common/runtime/src/main/java/io/quarkus/hibernate/orm/panache/common/runtime/CommonPanacheQueryImpl.java +++ b/extensions/panache/hibernate-orm-panache-common/runtime/src/main/java/io/quarkus/hibernate/orm/panache/common/runtime/CommonPanacheQueryImpl.java @@ -46,7 +46,12 @@ public void close() { * this is the original Panache-Query, if any (can be null) */ private String originalQuery; - protected String countQuery; + /** + * This is only used by the Spring Data JPA extension, due to Spring's Query annotation allowing a custom count query + * See https://docs.spring.io/spring-data/jpa/reference/jpa/query-methods.html#jpa.query-methods.at-query.native + * Otherwise we do not use this, and rely on ORM to generate count queries + */ + protected String customCountQueryForSpring; private String orderBy; private EntityManager em; @@ -72,7 +77,7 @@ public CommonPanacheQueryImpl(EntityManager em, String query, String originalQue private CommonPanacheQueryImpl(CommonPanacheQueryImpl previousQuery, String newQueryString, String countQuery) { this.em = previousQuery.em; this.query = newQueryString; - this.countQuery = countQuery; + this.customCountQueryForSpring = countQuery; this.orderBy = previousQuery.orderBy; this.paramsArrayOrMap = previousQuery.paramsArrayOrMap; this.page = previousQuery.page; @@ -118,13 +123,14 @@ public CommonPanacheQueryImpl project(Class type) { } newQuery.append("new ").append(type.getName()).append("(").append(selectClause).append(")").append(from); - return new CommonPanacheQueryImpl<>(this, newQuery.toString(), "select count(*) " + from); + // I think projections do not change the result count, so we can keep the custom count query + return new CommonPanacheQueryImpl<>(this, newQuery.toString(), customCountQueryForSpring); } // build select clause with a constructor expression String selectClause = "SELECT " + getParametersFromClass(type, null); - return new CommonPanacheQueryImpl<>(this, selectClause + selectQuery, - "select count(*) " + selectQuery); + // I think projections do not change the result count, so we can keep the custom count query + return new CommonPanacheQueryImpl<>(this, selectClause + selectQuery, customCountQueryForSpring); } private StringBuilder getParametersFromClass(Class type, String parentParameter) { @@ -276,35 +282,28 @@ public void withHint(String hintName, Object value) { // Results - @SuppressWarnings("unchecked") public long count() { if (count == null) { - String selectQuery = query; - if (PanacheJpaUtil.isNamedQuery(query)) { - org.hibernate.query.Query q = (org.hibernate.query.Query) em.createNamedQuery(query.substring(1)); - selectQuery = q.getQueryString(); - } - - Query countQuery = em.createQuery(countQuery(selectQuery)); - if (paramsArrayOrMap instanceof Map) - AbstractJpaOperations.bindParameters(countQuery, (Map) paramsArrayOrMap); - else - AbstractJpaOperations.bindParameters(countQuery, (Object[]) paramsArrayOrMap); - try (NonThrowingCloseable c = applyFilters()) { - count = (Long) countQuery.getSingleResult(); + if (customCountQueryForSpring != null) { + Query countQuery = em.createQuery(customCountQueryForSpring); + if (paramsArrayOrMap instanceof Map) + AbstractJpaOperations.bindParameters(countQuery, (Map) paramsArrayOrMap); + else + AbstractJpaOperations.bindParameters(countQuery, (Object[]) paramsArrayOrMap); + try (NonThrowingCloseable c = applyFilters()) { + count = (Long) countQuery.getSingleResult(); + } + } else { + @SuppressWarnings("rawtypes") + org.hibernate.query.Query query = (org.hibernate.query.Query) createBaseQuery(); + try (NonThrowingCloseable c = applyFilters()) { + count = query.getResultCount(); + } } } return count; } - private String countQuery(String selectQuery) { - if (countQuery != null) { - return countQuery; - } - - return PanacheJpaUtil.getFastCountQuery(selectQuery); - } - @SuppressWarnings("unchecked") public List list() { Query jpaQuery = createQuery(); diff --git a/extensions/panache/hibernate-orm-panache/runtime/src/main/java/io/quarkus/hibernate/orm/panache/runtime/CustomCountPanacheQuery.java b/extensions/panache/hibernate-orm-panache/runtime/src/main/java/io/quarkus/hibernate/orm/panache/runtime/CustomCountPanacheQuery.java index 5d2d173755d16..81844bca52ee0 100644 --- a/extensions/panache/hibernate-orm-panache/runtime/src/main/java/io/quarkus/hibernate/orm/panache/runtime/CustomCountPanacheQuery.java +++ b/extensions/panache/hibernate-orm-panache/runtime/src/main/java/io/quarkus/hibernate/orm/panache/runtime/CustomCountPanacheQuery.java @@ -15,7 +15,7 @@ public CustomCountPanacheQuery(EntityManager em, Query jpaQuery, String customCo Object paramsArrayOrMap) { super(new CommonPanacheQueryImpl<>(em, castQuery(jpaQuery).getQueryString(), null, null, paramsArrayOrMap) { { - this.countQuery = customCountQuery; + this.customCountQueryForSpring = customCountQuery; } }); } diff --git a/extensions/panache/panache-hibernate-common/runtime/src/main/java/io/quarkus/panache/hibernate/common/runtime/PanacheJpaUtil.java b/extensions/panache/panache-hibernate-common/runtime/src/main/java/io/quarkus/panache/hibernate/common/runtime/PanacheJpaUtil.java index 6685c3bcee2e1..6ad219a98bfd0 100644 --- a/extensions/panache/panache-hibernate-common/runtime/src/main/java/io/quarkus/panache/hibernate/common/runtime/PanacheJpaUtil.java +++ b/extensions/panache/panache-hibernate-common/runtime/src/main/java/io/quarkus/panache/hibernate/common/runtime/PanacheJpaUtil.java @@ -38,6 +38,8 @@ public class PanacheJpaUtil { /** * This turns an HQL (already expanded from Panache-QL) query into a count query, using text manipulation * if we can, because it's faster, or fall back to using the ORM HQL parser in {@link #getCountQueryUsingParser(String)} + * + * @deprecated we should use SelectionQuery.getResultCount() when supported by HR (ORM already supports it) */ public static String getFastCountQuery(String query) { // try to generate a good count query from the existing query @@ -85,6 +87,8 @@ public static String getFastCountQuery(String query) { /** * This turns an HQL (already expanded from Panache-QL) query into a count query, using the * ORM HQL parser. Slow version, see {@link #getFastCountQuery(String)} for the fast version. + * + * @deprecated we should use SelectionQuery.getResultCount() when supported by HR (ORM already supports it) */ public static String getCountQueryUsingParser(String query) { HqlLexer lexer = new HqlLexer(CharStreams.fromString(query)); diff --git a/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/Bug40962Entity.java b/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/Bug40962Entity.java new file mode 100644 index 0000000000000..c24fcfcc3a644 --- /dev/null +++ b/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/Bug40962Entity.java @@ -0,0 +1,11 @@ +package io.quarkus.it.panache; + +import jakarta.persistence.Entity; + +import io.quarkus.hibernate.orm.panache.PanacheEntity; + +@Entity +public class Bug40962Entity extends PanacheEntity { + public String name; + public String location; +} diff --git a/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java b/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java index b2e8f6a9657af..ab3dd3c6b9630 100644 --- a/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java +++ b/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java @@ -11,6 +11,7 @@ import java.util.Arrays; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -1843,4 +1844,16 @@ public String testBug36496() { Person.count("WITH id AS (SELECT p.id AS pid FROM Person2 AS p) SELECT count(*) FROM Person2 p")); return "OK"; } + + @GET + @Path("40962") + @Transactional + public String testBug40962() { + // should not throw + // Bug40962Entity.find("name = :name ORDER BY locate(location, :location) DESC", + // Map.of("name", "Demo", "location", "something")).count(); + Bug40962Entity.find("FROM Bug40962Entity WHERE name = :name ORDER BY locate(location, :location) DESC", + Map.of("name", "Demo", "location", "something")).count(); + return "OK"; + } } diff --git a/integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java b/integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java index 6ae13eb3efa5a..49f8b37e3d305 100644 --- a/integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java +++ b/integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java @@ -254,4 +254,9 @@ public void testBug26308() { public void testBug36496() { RestAssured.when().get("/test/36496").then().body(is("OK")); } + + @Test + public void testBug40962() { + RestAssured.when().get("/test/40962").then().body(is("OK")); + } }