Value extraction does not preserve generic type information

Issue description

When using a wrapper type such as Optional and unwrapping a constraints on a given field (i.e using payload = Unwrapping.Unwrap.class) the behaviour I would expect is that the generic information of the type unwrapped is preserved and used for matching a validator, but this does not seem to be the case. I.e when an Optional<List<String>> is unwrapped the type used to find a matching validator is the raw List instead of List<String>.

Example

Here is an example to better showcase this issue:
Assume a custom constraint used to validate that a list of the string is sorted alphabetically:

@Target({FIELD, METHOD, PARAMETER, ANNOTATION_TYPE, TYPE_USE})
@Retention(RUNTIME)
@Constraint(validatedBy = {})
public @interface AlphabeticallyOrderedList {
    String message() default "The list must be sorted alphabetically!";

    Class<?>[] groups() default {};

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

Assume a dto with the following structure:

public record TestDto(@AlphabeticallyOrderedList(payload = Unwrapping.Unwrap.class) Optional<List<String>> listFieldToUnwrap) {}

And finally the following validator for @AlphabeticallyOrderedList (note that we assume the existence of a utility method which can use to validate that a list of string is sorted alphabetically and the method has the following signature: private static boolean validateStringListIsAlphabeticallySorted(List<String> list)):

public static class AlphabeticallyOrderedListValidatorWithGenerics implements ConstraintValidator<AlphabeticallyOrderedList, List<String>> {
      @Override
      public boolean isValid(List<String> list, ConstraintValidatorContext context) {
          return validateStringListIsAlphabeticallySorted(list);
      }
}

Now if try to use this validator to validate TestDto validation will fail as hibernate validator will first unwrap listFieldToUnwrap but while doing so it erases the generic type information. Therefore when attempting to validate @AlphabeticallyOrderedList it will look for a validator targeting List while the one which was registered targets List<String>. Here is a test which showcases what happens:

@Test
void testDtoUnwrappingContainerTypesLeadsToExceptionWhenValidatorUsesGenerics() {
    // setup validator using generics when validating @AlphabeticallyOrderedList
    HibernateValidatorConfiguration configuration = Validation.byProvider(HibernateValidator.class).configure();
    ConstraintMapping constraintMapping = configuration.createConstraintMapping();

constraintMapping.constraintDefinition(AlphabeticallyOrderedList.class).validatedBy(AlphabeticallyOrderedListValidatorWithGenerics.class);
    configuration.addMapping(constraintMapping);
    Validator validator = configuration.buildValidatorFactory().getValidator();

    // no matter if the dto is valid or invalid the validation will fail with an exception
    List<String> validList = List.of("A", "B");
    Assertions.assertThrows(UnexpectedTypeException.class, () -> validator.validate(new TestDto(Optional.of(validList))));
    List<String> invalidList = List.of("B", "A");
    Assertions.assertThrows(UnexpectedTypeException.class, () -> validator.validate(new TestDto(Optional.of(invalidList))));
}

The problem becomes clear as validation can work when the validator for @AlphabeticallyOrderedList targets a raw List instead of a List<String>. This can be seen with this example:

public static class AlphabeticallyOrderedListValidatorWithRawTypes implements ConstraintValidator<AlphabeticallyOrderedList, List> {
    @Override
    public boolean isValid(List list, ConstraintValidatorContext context) {
        return validateStringListIsAlphabeticallySorted((List<String>)list);
    }
}



@Test
void testDtoUnwrappingContainerTypesWorksWhenValidatorUsesRawTypes() {
    // setup validator using raw types when validating @AlphabeticallyOrderedList
    HibernateValidatorConfiguration configuration = Validation.byProvider(HibernateValidator.class).configure();
    ConstraintMapping constraintMapping = configuration.createConstraintMapping();
    constraintMapping.constraintDefinition(AlphabeticallyOrderedList.class).validatedBy(AlphabeticallyOrderedListValidatorWithRawTypes.class);
    configuration.addMapping(constraintMapping);
    Validator validator = configuration.buildValidatorFactory().getValidator();

    List<String> validList = List.of("A", "B");
    // validation is correctly performed and dto results valid
    Assertions.assertEquals(0, validator.validate(new TestDto(Optional.of(validList))).size());

    List<String> invalidList = List.of("B", "A");
    // validation is correctly performed and dto results invalid
    Assertions.assertEquals(1, validator.validate(new TestDto(Optional.of(invalidList))).size());
}

Cause of the issue

The underlying cause of the issue seems to be that the generic information is erased when invoking MetaConstraints#getWrappedValueType after that the container type is unwrapped. This behaviour seems incorrect to me also because it’s inconsistent with how types are handled in the absence of any ValueExtractor (i.e generics information is preserved and used to find a matching Validator for a given constraint). Should this be fixed in the library by propagating the generics or is there some other way which I did not notice in which I can always preserve the underlying generic information when unwrapping container types?

Hello @Lorenzo-Bracci

Thanks for reaching out. I would suggest applying the constraint to the list instead of adding unwrapping. Starting with Bean Validation 2.0, constraints can be applied to type elements:

public record TestDto(Optional<@AlphabeticallyOrderedList List<String>> listFieldToUnwrap) {
}

It’s actually a recommended way to apply them (and should also correctly discover the validator for your case).

I’ll try looking closer to see if there is anything we can do to improve the case of “unwrapping”, but I think there were some limitations (or was it regarding the discovery of container extractors … it’s been a while since I’ve looked at that part :smiley:)

Thank you for the quick reply @mbekhta

I am aware that this would work but this code is automatically generated so I cannot annotate the type parameters. Also I believe that the behaviour should be consistent, so given that ValueExtractors do the unwrapping and the unwrapped type is used to look for another validator then type information should probably be held in a consistent way as long as unwrapping of non raw types is supported.

I looked into this issue and indeed the problem is here:

The getErasedType method returns the raw type instead of the parameterized type.

I recreated the issue described by @Lorenzo-Bracci and added the test case locally.
I managed to fix it by using TypeUtils from commons-lang3, which has a similar method, but returns the (possibly) ParameterizedType

Draft Poc:

/**
 * Returns the sub-types binding for the single type parameter of the super-type. E.g. for {@code IntegerProperty}
 * and {@code Property<T>}, {@code Integer} would be returned.
 */
private static Type getWrappedValueType(Type declaredType, ValueExtractorDescriptor valueExtractorDescriptor) {
    Map<TypeVariable<?>, Type> typeArguments = TypeUtils.getTypeArguments(declaredType, valueExtractorDescriptor.getContainerType());
    Type wrappedValueType = typeArguments.get(valueExtractorDescriptor.getExtractedTypeParameter());

    if ( wrappedValueType == null ) {
       throw LOG.getNoValueExtractorFoundForUnwrapException( declaredType );
    }
    return wrappedValueType;
}

However, that would add a new dependency on commons-lang3.

The other option I can think of, would be to add a new method in classmate which returns not only the raw type but the parameterized type.

What is your opinion about adding commons-lang3 to the hibernate-validator (to be exact to the engine module)? I have not contributed before, so I am not sure what the guidelines are about adding additional external dependencies.

Hey @Luc

Thanks for taking an interest in this case :slightly_smiling_face:

We try to keep the list of dependencies as small as possible, so it would be better if this could be solved without extra dependencies.

Have you encountered a similar case? If so, it would be nice to add it here if it’s different from the original.

BTW keep in mind that the use of constraints at the container level that are meant to be on the arguments is discouraged:

Hey @mbekhta ! Thank you for your very fast response!

It is the exact same issue indeed! When trying to use the ValueExtractor on containers in combination with typed classes, it does not seem to work with custom defined ConstraintValidators for these typed classes.

Indeed if we were to add the annotation on the value itself, it would work, however, we use the openapi generator to generate code for us, so it is hard to modify that.

Good to know using the commons-lang3 is discouraged! I will see if I can find another way with the current libraries on the classpath, and look at the classmate a bit more!

Hope to give an update soon :slight_smile:

Thanks again!

Hey,

Indeed, thanks for looking into this!

Maybe that’s a dumb suggestion, but if you’re considering contributions to a project, and you know the problem comes from the openapi generator… could you consider contributing a fix to the openapi generator?

Hey @yrodiere !

Also thank you for the fast response!

Mmm, interesting suggestion. That might solve our problems with future-generated code generated, however, I think solving it by moving the annotations around is solving a symptom instead of the root cause.

Right now, the ValueExtractors and ConstraintValidator on nested generics are not really working well together, maybe we run into a similar issue in the future and I would be happy to contribute to fix this!

I did some reading into the open issues for classmate and found this one:

Later in the thread there is a suggestion to add a mapper to convert ResolvedType → java.lang.reflect.Type . If this would exist, that could be used to solve the losing of type information in getWrappedValueType I think. I will ask in the classmate issues if there are plans for that.

Thank you for the very active communication!

The root cause wouldn’t be solved, that’s true, but then the feature you (or rather, OpenAPI) are trying to use is being discussed for something that looks like deprecation (Clarify that placing `@Valid` on the containers to validate container elements is discouraged · Issue #266 · jakartaee/validation · GitHub : “discourage usage”, “migrate”), so moving away from it looks like a good idea regardless.

That being said, if you would rather work on fixing the bug, that’s great and I’m sure @mbekhta will happily help you with that, as he always does :slight_smile:

1 Like