From b02081b439acabf78b0d0497d11cdc4090bb0a0a Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 16 Apr 2024 08:25:08 +0200 Subject: [PATCH] Fix order by rendering for queries containing UNION. Make sure to append space after order by clause and fix alias detection for wrapped sub select. Also make sure to ignore alias used in subselect so they do not conflict with root ones. Render order by only on full select if set operator is present in EQL. Original pull request: #3429 Closes #3427 --- .../query/EqlSortedQueryTransformer.java | 6 +- .../query/HqlSortedQueryTransformer.java | 101 +++++++++++------- .../query/JpaQueryTransformerSupport.java | 2 +- .../query/EqlQueryTransformerTests.java | 9 ++ .../query/HqlQueryTransformerTests.java | 38 +++++++ 5 files changed, 117 insertions(+), 39 deletions(-) diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/EqlSortedQueryTransformer.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/EqlSortedQueryTransformer.java index 525f48acf..ed14e9afd 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/EqlSortedQueryTransformer.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/EqlSortedQueryTransformer.java @@ -23,6 +23,7 @@ import org.springframework.data.domain.Sort; import org.springframework.data.jpa.repository.query.QueryRenderer.QueryRendererBuilder; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.ObjectUtils; /** * An ANTLR {@link org.antlr.v4.runtime.tree.ParseTreeVisitor} that transforms a parsed EQL query by applying @@ -30,6 +31,7 @@ import org.springframework.util.Assert; * * @author Greg Turnquist * @author Mark Paluch + * @author Christoph Strobl * @since 3.2 */ @SuppressWarnings("ConstantValue") @@ -67,7 +69,7 @@ class EqlSortedQueryTransformer extends EqlQueryRenderer { builder.appendExpression(visit(ctx.having_clause())); } - doVisitOrderBy(builder, ctx); + doVisitOrderBy(builder, ctx, ObjectUtils.isEmpty(ctx.setOperator()) ? this.sort : Sort.unsorted()); for (int i = 0; i < ctx.setOperator().size(); i++) { @@ -78,7 +80,7 @@ class EqlSortedQueryTransformer extends EqlQueryRenderer { return builder; } - private void doVisitOrderBy(QueryRendererBuilder builder, EqlParser.Select_statementContext ctx) { + private void doVisitOrderBy(QueryRendererBuilder builder, EqlParser.Select_statementContext ctx, Sort sort) { if (ctx.orderby_clause() != null) { QueryTokenStream existingOrder = visit(ctx.orderby_clause()); diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/HqlSortedQueryTransformer.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/HqlSortedQueryTransformer.java index fc2860404..f439ad669 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/HqlSortedQueryTransformer.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/HqlSortedQueryTransformer.java @@ -23,6 +23,7 @@ import org.springframework.data.domain.Sort; import org.springframework.data.jpa.repository.query.QueryRenderer.QueryRendererBuilder; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.ObjectUtils; /** * An ANTLR {@link org.antlr.v4.runtime.tree.ParseTreeVisitor} that transforms a parsed HQL query. @@ -46,8 +47,72 @@ class HqlSortedQueryTransformer extends HqlQueryRenderer { this.primaryFromAlias = primaryFromAlias; } + + public QueryTokenStream visitQueryExpression(HqlParser.QueryExpressionContext ctx) { + + if(ObjectUtils.isEmpty(ctx.setOperator())) { + return super.visitQueryExpression(ctx); + } + + QueryRendererBuilder builder = QueryRenderer.builder(); + if (ctx.withClause() != null) { + builder.appendExpression(visit(ctx.withClause())); + } + + builder.append(visitOrderedQuery(ctx.orderedQuery(0), Sort.unsorted())); + + for (int i = 1; i < ctx.orderedQuery().size(); i++) { + + builder.append(visit(ctx.setOperator(i - 1))); + builder.append(visit(ctx.orderedQuery(i))); + } + + + return builder; + } + @Override public QueryRendererBuilder visitOrderedQuery(HqlParser.OrderedQueryContext ctx) { + return visitOrderedQuery(ctx, this.sort); + } + + @Override + public QueryTokenStream visitJoinPath(HqlParser.JoinPathContext ctx) { + + QueryTokenStream tokens = super.visitJoinPath(ctx); + + if (ctx.variable() != null && !isSubquery(ctx)) { + transformerSupport.registerAlias(tokens.getLast()); + } + + return tokens; + } + + @Override + public QueryTokenStream visitJoinSubquery(HqlParser.JoinSubqueryContext ctx) { + + QueryTokenStream tokens = super.visitJoinSubquery(ctx); + + if (ctx.variable() != null && !tokens.isEmpty() && !isSubquery(ctx)) { + transformerSupport.registerAlias(tokens.getLast()); + } + + return tokens; + } + + @Override + public QueryTokenStream visitVariable(HqlParser.VariableContext ctx) { + + QueryTokenStream tokens = super.visitVariable(ctx); + + if (ctx.identifier() != null && !tokens.isEmpty() && !isSubquery(ctx)) { + transformerSupport.registerAlias(tokens.getLast()); + } + + return tokens; + } + + private QueryRendererBuilder visitOrderedQuery(HqlParser.OrderedQueryContext ctx, Sort sort) { QueryRendererBuilder builder = QueryRenderer.builder(); @@ -95,40 +160,4 @@ class HqlSortedQueryTransformer extends HqlQueryRenderer { return builder; } - @Override - public QueryTokenStream visitJoinPath(HqlParser.JoinPathContext ctx) { - - QueryTokenStream tokens = super.visitJoinPath(ctx); - - if (ctx.variable() != null) { - transformerSupport.registerAlias(tokens.getLast()); - } - - return tokens; - } - - @Override - public QueryTokenStream visitJoinSubquery(HqlParser.JoinSubqueryContext ctx) { - - QueryTokenStream tokens = super.visitJoinSubquery(ctx); - - if (ctx.variable() != null && !tokens.isEmpty()) { - transformerSupport.registerAlias(tokens.getLast()); - } - - return tokens; - } - - @Override - public QueryTokenStream visitVariable(HqlParser.VariableContext ctx) { - - QueryTokenStream tokens = super.visitVariable(ctx); - - if (ctx.identifier() != null && !tokens.isEmpty()) { - transformerSupport.registerAlias(tokens.getLast()); - } - - return tokens; - } - } 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 index c5e4cacba..5ec36640e 100644 --- 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 @@ -53,7 +53,7 @@ class JpaQueryTransformerSupport { * @param sort * @return */ - List orderBy(String primaryFromAlias, Sort sort) { + List orderBy(@Nullable String primaryFromAlias, Sort sort) { List tokens = new ArrayList<>(); diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/EqlQueryTransformerTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/EqlQueryTransformerTests.java index 52bdcd82d..e0de136bc 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/EqlQueryTransformerTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/EqlQueryTransformerTests.java @@ -759,6 +759,15 @@ class EqlQueryTransformerTests { """); } + @Test // GH-3427 + void sortShouldBeAppendedToFullSelectOnlyInCaseOfSetOperator() { + + String source = "SELECT tb FROM Test tb WHERE (tb.type='A') UNION SELECT tb FROM Test tb WHERE (tb.type='B')"; + String target = createQueryFor(source, Sort.by("Type").ascending()); + + assertThat(target).isEqualTo("SELECT tb FROM Test tb WHERE (tb.type = 'A') UNION SELECT tb FROM Test tb WHERE (tb.type = 'B') order by tb.Type asc"); + } + static Stream queriesWithReservedWordsAsIdentifiers() { return Stream.of( // 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 b4ce21db0..44350d2f0 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 @@ -17,6 +17,8 @@ package org.springframework.data.jpa.repository.query; import static org.assertj.core.api.Assertions.*; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import java.util.stream.Stream; import org.assertj.core.api.SoftAssertions; @@ -24,11 +26,13 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; 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; +import org.springframework.util.StringUtils; /** * Verify that HQL queries are properly transformed through the {@link JpaQueryEnhancer} and the @@ -1061,6 +1065,40 @@ class HqlQueryTransformerTests { "select count(distinct a, count(b)) from Employee AS __ GROUP BY n"); } + @Test // GH-3427 + void sortShouldBeAppendedWithSpacingInCaseOfSetOperator() { + + String source = "SELECT tb FROM Test tb WHERE (tb.type='A') UNION SELECT tb FROM Test tb WHERE (tb.type='B')"; + String target = createQueryFor(source, Sort.by("Type").ascending()); + + assertThat(target).isEqualTo("SELECT tb FROM Test tb WHERE (tb.type = 'A') UNION SELECT tb FROM Test tb WHERE (tb.type = 'B') order by tb.Type asc"); + } + + @ParameterizedTest // GH-3427 + @ValueSource(strings = {"", "res"}) + void sortShouldBeAppendedToSubSelectWithSetOperatorInSubselect(String alias) { + + String prefix = StringUtils.hasText(alias) ? (alias + ".") : ""; + String source = "SELECT %sname FROM (SELECT c.name as name FROM Category c UNION SELECT t.name as name FROM Tag t)".formatted(prefix); + if(StringUtils.hasText(alias)) { + source = source + " %s".formatted(alias); + } + + String target = createQueryFor(source, Sort.by("name").ascending()); + + assertThat(target).contains(" UNION SELECT ").doesNotContainPattern(Pattern.compile(".*\\SUNION")); + assertThat(target).endsWith("order by %sname asc".formatted(prefix)).satisfies(it -> { + Pattern pattern = Pattern.compile("order by %sname".formatted(prefix)); + Matcher matcher = pattern.matcher(target); + int count = 0; + while(matcher.find()) { + count++; + } + assertThat(count).describedAs("Found order by clause more than once in: \n%s", it).isOne(); + }); + + } + private void assertCountQuery(String originalQuery, String countQuery) { assertThat(createCountQueryFor(originalQuery)).isEqualTo(countQuery); }