Copilot commented on code in PR #16333:
URL: https://github.com/apache/grails-core/pull/16333#discussion_r3969795424
##########
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:
Casting to `Map<String, Object>` and using `forEach((String prop, ...))` can
produce a `ClassCastException` for non-`String` keys (e.g., Groovy `GString`
keys in argument maps), which bypasses the intentional
`IllegalArgumentException` validation path. Consider iterating as `Map<?, ?>`,
coercing keys via `toString()` (or rejecting non-`CharSequence` keys
explicitly), and then validating/using the resulting string so the failure mode
is consistently `IllegalArgumentException("Invalid fetch property")`.
##########
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:
This states that `order` validation rejects “any other value” with
`IllegalArgumentException` and that surrounding whitespace is ignored. In the
reviewed code, Hibernate list building enforces this, but
non-Hibernate/criteria paths may still treat unknown/whitespace-padded
directions as default ascending rather than rejecting. Consider either
narrowing this paragraph to the Hibernate-backed behavior or updating the
non-Hibernate implementation(s) so this statement is accurate across supported
datastores/entry points.
##########
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:
`buildOrder` doesn’t trim/normalize `direction`, so common inputs like `"
DESC "` will silently sort ascending. Since this PR’s docs/tests emphasize
normalization/validation of sort direction, consider trimming whitespace and
either (a) treating only `asc`/`desc` (case-insensitive) as valid and throwing
`IllegalArgumentException` otherwise, or (b) at minimum trimming before the
`equalsIgnoreCase` check so surrounding whitespace is ignored consistently.
--
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]
