Correctly handle order by aliases.
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. Commitc93aa25resulted 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](https://github.com/spring-projects/spring-data-jpa/issues/2260#issuecomment-929510358) 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.
This commit is contained in:
Darin Manica
committed by
Greg L. Turnquist
Greg L. Turnquist
parent
1f2e4a179a
commit
5103be3f28
@@ -76,6 +76,7 @@ import org.springframework.util.StringUtils;
|
||||
* @author Greg Turnquist
|
||||
* @author Diego Krupitza
|
||||
* @author Jędrzej Biedrzycki
|
||||
* @author Darin Manica
|
||||
*/
|
||||
public abstract class QueryUtils {
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -431,13 +434,70 @@ public abstract class QueryUtils {
|
||||
@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.
|
||||
|
||||
@@ -20,6 +20,8 @@ import static org.springframework.data.jpa.repository.query.QueryUtils.*;
|
||||
|
||||
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;
|
||||
@@ -27,6 +29,7 @@ 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}.
|
||||
@@ -41,6 +44,7 @@ import org.springframework.data.jpa.domain.JpaSort;
|
||||
* @author Mohammad Hewedy
|
||||
* @author Greg Turnquist
|
||||
* @author Jędrzej Biedrzycki
|
||||
* @author Darin Manica
|
||||
*/
|
||||
class QueryUtilsUnitTests {
|
||||
|
||||
@@ -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() {
|
||||
@@ -102,7 +107,7 @@ class QueryUtilsUnitTests {
|
||||
assertCountQuery(SIMPLE_QUERY, COUNT_QUERY);
|
||||
}
|
||||
|
||||
@Test
|
||||
@Test // GH-2260
|
||||
void detectsAliasCorrectly() {
|
||||
|
||||
assertThat(detectAlias(QUERY)).isEqualTo("u");
|
||||
@@ -113,6 +118,47 @@ class QueryUtilsUnitTests {
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user