Skip to content

Commit

Permalink
HV-1444 Improvements to the comments and some minor cosmetic changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@gsmet
gsmet committed Mar 6, 2018
1 parent 50df924 commit ff3c851
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 61 deletions.
63 changes: 38 additions & 25 deletions ...n/java/org/hibernate/validator/internal/engine/valueextraction/ValueExtractorManager.java
View file
Expand Up @@ -12,18 +12,17 @@
import java.security.PrivilegedAction;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import javax.validation.ConstraintDeclarationException;
import javax.validation.ValidationException;
import javax.validation.valueextraction.ValueExtractor;

import org.hibernate.validator.internal.metadata.core.MetaConstraint;
import org.hibernate.validator.internal.util.CollectionHelper;
import org.hibernate.validator.internal.util.privilegedactions.LoadClass;
import org.hibernate.validator.internal.util.stereotypes.Immutable;

Expand Down Expand Up @@ -97,7 +96,7 @@ public ValueExtractorManager(Set<ValueExtractor<?>> externalExtractors) {
}

valueExtractors = Collections.unmodifiableMap( tmpValueExtractors );
valueExtractorResolver = new ValueExtractorResolver( getValueExtractorDescriptorsAsList() );
valueExtractorResolver = new ValueExtractorResolver( new HashSet<>( valueExtractors.values() ) );
}

