Clarify that placing `@Valid` on the containers to validate container elements is discouraged

[marko-bekhta]

Having the code as

@Valid
List<MyObject> strings

to apply cascading validation to the list elements is trailing from the BV 1.0 1.1 specs and in BV 2.0 the new way to define such constraints was introduced (using type argument annotations):

List<@Valid MyObject> strings

We should highlight to the users that applying cascading validation in a "legacy-way" will not be supported anymore in future versions of the spec, and that

@Valid
List<MyObject> strings

would mean that cascading validation is applied to the container itself (i.e. to the list and its properties) rather than its elements. Implementations of the current spec are encouraged to warn their users about this planned change and guide them to migrate to the correct way of achieving this through the usage of a type argument annotation

Things to also update:

• Examples in https://jakarta.ee/specifications/bean-validation/3.1/jakarta-validation-spec-3.1.html#constraintdeclarationvalidationprocess-requirements-graphvalidation-examples make sure that the "traditional style" is now "discouraged" instead.

[marko-bekhta]

A case to consider:

class MyContainer {
	Object[] elements;

	@AssertTrue
	public boolean isValidContainer() {
		return elements != null;
	}
}

class MyContainerValueExtractor implements ValueExtractor<@ExtractedValue(type = Object.class) MyContainer> {
	@Override
	public void extractValues(MyContainer originalValue, ValueReceiver receiver) {
		for ( int i = 0; i < originalValue.elements.length; i++ ) {
			receiver.indexedValue( "<iterable element>", i, originalValue.elements[i] );
		}
	}
}

where there are no type parameters for the container, and we want to be able to validate both the container and the elements. If we want to only validate the container example above would work, if it is only elements, we could add the @UnwrapByDefault to the value extractor (?). But if we want both or one here, the other there, it seems impossible unless we also add the payload to the @Valid annotation, and then we could do something like:

class MyBean {
	// how to apply @Valid to elements and/or to the container itself?
	@Valid(payload = Unwrapping.Unwrap ) // to apply to the elements
	@Valid // to apply to the container itself
	MyContainer container;
}

[yrodiere]

Could you please clarify what is the problem with @Valid's legacy auto-extraction? I can make a guess, but it's probably best to state this explicitly :)

Also... at the risk of saying something stupid... Would it make sense for @Valid to keep auto-unwrapping in the specific case where validating the container itself would not actually validate anything? I.e. when applying @Valid directly on the container would basically be meaningless?

I know I considered something like this in Search, though it's the opposite problem there: https://hibernate.atlassian.net/browse/HSEARCH-4271

it seems impossible unless we also add the payload to the @Valid annotation

That payload naming is... questionable. But I assume it's something we already have in Jakarta Validation?

My problem with Unwrapping.Unwrap is that it's an all-or-nothing, e.g. you wouldn't be able to unwrap just one level of containers. That is one of the few things that Hibernate Search does better when it comes to unwrapping IMO: https://docs.hibernate.org/stable/search/reference/en-US/html_single/#mapper-orm-containerextractor .

But it certainly is better than nothing, and it would achieve the same result as the current auto-unwrapping @Valid, so... makes sense to me?

[marko-bekhta]

Could you please clarify what is the problem with @Valid's legacy auto-extraction? I can make a guess, but it's probably best to state this explicitly :)

Overall Jakarta Validation tries to stay as explicit as possible about the constraints you are applying. Before BV 2.0 when it wasn't possible to put the annotations on the type arguments doing this

@NotNull @Valid List<MyBean> beans;

was the only option. Now that that's not the case anymore if we have the following example:

class MyBean {
   @Size(max = 5) List<@Size(min = 10) String> strings;
}

we "expect" to have list of maximum 5 strings, where each string is at least 10 chars long.
But with @Valid it's different as the @Valid placed on the list gets applied to the elements, rather than the list. That was done so (AFAIR) to let the users gradually migrate of this "legacy" approach. And it's been quite a while now...

So to summerize, we want people to be explicit and consistent about their constraints and cascading which makes it easier to reason about what's applied where.

