quarkus-bot[bot]
commented
2 years ago /cc @FroMage, @geoand, @stuartwdouglas
Open fedinskiy opened 2 years ago
quarkus-bot[bot]
commented
2 years ago /cc @FroMage, @geoand, @stuartwdouglas
geoand
commented
2 years ago Do we validate output values not wrapped in a Uni (cc @gsmet)?
gsmet
commented
2 years ago I don't know if you do but you should :). That's how the RESTEasy Classic integration is working and I think you mimicked this behavior in RESTEasy Reactive, at least that's what the OP is saying AFAICS.
As for Uni, I don't know if they got some love. I'm pretty sure they will require some special treatment. I didn't do anything on that front myself, not sure if anyone did something about it.
geoand
commented
2 years ago I just wanted to make sure it's expected behavior, thanks
geoand
commented
2 years ago This is not specific to RESTEasy Reactive - it's a general issue with using Hibernate Validator with a Uni response type, so we'll need to figure out how to deal with it.
geoand
commented
2 years ago @gsmet @yrodiere how can I invoke the validator with using a different return type than what the method has?
What I essentially want to do is something like: https://github.com/quarkusio/quarkus/compare/main...geoand:%2328421?expand=1
yrodiere
commented
2 years ago So first, that's not just a problem with Validator and Uni: that's a problem with reactive code and Validator not being designed to deal with reactive code (since the spec it implements, Bean Validation, only addresses imperative/blocking use cases). We'll have the same problem with Multi, CompletableFuture, etc.
If we start with Uni... Calling validateReturnValue will be problematic as Hibernate Validator will generate its metadata based on the Uni<T> return type, and we're passing a T as the value to validate.
One obvious solution would be to register a ValueExtractor for Uni, to have Hibernate Validator understand what Uni is and how to extract a value from it, in a blocking fashion:
public class UniValueExtractor<T> implements ContainerExtractor<Uni<T>, T> {
public UniValueExtractor() {
}
public String toString() {
return "uni";
}
public <T1, C2> void extract(Uni<T> uni, ValueProcessor<T1, ? super T, C2> perValueProcessor, T1 target, C2 context, ContainerExtractionContext extractionContext) {
if (uni != null) {
T value = uni.await().indifinitely();
perValueProcessor.process(target, value, context, extractionContext);
}
}
public boolean multiValued() {
return false;
}
}Then, to avoid blocking when validating RestEasy responses, we'd use your code to validate in a callback, but re-create a (completed) Uni before passing the value to Validator:
return uni.onItem().invoke(new Consumer<Object>() {
@Override
public void accept(Object o) {
var uniValueViolations = executableValidator.validateReturnValue(ctx.getTarget(), ctx.getMethod(), Uni.of(o));
if (!uniValueViolations.isEmpty()) {
throw new ConstraintViolationException(getMessage(ctx.getMethod(), ctx.getParameters(),
uniValueViolations), uniValueViolations);
}
}
});That way, Validator handles a Uni in the blocking fashion it knows... but does not block.
Admittedly, that's hackish and ugly. But that should work, and would not block, so I guess it checks all the boxes.
We would however have to be extra sure that this blocking-but-not-quite hack is not invoked in a context where we didn't wrap it all in a callback, because that would potentially result in blocking the event loop (people won't like that).
I'm thinking in particular of validation of method parameters of Uni type... We probably want to delay validating those as well? Be aware that we would (probably?) need to replace the Uni passed to the method with one that also has a Hibernate Validator callback (onItem(... -> validate))
Finally... I really don't know how we would handle nested fields in classes annotated with @Valid. Imagine a field like @Min(0) Uni<Integer> foo; how do we handle that in a non-blocking fashion? I don't think we can without significant changes to Hibernate Validator.
Other solutions would be to add built-in support for reactive code in Validator (hard) or reimplementing part of Validator's support for validateReturnValue in Quarkus (even more hackish/ugly than the above, and doesn't address method parameters/nested fields). So I guess the above would be our... least bad solution? Though with major caveats.
I'll let @gsmet comment on that though, he will probably have better insight.
yrodiere
commented
2 years ago Oh, an alternative would be to generate some abstract method with the same signature as the one we're validating, except the return type is T instead of Uni<T>. Then we would be able to call validateReturnValue passing that method as an argument.
Still a hack, though.
fedinskiy
commented
9 months ago @gsmet @yrodiere this issue lingers for more, that a year now and it seems, that the discussion suddenly stopped. Was there any progress?
yrodiere
commented
9 months ago @fedinskiy There was not. I explained the challenges and offered solutions above, but I have more pressing matters to attend to, and suspect @geoand and @gsmet even more so. So, unless @geoand made some progress, I think this issue is basically up for grabs.
geoand
commented
9 months ago Needless to say it's fallen completely out of my queue 😞
Describe the bug
I have two Resteasy endpoints, both annotated with. When I make them return invalid values, the String endpoint sends correct validation message, but the Uni endpoint fails with
@Sizefor output values. One of them returns String, another returns UniNo validator could be found for constraint.Expected behavior
Output of reactive endpoints should be validated in the same way as output of non-reactive endpoints.
Actual behavior
An error:
How to Reproduce?
git clone git@github.com:fedinskiy/reproducer.git -b reproducer/output-validationcd reproducermvn clean verify -Dtest=GreetingResourceTest#reactivemvn clean verify -Dtest=GreetingResourceTest#classicOutput of
uname -aorver5.19.11-200.fc36.x86_64
Output of
java -version11.0.16 temurin
GraalVM version (if different from Java)
No response
Quarkus version or git rev
2.13.0.Final
Build tool (ie. output of
mvnw --versionorgradlew --version)Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
Additional information
Last year similar issue was found in reactive-routes[1] and it was decided[2], that Resteasy is a way to go for that use case.
[1] https://github.com/quarkusio/quarkus/issues/15168 [2] https://github.com/quarkusio/quarkus/issues/15168#issuecomment-922661864