You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@grails.apache.org by ja...@apache.org on 2026/09/09 16:47:44 UTC
(grails-core) branch fix/dynamic-finder-sort-validation updated: fix: trim and validate DynamicFinder order direction
This is an automated email from the ASF dual-hosted git repository.
jamesfredley pushed a commit to branch fix/dynamic-finder-sort-validation
in repository https://gitbox.apache.org/repos/asf/grails-core.git
The following commit(s) were added to refs/heads/fix/dynamic-finder-sort-validation by this push:
new 91885f07ed fix: trim and validate DynamicFinder order direction
91885f07ed is described below
commit 91885f07ed3da43398901da7a2726e6b5926bb33
Author: James Fredley <ja...@users.noreply.github.com>
AuthorDate: Wed Sep 9 12:47:31 2026 -0400
fix: trim and validate DynamicFinder order direction
DynamicFinder.buildOrder treated any order other than exact
equalsIgnoreCase("desc") as ascending, so values like " DESC "
silently sorted ascending. Trim whitespace, accept asc/desc
case-insensitively, and reject anything else with
IllegalArgumentException("Invalid sort direction"), matching
HqlListQueryBuilder.normalizeDirection.
Assisted-by: opencode:xai/grok-4.6
---
.../gorm/tests/SortArgumentValidationSpec.groovy | 25 +++++++++++++
.../datastore/gorm/finders/DynamicFinder.java | 26 +++++++++++--
.../gorm/finders/DynamicFinderCoverageSpec.groovy | 43 ++++++++++++++++++++++
3 files changed, 91 insertions(+), 3 deletions(-)
diff --git a/grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/SortArgumentValidationSpec.groovy b/grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/SortArgumentValidationSpec.groovy
index ca570cfe0f..2df5dc6d4d 100644
--- a/grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/SortArgumentValidationSpec.groovy
+++ b/grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/SortArgumentValidationSpec.groovy
@@ -124,6 +124,9 @@ class SortArgumentValidationSpec extends HibernateGormDatastoreSpec {
SavClub.list(sort: 'name', order: ' DESC ')*.name == ['United', 'Arsenal']
SavClub.list(sort: 'name', order: 'Asc')*.name == ['Arsenal', 'United']
SavClub.list(sort: [name: ' desc'])*.name == ['United', 'Arsenal']
+ SavClub.findAllByNameLike('%', [sort: 'name', order: ' DESC '])*.name == ['United', 'Arsenal']
+ SavClub.findAllByNameLike('%', [sort: 'name', order: ' asc '])*.name == ['Arsenal', 'United']
+ SavClub.where { name != null }.list(sort: 'name', order: ' DESC ')*.name == ['United', 'Arsenal']
}
void "each sort map entry takes its direction from its value rather than the order argument"() {
@@ -170,6 +173,28 @@ class SortArgumentValidationSpec extends HibernateGormDatastoreSpec {
e.message == 'Invalid sort direction'
}
+ @Unroll
+ void "dynamic finders and where queries reject order direction #description without echoing it"() {
+ when:
+ SavClub.findAllByNameLike('%', [sort: 'name', order: order])
+
+ then:
+ def finder = thrown(IllegalArgumentException)
+ finder.message == 'Invalid sort direction'
+
+ when:
+ SavClub.where { name != null }.list(sort: 'name', order: order)
+
+ then:
+ def where = thrown(IllegalArgumentException)
+ where.message == 'Invalid sort direction'
+
+ where:
+ order | description
+ 'sideways' | 'that is not asc or desc'
+ 'desc extra' | 'carrying extra tokens'
+ }
+
@Unroll
void "list rejects the fetch key #description without echoing it"() {
when:
diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java
index aa330dc3fa..d9804ed4bc 100644
--- a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java
+++ b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java
@@ -107,6 +107,7 @@ public abstract class DynamicFinder extends AbstractFinder implements QueryBuild
private static final String NOT = "Not";
private static final String INVALID_SORT_PROPERTY = "Invalid sort property";
+ private static final String INVALID_SORT_DIRECTION = "Invalid sort direction";
private static final Map<String, Constructor> methodExpressions = new LinkedHashMap<String, Constructor>();
protected final MappingContext mappingContext;
@@ -570,8 +571,7 @@ public abstract class DynamicFinder extends AbstractFinder implements QueryBuild
if (sortObject != null) {
if (sortObject instanceof CharSequence) {
final String sort = sortObject.toString();
- final String order = ORDER_DESC.equalsIgnoreCase(orderParam) ? ORDER_DESC : ORDER_ASC;
- addSimpleSort(query, sort, order, ignoreCase);
+ addSimpleSort(query, sort, orderParam, ignoreCase);
}
else if (sortObject instanceof Map) {
Map sortMap = (Map) sortObject;
@@ -831,8 +831,28 @@ public abstract class DynamicFinder extends AbstractFinder implements QueryBuild
return null;
}
+ /**
+ * Trims and accepts only {@code asc} or {@code desc}, case-insensitively. Blank and
+ * {@code null} default to {@code asc}. The exception message omits the caller-supplied
+ * value, which usually originates from request parameters.
+ *
+ * @param direction the caller-supplied order argument
+ * @return {@link #ORDER_ASC} or {@link #ORDER_DESC}
+ * @throws IllegalArgumentException if the value is neither asc nor desc
+ */
+ private static String normalizeDirection(String direction) {
+ String normalized = direction == null ? "" : direction.trim();
+ if (normalized.isEmpty() || ORDER_ASC.equalsIgnoreCase(normalized)) {
+ return ORDER_ASC;
+ }
+ if (ORDER_DESC.equalsIgnoreCase(normalized)) {
+ return ORDER_DESC;
+ }
+ throw new IllegalArgumentException(INVALID_SORT_DIRECTION);
+ }
+
private static Query.Order buildOrder(String sort, String direction, boolean ignoreCase) {
- Query.Order order = ORDER_DESC.equalsIgnoreCase(direction) ? Query.Order.desc(sort) : Query.Order.asc(sort);
+ Query.Order order = ORDER_DESC.equals(normalizeDirection(direction)) ? Query.Order.desc(sort) : Query.Order.asc(sort);
return ignoreCase ? order.ignoreCase() : order;
}
diff --git a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/finders/DynamicFinderCoverageSpec.groovy b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/finders/DynamicFinderCoverageSpec.groovy
index 68d556a557..bd3000eb0b 100644
--- a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/finders/DynamicFinderCoverageSpec.groovy
+++ b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/finders/DynamicFinderCoverageSpec.groovy
@@ -109,6 +109,30 @@ class DynamicFinderCoverageSpec extends Specification {
DynamicFinderThing.list(sort: 'age', order: 'desc')*.name == ['Charlie', 'Alice', 'Bob']
}
+ void "list(Map) trims surrounding whitespace on the order direction"() {
+ expect:
+ DynamicFinderThing.list(sort: 'age', order: ' DESC ')*.name == ['Charlie', 'Alice', 'Bob']
+ DynamicFinderThing.list(sort: 'age', order: ' asc ')*.name == ['Bob', 'Alice', 'Charlie']
+ DynamicFinderThing.list(sort: [age: ' DESC '])*.age == [35, 30, 25]
+ DynamicFinderThing.findAllByTitle('Engineer', [sort: 'age', order: ' DESC '])*.name == ['Charlie', 'Alice']
+ }
+
+ @Unroll
+ void "list(Map) rejects order direction #description without echoing it"() {
+ when:
+ DynamicFinderThing.list(sort: 'age', order: order)
+
+ then:
+ def e = thrown(IllegalArgumentException)
+ e.message == 'Invalid sort direction'
+
+ where:
+ order | description
+ 'sideways' | 'that is not asc or desc'
+ 'desc extra' | 'carrying extra tokens'
+ 'descending' | 'that is a longer word'
+ }
+
void "list(Map) sorts by a Map of property to direction"() {
expect:
DynamicFinderThing.list(sort: [age: 'asc'])*.name == ['Bob', 'Alice', 'Charlie']
@@ -180,6 +204,25 @@ class DynamicFinderCoverageSpec extends Specification {
}
}
+ void "populateArgumentsForCriteria(BuildableCriteria, Map) trims and validates the order direction"() {
+ given:
+ def api = new org.grails.datastore.gorm.GormStaticApi(DynamicFinderThing, datastore, [])
+ def criteria = api.createCriteria()
+
+ when:
+ DynamicFinder.populateArgumentsForCriteria(criteria, [sort: 'age', order: ' DESC '])
+
+ then:
+ criteria.list(null)*.age == [35, 30, 25]
+
+ when:
+ DynamicFinder.populateArgumentsForCriteria(api.createCriteria(), [sort: 'age', order: 'sideways'])
+
+ then:
+ def e = thrown(IllegalArgumentException)
+ e.message == 'Invalid sort direction'
+ }
+
void "populateArgumentsForCriteria(BuildableCriteria, Map) applies sort and order to a real criteria query"() {
given: "not called from any production code path in this repo (every real caller uses the Query overload), but still a public static API worth covering directly. fetch/cache require an already-active query the bare CriteriaBuilder here doesn't have."
def api = new org.grails.datastore.gorm.GormStaticApi(DynamicFinderThing, datastore, [])