That payload naming is... questionable. But I assume it's something we already have in Jakarta Validation?

and yes, payload is a part of the 3 requreid constraint attributes (as well as the unwrapping payload (Unwrapping.class)):

validation/src/main/java/jakarta/validation/constraints/Size.java

Lines 48 to 52 in 75a5452

	String message() default "{jakarta.validation.constraints.Size.message}";
	Class<?>[] groups() default { };
	Class<? extends Payload>[] payload() default { };

it's just that the @Valid is more of a marker for cascading than a constraint so it has none at the moment.

My problem with Unwrapping.Unwrap is that it's an all-or-nothing, e.g. you wouldn't be able to unwrap just one level of containers

I think you would be able to ... unwrapping applies only to the specific container it is requested on, let's say you'll have:

class MyBean{
  @Valid(payload = Unwrapping.Unwrap)  // <-- applied to the "elements" of `MyContainer1` and not to the `MyContainer1` itself
  MyContainer1 c1;
}

class MyContainer1 {
   List<@Valid(payload = Unwrapping.Unwrap) MyOtherContainer> list; // <-- applied to the "elements" of `MyOtherContainer` and not to the `MyOtherContainer` itself
}

class MyOtherContainer {
  @Valid MaybeContainerMaybeNot smth; // <- will get applied to the MaybeContainerMaybeNot rather than the elements
}

^ so it's not like if you put one unwrap payload it'll "cascade" further, it's just that one level where it is applied to explicitly.

[yrodiere]

My problem with Unwrapping.Unwrap is that it's an all-or-nothing, e.g. you wouldn't be able to unwrap just one level of containers

I think you would be able to ... unwrapping applies only to the specific container it is requested on, let's say you'll have:

class MyBean{
  @Valid(payload = Unwrapping.Unwrap)  // <-- applied to the "elements" of `MyContainer1` and not to the `MyContainer1` itself
  MyContainer1 c1;
}

class MyContainer1 {
   List<@Valid(payload = Unwrapping.Unwrap) MyOtherContainer> list; // <-- applied to the "elements" of `MyOtherContainer` and not to the `MyOtherContainer` itself
}

class MyOtherContainer {
  @Valid MaybeContainerMaybeNot smth; // <- will get applied to the MaybeContainerMaybeNot rather than the elements
}

^ so it's not like if you put one unwrap payload it'll "cascade" further, it's just that one level where it is applied to explicitly.

I have a suspicion you're trying to embezzle me :D I don't see what the last @Valid annotation brings if MyOtherContainer is being unwrapped anyway?

[marko-bekhta]

here's a better example, as that original one is ... incorrect 😖

class MyBean{
	// we "unwrap" here, hence we expect to cascade into elements 
	// (whatever those are based on the value extractor applied to this container type) 
	@Valid(payload = Unwrapping.Unwrap.class)
	MyContainer1 c1;
}

class MyContainer1 {
	MyInnerContainer[] elements;
}

class MyContainer1Extractor implements ValueExtractor<MyContainer1> {

	@Override
	public void extractValues(MyContainer1 originalValue, ValueReceiver receiver) {
		for ( int i = 0; i < originalValue.elements.length; i++ ) {
			// since we are "cascading" into elements we expect that @Valid will be "applied" 
			// to each originalValue.elements[i]. 
			// at this point "unwrapping" is gone, as we've already "unwrapped" the container
			// and cascading validation will be applied to originalValue.elements[i] almost as if it's a call to validator.validate(originalValue.elements[i]).
			// which means the type of the `originalValue.elements[i]` will be determined and the constraint metadata for it built/retrieved and based on it the validation checks will be performed.
			receiver.indexedValue( "<my container>", i, originalValue.elements[i] );
		}
	}
}

// Now, when the validator received this instance from the receiver ^
// it looks at the constraint metadata and sees that there's a Size constraint and another cascading.
// but this time if we don't have the unwrapping then the cascading will be applied to MyOtherContainer itself 
// and there will be no attempts to find/apply a value extractor. 
// Which in this case means we'll check the @Positive constraint only, and won't inspect MyOtherContainer#elements for anything.
class MyInnerContainer {
	@Size(min = 1, max = 10)
	List<@Valid MyOtherContainer> elements;
}

