-
Notifications
You must be signed in to change notification settings - Fork 579
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
Closed
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 af67fd7
HV-1623 Reintroduce field and getter constraint locations
marko-bekhta 556d277
HV-1623 Clean ups around cascadable properties in PropertyMetaData
marko-bekhta dbffeba
HV-1623 Remove redundant check in ConfiguredConstraint
marko-bekhta 3063210
HV-1623 Clean up property metadata
marko-bekhta 660b64c
HV-1623 Add additional checks before casting
marko-bekhta 0b0835c
HV-1623 Remove ConstrainableFormatter and push the logic to toString …
marko-bekhta fc0824a
HV-1623 Wrap ElementType with ConstraintLocationKind
marko-bekhta 9376899
HV-1623 More abstraction work around executables
gsmet f44ecb4
HV-1623 Propagate ConstraintLocationKind as far as possible
gsmet 418e010
HV-1623 Encapsulate ConstraintLocationKind inside ConstraintLocation
gsmet 8bb498e
HV-1623 Extract class-level metadata from property metadata case
marko-bekhta cace416
HV-1623 Create different programmatic mapping contexts for getter and…
marko-bekhta 34e9db9
HV-1623 Make parameters more specific in programmatic API executable …
marko-bekhta cd64f25
HV-1623 Deprecate property() in programmatic API and introduce field(…
marko-bekhta 49c11c6
HV-1623 Replace empty check in Contracts with null and empty check at…
marko-bekhta 7475853
HV-1623 Introduce getter as a constraint location kind
marko-bekhta e5b7c8d
HV-1623 Introduce ConstrainedElementKind.GETTER
gsmet 3a1944c
HV-1623 Stop the propagation of JavaBeanField/Getter
gsmet ce6f342
HV-1623 Rename a couple of classes to Abstract*
gsmet 6d7b067
HV-1623 Introduce Field and Getter as subclasses of Property
gsmet edb5efd
HV-1623 Throw exception in case of unexpected constrainedElement clas…
marko-bekhta 0332e6c
HV-1623 Make the fields/getters accessible lazily
gsmet be6861c
HV-1623 Reduce the memory footprint of the new reflection abstraction
gsmet File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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; | ||
|
||
/** | ||
|
@@ -34,50 +26,27 @@ | |
* @author Hardy Ferentschik | ||
* @author Gunnar Morling | ||
* @author Kevin Pollet <[email protected]> (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; | ||
} | ||
|
||
|
@@ -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; | ||
} | ||
|
||
|
@@ -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 ); | ||
|
@@ -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; | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.).
There was a problem hiding this comment.
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 overloadproperty(String name)
. So it feels we're limiting ourselves with this change a little bit.There was a problem hiding this comment.
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.
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 keepinggetter()
andfield()
makes sense.There was a problem hiding this comment.
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: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 ?There was a problem hiding this comment.
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? 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...).
Mh, I see. I was hoping that wouldn't be the case.
I see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.