bval-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thomas Andraschko <andraschko.tho...@gmail.com>
Subject Re: BVAL-174 Return Parameter Validation Ignore void methods
Date Wed, 22 May 2019 07:31:10 GMT
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