From 61b40309cce39e33d3c6a2639ffb959158f893fe Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 16 Apr 2024 07:47:28 +0200 Subject: [PATCH 1/5] Prepare issue branch. --- pom.xml | 2 +- spring-data-envers/pom.xml | 4 ++-- spring-data-jpa-distribution/pom.xml | 2 +- spring-data-jpa/pom.xml | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pom.xml b/pom.xml index 617cc011d3..c876280cb5 100755 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-jpa-parent - 3.4.0-SNAPSHOT + 3.4.x-3427-SNAPSHOT pom Spring Data JPA Parent diff --git a/spring-data-envers/pom.xml b/spring-data-envers/pom.xml index 8b836ae2f3..5896ecf294 100755 --- a/spring-data-envers/pom.xml +++ b/spring-data-envers/pom.xml @@ -5,12 +5,12 @@ org.springframework.data spring-data-envers - 3.4.0-SNAPSHOT + 3.4.x-3427-SNAPSHOT org.springframework.data spring-data-jpa-parent - 3.4.0-SNAPSHOT + 3.4.x-3427-SNAPSHOT ../pom.xml diff --git a/spring-data-jpa-distribution/pom.xml b/spring-data-jpa-distribution/pom.xml index a90c1f7282..3ab9ac6565 100644 --- a/spring-data-jpa-distribution/pom.xml +++ b/spring-data-jpa-distribution/pom.xml @@ -14,7 +14,7 @@ org.springframework.data spring-data-jpa-parent - 3.4.0-SNAPSHOT + 3.4.x-3427-SNAPSHOT ../pom.xml diff --git a/spring-data-jpa/pom.xml b/spring-data-jpa/pom.xml index 3006702796..2065b14663 100644 --- a/spring-data-jpa/pom.xml +++ b/spring-data-jpa/pom.xml @@ -6,7 +6,7 @@ org.springframework.data spring-data-jpa - 3.4.0-SNAPSHOT + 3.4.x-3427-SNAPSHOT Spring Data JPA Spring Data module for JPA repositories. @@ -15,7 +15,7 @@ org.springframework.data spring-data-jpa-parent - 3.4.0-SNAPSHOT + 3.4.x-3427-SNAPSHOT ../pom.xml From 25beda50c42f164129ee356882a01b588b12290e Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 16 Apr 2024 08:25:08 +0200 Subject: [PATCH 2/5] 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. --- .../query/HqlSortedQueryTransformer.java | 6 +-- .../query/HqlQueryTransformerTests.java | 38 +++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) 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 fc28604047..e6ec3d34de 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 @@ -100,7 +100,7 @@ public QueryTokenStream visitJoinPath(HqlParser.JoinPathContext ctx) { QueryTokenStream tokens = super.visitJoinPath(ctx); - if (ctx.variable() != null) { + if (ctx.variable() != null && !isSubquery(ctx)) { transformerSupport.registerAlias(tokens.getLast()); } @@ -112,7 +112,7 @@ public QueryTokenStream visitJoinSubquery(HqlParser.JoinSubqueryContext ctx) { QueryTokenStream tokens = super.visitJoinSubquery(ctx); - if (ctx.variable() != null && !tokens.isEmpty()) { + if (ctx.variable() != null && !tokens.isEmpty() && !isSubquery(ctx)) { transformerSupport.registerAlias(tokens.getLast()); } @@ -124,7 +124,7 @@ public QueryTokenStream visitVariable(HqlParser.VariableContext ctx) { QueryTokenStream tokens = super.visitVariable(ctx); - if (ctx.identifier() != null && !tokens.isEmpty()) { + if (ctx.identifier() != null && !tokens.isEmpty() && !isSubquery(ctx)) { transformerSupport.registerAlias(tokens.getLast()); } 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 b4ce21db0a..917e6f32ae 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 @@ 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.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 @@ void createsCountQueryUsingAliasCorrectly() { "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).contains(" UNION SELECT ").doesNotContainPattern(Pattern.compile(".*\\SUNION")); + } + + @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); } From 82b28a42435c731f387c47dd48d24381a2135bc2 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 27 Jun 2024 13:18:45 +0200 Subject: [PATCH 3/5] hacking --- .../query/HqlSortedQueryTransformer.java | 32 ++++++++++++++++++- .../query/HqlQueryTransformerTests.java | 5 ++- 2 files changed, 35 insertions(+), 2 deletions(-) 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 e6ec3d34de..23d9ca9cf7 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.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. @@ -35,7 +36,7 @@ class HqlSortedQueryTransformer extends HqlQueryRenderer { private final JpaQueryTransformerSupport transformerSupport = new JpaQueryTransformerSupport(); - private final Sort sort; + private Sort sort; private final @Nullable String primaryFromAlias; HqlSortedQueryTransformer(Sort sort, @Nullable String primaryFromAlias) { @@ -46,9 +47,38 @@ 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())); + } + + Sort tmp = this.sort; + this.sort = Sort.unsorted(); + builder.append(visit(ctx.orderedQuery(0))); + this.sort = tmp; + + 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) { + QueryRendererBuilder builder = QueryRenderer.builder(); if (ctx.query() != null) { 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 917e6f32ae..529ac4c5f5 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 @@ -1070,8 +1070,11 @@ 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()); + + System.out.println("target: " + target); - assertThat(target).contains(" UNION SELECT ").doesNotContainPattern(Pattern.compile(".*\\SUNION")); + 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"); +// assertThat(target).contains(" UNION SELECT ").doesNotContainPattern(Pattern.compile(".*\\SUNION")); } @ParameterizedTest // GH-3427 From c5064ededb9ba79124f881ea78ca6b43f64e8cc3 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 27 Jun 2024 15:30:22 +0200 Subject: [PATCH 4/5] a little better :) --- .../query/HqlSortedQueryTransformer.java | 83 +++++++++---------- .../query/JpaQueryTransformerSupport.java | 2 +- .../query/HqlQueryTransformerTests.java | 3 - 3 files changed, 42 insertions(+), 46 deletions(-) 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 23d9ca9cf7..f439ad6695 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 @@ -36,7 +36,7 @@ class HqlSortedQueryTransformer extends HqlQueryRenderer { private final JpaQueryTransformerSupport transformerSupport = new JpaQueryTransformerSupport(); - private Sort sort; + private final Sort sort; private final @Nullable String primaryFromAlias; HqlSortedQueryTransformer(Sort sort, @Nullable String primaryFromAlias) { @@ -59,10 +59,7 @@ public QueryTokenStream visitQueryExpression(HqlParser.QueryExpressionContext ct builder.appendExpression(visit(ctx.withClause())); } - Sort tmp = this.sort; - this.sort = Sort.unsorted(); - builder.append(visit(ctx.orderedQuery(0))); - this.sort = tmp; + builder.append(visitOrderedQuery(ctx.orderedQuery(0), Sort.unsorted())); for (int i = 1; i < ctx.orderedQuery().size(); i++) { @@ -74,10 +71,48 @@ public QueryTokenStream visitQueryExpression(HqlParser.QueryExpressionContext ct 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(); @@ -125,40 +160,4 @@ public QueryRendererBuilder visitOrderedQuery(HqlParser.OrderedQueryContext ctx) return builder; } - @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; - } - } 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 c5e4cacba4..5ec36640e9 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 @@ void registerAlias(QueryToken token) { * @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/HqlQueryTransformerTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/HqlQueryTransformerTests.java index 529ac4c5f5..44350d2f0a 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 @@ -1071,10 +1071,7 @@ 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()); - System.out.println("target: " + target); - 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"); -// assertThat(target).contains(" UNION SELECT ").doesNotContainPattern(Pattern.compile(".*\\SUNION")); } @ParameterizedTest // GH-3427 From 132d54b0e8fff95ed771c9dcf6d1910161124cd1 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Fri, 28 Jun 2024 11:24:20 +0200 Subject: [PATCH 5/5] Render order by only on full select if set operator is present in EQL. --- .../jpa/repository/query/EqlSortedQueryTransformer.java | 6 ++++-- .../jpa/repository/query/EqlQueryTransformerTests.java | 9 +++++++++ 2 files changed, 13 insertions(+), 2 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 525f48acfb..ed14e9afdf 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.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 @@ * * @author Greg Turnquist * @author Mark Paluch + * @author Christoph Strobl * @since 3.2 */ @SuppressWarnings("ConstantValue") @@ -67,7 +69,7 @@ public QueryRendererBuilder visitSelect_statement(EqlParser.Select_statementCont 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 @@ public QueryRendererBuilder visitSelect_statement(EqlParser.Select_statementCont 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/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 52bdcd82d1..e0de136bc8 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 @@ void sortingRecognizesJoinAliases() { """); } + @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( //