Review follow-up: keep alias sorting working, validate fetch keys, share the path check

[jamesfredley]

Relocated from jamesfredley/grails-core#5. That follow-up was opened on an archive fork by mistake; Grails PRs belong on apache/grails-core. Parent: #16312.

Follow-up to the review on #16312, targeting fix/dynamic-finder-sort-validation.

Blocking items

• Alias sorting regression. validateSortProperty rejected any first path segment that is not a persistent property of the root entity, which is what a criteria / where-query alias is. WhereQueryWithAssociationSortSpec failed in grails-data-hibernate7-core and grails-data-hibernate5-core, and createCriteria().list(sort: 'c.name') { createAlias('club', 'c') } failed the same way. A first segment that is not a persistent property is now accepted on the shape check alone and left to the query implementation; segments beneath a known property must still resolve through the mapping.
• fetch keys in HqlListQueryBuilder. They are now validated the same way as sort keys before being concatenated into HQL.

Smaller points

• validateSortProperty is private static; one generic message that does not echo the caller-supplied value; composite identities checked explicitly via getCompositeIdentity().
• The duplicated regex is replaced by NameUtils.isValidPropertyPath, shared by both modules and based on Character.isJavaIdentifierStart/Part, so $ and non-ASCII identifiers are accepted; ignorable control characters are not. The constant is gone from the ARGUMENT_* block.
• resolvePersistentEntity is reused in the sortObject == null && orderParam != null branch.
• Blank sort properties are rejected (no more order by , e.name), and normalizeDirection trims, so order: ' desc' keeps working.
• Sort-map direction: each entry keeps taking its direction from its value (matching applySortForMap), with the same asc fallback for a null value in both overloads. This is tested and documented rather than reverted.

Tests

• SortArgumentValidationSpec (hibernate7-core) runs against real mappings through list(), dynamic finders, where queries and criteria: sort: 'id' / 'version', a subclass sorted by an inherited property, an embedded path, association paths including club.id, composite identity members, the mapping { sort } default, createAlias and where-query aliases, direction normalization, map directions, valid join fetches, and the rejected sort / order / fetch values.
• DynamicFinderCoverageSpec covers nested association traversal, identity and composite identity, a non-association mid-path, a basic collection with no associated entity, alias-shaped roots, the map-direction change and both populateArgumentsForCriteria overloads.
• HqlListQueryBuilderSpec covers the blank-key and trimmed-direction edges, $ identifiers, and injected / unknown fetch keys.
• NameUtilsSpec covers the shared path check.

Docs

grails-doc list.adoc and the Hibernate 7 / Hibernate 5 finders.adoc describe the accepted sort, order and fetch values and the alias pass-through.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Follow-up to fix dynamic-finder/list sorting validation by restoring alias-based sorting, hardening input validation for sort/order/fetch, and sharing a common “identifier-shaped property path” check.

Changes:

• Add NameUtils.isValidPropertyPath and adopt it for sort/fetch validation to prevent query-string injection via malformed keys.
• Adjust DynamicFinder.validateSortProperty logic to allow alias-shaped root segments (e.g., c1.name) while still validating mapped paths beneath known persistent properties.
• Validate fetch keys in HqlListQueryBuilder before concatenating into HQL; update docs and expand test coverage across modules.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
grails-doc/src/en/ref/Domain Classes/list.adoc	Documents validated sort/order/fetch semantics and alias pass-through behavior.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/NameUtils.java	Adds shared isValidPropertyPath implementation used by multiple modules.
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/reflect/NameUtilsSpec.groovy	Adds unit tests for valid/invalid property-path shapes (incl. Unicode and $).
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java	Updates sort validation + ordering logic; extracts helpers; restores alias sorting.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/finders/DynamicFinderCoverageSpec.groovy	Expands coverage around list/dynamic finder/criteria argument validation.
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilder.java	Validates fetch keys and reworks sort validation to use shared path check.
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilderSpec.groovy	Adds tests for trimmed direction, blank keys, $ identifiers, and fetch-key rejection.
grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/SortArgumentValidationSpec.groovy	New integration-style spec exercising validation against real Hibernate mappings.
grails-data-hibernate7/docs/src/docs/asciidoc/querying/finders.adoc	Documents accepted sort shapes and alias pass-through.
grails-data-hibernate5/docs/src/docs/asciidoc/querying/finders.adoc	Same as above for Hibernate 5 docs.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.