public ValueExtractorManager(ValueExtractorManager template,
Expand All @@ -106,7 +105,7 @@ public ValueExtractorManager(ValueExtractorManager template,
tmpValueExtractors.putAll( externalValueExtractorDescriptors );

valueExtractors = Collections.unmodifiableMap( tmpValueExtractors );
valueExtractorResolver = new ValueExtractorResolver( getValueExtractorDescriptorsAsList() );
valueExtractorResolver = new ValueExtractorResolver( new HashSet<>( valueExtractors.values() ) );
}

public static Set<ValueExtractor<?>> getDefaultValueExtractors() {
Expand All @@ -116,31 +115,41 @@ public static Set<ValueExtractor<?>> getDefaultValueExtractors() {
}

/**
* Should be used to find a most specific {@link ValueExtractor} based on a declared type, when creating
* {@link MetaConstraint} and there's a need to determine a {@link ValueExtractorDescriptor} for a wrapped value.
* Used to find all the maximally specific value extractors based on a declared type in the case of value unwrapping.
* <p>
* There might be several of them as there might be several type parameters.
* <p>
* Used for container element constraints.
*
* @see ValueExtractorResolver#getMaximallySpecificValueExtractors(Class)
*/
public Set<ValueExtractorDescriptor> getMaximallySpecificValueExtractors(Class<?> valueType) {
return valueExtractorResolver.getMaximallySpecificValueExtractors( valueType );
public Set<ValueExtractorDescriptor> getMaximallySpecificValueExtractors(Class<?> declaredType) {
return valueExtractorResolver.getMaximallySpecificValueExtractors( declaredType );
}

/**
* Should be used to find a most specific {@link ValueExtractor} based on a declared type, when creating
* {@link MetaConstraint} and there's a need to determine a {@link ValueExtractorDescriptor} for a type argument.
* Used to find the maximally specific and container element compliant value extractor based on the declared type
* and the type parameter.
* <p>
* Used for container element constraints.
*
* @see ValueExtractorResolver#getMaximallySpecificAndContainerElementCompliantValueExtractor(Class, TypeVariable)
* @throws ConstraintDeclarationException if more than 2 maximally specific container-element-compliant value extractors are found
*/
public ValueExtractorDescriptor getMaximallySpecificAndContainerElementCompliantValueExtractor(Class<?> declaredType, TypeVariable<?> typeParameter) {
return valueExtractorResolver.getMaximallySpecificAndContainerElementCompliantValueExtractor( declaredType, typeParameter );
}

/**
* Should be used to find a most specific {@link ValueExtractor} based on a runtime type, when the actual
* cascading container validation should perform extraction. Most specific one is determined from candidates
* passed to this method.
* Used to find the maximally specific and container element compliant value extractor based on the runtime type.
* <p>
* The maximally specific one is chosen among the candidates passed to this method.
* <p>
* Used for cascading validation.
*
* @see ValueExtractorResolver#getMaximallySpecificAndRuntimeContainerElementCompliantValueExtractor(Type, TypeVariable, Class, Collection)
* @see ValueExtractorResolver#getMaximallySpecificAndRuntimeContainerElementCompliantValueExtractor(Type,
* TypeVariable, Class, Collection)
* @throws ConstraintDeclarationException if more than 2 maximally specific container-element-compliant value extractors are found
*/
public ValueExtractorDescriptor getMaximallySpecificAndRuntimeContainerElementCompliantValueExtractor(Type declaredType, TypeVariable<?> typeParameter,
Class<?> runtimeType, Collection<ValueExtractorDescriptor> valueExtractorCandidates) {
Expand All @@ -167,11 +176,16 @@ else if ( !valueExtractorCandidates.isEmpty() ) {
}

/**
* Should be used to determine if passed runtime type is a container and if so return a corresponding
* maximally specific {@link ValueExtractor} for it.
* Used to determine if the passed runtime type is a container and if so return a corresponding maximally specific
* value extractor.
* <p>
* Obviously, it only works if there's only one value extractor corresponding to the runtime type as we don't
* precise any type parameter.
* <p>
* The special case is when passed type is assignable to {@link Map} to support legacy container validation.
* In such case {@link MapValueExtractor} will be returned.
* There is a special case: when the passed type is assignable to a {@link Map}, the {@link MapValueExtractor} will
* be returned. This is required by the Bean Validation specification.
* <p>
* Used for cascading validation when the {@code @Valid} annotation is placed on the whole container.
*
* @see ValueExtractorResolver#getMaximallySpecificValueExtractorForAllContainerElements(Class)
*/
Expand All @@ -180,7 +194,11 @@ public ValueExtractorDescriptor getMaximallySpecificValueExtractorForAllContaine
}

/**
* Should be used to find possible {@link ValueExtractor} candidates based on declared type and type variable.
* Used to determine the value extractor candidates valid for a declared type and type variable.
* <p>
* The effective value extractor will be narrowed from these candidates using the runtime type.
* <p>
* Used to optimize the choice of the value extractor in the case of cascading validation.
*
* @see ValueExtractorResolver#getValueExtractorCandidatesForCascadedValidation(Type, TypeVariable)
*/
Expand Down Expand Up @@ -226,11 +244,6 @@ private static boolean isClassPresent(String className, boolean fallbackOnTCCL)
}
}

private List<ValueExtractorDescriptor> getValueExtractorDescriptorsAsList() {
return valueExtractors.values().stream().collect(
Collectors.collectingAndThen( Collectors.toList(), CollectionHelper::toImmutableList ) );
}

/**
* Runs the given privileged action, using a privileged block if required.
* <p>
Expand Down
77 changes: 55 additions & 22 deletions .../java/org/hibernate/validator/internal/engine/valueextraction/ValueExtractorResolver.java
View file
Expand Up @@ -13,7 +13,6 @@
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
Expand All @@ -28,6 +27,7 @@
import org.hibernate.validator.internal.util.TypeVariables;
import org.hibernate.validator.internal.util.logging.Log;
import org.hibernate.validator.internal.util.logging.LoggerFactory;
import org.hibernate.validator.internal.util.stereotypes.Immutable;

/**
* Contains resolving algorithms for {@link ValueExtractor}s, and caches for these
Expand All @@ -43,28 +43,37 @@ class ValueExtractorResolver {

private static final Object NON_CONTAINER_VALUE = new Object();

@Immutable
private final Set<ValueExtractorDescriptor> valueExtractors;

private final ConcurrentHashMap<ValueExtractorCacheKey, Set<ValueExtractorDescriptor>> possibleValueExtractorsByRuntimeTypeAndTypeParameter = new ConcurrentHashMap<>();

private final ConcurrentHashMap<Class<?>, Set<ValueExtractorDescriptor>> possibleValueExtractorsByRuntimeType = new ConcurrentHashMap<>();
private final ConcurrentHashMap<Class<?>, Object> nonContainerTypes = new ConcurrentHashMap<>();

private final List<ValueExtractorDescriptor> valueExtractors;
private final ConcurrentHashMap<Class<?>, Object> nonContainerTypes = new ConcurrentHashMap<>();

ValueExtractorResolver(List<ValueExtractorDescriptor> valueExtractors) {
this.valueExtractors = valueExtractors;
ValueExtractorResolver(Set<ValueExtractorDescriptor> valueExtractors) {
this.valueExtractors = CollectionHelper.toImmutableSet( valueExtractors );
}

/**
* Returns the maximally specific type compliant value extractors or an empty set if none was found.
* Used to find all the maximally specific value extractors based on a declared type in the case of value unwrapping.
* <p>
* There might be several of them as there might be several type parameters.
* <p>
* Used for container element constraints.
*/
public Set<ValueExtractorDescriptor> getMaximallySpecificValueExtractors(Class<?> valueType) {
return getRuntimeComplaintValueExtractors( valueType );
public Set<ValueExtractorDescriptor> getMaximallySpecificValueExtractors(Class<?> declaredType) {
return getRuntimeCompliantValueExtractors( declaredType );
}

/**
* Returns the maximally specific type-compliant and container-element-compliant value extractor or
* {@code null} if none was found.
* Used to find the maximally specific and container element compliant value extractor based on the declared type
* and the type parameter.
* <p>
* Throws an exception if more than 2 maximally specific container-element-compliant value extractors are found.
* Used for container element constraints.
*
* @throws ConstraintDeclarationException if more than 2 maximally specific container-element-compliant value extractors are found
*/
public ValueExtractorDescriptor getMaximallySpecificAndContainerElementCompliantValueExtractor(Class<?> declaredType, TypeVariable<?> typeParameter) {
return getUniqueValueExtractorOrThrowException(
Expand All @@ -74,10 +83,13 @@ public ValueExtractorDescriptor getMaximallySpecificAndContainerElementCompliant
}

/**
* Returns the maximally specific runtime-type-compliant and container-element-compliant value extractor or
* {@code null} if none was found.
* Used to find the maximally specific and container element compliant value extractor based on the runtime type.
* <p>
* The maximally specific one is chosen among the candidates passed to this method.
* <p>
* Throws an exception if more than 2 maximally specific container-element-compliant value extractors are found.
* Used for cascading validation.
*
* @throws ConstraintDeclarationException if more than 2 maximally specific runtime container-element-compliant value extractors are found
*/
public ValueExtractorDescriptor getMaximallySpecificAndRuntimeContainerElementCompliantValueExtractor(Type declaredType, TypeVariable<?> typeParameter,
Class<?> runtimeType, Collection<ValueExtractorDescriptor> valueExtractorCandidates) {
Expand All @@ -90,18 +102,35 @@ public ValueExtractorDescriptor getMaximallySpecificAndRuntimeContainerElementCo
}

/**
* Returns the maximally specific runtime-type-compliant value extractor or {@code null} if none was found.
* Used to determine if the passed runtime type is a container and if so return a corresponding maximally specific
* value extractor.
* <p>
* Obviously, it only works if there's only one value extractor corresponding to the runtime type as we don't
* precise any type parameter.
* <p>
* Throws an exception if more than 2 maximally specific container-element-compliant value extractors are found.
* There is a special case: when the passed type is assignable to a {@link Map}, the {@link MapValueExtractor} will
* be returned. This is required by the Bean Validation specification.
* <p>
* Used for cascading validation when the {@code @Valid} annotation is placed on the whole container.
*
* @throws ConstraintDeclarationException if more than 2 maximally specific container-element-compliant value extractors are found
*/
public ValueExtractorDescriptor getMaximallySpecificValueExtractorForAllContainerElements(Class<?> runtimeType) {
// if it's a Map assignable type it gets a special treatment to support legacy containers
// if it's a Map assignable type, it gets a special treatment to conform to the Bean Validation specification
if ( TypeHelper.isAssignable( Map.class, runtimeType ) ) {
return MapValueExtractor.DESCRIPTOR;
}
return getUniqueValueExtractorOrThrowException( runtimeType, getRuntimeComplaintValueExtractors( runtimeType ) );

return getUniqueValueExtractorOrThrowException( runtimeType, getRuntimeCompliantValueExtractors( runtimeType ) );
}

/**
* Used to determine the value extractor candidates valid for a declared type and type variable.
* <p>
* The effective value extractor will be narrowed from these candidates using the runtime type.
* <p>
* Used to optimize the choice of the value extractor in the case of cascading validation.
*/
public Set<ValueExtractorDescriptor> getValueExtractorCandidatesForCascadedValidation(Type declaredType, TypeVariable<?> typeParameter) {
Set<ValueExtractorDescriptor> valueExtractorDescriptors = new HashSet<>();

Expand Down Expand Up @@ -201,13 +230,14 @@ else if ( TypeHelper.isAssignable( descriptor.getContainerType(), candidate.getC
}

/**
* @return a set of runtime complaint value extractors based on a runtime type. If there are no available value extractors
* an empty set will be returned which means a type is not a container.
* @return a set of runtime compliant value extractors based on a runtime type. If there are no available value extractors
* an empty set will be returned which means the type is not a container.
*/
private Set<ValueExtractorDescriptor> getRuntimeComplaintValueExtractors(Class<?> runtimeType) {
private Set<ValueExtractorDescriptor> getRuntimeCompliantValueExtractors(Class<?> runtimeType) {
if ( nonContainerTypes.contains( runtimeType ) ) {
return Collections.emptySet();
}

Set<ValueExtractorDescriptor> valueExtractorDescriptors = possibleValueExtractorsByRuntimeType.get( runtimeType );
if ( valueExtractorDescriptors == null ) {
//otherwise we just look for maximally specific extractors for the runtime type
Expand All @@ -218,6 +248,7 @@ private Set<ValueExtractorDescriptor> getRuntimeComplaintValueExtractors(Class<?

valueExtractorDescriptors = getMaximallySpecificValueExtractors( possibleValueExtractors );
}

if ( valueExtractorDescriptors.isEmpty() ) {
nonContainerTypes.put( runtimeType, NON_CONTAINER_VALUE );
}
Expand All @@ -235,6 +266,7 @@ private Set<ValueExtractorDescriptor> getRuntimeAndContainerElementComplaintValu
if ( nonContainerTypes.contains( runtimeType ) ) {
return Collections.emptySet();
}

ValueExtractorCacheKey cacheKey = new ValueExtractorCacheKey( runtimeType, typeParameter );

Set<ValueExtractorDescriptor> valueExtractorDescriptors = possibleValueExtractorsByRuntimeTypeAndTypeParameter.get( cacheKey );
Expand Down Expand Up @@ -283,6 +315,7 @@ private boolean validateValueExtractorCompatibility(boolean isInternal,
TypeVariable<?> typeParameterForBinding,
TypeVariable<?> typeParameterToCompare) {
TypeVariable<?> typeParameterBoundToExtractorType;

if ( !isInternal ) {
Map<Class<?>, Map<TypeVariable<?>, TypeVariable<?>>> allBindings =
TypeVariableBindings.getTypeVariableBindings( typeForBinding );
Expand All @@ -293,6 +326,7 @@ private boolean validateValueExtractorCompatibility(boolean isInternal,
else {
typeParameterBoundToExtractorType = typeParameterForBinding;
}

return Objects.equals( typeParameterToCompare, typeParameterBoundToExtractorType );
}

Expand Down Expand Up @@ -336,5 +370,4 @@ private int buildHashCode() {
return result;
}
}

}
29 changes: 15 additions & 14 deletions ...e/validator/test/internal/engine/valueextraction/MultipleContainersAtTheSameTimeTest.java
View file
Expand Up @@ -54,9 +54,9 @@ public void setupValidator() {
}

/**
* Even though there runtime type implements both {@link List} and {@link CustomContainer} the constraint
* is declared only on the list side of the hierarchy hence we cannot bind this constraint to
* {@link CustomContainer} and as a result use {@code CustomContainerValueExtractor}
* Even though, there, the runtime type implements both {@link List} and {@link CustomContainer}, the constraint is
* declared only on the list side of the hierarchy hence we cannot bind this constraint to {@link CustomContainer}
* and as a result we only retain {@code CustomContainerValueExtractor}.
*/
@Test
public void testMultipleContainersAtTheSameTimeShouldNotThrowException() throws Exception {
Expand All @@ -71,9 +71,9 @@ public void testMultipleContainersAtTheSameTimeShouldNotThrowException() throws
}

/**
* Even though there runtime type implements both {@link List} and {@link CustomContainer} the constraint
* Even though, there, the runtime type implements both {@link List} and {@link CustomContainer}, the constraint
* is declared only on the list side of the hierarchy hence we cannot bind this constraint to
* {@link CustomContainer} and as a result use {@code CustomContainerValueExtractor}
* {@link CustomContainer} and as a result we only retain {@code CustomContainerValueExtractor}
*/
@Test
public void testMultipleContainersAtTheSameTimeShouldAlsoNotThrowException() throws Exception {
Expand All @@ -87,25 +87,26 @@ public void testMultipleContainersAtTheSameTimeShouldAlsoNotThrowException() thr
}

/**
* Test should fail as constraint is declared on a {@link FooBarContainer} which accept both
* {@code ListValueExtractor} as well as {@code CustomContainerValueExtractor}
* Test should fail as the constraint is declared on a {@link FooBarContainer} which accepts both
* {@code ListValueExtractor} and {@code CustomContainerValueExtractor}.
*/
@Test(expectedExceptions = ConstraintDeclarationException.class)
public void testMultipleContainersAtTheSameTimeShouldThrowException() throws Exception {
validator.validate( new FooBar( new FooBarContainer<String>().add( "" ) ) );
}

/**
* Test should fail as constraint is declared on a {@link FooBarContainer} which accept both
* {@code ListValueExtractor} as well as {@code CustomContainerValueExtractor}
* Test should fail as the constraint is declared on a {@link FooBarContainer} which accepts both
* {@code ListValueExtractor} and {@code CustomContainerValueExtractor}.
*/
@Test(expectedExceptions = ConstraintDeclarationException.class)
public void testMultipleContainersAtTheSameTimeWithTypeParameterSpecificShouldThrowException() throws Exception {
validator.validate( new FooBar( new FooBarContainer<String>().add( "" ) ) );
}

/**
* Test that container is determined at runtime and that maximally specific VE is used for that container.
* Test that container is determined at runtime and that the maximally specific value extractor is used for that
* container.
*/
@Test
public void testCascadingWhenUsingObjectReferenceUsesMostSpecificValueExtractor() throws Exception {
Expand All @@ -125,9 +126,10 @@ class Bar {
}

/**
* Test for selecting a correct VE. Even though {@link ImprovedCustomContainerValueExtractor} is more
* specific by container type - it shouldn't be selected based on the type argument where the constraint is placed.
* If {@link ImprovedCustomContainerValueExtractor} is selected an {@link IllegalStateException} will be thrown
* Test that the correct value extractor is selected. Even though {@link ImprovedCustomContainerValueExtractor} is more
* specific by container type, it shouldn't be selected based on the type argument where the constraint is placed.
* <p>
* If {@link ImprovedCustomContainerValueExtractor} is selected, an {@link IllegalStateException} will be thrown
* by this extractor and test will fail.
*/
@Test
Expand Down Expand Up @@ -319,5 +321,4 @@ public List<String> subList(int fromIndex, int toIndex) {
return null;
}
}

}

0 comments on commit ff3c851

Please sign in to comment.