From 64b5a22a24b42e8660f441897b001e4b594fff1c Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 25 Jan 2023 12:08:19 +0100 Subject: [PATCH] Differentiate between JPQL and native queries in count query derivation. We now consider whether a query is a native one when deriving a count query for pagination. Previously, the generated queries used JPQL syntax that doesn't comply with native SQL syntax rules. Closes #2773 Original pull request #2777 --- .../query/DefaultQueryEnhancer.java | 2 +- .../query/JSqlParserQueryEnhancer.java | 23 +++-- .../data/jpa/repository/query/QueryUtils.java | 43 +++++++-- .../query/DefaultQueryEnhancerUnitTests.java | 30 +++++++ .../JSqlParserQueryEnhancerUnitTests.java | 30 +++++++ .../query/QueryEnhancerTckTests.java | 87 +++++++++++++++++++ .../query/QueryEnhancerUnitTests.java | 26 +++--- 7 files changed, 217 insertions(+), 24 deletions(-) create mode 100644 spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/DefaultQueryEnhancerUnitTests.java create mode 100644 spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JSqlParserQueryEnhancerUnitTests.java create mode 100644 spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryEnhancerTckTests.java diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/DefaultQueryEnhancer.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/DefaultQueryEnhancer.java index 53a07bf6f..92387c973 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/DefaultQueryEnhancer.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/DefaultQueryEnhancer.java @@ -46,7 +46,7 @@ public class DefaultQueryEnhancer implements QueryEnhancer { @Override public String createCountQueryFor(@Nullable String countProjection) { - return QueryUtils.createCountQueryFor(this.query.getQueryString(), countProjection); + return QueryUtils.createCountQueryFor(this.query.getQueryString(), countProjection, this.query.isNativeQuery()); } @Override diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JSqlParserQueryEnhancer.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JSqlParserQueryEnhancer.java index 7a44873ac..cd76876ac 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JSqlParserQueryEnhancer.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JSqlParserQueryEnhancer.java @@ -15,9 +15,8 @@ */ package org.springframework.data.jpa.repository.query; -import static org.springframework.data.jpa.repository.query.JSqlParserUtils.getJSqlCount; -import static org.springframework.data.jpa.repository.query.JSqlParserUtils.getJSqlLower; -import static org.springframework.data.jpa.repository.query.QueryUtils.checkSortExpression; +import static org.springframework.data.jpa.repository.query.JSqlParserUtils.*; +import static org.springframework.data.jpa.repository.query.QueryUtils.*; import net.sf.jsqlparser.JSQLParserException; import net.sf.jsqlparser.expression.Alias; @@ -29,11 +28,23 @@ import net.sf.jsqlparser.statement.Statement; import net.sf.jsqlparser.statement.delete.Delete; import net.sf.jsqlparser.statement.insert.Insert; import net.sf.jsqlparser.statement.merge.Merge; -import net.sf.jsqlparser.statement.select.*; +import net.sf.jsqlparser.statement.select.OrderByElement; +import net.sf.jsqlparser.statement.select.PlainSelect; +import net.sf.jsqlparser.statement.select.Select; +import net.sf.jsqlparser.statement.select.SelectBody; +import net.sf.jsqlparser.statement.select.SelectExpressionItem; +import net.sf.jsqlparser.statement.select.SelectItem; +import net.sf.jsqlparser.statement.select.SetOperationList; +import net.sf.jsqlparser.statement.select.WithItem; import net.sf.jsqlparser.statement.update.Update; import net.sf.jsqlparser.statement.values.ValuesStatement; -import java.util.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Objects; +import java.util.Set; import java.util.stream.Collectors; import org.springframework.data.domain.Sort; @@ -400,7 +411,7 @@ public class JSqlParserQueryEnhancer implements QueryEnhancer { return selectBody.toString(); } - String countProp = tableAlias == null ? "*" : tableAlias; + String countProp = query.isNativeQuery() ? (distinct ? "*" : "1") : tableAlias == null ? "*" : tableAlias; Function jSqlCount = getJSqlCount(Collections.singletonList(countProp), distinct); selectBody.setSelectItems(Collections.singletonList(new SelectExpressionItem(jSqlCount))); diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java index 5707caec2..785dde580 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java @@ -18,10 +18,23 @@ package org.springframework.data.jpa.repository.query; import static jakarta.persistence.metamodel.Attribute.PersistentAttributeType.*; import static java.util.regex.Pattern.*; -import jakarta.persistence.*; -import jakarta.persistence.criteria.*; -import jakarta.persistence.metamodel.*; +import jakarta.persistence.EntityManager; +import jakarta.persistence.ManyToOne; +import jakarta.persistence.OneToOne; +import jakarta.persistence.Parameter; +import jakarta.persistence.Query; +import jakarta.persistence.criteria.CriteriaBuilder; +import jakarta.persistence.criteria.Expression; +import jakarta.persistence.criteria.Fetch; +import jakarta.persistence.criteria.From; +import jakarta.persistence.criteria.Join; +import jakarta.persistence.criteria.JoinType; +import jakarta.persistence.metamodel.Attribute; import jakarta.persistence.metamodel.Attribute.PersistentAttributeType; +import jakarta.persistence.metamodel.Bindable; +import jakarta.persistence.metamodel.ManagedType; +import jakarta.persistence.metamodel.PluralAttribute; +import jakarta.persistence.metamodel.SingularAttribute; import java.lang.annotation.Annotation; import java.lang.reflect.AnnotatedElement; @@ -570,6 +583,19 @@ public abstract class QueryUtils { */ @Deprecated public static String createCountQueryFor(String originalQuery, @Nullable String countProjection) { + return createCountQueryFor(originalQuery, countProjection, false); + } + + /** + * Creates a count projected query from the given original query. + * + * @param originalQuery must not be {@literal null}. + * @param countProjection may be {@literal null}. + * @param nativeQuery whether the underlying query is a native query. + * @return a query String to be used a count query for pagination. Guaranteed to be not {@literal null}. + * @since 2.7.8 + */ + static String createCountQueryFor(String originalQuery, @Nullable String countProjection, boolean nativeQuery) { Assert.hasText(originalQuery, "OriginalQuery must not be null or empty"); @@ -591,9 +617,14 @@ public abstract class QueryUtils { String replacement = useVariable ? SIMPLE_COUNT_VALUE : complexCountValue; - String alias = QueryUtils.detectAlias(originalQuery); - if ("*".equals(variable) && alias != null) { - replacement = alias; + if (nativeQuery && (variable.contains(",") || "*".equals(variable))) { + replacement = "1"; + } else { + + String alias = QueryUtils.detectAlias(originalQuery); + if (("*".equals(variable) && alias != null)) { + replacement = alias; + } } countQuery = matcher.replaceFirst(String.format(COUNT_REPLACEMENT_TEMPLATE, replacement)); diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/DefaultQueryEnhancerUnitTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/DefaultQueryEnhancerUnitTests.java new file mode 100644 index 000000000..f113586fc --- /dev/null +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/DefaultQueryEnhancerUnitTests.java @@ -0,0 +1,30 @@ +/* + * Copyright 2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.jpa.repository.query; + +/** + * TCK Tests for {@link DefaultQueryEnhancer}. + * + * @author Mark Paluch + */ +public class DefaultQueryEnhancerUnitTests extends QueryEnhancerTckTests { + + @Override + QueryEnhancer createQueryEnhancer(DeclaredQuery declaredQuery) { + return new DefaultQueryEnhancer(declaredQuery); + } + +} diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JSqlParserQueryEnhancerUnitTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JSqlParserQueryEnhancerUnitTests.java new file mode 100644 index 000000000..940af9147 --- /dev/null +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JSqlParserQueryEnhancerUnitTests.java @@ -0,0 +1,30 @@ +/* + * Copyright 2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.jpa.repository.query; + +/** + * TCK Tests for {@link JSqlParserQueryEnhancer}. + * + * @author Mark Paluch + */ +public class JSqlParserQueryEnhancerUnitTests extends QueryEnhancerTckTests { + + @Override + QueryEnhancer createQueryEnhancer(DeclaredQuery declaredQuery) { + return new JSqlParserQueryEnhancer(declaredQuery); + } + +} diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryEnhancerTckTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryEnhancerTckTests.java new file mode 100644 index 000000000..cc8bb6155 --- /dev/null +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryEnhancerTckTests.java @@ -0,0 +1,87 @@ +/* + * Copyright 2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.jpa.repository.query; + +import static org.assertj.core.api.Assertions.*; + +import java.util.stream.Stream; + +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +/** + * TCK Tests for {@link QueryEnhancer}. + * + * @author Mark Paluch + */ +abstract class QueryEnhancerTckTests { + + @ParameterizedTest + @MethodSource("nativeCountQueries") // GH-2773 + void shouldDeriveNativeCountQuery(String query, String expected) { + + DeclaredQuery declaredQuery = DeclaredQuery.of(query, true); + QueryEnhancer enhancer = createQueryEnhancer(declaredQuery); + String countQueryFor = enhancer.createCountQueryFor(null); + + assertThat(countQueryFor).isEqualToIgnoringCase(expected); + } + + static Stream nativeCountQueries() { + + return Stream.of(Arguments.of( // + "SELECT * FROM table_name some_alias", // + "select count(1) FROM table_name some_alias"), // + + Arguments.of( // + "SELECT name FROM table_name some_alias", // + "select count(name) FROM table_name some_alias"), // + + Arguments.of( // + "SELECT DISTINCT name FROM table_name some_alias", // + "select count(DISTINCT name) FROM table_name some_alias")); + } + + @ParameterizedTest // GH-2773 + @MethodSource("jpqlCountQueries") + void shouldDeriveJpqlCountQuery(String query, String expected) { + + DeclaredQuery declaredQuery = DeclaredQuery.of(query, false); + QueryEnhancer enhancer = createQueryEnhancer(declaredQuery); + String countQueryFor = enhancer.createCountQueryFor(null); + + assertThat(countQueryFor).isEqualToIgnoringCase(expected); + } + + static Stream jpqlCountQueries() { + + return Stream.of(Arguments.of( // + "SELECT some_alias FROM table_name some_alias", // + "select count(some_alias) FROM table_name some_alias"), // + + Arguments.of( // + "SELECT name FROM table_name some_alias", // + "select count(name) FROM table_name some_alias"), // + + Arguments.of( // + "SELECT DISTINCT name FROM table_name some_alias", // + "select count(DISTINCT name) FROM table_name some_alias")); + } + + abstract QueryEnhancer createQueryEnhancer(DeclaredQuery declaredQuery); + +} diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryEnhancerUnitTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryEnhancerUnitTests.java index 5c1137aea..8cfe0e0a3 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryEnhancerUnitTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryEnhancerUnitTests.java @@ -15,9 +15,7 @@ */ package org.springframework.data.jpa.repository.query; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatExceptionOfType; -import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.*; import java.util.Arrays; import java.util.Collections; @@ -229,7 +227,12 @@ class QueryEnhancerUnitTests { @Test // DATAJPA-420 void createsCountQueryForScalarSelects() { - assertCountQuery("select p.lastname,p.firstname from Person p", "select count(p) from Person p", true); + assertCountQuery("select p.lastname,p.firstname from Person p", "select count(p) from Person p", false); + } + + @Test // DATAJPA-420 + void createsCountQueryForNativeScalarSelects() { + assertCountQuery("select p.lastname,p.firstname from Person p", "select count(1) from Person p", true); } @Test // DATAJPA-456 @@ -490,7 +493,7 @@ class QueryEnhancerUnitTests { " order by user.name\n ", true); assertThat(getEnhancer(query).createCountQueryFor()) - .isEqualToIgnoringCase("select count(user) from User user where user.age = 18"); + .isEqualToIgnoringCase("select count(1) from User user where user.age = 18"); } @Test @@ -503,7 +506,7 @@ class QueryEnhancerUnitTests { " order\nby\nuser.name\n ", true); assertThat(getEnhancer(query).createCountQueryFor()) - .isEqualToIgnoringCase("select count(user) from User user where user.age = 18"); + .isEqualToIgnoringCase("select count(1) from User user where user.age = 18"); } @Test // DATAJPA-1061 @@ -724,17 +727,17 @@ class QueryEnhancerUnitTests { QueryEnhancer queryEnhancer = getEnhancer(nativeQuery); String countQueryFor = queryEnhancer.createCountQueryFor(); - assertThat(countQueryFor).isEqualTo("SELECT count(*) FROM User WHERE created_at > $1"); + assertThat(countQueryFor).isEqualTo("SELECT count(1) FROM User WHERE created_at > $1"); nativeQuery = new StringQuery("SELECT * FROM (select * from test) ", true); queryEnhancer = getEnhancer(nativeQuery); countQueryFor = queryEnhancer.createCountQueryFor(); - assertThat(countQueryFor).isEqualTo("SELECT count(*) FROM (SELECT * FROM test)"); + assertThat(countQueryFor).isEqualTo("SELECT count(1) FROM (SELECT * FROM test)"); nativeQuery = new StringQuery("SELECT * FROM (select * from test) as test", true); queryEnhancer = getEnhancer(nativeQuery); countQueryFor = queryEnhancer.createCountQueryFor(); - assertThat(countQueryFor).isEqualTo("SELECT count(test) FROM (SELECT * FROM test) AS test"); + assertThat(countQueryFor).isEqualTo("SELECT count(1) FROM (SELECT * FROM test) AS test"); } @Test // GH-2555 @@ -864,7 +867,7 @@ class QueryEnhancerUnitTests { assertThat(queryEnhancer.createCountQueryFor()).isEqualToIgnoringCase( "with sample_data (day, value) AS (VALUES ((0, 13), (1, 12), (2, 15), (3, 4), (4, 8), (5, 16)))\n" - + "SELECT count(a) FROM sample_data AS a"); + + "SELECT count(1) FROM sample_data AS a"); assertThat(queryEnhancer.applySorting(Sort.by("day").descending())).endsWith("ORDER BY a.day DESC"); assertThat(queryEnhancer.getJoinAliases()).isEmpty(); assertThat(queryEnhancer.detectAlias()).isEqualToIgnoringCase("a"); @@ -887,7 +890,7 @@ class QueryEnhancerUnitTests { assertThat(queryEnhancer.createCountQueryFor()).isEqualToIgnoringCase( "with sample_data (day, value) AS (VALUES ((0, 13), (1, 12), (2, 15), (3, 4), (4, 8), (5, 16))),test2 AS (VALUES (1, 2, 3))\n" - + "SELECT count(a) FROM sample_data AS a"); + + "SELECT count(1) FROM sample_data AS a"); assertThat(queryEnhancer.applySorting(Sort.by("day").descending())).endsWith("ORDER BY a.day DESC"); assertThat(queryEnhancer.getJoinAliases()).isEmpty(); assertThat(queryEnhancer.detectAlias()).isEqualToIgnoringCase("a"); @@ -985,4 +988,5 @@ class QueryEnhancerUnitTests { private static QueryEnhancer getEnhancer(DeclaredQuery query) { return QueryEnhancerFactory.forQuery(query); } + }