jdaugherty commented on code in PR #16624:
URL: https://github.com/apache/grails-core/pull/16624#discussion_r4419861528
##########
grails-datamapping-validation/src/main/groovy/grails/gorm/validation/PersistentEntityValidator.groovy:
##########
@@ -232,63 +427,37 @@ class PersistentEntityValidator implements
CascadingValidator, ConstrainedEntity
MappingContext mappingContext = associatedEntity.getMappingContext()
EntityReflector associatedReflector =
mappingContext.getEntityReflector(associatedEntity)
- Association otherSide = null
- if (association.isBidirectional()) {
- otherSide = association.getInverseSide()
- }
-
- Map associatedConstrainedProperties
+ PersistentEntityValidator associatedValidator = null
+ Map<String, ConstrainedProperty> associatedConstrainedProperties =
Collections.<String, ConstrainedProperty>emptyMap()
def validator = mappingContext.getEntityValidator(associatedEntity)
if (validator instanceof PersistentEntityValidator) {
- associatedConstrainedProperties = ((PersistentEntityValidator)
validator).getConstrainedProperties()
- }
- else {
- associatedConstrainedProperties = Collections.<String,
ConstrainedProperty>emptyMap()
+ associatedValidator = (PersistentEntityValidator) validator
+ associatedConstrainedProperties =
associatedValidator.getValidatedConstrainedProperties(associatedAllHints)
}
- // Invoke any beforeValidate callbacks on the associated object before
validating
- validateHelper.invokeBeforeValidate(associatedObject,
associatedConstrainedProperties.keySet() as List<String>)
-
- List<PersistentProperty> associatedPersistentProperties =
associatedEntity.getPersistentProperties()
String nestedPath = errors.getNestedPath()
try {
errors.setNestedPath(buildNestedPath(nestedPath, propertyName,
indexOrKey))
+ CASCADE.set(new Cascade(associatedHints, associatedAllHints,
association))
- for (PersistentProperty associatedPersistentProperty :
associatedPersistentProperties) {
- if (association.isEmbedded() &&
EMBEDDED_EXCLUDES.contains(associatedPersistentProperty.getName())) {
- continue
- }
-
- String associatedPropertyName =
associatedPersistentProperty.getName()
- if
(associatedConstrainedProperties.containsKey(associatedPropertyName)) {
- ConstrainedProperty associatedConstrainedProperty =
associatedConstrainedProperties.get(associatedPropertyName)
- validatePropertyWithConstraint(associatedObject,
errors.getNestedPath() + associatedPropertyName, associatedReflector, errors,
associatedConstrainedProperty, associatedPersistentProperty)
- }
-
- // Don't continue cascade if the the other side is equal to
avoid stack overflow
- if (associatedPersistentProperty == otherSide) {
- continue
- }
-
- if (associatedPersistentProperty instanceof Association) {
- if (association.isBidirectional() &&
associatedPersistentProperty == association.inverseSide) {
- // If this property is the inverse side of the
currently processed association then
- // we don't want to process it
- continue
- }
+ // the constraints do not depend on the hints of a step, so they
are validated once for all of them
+ if (validatedObjects.add(new
ValidatedConstraints(associatedObject, associatedAllHints))) {
+ // Invoke any beforeValidate callbacks on the associated
object before validating
+ validateHelper.invokeBeforeValidate(associatedObject,
associatedConstrainedProperties.keySet() as List<String>)
+ validateConstraints(associatedObject, associatedEntity,
errors, associatedReflector, associatedConstrainedProperties, association,
false)
+ }
- cascadeToAssociativeProperty(
- associatedObject,
- errors,
- associatedReflector,
- (Association) associatedPersistentProperty,
- validatedObjects)
- }
+ if (associatedValidator != null) {
+
associatedValidator.validateAssociationsAndBeyond(associatedObject, errors,
true, associatedHints, validatedObjects)
Review Comment:
Fixed in 837f6d9348: `cascadeValidationToOne` unwraps the associated object
first, as `validate()` does for the object it starts with, so the visited set,
the cascade decision and the validators see the instance, whose fields the
provider reads. `PersistentEntityValidatorSpec` validates a Javassist proxy
reached through a to-one and through a collection,
`JakartaValidatorRegistrySpec` a proxy of a valid annotated book, with no
`NotBlank`, and then of an invalid one, with the book's own value, and the TCK
a `proxy(id)` as the book of a shelf on Hibernate 5, Hibernate 7, MongoDB and
Neo4j. Details in the review body.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/validation/jakarta/GormValidatorAdapter.groovy:
##########
@@ -60,6 +67,58 @@ class GormValidatorAdapter extends SpringValidatorAdapter
implements CascadingVa
}
}
+ /**
+ * Validates the object in the given groups, or in the default group when
there are none
+ *
+ * @param obj The object to validate
+ * @param errors The errors to report the violations to
+ * @param cascade Whether validation cascades to the persistent
associations of the entity through the
+ * properties marked {@code @Valid}; it cascades through the other
properties marked {@code @Valid} either way
+ * @param groups The groups to validate
+ * @since 8.0.1
+ */
+ void validate(Object obj, Errors errors, boolean cascade, Class<?>[]
groups) {
+ CASCADE_VALIDATION.set(cascade)
+ try {
+ validate(obj, errors, (Object[]) groups)
+ } finally {
+ CASCADE_VALIDATION.remove()
+ }
+ }
+
+ /**
+ * Validates the object in the given groups as {@link #validate(Object,
Errors, boolean, Class[])} does,
+ * reporting only the violations of the constraints declared in one of the
reported groups. The provider
+ * validates a group together with the groups it extends; when validation
cascades from an object validated in
+ * a group whose extended groups a {@code @ConvertGroup} converts, the
associated object is validated in the
+ * group alone, so the violations of the converted groups are left out.
+ *
+ * @param obj The object to validate
+ * @param errors The errors to report the violations to
+ * @param cascade Whether validation cascades to the persistent
associations of the entity
+ * @param groups The groups to validate
+ * @param reportedGroups The groups whose violations to report, null to
report every violation
+ * @since 8.0.1
+ */
+ void validate(Object obj, Errors errors, boolean cascade, Class<?>[]
groups, Set<Class<?>> reportedGroups) {
+ if (reportedGroups == null) {
+ validate(obj, errors, cascade, groups)
+ return
+ }
+ CASCADE_VALIDATION.set(cascade)
+ try {
+ Set<ConstraintViolation<Object>> reported = new LinkedHashSet<>()
+ for (ConstraintViolation<Object> violation :
thisValidator.validate(obj, groups)) {
+ if
(!Collections.disjoint(violation.constraintDescriptor.groups, reportedGroups)) {
+ reported.add(violation)
Review Comment:
Fixed in 837f6d9348: the adapter filters each violation with the groups in
effect for the bean holding the constraint, the reported groups converted along
the path of the violation as the `@ConvertGroup` annotations of each cascaded
property declare, a converted group expanded, an unconverted one alone, and the
default group of each bean expanded to the sequence redefining it. Your case is
in `JakartaValidatorRegistrySpec`: the transient details converting `Extended`
to `DetailsChecks` report `child.details.summary` and not their `Base`
violation, as the provider does; `GormValidatorAdapterSpec` covers the filter
on its own. Details in the review body.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/validation/jakarta/JakartaPersistentEntityValidator.groovy:
##########
@@ -0,0 +1,339 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.grails.datastore.gorm.validation.jakarta
+
+import java.util.concurrent.ConcurrentHashMap
+import java.util.function.Function
+
+import groovy.transform.CompileStatic
+
+import jakarta.validation.Validator
+import jakarta.validation.groups.Default
+import jakarta.validation.metadata.CascadableDescriptor
+import jakarta.validation.metadata.GroupConversionDescriptor
+
+import org.springframework.context.MessageSource
+import org.springframework.validation.Errors
+
+import grails.gorm.validation.ConstrainedProperty
+import grails.gorm.validation.PersistentEntityValidator
+import
org.grails.datastore.gorm.validation.constraints.eval.ConstraintsEvaluator
+import
org.grails.datastore.gorm.validation.constraints.registry.ConstraintRegistry
+import org.grails.datastore.mapping.model.PersistentEntity
+import org.grails.datastore.mapping.model.types.Association
+
+/**
+ * Validates a persistent entity constrained with Jakarta Bean Validation
annotations: its GORM constraints (the
+ * {@code constraints} block and the defaults) as a {@link
PersistentEntityValidator}, then its annotations with the
+ * Bean Validation provider, whose violations are reported as field errors
with Spring's Bean Validation message
+ * codes (such as {@code NotNull.member.name}, {@code NotNull.name} and {@code
NotNull}).
+ * <p>
+ * The GORM {@code nullable} constraint is left to Bean Validation for a
property whose nullability an annotation
+ * of the validated groups declares ({@code @NotNull}, {@code @NotBlank},
{@code @NotEmpty} or {@code @Null}), so a
+ * null value is reported once. The GORM validator cascades to the persistent
associations as it does for any
+ * entity, and validates an annotated associated entity both ways through
{@link #validateBeyondConstraints};
+ * Bean Validation cascades only through the other properties marked {@code
@Valid}, such as a transient one. An
+ * object is validated in the default group, or in the groups given as
validation hints to
+ * {@link #validate(Object, Errors, Object...)}, and an object associated
through a property marked {@code @Valid}
+ * in the groups of the object owning it, converted as the {@code
@ConvertGroup} annotations of the property
+ * declare, as Bean Validation cascades, see {@link
#getAssociationValidationHints}. Any other associated object is
+ * validated in the default group, as it is on its own. An associated object
reached through two associations is
+ * validated once in each distinct list of groups. A group sequence is
validated group by group, each group over
+ * the object and the objects it cascades to, and stops at the first group
with a violation among them, see
+ * {@link #validateAssociationsAndBeyond}.
+ * <p>
+ * {@link #getConstrainedProperties()} describes the annotations as GORM
constraints next to the ones of the block,
+ * see {@link JakartaConstraintsTranslator}.
+ *
+ * @since 8.0.1
+ */
+@CompileStatic
+class JakartaPersistentEntityValidator extends PersistentEntityValidator {
+
+ private static final List<Class<?>> DEFAULT_GROUP_LIST =
Collections.<Class<?>> singletonList(Default)
+
+ /**
+ * The validator for the Bean Validation annotations of the entity
+ */
+ final GormValidatorAdapter beanValidator
+
+ private final JakartaConstraintsTranslator translator
+ private final ConstraintsEvaluator constraintsEvaluator
+ private final Map<String, ConstrainedProperty>
describedConstrainedProperties
+ private final Map<List<Object>, Map<String, ConstrainedProperty>>
validatedConstrainedPropertiesByGroups = new ConcurrentHashMap<>()
+
+ /**
+ * @param entity The entity
+ * @param messageSource The message source
+ * @param constraintsEvaluator The evaluator of the GORM constraints
+ * @param constraintRegistry The registry the constraints describing the
annotations are built with
+ * @param beanValidator The Bean Validation validator
+ */
+ JakartaPersistentEntityValidator(PersistentEntity entity, MessageSource
messageSource, ConstraintsEvaluator constraintsEvaluator,
+ ConstraintRegistry constraintRegistry, Validator beanValidator) {
+ super(entity, messageSource, constraintsEvaluator)
+ this.beanValidator = beanValidator instanceof GormValidatorAdapter
+ ? (GormValidatorAdapter) beanValidator
+ : new GormValidatorAdapter(beanValidator)
+ this.constraintsEvaluator = constraintsEvaluator
+ this.translator = new JakartaConstraintsTranslator(this.beanValidator,
targetClass)
+ Map<String, ConstrainedProperty> validated =
super.getValidatedConstrainedProperties(NO_VALIDATION_HINTS)
Review Comment:
Fixed in 1bd4754bbd: a class with `@GroupSequenceProvider` redefines the
default group for `JakartaConstraintsTranslator`, as it does in
`validateConstraintsForDefaultGroup`; the class is described with the sequence
the provider gives for no instance, as its `BeanDescriptor` is, and
`findDefaultGroups(instance)` gives the groups of the sequence the provider
chooses for an instance. `getValidatedConstrainedProperties` now receives the
object, so the `nullable` constraints are relaxed with the groups of the
instance, and the adapter's filter expands the default group of each bean it
reads for that bean. Regressions: `JakartaConstraintsTranslatorSpec` (the
static and the per-instance groups of `ProvidedProduct`),
`JakartaValidatorRegistrySpec` and the TCK `BeanValidationSpec` (your case,
`nullable: false` with `@Null(groups = RetiredChecks)`: an active member
without a desk gets `nullable`, a retired one is valid, as the provider says,
and a retired one with a desk gets `Null`; the `
constrainedProperties` describe the class for no instance, so `desk` stays
required there). All fail without the fix.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/validation/jakarta/GormValidatorAdapter.groovy:
##########
@@ -60,6 +74,147 @@ class GormValidatorAdapter extends SpringValidatorAdapter
implements CascadingVa
}
}
+ /**
+ * Validates the object in the given groups, or in the default group when
there are none
+ *
+ * @param obj The object to validate
+ * @param errors The errors to report the violations to
+ * @param cascade Whether validation cascades to the persistent
associations of the entity through the
+ * properties marked {@code @Valid}; it cascades through the other
properties marked {@code @Valid} either way
+ * @param groups The groups to validate
+ * @since 8.0.1
+ */
+ void validate(Object obj, Errors errors, boolean cascade, Class<?>[]
groups) {
+ CASCADE_VALIDATION.set(cascade)
+ try {
+ validate(obj, errors, (Object[]) groups)
+ } finally {
+ CASCADE_VALIDATION.remove()
+ }
+ }
+
+ /**
+ * Validates the object in the given groups as {@link #validate(Object,
Errors, boolean, Class[])} does,
+ * reporting only the violations of the constraints declared in one of the
reported groups. The provider
+ * validates a group together with the groups it extends; when validation
cascades from an object validated in
+ * a group whose extended groups a {@code @ConvertGroup} converts, the
associated object is validated in the
+ * group alone, so the violations of the converted groups are left out.
The reported groups are the ones of the
+ * object; for an object the provider cascades to through the properties
marked {@code @Valid}, they are converted
+ * along the path of the violation as the {@code @ConvertGroup}
annotations of those properties declare, the way
+ * the provider converts the groups it validates in, see {@link
#isReported}.
+ *
+ * @param obj The object to validate
+ * @param errors The errors to report the violations to
+ * @param cascade Whether validation cascades to the persistent
associations of the entity
+ * @param groups The groups to validate
+ * @param reportedGroups The groups whose violations to report, null to
report every violation
+ * @since 8.0.1
+ */
+ void validate(Object obj, Errors errors, boolean cascade, Class<?>[]
groups, Set<Class<?>> reportedGroups) {
+ if (reportedGroups == null) {
+ validate(obj, errors, cascade, groups)
+ return
+ }
+ CASCADE_VALIDATION.set(cascade)
+ try {
+ Set<ConstraintViolation<Object>> reported = new LinkedHashSet<>()
+ for (ConstraintViolation<Object> violation :
thisValidator.validate(obj, groups)) {
+ if (isReported(violation, reportedGroups)) {
+ reported.add(violation)
+ }
+ }
+ processConstraintViolations(reported, errors)
+ } finally {
+ CASCADE_VALIDATION.remove()
+ }
+ }
+
+ /**
+ * Whether the violation is of a constraint declared in one of the
reported groups, as they apply to the bean
+ * holding the constraint. The provider cascades to a bean through a
property marked {@code @Valid} once for each
+ * group it validates the owner in, converting the group as the {@code
@ConvertGroup} annotations of the property
+ * declare: a converted group is validated as the group it converts to,
with the groups that one extends or
+ * sequences, and an unconverted group alone. The reported groups follow
the same conversions along the path of
+ * the violation, from the validated object to the bean holding the
constraint, so a violation the provider found
+ * through a conversion of a reported group is reported, and one it found
through a group that is not reported is
+ * not. The default group of a bean stands for the groups of the sequence
redefining it, if any.
+ */
+ private boolean isReported(ConstraintViolation<?> violation, Set<Class<?>>
reportedGroups) {
+ List<Path.Node> nodes = violation.propertyPath.toList()
+ Class<?> beanClass = violation.rootBeanClass
+ Set<Class<?>> groups = reportedGroups
+ // the last node is the constrained element, the nodes before it the
properties the provider cascaded through
+ for (int i = 0; i < nodes.size() - 1; i++) {
+ Path.Node node = nodes.get(i)
+ if (node.kind != ElementKind.PROPERTY) {
+ continue
+ }
+ PropertyDescriptor property =
thisValidator.getConstraintsForClass(beanClass).getConstraintsForProperty(node.name)
+ CascadableDescriptor cascaded =
JakartaConstraintsTranslator.cascadedDescriptorOf(property)
+ if (cascaded != null) {
+ groups = convertedGroupsOf(withDefaultGroupsOf(groups,
beanClass), cascaded.groupConversions)
+ }
+ beanClass = i == nodes.size() - 2
Review Comment:
Fixed in 1bd4754bbd: the walk reads the beans the provider cascaded to from
the validated object, by property and, for an element of a container, by the
index or key of the next node, and looks up the descriptor, the conversions and
the default groups of the class of each bean; the leaf bean comes from the
violation as before. Only an element of a set, which the path does not
identify, is still read as the declared type. Regressions:
`GormValidatorAdapterSpec` (`Stamp.attachment`, declared as an `Object`,
converting `ExtendedGroup` to `DetailGroup` for its details) and
`JakartaValidatorRegistrySpec` (your case: `ExtendedChild.record`, an `Object`
holding a record that converts `Extended` to `DetailsChecks` for its details;
validated in `Extended`, the owner reports `child.record.details.summary`, as
the provider does). Both fail without the fix.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/validation/jakarta/GormValidatorAdapter.groovy:
##########
@@ -60,6 +74,147 @@ class GormValidatorAdapter extends SpringValidatorAdapter
implements CascadingVa
}
}
+ /**
+ * Validates the object in the given groups, or in the default group when
there are none
+ *
+ * @param obj The object to validate
+ * @param errors The errors to report the violations to
+ * @param cascade Whether validation cascades to the persistent
associations of the entity through the
+ * properties marked {@code @Valid}; it cascades through the other
properties marked {@code @Valid} either way
+ * @param groups The groups to validate
+ * @since 8.0.1
+ */
+ void validate(Object obj, Errors errors, boolean cascade, Class<?>[]
groups) {
+ CASCADE_VALIDATION.set(cascade)
+ try {
+ validate(obj, errors, (Object[]) groups)
+ } finally {
Review Comment:
Fixed in 1bd4754bbd: each `validate` overload remembers the flag it found
and puts it back when it ends, removing it only when there was none, so the
enclosing validation cascades as it did. The three-argument overload leaked the
same way and got the same treatment. Regressions: `GormValidatorAdapterSpec`
(the flag a constraint validating another object with the adapter sees before
and after its own validation), `JakartaValidatorRegistrySpec` (your case: with
the auditor validated by its constraint in between, `details.leaf.label` is
still reported) and the TCK `BeanValidationSpec` (`BeanValidatedLedger`, whose
`@ValidCounterpart` calls `counterpart.validate()`). All fail without the fix.
--
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]
