spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.62k stars 38.13k forks source link

`SpringValidatorAdapter` fails in `getRejectedValue` if `ValueExtractor` used in property path to unwrap a container type #29043

Closed xak2000 closed 10 months ago

xak2000 commented 2 years ago

Affects: 5.3.22

Related issue: #16855.

This issue is basically the same as #16855, but for a custom wrapper (#16855 only handles Optional).

SpringValidatorAdapter couldn't process ConstraintViolations because it can't traverse property path. It throws this exception:

Caused by: java.lang.IllegalStateException: JSR-303 validated property 'location.name' does not have a corresponding accessor for Spring data binding - check your DataBinder's configuration (bean property versus direct field access)
    at org.springframework.validation.beanvalidation.SpringValidatorAdapter.processConstraintViolations(SpringValidatorAdapter.java:188) ~[spring-context-5.3.22.jar:5.3.22]
    at org.springframework.validation.beanvalidation.SpringValidatorAdapter.validate(SpringValidatorAdapter.java:109) ~[spring-context-5.3.22.jar:5.3.22]
    at com.example.demo.DemoApplication.lambda$run$0(DemoApplication.java:29) ~[classes/:na]
    at org.springframework.boot.SpringApplication.callRunner(SpringApplication.java:762) ~[spring-boot-2.7.3.jar:2.7.3]
    ... 5 common frames omitted
Caused by: org.springframework.beans.NotReadablePropertyException: Invalid property 'location.name' of bean class [com.example.demo.UpdateRequest]: Bean property 'location.name' is not readable or has an invalid getter method: Does the return type of the getter match the parameter type of the setter?
    at org.springframework.beans.AbstractNestablePropertyAccessor.getPropertyValue(AbstractNestablePropertyAccessor.java:627) ~[spring-beans-5.3.22.jar:5.3.22]
    at org.springframework.beans.AbstractNestablePropertyAccessor.getPropertyValue(AbstractNestablePropertyAccessor.java:617) ~[spring-beans-5.3.22.jar:5.3.22]
    at org.springframework.validation.AbstractPropertyBindingResult.getActualFieldValue(AbstractPropertyBindingResult.java:104) ~[spring-context-5.3.22.jar:5.3.22]
    at org.springframework.validation.AbstractBindingResult.getRawFieldValue(AbstractBindingResult.java:284) ~[spring-context-5.3.22.jar:5.3.22]
    at org.springframework.validation.beanvalidation.SpringValidatorAdapter.getRejectedValue(SpringValidatorAdapter.java:318) ~[spring-context-5.3.22.jar:5.3.22]
    at org.springframework.validation.beanvalidation.SpringValidatorAdapter.processConstraintViolations(SpringValidatorAdapter.java:174) ~[spring-context-5.3.22.jar:5.3.22]
    ... 8 common frames omitted

But the problem in this case not with getter/setter, but with the fact that actual field type is not simple POJO, but a custom wrapper class.

Example:

public class UpdateRequest {

    // Note the custom wrapper type
    @NotNull
    @Valid
    private NullableOptional<Location> location = NullableOptional.absent();

    public NullableOptional<Location> getLocation() {
        return location;
    }

    public void setLocation(NullableOptional<Location> location) {
        this.location = location;
    }

}

public class Location {

    @NotEmpty
    private String name;

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

}

public final class NullableOptional<T> {

    private static final NullableOptional<?> ABSENT = new NullableOptional<>(true, null);

    private final boolean absent;
    private final T value;

    private NullableOptional(boolean absent, T value) {
        this.absent = absent;
        this.value = !absent ? value : null;
    }

    public static <T> NullableOptional<T> absent() {
        @SuppressWarnings("unchecked")
        NullableOptional<T> t = (NullableOptional<T>) ABSENT;
        return t;
    }

    public static <T> NullableOptional<T> of(T value) {
        return new NullableOptional<>(false, value);
    }

    public T get() {
        if (absent) {
            throw new NoSuchElementException("No value present");
        }
        return value;
    }

