From 39e12eae6d64ba7192835e052a082b93b287d911 Mon Sep 17 00:00:00 2001 From: "Greg L. Turnquist" Date: Wed, 15 Mar 2023 11:08:30 -0500 Subject: [PATCH] Introduce support for ordering by aliased columns. If a projection of either an HQL or JPQL query is aliased, do NOT prefix the FROM clause's alias prefix to any relevant applied sorting. Same for function-based order by arguments. Resolves #2863. Related: #2626, #2322, #1655. Original pull request: #2865. --- .../data/jpa/repository/query/Jpql.g4 | 14 +- .../repository/query/HqlQueryTransformer.java | 75 ++++------ .../query/JpaQueryParserSupport.java | 26 ---- .../query/JpaQueryTransformerSupport.java | 134 ++++++++++++++++++ .../query/JpqlQueryTransformer.java | 59 ++++---- .../query/HqlQueryTransformerTests.java | 121 +++++++++++----- .../query/JpqlQueryTransformerTests.java | 51 +++---- 7 files changed, 306 insertions(+), 174 deletions(-) create mode 100644 spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryTransformerSupport.java diff --git a/spring-data-jpa/src/main/antlr4/org/springframework/data/jpa/repository/query/Jpql.g4 b/spring-data-jpa/src/main/antlr4/org/springframework/data/jpa/repository/query/Jpql.g4 index 9bad5f4c7..00491c841 100644 --- a/spring-data-jpa/src/main/antlr4/org/springframework/data/jpa/repository/query/Jpql.g4 +++ b/spring-data-jpa/src/main/antlr4/org/springframework/data/jpa/repository/query/Jpql.g4 @@ -68,11 +68,11 @@ identification_variable_declaration ; range_variable_declaration - : entity_name (AS)? identification_variable + : entity_name AS? identification_variable ; join - : join_spec join_association_path_expression (AS)? identification_variable (join_condition)? + : join_spec join_association_path_expression AS? identification_variable (join_condition)? ; fetch_join @@ -103,7 +103,7 @@ join_single_valued_path_expression ; collection_member_declaration - : IN '(' collection_valued_path_expression ')' (AS)? identification_variable + : IN '(' collection_valued_path_expression ')' AS? identification_variable ; qualified_identification_variable @@ -160,7 +160,7 @@ collection_valued_path_expression ; update_clause - : UPDATE entity_name ((AS)? identification_variable)? SET update_item (',' update_item)* + : UPDATE entity_name (AS? identification_variable)? SET update_item (',' update_item)* ; update_item @@ -174,7 +174,7 @@ new_value ; delete_clause - : DELETE FROM entity_name ((AS)? identification_variable)? + : DELETE FROM entity_name (AS? identification_variable)? ; select_clause @@ -182,7 +182,7 @@ select_clause ; select_item - : select_expression ((AS)? result_variable)? + : select_expression (AS? result_variable)? ; select_expression @@ -247,7 +247,7 @@ subquery_from_clause subselect_identification_variable_declaration : identification_variable_declaration - | derived_path_expression (AS)? identification_variable (join)* + | derived_path_expression AS? identification_variable (join)* | derived_collection_member_declaration ; diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/HqlQueryTransformer.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/HqlQueryTransformer.java index 906942f75..18a40573e 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/HqlQueryTransformer.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/HqlQueryTransformer.java @@ -41,13 +41,15 @@ class HqlQueryTransformer extends HqlQueryRenderer { private final @Nullable String countProjection; - private @Nullable String alias = null; + private @Nullable String primaryFromAlias = null; private List projection = Collections.emptyList(); private boolean projectionProcessed; private boolean hasConstructorExpression = false; + private JpaQueryTransformerSupport transformerSupport; + HqlQueryTransformer() { this(Sort.unsorted(), false, null); } @@ -67,11 +69,12 @@ class HqlQueryTransformer extends HqlQueryRenderer { this.sort = sort; this.countQuery = countQuery; this.countProjection = countProjection; + this.transformerSupport = new JpaQueryTransformerSupport(); } @Nullable public String getAlias() { - return this.alias; + return this.primaryFromAlias; } public List getProjection() { @@ -118,7 +121,7 @@ class HqlQueryTransformer extends HqlQueryRenderer { tokens.addAll(visit(ctx.queryOrder())); } - if (this.sort.isSorted()) { + if (sort.isSorted()) { if (ctx.queryOrder() != null) { @@ -130,29 +133,7 @@ class HqlQueryTransformer extends HqlQueryRenderer { tokens.add(TOKEN_ORDER_BY); } - this.sort.forEach(order -> { - - JpaQueryParserSupport.checkSortExpression(order); - - if (order.isIgnoreCase()) { - tokens.add(TOKEN_LOWER_FUNC); - } - tokens.add(new JpaQueryParsingToken(() -> { - - if (order.getProperty().contains("(")) { - return order.getProperty(); - } - - return this.alias + "." + order.getProperty(); - }, true)); - if (order.isIgnoreCase()) { - NOSPACE(tokens); - tokens.add(TOKEN_CLOSE_PAREN); - } - tokens.add(order.isDescending() ? TOKEN_DESC : TOKEN_ASC); - tokens.add(TOKEN_COMMA); - }); - CLIP(tokens); + tokens.addAll(transformerSupport.generateOrderByArguments(primaryFromAlias, sort)); } } else { @@ -176,7 +157,7 @@ class HqlQueryTransformer extends HqlQueryRenderer { if (countProjection != null) { tokens.add(new JpaQueryParsingToken(countProjection)); } else { - tokens.add(new JpaQueryParsingToken(() -> this.alias, false)); + tokens.add(new JpaQueryParsingToken(() -> primaryFromAlias, false)); } tokens.add(TOKEN_CLOSE_PAREN); @@ -240,8 +221,8 @@ class HqlQueryTransformer extends HqlQueryRenderer { if (ctx.variable() != null) { tokens.addAll(visit(ctx.variable())); - if (this.alias == null && !isSubquery(ctx)) { - this.alias = tokens.get(tokens.size() - 1).getToken(); + if (primaryFromAlias == null && !isSubquery(ctx)) { + primaryFromAlias = tokens.get(tokens.size() - 1).getToken(); } } else { @@ -250,8 +231,8 @@ class HqlQueryTransformer extends HqlQueryRenderer { tokens.add(TOKEN_AS); tokens.add(TOKEN_DOUBLE_UNDERSCORE); - if (this.alias == null && !isSubquery(ctx)) { - this.alias = TOKEN_DOUBLE_UNDERSCORE.getToken(); + if (primaryFromAlias == null && !isSubquery(ctx)) { + primaryFromAlias = TOKEN_DOUBLE_UNDERSCORE.getToken(); } } } @@ -267,8 +248,8 @@ class HqlQueryTransformer extends HqlQueryRenderer { if (ctx.variable() != null) { tokens.addAll(visit(ctx.variable())); - if (this.alias == null && !isSubquery(ctx)) { - this.alias = tokens.get(tokens.size() - 1).getToken(); + if (primaryFromAlias == null && !isSubquery(ctx)) { + primaryFromAlias = tokens.get(tokens.size() - 1).getToken(); } } } @@ -302,16 +283,22 @@ class HqlQueryTransformer extends HqlQueryRenderer { @Override public List visitAlias(HqlParser.AliasContext ctx) { - List tokens = newArrayList(); + List tokens = super.visitAlias(ctx); - if (ctx.AS() != null) { - tokens.add(new JpaQueryParsingToken(ctx.AS())); + if (primaryFromAlias == null && !isSubquery(ctx)) { + primaryFromAlias = tokens.get(tokens.size() - 1).getToken(); } - tokens.addAll(visit(ctx.identifier())); + return tokens; + } - if (this.alias == null && !isSubquery(ctx)) { - this.alias = tokens.get(tokens.size() - 1).getToken(); + @Override + public List visitVariable(HqlParser.VariableContext ctx) { + + List tokens = super.visitVariable(ctx); + + if (ctx.identifier() != null) { + transformerSupport.registerAlias(tokens.get(tokens.size() - 1).getToken()); } return tokens; @@ -346,13 +333,13 @@ class HqlQueryTransformer extends HqlQueryRenderer { if (selectionListTokens.stream().anyMatch(hqlToken -> hqlToken.getToken().contains("new"))) { // constructor - tokens.add(new JpaQueryParsingToken(() -> this.alias)); + tokens.add(new JpaQueryParsingToken(() -> primaryFromAlias)); } else { // keep all the select items to distinct against tokens.addAll(selectionListTokens); } } else { - tokens.add(new JpaQueryParsingToken(() -> this.alias)); + tokens.add(new JpaQueryParsingToken(() -> primaryFromAlias)); } } @@ -363,8 +350,8 @@ class HqlQueryTransformer extends HqlQueryRenderer { } if (!projectionProcessed && !isSubquery(ctx)) { - this.projection = selectionListTokens; - this.projectionProcessed = true; + projection = selectionListTokens; + projectionProcessed = true; } return tokens; @@ -373,7 +360,7 @@ class HqlQueryTransformer extends HqlQueryRenderer { @Override public List visitInstantiation(HqlParser.InstantiationContext ctx) { - this.hasConstructorExpression = true; + hasConstructorExpression = true; return super.visitInstantiation(ctx); } diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryParserSupport.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryParserSupport.java index 892476ad2..a1a866536 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryParserSupport.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryParserSupport.java @@ -18,15 +18,12 @@ package org.springframework.data.jpa.repository.query; import static org.springframework.data.jpa.repository.query.JpaQueryParsingToken.*; import java.util.List; -import java.util.regex.Pattern; import org.antlr.v4.runtime.Lexer; import org.antlr.v4.runtime.Parser; import org.antlr.v4.runtime.ParserRuleContext; import org.antlr.v4.runtime.atn.PredictionMode; -import org.springframework.dao.InvalidDataAccessApiUsageException; import org.springframework.data.domain.Sort; -import org.springframework.data.jpa.domain.JpaSort; import org.springframework.data.util.Lazy; import org.springframework.lang.Nullable; @@ -39,12 +36,6 @@ import org.springframework.lang.Nullable; */ abstract class JpaQueryParserSupport { - private static final Pattern PUNCTUATION_PATTERN = Pattern.compile(".*((?![._])[\\p{Punct}|\\s])"); - - private static final String UNSAFE_PROPERTY_REFERENCE = "Sort expression '%s' must only contain property references or " - + "aliases used in the select clause; If you really want to use something other than that for sorting, please use " - + "JpaSort.unsafe(…)"; - private final ParseState state; JpaQueryParserSupport(String query) { @@ -177,23 +168,6 @@ abstract class JpaQueryParserSupport { protected abstract boolean doCheckForConstructor(ParserRuleContext parsedQuery); - /** - * Check any given {@link JpaSort.JpaOrder#isUnsafe()} order for presence of at least one property offending the - * {@link #PUNCTUATION_PATTERN} and throw an {@link Exception} indicating potential unsafe order by expression. - * - * @param order - */ - static void checkSortExpression(Sort.Order order) { - - if (order instanceof JpaSort.JpaOrder && ((JpaSort.JpaOrder) order).isUnsafe()) { - return; - } - - if (PUNCTUATION_PATTERN.matcher(order.getProperty()).find()) { - throw new InvalidDataAccessApiUsageException(String.format(UNSAFE_PROPERTY_REFERENCE, order)); - } - } - /** * Parser state capturing the lazily-parsed parser context. */ diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryTransformerSupport.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryTransformerSupport.java new file mode 100644 index 000000000..acc73c772 --- /dev/null +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryTransformerSupport.java @@ -0,0 +1,134 @@ +package org.springframework.data.jpa.repository.query; + +import static org.springframework.data.jpa.repository.query.JpaQueryParsingToken.*; + +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Pattern; + +import org.springframework.dao.InvalidDataAccessApiUsageException; +import org.springframework.data.domain.Sort; +import org.springframework.data.jpa.domain.JpaSort; +import org.springframework.lang.Nullable; + +/** + * Transformational operations needed to support either {@link HqlQueryTransformer} or {@link JpqlQueryTransformer}. + * + * @author Greg Turnquist + * @since 3.1 + */ +class JpaQueryTransformerSupport { + + private static final Pattern PUNCTUATION_PATTERN = Pattern.compile(".*((?![._])[\\p{Punct}|\\s])"); + + private static final String UNSAFE_PROPERTY_REFERENCE = "Sort expression '%s' must only contain property references or " + + "aliases used in the select clause; If you really want to use something other than that for sorting, please use " + + "JpaSort.unsafe(…)"; + + private Set projectionAliases; + + JpaQueryTransformerSupport() { + this.projectionAliases = new HashSet<>(); + } + + /** + * Register an {@literal alias} so it can later be evaluated when applying {@link Sort}s. + * + * @param token + */ + void registerAlias(String token) { + projectionAliases.add(token); + } + + /** + * Using the primary {@literal FROM} clause's alias and a {@link Sort}, construct all the {@literal ORDER BY} + * arguments. + * + * @param primaryFromAlias + * @param sort + * @return + */ + List generateOrderByArguments(String primaryFromAlias, Sort sort) { + + List tokens = new ArrayList<>(); + + sort.forEach(order -> { + + checkSortExpression(order); + + if (order.isIgnoreCase()) { + tokens.add(TOKEN_LOWER_FUNC); + } + + tokens.add(new JpaQueryParsingToken(() -> generateOrderByArgument(primaryFromAlias, order))); + + if (order.isIgnoreCase()) { + NOSPACE(tokens); + tokens.add(TOKEN_CLOSE_PAREN); + } + tokens.add(order.isDescending() ? TOKEN_DESC : TOKEN_ASC); + tokens.add(TOKEN_COMMA); + }); + CLIP(tokens); + + return tokens; + } + + /** + * Check any given {@link JpaSort.JpaOrder#isUnsafe()} order for presence of at least one property offending the + * {@link #PUNCTUATION_PATTERN} and throw an {@link Exception} indicating potential unsafe order by expression. + * + * @param order + */ + private void checkSortExpression(Sort.Order order) { + + if (order instanceof JpaSort.JpaOrder && ((JpaSort.JpaOrder) order).isUnsafe()) { + return; + } + + if (PUNCTUATION_PATTERN.matcher(order.getProperty()).find()) { + throw new InvalidDataAccessApiUsageException(String.format(UNSAFE_PROPERTY_REFERENCE, order)); + } + } + + /** + * Using the {@code primaryFromAlias} and the {@link org.springframework.data.domain.Sort.Order}, construct a suitable + * argument to be added to an {@literal ORDER BY} expression. + * + * @param primaryFromAlias + * @param order + * @return + */ + private String generateOrderByArgument(@Nullable String primaryFromAlias, Sort.Order order) { + + if (shouldPrefixWithAlias(order)) { + return primaryFromAlias + "." + order.getProperty(); + } else { + return order.getProperty(); + } + } + + /** + * Determine when an {@link org.springframework.data.domain.Sort.Order} parameter should be prefixed with the primary + * FROM clause's alias. + * + * @param order + * @return boolean whether or not to apply the primary FROM clause's alias as a prefix + */ + private boolean shouldPrefixWithAlias(Sort.Order order) { + + // If the Sort contains a function + if (order.getProperty().contains("(")) { + return false; + } + + // If the Sort references an alias + if (projectionAliases.contains(order.getProperty())) { + return false; + } + + return true; + } +} diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpqlQueryTransformer.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpqlQueryTransformer.java index 3749250ff..a89abcb68 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpqlQueryTransformer.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpqlQueryTransformer.java @@ -39,13 +39,15 @@ class JpqlQueryTransformer extends JpqlQueryRenderer { private final @Nullable String countProjection; - private @Nullable String alias = null; + private @Nullable String primaryFromAlias = null; private List projection = Collections.emptyList(); private boolean projectionProcessed; private boolean hasConstructorExpression = false; + private JpaQueryTransformerSupport transformerSupport; + JpqlQueryTransformer() { this(Sort.unsorted(), false, null); } @@ -65,11 +67,12 @@ class JpqlQueryTransformer extends JpqlQueryRenderer { this.sort = sort; this.countQuery = countQuery; this.countProjection = countProjection; + this.transformerSupport = new JpaQueryTransformerSupport(); } @Nullable public String getAlias() { - return this.alias; + return this.primaryFromAlias; } public List getProjection() { @@ -106,7 +109,7 @@ class JpqlQueryTransformer extends JpqlQueryRenderer { tokens.addAll(visit(ctx.orderby_clause())); } - if (this.sort.isSorted()) { + if (sort.isSorted()) { if (ctx.orderby_clause() != null) { @@ -118,29 +121,7 @@ class JpqlQueryTransformer extends JpqlQueryRenderer { tokens.add(TOKEN_ORDER_BY); } - this.sort.forEach(order -> { - - JpaQueryParserSupport.checkSortExpression(order); - - if (order.isIgnoreCase()) { - tokens.add(TOKEN_LOWER_FUNC); - } - tokens.add(new JpaQueryParsingToken(() -> { - - if (order.getProperty().contains("(")) { - return order.getProperty(); - } - - return this.alias + "." + order.getProperty(); - }, true)); - if (order.isIgnoreCase()) { - NOSPACE(tokens); - tokens.add(TOKEN_CLOSE_PAREN); - } - tokens.add(order.isDescending() ? TOKEN_DESC : TOKEN_ASC); - tokens.add(TOKEN_COMMA); - }); - CLIP(tokens); + tokens.addAll(transformerSupport.generateOrderByArguments(primaryFromAlias, sort)); } } @@ -182,13 +163,13 @@ class JpqlQueryTransformer extends JpqlQueryRenderer { if (selectItemTokens.stream().anyMatch(jpqlToken -> jpqlToken.getToken().contains("new"))) { // constructor - tokens.add(new JpaQueryParsingToken(() -> this.alias)); + tokens.add(new JpaQueryParsingToken(() -> primaryFromAlias)); } else { // keep all the select items to distinct against tokens.addAll(selectItemTokens); } } else { - tokens.add(new JpaQueryParsingToken(() -> this.alias)); + tokens.add(new JpaQueryParsingToken(() -> primaryFromAlias)); } } @@ -199,8 +180,20 @@ class JpqlQueryTransformer extends JpqlQueryRenderer { } if (!projectionProcessed) { - this.projection = selectItemTokens; - this.projectionProcessed = true; + projection = selectItemTokens; + projectionProcessed = true; + } + + return tokens; + } + + @Override + public List visitSelect_item(JpqlParser.Select_itemContext ctx) { + + List tokens = super.visitSelect_item(ctx); + + if (ctx.result_variable() != null) { + transformerSupport.registerAlias(tokens.get(tokens.size() - 1).getToken()); } return tokens; @@ -219,8 +212,8 @@ class JpqlQueryTransformer extends JpqlQueryRenderer { tokens.addAll(visit(ctx.identification_variable())); - if (this.alias == null) { - this.alias = tokens.get(tokens.size() - 1).getToken(); + if (primaryFromAlias == null) { + primaryFromAlias = tokens.get(tokens.size() - 1).getToken(); } return tokens; @@ -229,7 +222,7 @@ class JpqlQueryTransformer extends JpqlQueryRenderer { @Override public List visitConstructor_expression(JpqlParser.Constructor_expressionContext ctx) { - this.hasConstructorExpression = true; + hasConstructorExpression = true; return super.visitConstructor_expression(ctx); } diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/HqlQueryTransformerTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/HqlQueryTransformerTests.java index 6d33af2fe..0b691a4c3 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/HqlQueryTransformerTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/HqlQueryTransformerTests.java @@ -25,6 +25,7 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.springframework.dao.InvalidDataAccessApiUsageException; +import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Sort; import org.springframework.data.jpa.domain.JpaSort; import org.springframework.lang.Nullable; @@ -392,94 +393,85 @@ class HqlQueryTransformerTests { assertThat(createQueryFor("select p from Person p", sort)).endsWith("order by sum(foo) asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixMultipleAliasedFunctionCalls() { String query = "SELECT AVG(m.price) AS avgPrice, SUM(m.stocks) AS sumStocks FROM Magazine m"; Sort sort = Sort.by("avgPrice", "sumStocks"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by avgPrice asc, sumStocks asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by avgPrice asc, sumStocks asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixSingleAliasedFunctionCalls() { String query = "SELECT AVG(m.price) AS avgPrice FROM Magazine m"; Sort sort = Sort.by("avgPrice"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by avgPrice asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by avgPrice asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void prefixesSingleNonAliasedFunctionCallRelatedSortProperty() { String query = "SELECT AVG(m.price) AS avgPrice FROM Magazine m"; Sort sort = Sort.by("someOtherProperty"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by m.someOtherProperty asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by m.someOtherProperty asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void prefixesNonAliasedFunctionCallRelatedSortPropertyWhenSelectClauseContainsAliasedFunctionForDifferentProperty() { String query = "SELECT m.name, AVG(m.price) AS avgPrice FROM Magazine m"; Sort sort = Sort.by("name", "avgPrice"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by m.name asc, avgPrice asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by m.name asc, avgPrice asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixAliasedFunctionCallNameWithMultipleNumericParameters() { String query = "SELECT SUBSTRING(m.name, 2, 5) AS trimmedName FROM Magazine m"; Sort sort = Sort.by("trimmedName"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by trimmedName asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by trimmedName asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixAliasedFunctionCallNameWithMultipleStringParameters() { String query = "SELECT CONCAT(m.name, 'foo') AS extendedName FROM Magazine m"; Sort sort = Sort.by("extendedName"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by extendedName asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by extendedName asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixAliasedFunctionCallNameWithUnderscores() { String query = "SELECT AVG(m.price) AS avg_price FROM Magazine m"; Sort sort = Sort.by("avg_price"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by avg_price asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by avg_price asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixAliasedFunctionCallNameWithDots() { String query = "SELECT AVG(m.price) AS m.avg FROM Magazine m"; Sort sort = Sort.by("m.avg"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by m.avg asc"); + assertThatIllegalArgumentException().isThrownBy(() -> createQueryFor(query, sort)); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixAliasedFunctionCallNameWhenQueryStringContainsMultipleWhiteSpaces() { String query = "SELECT AVG( m.price ) AS avgPrice FROM Magazine m"; Sort sort = Sort.by("avgPrice"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by avgPrice asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by avgPrice asc"); } @Test // DATAJPA-1506 @@ -529,7 +521,7 @@ class HqlQueryTransformerTests { """); } - @Test // DATAJPA-1061 + @Test // DATAJPA-1061, GH-2863 void appliesSortCorrectlyForFieldAliases() { String query = "SELECT m.price, lower(m.title) AS title, a.name as authorName FROM Magazine m INNER JOIN m.author a"; @@ -537,10 +529,10 @@ class HqlQueryTransformerTests { String fullQuery = createQueryFor(query, sort); - assertThat(fullQuery).endsWith("order by m.authorName asc"); + assertThat(fullQuery).endsWith("order by authorName asc"); } - @Test // GH-2280 + @Test // GH-2280, GH-2863 void appliesOrderingCorrectlyForFieldAliasWithIgnoreCase() { String query = "SELECT customer.id as id, customer.name as name FROM CustomerEntity customer"; @@ -549,10 +541,10 @@ class HqlQueryTransformerTests { String fullQuery = createQueryFor(query, sort); assertThat(fullQuery).isEqualTo( - "SELECT customer.id as id, customer.name as name FROM CustomerEntity customer order by lower(customer.name) asc"); + "SELECT customer.id as id, customer.name as name FROM CustomerEntity customer order by lower(name) asc"); } - @Test // DATAJPA-1061 + @Test // DATAJPA-1061, GH-2863 void appliesSortCorrectlyForFunctionAliases() { String query = "SELECT m.price, lower(m.title) AS title, a.name as authorName FROM Magazine m INNER JOIN m.author a"; @@ -560,7 +552,7 @@ class HqlQueryTransformerTests { String fullQuery = createQueryFor(query, sort); - assertThat(fullQuery).endsWith("order by m.title asc"); + assertThat(fullQuery).endsWith("order by title asc"); } @Test // DATAJPA-1061 @@ -902,7 +894,6 @@ class HqlQueryTransformerTests { .isEqualTo("u"); } - @Test // GH-2348 void removeFetchFromJoinsDuringCountQueryCreation() { @@ -913,6 +904,68 @@ class HqlQueryTransformerTests { "SELECT count(DISTINCT b) FROM Board b LEFT JOIN b.comments"); } + @Test // GH-2626, GH-2863 + void orderByAliasedColumn() { + + assertThat(createQueryFor(""" + select + max(resource.name) as resourceName, + max(resource.id) as id, + max(resource.description) as description, + max(resource.uuid) as uuid, + max(resource.type) as type, + max(resource.createdOn) as createdOn, + max(users.firstName) as authorFirstName, + max(users.lastName) as authorLastName, + max(file.version) as version, + max(file.comment) as comment, + file.deployed as deployed, + max(log.date) as modifiedOn + from Resource resource + where ( + cast(:startDate as date) is null + or resource.latestLogRecord.date between cast(:startDate as date) and cast(:endDate as date) + ) + group by resource.id, file.deployed, log.author.firstName, file.comment + """, Sort.by(Sort.Direction.DESC, "uuid"))).endsWith("order by uuid desc"); + } + + @Test // GH-2863, GH-2322 + void sortShouldWorkWhenAliasingFunctions() { + + assertThat(createQueryFor(""" + SELECT + DISTINCT(event.id) as id, + event.name as name, + MIN(bundle.base_price_amount) as cheapestBundlePrice, + MIN(DATE(bundle.start)) as earliestBundleStart + FROM event event + LEFT JOIN bundle bundle ON event.id = bundle.event_id + GROUP BY event.id + """, Sort.by(Sort.Direction.ASC, "cheapestBundlePrice") // + .and(Sort.by(Sort.Direction.ASC, "earliestBundleStart")) // + .and(Sort.by(Sort.Direction.ASC, "name")))) + .endsWith(" order by cheapestBundlePrice asc, earliestBundleStart asc, name asc"); + } + + @Test // GH-2863, GH-1655 + void shouldHandleAliasInsideCaseStatement() { + + Sort sort = PageRequest.of(0, 20, Sort.Direction.DESC, "newDateDue").getSort(); + + assertThat(createQueryFor("Select DISTINCT new " + // + "com.api.dto.FilterDTO(c.id, p.id, CASE WHEN item.dateDue IS NOT NULL THEN item.dateDue ELSE p.dateDue END AS newDateDue) " + + "FROM Customer c " + // + "join c.productOrder p " + // + "JOIN p.items item", // + sort)).isEqualTo("Select DISTINCT new " + // + "com.api.dto.FilterDTO(c.id, p.id, CASE WHEN item.dateDue IS NOT NULL THEN item.dateDue ELSE p.dateDue END AS newDateDue) " + + "FROM Customer c " + // + "join c.productOrder p " + // + "JOIN p.items item " + // + "order by newDateDue desc"); + } + private void assertCountQuery(String originalQuery, String countQuery) { assertThat(createCountQueryFor(originalQuery)).isEqualTo(countQuery); } diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JpqlQueryTransformerTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JpqlQueryTransformerTests.java index c9beede02..bd2b6a469 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JpqlQueryTransformerTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JpqlQueryTransformerTests.java @@ -383,94 +383,85 @@ class JpqlQueryTransformerTests { assertThat(createQueryFor("select p from Person p", sort)).endsWith("order by sum(foo) asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixMultipleAliasedFunctionCalls() { String query = "SELECT AVG(m.price) AS avgPrice, SUM(m.stocks) AS sumStocks FROM Magazine m"; Sort sort = Sort.by("avgPrice", "sumStocks"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by avgPrice asc, sumStocks asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by avgPrice asc, sumStocks asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixSingleAliasedFunctionCalls() { String query = "SELECT AVG(m.price) AS avgPrice FROM Magazine m"; Sort sort = Sort.by("avgPrice"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by avgPrice asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by avgPrice asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void prefixesSingleNonAliasedFunctionCallRelatedSortProperty() { String query = "SELECT AVG(m.price) AS avgPrice FROM Magazine m"; Sort sort = Sort.by("someOtherProperty"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by m.someOtherProperty asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by m.someOtherProperty asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void prefixesNonAliasedFunctionCallRelatedSortPropertyWhenSelectClauseContainsAliasedFunctionForDifferentProperty() { String query = "SELECT m.name, AVG(m.price) AS avgPrice FROM Magazine m"; Sort sort = Sort.by("name", "avgPrice"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by m.name asc, avgPrice asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by m.name asc, avgPrice asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixAliasedFunctionCallNameWithMultipleNumericParameters() { String query = "SELECT SUBSTRING(m.name, 2, 5) AS trimmedName FROM Magazine m"; Sort sort = Sort.by("trimmedName"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by trimmedName asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by trimmedName asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixAliasedFunctionCallNameWithMultipleStringParameters() { String query = "SELECT CONCAT(m.name, 'foo') AS extendedName FROM Magazine m"; Sort sort = Sort.by("extendedName"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by extendedName asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by extendedName asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixAliasedFunctionCallNameWithUnderscores() { String query = "SELECT AVG(m.price) AS avg_price FROM Magazine m"; Sort sort = Sort.by("avg_price"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by avg_price asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by avg_price asc"); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixAliasedFunctionCallNameWithDots() { String query = "SELECT AVG(m.price) AS m.avg FROM Magazine m"; Sort sort = Sort.by("m.avg"); - // TODO: Add support for aliased functions - // assertThat(query(query, (Sort) "m")).endsWith("order by m.avg asc"); + assertThatIllegalArgumentException().isThrownBy(() -> createQueryFor(query, sort)); } - @Test // DATAJPA-965, DATAJPA-970 + @Test // DATAJPA-965, DATAJPA-970, GH-2863 void doesNotPrefixAliasedFunctionCallNameWhenQueryStringContainsMultipleWhiteSpaces() { String query = "SELECT AVG( m.price ) AS avgPrice FROM Magazine m"; Sort sort = Sort.by("avgPrice"); - // TODO: Add support for aliased functions - // assertThat(createQueryFor(query, sort)).endsWith("order by avgPrice asc"); + assertThat(createQueryFor(query, sort)).endsWith("order by avgPrice asc"); } @Test // DATAJPA-1506 @@ -528,7 +519,7 @@ class JpqlQueryTransformerTests { String fullQuery = createQueryFor(query, sort); - assertThat(fullQuery).endsWith("order by m.authorName asc"); + assertThat(fullQuery).endsWith("order by authorName asc"); } @Test // GH-2280 @@ -540,7 +531,7 @@ class JpqlQueryTransformerTests { String fullQuery = createQueryFor(query, sort); assertThat(fullQuery).isEqualTo( - "SELECT customer.id as id, customer.name as name FROM CustomerEntity customer order by lower(customer.name) asc"); + "SELECT customer.id as id, customer.name as name FROM CustomerEntity customer order by lower(name) asc"); } @Test // DATAJPA-1061 @@ -551,7 +542,7 @@ class JpqlQueryTransformerTests { String fullQuery = createQueryFor(query, sort); - assertThat(fullQuery).endsWith("order by m.title asc"); + assertThat(fullQuery).endsWith("order by title asc"); } @Test // DATAJPA-1061