jdaugherty commented on PR #16624: URL: https://github.com/apache/grails-core/pull/16624#issuecomment-6485914464
@jamesfredley Thanks. All three are fixed in 41c275b549. It is a separate commit on top of 051efbe12c, so [its diff](https://github.com/apache/grails-core/pull/16624/commits/41c275b549) is exactly this round. ### 1. `@ConvertGroup` on a cascaded association There is still one cascade, GORM's, and the groups now travel down it: - `PersistentEntityValidator` carries validation hints (Spring's term; for Bean Validation, the groups) from each object to its associations. - The validator of the entity that owns an association gives the hints for the associated object (`getAssociationValidationHints`). - The validator of the associated entity receives them in `getValidatedConstrainedProperties` and `validateBeyondConstraints`. - The root object has no hints, which means the default group. - For an association marked `@Valid`, `JakartaPersistentEntityValidator` returns the owner's groups converted by the association's `@ConvertGroup`, read from its `PropertyDescriptor` or from a cascaded container element. That is how Bean Validation cascades. Any other association gets the default group, the same as when the object is validated on its own. - The `nullable` relaxation follows the same groups. Under the converted group, `nullable` gives way to the annotations of that group that declare nullability. A default-group `@NotNull` that the converted group doesn't run leaves the block in force. A null is still reported once, and the block still rejects a null that the annotations no longer check. Your example now reports `child.label` (`NotBlank`). When the child is reached through a conversion, its default-group annotations aren't checked, which is what Bean Validation's own cascade does. One correction on "the provider used to follow @Valid": on 8.0.x it never cascaded. `MappingContextTraversableResolver.isCascadable` walks the path to the traversable object, and the root node of that path has no name, so it returned `false`. A probe on `e4b22127dc` accepts your example. Even a plain Hibernate Validator run with that resolver reports nothing through `@Valid`, whether into an association or into a non-entity object. @matrei saw the same in the first review. So honoring the conversion is new behavior rather than a restoration, but it is the right behavior. Regressions: - **TCK `BeanValidationSpec`:** `BeanValidatedPost` has `@Valid @ConvertGroup(from = Default, to = PublishChecks) BeanValidatedTeaser teaser`. It reports `teaser.label` (`NotBlank`) and `teaser.note` (`nullable`, from the block, because the teaser's default-group `@NotNull` isn't run). - **`JakartaValidatorRegistrySpec`:** - a conversion on a `hasMany`; - propagation along a nested `@Valid`; - the default group along a plain association; - the same book validated on its own. - **`PersistentEntityValidatorSpec`:** - the hints reach each level and propagate two levels down; - a validation started mid-cascade starts without hints and leaves the outer one intact. ### 2. A superclass `@GroupSequence` I didn't take "the sequence of the class that declares the constraint" literally, because Hibernate Validator doesn't work that way (`ValidatorImpl.validateConstraintsForDefaultGroup`). Walking up from the validated class, the first class that redefines the default group applies its sequence to its own constraints and to those of its superclasses, and stops there. Constraints of the classes below it are validated in `Default` only. So: - **A subclass without a sequence:** the superclass's sequence covers the superclass's annotations, but not the subclass's own `@Null(groups = PublishChecks)`. - **A subclass with its own sequence:** that sequence covers every class above it, and the superclass's sequence is ignored. `JakartaConstraintsTranslator` now reads each class's own constraints (`Scope.LOCAL_ELEMENT` on that class's descriptor, in the provider's hierarchy order) and applies this rule. Regressions: - **`JakartaConstraintsTranslatorSpec`:** one feature for each case. Each first asserts what Hibernate Validator itself reports for the same instances. - **TCK `BeanValidationSpec`:** `BeanValidatedRecall extends BeanValidatedNotice`, which has `@GroupSequence([DraftChecks, BeanValidatedNotice])`. The superclass's `headline` is reported once (`NotNull`), and the subclass's `reason` by the block. ### 3. Decimal bounds on integer properties An inclusive `@DecimalMin` is now rounded up and an inclusive `@DecimalMax` rounded down before the conversion, as the exclusive bounds already were. `@DecimalMin('1.5') @DecimalMax('4.5') Integer quantity` is described as `min: 2, max: 4`. The translator spec covers inclusive, exclusive, negative and whole bounds, and checks each one against what Hibernate Validator accepts at and beyond the bound. ### Docs - **Guide:** covers the superclass sequence, `@ConvertGroup` on an association marked `@Valid`, and `@DecimalMin`/`@DecimalMax`. - **Upgrade note:** the cascade bullet mentions the conversion. ### Run locally - **Module tests:** - `:grails-datamapping-validation:test`: 62 tests. - `:grails-datamapping-core:test`: 1827 tests. - `:grails-testing-support-datamapping:test`: 9 tests. - `:grails-openapi:test`: 297 tests. - `:grails-fields:test`: 699 tests. - **TCK `BeanValidationSpec`:** all 10 features pass on Hibernate 5, Hibernate 7, MongoDB and Neo4j. The two new features fail with the main sources of 051efbe12c (checked on Hibernate 5). - **Style:** `codeStyle` on `grails-datamapping-core`, `grails-datamapping-validation` and `grails-datamapping-tck` is clean. -- 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]