class MyOtherContainer {
	@Positive
	int someNumber;
	Something[] elements;
}

in short, payload = Unwrapping.Unwrap.class determins whether we try to apply a constraint (or what we are considering here: the cascading marker @valid) to the property (or parameter) itself, or if we should try to find a value extractor for it and apply the constraint to whatever that value extractor "receives". And as soon as that happens we "forget" about the unwrapping and follow the validation paths furhter

[yrodiere]

Sorry if I'm being dense, but I think you confirmed what I was saying:

My problem with Unwrapping.Unwrap is that it's an all-or-nothing, e.g. you wouldn't be able to unwrap just one level of containers. That is one of the few things that Hibernate Search does better when it comes to unwrapping IMO: https://docs.hibernate.org/stable/search/reference/en-US/html_single/#mapper-orm-containerextractor .

There is no way to apply @Valid to MyBean#c1 in such a way that it will extract values from MyContainer1, but will stop there and will validate all constraints on MyInnerContainer, including the @Size constraint.

But like I said:

But it certainly is better than nothing, and it would achieve the same result as the current auto-unwrapping @Valid, so... makes sense to me?

[AlexanderSchuetz97]

Hello there,

I do have a question,
currently I am using Bean Validation to validate a bean contains a generic payload in addition to some trivial metadata.
Something like this:

class A {
@Valid
private Object payload;

@Positive
private int x;
}

class B {
@NotNull
private String message;
}

In my concrete case the payload may be a Collection (List or Set) or a concrete Pojo like an instance of B.
My intentions is to validate the payload objects as follows:

1. If its a Collection, validate each element.
2. If its a pojo instance validate the pojo.

Currently this just works.

Am I correct to assume that such a validation would require writing a custom constraint in the future?
Something like:

@Target({ ElementType.TYPE_USE, ElementType.TYPE, ElementType.FIELD })
@Retention(RetentionPolicy.RUNTIME)
@Constraint(validatedBy = NestedValidatorImpl.class)
public @interface NestedValidator {
    Class<?>[] groups() default {};

    String message() default "Beep bop";

    Class<? extends Payload>[] payload() default {};
}

public class NestedValidatorImpl implements ConstraintValidator<NestedValidator, Object> {

    public NestedValidatorImpl() {
        
    }

    @Override
    public boolean isValid(Object value, ConstraintValidatorContext context) {
        if (value instanceOf Iterable) {
            // invoke validator for each element in iterable
           return true/false;
        }

        if (value != null) {
          // Invoke validator for pojo.
         return true/false;
        }

       return true;
    }
}

This does not just concern "Object" but any other Interface/Inheritance based object as any level of inheritance may bring in Iterable/Collection.

There is a lot of code written that makes this assumption. A lot of which is not even properly maintained anymore. (I already dread the amount of effort I will have to do in a byte code editor to fix this.)

Would it not be a good idea to have a dedicated annotation that behaves like I outlined into the standard, as that is what everyone has always been assuming for years now. Something like @ValidMapKey(constraints go here)/@ValidMapValue(constraings go here)/@ValidCollectionElement(constraints go here)? These would do nothing if the type at runtime does not implement Map or Iterable.

Something like @Valid(containers = true); or @Valid(containers = false); also seems more reasonable and can be implemented without a massive breaking change no?

I do happen to also maintain a code generator which generates data structures with a pattern similar as this. I do not see a way that I can support nested validation with this proposed change in that code generator.

[marko-bekhta]

Hi @AlexanderSchuetz97 ,

Thanks for sharing your use cases!

