Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HV-1494 hibernate validator error when @NotEmpty used on return type #862

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import javax.validation.Constraint;
import javax.validation.ConstraintTarget;
Expand Down Expand Up @@ -223,7 +224,8 @@ public ConstraintDescriptorImpl(ConstraintHelper constraintHelper,
type,
!genericValidatorDescriptors.isEmpty(),
!crossParameterValidatorDescriptors.isEmpty(),
externalConstraintType
externalConstraintType,
constraintHelper
);
this.composingConstraints = parseComposingConstraints( member, constraintHelper, constraintType );
this.compositionType = parseCompositionType( constraintHelper );
Expand Down Expand Up @@ -397,13 +399,15 @@ public String toString() {
* specify the target explicitly).</li>
* </ul>
*
* @param constraintAnnotationType The type representing constraint annotation
* @param member The annotated member
* @param elementType The type of the annotated element
* @param hasGenericValidators Whether the constraint has at least one generic validator or
* not
* @param hasCrossParameterValidator Whether the constraint has a cross-parameter validator
* @param externalConstraintType constraint type as derived from external context, e.g. for
* constraints declared in XML via {@code &lt;return-value/gt;}
* @param constraintHelper An instance of {@link ConstraintHelper}
*
* @return The type of this constraint
*/
Expand All @@ -412,7 +416,8 @@ private ConstraintType determineConstraintType(Class<? extends Annotation> const
ElementType elementType,
boolean hasGenericValidators,
boolean hasCrossParameterValidator,
ConstraintType externalConstraintType) {
ConstraintType externalConstraintType,
ConstraintHelper constraintHelper) {
ConstraintTarget constraintTarget = validationAppliesTo;
ConstraintType constraintType = null;
boolean isExecutable = isExecutable( elementType );
Expand Down Expand Up @@ -474,9 +479,31 @@ else if ( hasParameters && !hasReturnValue ) {
}
}

// Now we are out of luck
if ( constraintType == null ) {
throw LOG.getImplicitConstraintTargetInAmbiguousConfigurationException( annotationType );
// check if it's a composite constraint and is built with constraints for which a type can be detected
Set<ConstraintType> constraintTypes = findComposingConstraints( constraintHelper )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already call parseComposingConstraints(); could we somehow combine this, instead of running the annotation tree another time?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll see what can be done about it and open another PR if something good comes out.

.map( element -> determineConstraintType(
element.annotationType(),
member,
elementType,
!constraintHelper.findValidatorDescriptors(
element.annotationType(),
ValidationTarget.ANNOTATED_ELEMENT
).isEmpty(),
!constraintHelper.findValidatorDescriptors(
element.annotationType(),
ValidationTarget.PARAMETERS
).isEmpty(),
externalConstraintType,
constraintHelper
) ).collect( Collectors.toSet() );
if ( constraintTypes.size() == 1 ) {
constraintType = constraintTypes.iterator().next();
}
else {
// Now we are out of luck
throw LOG.getImplicitConstraintTargetInAmbiguousConfigurationException( annotationType );
}
}

