Properly handle nested ORDER BY clauses.

In several ways, it's possible to have various ORDER BY clauses, including SQL OVER (windowing) clauses. Need to improve handling of ORDER BY.

See #2260.

Author: TAndronicus

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java

@@ -75,6 +75,7 @@
  * @author Peter Großmann
  * @author Greg Turnquist
  * @author Diego Krupitza
+ * @author Jędrzej Biedrzycki
  */
 public abstract class QueryUtils {
 
@@ -108,7 +109,8 @@ public abstract class QueryUtils {
 	private static final Pattern JOIN_PATTERN = Pattern.compile(JOIN, Pattern.CASE_INSENSITIVE);
 
 	private static final String EQUALS_CONDITION_STRING = "%s.%s = :%s";
-	private static final Pattern ORDER_BY = Pattern.compile(".*order\\s+by\\s+.*", CASE_INSENSITIVE);
+	private static final Pattern ORDER_BY = Pattern.compile("(order\\s+by\\s+)", CASE_INSENSITIVE);
+	private static final Pattern ORDER_BY_IN_WINDOW_OR_SUBSELECT = Pattern.compile("(\\(\\s*[a-z0-9 ,.*]*order\\s+by\\s+[a-z0-9 ,.]*\\s*\\))", CASE_INSENSITIVE);
 
 	private static final Pattern NAMED_PARAMETER = Pattern.compile(COLON_NO_DOUBLE_COLON + IDENTIFIER + "|#" + IDENTIFIER,
 			CASE_INSENSITIVE);
@@ -257,10 +259,10 @@ public static String applySorting(String query, Sort sort, @Nullable String alia
 
 		StringBuilder builder = new StringBuilder(query);
 
-		if (!ORDER_BY.matcher(query).matches()) {
-			builder.append(" order by ");
-		} else {
+		if (hasOrderByClause(query)) {
 			builder.append(", ");
+		} else {
+			builder.append(" order by ");
 		}
 
 		Set<String> joinAliases = getOuterJoinAliases(query);
@@ -276,6 +278,31 @@ public static String applySorting(String query, Sort sort, @Nullable String alia
 		return builder.toString();
 	}
 
+	/**
+	 * Returns {@code true} if the query has {@code order by} clause.
+	 * The query has {@code order by} clause if there is an {@code order by} which is not part of window clause.
+	 *
+	 * @param query the analysed query string
+	 * @return {@code true} if the query has {@code order by} clause, {@code false} otherwise
+	 */
+	private static boolean hasOrderByClause(String query) {
+		return countOccurences(ORDER_BY, query) > countOccurences(ORDER_BY_IN_WINDOW_OR_SUBSELECT, query);
+	}
+
+	/**
+	 * Counts the number of occurrences of the pattern in the string
+	 *
+	 * @param pattern regex with a group to match
+	 * @param string analysed string
+	 * @return the number of occurences of the pattern in the string
+	 */
+	private static int countOccurences(Pattern pattern, String string) {
+		Matcher matcher = pattern.matcher(string);
+		int occurences = 0;
+		while (matcher.find()) occurences++;
+		return occurences;
+	}
+
 	/**
 	 * Returns the order clause for the given {@link Order}. Will prefix the clause with the given alias if the referenced
 	 * property refers to a join alias, i.e. starts with {@code $alias.}.
@@ -397,10 +424,12 @@ private static String toJpaDirection(Order order) {
 	@Nullable
 	@Deprecated
 	public static String detectAlias(String query) {
-
+		String alias = null;
 		Matcher matcher = ALIAS_MATCH.matcher(query);
-
-		return matcher.find() ? matcher.group(2) : null;
+		while (matcher.find()) {
+			alias = matcher.group(2);
+		}
+		return alias;
 	}
 
 	/**

spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsUnitTests.java

@@ -40,6 +40,7 @@
  * @author Grégoire Druant
  * @author Mohammad Hewedy
  * @author Greg Turnquist
+ * @author Jędrzej Biedrzycki
  */
 class QueryUtilsUnitTests {
 
@@ -552,4 +553,79 @@ void countProjectionDistinctQueryIncludesNewLineAfterEntityAndBeforeWhere() {
 	private static void assertCountQuery(String originalQuery, String countQuery) {
 		assertThat(createCountQueryFor(originalQuery)).isEqualTo(countQuery);
 	}
+
+	@Test // GH-2260
+	void applySortingAccountsForNativeWindowFunction() {
+
+		Sort sort = Sort.by(Order.desc("age"));
+
+		// order by absent
+		assertThat(QueryUtils.applySorting("select * from user u", sort))
+				.isEqualTo("select * from user u order by u.age desc");
+
+		// order by present
+		assertThat(QueryUtils.applySorting("select * from user u order by u.lastname", sort))
+				.isEqualTo("select * from user u order by u.lastname, u.age desc");
+
+		// partition by
+		assertThat(QueryUtils.applySorting("select dense_rank() over (partition by age) from user u", sort))
+				.isEqualTo("select dense_rank() over (partition by age) from user u order by u.age desc");
+
+		// order by in over clause
+		assertThat(QueryUtils.applySorting("select dense_rank() over (order by lastname) from user u", sort))
+				.isEqualTo("select dense_rank() over (order by lastname) from user u order by u.age desc");
+
+		// order by in over clause (additional spaces)
+		assertThat(QueryUtils.applySorting("select dense_rank() over ( order by lastname ) from user u", sort))
+				.isEqualTo("select dense_rank() over ( order by lastname ) from user u order by u.age desc");
+
+		// order by in over clause + at the end
+		assertThat(
+				QueryUtils.applySorting("select dense_rank() over (order by lastname) from user u order by u.lastname", sort))
+						.isEqualTo("select dense_rank() over (order by lastname) from user u order by u.lastname, u.age desc");
+
+		// partition by + order by in over clause
+		assertThat(QueryUtils.applySorting(
+				"select dense_rank() over (partition by active, age order by lastname) from user u", sort)).isEqualTo(
+						"select dense_rank() over (partition by active, age order by lastname) from user u order by u.age desc");
+
+		// partition by + order by in over clause + order by at the end
+		assertThat(QueryUtils.applySorting(
+				"select dense_rank() over (partition by active, age order by lastname) from user u order by active", sort))
+						.isEqualTo(
+								"select dense_rank() over (partition by active, age order by lastname) from user u order by active, u.age desc");
+
+		// partition by + order by in over clause + frame clause
+		assertThat(QueryUtils.applySorting(
+				"select dense_rank() over ( partition by active, age order by username rows between current row and unbounded following ) from user u",
+				sort)).isEqualTo(
+						"select dense_rank() over ( partition by active, age order by username rows between current row and unbounded following ) from user u order by u.age desc");
+
+		// partition by + order by in over clause + frame clause + order by at the end
+		assertThat(QueryUtils.applySorting(
+				"select dense_rank() over ( partition by active, age order by username rows between current row and unbounded following ) from user u order by active",
+				sort)).isEqualTo(
+						"select dense_rank() over ( partition by active, age order by username rows between current row and unbounded following ) from user u order by active, u.age desc");
+
+		// order by in subselect (select expression)
+		assertThat(
+				QueryUtils.applySorting("select lastname, (select i.id from item i order by i.id limit 1) from user u", sort))
+						.isEqualTo(
+								"select lastname, (select i.id from item i order by i.id limit 1) from user u order by u.age desc");
+
+		// order by in subselect (select expression) + at the end
+		assertThat(QueryUtils.applySorting(
+				"select lastname, (select i.id from item i order by 1 limit 1) from user u order by active", sort)).isEqualTo(
+						"select lastname, (select i.id from item i order by 1 limit 1) from user u order by active, u.age desc");
+
+		// order by in subselect (from expression)
+		assertThat(QueryUtils.applySorting("select * from (select * from user order by age desc limit 10) u", sort))
+				.isEqualTo("select * from (select * from user order by age desc limit 10) u order by age desc");
+
+		// order by in subselect (from expression) + at the end
+		assertThat(QueryUtils.applySorting(
+				"select * from (select * from user order by 1, 2, 3 desc limit 10) u order by u.active asc", sort)).isEqualTo(
+						"select * from (select * from user order by 1, 2, 3 desc limit 10) u order by u.active asc, age desc");
+	}
+
 }