Let's address the first one. I'm not aware of the details of your particular case, but I've seen this approach in REST, where you may receive a single element or a list. Usually, the way I would recommend handling the payload in such a scenario is to go with the collection representation all the time. If you end up with a single element -- it'll get wrapped into that collection, and if you have multiple .. well you already have the collection in place. With Jackson, one can do it with this ACCEPT_SINGLE_VALUE_AS_ARRAY (https://fasterxml.github.io/jackson-databind/javadoc/2.8/com/fasterxml/jackson/databind/DeserializationFeature.html#ACCEPT_SINGLE_VALUE_AS_ARRAY) feature (either globally or at the property level).
Handling payloads this way tends to be simpler since you just need to account for the "collection" case and maybe even have some type bounds on it so it's easier to understand what you are dealing with (List<? extends SomeCommonType> payload;).

Am I correct to assume that such a validation would require writing a custom constraint in the future?

It's definitely not where we'd want users to go to.... writing such a constraint wouldn't be easy since you'd need to delegate to some Validator provider implementation internals (simply calling some validator.validate(..) wouldn't really work as it probably wouldn't be able to correctly deal with groups).

The plan is to figure out a way to handle the containers without parameters as well (#266 (comment)). And there are a few more ideas already 🤞🏻🙂, but I'll need some time to think through those first.

If its a Collection, validate each element.
If its a pojo instance validate the pojo.
Currently this just works.

Let's take a step back here... if we look at the spec itself: https://jakarta.ee/specifications/bean-validation/3.1/jakarta-validation-spec-3.1#constraintdeclarationvalidationprocess-requirements-graphvalidation, it doesn't have this scenario described explicitly, but yeah, we can derive it from the following:

• The exact type Z of the value contained in the field declared of type Y (subclass, implementation) is determined at runtime. The constraint definitions of Z are used.
• Collection-valued, array-valued and generally Iterable fields and properties may also be decorated with the @Valid annotation. This causes the contents of the iterator to be validated.

Now, what's also not specified in there is what happens when we have the following example:

@Valid List<MyBean> myBeans; 

// where at runtime the list is of type MyList:

class MyList<T> implements List<T> {

   // bunch of properties with validation constraints:
   @NotNull Something something;
   @Size(min=5) SomethingElse somethingElse;

    // list implementation...
}

Do we validate only the elements because we stated that "the contents of the iterator to be validated" ? Do we validate the container itself (those @NotNull/@Size constraints? Do we validate both elements and the container?

So the idea is to come up with a solution that lets you explicitly state what you want validated in such circumstances.

When we deal with type arguments, things could be as simple as:

• @Valid List<Something> something; -- we validate the list and not the elements
• List<@Valid Something> something; -- we validate elements and not the list
• @Valid List<@Valid Something> something; -- we validate both the list and the elements

Now .. because there can be containers without type arguments (e.g. some OptionalInt or similar) or raw types, or like in your example, just an Object, things get a bit more complex.

^ That's just purely from the spec perspective to be clear what to expect in such scenarios ...

Now.. when we look at it from the implementation perspective (i.e. from the validation provider side), when we encounter something like @Valid object or even @Valid List<Something> we need to perform (usually) unnecessary operations that aren't negligible. Since we need to determine a set of constraints based on the runtime type we encountered, when we have the @Valid List<Something> scenario, we go and lookup the metadata for some ArrayListImpl or whatever the actual type is to check if it has any constraints on it (which it obviously does not), so that's just a waist of a costly lookup. When we have the @Valid object case, things get worse in the sense that every time we encounter something like that at runtime, we need to go through all available value extractors and see if any can be applied, because it might be some sort of container...

So you see, the more explicit the user is about their intentions, the better the validation provider can perform. And I do not discard scenarios where things are just "unknown", and we want to make those extra checks, but that's more of an edge case than the most common one (e.g., going back to checking whether a List implementation has any constraints on it).

Something like @ValidMapKey(constraints go here)/@ValidMapValue(constraings go here)/@ValidCollectionElement(constraints go here)? These would do nothing if the type at runtime does not implement Map or Iterable.

yeah... sadly (or maybe not 🙂) Java wouldn't allow one to have @interface ValidCollectionElement { @AnyPossibleConstraintAnnotation[] constraints(); } and it wouldn't scale well to "custom containers", so we'd have to go the other way around and have a new annotation property or something in the current payload (property) on the constraint annotaion side instead.