Skip to content

Fix order by rendering for queries containing UNION #3429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Fix order by rendering for queries containing UNION #3429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
61b4030
Prepare issue branch.
christophstrobl Apr 16, 2024
25beda5
Fix order by rendering for queries containing UNION
christophstrobl Apr 16, 2024
82b28a4
hacking
christophstrobl Jun 27, 2024
c5064ed
a little better :)
christophstrobl Jun 27, 2024
132d54b
Render order by only on full select if set operator is present in EQL.
christophstrobl Jun 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-jpa-parent</artifactId>
<version>3.4.0-SNAPSHOT</version>
<version>3.4.x-3427-SNAPSHOT</version>
<packaging>pom</packaging>

<name>Spring Data JPA Parent</name>
Expand Down
4 changes: 2 additions & 2 deletions spring-data-envers/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-envers</artifactId>
<version>3.4.0-SNAPSHOT</version>
<version>3.4.x-3427-SNAPSHOT</version>

<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-jpa-parent</artifactId>
<version>3.4.0-SNAPSHOT</version>
<version>3.4.x-3427-SNAPSHOT</version>
<relativePath>../pom.xml</relativePath>
</parent>

Expand Down
2 changes: 1 addition & 1 deletion spring-data-jpa-distribution/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-jpa-parent</artifactId>
<version>3.4.0-SNAPSHOT</version>
<version>3.4.x-3427-SNAPSHOT</version>
<relativePath>../pom.xml</relativePath>
</parent>

Expand Down
4 changes: 2 additions & 2 deletions spring-data-jpa/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-jpa</artifactId>
<version>3.4.0-SNAPSHOT</version>
<version>3.4.x-3427-SNAPSHOT</version>

<name>Spring Data JPA</name>
<description>Spring Data module for JPA repositories.</description>
Expand All @@ -15,7 +15,7 @@
<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-jpa-parent</artifactId>
<version>3.4.0-SNAPSHOT</version>
<version>3.4.x-3427-SNAPSHOT</version>
<relativePath>../pom.xml</relativePath>
</parent>

Expand Down
6 changes: 4 additions & 2 deletions ...rc/main/java/org/springframework/data/jpa/repository/query/EqlSortedQueryTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,15 @@
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
* {@link Sort}.
*
* @author Greg Turnquist
* @author Mark Paluch
* @author Christoph Strobl
* @since 3.2
*/
@SuppressWarnings("ConstantValue")
Expand Down Expand Up @@ -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++) {

Expand All @@ -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());
Expand Down
101 changes: 65 additions & 36 deletions ...rc/main/java/org/springframework/data/jpa/repository/query/HqlSortedQueryTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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();

Expand Down Expand Up @@ -95,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) {
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;
}

}
2 changes: 1 addition & 1 deletion ...c/main/java/org/springframework/data/jpa/repository/query/JpaQueryTransformerSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ void registerAlias(QueryToken token) {
* @param sort
* @return
*/
List<QueryToken> orderBy(String primaryFromAlias, Sort sort) {
List<QueryToken> orderBy(@Nullable String primaryFromAlias, Sort sort) {

List<QueryToken> tokens = new ArrayList<>();

Expand Down
9 changes: 9 additions & 0 deletions ...src/test/java/org/springframework/data/jpa/repository/query/EqlQueryTransformerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Arguments> queriesWithReservedWordsAsIdentifiers() {

return Stream.of( //
Expand Down
38 changes: 38 additions & 0 deletions ...src/test/java/org/springframework/data/jpa/repository/query/HqlQueryTransformerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,22 @@

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;
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
Expand Down Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this line target has the value

SELECT tb 
    FROM Test tb 
    WHERE (tb.type = 'A') 
    order by tb.Type asc 
UNION 
SELECT tb 
    FROM Test tb 
    WHERE (tb.type = 'B') 
    order by tb.Type asc`

i.e. the order by is applied twice which is illegal, since according to answers here https://stackoverflow.com/questions/4715820/how-to-order-by-with-union-in-sql the later order by applies to the full union and an order by on the individual selects might be even illegal (probably depends on the database)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we do not know the target db nor how it handles order by we cannot imply ordering the last is the correct behaviour, nor do we have the means to define which of the selects to decorate. an alternative would be to raise an error and demand wrapping the entire thing as it's done in one of the follow up tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, let's wait and see what kind of reports we get. We can then still react to the feedback we receive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reading through IBM an Oracle SQL reference docs @schauder has a good point in the above mentioned query not being compliant (at least not in the form it is currently rendered)

For example, the following is not valid (SQLSTATE 428FJ):
SELECT * FROM T1
ORDER BY C1
UNION
SELECT * FROM T2
ORDER BY C1

The following example is valid:
(SELECT * FROM T1
ORDER BY C1)
UNION
(SELECT * FROM T2
ORDER BY C1)


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"})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment would be nice explaining why we need to run this with different prefixes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deemed to be obvious - apparently isn't

void sortShouldBeAppendedToSubSelectWithSetOperatorInSubselect(String alias) {

String prefix = StringUtils.hasText(alias) ? (alias + ".") : "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again I think we should be able to and prefer a simple equals-assertion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of duplication.

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);
}
Expand Down