Skip to content

Commit

Permalink
Correctly handle order by aliases.
Browse files Browse the repository at this point in the history
In order to correctly identify aliases in the order by clause, we cannot process the from clauses left-to-right using regular expressions. These must be removed inner-to-outer. Commit c93aa25 resulted in a bug where the subquery would be incorrectly identified as the alias, as by the following query:

```
from Parent p join p.children c where not c.id not in (select c2.id from Child c2)
```

Passing in a Sort.by("name") would result in "order by c2.name" instead of "order by p.name". Thus, it was using the alias of the inner query instead of the outer query. [This comment](#2260 (comment)) suggests removing the content of the inner query, with the caveat of the entire query being surrounded by parenthesis. This commit does exactly that, by removing the subquery before the alias is identified. It also handles the case when the entire query is surrounded by parenthesis. Unit tests illustrate this along with several examples of removing the subquery to correctly identify the alias for the order by clause.

See #2260 (c93aa25), #2500, #2518.
  • Loading branch information
darinmanica authored and gregturn committed May 4, 2022
1 parent e5d0841 commit 44406b0
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
* @author Greg Turnquist
* @author Diego Krupitza
* @author Jędrzej Biedrzycki
* @author Darin Manica
*/
public abstract class QueryUtils {

Expand All @@ -100,6 +101,8 @@ public abstract class QueryUtils {

private static final Pattern ALIAS_MATCH;
private static final Pattern COUNT_MATCH;
private static final Pattern STARTS_WITH_PAREN = Pattern.compile("^\\s*\\(");
private static final Pattern PARENS_TO_REMOVE = Pattern.compile("(\\(.*\\bfrom\\b[^)]+\\))", CASE_INSENSITIVE);
private static final Pattern PROJECTION_CLAUSE = Pattern.compile("select\\s+(?:distinct\\s+)?(.+)\\s+from",
Pattern.CASE_INSENSITIVE);

Expand Down Expand Up @@ -431,13 +434,70 @@ private static String toJpaDirection(Order order) {
@Deprecated
public static String detectAlias(String query) {
String alias = null;
Matcher matcher = ALIAS_MATCH.matcher(query);
Matcher matcher = ALIAS_MATCH.matcher(removeSubqueries(query));
while (matcher.find()) {
alias = matcher.group(2);
}
return alias;
}

/**
* Remove subqueries from the query, in order to identify the correct alias
* in order by clauses. If the entire query is surrounded by parenthesis, the
* outermost parenthesis are not removed.
*
* @param query
* @return query with all subqueries removed.
*/
static String removeSubqueries(String query) {
if (!StringUtils.hasText(query)) {
return query;
}

final List<Integer> opens = new ArrayList<>();
final List<Integer> closes = new ArrayList<>();
final List<Boolean> closeMatches = new ArrayList<>();
for (int i=0; i<query.length(); i++) {
final char c = query.charAt(i);
if (c == '(') {
opens.add(i);
} else if (c == ')') {
closes.add(i);
closeMatches.add(Boolean.FALSE);
}
}

final StringBuilder sb = new StringBuilder(query);
final boolean startsWithParen = STARTS_WITH_PAREN.matcher(query).find();
for (int i=opens.size()-1; i>=(startsWithParen?1:0); i--) {
final Integer open = opens.get(i);
final Integer close = findClose(open, closes, closeMatches) + 1;


if (close > open) {
final String subquery = sb.substring(open, close);
final Matcher matcher = PARENS_TO_REMOVE.matcher(subquery);
if (matcher.find()) {
sb.replace(open, close, new String(new char[close-open]).replace('\0', ' '));
}
}
}

return sb.toString();
}

private static Integer findClose(final Integer open, final List<Integer> closes, final List<Boolean> closeMatches) {
for (int i=0; i<closes.size(); i++) {
final int close = closes.get(i);
if (close > open && !closeMatches.get(i)) {
closeMatches.set(i, Boolean.TRUE);
return close;
}
}

return -1;
}

/**
* Creates a where-clause referencing the given entities and appends it to the given query string. Binds the given
* entities to the query.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,16 @@

import java.util.Collections;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.assertj.core.api.SoftAssertions;
import org.junit.jupiter.api.Test;
import org.springframework.dao.InvalidDataAccessApiUsageException;
import org.springframework.data.domain.Sort;
import org.springframework.data.domain.Sort.Order;
import org.springframework.data.jpa.domain.JpaSort;
import org.springframework.util.StringUtils;

/**
* Unit test for {@link QueryUtils}.
Expand All @@ -41,6 +44,7 @@
* @author Mohammad Hewedy
* @author Greg Turnquist
* @author Jędrzej Biedrzycki
* @author Darin Manica
*/
class QueryUtilsUnitTests {

Expand All @@ -50,6 +54,7 @@ class QueryUtilsUnitTests {
private static final String COUNT_QUERY = "select count(u) from User u";

private static final String QUERY_WITH_AS = "select u from User as u where u.username = ?";
private static final Pattern MULTI_WHITESPACE = Pattern.compile("\\s+");

@Test
void createsCountQueryCorrectly() throws Exception {
Expand Down Expand Up @@ -104,7 +109,7 @@ void allowsShortJpaSyntax() throws Exception {
assertCountQuery(SIMPLE_QUERY, COUNT_QUERY);
}

@Test
@Test // GH-2260
void detectsAliasCorrectly() throws Exception {

assertThat(detectAlias(QUERY)).isEqualTo("u");
Expand All @@ -115,6 +120,47 @@ void detectsAliasCorrectly() throws Exception {
assertThat(detectAlias("select u from User u")).isEqualTo("u");
assertThat(detectAlias("select u from com.acme.User u")).isEqualTo("u");
assertThat(detectAlias("select u from T05User u")).isEqualTo("u");
assertThat(detectAlias("select u from User u where not exists (from User u2)")).isEqualTo("u");
assertThat(detectAlias("(select u from User u where not exists (from User u2))")).isEqualTo("u");
assertThat(detectAlias("(select u from User u where not exists ((from User u2 where not exists (from User u3))))")).isEqualTo("u");
assertThat(detectAlias("from Foo f left join f.bar b with type(b) = BarChild where (f.id = (select max(f.id) from Foo f2 where type(f2) = FooChild) or 1 <> 1) and 1=1")).isEqualTo("f");
assertThat(detectAlias("(from Foo f max(f) ((((select * from Foo f2 (from Foo f3) max(*)) (from Foo f4)) max(f5)) (f6)) (from Foo f7))")).isEqualTo("f");
}

@Test // GH-2260
void testRemoveSubqueries() throws Exception {
// boundary conditions
assertThat(removeSubqueries(null)).isNull();
assertThat(removeSubqueries("")).isEmpty();
assertThat(removeSubqueries(" ")).isEqualTo(" ");
assertThat(removeSubqueries("(")).isEqualTo("(");
assertThat(removeSubqueries(")")).isEqualTo(")");
assertThat(removeSubqueries("(()")).isEqualTo("(()");
assertThat(removeSubqueries("())")).isEqualTo("())");

// realistic conditions
assertThat(removeSubqueries(QUERY)).isEqualTo(QUERY);
assertThat(removeSubqueries(SIMPLE_QUERY)).isEqualTo(SIMPLE_QUERY);
assertThat(removeSubqueries(COUNT_QUERY)).isEqualTo(COUNT_QUERY);
assertThat(removeSubqueries(QUERY_WITH_AS)).isEqualTo(QUERY_WITH_AS);
assertThat(removeSubqueries("SELECT FROM USER U")).isEqualTo("SELECT FROM USER U");
assertThat(removeSubqueries("select u from User u")).isEqualTo("select u from User u");
assertThat(removeSubqueries("select u from com.acme.User u")).isEqualTo("select u from com.acme.User u");
assertThat(removeSubqueries("select u from T05User u")).isEqualTo("select u from T05User u");
assertThat(normalizeWhitespace(removeSubqueries("select u from User u where not exists (from User u2)"))).isEqualTo("select u from User u where not exists");
assertThat(normalizeWhitespace(removeSubqueries("(select u from User u where not exists (from User u2))"))).isEqualTo("(select u from User u where not exists )");
assertThat(normalizeWhitespace(removeSubqueries("select u from User u where not exists (from User u2 where not exists (from User u3))"))).isEqualTo("select u from User u where not exists");
assertThat(normalizeWhitespace(removeSubqueries("select u from User u where not exists ((from User u2 where not exists (from User u3)))"))).isEqualTo("select u from User u where not exists ( )");
assertThat(normalizeWhitespace(removeSubqueries("(select u from User u where not exists ((from User u2 where not exists (from User u3))))"))).isEqualTo("(select u from User u where not exists ( ))");
}

private String normalizeWhitespace(String s) {
Matcher matcher = MULTI_WHITESPACE.matcher(s);
if (matcher.find()) {
return matcher.replaceAll(" ").trim();
}

return StringUtils.trimWhitespace(s);
}

@Test
Expand Down

0 comments on commit 44406b0

Please sign in to comment.