jamesfredley commented on code in PR #16333:
URL: https://github.com/apache/grails-core/pull/16333#discussion_r3970762440
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilder.java:
##########
@@ -55,6 +59,7 @@ public String buildListHql() {
Map<String, Object> fetchMap = (Map<String, Object>) fetchObj;
fetchMap.forEach((prop, type) -> {
if (HibernateQueryArgument.JOIN.value().equals(type) ||
HibernateQueryArgument.EAGER.value().equals(type)) {
+ requireMappedProperty(prop,
HibernateQueryArgument.FETCH.value());
hql.append(" join fetch e.").append(prop);
Review Comment:
Thanks. Non-String fetch keys, including Groovy GString keys, are an edge
case; fetch maps are normally shaped like [authors: eager]. This is not the
sort-injection path hardened by this PR, so it is not a blocker here.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java:
##########
@@ -767,18 +756,89 @@ private static void resetMethodExpressionPattern() {
methodExpressinPattern = Pattern.compile("\\p{Upper}[\\p{Lower}\\d]+("
+ expressionPattern + ")");
}
- private static void addSimpleSort(Query q, String sort, String order,
boolean ignoreCase) {
- Query.Order o;
- if (ORDER_DESC.equalsIgnoreCase(order)) {
- o = Query.Order.desc(sort);
+ private static PersistentEntity resolvePersistentEntity(BuildableCriteria
query) {
+ if (query instanceof AbstractCriteriaBuilder) {
+ return ((AbstractCriteriaBuilder) query).getPersistentEntity();
}
- else {
- o = Query.Order.asc(sort);
+ if (query instanceof AbstractDetachedCriteria) {
+ return ((AbstractDetachedCriteria) query).getPersistentEntity();
+ }
+ return null;
+ }
+
+ /**
+ * Rejects a sort key that is not shaped like a property path. When the
entity is known and the
+ * first segment names one of its persistent properties, every further
segment must also resolve
+ * through the mapping: associations and embedded components are
traversed, and identity
+ * properties, including the members of a composite identity, are
recognised. A first segment
+ * that is not a persistent property is accepted on the shape check alone,
because criteria and
+ * where-query aliases such as {@code c1.name} are not persistent
properties; the underlying
+ * query implementation resolves them, or reports an unknown name, itself.
+ * <p>
+ * The exception message deliberately omits the caller-supplied value:
sort keys are commonly
+ * taken straight from request parameters.
+ *
+ * @param entity the entity being queried, or {@code null} when it cannot
be resolved
+ * @param sort the requested sort property
+ * @throws IllegalArgumentException if the sort key is malformed or does
not resolve
+ */
+ private static void validateSortProperty(PersistentEntity entity, String
sort) {
+ if (!NameUtils.isValidPropertyPath(sort)) {
+ throw new IllegalArgumentException(INVALID_SORT_PROPERTY);
+ }
+ if (entity == null) {
+ return;
+ }
+ String[] segments = sort.split("\\.");
+ PersistentProperty property = resolveProperty(entity, segments[0]);
+ if (property == null) {
+ return;
+ }
+ for (int i = 1; i < segments.length; i++) {
+ PersistentEntity associated = property instanceof Association ?
((Association) property).getAssociatedEntity() : null;
+ if (associated == null) {
+ throw new IllegalArgumentException(INVALID_SORT_PROPERTY);
+ }
+ property = resolveProperty(associated, segments[i]);
+ if (property == null) {
+ throw new IllegalArgumentException(INVALID_SORT_PROPERTY);
+ }
}
+ }
- if (ignoreCase) o = o.ignoreCase();
+ /**
+ * Resolves one path segment against an entity, including its identity
property and the members
+ * of a composite identity, which are not guaranteed to be reachable
through
+ * {@link PersistentEntity#getPropertyByName(String)}.
+ */
+ private static PersistentProperty resolveProperty(PersistentEntity entity,
String name) {
+ PersistentProperty property = entity.getPropertyByName(name);
+ if (property != null) {
+ return property;
+ }
+ PersistentProperty identity = entity.getIdentity();
+ if (identity != null && name.equals(identity.getName())) {
+ return identity;
+ }
+ PersistentProperty[] compositeIdentity = entity.getCompositeIdentity();
+ if (compositeIdentity != null) {
+ for (PersistentProperty candidate : compositeIdentity) {
+ if (candidate != null && name.equals(candidate.getName())) {
+ return candidate;
+ }
+ }
+ }
+ return null;
+ }
- q.order(o);
+ 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);
Review Comment:
Agreed. We are aligning DynamicFinder.buildOrder with Hibernate
normalizeDirection so it trims surrounding whitespace and rejects junk values.
Leaving this open until the fix lands.
##########
grails-doc/src/en/ref/Domain Classes/list.adoc:
##########
@@ -60,10 +60,12 @@ Parameters:
* `max` - The maximum number to list
* `offset` - The offset from the first result to list from
* `order` - How to order the list, either `"desc"` or `"asc"`
-* `sort` - The property name to sort by
+* `sort` - The property name to sort by, which may be a path through an
association or embedded component such as `"author.name"`, or a `Map` of
property names to directions such as `[title: "asc", "author.name": "desc"]`
* `ignoreCase` - Whether to ignore the case when sorting. Default is `true`.
* `fetch` - The fetch policy for the object's associations as a `Map`
* `readOnly` - true if returned objects should not be automatically
dirty-checked (simlar to `read()`)
* `fetchSize` - number of rows fetched by the underlying JDBC driver per round
trip
* `flushMode` - Hibernate `FlushMode` override, defaults to `FlushMode.AUTO`
* `timeout` - query timeout in seconds
+
+The `sort`, `order` and `fetch` arguments are validated before the query is
built. A `sort` key must be a property path made up of identifiers separated by
dots that resolves through the domain class mapping, `order` must be `"asc"` or
`"desc"` (case-insensitive, surrounding whitespace ignored), and a `fetch` key
must name a persistent property. Any other value, for example one carrying a
second expression or a function call, is rejected with an
`IllegalArgumentException`, so request parameters can be passed to `list()`
without further checks.
Review Comment:
Agreed. We are aligning DynamicFinder.buildOrder with Hibernate and will
keep the documentation accurate. Leaving this open until the implementation and
documentation are confirmed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