    public boolean isPresent() {
        return !absent;
    }

    public boolean isAbsent() {
        return absent;
    }

    public void ifPresent(Consumer<? super T> action) {
        if (!absent) {
            action.accept(value);
        }
    }

    public void ifPresentOrElse(Consumer<? super T> action, Runnable absentAction) {
        if (!absent) {
            action.accept(value);
        } else {
            absentAction.run();
        }
    }

    public T orElse(T other) {
        return !absent ? value : other;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj) {
            return true;
        }

        return obj instanceof NullableOptional<?> other
                && absent == other.absent
                && Objects.equals(value, other.value);
    }

    @Override
    public int hashCode() {
        return Objects.hash(absent, value);
    }

    @Override
    public String toString() {
        return !absent
                ? String.format("NullableOptional[%s]", value)
                : "NullableOptional.absent";
    }

}

@UnwrapByDefault
public class NullableOptionalValueExtractor implements ValueExtractor<NullableOptional<@ExtractedValue ?>> {

    @Override
    public void extractValues(NullableOptional<?> originalValue, ValueReceiver receiver) {
        if (originalValue.isPresent()) {
            receiver.value(null, originalValue.get());
        }
    }

}

Note, that NullableOptionalValueExtractor is registered using META-INF/services/javax.validation.valueextraction.ValueExtractor as described here and correctly unwraps the field value for validation purposes. @UnwrapByDefault makes validation annotations (e.g. @NotNull) on NullableOptional field work as if they were on a wrapped type (Location in this example).

And the validation example, that throws mentioned exception:

@SpringBootApplication
public class DemoApplication {

    public static void main(String[] args) {
        SpringApplication.run(DemoApplication.class, args);
    }

    @Bean
    ApplicationRunner run(Validator validator) {
        return args -> {
            Location location = new Location();
            location.setName(null);

            UpdateRequest updateRequest = new UpdateRequest();
            updateRequest.setLocation(NullableOptional.of(location));

            BeanPropertyBindingResult bindingResult = new BeanPropertyBindingResult(updateRequest, "updateRequest");
            validator.validate(updateRequest, bindingResult);
            System.out.println("bindingResult: " + bindingResult);
        };
    }

}

The validation itself that Hibernate-validator performs works fine. But then SpringValidatorAdapter throws. Looks like it isn't aware of ValueExtractor concept.

