HV-877 Supporting type use constraints on type arguments #332

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

khalidq commented Jul 24, 2014

This applies to PropertyMetaData. After your feedback, I'll continue the work on parameters and return values and I'll extend ParameterMetaData and ReturnValueMetaData.

HV-5-MASTER #523 FAILURE
Looks like there's a problem with this pull request

HV-5-MASTER #524 SUCCESS
This pull request looks good

Owner

gunnarmorling commented Jul 25, 2014

Hi @khalidq, looking right now. Sorry btw. for being a bit slow with feedback in the last days, there was another urgent issue to be handled.

Could you add some high-level test (using the Validator API) which shows what uses cases / scenarios are supported with this change? Ah, seeing it now. TypeUseValidationTest is what I was looking for :)

...g/hibernate/validator/internal/metadata/aggregated/PropertyMetaData.java
if ( constrainedElement.isCascading() && cascadingMember == null ) {
cascadingMember = constrainedElement.getLocation().getMember();
}
}
+ void buildTypeArgumentMetaData(ConstrainedElement constrainedElement) {
+ if ( constrainedElement.getKind() == ConstrainedElementKind.FIELD ) {
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

Only FIELD? Properties could be represented by a getter (METHOD) as well.

@khalidq

khalidq Jul 25, 2014

Contributor

Only FIELD? Properties could be represented by a getter (METHOD) as well.

This is just for now, I'll expand it to getters, return values, and parameters.

...src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java
@@ -658,6 +669,11 @@ else if ( TypeHelper.isArray( type ) ) {
iter = arrayList.iterator();
valueContext.markCurrentPropertyAsIterable();
}
+ else {
+ List<Object> list = newArrayList();
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

You could use Collections#singletonList() here.

@khalidq

khalidq Jul 25, 2014

Contributor

You could use Collections#singletonList() here.

Will do.

...src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java
+ false,
+ valueContext,
+ validationOrder,
+ Collections.<TypeArgumentMetaData>emptySet()
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

An empty set? This seems odd, or is it just some intermediate state?

@khalidq

khalidq Jul 25, 2014

Contributor

An empty set? This seems odd, or is it just some intermediate state?

This call is for HV-902, since we will be validating the bean itself (e.g. Bean implements Iterable), there will be an error when we evaluate the type argument constraints (e.g. ArrayList cannot be cast to Charsequence).

@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

Ok, we can ignore this for now. Note though that generally a type parameter constraint could also apply to a property of the iterable type and thus be relevant for the validation done for HV-902.

@khalidq

khalidq Jul 28, 2014

Contributor

It is already supported. For example, a volition on an Iterable with constrained type argument list will be iterableExt.names[0].

@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

What I mean is the usage of a type variable on a property of a custom iterable:

public class MyList<V> implements List<V> {
    public V getDefault() { ... }
}

public class MyListUser {
    @Valid
    private MyList<@NotNull String> myList = ...;
}

When validating MyListUser, the @NotNull constraint would have to be applied to the "default" property in the course of the HV-902 validation. But it's a rather exotic use case and I agree to handle it separately.

...src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java
@@ -728,6 +748,22 @@ else if ( isIndexable ) {
}
}
+ private void validateTypeArgumentConstraints(ValidationContext<?> context, ValueContext<?, Object> valueContext, Object value, Set<TypeArgumentMetaData> typeArgumentMetaDataSet) {
+ valueContext.setCurrentValidatedValue( value );
+ valueContext.setValidatedValueHandler( null );
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

Why is the value handler set to null here? Isn't it re-set later on anyways?

@khalidq

khalidq Jul 25, 2014

Contributor

Why is the value handler set to null here? Isn't it re-set later on anyways?

I keep coming with this problem. Say the the field is Optional, so we have the optional value handler set in the ValueContext. Now when we get the wrapped value, say a String, the value handler is still set to optional, which will raise an error when trying to determining the type or getting the value.

I suggest that we update #setValidatedValueHandlerToValueContextIfPresent by doing something like:

if ( metaData.requiresUnwrapping() )
set handler
else
set null

What do you think?

@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

Yes, I guess that'd make sense.

...src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java
+ }
+ }
+ // Reset path
+ valueContext.setPropertyPath( PathImpl.createCopy( valueContext.getPropertyPath() ) );
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

Re-setting the path to the same value doesn't seem right. Does the validation of constraints on type arguments change the path at all?

@khalidq

khalidq Jul 25, 2014

Contributor

Re-setting the path to the same value doesn't seem right. Does the validation of constraints on type arguments change the path at all?

In ValidationContext#createConstraintViolation there is this line:

Path path = constraintViolationCreationContext.getPath();

Now, the constraint violation is tied with the current value context path, say names[1]. If we iterate to the next element, then the failing constraint path will also change. So failing constraints will always have the last index.

Therefore, I have to create a separate copy so that paths and indices don't get mixed.

Alternatively, I could create the path copy in the constructor of ConstraintValidatorContextImpl which might be a better.

@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

The reason may be that in validateCascadedConstraint the current value context is used for the validation of type arguments rather than newValueContext as is for the actual cascaded validation. I think there is a relation between the test issue commented below and the path handling here.

...e/validator/internal/metadata/descriptor/TypeArgumentDescriptorImpl.java
+ *
+ * @author Khalid Alqinyah
+ */
+public class TypeArgumentDescriptorImpl extends ElementDescriptorImpl
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

Ah, good to think about the BV meta-model as well. I don't think we really need it right now, though. I don't think a user could access it in any way via the BV API?

@khalidq

khalidq Jul 25, 2014

Contributor

Ah, good to think about the BV meta-model as well. I don't think we really need it right now, though.

I thought I have everything in place for BV1.2. Do you prefer to remove this?

I don't think a user could access it in any way via the BV API?

Can't the user access it through TypeArgumentMetaData#asDescriptor ?

@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

TypeArgumentMetaData is no public API, a user would never get hold of it. The entry point into the BV meta-data API is via Validator#getConstraintsForClass().

I would leave this out for now as we don't yet really now how constraints on type parameters should be represented in the meta-data. I'd definitely do it separately from the issue at hand.

...idator/internal/metadata/provider/TypeUseAnnotationMetaDataProvider.java
+ }
+
+ @Override
+ protected ConstrainedField findPropertyMetaData(Field field) {
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

This implementation looks quite similar to the one in the base class. The redundancy concerns me a bit. What do you think about the removing findPropertyMetaData() here and only defining the required hooks in the base class (where they would be implemented empty)? So you'd have the following in the base class:

protected ConstrainedField findPropertyMetaData(Field field) {
    Set<MetaConstraint<?>> constraints = convertToMetaConstraints(
            findConstraints( field, ElementType.FIELD ),
            field
    );

    Map<Class<?>, Class<?>> groupConversions = getGroupConversions(
            field.getAnnotation( ConvertGroup.class ),
            field.getAnnotation( ConvertGroup.List.class )
    );

    Set<ConstrainedTypeArgument> constrainedTypeArguments = findFieldConstrainedTypeArguments( field );
    boolean isCascading = field.isAnnotationPresent( Valid.class );
    boolean requiresUnwrapping = determineIfRequiresUnwrapping( field, !constrainedTypeArguments.isEmpty() );

    return new ConstrainedField(
            ConfigurationSource.ANNOTATION,
            ConstraintLocation.forProperty( field ),
            constraints,
            groupConversions,
            isCascading,
            requiresUnwrapping
    );
}

// overridden in the sub-class with the proper Java 8 implementation
protected Set<ConstrainedTypeArgument> findFieldConstrainedTypeArguments(Field field) {
    return Collections.emptySet();
}

// overridden in the sub-class with the proper Java 8 implementation
protected determineIfRequiresUnwrapping(...) {
    ...
}

That way you would have no redundancy at all, only those aspects which need to be different for the type annotation case can be customized by overiding the hook methods.

@hferentschik

hferentschik Jul 25, 2014

Owner

This implementation looks quite similar to the one in the base class. The redundancy concerns me a bit.

+1

@khalidq

khalidq Jul 25, 2014

Contributor

What do you think about the removing findPropertyMetaData() here and only defining the required hooks in the base class

Ok, I'll update it.

...idator/internal/metadata/provider/TypeUseAnnotationMetaDataProvider.java
+ return false;
+ }
+
+ if ( hasConstrainedTypeArguments ) {
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

Why does the presence of type parameters per-se require an unwrapping? Isn't only the UnwrapValidatedValue annotation the trigger for this (this may have changed recently and I have forgotten, I'm not sure)?

