Skip to content

HV-1623 Build an abstraction over reflection in engine code #963

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

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
53ea896
HV-1623 Build abstraction over reflection in ConstraintLocation and r…
marko-bekhta Mar 10, 2018
af67fd7
HV-1623 Reintroduce field and getter constraint locations
marko-bekhta Jun 7, 2018
556d277
HV-1623 Clean ups around cascadable properties in PropertyMetaData
marko-bekhta Jun 5, 2018
dbffeba
HV-1623 Remove redundant check in ConfiguredConstraint
marko-bekhta May 30, 2018
3063210
HV-1623 Clean up property metadata
marko-bekhta May 30, 2018
660b64c
HV-1623 Add additional checks before casting
marko-bekhta May 30, 2018
0b0835c
HV-1623 Remove ConstrainableFormatter and push the logic to toString …
marko-bekhta May 30, 2018
fc0824a
HV-1623 Wrap ElementType with ConstraintLocationKind
marko-bekhta May 31, 2018
9376899
HV-1623 More abstraction work around executables
gsmet Jun 1, 2018
f44ecb4
HV-1623 Propagate ConstraintLocationKind as far as possible
gsmet Jun 4, 2018
418e010
HV-1623 Encapsulate ConstraintLocationKind inside ConstraintLocation
gsmet Jun 4, 2018
8bb498e
HV-1623 Extract class-level metadata from property metadata case
marko-bekhta Jun 4, 2018
cace416
HV-1623 Create different programmatic mapping contexts for getter and…
marko-bekhta Jun 5, 2018
34e9db9
HV-1623 Make parameters more specific in programmatic API executable …
marko-bekhta Jun 5, 2018
cd64f25
HV-1623 Deprecate property() in programmatic API and introduce field(…
marko-bekhta Jun 6, 2018
49c11c6
HV-1623 Replace empty check in Contracts with null and empty check at…
marko-bekhta Jun 7, 2018
7475853
HV-1623 Introduce getter as a constraint location kind
marko-bekhta Jun 7, 2018
e5b7c8d
HV-1623 Introduce ConstrainedElementKind.GETTER
gsmet Jun 8, 2018
3a1944c
HV-1623 Stop the propagation of JavaBeanField/Getter
gsmet Jun 8, 2018
ce6f342
HV-1623 Rename a couple of classes to Abstract*
gsmet Jun 8, 2018
6d7b067
HV-1623 Introduce Field and Getter as subclasses of Property
gsmet Jun 8, 2018
edb5efd
HV-1623 Throw exception in case of unexpected constrainedElement clas…
marko-bekhta Jun 9, 2018
0332e6c
HV-1623 Make the fields/getters accessible lazily
gsmet Jun 9, 2018
be6861c
HV-1623 Reduce the memory footprint of the new reflection abstraction
gsmet Jun 11, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,46 @@ public interface PropertyTarget {
* <p>
* Until this method is called constraints apply on class level. After calling this method constraints
* apply on the specified property with the given access type.
* </p>
* <p>
* A given property may only be configured once.
* </p>
*
* @param property The property on which to apply the following constraints (Java Bean notation).
* @param type The access type (field/property).
*
* @return A creational context representing the selected property.
*
* @deprecated Since 6.1. Planned for removal. Use either {@link PropertyTarget#field(String)} or
* {@link PropertyTarget#getter(String)} instead.
*/
@Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

@gsmet While I agree that the two new methods are nicer, I'm unsure whether it's worth the API change (hazzle with WildFly etc.).

Copy link
Member

Choose a reason for hiding this comment

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

Btw. one advantage of the original form was that it'll also better work with models that don't distinguish between fields/getters (HV-1000). In that case we could think of either accepting null for type (or simply ignoring whatever is passed) or add an overload property(String name). So it feels we're limiting ourselves with this change a little bit.

Copy link
Member

Choose a reason for hiding this comment

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

While I agree that the two new methods are nicer, I'm unsure whether it's worth the API change (hazzle with WildFly etc.).

The old method is still there, it's marked as deprecated.

Btw. one advantage of the original form was that it'll also better work with models that don't distinguish between fields/getters (HV-1000).

The original form was misleading because there is indeed a difference in treatment between fields and getters and they are totally different beasts.

As for HV-1000 (which is one of the ultimate goals of this PR), there's a good chance the programmatic API entry points will be different (mostly because we will have to define the type for cascaded validation) so we will be able to have a property().

If we end up reusing the same API classes, then we will unmark property() as deprecated but I still think keeping getter() and field() makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gunnarmorling from what we have right now for json and maps, PropertyTarget for new programmatic mapping has just this method:

public interface PropertyTarget {
    PropertyConstraintMappingContext property(String property, Class<?> propertyType);
}

as we need to pass the type of the property. Otherwise we would have all the info to create Property. Passing this type also serves as a type validation which I think is a nice bonus :).

So most likely we will end up adding another method anyway.

Having those two shortcuts for field and getter is consistent with what we have for XML configuration (that's what we talked with @gsmet about). Hence if there might be issues with deprecation of property() maybe we should leave it as is also but keep the two shortcuts ?

Copy link
Member

Choose a reason for hiding this comment

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

The old method is still there, it's marked as deprecated.

Understood, but the goal of deprecation is removal, no? Feel free do it if you think it's worth it, to me it doesn't necessarily appear that way (given all the discussions we had in the past...).

As for HV-1000 [...] entry points will be different

Mh, I see. I was hoping that wouldn't be the case.

so we will be able to have a property().

I see.

Copy link
Member

Choose a reason for hiding this comment

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

Understood, but the goal of deprecation is removal, no?

Maybe at some point. But I agree we might just keep the method here forever.

To be honest, it's more to be clear than getters and fields should be different from the get to and to offer similar APIs for programmatic API and XML definition.

PropertyConstraintMappingContext property(String property, ElementType type);

/**
* Selects a field to which the next operations shall apply.
* <p>
* Until this method is called constraints apply on class level. After calling this method constraints
* apply on the specified field property.
* <p>
* A given field may only be configured once.
*
* @param property The field name that represents a property on which to apply the following constraints.
*
* @return A creational context representing the selected field property.
*/
PropertyConstraintMappingContext field(String property);

/**
* Selects a getter to which the next operations shall apply.
* <p>
* Until this method is called constraints apply on class level. After calling this method constraints
* apply on the specified getter property.
* <p>
* A given getter may only be configured once.
*
* @param property The getter property name (using the Java Bean notation, e.g. {@code name} to address {@code getName()})
* that represents a property on which to apply the following constraints.
*
* @return A creational context representing the selected getter property.
*/
PropertyConstraintMappingContext getter(String property);
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,7 @@
package org.hibernate.validator.internal.cfg.context;

import java.lang.annotation.ElementType;
import java.lang.reflect.Executable;
import java.lang.reflect.Field;
import java.lang.reflect.Member;
import java.lang.reflect.Method;

import org.hibernate.validator.cfg.ConstraintDef;
import org.hibernate.validator.cfg.context.ConstructorConstraintMappingContext;
import org.hibernate.validator.cfg.context.ContainerElementConstraintMappingContext;
import org.hibernate.validator.cfg.context.MethodConstraintMappingContext;
Expand All @@ -21,11 +16,8 @@
import org.hibernate.validator.internal.metadata.core.ConstraintHelper;
import org.hibernate.validator.internal.metadata.descriptor.ConstraintDescriptorImpl.ConstraintType;
import org.hibernate.validator.internal.metadata.location.ConstraintLocation;
import org.hibernate.validator.internal.metadata.raw.ConfigurationSource;
import org.hibernate.validator.internal.metadata.raw.ConstrainedElement;
import org.hibernate.validator.internal.metadata.raw.ConstrainedExecutable;
import org.hibernate.validator.internal.metadata.raw.ConstrainedField;
import org.hibernate.validator.internal.util.ReflectionHelper;
import org.hibernate.validator.internal.properties.Property;
import org.hibernate.validator.internal.util.TypeResolutionHelper;

/**
Expand All @@ -34,50 +26,27 @@
* @author Hardy Ferentschik
* @author Gunnar Morling
* @author Kevin Pollet &lt;[email protected]&gt; (C) 2011 SERLI
* @author Marko Bekhta
*/
final class PropertyConstraintMappingContextImpl
abstract class AbstractPropertyConstraintMappingContextImpl<T extends Property>
extends CascadableConstraintMappingContextImplBase<PropertyConstraintMappingContext>
implements PropertyConstraintMappingContext {

private final TypeConstraintMappingContextImpl<?> typeContext;

// either Field or Method
private final Member member;
private final T property;
private final ConstraintLocation location;

PropertyConstraintMappingContextImpl(TypeConstraintMappingContextImpl<?> typeContext, Member member) {
super( typeContext.getConstraintMapping(), ReflectionHelper.typeOf( member ) );
protected AbstractPropertyConstraintMappingContextImpl(TypeConstraintMappingContextImpl<?> typeContext, T property, ConstraintLocation location) {
super( typeContext.getConstraintMapping(), property.getType() );
this.typeContext = typeContext;
this.member = member;
if ( member instanceof Field ) {
this.location = ConstraintLocation.forField( (Field) member );
}
else {
this.location = ConstraintLocation.forGetter( (Method) member );
}
this.property = property;
this.location = location;
}

@Override
protected PropertyConstraintMappingContextImpl getThis() {
return this;
}

@Override
public PropertyConstraintMappingContext constraint(ConstraintDef<?, ?> definition) {
if ( member instanceof Field ) {
super.addConstraint(
ConfiguredConstraint.forProperty(
definition, member
)
);
}
else {
super.addConstraint(
ConfiguredConstraint.forExecutable(
definition, (Method) member
)
);
}
protected AbstractPropertyConstraintMappingContextImpl<?> getThis() {
return this;
}

Expand All @@ -88,7 +57,7 @@ public PropertyConstraintMappingContext ignoreAnnotations() {

@Override
public PropertyConstraintMappingContext ignoreAnnotations(boolean ignoreAnnotations) {
mapping.getAnnotationProcessingOptions().ignoreConstraintAnnotationsOnMember( member, ignoreAnnotations );
mapping.getAnnotationProcessingOptions().ignoreConstraintAnnotationsOnMember( property, ignoreAnnotations );
return this;
}

Expand All @@ -97,6 +66,16 @@ public PropertyConstraintMappingContext property(String property, ElementType el
return typeContext.property( property, elementType );
}

@Override
public PropertyConstraintMappingContext field(String property) {
return typeContext.field( property );
}

@Override
public PropertyConstraintMappingContext getter(String property) {
return typeContext.getter( property );
}

@Override
public ConstructorConstraintMappingContext constructor(Class<?>... parameterTypes) {
return typeContext.constructor( parameterTypes );
Expand All @@ -117,29 +96,14 @@ public ContainerElementConstraintMappingContext containerElementType(int index,
return super.containerElement( this, typeContext, location, index, nestedIndexes );
}

ConstrainedElement build(ConstraintHelper constraintHelper, TypeResolutionHelper typeResolutionHelper, ValueExtractorManager valueExtractorManager) {
if ( member instanceof Field ) {
return new ConstrainedField(
ConfigurationSource.API,
(Field) member,
getConstraints( constraintHelper, typeResolutionHelper, valueExtractorManager ),
getTypeArgumentConstraints( constraintHelper, typeResolutionHelper, valueExtractorManager ),
getCascadingMetaDataBuilder()
);
}
else {
return new ConstrainedExecutable(
ConfigurationSource.API,
(Executable) member,
getConstraints( constraintHelper, typeResolutionHelper, valueExtractorManager ),
getTypeArgumentConstraints( constraintHelper, typeResolutionHelper, valueExtractorManager ),
getCascadingMetaDataBuilder()
);
}
}
abstract ConstrainedElement build(ConstraintHelper constraintHelper, TypeResolutionHelper typeResolutionHelper, ValueExtractorManager valueExtractorManager);

@Override
protected ConstraintType getConstraintType() {
return ConstraintType.GENERIC;
}

protected T getProperty() {
return property;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,8 @@
package org.hibernate.validator.internal.cfg.context;

import java.lang.annotation.Annotation;
import java.lang.annotation.ElementType;
import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.reflect.Executable;
import java.lang.reflect.Field;
import java.lang.reflect.Member;
import java.lang.reflect.Method;
import java.lang.reflect.Type;
import java.lang.reflect.TypeVariable;
import java.security.AccessController;
Expand All @@ -24,7 +19,9 @@
import org.hibernate.validator.cfg.AnnotationDef;
import org.hibernate.validator.cfg.ConstraintDef;
import org.hibernate.validator.internal.metadata.location.ConstraintLocation;
import org.hibernate.validator.internal.util.ExecutableHelper;
import org.hibernate.validator.internal.properties.Callable;
import org.hibernate.validator.internal.properties.javabean.JavaBeanField;
import org.hibernate.validator.internal.properties.javabean.JavaBeanGetter;
import org.hibernate.validator.internal.util.annotation.AnnotationDescriptor;
import org.hibernate.validator.internal.util.annotation.ConstraintAnnotationDescriptor;
import org.hibernate.validator.internal.util.logging.Log;
Expand All @@ -36,6 +33,7 @@
* related to its location (bean type etc.).
*
* @author Gunnar Morling
* @author Guillaume Smet
*/
class ConfiguredConstraint<A extends Annotation> {

Expand All @@ -46,58 +44,40 @@ class ConfiguredConstraint<A extends Annotation> {

private final ConstraintDef<?, A> constraint;
private final ConstraintLocation location;
private final ElementType elementType;

private ConfiguredConstraint(ConstraintDef<?, A> constraint, ConstraintLocation location, ElementType elementType) {
private ConfiguredConstraint(ConstraintDef<?, A> constraint, ConstraintLocation location) {
this.constraint = constraint;
this.location = location;
this.elementType = elementType;
}

static <A extends Annotation> ConfiguredConstraint<A> forType(ConstraintDef<?, A> constraint, Class<?> beanType) {
return new ConfiguredConstraint<>( constraint, ConstraintLocation.forClass( beanType ), ElementType.TYPE );
return new ConfiguredConstraint<>( constraint, ConstraintLocation.forClass( beanType ) );
}

static <A extends Annotation> ConfiguredConstraint<A> forProperty(ConstraintDef<?, A> constraint, Member member) {
if ( member instanceof Field ) {
return new ConfiguredConstraint<>(
constraint,
ConstraintLocation.forField( (Field) member ),
ElementType.FIELD
);
}
else {
return new ConfiguredConstraint<>(
constraint,
ConstraintLocation.forGetter( (Method) member ),
ElementType.METHOD
);
}
static <A extends Annotation> ConfiguredConstraint<A> forField(ConstraintDef<?, A> constraint, JavaBeanField javaBeanField) {
return new ConfiguredConstraint<>( constraint, ConstraintLocation.forField( javaBeanField ) );
}

public static <A extends Annotation> ConfiguredConstraint<A> forParameter(ConstraintDef<?, A> constraint, Executable executable, int parameterIndex) {
return new ConfiguredConstraint<>(
constraint, ConstraintLocation.forParameter( executable, parameterIndex ), ExecutableHelper.getElementType( executable )
);
static <A extends Annotation> ConfiguredConstraint<A> forGetter(ConstraintDef<?, A> constraint, JavaBeanGetter javaBeanGetter) {
return forExecutable( constraint, javaBeanGetter );
}

public static <A extends Annotation> ConfiguredConstraint<A> forExecutable(ConstraintDef<?, A> constraint, Executable executable) {
return new ConfiguredConstraint<>(
constraint, ConstraintLocation.forReturnValue( executable ), ExecutableHelper.getElementType( executable )
);
public static <A extends Annotation> ConfiguredConstraint<A> forParameter(ConstraintDef<?, A> constraint, Callable callable, int parameterIndex) {
return new ConfiguredConstraint<>( constraint, ConstraintLocation.forParameter( callable, parameterIndex ) );
}

public static <A extends Annotation> ConfiguredConstraint<A> forCrossParameter(ConstraintDef<?, A> constraint, Executable executable) {
return new ConfiguredConstraint<>(
constraint, ConstraintLocation.forCrossParameter( executable ), ExecutableHelper.getElementType( executable )
);
public static <A extends Annotation> ConfiguredConstraint<A> forExecutable(ConstraintDef<?, A> constraint, Callable callable) {
return new ConfiguredConstraint<>( constraint, ConstraintLocation.forReturnValue( callable ) );
}

public static <A extends Annotation> ConfiguredConstraint<A> forTypeArgument(ConstraintDef<?, A> constraint,ConstraintLocation delegate, TypeVariable<?> typeArgument, Type typeOfAnnotatedElement) {
public static <A extends Annotation> ConfiguredConstraint<A> forCrossParameter(ConstraintDef<?, A> constraint, Callable callable) {
return new ConfiguredConstraint<>( constraint, ConstraintLocation.forCrossParameter( callable ) );
}

public static <A extends Annotation> ConfiguredConstraint<A> forTypeArgument(ConstraintDef<?, A> constraint, ConstraintLocation delegate, TypeVariable<?> typeArgument, Type typeOfAnnotatedElement) {
return new ConfiguredConstraint<>(
constraint,
ConstraintLocation.forTypeArgument( delegate, typeArgument, typeOfAnnotatedElement ),
ElementType.TYPE_USE
ConstraintLocation.forTypeArgument( delegate, typeArgument, typeOfAnnotatedElement )
);
}

Expand Down Expand Up @@ -127,10 +107,6 @@ public String toString() {
return constraint.toString();
}

public ElementType getElementType() {
return elementType;
}

/**
* Runs the given privileged action, using a privileged block if required.
* <b>NOTE:</b> This must never be changed into a publicly available method to avoid execution of arbitrary
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,9 @@ private <A extends Annotation> MetaConstraint<A> asMetaConstraint(ConfiguredCons
TypeResolutionHelper typeResolutionHelper, ValueExtractorManager valueExtractorManager) {
ConstraintDescriptorImpl<A> constraintDescriptor = new ConstraintDescriptorImpl<A>(
constraintHelper,
config.getLocation().getMember(),
config.getLocation().getConstrainable(),
config.createAnnotationDescriptor(),
config.getElementType(),
config.getLocation().getKind(),
getConstraintType()
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@
*/
package org.hibernate.validator.internal.cfg.context;

import java.lang.reflect.Constructor;

import org.hibernate.validator.cfg.context.ConstructorConstraintMappingContext;
import org.hibernate.validator.internal.properties.javabean.JavaBeanConstructor;

/**
* Constraint mapping creational context representing a constructor.
Expand All @@ -17,15 +16,15 @@
*/
class ConstructorConstraintMappingContextImpl extends ExecutableConstraintMappingContextImpl implements ConstructorConstraintMappingContext {

<T> ConstructorConstraintMappingContextImpl(TypeConstraintMappingContextImpl<T> typeContext, Constructor<T> constructor) {
<T> ConstructorConstraintMappingContextImpl(TypeConstraintMappingContextImpl<T> typeContext, JavaBeanConstructor constructor) {
super( typeContext, constructor );
}

@Override
public ConstructorConstraintMappingContext ignoreAnnotations(boolean ignoreAnnotations) {
typeContext.mapping
.getAnnotationProcessingOptions()
.ignoreConstraintAnnotationsOnMember( executable, ignoreAnnotations );
.ignoreConstraintAnnotationsOnMember( callable, ignoreAnnotations );

return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,16 @@ public PropertyConstraintMappingContext property(String property, ElementType el
return typeContext.property( property, elementType );
}

@Override
public PropertyConstraintMappingContext field(String property) {
return typeContext.field( property );
}

@Override
public PropertyConstraintMappingContext getter(String property) {
return typeContext.getter( property );
}

@Override
public ConstructorConstraintMappingContext constructor(Class<?>... parameterTypes) {
return typeContext.constructor( parameterTypes );
Expand Down Expand Up @@ -237,9 +247,9 @@ private <A extends Annotation> MetaConstraint<A> asMetaConstraint(ConfiguredCons
TypeResolutionHelper typeResolutionHelper, ValueExtractorManager valueExtractorManager) {
ConstraintDescriptorImpl<A> constraintDescriptor = new ConstraintDescriptorImpl<>(
constraintHelper,
config.getLocation().getMember(),
config.getLocation().getConstrainable(),
config.createAnnotationDescriptor(),
config.getElementType(),
config.getLocation().getKind(),
getConstraintType()
);

Expand Down
Loading