You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bval.apache.org by David Blevins <da...@gmail.com> on 2019/05/21 19:35:23 UTC
BVAL-174 Return Parameter Validation Ignore void methods
Hi All!
I'm personally open to any of the proposed resolutions of BVAL-175. More important for me would be BVAL-174 as it blocks this feature:
- http://tomee.apache.org/tomee-8.0/examples/mp-jwt-bean-validation.html <http://tomee.apache.org/tomee-8.0/examples/mp-jwt-bean-validation.html>
Any chance this PR could be reviewed?
- https://github.com/apache/bval/pull/3 <https://github.com/apache/bval/pull/3>
--
David Blevins
http://twitter.com/dblevins
http://www.tomitribe.com
Re: BVAL-174 Return Parameter Validation Ignore void methods
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hmm, think a few things are mixed up here so let's try to develop my view
and see how it can meet:
1. I think the spirit of Bean Validation is the same than in EE, i.e. try
to harness the programming model so if anything can't be supported then
fail (~https://beanvalidation.org/2.0/spec/#exception-constraintdeclaration to
quote a simple part of the spec)
2. Bean Validation is also about being declarative so not about being
"contextual" as CDI can be which would break its static side which makes
its value
3. What you describe is more a CDI interceptor doing the Jwt lookup and
validating it - potentally with Bean Validation programmatically.
Now personally I tend to validate the jwt through a parameter passing in
method which would enable to use bean validation as usual. If you think
JAX-RS - I guess it is where you are coming from? it can be trivial to make
the Jwt a @Context through a context provider and then validating it is as
easy as putting a @ValidJwtXXXX on the param.
Does it make some sense?
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le jeu. 11 juil. 2019 à 10:51, David Blevins <da...@gmail.com> a
écrit :
> Thanks for the ping and sorry this fell off my radar. Even more, thanks
> for the work!
>
> If we threw some kind of exception, this can work. It would be a little
> bit expensive in practice as it would be happening every time, which means
> anyone who uses this feature will pay the cost of triggering a stack trace
> each call. This could still be someone what livable as they're
> theoretically also doing an RSA public key check in the same call, which
> I'd expect to be more expensive (untested assumption).
>
> For this to work fully, we'd need some way to tell the BValInterceptor to
> catch the exception and keep going. Right around here we'd need some logic
> that catches the "no valid constraints" exception and still keeps going:
>
> -
> https://github.com/apache/bval/blob/master/bval-jsr/src/main/java/org/apache/bval/cdi/BValInterceptor.java#L163
>
> Which TCK test failed and is there a PR that has the code you two tried
> that caused it to fail?
>
> Thinking out loud, the most ideal outcome is that the constraints that can
> apply are applied. Those that cannot apply are simply ignored.
> Theoretically, a user could have a few constraints on the method: some
> aimed at the return value, some aimed at contextual data like the JWT.
> Let's imagine this scenario:
>
> @OrangeToken
> @GreenReturn
> public Green paint() {
> //...
> }
>
> We'll say under the covers the validator impls are:
>
> - @OrangeToken validatedBy = class OrangeTokenConstraint implements
> ConstraintValidator<OrangeToken, JsonWebToken>
> - @GreenReturn validatedBy = class GreenReturnConstraint implements
> ConstraintValidator<GreenReturn, Green>
>
> Here's how throwing an exception could play out:
>
> - BValInterceptor sees `OrangeTokenConstraint` cannot apply to `Green`
> and throws an exception. `GreenReturnConstraint` is therefore not
> enforced, but should.
> - MPJWTValidatorInterceptor sees `GreenReturnConstraint` cannot apply
> to `JsonWebToken` and throws an exception. `OrangeTokenConstraint` is
> therefore not enforced, but should.
>
> The call is allowed to complete even though both the return value and
> contextual @RequestScoped JsonWebToken are potentially not valid.
>
> I'd have to look around the failing TCK test to figure out what is most in
> spirit with the Bean Validation TCK, but it might be the case there's some
> future standardization work that has to be done and some meta-data we need
> to invent. Maybe the opportunity for us to pave some new ground.
>
> The basic concept is:
>
> - There might be CDI contextual objects a user might want to validate
> before a method is called (new feature)
> - There might be a return value a user might want to validate after a
> method is called (the existing feature)
>
> Essentially, some of them are before advice and some after advice. One
> flavor of the before advice could be to validate an object that is found in
> a CDI Scope.
>
> Short term, could we perhaps require someone put some kind of annotation
> on their Constraint annotation or ConstraintValidator that basically says
> "don't include me in the return value constraints", so those do not show up
> in `ReturnValueDescriptor`. It could be something very blunt and direct
> like @DescriptorType
>
> This is not really a complete idea, but want to inch the conversation
> forward.
>
>
> --
> David Blevins
> http://twitter.com/dblevins
> http://www.tomitribe.com
>
> > On Jun 28, 2019, at 12:35 AM, Thomas Andraschko <
> andraschko.thomas@gmail.com> wrote:
> >
> > ping :D
> >
> > Am Fr., 7. Juni 2019 um 14:08 Uhr schrieb Thomas Andraschko <
> > andraschko.thomas@gmail.com>:
> >
> >> Hi David,
> >>
> >> i worked together with Romain on the issue and already commited it.
> >>
> >> Our first idea was to just remove the constraint, if there is no
> matching
> >> validator for the validated return type (void in your case).
> >> This worked fine and fixed your example as the constraint was ignored.
> >> The problem however is, that the TCK forces us to not remove the
> >> constraint.
> >> So our "solution" is now to validate the constraint when constructing
> >> ReturnD and throw a exception, to indicate that there is no validator
> >> and the constraint on the method doesn't make any sense.
> >>
> >> WDYT?
> >>
> >> Best regards,
> >> Thomas
> >>
> >>
> >>
> >> Am Do., 23. Mai 2019 um 07:13 Uhr schrieb David Blevins <
> >> david.blevins@gmail.com>:
> >>
> >>>> On May 22, 2019, at 12:34 AM, Romain Manni-Bucau <
> rmannibucau@gmail.com>
> >>> wrote:
> >>>>
> >>>> Have to admit i also see it as a good opportunity to enter the
> >>>> codebase to maybe a good call for contribution ;)
> >>>
> >>> Agree. Also provides some potential way to add committers and future
> >>> potential binding votes to help get releases out the door.
> >>>
> >>> Thomas, I gave you write permission to my bval fork so you can push
> >>> commits into PR #3. You up for hacking on this together?
> >>>
> >>> - https://github.com/dblevins/bval/tree/bval-174-voidreturns
> >>>
> >>> I'm not going to have time till the TomEE 8.0.0-M3 release is out, but
> if
> >>> you add Romain's test case I can rejoin the fun next week perhaps. If
> you
> >>> are up for some pair-programming fun, just push commits right into the
> >>> fork, no need to request a review. Yay the power of git :)
> >>>
> >>>
> >>> -David
> >>>
> >>>
>
>
Re: BVAL-174 Return Parameter Validation Ignore void methods
Posted by David Blevins <da...@gmail.com>.
Thanks for the ping and sorry this fell off my radar. Even more, thanks for the work!
If we threw some kind of exception, this can work. It would be a little bit expensive in practice as it would be happening every time, which means anyone who uses this feature will pay the cost of triggering a stack trace each call. This could still be someone what livable as they're theoretically also doing an RSA public key check in the same call, which I'd expect to be more expensive (untested assumption).
For this to work fully, we'd need some way to tell the BValInterceptor to catch the exception and keep going. Right around here we'd need some logic that catches the "no valid constraints" exception and still keeps going:
- https://github.com/apache/bval/blob/master/bval-jsr/src/main/java/org/apache/bval/cdi/BValInterceptor.java#L163
Which TCK test failed and is there a PR that has the code you two tried that caused it to fail?
Thinking out loud, the most ideal outcome is that the constraints that can apply are applied. Those that cannot apply are simply ignored. Theoretically, a user could have a few constraints on the method: some aimed at the return value, some aimed at contextual data like the JWT. Let's imagine this scenario:
@OrangeToken
@GreenReturn
public Green paint() {
//...
}
We'll say under the covers the validator impls are:
- @OrangeToken validatedBy = class OrangeTokenConstraint implements ConstraintValidator<OrangeToken, JsonWebToken>
- @GreenReturn validatedBy = class GreenReturnConstraint implements ConstraintValidator<GreenReturn, Green>
Here's how throwing an exception could play out:
- BValInterceptor sees `OrangeTokenConstraint` cannot apply to `Green` and throws an exception. `GreenReturnConstraint` is therefore not enforced, but should.
- MPJWTValidatorInterceptor sees `GreenReturnConstraint` cannot apply to `JsonWebToken` and throws an exception. `OrangeTokenConstraint` is therefore not enforced, but should.
The call is allowed to complete even though both the return value and contextual @RequestScoped JsonWebToken are potentially not valid.
I'd have to look around the failing TCK test to figure out what is most in spirit with the Bean Validation TCK, but it might be the case there's some future standardization work that has to be done and some meta-data we need to invent. Maybe the opportunity for us to pave some new ground.
The basic concept is:
- There might be CDI contextual objects a user might want to validate before a method is called (new feature)
- There might be a return value a user might want to validate after a method is called (the existing feature)
Essentially, some of them are before advice and some after advice. One flavor of the before advice could be to validate an object that is found in a CDI Scope.
Short term, could we perhaps require someone put some kind of annotation on their Constraint annotation or ConstraintValidator that basically says "don't include me in the return value constraints", so those do not show up in `ReturnValueDescriptor`. It could be something very blunt and direct like @DescriptorType
This is not really a complete idea, but want to inch the conversation forward.
--
David Blevins
http://twitter.com/dblevins
http://www.tomitribe.com
> On Jun 28, 2019, at 12:35 AM, Thomas Andraschko <an...@gmail.com> wrote:
>
> ping :D
>
> Am Fr., 7. Juni 2019 um 14:08 Uhr schrieb Thomas Andraschko <
> andraschko.thomas@gmail.com>:
>
>> Hi David,
>>
>> i worked together with Romain on the issue and already commited it.
>>
>> Our first idea was to just remove the constraint, if there is no matching
>> validator for the validated return type (void in your case).
>> This worked fine and fixed your example as the constraint was ignored.
>> The problem however is, that the TCK forces us to not remove the
>> constraint.
>> So our "solution" is now to validate the constraint when constructing
>> ReturnD and throw a exception, to indicate that there is no validator
>> and the constraint on the method doesn't make any sense.
>>
>> WDYT?
>>
>> Best regards,
>> Thomas
>>
>>
>>
>> Am Do., 23. Mai 2019 um 07:13 Uhr schrieb David Blevins <
>> david.blevins@gmail.com>:
>>
>>>> On May 22, 2019, at 12:34 AM, Romain Manni-Bucau <rm...@gmail.com>
>>> wrote:
>>>>
>>>> Have to admit i also see it as a good opportunity to enter the
>>>> codebase to maybe a good call for contribution ;)
>>>
>>> Agree. Also provides some potential way to add committers and future
>>> potential binding votes to help get releases out the door.
>>>
>>> Thomas, I gave you write permission to my bval fork so you can push
>>> commits into PR #3. You up for hacking on this together?
>>>
>>> - https://github.com/dblevins/bval/tree/bval-174-voidreturns
>>>
>>> I'm not going to have time till the TomEE 8.0.0-M3 release is out, but if
>>> you add Romain's test case I can rejoin the fun next week perhaps. If you
>>> are up for some pair-programming fun, just push commits right into the
>>> fork, no need to request a review. Yay the power of git :)
>>>
>>>
>>> -David
>>>
>>>
Re: BVAL-174 Return Parameter Validation Ignore void methods
Posted by Thomas Andraschko <an...@gmail.com>.
ping :D
Am Fr., 7. Juni 2019 um 14:08 Uhr schrieb Thomas Andraschko <
andraschko.thomas@gmail.com>:
> Hi David,
>
> i worked together with Romain on the issue and already commited it.
>
> Our first idea was to just remove the constraint, if there is no matching
> validator for the validated return type (void in your case).
> This worked fine and fixed your example as the constraint was ignored.
> The problem however is, that the TCK forces us to not remove the
> constraint.
> So our "solution" is now to validate the constraint when constructing
> ReturnD and throw a exception, to indicate that there is no validator
> and the constraint on the method doesn't make any sense.
>
> WDYT?
>
> Best regards,
> Thomas
>
>
>
> Am Do., 23. Mai 2019 um 07:13 Uhr schrieb David Blevins <
> david.blevins@gmail.com>:
>
>> > On May 22, 2019, at 12:34 AM, Romain Manni-Bucau <rm...@gmail.com>
>> wrote:
>> >
>> > Have to admit i also see it as a good opportunity to enter the
>> > codebase to maybe a good call for contribution ;)
>>
>> Agree. Also provides some potential way to add committers and future
>> potential binding votes to help get releases out the door.
>>
>> Thomas, I gave you write permission to my bval fork so you can push
>> commits into PR #3. You up for hacking on this together?
>>
>> - https://github.com/dblevins/bval/tree/bval-174-voidreturns
>>
>> I'm not going to have time till the TomEE 8.0.0-M3 release is out, but if
>> you add Romain's test case I can rejoin the fun next week perhaps. If you
>> are up for some pair-programming fun, just push commits right into the
>> fork, no need to request a review. Yay the power of git :)
>>
>>
>> -David
>>
>>
Re: BVAL-174 Return Parameter Validation Ignore void methods
Posted by Thomas Andraschko <an...@gmail.com>.
Hi David,
i worked together with Romain on the issue and already commited it.
Our first idea was to just remove the constraint, if there is no matching
validator for the validated return type (void in your case).
This worked fine and fixed your example as the constraint was ignored.
The problem however is, that the TCK forces us to not remove the constraint.
So our "solution" is now to validate the constraint when constructing
ReturnD and throw a exception, to indicate that there is no validator
and the constraint on the method doesn't make any sense.
WDYT?
Best regards,
Thomas
Am Do., 23. Mai 2019 um 07:13 Uhr schrieb David Blevins <
david.blevins@gmail.com>:
> > On May 22, 2019, at 12:34 AM, Romain Manni-Bucau <rm...@gmail.com>
> wrote:
> >
> > Have to admit i also see it as a good opportunity to enter the
> > codebase to maybe a good call for contribution ;)
>
> Agree. Also provides some potential way to add committers and future
> potential binding votes to help get releases out the door.
>
> Thomas, I gave you write permission to my bval fork so you can push
> commits into PR #3. You up for hacking on this together?
>
> - https://github.com/dblevins/bval/tree/bval-174-voidreturns
>
> I'm not going to have time till the TomEE 8.0.0-M3 release is out, but if
> you add Romain's test case I can rejoin the fun next week perhaps. If you
> are up for some pair-programming fun, just push commits right into the
> fork, no need to request a review. Yay the power of git :)
>
>
> -David
>
>
Re: BVAL-174 Return Parameter Validation Ignore void methods
Posted by David Blevins <da...@gmail.com>.
> On May 22, 2019, at 12:34 AM, Romain Manni-Bucau <rm...@gmail.com> wrote:
>
> Have to admit i also see it as a good opportunity to enter the
> codebase to maybe a good call for contribution ;)
Agree. Also provides some potential way to add committers and future potential binding votes to help get releases out the door.
Thomas, I gave you write permission to my bval fork so you can push commits into PR #3. You up for hacking on this together?
- https://github.com/dblevins/bval/tree/bval-174-voidreturns
I'm not going to have time till the TomEE 8.0.0-M3 release is out, but if you add Romain's test case I can rejoin the fun next week perhaps. If you are up for some pair-programming fun, just push commits right into the fork, no need to request a review. Yay the power of git :)
-David
Re: BVAL-174 Return Parameter Validation Ignore void methods
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Will wait for Matt feedback and we should recheck the spec but can help mid
june yes. Have to admit i also see it as a good opportunity to enter the
codebase to maybe a good call for contribution ;)
We should probably try to release the 2.0.3 which fix some thread safety
issues
Le mer. 22 mai 2019 à 09:31, Thomas Andraschko <an...@gmail.com>
a écrit :
> So will you take care of it, Romain? You also know the codebase much better
> :D
>
> Am Mi., 22. Mai 2019 um 07:30 Uhr schrieb Romain Manni-Bucau <
> rmannibucau@gmail.com>:
>
> > PS: quick workaround is to add the supported target explicitly to avoid
> > RETURN_TYPE I guess, not sure how elegant it is though
> >
> > Romain Manni-Bucau
> > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > <https://rmannibucau.metawerx.net/> | Old Blog
> > <http://rmannibucau.wordpress.com> | Github <
> > https://github.com/rmannibucau> |
> > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> > <
> >
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> > >
> >
> >
> > Le mer. 22 mai 2019 à 07:18, Romain Manni-Bucau <rm...@gmail.com>
> a
> > écrit :
> >
> > > Ok,
> > >
> > > had a quick look, for executable validator types are not validated -
> > > see org.apache.bval.jsr.metadata.ReflectionBuilder.ForExecutable
> > >
> > > I guess the ComputeConstraintValidatorClass logic should be imported in
> > > executable path, not fully sure what the spec says but we should
> clearly
> > > avoid the runtime solution since here we can avoid the interceptor
> > > completely IMHO.
> > >
> > > Here is a test to reproduce - simpler - if it helps:
> > >
> > > /*
> > > * Licensed to the Apache Software Foundation (ASF) under one or more
> > > * contributor license agreements. See the NOTICE file distributed
> with
> > > * this work for additional information regarding copyright ownership.
> > > * The ASF licenses this file to You under the Apache License, Version
> > 2.0
> > > * (the "License"); you may not use this file except in compliance
> with
> > > * the License. You may obtain a copy of the License at
> > > *
> > > * http://www.apache.org/licenses/LICENSE-2.0
> > > *
> > > * Unless required by applicable law or agreed to in writing, software
> > > * distributed under the License is distributed on an "AS IS" BASIS,
> > > * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > implied.
> > > * See the License for the specific language governing permissions and
> > > * limitations under the License.
> > > */
> > > package org.apache.bval.jsr;
> > >
> > > import static java.lang.annotation.RetentionPolicy.RUNTIME;
> > > import static org.junit.Assert.assertFalse;
> > >
> > > import java.lang.annotation.Retention;
> > >
> > > import javax.validation.Constraint;
> > > import javax.validation.ConstraintValidator;
> > > import javax.validation.ConstraintValidatorContext;
> > > import javax.validation.Payload;
> > > import javax.validation.Validation;
> > > import javax.validation.ValidatorFactory;
> > > import javax.validation.metadata.MethodDescriptor;
> > > import javax.validation.metadata.ReturnValueDescriptor;
> > >
> > > import org.junit.Test;
> > >
> > > public class ReturnVoidConstraintTest {
> > > @Test
> > > public void checkIgnored() {
> > > final ValidatorFactory factory =
> > Validation.buildDefaultValidatorFactory();
> > > final MethodDescriptor method = factory.getValidator()
> > > .getConstraintsForClass(Container.class)
> > > .getConstraintsForMethod("passthrough");
> > > final ReturnValueDescriptor descriptor =
> > method.getReturnValueDescriptor();
> > > if (descriptor != null) {
> > >
> > assertFalse(descriptor.getConstraintDescriptors().toString(),
> > descriptor.hasConstraints());
> > > } // else ok
> > > factory.close();
> > > }
> > >
> > > public static class Container {
> > > @Password
> > > public void passthrough() {
> > > // no-op
> > > }
> > > }
> > >
> > > @Retention(RUNTIME)
> > > @Constraint(validatedBy = Password.Impl.class)
> > > public @interface Password {
> > > Class<?>[] groups() default {};
> > > String message() default "failed";
> > > Class<? extends Payload>[] payload() default {};
> > >
> > > class Impl implements ConstraintValidator<Password, String> {
> > > @Override
> > > public boolean isValid(final String value, final
> > ConstraintValidatorContext context) {
> > > return false;
> > > }
> > > }
> > > }
> > > }
> > >
> > >
> > > Unrelated notes: seems tomee lost the github links for examples + a
> > > requestscoped jaxrs endpoint with a local map as storage will likely
> > always
> > > return an empty list in the @GET ;).
> > >
> > > Romain Manni-Bucau
> > > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > > <https://rmannibucau.metawerx.net/> | Old Blog
> > > <http://rmannibucau.wordpress.com> | Github
> > > <https://github.com/rmannibucau> | LinkedIn
> > > <https://www.linkedin.com/in/rmannibucau> | Book
> > > <
> >
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> > >
> > >
> > >
> > > Le mer. 22 mai 2019 à 05:17, David Blevins <da...@gmail.com> a
> > > écrit :
> > >
> > >> > On May 21, 2019, at 2:20 PM, Romain Manni-Bucau <
> > rmannibucau@gmail.com>
> > >> wrote:
> > >> >
> > >> > Probably a "too late to comment" thing but why is this true
> > >> > constraintsForMethod.hasConstrainedReturnValue())
> > >> >
> > >> > Guess the code was right and either this method impl needs your diff
> > or
> > >> the
> > >> > use case is invalid.
> > >>
> > >> Never too late :) There is very likely a better fix and perhaps there
> > is
> > >> a bug here that needs a much better resolution than the hack I threw
> in
> > >> under time pressure.
> > >>
> > >> What I noticed is that if the method return type is say `String`, all
> of
> > >> the ConstraintValidators on the method that do not apply to String are
> > >> simply ignored. I am not an Bean Validation expert, so I convinced
> > myself
> > >> this was by design as you can compose constraints from other
> > constraints.
> > >> You could have an annotation called @GoodReturnData that validated all
> > >> sorts of return values using other annotated constraints and put it on
> > all
> > >> your methods. I tested this and BVal will happily ignore the ones
> that
> > do
> > >> not apply and enforce the ones that do. If none of the
> > >> ConstraintValidators match, the return value is said to be valid.
> > >>
> > >> Unless the return type is void, in which case an exception is thrown
> > >> saying it cannot find a ConstraintValidator for type Void. I forget
> the
> > >> exact stack trace, but I can get it easily enough. I took a quick
> look
> > at
> > >> what it would take to make
> > constraintsForMethod.hasConstrainedReturnValue()
> > >> return false, but spun wheels a bit too long so put the hack in
> instead
> > so
> > >> we could have a better discussion.
> > >>
> > >> Indeed, the better fix would likely be returning false. This seems
> > >> consistent the "ignore constraints that can't apply" philosophy and
> the
> > TCK
> > >> seems to be ok with that.
> > >>
> > >> Where the use case would come in is my fix does not help people using
> > the
> > >> Validator API directly to pass methods in and say "is this valid." If
> > they
> > >> were doing that via reflection, which we know they are because the API
> > call
> > >> takes a java.lang.reflect.Method instance, they'd need to filter the
> > void
> > >> methods themselves like I did in the hack. I.e. they'd need the "skip
> > >> these methods" hack too in their reflection code.
> > >>
> > >> Side note, I'd have to double check, but I seem to recall it returning
> > >> true even in the above case where none of the ConstraintValidators
> could
> > >> match the method. I'm not sure if this is because the matching is
> > dynamic
> > >> -- meaning a method could return java.lang.Object in the declaration,
> > but
> > >> at runtime return a String value and trigger validation. I don't know
> > if
> > >> Bean Validation is specified to that level or not.
> > >>
> > >>
> > >> -David
> > >>
> > >>
> >
>
Re: BVAL-174 Return Parameter Validation Ignore void methods
Posted by Thomas Andraschko <an...@gmail.com>.
So will you take care of it, Romain? You also know the codebase much better
:D
Am Mi., 22. Mai 2019 um 07:30 Uhr schrieb Romain Manni-Bucau <
rmannibucau@gmail.com>:
> PS: quick workaround is to add the supported target explicitly to avoid
> RETURN_TYPE I guess, not sure how elegant it is though
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <
> https://github.com/rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> <
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >
>
>
> Le mer. 22 mai 2019 à 07:18, Romain Manni-Bucau <rm...@gmail.com> a
> écrit :
>
> > Ok,
> >
> > had a quick look, for executable validator types are not validated -
> > see org.apache.bval.jsr.metadata.ReflectionBuilder.ForExecutable
> >
> > I guess the ComputeConstraintValidatorClass logic should be imported in
> > executable path, not fully sure what the spec says but we should clearly
> > avoid the runtime solution since here we can avoid the interceptor
> > completely IMHO.
> >
> > Here is a test to reproduce - simpler - if it helps:
> >
> > /*
> > * Licensed to the Apache Software Foundation (ASF) under one or more
> > * contributor license agreements. See the NOTICE file distributed with
> > * this work for additional information regarding copyright ownership.
> > * The ASF licenses this file to You under the Apache License, Version
> 2.0
> > * (the "License"); you may not use this file except in compliance with
> > * the License. You may obtain a copy of the License at
> > *
> > * http://www.apache.org/licenses/LICENSE-2.0
> > *
> > * Unless required by applicable law or agreed to in writing, software
> > * distributed under the License is distributed on an "AS IS" BASIS,
> > * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> > * See the License for the specific language governing permissions and
> > * limitations under the License.
> > */
> > package org.apache.bval.jsr;
> >
> > import static java.lang.annotation.RetentionPolicy.RUNTIME;
> > import static org.junit.Assert.assertFalse;
> >
> > import java.lang.annotation.Retention;
> >
> > import javax.validation.Constraint;
> > import javax.validation.ConstraintValidator;
> > import javax.validation.ConstraintValidatorContext;
> > import javax.validation.Payload;
> > import javax.validation.Validation;
> > import javax.validation.ValidatorFactory;
> > import javax.validation.metadata.MethodDescriptor;
> > import javax.validation.metadata.ReturnValueDescriptor;
> >
> > import org.junit.Test;
> >
> > public class ReturnVoidConstraintTest {
> > @Test
> > public void checkIgnored() {
> > final ValidatorFactory factory =
> Validation.buildDefaultValidatorFactory();
> > final MethodDescriptor method = factory.getValidator()
> > .getConstraintsForClass(Container.class)
> > .getConstraintsForMethod("passthrough");
> > final ReturnValueDescriptor descriptor =
> method.getReturnValueDescriptor();
> > if (descriptor != null) {
> >
> assertFalse(descriptor.getConstraintDescriptors().toString(),
> descriptor.hasConstraints());
> > } // else ok
> > factory.close();
> > }
> >
> > public static class Container {
> > @Password
> > public void passthrough() {
> > // no-op
> > }
> > }
> >
> > @Retention(RUNTIME)
> > @Constraint(validatedBy = Password.Impl.class)
> > public @interface Password {
> > Class<?>[] groups() default {};
> > String message() default "failed";
> > Class<? extends Payload>[] payload() default {};
> >
> > class Impl implements ConstraintValidator<Password, String> {
> > @Override
> > public boolean isValid(final String value, final
> ConstraintValidatorContext context) {
> > return false;
> > }
> > }
> > }
> > }
> >
> >
> > Unrelated notes: seems tomee lost the github links for examples + a
> > requestscoped jaxrs endpoint with a local map as storage will likely
> always
> > return an empty list in the @GET ;).
> >
> > Romain Manni-Bucau
> > @rmannibucau <https://twitter.com/rmannibucau> | Blog
> > <https://rmannibucau.metawerx.net/> | Old Blog
> > <http://rmannibucau.wordpress.com> | Github
> > <https://github.com/rmannibucau> | LinkedIn
> > <https://www.linkedin.com/in/rmannibucau> | Book
> > <
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >
> >
> >
> > Le mer. 22 mai 2019 à 05:17, David Blevins <da...@gmail.com> a
> > écrit :
> >
> >> > On May 21, 2019, at 2:20 PM, Romain Manni-Bucau <
> rmannibucau@gmail.com>
> >> wrote:
> >> >
> >> > Probably a "too late to comment" thing but why is this true
> >> > constraintsForMethod.hasConstrainedReturnValue())
> >> >
> >> > Guess the code was right and either this method impl needs your diff
> or
> >> the
> >> > use case is invalid.
> >>
> >> Never too late :) There is very likely a better fix and perhaps there
> is
> >> a bug here that needs a much better resolution than the hack I threw in
> >> under time pressure.
> >>
> >> What I noticed is that if the method return type is say `String`, all of
> >> the ConstraintValidators on the method that do not apply to String are
> >> simply ignored. I am not an Bean Validation expert, so I convinced
> myself
> >> this was by design as you can compose constraints from other
> constraints.
> >> You could have an annotation called @GoodReturnData that validated all
> >> sorts of return values using other annotated constraints and put it on
> all
> >> your methods. I tested this and BVal will happily ignore the ones that
> do
> >> not apply and enforce the ones that do. If none of the
> >> ConstraintValidators match, the return value is said to be valid.
> >>
> >> Unless the return type is void, in which case an exception is thrown
> >> saying it cannot find a ConstraintValidator for type Void. I forget the
> >> exact stack trace, but I can get it easily enough. I took a quick look
> at
> >> what it would take to make
> constraintsForMethod.hasConstrainedReturnValue()
> >> return false, but spun wheels a bit too long so put the hack in instead
> so
> >> we could have a better discussion.
> >>
> >> Indeed, the better fix would likely be returning false. This seems
> >> consistent the "ignore constraints that can't apply" philosophy and the
> TCK
> >> seems to be ok with that.
> >>
> >> Where the use case would come in is my fix does not help people using
> the
> >> Validator API directly to pass methods in and say "is this valid." If
> they
> >> were doing that via reflection, which we know they are because the API
> call
> >> takes a java.lang.reflect.Method instance, they'd need to filter the
> void
> >> methods themselves like I did in the hack. I.e. they'd need the "skip
> >> these methods" hack too in their reflection code.
> >>
> >> Side note, I'd have to double check, but I seem to recall it returning
> >> true even in the above case where none of the ConstraintValidators could
> >> match the method. I'm not sure if this is because the matching is
> dynamic
> >> -- meaning a method could return java.lang.Object in the declaration,
> but
> >> at runtime return a String value and trigger validation. I don't know
> if
> >> Bean Validation is specified to that level or not.
> >>
> >>
> >> -David
> >>
> >>
>
Re: BVAL-174 Return Parameter Validation Ignore void methods
Posted by Romain Manni-Bucau <rm...@gmail.com>.
PS: quick workaround is to add the supported target explicitly to avoid
RETURN_TYPE I guess, not sure how elegant it is though
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le mer. 22 mai 2019 à 07:18, Romain Manni-Bucau <rm...@gmail.com> a
écrit :
> Ok,
>
> had a quick look, for executable validator types are not validated -
> see org.apache.bval.jsr.metadata.ReflectionBuilder.ForExecutable
>
> I guess the ComputeConstraintValidatorClass logic should be imported in
> executable path, not fully sure what the spec says but we should clearly
> avoid the runtime solution since here we can avoid the interceptor
> completely IMHO.
>
> Here is a test to reproduce - simpler - if it helps:
>
> /*
> * Licensed to the Apache Software Foundation (ASF) under one or more
> * contributor license agreements. See the NOTICE file distributed with
> * this work for additional information regarding copyright ownership.
> * The ASF licenses this file to You under the Apache License, Version 2.0
> * (the "License"); you may not use this file except in compliance with
> * the License. You may obtain a copy of the License at
> *
> * http://www.apache.org/licenses/LICENSE-2.0
> *
> * Unless required by applicable law or agreed to in writing, software
> * distributed under the License is distributed on an "AS IS" BASIS,
> * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> * See the License for the specific language governing permissions and
> * limitations under the License.
> */
> package org.apache.bval.jsr;
>
> import static java.lang.annotation.RetentionPolicy.RUNTIME;
> import static org.junit.Assert.assertFalse;
>
> import java.lang.annotation.Retention;
>
> import javax.validation.Constraint;
> import javax.validation.ConstraintValidator;
> import javax.validation.ConstraintValidatorContext;
> import javax.validation.Payload;
> import javax.validation.Validation;
> import javax.validation.ValidatorFactory;
> import javax.validation.metadata.MethodDescriptor;
> import javax.validation.metadata.ReturnValueDescriptor;
>
> import org.junit.Test;
>
> public class ReturnVoidConstraintTest {
> @Test
> public void checkIgnored() {
> final ValidatorFactory factory = Validation.buildDefaultValidatorFactory();
> final MethodDescriptor method = factory.getValidator()
> .getConstraintsForClass(Container.class)
> .getConstraintsForMethod("passthrough");
> final ReturnValueDescriptor descriptor = method.getReturnValueDescriptor();
> if (descriptor != null) {
> assertFalse(descriptor.getConstraintDescriptors().toString(), descriptor.hasConstraints());
> } // else ok
> factory.close();
> }
>
> public static class Container {
> @Password
> public void passthrough() {
> // no-op
> }
> }
>
> @Retention(RUNTIME)
> @Constraint(validatedBy = Password.Impl.class)
> public @interface Password {
> Class<?>[] groups() default {};
> String message() default "failed";
> Class<? extends Payload>[] payload() default {};
>
> class Impl implements ConstraintValidator<Password, String> {
> @Override
> public boolean isValid(final String value, final ConstraintValidatorContext context) {
> return false;
> }
> }
> }
> }
>
>
> Unrelated notes: seems tomee lost the github links for examples + a
> requestscoped jaxrs endpoint with a local map as storage will likely always
> return an empty list in the @GET ;).
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github
> <https://github.com/rmannibucau> | LinkedIn
> <https://www.linkedin.com/in/rmannibucau> | Book
> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>
>
> Le mer. 22 mai 2019 à 05:17, David Blevins <da...@gmail.com> a
> écrit :
>
>> > On May 21, 2019, at 2:20 PM, Romain Manni-Bucau <rm...@gmail.com>
>> wrote:
>> >
>> > Probably a "too late to comment" thing but why is this true
>> > constraintsForMethod.hasConstrainedReturnValue())
>> >
>> > Guess the code was right and either this method impl needs your diff or
>> the
>> > use case is invalid.
>>
>> Never too late :) There is very likely a better fix and perhaps there is
>> a bug here that needs a much better resolution than the hack I threw in
>> under time pressure.
>>
>> What I noticed is that if the method return type is say `String`, all of
>> the ConstraintValidators on the method that do not apply to String are
>> simply ignored. I am not an Bean Validation expert, so I convinced myself
>> this was by design as you can compose constraints from other constraints.
>> You could have an annotation called @GoodReturnData that validated all
>> sorts of return values using other annotated constraints and put it on all
>> your methods. I tested this and BVal will happily ignore the ones that do
>> not apply and enforce the ones that do. If none of the
>> ConstraintValidators match, the return value is said to be valid.
>>
>> Unless the return type is void, in which case an exception is thrown
>> saying it cannot find a ConstraintValidator for type Void. I forget the
>> exact stack trace, but I can get it easily enough. I took a quick look at
>> what it would take to make constraintsForMethod.hasConstrainedReturnValue()
>> return false, but spun wheels a bit too long so put the hack in instead so
>> we could have a better discussion.
>>
>> Indeed, the better fix would likely be returning false. This seems
>> consistent the "ignore constraints that can't apply" philosophy and the TCK
>> seems to be ok with that.
>>
>> Where the use case would come in is my fix does not help people using the
>> Validator API directly to pass methods in and say "is this valid." If they
>> were doing that via reflection, which we know they are because the API call
>> takes a java.lang.reflect.Method instance, they'd need to filter the void
>> methods themselves like I did in the hack. I.e. they'd need the "skip
>> these methods" hack too in their reflection code.
>>
>> Side note, I'd have to double check, but I seem to recall it returning
>> true even in the above case where none of the ConstraintValidators could
>> match the method. I'm not sure if this is because the matching is dynamic
>> -- meaning a method could return java.lang.Object in the declaration, but
>> at runtime return a String value and trigger validation. I don't know if
>> Bean Validation is specified to that level or not.
>>
>>
>> -David
>>
>>
Re: BVAL-174 Return Parameter Validation Ignore void methods
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Ok,
had a quick look, for executable validator types are not validated -
see org.apache.bval.jsr.metadata.ReflectionBuilder.ForExecutable
I guess the ComputeConstraintValidatorClass logic should be imported in
executable path, not fully sure what the spec says but we should clearly
avoid the runtime solution since here we can avoid the interceptor
completely IMHO.
Here is a test to reproduce - simpler - if it helps:
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.bval.jsr;
import static java.lang.annotation.RetentionPolicy.RUNTIME;
import static org.junit.Assert.assertFalse;
import java.lang.annotation.Retention;
import javax.validation.Constraint;
import javax.validation.ConstraintValidator;
import javax.validation.ConstraintValidatorContext;
import javax.validation.Payload;
import javax.validation.Validation;
import javax.validation.ValidatorFactory;
import javax.validation.metadata.MethodDescriptor;
import javax.validation.metadata.ReturnValueDescriptor;
import org.junit.Test;
public class ReturnVoidConstraintTest {
@Test
public void checkIgnored() {
final ValidatorFactory factory =
Validation.buildDefaultValidatorFactory();
final MethodDescriptor method = factory.getValidator()
.getConstraintsForClass(Container.class)
.getConstraintsForMethod("passthrough");
final ReturnValueDescriptor descriptor =
method.getReturnValueDescriptor();
if (descriptor != null) {
assertFalse(descriptor.getConstraintDescriptors().toString(),
descriptor.hasConstraints());
} // else ok
factory.close();
}
public static class Container {
@Password
public void passthrough() {
// no-op
}
}
@Retention(RUNTIME)
@Constraint(validatedBy = Password.Impl.class)
public @interface Password {
Class<?>[] groups() default {};
String message() default "failed";
Class<? extends Payload>[] payload() default {};
class Impl implements ConstraintValidator<Password, String> {
@Override
public boolean isValid(final String value, final
ConstraintValidatorContext context) {
return false;
}
}
}
}
Unrelated notes: seems tomee lost the github links for examples + a
requestscoped jaxrs endpoint with a local map as storage will likely always
return an empty list in the @GET ;).
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le mer. 22 mai 2019 à 05:17, David Blevins <da...@gmail.com> a
écrit :
> > On May 21, 2019, at 2:20 PM, Romain Manni-Bucau <rm...@gmail.com>
> wrote:
> >
> > Probably a "too late to comment" thing but why is this true
> > constraintsForMethod.hasConstrainedReturnValue())
> >
> > Guess the code was right and either this method impl needs your diff or
> the
> > use case is invalid.
>
> Never too late :) There is very likely a better fix and perhaps there is
> a bug here that needs a much better resolution than the hack I threw in
> under time pressure.
>
> What I noticed is that if the method return type is say `String`, all of
> the ConstraintValidators on the method that do not apply to String are
> simply ignored. I am not an Bean Validation expert, so I convinced myself
> this was by design as you can compose constraints from other constraints.
> You could have an annotation called @GoodReturnData that validated all
> sorts of return values using other annotated constraints and put it on all
> your methods. I tested this and BVal will happily ignore the ones that do
> not apply and enforce the ones that do. If none of the
> ConstraintValidators match, the return value is said to be valid.
>
> Unless the return type is void, in which case an exception is thrown
> saying it cannot find a ConstraintValidator for type Void. I forget the
> exact stack trace, but I can get it easily enough. I took a quick look at
> what it would take to make constraintsForMethod.hasConstrainedReturnValue()
> return false, but spun wheels a bit too long so put the hack in instead so
> we could have a better discussion.
>
> Indeed, the better fix would likely be returning false. This seems
> consistent the "ignore constraints that can't apply" philosophy and the TCK
> seems to be ok with that.
>
> Where the use case would come in is my fix does not help people using the
> Validator API directly to pass methods in and say "is this valid." If they
> were doing that via reflection, which we know they are because the API call
> takes a java.lang.reflect.Method instance, they'd need to filter the void
> methods themselves like I did in the hack. I.e. they'd need the "skip
> these methods" hack too in their reflection code.
>
> Side note, I'd have to double check, but I seem to recall it returning
> true even in the above case where none of the ConstraintValidators could
> match the method. I'm not sure if this is because the matching is dynamic
> -- meaning a method could return java.lang.Object in the declaration, but
> at runtime return a String value and trigger validation. I don't know if
> Bean Validation is specified to that level or not.
>
>
> -David
>
>
Re: BVAL-174 Return Parameter Validation Ignore void methods
Posted by David Blevins <da...@gmail.com>.
> On May 21, 2019, at 2:20 PM, Romain Manni-Bucau <rm...@gmail.com> wrote:
>
> Probably a "too late to comment" thing but why is this true
> constraintsForMethod.hasConstrainedReturnValue())
>
> Guess the code was right and either this method impl needs your diff or the
> use case is invalid.
Never too late :) There is very likely a better fix and perhaps there is a bug here that needs a much better resolution than the hack I threw in under time pressure.
What I noticed is that if the method return type is say `String`, all of the ConstraintValidators on the method that do not apply to String are simply ignored. I am not an Bean Validation expert, so I convinced myself this was by design as you can compose constraints from other constraints. You could have an annotation called @GoodReturnData that validated all sorts of return values using other annotated constraints and put it on all your methods. I tested this and BVal will happily ignore the ones that do not apply and enforce the ones that do. If none of the ConstraintValidators match, the return value is said to be valid.
Unless the return type is void, in which case an exception is thrown saying it cannot find a ConstraintValidator for type Void. I forget the exact stack trace, but I can get it easily enough. I took a quick look at what it would take to make constraintsForMethod.hasConstrainedReturnValue() return false, but spun wheels a bit too long so put the hack in instead so we could have a better discussion.
Indeed, the better fix would likely be returning false. This seems consistent the "ignore constraints that can't apply" philosophy and the TCK seems to be ok with that.
Where the use case would come in is my fix does not help people using the Validator API directly to pass methods in and say "is this valid." If they were doing that via reflection, which we know they are because the API call takes a java.lang.reflect.Method instance, they'd need to filter the void methods themselves like I did in the hack. I.e. they'd need the "skip these methods" hack too in their reflection code.
Side note, I'd have to double check, but I seem to recall it returning true even in the above case where none of the ConstraintValidators could match the method. I'm not sure if this is because the matching is dynamic -- meaning a method could return java.lang.Object in the declaration, but at runtime return a String value and trigger validation. I don't know if Bean Validation is specified to that level or not.
-David
Re: BVAL-174 Return Parameter Validation Ignore void methods
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hello David,
Probably a "too late to comment" thing but why is this true
constraintsForMethod.hasConstrainedReturnValue())
Guess the code was right and either this method impl needs your diff or the
use case is invalid.
Wdyt?
Le mar. 21 mai 2019 à 22:18, David Blevins <da...@gmail.com> a
écrit :
> Awesome.
>
> To critique my own PR, I probably need a test case in there :) I was kind
> of in a rush and cut a corner I shouldn't have.
>
> Regardless, general thoughts are great.
>
>
> --
> David Blevins
> http://twitter.com/dblevins
> http://www.tomitribe.com
>
> > On May 21, 2019, at 1:00 PM, Thomas Andraschko <
> andraschko.thomas@gmail.com> wrote:
> >
> > Hi david,
> >
> > I will review it tommorow :)
> >
> > David Blevins <da...@gmail.com> schrieb am Di., 21. Mai 2019,
> 21:43:
> >
> >> Hi All!
> >>
> >> I'm personally open to any of the proposed resolutions of BVAL-175.
> More
> >> important for me would be BVAL-174 as it blocks this feature:
> >>
> >> -
> http://tomee.apache.org/tomee-8.0/examples/mp-jwt-bean-validation.html
> >> <http://tomee.apache.org/tomee-8.0/examples/mp-jwt-bean-validation.html
> >
> >>
> >> Any chance this PR could be reviewed?
> >>
> >> - https://github.com/apache/bval/pull/3 <
> >> https://github.com/apache/bval/pull/3>
> >>
> >>
> >> --
> >> David Blevins
> >> http://twitter.com/dblevins
> >> http://www.tomitribe.com
> >>
> >>
>
>
Re: BVAL-174 Return Parameter Validation Ignore void methods
Posted by David Blevins <da...@gmail.com>.
Awesome.
To critique my own PR, I probably need a test case in there :) I was kind of in a rush and cut a corner I shouldn't have.
Regardless, general thoughts are great.
--
David Blevins
http://twitter.com/dblevins
http://www.tomitribe.com
> On May 21, 2019, at 1:00 PM, Thomas Andraschko <an...@gmail.com> wrote:
>
> Hi david,
>
> I will review it tommorow :)
>
> David Blevins <da...@gmail.com> schrieb am Di., 21. Mai 2019, 21:43:
>
>> Hi All!
>>
>> I'm personally open to any of the proposed resolutions of BVAL-175. More
>> important for me would be BVAL-174 as it blocks this feature:
>>
>> - http://tomee.apache.org/tomee-8.0/examples/mp-jwt-bean-validation.html
>> <http://tomee.apache.org/tomee-8.0/examples/mp-jwt-bean-validation.html>
>>
>> Any chance this PR could be reviewed?
>>
>> - https://github.com/apache/bval/pull/3 <
>> https://github.com/apache/bval/pull/3>
>>
>>
>> --
>> David Blevins
>> http://twitter.com/dblevins
>> http://www.tomitribe.com
>>
>>
Re: BVAL-174 Return Parameter Validation Ignore void methods
Posted by Thomas Andraschko <an...@gmail.com>.
Hi david,
I will review it tommorow :)
David Blevins <da...@gmail.com> schrieb am Di., 21. Mai 2019, 21:43:
> Hi All!
>
> I'm personally open to any of the proposed resolutions of BVAL-175. More
> important for me would be BVAL-174 as it blocks this feature:
>
> - http://tomee.apache.org/tomee-8.0/examples/mp-jwt-bean-validation.html
> <http://tomee.apache.org/tomee-8.0/examples/mp-jwt-bean-validation.html>
>
> Any chance this PR could be reviewed?
>
> - https://github.com/apache/bval/pull/3 <
> https://github.com/apache/bval/pull/3>
>
>
> --
> David Blevins
> http://twitter.com/dblevins
> http://www.tomitribe.com
>
>