What can be done in this case to make it work (or at least not throw)? Can the support of custom wrapper types be added in some generic way that will work out of the box (just like it's done in Hibernate validator thanks to ValueExtractor)? Or, at least, skip the extraction of a value if not possible instead of throw the error.

What workaround exists for now? I tried to debug the source code and unfortunately didn't found any possibility for a workaround.

The main goal is to use NullableOptional<T> (or any other custom wrapper) for fields of JSON body of PATCH requests, where a field value could be either null, non-null or absent (when field is absent in JSON representation altogether) and business logic needs to distinguish between all 3 cases. Sometimes people use Optional<T> for this purpose, but it's error-prone and ugly, as third state of Optional field is the null-reference, that is anti-pattern and usually unexpected usage of Optional type. Custom wrapper type solves these problems as it's intended to be used specifically for the mentioned case and will never reference to null (but could contain null inside).

Minimal Demo Project: custom-container-validation-bug.zip

xak2000 commented 2 years ago

The workaround:

I created a subclass of LocalValidatorFactoryBean and exposed it as a bean. That subclass just ignores the exception thrown from org.springframework.validation.beanvalidation.SpringValidatorAdapter#getRejectedValue method and uses violation.getInvalidValue() directly instead:

@Slf4j
public class WorkaroundLocalValidatorFactoryBean extends LocalValidatorFactoryBean {

    @Override
    protected Object getRejectedValue(@NonNull String field, @NonNull ConstraintViolation<Object> violation,
            @NonNull BindingResult bindingResult) {

        try {
            return super.getRejectedValue(field, violation, bindingResult);
        } catch (Exception e) {
            log.warn("Workaround for spring-projects/spring-framework#29043: {}", e.getMessage());
            return violation.getInvalidValue();
        }
    }

}

The original getRejectedValue method of SpringValidatorAdapter class already calls bindingResult.getRawFieldValue(field) conditionally:

    @Nullable
    protected Object getRejectedValue(String field, ConstraintViolation<Object> violation, BindingResult bindingResult) {
        Object invalidValue = violation.getInvalidValue();
        if (!field.isEmpty() && !field.contains("[]") &&
                (invalidValue == violation.getLeafBean() || field.contains("[") || field.contains("."))) {
            // Possibly a bean constraint with property path: retrieve the actual property value.
            // However, explicitly avoid this for "address[]" style paths that we can't handle.
            invalidValue = bindingResult.getRawFieldValue(field);
        }
        return invalidValue;
    }

Maybe it's possible to add more conditions here, if it turns out that generic support of custom field-wrapper types will not be possible? I'm not sure why bindingResult.getRawFieldValue(field) is required if we already have violation.getInvalidValue(), though. Maybe the first one uses ConversionService or something...

mateusbandeiraa commented 2 years ago

I'm facing a similar problem but not sure if it is exactly the same or if I should raise another issue. I created a minimal example:

public class Foo {
    private Map<Class<?>, @Valid Bar> instanceExamples;

    // Omitted constructors, getters and setters
}

public class Bar {
    @NotBlank
    @Length(max = 5)
    public String value;

    // Omitted constructors, getters and setters
}
@SpringBootApplication
@RestController
public class SpringValidationReproducerApplication {

    public static void main(String[] args) {
        SpringApplication.run(SpringValidationReproducerApplication.class, args);
    }

    @Bean
    ApplicationRunner run(Validator validator) {
        return args -> {
            Foo foo = new Foo(
                    Map.of(Bar.class, new Bar("Invalid instance example because it is too long")));
            BeanPropertyBindingResult bindingResult = new BeanPropertyBindingResult(foo, "foo");
            validator.validate(foo, bindingResult);
            System.out.println("bindingResult: " + bindingResult);
        };
    }

}

Stacktrace:

java.lang.IllegalStateException: Failed to execute ApplicationRunner
    at org.springframework.boot.SpringApplication.callRunner(SpringApplication.java:765) ~[spring-boot-2.7.3.jar:2.7.3]
    at org.springframework.boot.SpringApplication.callRunners(SpringApplication.java:752) ~[spring-boot-2.7.3.jar:2.7.3]
    at org.springframework.boot.SpringApplication.run(SpringApplication.java:315) ~[spring-boot-2.7.3.jar:2.7.3]
    at org.springframework.boot.SpringApplication.run(SpringApplication.java:1306) ~[spring-boot-2.7.3.jar:2.7.3]
    at org.springframework.boot.SpringApplication.run(SpringApplication.java:1295) ~[spring-boot-2.7.3.jar:2.7.3]
    at dev.bandeira.springvalidationreproducer.SpringValidationReproducerApplication.main(SpringValidationReproducerApplication.java:18) ~[classes/:na]
    at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) ~[na:na]
    at java.base/java.lang.reflect.Method.invoke(Method.java:577) ~[na:na]
    at org.springframework.boot.devtools.restart.RestartLauncher.run(RestartLauncher.java:49) ~[spring-boot-devtools-2.7.3.jar:2.7.3]