if ( constraintType == ConstraintType.CROSS_PARAMETER ) {
Expand All @@ -486,6 +513,22 @@ else if ( hasParameters && !hasReturnValue ) {
return constraintType;
}

private Stream<Annotation> findComposingConstraints(ConstraintHelper constraintHelper) {
return Arrays.stream( annotationType.getDeclaredAnnotations() )
.filter( element -> !NON_COMPOSING_CONSTRAINT_ANNOTATIONS.contains( element.annotationType().getName() ) )
.flatMap( element -> {
if ( constraintHelper.isConstraintAnnotation( element.annotationType() ) ) {
return Stream.of( element );
}
else if ( constraintHelper.isMultiValueConstraint( element.annotationType() ) ) {
return constraintHelper.getConstraintsFromMultiValueConstraint( element ).stream();
}
else {
return Stream.empty();
}
} );
}

private static ValidateUnwrappedValue determineValueUnwrapping(Set<Class<? extends Payload>> payloads, Member member, Class<? extends Annotation> annotationType) {
if ( payloads.contains( Unwrapping.Unwrap.class ) ) {
if ( payloads.contains( Unwrapping.Skip.class ) ) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
/*
* Hibernate Validator, declare and validate application constraints
*
* License: Apache License, Version 2.0
* See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>.
*/
package org.hibernate.validator.test.internal.engine.methodvalidation;

import static java.lang.annotation.ElementType.ANNOTATION_TYPE;
import static java.lang.annotation.ElementType.CONSTRUCTOR;
import static java.lang.annotation.ElementType.FIELD;
import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.ElementType.PARAMETER;
import static java.lang.annotation.ElementType.TYPE_USE;
import static java.lang.annotation.RetentionPolicy.RUNTIME;
import static org.hibernate.validator.testutil.ConstraintViolationAssert.assertNoViolations;
import static org.hibernate.validator.testutil.ConstraintViolationAssert.assertThat;
import static org.hibernate.validator.testutil.ConstraintViolationAssert.pathWith;
import static org.hibernate.validator.testutil.ConstraintViolationAssert.violationOf;

import java.lang.annotation.Documented;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;
import java.util.Set;

import javax.validation.Constraint;
import javax.validation.ConstraintViolation;
import javax.validation.Payload;
import javax.validation.ReportAsSingleViolation;
import javax.validation.Validator;
import javax.validation.constraints.NotNull;
import javax.validation.constraints.Pattern;
import javax.validation.constraints.Size;

import org.hibernate.validator.constraints.NotEmpty;
import org.hibernate.validator.testutil.TestForIssue;
import org.hibernate.validator.testutils.ValidatorUtil;

import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;

/**
* @author Marko Bekhta
*/
@SuppressWarnings("deprecation")
public class CustomCompositeConstrainedTest {
private Validator validator;
private Foo foo;

@BeforeMethod
public void setUp() throws Exception {
validator = ValidatorUtil.getValidator();
foo = new Foo( "" );
}

@Test
@TestForIssue( jiraKey = "HV-1494")
public void testNotEmptyInvalid() throws Exception {
Set<ConstraintViolation<Foo>> violations = validator.forExecutables()
.validateReturnValue(
foo,
Foo.class.getDeclaredMethod( "createBarString", String.class ),
""
);
assertThat( violations ).containsOnlyViolations(
violationOf( NotEmpty.class )
);

violations = validator.forExecutables()
.validateReturnValue(
foo,
Foo.class.getDeclaredMethod( "createBarString", String.class ),
" "
);
assertNoViolations( violations );
}

@Test
@TestForIssue( jiraKey = "HV-1494")
public void testCustomComposingConstraintReturnValues() throws Exception {
Set<ConstraintViolation<Foo>> violations = validator.forExecutables()
.validateReturnValue(
foo,
Foo.class.getDeclaredMethod( "createCustomBarString", String.class ),
"a"
);
assertThat( violations ).containsOnlyViolations(
violationOf( CustomCompositeConstraint.class )
.withPropertyPath( pathWith()
.method( "createCustomBarString" )
.returnValue()
)
);

violations = validator.forExecutables()
.validateReturnValue(
foo,
Foo.class.getDeclaredMethod( "createCustomBarString", String.class ),
"1"
);
assertNoViolations( violations );
}

@Test
@TestForIssue( jiraKey = "HV-1494")
public void testCustomComposingConstraintParameters() throws Exception {
Set<ConstraintViolation<Foo>> violations = validator.forExecutables()
.validateParameters(
foo,
Foo.class.getDeclaredMethod( "createCustomBarString", String.class ),
new String[] { "abc" }
);
assertThat( violations ).containsOnlyViolations(
violationOf( CustomCompositeConstraint.class )
.withPropertyPath( pathWith()
.method( "createCustomBarString" )
.parameter( "a", 0 )
)
);

violations = validator.forExecutables()
.validateParameters(
foo,
Foo.class.getDeclaredMethod( "createCustomBarString", String.class ),
new String[] { "1" }
);
assertNoViolations( violations );
}

private static class Foo {

private String bar;

public Foo(String bar) {
this.bar = bar;
}

@NotEmpty
public String createBarString(String a) {
return bar;
}

@CustomCompositeConstraint
public String createCustomBarString(@CustomCompositeConstraint String a) {
return bar;
}
}


@Documented
@Constraint(validatedBy = { })
@Target({ METHOD, FIELD, ANNOTATION_TYPE, CONSTRUCTOR, PARAMETER, TYPE_USE })
@Retention(RUNTIME)
@ReportAsSingleViolation
@NotNull
@Size(min = 1)
@Pattern(regexp = "\\d*")
public @interface CustomCompositeConstraint {
String message() default "no message";

Class<?>[] groups() default { };

Class<? extends Payload>[] payload() default { };

}

}