Introduce Sort unSafe checks.

QueryUtils checks if a given sort parameter is "safe" or not. Pulled this into the parsing solution.

Also re-renabled a bunch of test cases, verifying much more functionality.

The one lingering thing I don't quite see how to implement are Sort parameters the reference function aliases. For example:

select avg(e.salary) as avg from Employee e // Sort.by("avg")

In this scenario, it should NOT prefix "order by avg" with the "e" alias. Perhaps, the easiest thing is to put ALL aliases into a Set and then if a given Sort properity is IN that set, don't apply the alias?

Author: gregturn

spring-data-jpa/src/main/antlr4/org/springframework/data/jpa/repository/query/Hql.g4

@@ -20,7 +20,7 @@ grammar Hql;
 /**
  * HQL per https://docs.jboss.org/hibernate/orm/6.1/userguide/html_single/Hibernate_User_Guide.html#query-language
  *
- * This is a mixture of Hibernate 6.1's BNF and missing bits of grammar. There are gaps and inconsistencies in the
+ * This is a mixture of Hibernate's BNF and missing bits of grammar. There are gaps and inconsistencies in the
  * BNF itself, explained by other fragments of their spec. Additionally, alternate labels are used to provide easier
  * management of complex rules in the generated Visitor. Finally, there are labels applied to rule elements (op=('+'|'-')
  * to simplify the processing.

spring-data-jpa/src/main/antlr4/org/springframework/data/jpa/repository/query/Jpql.g4

@@ -19,7 +19,7 @@ grammar Jpql;
 /**
  * JPQL per https://jakarta.ee/specifications/persistence/3.1/jakarta-persistence-spec-3.1.html#bnf
  *
- * This is JPA 3.1 BNF for JPQL. There are gaps and inconsistencies in the BNF itself, explained by other fragments of the spec.
+ * This is JPA BNF for JPQL. There are gaps and inconsistencies in the BNF itself, explained by other fragments of the spec.
  *
  * @see https://github.com/jakartaee/persistence/blob/master/spec/src/main/asciidoc/ch04-query-language.adoc#bnf
  * @author Greg Turnquist

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

@@ -24,7 +24,7 @@
 import org.springframework.lang.Nullable;
 
 /**
- * Implements the various parsing operations using the ANTLR-generated {@link HqlParser} and
+ * Implements the parsing operations of a {@link QueryParser} using the ANTLR-generated {@link HqlParser} and
  * {@link HqlQueryTransformer}.
  * 
  * @author Greg Turnquist
@@ -41,7 +41,7 @@ class HqlQueryParser extends QueryParser {
 	}
 
 	/**
-	 * Convenience method to parse an HQL query using the ANTLR-generated {@link HqlParser}.
+	 * Convenience method to parse an HQL query. Will throw a {@link QueryParsingSyntaxError} if the query is invalid.
 	 *
 	 * @param query
 	 * @return a parsed query, ready for postprocessing
@@ -67,7 +67,7 @@ ParserRuleContext parse() {
 	}
 
 	/**
-	 * Use the {@link HqlQueryTransformer} to transform the parsed query into a query with the {@link Sort} applied.
+	 * Use the {@link HqlQueryTransformer} to transform the original query into a query with the {@link Sort} applied.
 	 *
 	 * @param parsedQuery
 	 * @param sort can be {@literal null}
@@ -79,7 +79,7 @@ List<QueryParsingToken> doCreateQuery(ParserRuleContext parsedQuery, Sort sort)
 	}
 
 	/**
-	 * Use the {@link HqlQueryTransformer} to transform the parsed query into a count query.
+	 * Use the {@link HqlQueryTransformer} to transform the original query into a count query.
 	 *
 	 * @param parsedQuery
 	 * @param countProjection
@@ -91,7 +91,7 @@ List<QueryParsingToken> doCreateCountQuery(ParserRuleContext parsedQuery, @Nulla
 	}
 
 	/**
-	 * Using the parsed query, run it through the {@link HqlQueryTransformer} and look up its alias.
+	 * Run the parsed query through {@link HqlQueryTransformer} to find the primary FROM clause's alias.
 	 *
 	 * @param parsedQuery
 	 * @return can be {@literal null}
@@ -105,10 +105,10 @@ String findAlias(ParserRuleContext parsedQuery) {
 	}
 
 	/**
-	 * Discern if the query has a new {@code com.example.Dto()} DTO constructor in the select clause.
+	 * Use {@link HqlQueryTransformer} to find the projection of the query.
 	 *
 	 * @param parsedQuery
-	 * @return Guaranteed to be {@literal true} or {@literal false}.
+	 * @return
 	 */
 	@Override
 	List<QueryParsingToken> doFindProjection(ParserRuleContext parsedQuery) {
@@ -118,6 +118,13 @@ List<QueryParsingToken> doFindProjection(ParserRuleContext parsedQuery) {
 		return transformVisitor.getProjection();
 	}
 
+	/**
+	 * Use {@link HqlQueryTransformer} to detect if the query uses a {@code new com.example.Dto()} DTO constructor in the
+	 * primary select clause.
+	 *
+	 * @param parsedQuery
+	 * @return Guaranteed to be {@literal true} or {@literal false}.
+	 */
 	@Override
 	boolean hasConstructor(ParserRuleContext parsedQuery) {
 

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

@@ -25,7 +25,7 @@
 import org.springframework.lang.Nullable;
 
 /**
- * An ANTLR visitor that transforms a parsed HQL query.
+ * An ANTLR {@link org.antlr.v4.runtime.tree.ParseTreeVisitor} that transforms a parsed HQL query.
  *
  * @author Greg Turnquist
  * @since 3.1
@@ -76,7 +76,7 @@ public boolean hasConstructorExpression() {
 	}
 
 	/**
-	 * Is this a {@literal selectState} (main select statement) or a {@literal subquery}?
+	 * Is this select clause a {@literal subquery}?
 	 *
 	 * @return boolean
 	 */
@@ -167,10 +167,19 @@ public List<QueryParsingToken> visitOrderedQuery(HqlParser.OrderedQueryContext c
 
 				this.sort.forEach(order -> {
 
+					QueryParser.checkSortExpression(order);
+
 					if (order.isIgnoreCase()) {
 						tokens.add(new QueryParsingToken("lower(", false));
 					}
-					tokens.add(new QueryParsingToken(() -> this.alias + "." + order.getProperty(), true));
+					tokens.add(new QueryParsingToken(() -> {
+
+						if (order.getProperty().contains("(")) {
+							return order.getProperty();
+						}
+
+						return this.alias + "." + order.getProperty();
+					}, true));
 					if (order.isIgnoreCase()) {
 						NOSPACE(tokens);
 						tokens.add(new QueryParsingToken(")", true));

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

@@ -24,7 +24,7 @@
 import org.springframework.lang.Nullable;
 
 /**
- * Implements the various parsing operations using the ANTLR-generated {@link JpqlParser} and
+ * Implements the parsing operations of a {@link QueryParser} using the ANTLR-generated {@link JpqlParser} and
  * {@link JpqlQueryTransformer}.
  *
  * @author Greg Turnquist
@@ -41,7 +41,7 @@ class JpqlQueryParser extends QueryParser {
 	}
 
 	/**
-	 * Convenience method to parse a JPQL query using the ANTLR-generated {@link JpqlParser}.
+	 * Convenience method to parse a JPQL query. Will throw a {@link QueryParsingSyntaxError} if the query is invalid.
 	 *
 	 * @param query
 	 * @return a parsed query, ready for postprocessing
@@ -67,7 +67,7 @@ ParserRuleContext parse() {
 	}
 
 	/**
-	 * Use the {@link JpqlQueryTransformer} to transform the parsed query into a query with the {@link Sort} applied.
+	 * Use the {@link JpqlQueryTransformer} to transform the original query into a query with the {@link Sort} applied.
 	 *
 	 * @param parsedQuery
 	 * @param sort can be {@literal null}
@@ -79,7 +79,7 @@ List<QueryParsingToken> doCreateQuery(ParserRuleContext parsedQuery, Sort sort)
 	}
 
 	/**
-	 * Use the {@link JpqlQueryTransformer} to transform the parsed query into a count query.
+	 * Use the {@link JpqlQueryTransformer} to transform the original query into a count query.
 	 *
 	 * @param parsedQuery
 	 * @param countProjection
@@ -91,7 +91,7 @@ List<QueryParsingToken> doCreateCountQuery(ParserRuleContext parsedQuery, @Nulla
 	}
 
 	/**
-	 * Using the parsed query, run it through the {@link JpqlQueryTransformer} and look up its alias.
+	 * Run the parsed query through {@link JpqlQueryTransformer} to find the primary FROM clause's alias.
 	 *
 	 * @param parsedQuery
 	 * @return can be {@literal null}
@@ -105,7 +105,7 @@ String findAlias(ParserRuleContext parsedQuery) {
 	}
 
 	/**
-	 * Find the projection portion of the query.
+	 * Use {@link JpqlQueryTransformer} to find the projection of the query.
 	 *
 	 * @param parsedQuery
 	 * @return
@@ -119,7 +119,8 @@ List<QueryParsingToken> doFindProjection(ParserRuleContext parsedQuery) {
 	}
 
 	/**
-	 * Discern if the query has a new {@code com.example.Dto()} DTO constructor in the select clause.
+	 * Use {@link JpqlQueryTransformer} to detect if the query uses a {@code new com.example.Dto()} DTO constructor in the
+	 * primary select clause.
 	 *
 	 * @param parsedQuery
 	 * @return Guaranteed to be {@literal true} or {@literal false}.

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

@@ -24,7 +24,7 @@
 import org.springframework.lang.Nullable;
 
 /**
- * An ANTLR visitor that transforms a parsed JPQL query.
+ * An ANTLR {@link org.antlr.v4.runtime.tree.ParseTreeVisitor} that transforms a parsed JPQL query.
  *
  * @author Greg Turnquist
  * @since 3.1
@@ -61,6 +61,7 @@ private JpqlQueryTransformer(@Nullable Sort sort, boolean countQuery, @Nullable
 		this.countProjection = countProjection;
 	}
 
+	@Nullable
 	public String getAlias() {
 		return this.alias;
 	}
@@ -81,17 +82,15 @@ public List<QueryParsingToken> visitStart(JpqlParser.StartContext ctx) {
 	@Override
 	public List<QueryParsingToken> visitQl_statement(JpqlParser.Ql_statementContext ctx) {
 
-		List<QueryParsingToken> tokens = new ArrayList<>();
-
 		if (ctx.select_statement() != null) {
-			tokens.addAll(visit(ctx.select_statement()));
+			return visit(ctx.select_statement());
 		} else if (ctx.update_statement() != null) {
-			tokens.addAll(visit(ctx.update_statement()));
+			return visit(ctx.update_statement());
 		} else if (ctx.delete_statement() != null) {
-			tokens.addAll(visit(ctx.delete_statement()));
+			return visit(ctx.delete_statement());
+		} else {
+			return List.of();
 		}
-
-		return tokens;
 	}
 
 	@Override
@@ -134,10 +133,19 @@ public List<QueryParsingToken> visitSelect_statement(JpqlParser.Select_statement
 
 				this.sort.forEach(order -> {
 
+					QueryParser.checkSortExpression(order);
+
 					if (order.isIgnoreCase()) {
 						tokens.add(new QueryParsingToken("lower(", false));
 					}
-					tokens.add(new QueryParsingToken(() -> this.alias + "." + order.getProperty(), true));
+					tokens.add(new QueryParsingToken(() -> {
+
+						if (order.getProperty().contains("(")) {
+							return order.getProperty();
+						}
+
+						return this.alias + "." + order.getProperty();
+					}, true));
 					if (order.isIgnoreCase()) {
 						NOSPACE(tokens);
 						tokens.add(new QueryParsingToken(")", true));

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

@@ -18,9 +18,12 @@
 import static org.springframework.data.jpa.repository.query.QueryParsingToken.*;
 
 import java.util.List;
+import java.util.regex.Pattern;
 
 import org.antlr.v4.runtime.ParserRuleContext;
+import org.springframework.dao.InvalidDataAccessApiUsageException;
 import org.springframework.data.domain.Sort;
+import org.springframework.data.jpa.domain.JpaSort;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 
@@ -56,8 +59,8 @@ String getQuery() {
 	abstract ParserRuleContext parse();
 
 	/**
-	 * Create a string-based query using the original query with an @literal order by} added (or amended) based upon
-	 * {@link Sort}
+	 * Generate a query using the original query with an @literal order by} clause added (or amended) based upon the
+	 * provider {@link Sort} parameter.
 	 *
 	 * @param parsedQuery
 	 * @param sort can be {@literal null}
@@ -69,7 +72,7 @@ String createQuery(ParserRuleContext parsedQuery, Sort sort) {
 	}
 
 	/**
-	 * Create a string-based count query using the original query.
+	 * Generate a count-based query using the original query.
 	 *
 	 * @param parsedQuery
 	 * @param countProjection
@@ -92,7 +95,8 @@ String projection(ParserRuleContext parsedQuery) {
 	}
 
 	/**
-	 * Create a {@link QueryParsingToken}-based query with an {@literal order by} applied/amended based upon {@link Sort}.
+	 * Create a {@link QueryParsingToken}-based query with an {@literal order by} applied/amended based upon the
+	 * {@link Sort} parameter.
 	 * 
 	 * @param parsedQuery
 	 * @param sort can be {@literal null}
@@ -108,25 +112,48 @@ String projection(ParserRuleContext parsedQuery) {
 	abstract List<QueryParsingToken> doCreateCountQuery(ParserRuleContext parsedQuery, @Nullable String countProjection);
 
 	/**
-	 * Find the alias of the query's FROM clause
+	 * Find the alias of the query's primary FROM clause
 	 *
 	 * @return can be {@literal null}
 	 */
 	abstract String findAlias(ParserRuleContext parsedQuery);
 
 	/**
-	 * Find the projection of the query's selection clause.
+	 * Find the projection of the query's primary SELECT clause.
 	 *
 	 * @param parsedQuery
 	 */
 	abstract List<QueryParsingToken> doFindProjection(ParserRuleContext parsedQuery);
 
 	/**
-	 * Discern if the query has a new {@code com.example.Dto()} DTO constructor in the select clause.
+	 * Discern if the query has a {@code new com.example.Dto()} DTO constructor in the select clause.
 	 * 
 	 * @param parsedQuery
 	 * @return Guaranteed to be {@literal true} or {@literal false}.
 	 */
 	abstract boolean hasConstructor(ParserRuleContext parsedQuery);
 
+	private static final Pattern PUNCTUATION_PATTERN = Pattern.compile(".*((?![._])[\\p{Punct}|\\s])");
+
+	private static final String UNSAFE_PROPERTY_REFERENCE = "Sort expression '%s' must only contain property references or "
+			+ "aliases used in the select clause; If you really want to use something other than that for sorting, please use "
+			+ "JpaSort.unsafe(…)";
+
+	/**
+	 * Check any given {@link JpaSort.JpaOrder#isUnsafe()} order for presence of at least one property offending the
+	 * {@link #PUNCTUATION_PATTERN} and throw an {@link Exception} indicating potential unsafe order by expression.
+	 *
+	 * @param order
+	 */
+	static void checkSortExpression(Sort.Order order) {
+
+		if (order instanceof JpaSort.JpaOrder && ((JpaSort.JpaOrder) order).isUnsafe()) {
+			return;
+		}
+
+		if (PUNCTUATION_PATTERN.matcher(order.getProperty()).find()) {
+			throw new InvalidDataAccessApiUsageException(String.format(UNSAFE_PROPERTY_REFERENCE, order));
+		}
+	}
+
 }

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

@@ -27,7 +27,7 @@
 /**
  * Implementation of {@link QueryEnhancer} using a {@link QueryParser}.<br/>
  * <br/>
- * NOTE: The parser can find everything it needs for create sorted and count queries. Thus, looking up the alias or the
+ * NOTE: The parser can find everything it needs to created sorted and count queries. Thus, looking up the alias or the
  * projection isn't needed for its primary function, and are simply implemented for test purposes.
  *
  * @author Greg Turnquist
@@ -122,6 +122,11 @@ public String createCountQueryFor() {
 		return createCountQueryFor(null);
 	}
 
+	/**
+	 * Create a count query from the original query, with potential custom projection.
+	 *
+	 * @param countProjection may be {@literal null}.
+	 */
 	@Override
 	public String createCountQueryFor(@Nullable String countProjection) {
 
@@ -193,6 +198,9 @@ public Set<String> getJoinAliases() {
 		return Set.of();
 	}
 
+	/**
+	 * Look up the {@link DeclaredQuery} from the {@link QueryParser}.
+	 */
 	@Override
 	public DeclaredQuery getQuery() {
 		return queryParser.getDeclaredQuery();

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

@@ -18,7 +18,7 @@
 import org.antlr.v4.runtime.misc.ParseCancellationException;
 
 /**
- * An exception to throw if a JPQL query is invalid.
+ * An exception thrown if the JPQL query is invalid.
  *
  * @author Greg Turnquist
  * @since 3.1