bval-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Romain Manni-Bucau <rmannibu...@gmail.com>
Subject Re: BVAL-174 Return Parameter Validation Ignore void methods
Date Wed, 22 May 2019 07:34:42 GMT
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 <andraschko.thomas@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 <rmannibucau@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 <david.blevins@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
> > >>
> > >>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message