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/07/11 08:51:05 UTC
Re: BVAL-174 Return Parameter Validation Ignore void methods
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 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
> >>>
> >>>
>
>