Copilot commented on code in PR #16329:
URL: https://github.com/apache/grails-core/pull/16329#discussion_r3969367959
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilder.java:
##########
@@ -58,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:
`requireMappedProperty` currently accepts dotted paths (e.g.,
`books.author`) because `NameUtils.isValidPropertyPath` allows them. However,
`join fetch e.` + prop will generate invalid HQL for nested paths (`join fetch
e.books.author` is not valid; nested fetches require separate joins/aliases).
If `fetch` is intended to only accept a single persistent property name (as the
docs say), enforce that by rejecting keys containing `.` (or by validating
`prop` is a direct association on `entity`), so the generated HQL is always
syntactically valid.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java:
##########
@@ -784,55 +767,78 @@ private static PersistentEntity
resolvePersistentEntity(BuildableCriteria query)
}
/**
- * Rejects sort keys that are not identifier-shaped property paths, and
when a
- * mapping is available, keys that do not resolve to a persistent property.
+ * 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
*/
- public static void validateSortProperty(PersistentEntity entity, String
sort) {
- if (sort == null || !SORT_PROPERTY_PATTERN.matcher(sort).matches()) {
- throw new IllegalArgumentException("Invalid sort property: " +
sort);
+ private static void validateSortProperty(PersistentEntity entity, String
sort) {
Review Comment:
Changing `validateSortProperty` from `public` to `private` is a
source/binary incompatible API change for any external callers (including other
Grails modules or plugins) that may have been using it. If this method is not
intended as public API, consider keeping it `public` but deprecating it (or
making it package-private) and introducing a new internal helper, to avoid
breaking downstream compilation unexpectedly.
--
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]