Caused by: org.springframework.beans.InvalidPropertyException: Invalid property 'instanceExamples[class dev.bandeira.springvalidationreproducer.Bar]' of bean class [dev.bandeira.springvalidationreproducer.Foo]: Invalid index in property path 'instanceExamples[class dev.bandeira.springvalidationreproducer.Bar]'; nested exception is org.springframework.beans.TypeMismatchException: Failed to convert property value of type 'java.lang.String' to required type 'java.lang.Class' for property 'null'; nested exception is java.lang.IllegalArgumentException: Could not find class [class dev.bandeira.springvalidationreproducer.Bar]
    at org.springframework.beans.AbstractNestablePropertyAccessor.getPropertyValue(AbstractNestablePropertyAccessor.java:704) ~[spring-beans-5.3.22.jar:5.3.22]
    at org.springframework.beans.AbstractNestablePropertyAccessor.getNestedPropertyAccessor(AbstractNestablePropertyAccessor.java:843) ~[spring-beans-5.3.22.jar:5.3.22]
    at org.springframework.beans.AbstractNestablePropertyAccessor.getPropertyAccessorForPropertyPath(AbstractNestablePropertyAccessor.java:820) ~[spring-beans-5.3.22.jar:5.3.22]
    at org.springframework.beans.AbstractNestablePropertyAccessor.getPropertyValue(AbstractNestablePropertyAccessor.java:615) ~[spring-beans-5.3.22.jar:5.3.22]
    at org.springframework.validation.AbstractPropertyBindingResult.getActualFieldValue(AbstractPropertyBindingResult.java:104) ~[spring-context-5.3.22.jar:5.3.22]
    at org.springframework.validation.AbstractBindingResult.getRawFieldValue(AbstractBindingResult.java:284) ~[spring-context-5.3.22.jar:5.3.22]
    at org.springframework.validation.beanvalidation.SpringValidatorAdapter.getRejectedValue(SpringValidatorAdapter.java:318) ~[spring-context-5.3.22.jar:5.3.22]
    at org.springframework.validation.beanvalidation.SpringValidatorAdapter.processConstraintViolations(SpringValidatorAdapter.java:174) ~[spring-context-5.3.22.jar:5.3.22]
    at org.springframework.validation.beanvalidation.SpringValidatorAdapter.validate(SpringValidatorAdapter.java:109) ~[spring-context-5.3.22.jar:5.3.22]
    at dev.bandeira.springvalidationreproducer.SpringValidationReproducerApplication.lambda$0(SpringValidationReproducerApplication.java:27) ~[classes/:na]
    at org.springframework.boot.SpringApplication.callRunner(SpringApplication.java:762) ~[spring-boot-2.7.3.jar:2.7.3]
    ... 8 common frames omitted
Caused by: org.springframework.beans.TypeMismatchException: Failed to convert property value of type 'java.lang.String' to required type 'java.lang.Class' for property 'null'; nested exception is java.lang.IllegalArgumentException: Could not find class [class dev.bandeira.springvalidationreproducer.Bar]
    at org.springframework.beans.AbstractNestablePropertyAccessor.convertIfNecessary(AbstractNestablePropertyAccessor.java:600) ~[spring-beans-5.3.22.jar:5.3.22]
    at org.springframework.beans.AbstractNestablePropertyAccessor.getPropertyValue(AbstractNestablePropertyAccessor.java:686) ~[spring-beans-5.3.22.jar:5.3.22]
    ... 18 common frames omitted
Caused by: java.lang.IllegalArgumentException: Could not find class [class dev.bandeira.springvalidationreproducer.Bar]
    at org.springframework.util.ClassUtils.resolveClassName(ClassUtils.java:334) ~[spring-core-5.3.22.jar:5.3.22]
    at org.springframework.beans.propertyeditors.ClassEditor.setAsText(ClassEditor.java:65) ~[spring-beans-5.3.22.jar:5.3.22]
    at org.springframework.beans.TypeConverterDelegate.doConvertTextValue(TypeConverterDelegate.java:429) ~[spring-beans-5.3.22.jar:5.3.22]
    at org.springframework.beans.TypeConverterDelegate.doConvertValue(TypeConverterDelegate.java:402) ~[spring-beans-5.3.22.jar:5.3.22]
    at org.springframework.beans.TypeConverterDelegate.convertIfNecessary(TypeConverterDelegate.java:155) ~[spring-beans-5.3.22.jar:5.3.22]
    at org.springframework.beans.AbstractNestablePropertyAccessor.convertIfNecessary(AbstractNestablePropertyAccessor.java:590) ~[spring-beans-5.3.22.jar:5.3.22]
    ... 19 common frames omitted