@hferentschik

hferentschik Jul 25, 2014

Owner

Provided it is not an iterable, the use of a type annotation implies that we will need to unwrap. With type annotation UnwrapValidatedValue becomes in many cases irrelevant (think Optional<@Foo String>). It is most useful pre Java 8 or in the case where the type to unwrap is not having some type argument (JavaFX types).

...idator/internal/metadata/provider/TypeUseAnnotationMetaDataProvider.java
+ );
+ metaConstraints.addAll( constraints );
+
+ Map<Class<?>, Class<?>> groupConversions = getGroupConversions(
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

How do group conversions relate to type argument constraints? Could you give an example for this?

@khalidq

khalidq Jul 25, 2014

Contributor

None really. I was inclined to set an empty map for groupConversions but I wasn't sure. Should I go ahead with that?

...idator/internal/metadata/provider/TypeUseAnnotationMetaDataProvider.java
+ * Creates meta constraints for type arguments constraints.
+ */
+ private Set<MetaConstraint<?>> convertToTypeArgumentMetaConstraints(List<ConstraintDescriptorImpl<?>> constraintDescriptors, Member member, Type type) {
+ Set<MetaConstraint<?>> constraints = newHashSet();
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

You can allocate the correct size.

@khalidq

khalidq Jul 25, 2014

Contributor

You can allocate the correct size.

Will do.

...java/org/hibernate/validator/internal/metadata/raw/ConstrainedField.java
+ * be performed or not.
+ * @param requiresUnwrapping Whether the value of the field must be unwrapped prior to validation or not
+ */
+ public ConstrainedField(ConfigurationSource source,
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

Should we really have this second constructor? I'd rather add the new argument to the existing one. After all, the info should always be given when instantiating a ConstraintField. In those cases where we don't have it yet (e.g. when parsing XML mappings), we should pass an empty set and probably add a TODO marker.

@khalidq

khalidq Jul 25, 2014

Contributor

Should we really have this second constructor? I'd rather add the new argument to the existing one

Ok. I wanted a separate constructor for AnnotationMetaDataProvider where we don't have constrained type arguments. But if you prefer, I'll make one constructor and refactor any code that reference it.

@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

But if you prefer, I'll make one constructor and refactor any code that reference it.

+1

+/**
+ * @author Gunnar Morling
+ */
+public abstract class AnnotationMetaDataProviderTestBase {
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

Nice to abstract the base class. Add yourself as author, too, you did it after all :)

pom.xml
@@ -321,7 +323,7 @@
<violationSeverity>error</violationSeverity>
<includeTestSourceDirectory>true</includeTestSourceDirectory>
<!-- These classes are imported from other sources and are not re-formatted-->
- <excludes>**/ConcurrentReferenceHashMap.java,**/TypeHelper*.java</excludes>
+ <excludes>**/ConcurrentReferenceHashMap.java,**/TypeHelper*.java, **/TypeUseAnnotationMetaDataProviderTest.java, **/TypeUseValidationTest.java</excludes>
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

I think I've asked before, but why are these tests excluded? Better add some comment on the reason.

@khalidq

khalidq Jul 25, 2014

Contributor

I think I've asked before, but why are these tests excluded? Better add some comment on the reason.

CheckStyle will fail on Java 8's type annotation syntax. I'll add a comment for TypeUseAnnotationMetaDataProviderTest and TypeUseValidationTest. Not sure about the others. The others are already explained.

@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

Ah, I remember now. Thanks for adding a comment.

...src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java
@@ -683,7 +699,7 @@ else if ( TypeHelper.isArray( type ) ) {
return isIndexable;
}
- private void validateCascadedConstraint(ValidationContext<?> context, Iterator<?> iter, boolean isIndexable, ValueContext<?, ?> valueContext, ValidationOrder validationOrder) {
+ private void validateCascadedConstraint(ValidationContext<?> context, Iterator<?> iter, boolean isIndexable, ValueContext<?, Object> valueContext, ValidationOrder validationOrder, Set<TypeArgumentMetaData> typeArgumentMetaData) {
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

Wow, that line is super-long. Can we wrap it?

@khalidq

khalidq Jul 25, 2014

Contributor

Wow, that line is super-long. Can we wrap it?

Will do.

...bernate/validator/internal/metadata/aggregated/TypeArgumentMetaData.java
+ *
+ * @author Khalid Alqinyah
+ */
+public class TypeArgumentMetaData extends AbstractConstraintMetaData implements Cascadable {
@gunnarmorling

gunnarmorling Jul 25, 2014

Owner

I'm wondering whether this actually should be a ConstraintMetaData / Cascadable. Aren't we in the end just interested in the set of constraints?

@hferentschik

hferentschik Jul 25, 2014

Owner

I have my doubts as well

@khalidq

khalidq Jul 25, 2014

Contributor

I'm wondering whether this actually should be a ConstraintMetaData / Cascadable. Aren't we in the end just interested in the set of constraints?

I thought we might need to treat the field and type arguments separately in terms of unwrapping (e.g. nested unwrapping). If you guys don't think that would be necessary, then it will be actually easier to only work with the constraints.

@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

Hum, I'm not really sure about the case you mention. I think I'd start with a simple set of MetaConstraint and see how far it gets us. We still may have to introduce a special type, but ConstraintMetaData / Cascadable don't seem to be the right abstraction to me. It doesn't really fit (see group conversions etc.).

.../java/org/hibernate/validator/internal/metadata/BeanMetaDataManager.java
- );
+ AnnotationMetaDataProvider defaultProvider = null;
+ if ( Version.getJavaRelease() >= 8 ) {
+ defaultProvider = new TypeUseAnnotationMetaDataProvider(
@hferentschik

hferentschik Jul 25, 2014

Owner

I don't think that for this approach we need to fully fledged metadata providers.

@khalidq

khalidq Jul 25, 2014

Contributor

I don't think that for this approach we need to fully fledged metadata providers.

Can you elaborate? How would we retrieve type use constraints with Java 8?

@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

Not sure what @hferentschik means either :) I think you should go for the two classes, the Java 8 one extending the other one, the first one providing the required hook methods as discussed above.

@hferentschik

hferentschik Aug 5, 2014

Owner

I think you should go for the two classes, the Java 8 one extending the other one, the first one providing the required hook methods as discussed above.

that's what I meant ;-)

Contributor

khalidq commented Jul 27, 2014

New push adds support for getters, return values, and parameters.

HV-5-MASTER #527 UNSTABLE
Looks like there's a problem with this pull request

Contributor

khalidq commented Jul 27, 2014

The failing tests are due to the same pre JDK 8u20 bug.

Owner

gunnarmorling commented Jul 28, 2014

New push adds support for getters, return values, and parameters.

Cool, thanks. Looking and going to address your remarks/questions. Btw. I think you better add new commits in this case, this makes it easier to review the incrementally added changes and also prevents single huge commits.

...src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java
@@ -567,6 +569,17 @@ private void validateCascadedConstraints(ValidationContext<?> validationContext,
Class<?> group = cascadable.convertGroup( originalGroup );
valueContext.setCurrentGroup( group );
+ Set<TypeArgumentMetaData> typeArgumentMetaData = Collections.<TypeArgumentMetaData>emptySet();
+ if ( cascadable instanceof PropertyMetaData ) {
@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

You could define another "facet interface" similar to Cascadable which provides access to the type parameter constraints instead of downcasting to all the different types.

.../validator/test/internal/engine/typeuse/model/TypeUseValidationTest.java
+ "Foo.arg1" );
+ }
+
+ static class Foo {
@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

Could you create separate classes for the different test cases? Using one and the same makes it much harder to follow the routine when debugging through the code, also it may create correlations between different test cases.

@khalidq

khalidq Jul 28, 2014

Contributor

Ok.

.../validator/test/internal/engine/typeuse/model/TypeUseValidationTest.java
+ foo.names = Arrays.asList( "First", "", null );
+ Set<ConstraintViolation<Foo>> constraintViolations = validator.validate( foo );
+ assertNumberOfViolations( constraintViolations, 3 );
+ assertCorrectPropertyPaths( constraintViolations, "names[1]", "names[2]", "names[2]" );
@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

I must be blind, but why are two violations expected for names[2]? I think it'd help if you also added assertCorrectConstraintTypes().

@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

Something seems odd, apparently the @NotBlank constraint is reported for names[2] but I don't think it should as it the value is null which is a valid value for @NotBlank. It may be related to not correctly isolating the value contexts between validation of several elements in the cascaded list.

@khalidq

khalidq Jul 28, 2014

Contributor

Actually this is expected. @NotBlank has a nested @NotNull, so names[2] will have two violations:

1- Nested @NotNull in @NotBlankTypeUse.
2- @NotNullTypeUse.

My intention is to test the correct behavior of two type arguments constraints. I'll add assertCorrectConstraintTypes() to make it clear.

@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

Ah, I see where you're coming from. I guess the usage of @NotBlankTypeUse and @NotNullTypeUse at the same time is kind of confusing (since the former already comprises the null check as you say).

.../hibernate/validator/internal/metadata/aggregated/ParameterMetaData.java
if ( name == null ) {
name = constrainedParameter.getName();
}
}
+ private Set<TypeArgumentMetaData> buildTypeArgumentMetaData() {
@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

Seeing the builder here, have we considered actually what should happen in case like this:

public class Parent {
    public List<@NotNull String> getNames() { ... }
}

class Child extends Parent {

   @Override
    public List<@NotEmpty String> getNames() { ... }
}

On first thought I'd say the same rules should apply as for constraints on return values / parameters in inheritance hierarchies outlined in the BV spec. I.e. constraints on return values add up, whereas no re-finement should be allowed for constraints on method parameters. But we should put some more thought into it, probably using a separate HV issue.

...idator/internal/metadata/provider/TypeUseAnnotationMetaDataProvider.java
+ requiresUnwrapping
+ );
+ }
+ return null;
@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

Returning null, is that right?

@khalidq

khalidq Jul 28, 2014

Contributor

This is intentional, the code returns ConstrainedTypeArgument only if the type argument has constraints, otherwise it will return a null, and the caller performs a check before adding to the set of constrained type arguments.

Anyway, this is not relevant anymore, since we're moving away from constrained elements to meta constrains to represent constraints on type arguments.

...g/hibernate/validator/internal/metadata/raw/ConstrainedTypeArgument.java
+ *
+ * @author Khalid Alqinyah
+ */
+public class ConstrainedTypeArgument extends AbstractConstrainedElement {
@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

I'm wondering the same as for TypeArgumentMetaData, should this actually extend AbstractConstrainedElement? It seems not a right match, e.g. group conversions don't really fit.

.../hibernate/validator/test/internal/util/constraints/NotBlankTypeUse.java
+ * @author Khalid Alqinyah
+ */
+@Documented
+@Constraint(validatedBy = { NotBlankTypeUseValidator.class })
@gunnarmorling

gunnarmorling Jul 28, 2014

Owner

It just crossed my mind that you actually don't need a validator for this. Just meta-annotate each Java 8 constraints with its existing counter-part. This should make it even simpler to add some more for testing purposes.

@khalidq

khalidq Jul 28, 2014

Contributor

Yeah, much better. I updated the constraints.

Contributor

khalidq commented Jul 28, 2014

New updates based on the feedback. For facets, I thought I use Cascadable instead of creating a new one, since Cascadable matches our needs perfectly.

HV-5-MASTER #528 UNSTABLE
Looks like there's a problem with this pull request

...idator/internal/metadata/provider/TypeUseAnnotationMetaDataProvider.java
+ *
+ * @author Khalid Alqinyah
+ */
+public class TypeUseAnnotationMetaDataProvider extends AnnotationMetaDataProvider {
@gunnarmorling

gunnarmorling Jul 30, 2014

Owner

Nice, I think this looks much better now :)

@hferentschik

hferentschik Aug 5, 2014

Owner

+1, no more duplication. Regarding the name TypeUseAnnotationMetaDataProvider implies to me somehow that this is "just" an annotation provider for type use. Maybe something like 'AnnotationMetaDataProviderWithTypeUse` is better. I admit though this is a mouth full. WDYT?

@khalidq

khalidq Aug 6, 2014

Contributor

Regarding the name TypeUseAnnotationMetaDataProvider implies to me somehow that this is "just" an annotation provider for type use. Maybe something like 'AnnotationMetaDataProviderWithTypeUse` is better. I admit though this is a mouth full. WDYT?

I think the documentation and code clearly shows that TypeUseAnnotationMetaDataProvider extends a base AnnotationMetaDataProvider which is sufficient. But I have no strong opinion here, I can go with whatever you prefer.

@gunnarmorling

gunnarmorling Aug 6, 2014

Owner

I'm also fine with the current name, but
"TypeAnnotationAwareMetaDataProvider" or
"TypeAnnotationAwareAnnotationMetaDataProvider" may help to clarify that
it's not "just" about type annotations.

2014-08-06 7:09 GMT+02:00 Khalid notifications@github.com:

In
engine/src/main/java/org/hibernate/validator/internal/metadata/provider/TypeUseAnnotationMetaDataProvider.java:

+import org.hibernate.validator.internal.metadata.core.MetaConstraint;
+import org.hibernate.validator.internal.metadata.descriptor.ConstraintDescriptorImpl;
+import org.hibernate.validator.internal.metadata.location.ConstraintLocation;
+import org.hibernate.validator.internal.util.ReflectionHelper;
+import org.hibernate.validator.internal.util.TypeHelper;
+
+import static org.hibernate.validator.internal.util.CollectionHelper.newArrayList;
+import static org.hibernate.validator.internal.util.CollectionHelper.newHashSet;
+
+/**

  • * Extends {@code AnnotationMetaDataProvider} by discovering and registering type use constraints defined on type
  • * arguments.
  • * @author Khalid Alqinyah
  • */
    +public class TypeUseAnnotationMetaDataProvider extends AnnotationMetaDataProvider {

Regarding the name TypeUseAnnotationMetaDataProvider implies to me
somehow that this is "just" an annotation provider for type use. Maybe
something like 'AnnotationMetaDataProviderWithTypeUse` is better. I admit
though this is a mouth full. WDYT?

I think the documentation and code clearly shows that
TypeUseAnnotationMetaDataProvider extends a base
AnnotationMetaDataProvider which is sufficient. But I have no strong
opinion here, I can go with whatever you prefer.


Reply to this email directly or view it on GitHub
https://github.com/hibernate/hibernate-validator/pull/332/files#r15856298
.

@hferentschik

hferentschik Aug 6, 2014

Owner

I think the documentation and code clearly shows that TypeUseAnnotationMetaDataProvider extends a base AnnotationMetaDataProvider which is sufficient.

All this requires that you actually open the class and look at the docs and what it extends. Just reading the name it can be ambiguous. I would always try to strive for less ambiguous names. I likes TypeAnnotationAwareMetaDataProvider better.

...idator/internal/metadata/provider/TypeUseAnnotationMetaDataProvider.java
+ return findTypeArgumentsConstraints( member, parameter.getAnnotatedType() );
+ }
+ catch ( ArrayIndexOutOfBoundsException ex ) {
+ // Synthetic parameter, ignore
@gunnarmorling

gunnarmorling Jul 30, 2014

Owner

Can we avoid running into this in the first place? There is Parameter#isSynthetic() which may help.

@khalidq

khalidq Jul 30, 2014

Contributor

It didn't help, I guess the problem is not about a synthetic parameter.

The reason I have this try-catch block is because ClassValidatorWithTypeVariableTest keeps failing. I mistakenly thought it has something to do with synthetic parameters, but apparently not.

Can you take a look? Is something wrong with the test?

@gunnarmorling

gunnarmorling Aug 1, 2014

Owner

Interesting; I just ran the test without the try/catch and it passed. Which error exactly do you get?

@khalidq

khalidq Aug 1, 2014

Contributor

You need to run the test in JDK 1.8u20. You'll get a java.lang.ArrayIndexOutOfBoundsException.

Maybe it's a JDK problem since the test runs successfully in JDK 8u5 and 8u11.

@khalidq

khalidq Aug 1, 2014

Contributor

I'm downloading a new build of 1.8u20. I'll see how it goes.

@gunnarmorling

gunnarmorling Aug 1, 2014

Owner

Hum, it's also passing with build 1.8.0_20-ea-b17. Which exact version do
you use?

2014-08-01 9:11 GMT+02:00 Khalid notifications@github.com:

In
engine/src/main/java/org/hibernate/validator/internal/metadata/provider/TypeUseAnnotationMetaDataProvider.java:

  •   if ( member instanceof Method ) {
    
  •       annotatedType = ( (Method) member ).getAnnotatedReturnType();
    
  •   }
    
  •   return findTypeArgumentsConstraints( member, annotatedType );
    
  • }
  • @override
  • protected Set<MetaConstraint<?>> findTypeArgumentsConstraints(Member member, int i) {
  •   Parameter parameter = ( (Executable) member ).getParameters()[i];
    
  •   try {
    
  •       return findTypeArgumentsConstraints( member, parameter.getAnnotatedType() );
    
  •   }
    
  •   catch ( ArrayIndexOutOfBoundsException ex ) {
    
  •       // Synthetic parameter, ignore
    

You need to run the test in JDK 1.8u20. You'll get a
java.lang.ArrayIndexOutOfBoundsException.

Maybe it's a JDK problem since the test runs successfully in JDK 8u5 and
8u11.


Reply to this email directly or view it on GitHub
https://github.com/hibernate/hibernate-validator/pull/332/files#r15684463
.

@khalidq

khalidq Aug 1, 2014

Contributor

I'm using 1.8.0_20-ea-b21. In a few minutes I'll try the latest one which is 1.8.0_20-ea-b23

@khalidq

khalidq Aug 1, 2014

Contributor

@gunnarmorling, b23 didn't help.

@gunnarmorling

gunnarmorling Aug 1, 2014

Owner

Ok, I had a closer look now.

I believe it's indeed an issue with the JDK itself,
Parameter#getAnnotatedType() fails when being invoked for methods with a
synthetic parameter (for non-static inner classes, the compiler inserts a
synthetic parameter at the first position through which an instance of the
outer class is passed). If the parameter(s) of such a constructor contain
generic types, Executable#getGenericParameterTypes() (which is invoked
underneath) doesn't return all the parameters, thus an AIOOBE is raised
when trying to access the last parameter.

There is bug JDK-5087240
which I think is the root cause.

There is not much we can do. I think we should log a warning in the catch
clause, because we won't properly validate the concerned parameter.

As a workaround, you could make the inner classes in the affected test
static (so no synthetic parameter is introduced) or you make the parameter
not a generic type.

2014-08-01 9:55 GMT+02:00 Khalid notifications@github.com:

In
engine/src/main/java/org/hibernate/validator/internal/metadata/provider/TypeUseAnnotationMetaDataProvider.java:

  •   if ( member instanceof Method ) {
    
  •       annotatedType = ( (Method) member ).getAnnotatedReturnType();
    
  •   }
    
  •   return findTypeArgumentsConstraints( member, annotatedType );
    
  • }
  • @override
  • protected Set<MetaConstraint<?>> findTypeArgumentsConstraints(Member member, int i) {
  •   Parameter parameter = ( (Executable) member ).getParameters()[i];
    
  •   try {
    
  •       return findTypeArgumentsConstraints( member, parameter.getAnnotatedType() );
    
  •   }
    
  •   catch ( ArrayIndexOutOfBoundsException ex ) {
    
  •       // Synthetic parameter, ignore
    

@gunnarmorling https://github.com/gunnarmorling, b23 didn't help.


Reply to this email directly or view it on GitHub
https://github.com/hibernate/hibernate-validator/pull/332/files#r15685449
.

@khalidq

khalidq Aug 1, 2014

Contributor

So are in favor of keeping the try-catch block, or adjusting the test and removing the try-catch block?

+ /**
+ * Type arguments constraints for this property
+ */
+ private final Set<MetaConstraint<?>> typeArgumentsConstraints;
@gunnarmorling

gunnarmorling Jul 30, 2014

Owner

So to be sure, this works for single type arguments only, right? For map-typed properties the parameters on the V type are represented, I assume?

@khalidq

khalidq Jul 30, 2014

Contributor

Ah, that's a good point. I didn't consider the instance where a user would annotate more than one type argument. I'll update the TypeUseAnnotationMetaDataProvider to only consider the last type argument (the value in the Map).

@hferentschik

hferentschik Aug 5, 2014

Owner

I'll update the TypeUseAnnotationMetaDataProvider to only consider the last type argument

Did you do that? Where? Do we have a test for Map?

@khalidq

khalidq Aug 6, 2014

Contributor

Did you do that? Where? Do we have a test for Map?

See TypeUseAnnotationMetaDataProvider#getAnnotatedActualTypeArguments.

And you've already found the test.

+ * @return the type arguments constraints for this cascadable, or an empty set if no constrained type arguments are
+ * found
+ */
+ Set<MetaConstraint<?>> getTypeArgumentsConstraints();
@gunnarmorling

gunnarmorling Jul 30, 2014

Owner

I guess at some later stage we need to change this to return a map of constraints, keyed by the type argument they are declared on.

@hferentschik

hferentschik Aug 5, 2014

Owner

Would a Map do? Is this not a positional thing? You can have the same type arguments used multiple times, right?

@gunnarmorling

gunnarmorling Aug 6, 2014

Owner

WDYM by "positional thing" exactly?

Yes, the same type arguments can be used (in its declaring type) multiple times, but there'd be only one set of constraints defined per type argument on the usage site. So it probably would have to be a Map<TypeParameter, Set<MetaConstraint<?>>. Then when validating a property or method parameter etc. which is of a generic type, we'd apply the constraints stored in the map for that type parameter in addition to any constraints declared on the element itself.

Contributor

khalidq commented Jul 30, 2014

New push for selecting only one type argument.

Should I squash these commits together? I think the project's history will be cleaner if we combine them before merging.

HV-5-MASTER #529 UNSTABLE
Looks like there's a problem with this pull request

...lidator/internal/cfg/context/ExecutableConstraintMappingContextImpl.java
@@ -130,12 +130,14 @@ public ExecutableElement getExecutable() {
}
public ConstrainedElement build(ConstraintHelper constraintHelper, ParameterNameProvider parameterNameProvider) {
+ // TODO: Support constrained type arguments
@gunnarmorling

gunnarmorling Aug 1, 2014

Owner

Could you add a reference to https://hibernate.atlassian.net/browse/HV-919, also at the other places (I've just created this as follow-up). Thanks!

@khalidq

khalidq Aug 1, 2014

Contributor

Ok.

...ate/validator/internal/metadata/provider/AnnotationMetaDataProvider.java
@@ -455,6 +473,15 @@ else if ( parameterAnnotation.annotationType().equals( UnwrapValidatedValue.clas
}
}
+ // Make the parameter cascaded if it has constrained type arguments
+ if ( !parameterIsCascading ) {
+ parameterIsCascading = !typeArgumentsConstraints.isEmpty();
@gunnarmorling

gunnarmorling Aug 1, 2014

Owner

So for e.g. List<@Email String> one wouldn't have to add @Valid explicitly? What's the motivation for this? This seems a bit different from the existent behavior.

@khalidq

khalidq Aug 1, 2014

Contributor

I'm just following @hferentschik suggestion of processing type arguments during cascaded validation routine. So, if the parameter has type arguments constraints, but is not marked as cascading, we mark it as cascaded to allow #validateCascadedConstraints to process the type arguments constraints.

This is done for fields, executables, and parameters.

@gunnarmorling

gunnarmorling Aug 1, 2014

Owner

Hum, but what is the motivation for this "auto-cascading"? Do you have a pointer to the place where Hardy suggested it? Just trying to make sense out of it. Maybe I'm missing a piece :)

@khalidq

khalidq Aug 1, 2014

Contributor

The auto-cascading is a necessity. If we have something like List<@NotBlank String> names, the field must be cascadable in order to be able to process it.

Here is some discussion between me and @hferentschik:

#326 (comment)

@hferentschik

hferentschik Aug 5, 2014

Owner

As a reference:

I don't think that if using List<@email String> emailList one should need to apply @Valid explicitly. However, that might be open for discussion.

I still feel it is a bit odd to specify @Valid explicitly. As I tried to explain to @gunnarmorling, I don't necessarily see a type annotation constraint on an Iterable as cascaded validation. For me the cascading occurs once I validate the constraints defined within the iterated element. The iteration itself is not "cascading" (not sure whether this makes sense the way I say it).

IMO there is a distinction between iterating the elements and cascading validation. If type annotations would have been possible in earlier BV spec versions, the use of @Valid in conjunction with Iterables would probably have been List<@Valid Foo> which imo expresses the intend much better (btw the latter use of @Valid is something we have not yet considered).

That all said, I am happy to not do the "auto-cascading" for now. First off, @gunnarmorling seems to be the opinion that it is the right thing anyways and secondly our code does not make this distinction between iteration and cascading. In fact we handle type annotation constraints as part of Cascadable. For this reason, I think we might be better off, with requiring an explicit @Valid.

@khalidq

khalidq Aug 6, 2014

Contributor

For this reason, I think we might be better off, with requiring an explicit @Valid.

So you suggest that we require the user to explicitly add a @Valid annotation when using type use constraints. If a @Valid annotation is not present, the type use constraint is ignored. Is that correct?

@gunnarmorling

gunnarmorling Aug 6, 2014

Owner

That all said, I am happy to not do the "auto-cascading" for now. First off, @gunnarmorling seems to be the opinion that it is the right thing anyways...

Thanks, +1 for going this route at least for the time being. We can and should re-discuss the options we have, also let's see what @emmanuelbernard has to say about it.

@hferentschik

hferentschik Aug 6, 2014

Owner

So you suggest that we require the user to explicitly add a @Valid annotation when using type use constraints.

+1, it seems more aligned with our code. Personally, I still feel there is a distinction to be made. But as @gunnarmorling is saying we will take this discussion later on.

.../validator/test/internal/engine/typeuse/model/TypeUseValidationTest.java
+ class Baz<T> {
+
+ }
+}
@gunnarmorling

gunnarmorling Aug 1, 2014

Owner

New line is missing. Hum, I thought CheckStyle should warn about this?

@khalidq

khalidq Aug 1, 2014

Contributor

That's because TypeUseValidationTest is excluded in the pom.xml file. I added the new line.

@gunnarmorling

gunnarmorling Aug 1, 2014

Owner

Ah, got it. Makes sense.

Owner

gunnarmorling commented Aug 1, 2014

@khalidq, two of the tests in TypeUseValidationTest are still failing for me, despite using 1.8.0_20 (specifically build 1.8.0_20-ea-b17). Do I need yet a newer version?

Contributor

khalidq commented Aug 1, 2014

@khalidq, two of the tests in TypeUseValidationTest are still failing for me, despite using 1.8.0_20 (specifically build 1.8.0_20-ea-b17). Do I need yet a newer version?

I'm using 1.8.0_20-ea-b21

Contributor

khalidq commented Aug 2, 2014

New update with HV-919 references. I squashed the commits together.

For the try-catch block, I decided to remove it as I don't think we should catch and log JDK bugs.

HV-5-MASTER #531 UNSTABLE
Looks like there's a problem with this pull request

Owner

hferentschik commented Aug 5, 2014

Where are we with this? Is this ready to be merged? I will try to have another look as well. Wondering though, whether it would make sense to close this pull request and open a new one?

Owner

gunnarmorling commented Aug 5, 2014

For the try-catch block, I decided to remove it as I don't think we should catch and log JDK bugs.

I've thought about it and I believe it should stay in place. We basically have the choice between not supporting this specific case (and logging a warning) but continue to work for others, or generally fail validation by letting the AIOOBE bubble up. I think we should go for the former. Btw. I've filed a bug with the JDK project for this.

Owner

gunnarmorling commented Aug 5, 2014

Wondering though, whether it would make sense to close this pull request and open a new one?

No need to close it IMO. I think it's good to go, apart from what you may notice and the try/catch issue discussed above.

Contributor

khalidq commented Aug 5, 2014

I've thought about it and I believe it should stay in place. We basically have the choice between not supporting this specific case (and logging a warning) but continue to work for others, or generally fail validation by letting the AIOOBE bubble up. I think we should go for the former

Ok, I'll put the try-catch block again. Any thoughts on what the warning log message should say?

Owner

gunnarmorling commented Aug 5, 2014

Something like "Constraints on the parameters of constructors of non-static
inner classes are not supported if those parameters have a generic type due
to JDK bug JDK-5087240."

2014-08-05 11:12 GMT+02:00 Khalid notifications@github.com:

I've thought about it and I believe it should stay in place. We basically
have the choice between not supporting this specific case (and logging a
warning) but continue to work for others, or generally fail validation by
letting the AIOOBE bubble up. I think we should go for the former

Ok, I'll put the try-catch block again. Any thoughts on what the warning
log message should say?


Reply to this email directly or view it on GitHub
#332 (comment)
.

Contributor

khalidq commented Aug 5, 2014

New update with the try-catch.

HV-5-MASTER #533 UNSTABLE
Looks like there's a problem with this pull request

Owner

gunnarmorling commented Aug 5, 2014

@khalidq, one more thing, could you configure the Enforcer plug-in to require JDK 1.8 Update 20 at least:

...
<version>[1.8.0-20,)</version>

That way we'll get a meaningful error when trying to build on any older version. Thx!

Contributor

khalidq commented Aug 5, 2014

New update with <version>[1.8.0-20,)</version>.

HV-5-MASTER #534 UNSTABLE
Looks like there's a problem with this pull request

.../validator/test/internal/engine/typeuse/model/TypeUseValidationTest.java
+ }
+
+ static class F {
+ Map<String, @NotBlankTypeUse String> namesMap;
@hferentschik

hferentschik Aug 5, 2014

Owner

Ahh, I guess here is the Map based test. So, how do you make sure that only the value type constraint is considered?

Owner

hferentschik commented Aug 5, 2014

I think we are almost there ;-)

Owner

hferentschik commented Aug 5, 2014

@khalidq, any ideas why the ci build is failing? Is this due to the used JDK? It uses 1.8.0_20-ea. Is that because we even require a specific sub build? I am using 1.8.0_20-ea-b23 which seems to work.

Contributor

khalidq commented Aug 6, 2014

@khalidq, any ideas why the ci build is failing? Is this due to the used JDK? It uses 1.8.0_20-ea. Is that because we even require a specific sub build? I am using 1.8.0_20-ea-b23 which seems to work.

According to the bug report, it has been fixed in b17.

https://bugs.openjdk.java.net/browse/JDK-8039916

Owner

gunnarmorling commented Aug 6, 2014

It's using 1.8.0_20-ea-b05, in which the bug still exists. @hferentschik,
the pre-installed JDKs on CloudBees don't seem to include any newer
version. Can we install the latest build from hand?

.../validator/test/internal/engine/typeuse/model/TypeUseValidationTest.java
+ assertCorrectConstraintTypes( constraintViolations, NotBlankTypeUse.class );
+ }
+
+ @Test( expectedExceptions = ValidationException.class )
@hferentschik

hferentschik Aug 6, 2014

Owner

We should verify that the right exception is returned here.

@khalidq

khalidq Aug 6, 2014

Contributor

We should verify that the right exception is returned here.

The test will fail if ValidationException is not thrown. Unless you have something else in mind for "the right exception".

@hferentschik

hferentschik Aug 6, 2014

Owner

There could be many reasons for a ValidationException. In this case there is no registered un-wrapper. I think that is HV000182. The test should verify the error message and that the right error key is returned.

@khalidq

khalidq Aug 6, 2014

Contributor

Ok.

...idator/internal/metadata/provider/TypeUseAnnotationMetaDataProvider.java
+
+ AnnotatedType[] annotatedArguments = ( (AnnotatedParameterizedType) annotatedType ).getAnnotatedActualTypeArguments();
+ if ( annotatedArguments.length > 0 ) {
+ return Optional.of( annotatedArguments[ annotatedArguments.length - 1 ] );
@hferentschik

hferentschik Aug 6, 2014

Owner

I find it dodgy to just return the last one. I think we really should differentiate here and check the annotatedType. We can differentiate between Itertable, Map and everything else. In the latter case we pretty much support only the case where we have a single annotated type, right? We could log an info message in case there are multiple and ignore any constraints.

@khalidq

khalidq Aug 6, 2014

Contributor

I find it dodgy to just return the last one. I think we really should differentiate here and check the annotatedType. We can differentiate between Itertable, Map and everything else

So, something like:

  • Itertable: take the only one.
  • Map: If constraints are specified on both key and value, take the value constraints, ignore the key constraints.
  • Other: If more than one type argument is constrained, ignore all and log a warning. How about we throw an exception here?
@hferentschik

hferentschik Aug 6, 2014

Owner

Sounds right. Exception or log message, either works for me now.

Owner

hferentschik commented Aug 6, 2014

It's using 1.8.0_20-ea-b05, in which the bug still exists.

Interesting. I see what we can do to update the JDK. @gunnarmorling, btw where did you find the build version. It does not seem to be in the log? If we cannot get an updated JDK by the time we merge, I'd suggest we disable the test until a new JDK is available.

Owner

gunnarmorling commented Aug 6, 2014

I see what we can do to update the JDK.

Cool, thanks.

@gunnarmorling https://github.com/gunnarmorling, btw where did you find
the build version.

I set up a FreeStyle job which just ran "java -version" via a shell script
build step :) We may add this as "pre-build step" to the normal job
actually.

Owner

hferentschik commented Aug 6, 2014

I set up a FreeStyle job which just ran "java -version" via a shell script build step :)

I thought so :-)

We may add this as "pre-build step" to the normal job actually.

I was close to doing this a while back, but then added the Maven option '-V' instead which also prints version information w/o stopping the build. Unfortunately, it does not seem to report the version build version ;-)

Contributor

khalidq commented Aug 6, 2014

If we cannot get an updated JDK by the time we merge, I'd suggest we disable the test until a new JDK is available

Please let me know once you tried it. If you cannot update the JDK, I'll disable the test on the next update.

Owner

hferentschik commented Aug 6, 2014

Please let me know once you tried it. If you cannot update the JDK, I'll disable the test on the next update.

It should work now. Master is now building with the latest OpenJDK build. Hopefully your next pull request build will work then.

Contributor

khalidq commented Aug 6, 2014

New update:

  • Require explicit @Valid.
  • Change to TypeAnnotationAwareMetaDataProvider.
  • Confirm exception code.
  • Behavior changes with more than one type argument.

HV-5-MASTER #541 SUCCESS
This pull request looks good

Owner

hferentschik commented Aug 6, 2014

Thanks @khalidq, looking good now. The last thing I noticed is that for the logging messages in TypeAnnotationAwareMetaDataProvider you are not going via the Log interface. I'll fix this myself and merge...

Owner

hferentschik commented Aug 6, 2014

Merged. Good work. I think this solution is good enough for our initial goals of Java 8 support. We can follow up on @gunnarmorling ideas later on.

Contributor

khalidq commented Aug 6, 2014

@hferentschik, there are some extra class files. It wasn't there with my original PR, I guess it came from your end when merging.

I have always had those extra class files (IDEA), but I just clean them after each build. Maybe you merged them by mistake.

I guess we should remove them.

Owner

gunnarmorling commented Aug 7, 2014

Merged. Good work. I think this solution is good enough for our initial
goals of Java 8 support.

+1 Well done, Khalid!

Owner

gunnarmorling commented Aug 7, 2014

Yes, they shouldn't be there. I've just removed them. There is already
https://hibernate.atlassian.net/browse/HV-907 to fix the issue. I think the
processor tests (which programmatically compile code) are writing their
class files into the source tree for some reason. Not exactly sure why, it
used to be not like this until recently.

Owner

hferentschik commented Aug 7, 2014

I have always had those extra class files (IDEA), but I just clean them after each build.

That's the thing. They should not be there and this only happens since we switched to Java 8. We should not have to clean them up. One alternative is to add *.class to the .gitignore list, but I'd prefer to find out what the problem is. Why just the model classes in this package. There are other test classes in the annotation processor module which don't "appear" magically.

Anyways, you seem to think that they are generate by Idea? You never seen these files created by the command line build?

Maybe you merged them by mistake.

That's exactly what happened and it was just a matter of time :-(

Yes, they shouldn't be there. I've just removed them.

Thanks @gunnarmorling

Contributor

khalidq commented Aug 7, 2014

That's the thing. They should not be there and this only happens since we switched to Java 8

Ah, I didn't know that. I thought they've always been there. Ok, I'll investigate the issue.

Anyways, you seem to think that they are generate by Idea? You never seen these files created by the command line build?

I don't know if they're generated by IDEA, I was just following up on your thought on HV-907 "not sure whether this is created by the IDE though".

Owner

gunnarmorling commented Aug 7, 2014

Ok, I'll investigate the issue.

Cool, thx! The issue is kind of annoying :)

I don't know if they're generated by IDEA...

No, it's not IDEA (only). I also get those files when building on the
command line.

Owner

hferentschik commented Aug 7, 2014

No, it's not IDEA (only). I also get those files when building on the command line.

+1 Just did another clean build with Idea closed and the files appeared.

@khalidq khalidq deleted the khalidq:HV-877 branch Aug 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment