Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
xak2000 opened this issue Aug 30, 2022 · 7 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@xak2000
Copy link
Contributor

xak2000 commented Aug 30, 2022

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

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 30, 2022
@xak2000
Copy link
Contributor Author

xak2000 commented Aug 30, 2022

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
Copy link

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
Copy link

jbesta commented Oct 14, 2022

Hi,

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

@poutsma poutsma added the in: core Issues in core modules (aop, beans, core, context, expression) label Jan 30, 2023
@niklasfi
Copy link

niklasfi commented Sep 13, 2023

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 rstoyanchev self-assigned this Dec 22, 2023
@rstoyanchev rstoyanchev removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 22, 2023
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Dec 22, 2023

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 jakartaee/validation#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.

@rstoyanchev rstoyanchev added this to the 6.1.3 milestone Dec 22, 2023
@rstoyanchev rstoyanchev added the type: enhancement A general enhancement label Dec 22, 2023
@rstoyanchev rstoyanchev changed the title Validation failed when bean property is wrapped by a custom container type SpringValidatorAdapter fails in getRejectedValue if ValueExtractor used in property path to unwrap a container type Dec 22, 2023
@xak2000
Copy link
Contributor Author

xak2000 commented Dec 23, 2023

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
Copy link
Contributor

rstoyanchev commented Jan 8, 2024

The code to to access the property value was introduced in this change aedccec. The commit goes back a long time but tests do fail if removed. The case as explained in the code comment and in #13536 (comment) 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

7 participants