Caused by: java.lang.ClassNotFoundException: class dev.bandeira.springvalidationreproducer.Bar
    at java.base/java.lang.Class.forNameImpl(Native Method) ~[na:na]
    at java.base/java.lang.Class.forNameHelper(Class.java:454) ~[na:na]
    at java.base/java.lang.Class.forName(Class.java:432) ~[na:na]
    at org.springframework.util.ClassUtils.forName(ClassUtils.java:284) ~[spring-core-5.3.22.jar:5.3.22]
    at org.springframework.util.ClassUtils.resolveClassName(ClassUtils.java:324) ~[spring-core-5.3.22.jar:5.3.22]
    ... 24 common frames omitted

From what I could tell, Spring could not convert the String class dev.bandeira.springvalidationreproducer.Bar into the actual Class reference dev.bandeira.springvalidationreproducer.Bar. min-reproduction.zip

jbesta commented 2 years ago

Hi,

I'm facing exactly same issues as @xak2000 mentions. I'm using validation of JsonNullable from https://github.com/OpenAPITools/jackson-databind-nullable

niklasfi commented 1 year ago

Finally I've found the issue I'm having. I am also using a custom reference class with semantics similar to java.util.Optional. Thank you @xak2000 for providing a workaround, which I can report to be working on my end. For completeness, the WorkaroundLocalValidatorFactoryBean looks like this on my end to make it work in combination with ValidationConfigurationCustomizers:

import jakarta.validation.Configuration;
import jakarta.validation.ConstraintViolation;
import jakarta.validation.MessageInterpolator;
import java.util.List;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.autoconfigure.validation.ValidationConfigurationCustomizer;
import org.springframework.lang.NonNull;
import org.springframework.stereotype.Component;
import org.springframework.validation.BindingResult;
import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean;

@Component
@Slf4j
@RequiredArgsConstructor(onConstructor = @__(@Autowired))
public class WorkaroundLocalValidatorFactoryBean extends LocalValidatorFactoryBean {

    private final List<ValidationConfigurationCustomizer> customizers;

    @Override
    protected Object getRejectedValue(
            @NonNull String field,
            @NonNull ConstraintViolation<Object> violation,
            @NonNull BindingResult bindingResult
    ) {

        try {
            return super.getRejectedValue(field, violation, bindingResult);
        } catch (Exception e) {
            log.warn("Workaround for spring-projects/spring-framework#29043: {}", e.getMessage());
            return violation.getInvalidValue();
        }
    }

    @Override
    protected void postProcessConfiguration(@NonNull Configuration<?> configuration) {
        super.postProcessConfiguration(configuration);

        customizers.forEach(customizer -> customizer.customize(configuration));
    }
}
rstoyanchev commented 10 months ago

Bean Validation uses registered ValueExtractors internally to unwrap values in the property path and there is no way to know what those values are. There is a related issue #31746 where a similar question came up, and I created https://github.com/jakartaee/validation/issues/194, which may provide help in the future if accepted.

In the meantime, I'll have a look at ways to improve getRejectedValue. In the very least we could be more lenient if the property path isn't always expected to work.

xak2000 commented 10 months ago

At this point I slightly out of the context of this task.

@rstoyanchev Do you mean that violation.getInvalidValue() actually returns wrapped value, and that is why getRejectedValue can't just return it as is (or at least it would be not desired). It needs to unwrap it, but it can't as there is no way to know how. Do I understand the problem correctly?

rstoyanchev commented 10 months ago

The code to to access the property value was introduced in this change aedccec67efc5512d0e39f5c888dc876625e2d4f. The commit goes back a long time but tests do fail if removed. The case as explained in the code comment and in https://github.com/spring-projects/spring-framework/issues/13536#issuecomment-453363730 has to do with bean level constraints that refer to properties like in this example for a failing test.

There are already protective checks for containers like !field.contains("[]") for Iterable. We need to make this even more defensive recognizing there are other custom containers that we can't detect that also prevent property access. We need to anticipate that and ignore property access exceptions.