matrei commented on PR #16312: URL: https://github.com/apache/grails-core/pull/16312#issuecomment-5597913811
## AI Review Findings Reviewed together with the follow-up at https://github.com/jamesfredley/grails-core/pull/5 (one commit, `035f9197`, on top of this PR's head `0d5b3167`). The follow-up addresses every item from the earlier review on this PR and should be merged into `fix/dynamic-finder-sort-validation` before this PR merges. This PR should not merge at its current head: the alias regression is real and reproduced by `WhereQueryWithAssociationSortSpec` in both Hibernate modules. Verified locally on the follow-up head: | Check | Result | |---|---| | `:grails-datastore-core:test --tests NameUtilsSpec` | 36 pass | | `:grails-datamapping-core:test` (full) | 928 pass | | `:grails-data-hibernate7-core:test` (full) | 3123 pass, including the 32 new `SortArgumentValidationSpec` cases | | `:grails-data-hibernate5-core:test` (full) | 812 pass, `WhereQueryWithAssociationSortSpec` green again | | `codeStyle` on `grails-datastore-core`, `grails-datamapping-core`, `grails-data-hibernate7-core` | pass | | `git merge-tree` against current `origin/8.0.x` | clean, no conflicts | Everything below is against the follow-up head. None of it is blocking. ### Suggestion: the alias pass-through is wider than it needs to be References: - `grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java:785-806` (`validateSortProperty`) A first segment that is not a persistent property is now accepted on the shape check alone so that criteria and where-query aliases keep working. An alias is only ever used as `alias.property`, so a *single-segment* root that does not resolve could still be rejected without touching the alias case. What the pass-through costs today, probed against a real Hibernate 7 mapping: ``` ZzProbe.findAllByNameLike('%', [sort: 'notAProperty']) ZzProbe.where { name != null }.list(sort: 'notAProperty') java.lang.NullPointerException: Cannot invoke "jakarta.persistence.criteria.Expression.getJavaType()" because "expression" is null ``` That NPE is pre-existing rather than introduced here, but the validation now has everything it needs to turn it into the generic `Invalid sort property` for the common `sort: params.sort` case. Multi-segment unknown roots such as `zz.name` must keep passing through for aliases and fail the same way. Either a one-line tightening in `validateSortProperty` (`segments.length == 1 && property == null` throws) plus a test row, or an explicit decision to leave it as documented. ### Nit: direction trimming differs between entry points References: - `grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java:834-837` (`buildOrder`) - `grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilder.java` (`normalizeDirection`) - `grails-doc/src/en/ref/Domain Classes/list.adoc` `list(sort: 'name', order: ' DESC ')` now sorts descending because the HQL builder trims, but the same arguments on a dynamic finder sort ascending because `buildOrder` compares the raw value: ``` ZzProbe.findAllByNameLike('%', [sort: 'name', order: ' DESC '])*.name == [a, b] ``` The new sentence in `list.adoc` says surrounding whitespace is ignored, which is only true for `list()`. Trimming in `buildOrder` would make the doc accurate for every entry point. ### Nit: `fetch` wording in `list.adoc` is Hibernate 7 specific References: - `grails-doc/src/en/ref/Domain Classes/list.adoc` The doc promises an `IllegalArgumentException` for a `fetch` key that does not name a persistent property. That holds for Hibernate 7, where `list()` builds HQL through `HqlListQueryBuilder`. Hibernate 5 builds `list()` through JPA Criteria and throws Hibernate's own exception for an unknown attribute. Fine for the 8.0.x guide, noting the scope only. ### Process - This PR is 116 commits behind `8.0.x` and CI does not run on it (the workflow pins predate the allowlist update). After merging the follow-up, rebase onto `8.0.x` so the checks actually exercise the change. The merge is clean, so the rebase should be mechanical. - The PR description still describes the original behaviour ("keys that do not resolve to a persistent property" are rejected). It should describe the alias pass-through and the sort-map direction change, since both are user-visible and both are now documented in `grails-doc`. ### Confirmed in the follow-up - `validateSortProperty` is `private static`, uses one generic message, and resolves identity and composite-identity members explicitly rather than relying on `propertiesByName`. - `NameUtils.isValidPropertyPath` is shared by both modules, accepts `$` and non-ASCII identifiers, and rejects identifier-ignorable code points (NUL, U+200B) that `Character.isJavaIdentifierPart` would otherwise let through. - `fetch` keys in `HqlListQueryBuilder` are validated through the same `requireMappedProperty` path before concatenation; blank sort-map keys are rejected instead of emitting `order by , e.name`. - The sort-map direction change is kept, uses the same `asc` fallback in both `populateArgumentsForCriteria` overloads, and is pinned by tests in `DynamicFinderCoverageSpec` and `SortArgumentValidationSpec`. - `SortArgumentValidationSpec` runs against real mappings and covers `id`, `version`, inherited, embedded, association and `club.id` paths, composite identities, the mapping default sort, `createAlias` and where-query aliases, and valid join fetches. - Docs are in place in `grails-doc` `list.adoc` and both Hibernate `finders.adoc` files. -- 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]
