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

marko-bekhta · 2018-06-01T13:52:38Z

this is an "abstraction" part of this PR #938

there are a few more clean up commits added on top of the existing changes in the mentioned PR.
As for the last commit that wraps ElementType - I am not very happy about it and I'll be looking into changing it. Initially I wanted to use ConstraintLocation to determine ElementType needed for calls like this - https://github.com/hibernate/hibernate-validator/blob/master/engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java#L1282-L1287 but then there's this usage https://github.com/hibernate/hibernate-validator/blob/master/engine/src/main/java/org/hibernate/validator/internal/metadata/descriptor/ElementDescriptorImpl.java#L145 of ElementType from constraint descriptors which I didn't know about and it broke the initial approach on this :) But I plan to revisit this idea once more and try to clean things up in this area of ElementType usage.

marko-bekhta · 2018-06-01T13:55:08Z

NOTE to myself: address this - #949 (comment)

gsmet

Hi @marko-bekhta ,

I pushed a few cleanup commits and changes. It would be nice if you could review them.

I also added a few comments, the biggest issue being the property / field / getter thing. It's the last big area that needs improving in this patch IMHO. AFAICS, Gunnar had the same doubts as me so let's tackle this.

Apart from that, and my other more minor comments, I think we are very near being in a commitable state.

gsmet · 2018-06-04T10:21:34Z

...va/org/hibernate/validator/internal/cfg/context/ConstructorConstraintMappingContextImpl.java

@@ -17,15 +16,15 @@
 */
 class ConstructorConstraintMappingContextImpl extends ExecutableConstraintMappingContextImpl implements ConstructorConstraintMappingContext {

-	<T> ConstructorConstraintMappingContextImpl(TypeConstraintMappingContextImpl<T> typeContext, Constructor<T> constructor) {
+	<T> ConstructorConstraintMappingContextImpl(TypeConstraintMappingContextImpl<T> typeContext, Callable constructor) {


I still have my reservations about not making Constructor a thing. Typically, I did it in the java bean part in one of my commit. I think it wouldn't be bad to have this distinction.

gsmet · 2018-06-04T10:22:20Z

...in/java/org/hibernate/validator/internal/cfg/context/MethodConstraintMappingContextImpl.java

@@ -17,15 +16,15 @@
 */
 class MethodConstraintMappingContextImpl extends ExecutableConstraintMappingContextImpl implements MethodConstraintMappingContext {

-	MethodConstraintMappingContextImpl(TypeConstraintMappingContextImpl<?> typeContext, Method method) {
-		super( typeContext, method );
+	MethodConstraintMappingContextImpl(TypeConstraintMappingContextImpl<?> typeContext, Callable callable) {


And I would also make Method a thing.

gsmet · 2018-06-04T10:25:43Z

.../java/org/hibernate/validator/internal/cfg/context/PropertyConstraintMappingContextImpl.java

@@ -64,17 +57,17 @@ protected PropertyConstraintMappingContextImpl getThis() {

 	@Override
 	public PropertyConstraintMappingContext constraint(ConstraintDef<?, ?> definition) {
-		if ( member instanceof Field ) {
+		if ( property instanceof JavaBeanField ) {


Our abstraction doesn't work very well here.

That's why I'm not a big fan of merging field and getters so early and not being able to have a distinction between them at the base layer (by base, I mean Property, Callable and so on).

TBH, considering the behavior is entirely different, I'm not a big fan of this class. Note that the issue was preexisting, it's not something you introduced.

gsmet · 2018-06-04T10:34:40Z

...e/src/main/java/org/hibernate/validator/internal/metadata/aggregated/PropertyCascadable.java

 		this.cascadingMetaData = cascadingMetaData;
+		this.constraintLocationKind = property instanceof JavaBeanField ? ConstraintLocationKind.PROPERTY : ConstraintLocationKind.METHOD;


We shouldn't have to rely on JavaBeanField here.

That will probably change if we reintroduce the difference between getters and properties. Frankly, I would really like to be able to follow the path of a getter and understand how it's specific.

If we end up still having to do that, I think we should have a ConstrainedElementKind getConstrainedElementKind() in Constrainable (instead of Callable). That might help for other spots too.

gsmet · 2018-06-04T10:43:13Z

engine/src/main/java/org/hibernate/validator/internal/metadata/location/ConstraintLocation.java

+		CONSTRUCTOR( ElementType.CONSTRUCTOR ),
+		METHOD( ElementType.METHOD ),
+		PARAMETER( ElementType.PARAMETER ),
+		PROPERTY( ElementType.FIELD ),


So it's used for getters + fields?

gsmet · 2018-06-04T10:43:34Z

engine/src/main/java/org/hibernate/validator/internal/metadata/location/ConstraintLocation.java

+	 */
+	ConstraintLocationKind getKind();
+
+	enum ConstraintLocationKind {


Wondering if we should move this one into a separate file as it's now a first class citizen.

gsmet · 2018-06-04T10:45:27Z

engine/src/main/java/org/hibernate/validator/internal/metadata/raw/ConstrainedElement.java

@@ -47,7 +47,7 @@
 	 * @author Gunnar Morling
 	 */
 	enum ConstrainedElementKind {
-		TYPE, FIELD, CONSTRUCTOR, METHOD, PARAMETER
+		TYPE, CONSTRUCTOR, METHOD, PARAMETER, PROPERTY


So that means PROPERTY is used for getters and fields, I suppose?

We would have to check this wrt the other changes regarding getter + field.

Hey, this ties in a bit with my recent comment: the "raw" model was not meant to do abstractions from actual elements (fields, getters etc.). Adding PROPERTY to ConstraintElementKind blurries that line. I didn't look into the entire PR, so perhaps it makes sense, but I wanted to bring up that it appears as quite some deviation from the existing abstractions.

+1 that's the same problem. Will work on it :)

@gunnarmorling yup, we are well aware of the issue. That's the big thing that still needs fixing in this PR.

gsmet · 2018-06-04T10:48:30Z

engine/src/main/java/org/hibernate/validator/internal/properties/javabean/JavaBeanField.java

@@ -136,7 +149,7 @@ private static Field getAccessible(Field original) {

 	/**
 	 * Runs the given privileged action, using a privileged block if required.
-	 * <p>
+	 *


I don't think we should remove this.

gsmet · 2018-06-04T10:48:58Z

engine/src/main/java/org/hibernate/validator/internal/properties/javabean/JavaBeanGetter.java

@@ -146,7 +127,7 @@ private static Method getAccessible(Method original) {

 	/**
 	 * Runs the given privileged action, using a privileged block if required.
-	 * <p>
+	 *


Same here. IMHO a quick check of the diff is in order :).

marko-bekhta

@gsmet you beat me to it with all new changes :) I've just looked through and added a couple questions/comments. Thanks!

marko-bekhta · 2018-06-04T10:24:37Z

engine/src/main/java/org/hibernate/validator/internal/engine/ConstraintViolationImpl.java

@@ -39,7 +39,6 @@
 	private final Map<String, Object> messageParameters;
 	private final Map<String, Object> expressionVariables;
 	private final Class<T> rootBeanClass;
-	private final ElementType elementType;


I've remembered what I wanted to ask about this commit :) I was just curious why we had ElementType in here in the first place ? Everything else in here looks fine.

No idea tbh. Probably a leftover of some sort.

marko-bekhta · 2018-06-04T10:31:13Z

...main/java/org/hibernate/validator/internal/properties/javabean/JavaBeanAnnotatedElement.java

+
+	TypeVariable<?>[] getTypeParameters();
+
+	<A extends Annotation> A getAnnotation(Class<A> annotationClass);


how about using Optional here? or would it be inconsistent with the rest of the code in, I suspect, AnnotationMetaDataProvider ?

I made it consistent with the reflection API. I think it's good enough as it is.

marko-bekhta · 2018-06-04T10:37:21Z

engine/src/main/java/org/hibernate/validator/internal/properties/javabean/JavaBeanGetter.java

@@ -88,6 +76,11 @@ public String getParameterName(ExecutableParameterNameProvider parameterNameProv
 		return declaringClass;
 	}

+	@Override
+	public ConstrainedElementKind getConstrainedElementKind() {


It seems that JavaBeanMethod already returns the same element kind. Do we need to override it here?

marko-bekhta · 2018-06-04T10:44:19Z

...e/src/main/java/org/hibernate/validator/internal/properties/javabean/JavaBeanExecutable.java

@@ -83,60 +107,67 @@ public Type getType() {
 	}

 	@Override
-	public Class<?>[] getParameterTypes() {


This one was hard to read 😄

marko-bekhta · 2018-06-04T10:48:35Z

engine/src/main/java/org/hibernate/validator/internal/properties/javabean/JavaBeanField.java

@@ -63,6 +66,31 @@ public String getPropertyName() {
 		return getName();
 	}

+	@Override
+	public AnnotatedType getAnnotatedType() {


I like these changes. We don't expose the reflection Member from these classes and annotation provider got cleaner. Only question left - should we iterate once more and hide the annotations as well ? I had such idea at first but discarded it till later times. I was thinking about moving all this logic of finding constraints into these org.hibernate.validator.internal.properties.javabean.* classes that would make the AnnotationMetaDataProvider much smaller. And also "hide" the reflection even more.

For now, I don't think we should hide the reflection from AnnotationMetaDataProvider: it's tied to Java beans anyway. Maybe we will need to do it later but I think it can be kept for another day.

marko-bekhta · 2018-06-04T10:51:48Z

...ain/java/org/hibernate/validator/internal/metadata/location/ParameterConstraintLocation.java

 	}

 	@Override
 	public Class<?> getDeclaringClass() {
-		return executable.getDeclaringClass();
+		return callable.getDeclaringClass();
 	}

 	@Override
 	public Constrainable getMember() {


Maybe let's rename the method as well? getConstrainable()?

marko-bekhta · 2018-06-04T11:02:42Z

As for this part:

the biggest issue being the property / field / getter thing

I agree. That's why I also left that ^ note to myself to fix things. It might be that we'd need to find a better name instead of a property. Right now we represent getters as methods all the way till the aggregation of the metadata. Hence using 'field' would be a better approach, but I'm not sure if these classes/enum values with property in the name are not also needed for non java bean types. I'll try to check that later today and will get back to you.

gsmet · 2018-06-04T11:06:33Z

Just force-pushed an update to fix the unused import issue.

gsmet · 2018-06-04T11:14:43Z

@marko-bekhta fixed the issues you raised. I'm done with my changes now.

As for the naming, I think I would go with property = field + getter. And be consistent with it all the way. I would also try to make the getter case clearer than before. It's full of special cases everywhere about it being a method or a property, it's kinda confusing IMHO.

gunnarmorling · 2018-06-04T12:22:17Z

Ok, thanks! Maybe let me know once it's sorted out and I could take another look.

gsmet · 2018-06-04T12:23:40Z

@gunnarmorling I'll ping you before we merge this one so that you can take a look if you want.

gunnarmorling · 2018-06-04T12:27:30Z

Excellent, thanks!

gsmet

@marko-bekhta nice progress, I added a couple of comments (one is hidden but it's still something to take into account).

Thanks!

gsmet · 2018-06-06T08:04:30Z

...e/src/main/java/org/hibernate/validator/internal/metadata/aggregated/ClassLevelMetaData.java

+ * @author Guillaume Smet
+ * @author Marko Bekhta
+ */
+public class ClassLevelMetaData extends AbstractConstraintMetaData {


+1 for this change.

I would call it ClassMetaData though, considering we don't have the Level part for the others.

gsmet · 2018-06-06T08:05:02Z

...main/java/org/hibernate/validator/internal/metadata/descriptor/ClassLevelDescriptorImpl.java

+ *
+ * @author Marko Bekhta
+ */
+public class ClassLevelDescriptorImpl extends ElementDescriptorImpl implements ElementDescriptor {


Same here about removing the Level part.

gsmet · 2018-06-06T08:05:31Z

...main/java/org/hibernate/validator/internal/metadata/descriptor/ClassLevelDescriptorImpl.java

+	@Override
+	public String toString() {
+		final StringBuilder sb = new StringBuilder();
+		sb.append( "ClassLevelDescriptorImpl" );


Let's use getClass().getSimpleName() here to avoid future inconsistencies.

gsmet · 2018-06-06T08:37:17Z

.../org/hibernate/validator/internal/cfg/context/FieldPropertyConstraintMappingContextImpl.java

+import org.hibernate.validator.internal.util.TypeResolutionHelper;
+
+/**
+ * Constraint mapping creational context implementation {@link PropertyConstraintMappingContextImpl} for


There's a word missing or a typo here.

gsmet · 2018-06-06T08:37:29Z

...org/hibernate/validator/internal/cfg/context/GetterPropertyConstraintMappingContextImpl.java

+import org.hibernate.validator.internal.util.TypeResolutionHelper;
+
+/**
+ * Constraint mapping creational context implementation {@link PropertyConstraintMappingContextImpl} for


gsmet · 2018-06-06T08:44:22Z

.../java/org/hibernate/validator/internal/cfg/context/PropertyConstraintMappingContextImpl.java

+		else {
+			throw LOG.getUnexpectedElementType( elementType, ElementType.FIELD, ElementType.METHOD );
+		}
+	}


I wonder if we should deprecate property(String property, ElementType type) and introduce field() and getter(). That would be more in line with what we do in the XML.

Then I would instantiate directly FieldPropertyConstraintMappingContextImpl and GetterPropertyConstraintMappingContextImpl in those methods and disambiguate the types and throw the exception in the deprecated property().

That if we take the road of having a specific DSL for JSON. My opinion is that it will be better. WDYT?

Applied the change in the last commit. As for the JSON and all that "free-form validation" I'd say that as a lot of stuff there would be different and probably tagged as @Incubating we'd better separate these things. We already know that cascading would be different and for properties we need to pass a property type.

gsmet · 2018-06-06T08:48:07Z

...main/java/org/hibernate/validator/internal/cfg/context/TypeConstraintMappingContextImpl.java

@@ -149,7 +150,7 @@ public MethodConstraintMappingContext method(String name, Class<?>... parameterT
 			throw LOG.getBeanDoesNotContainMethodException( beanClass, name, parameterTypes );
 		}

-		JavaBeanExecutable<?> javaBeanExecutable = JavaBeanExecutable.of( method );
+		JavaBeanMethod javaBeanExecutable = (JavaBeanMethod) JavaBeanExecutable.of( method );


Mmmh, let's instantiate a JavaBeanMethod directly with the constructor instead of casting then :)

And let's call the variable javaBeanMethod.

as method here can be either method or getter we couldn't just use a constructor hence I've added an overloaded version of JavaBeanExecutable#of() one takes Method and returns JavaBeanMethod which might also be a getter, and the other one is the one that we already have that can also handle constructors.

gsmet

@marko-bekhta great work. Added a couple of comments on your last commit.

gsmet · 2018-06-07T07:51:05Z

engine/src/main/java/org/hibernate/validator/cfg/context/PropertyTarget.java

+	 * <p>
+	 * Until this method is called constraints apply on class level. After calling this method constraints
+	 * apply on the specified field property.
+	 * </p>


Don't ask me why but in the javadoc, we don't close the <p>, we only open a new one when needed.

gsmet · 2018-06-07T07:52:10Z

engine/src/main/java/org/hibernate/validator/cfg/context/PropertyTarget.java

+	 * A given field may only be configured once.
+	 * </p>
+	 *
+	 * @param property The field name, that represents a property on which to apply the following constraints.


Minor nitpicking: no comma here, that directly qualifies the field name.

gsmet · 2018-06-07T07:56:00Z

...main/java/org/hibernate/validator/internal/cfg/context/TypeConstraintMappingContextImpl.java

+
+	@Override
+	public PropertyConstraintMappingContext field(String property) {
+		Contracts.assertNotNull( property, "The property name must not be null." );
 		Contracts.assertNotEmpty( property, MESSAGES.propertyNameMustNotBeEmpty() );


I think I would instead change Contracts.assertNotEmpty() to use StringHelper.isNullOrEmptyString().

gsmet · 2018-06-07T07:57:17Z

engine/src/main/java/org/hibernate/validator/cfg/context/PropertyTarget.java

+	 * @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.
+	 *


Let's remove this line.

gsmet · 2018-06-07T07:57:30Z

engine/src/main/java/org/hibernate/validator/cfg/context/PropertyTarget.java

+	 * to apply the following constraints.
+	 *
+	 * @return A creational context representing the selected getter property.
+	 *


Let's remove this line.

gsmet · 2018-06-07T07:59:26Z

engine/src/main/java/org/hibernate/validator/cfg/context/PropertyTarget.java

+	 * </p>
+	 *
+	 * @param property The getter property name (Java Bean notation), that represents a property on which
+	 * to apply the following constraints.


I think we should give an example. Something like: "(using the JavaBean convention, e.g. {@code name} to address {@code getName()})".

gsmet · 2018-06-08T08:41:20Z

...main/java/org/hibernate/validator/internal/cfg/context/TypeConstraintMappingContextImpl.java

@@ -133,8 +133,7 @@ public PropertyConstraintMappingContext property(String property, ElementType el

 	@Override
 	public PropertyConstraintMappingContext field(String property) {
-		Contracts.assertNotNull( property, "The property name must not be null." );
-		Contracts.assertNotEmpty( property, MESSAGES.propertyNameMustNotBeEmpty() );
+		Contracts.assertNeitherNullNorEmpty( property, MESSAGES.propertyNameMustBeNeitherNullNorEmpty() );


Hum, sorry if I wasn't clear but I did expect you to only change the method content, not the method name or the message.

Being not empty implies being not null. See NotEmptyValidatorForCharSequence for instance :).

marko-bekhta · 2018-06-10T22:25:03Z

@gsmet that's good. I've added one more commit for naming consistency - it should probably be squashed either into your naming commit or maybe into the first one? As for the PropertyAccessor it solves the problem and I cannot see a better solution atm.

…elated code

- based on the usage of the method we cannot receive methods into it, hence the check can be removed. And also the method can be named more specific to represent what it is really should be doing.

- changed Constrainable to Property to better represent the intended objects to be processed by the builder - removed unused parameters, fields and methods

…implementations

… field - create different implementations of PropertyConstraintMappingContextImpl for getter and field to separate the logic.

…mapping contexts

…) and getter() instead - add two new methods field() and getter() to replace deprecated property() - update property constraint mapping implementations to use generics and get rid of casting objects

… the same time - based on our usage of not empty check passed parameters should not be null as well hence the implementation of the check can use StringHelper.isNullOrEmptyString utility method.

We don't need the elements to be typed so let's be looser to open the gate for JSON properties (which will probably simply be treated as fields).

…s in property metadata - need to throw an exception rather than just create it - removed the usage of Optional as it isn't needed anymore

gunnarmorling

That looks pretty cool! A few comments inline! Apart from the comment API change I don't see anything critical, we'll see how the abstraction works once adding another (non-JavaBeans) implementation.

gunnarmorling · 2018-06-11T08:25:16Z

engine/src/main/java/org/hibernate/validator/cfg/context/PropertyTarget.java

 	 */
+	@Deprecated


@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.).

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.

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.

@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 ?

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.

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.

gunnarmorling · 2018-06-11T08:29:48Z

engine/src/main/java/org/hibernate/validator/internal/engine/ConstraintViolationImpl.java

+			Object value,
+			Path propertyPath,
+			ConstraintDescriptor<?> constraintDescriptor,
+			Object dynamicPayload) {


gunnarmorling · 2018-06-11T08:31:22Z

engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/ClassMetaData.java

+
+	@Override
+	public String toString() {
+		return "ClassLevelMetaData [type=" + getType() + "]]";


s/ClassLevelMetaData/ClassMetaData/

@marko-bekhta see, that's why I want us to use getClass().getSimpleName() :)

Fixed.

And I trusted IDEA to be good with such kind of refactorings!!! 😄 Thanks!

gunnarmorling · 2018-06-11T08:33:32Z

...ain/java/org/hibernate/validator/internal/metadata/core/AnnotationProcessingOptionsImpl.java

-			return annotationIgnoredForMembers.get( member );
+	public boolean areMemberConstraintsIgnoredFor(Constrainable constrainable) {
+		Class<?> clazz = constrainable.getDeclaringClass();
+		if ( annotationIgnoredForMembers.containsKey( constrainable ) ) {


I'd prefer to get() once and then check on null, so to avoid one lookup.

OK, this is a bit outside the scope of this PR as it's preexisting.

I opened https://hibernate.atlassian.net/browse/HV-1631. Will push an additional commit with this change.

gunnarmorling · 2018-06-11T08:33:39Z

...ain/java/org/hibernate/validator/internal/metadata/core/AnnotationProcessingOptionsImpl.java

-		if ( annotationIgnoresForReturnValues.containsKey( member ) ) {
-			return annotationIgnoresForReturnValues.get( member );
+	public boolean areReturnValueConstraintsIgnoredFor(Constrainable constrainable) {
+		if ( annotationIgnoresForReturnValues.containsKey( constrainable ) ) {


gunnarmorling · 2018-06-11T08:39:48Z

engine/src/main/java/org/hibernate/validator/internal/properties/Callable.java

+
+	boolean isPrivate();
+
+	String getSignature();


I suppose that's for logging only?

Not only. The signature is used a mean to identify the executable in various parts of the code.

gunnarmorling · 2018-06-11T08:40:10Z

engine/src/main/java/org/hibernate/validator/internal/properties/Constrainable.java

+/**
+ * @author Marko Bekhta
+ */
+public interface Constrainable {


A class-level comment would be nice.

gunnarmorling · 2018-06-11T08:40:40Z

engine/src/main/java/org/hibernate/validator/internal/properties/Constrainable.java

+
+	String getName();
+
+	Class<?> getDeclaringClass();


Doesn't this break the "abstraction"? I.e. what to return for a JSON type?

Mmmh, yes it does. I think @marko-bekhta had in mind to provide a PropertyHolder class here or something similar.

The class is used in some portions of the code where it's not really easy to abstract from it.

Let's see how it goes with the JSON PR and act accordingly.

I created https://hibernate.atlassian.net/browse/HV-1632 where I describe the challenges. It's really not a simple matter.

gunnarmorling · 2018-06-11T08:40:57Z

engine/src/main/java/org/hibernate/validator/internal/properties/ConstrainableType.java

+/**
+ * @author Marko Bekhta
+ */
+public interface ConstrainableType {


What is this doing?

For now, nothing. I think Marko wanted a common interface for the JavaBean and the upcoming PropertyHolder.

Let's add a comment.

gsmet · 2018-06-11T10:56:56Z

And it's merged!

@marko-bekhta thanks for the hard work on this one. @gunnarmorling thanks for the review.

gsmet force-pushed the HV-1363-abstraction branch 2 times, most recently from 914ae77 to f7d2382 Compare June 4, 2018 10:52

gsmet reviewed Jun 4, 2018

View reviewed changes

gsmet added Changes suggested 6.1 labels Jun 4, 2018

marko-bekhta commented Jun 4, 2018

View reviewed changes

gsmet force-pushed the HV-1363-abstraction branch from f7d2382 to 9d4d486 Compare June 4, 2018 11:06

gsmet force-pushed the HV-1363-abstraction branch from 9d4d486 to 6b119f8 Compare June 4, 2018 11:12

gsmet force-pushed the HV-1363-abstraction branch from 6b119f8 to 4c8d377 Compare June 4, 2018 12:34

marko-bekhta force-pushed the HV-1363-abstraction branch 2 times, most recently from 0902c7e to 730323b Compare June 6, 2018 07:28

gsmet reviewed Jun 6, 2018

View reviewed changes

gsmet changed the title ~~HV-1363 Built an abstraction over reflection in engine code~~ HV-1623 Build an abstraction over reflection in engine code Jun 6, 2018

marko-bekhta force-pushed the HV-1363-abstraction branch from 730323b to b6f913d Compare June 6, 2018 21:44

gsmet reviewed Jun 7, 2018

View reviewed changes

gsmet self-assigned this Jun 7, 2018

marko-bekhta force-pushed the HV-1363-abstraction branch 2 times, most recently from a4e97df to 74c7d49 Compare June 7, 2018 14:27

gsmet reviewed Jun 8, 2018

View reviewed changes

gsmet force-pushed the HV-1363-abstraction branch 4 times, most recently from 52471dd to 0b0438e Compare June 8, 2018 18:13

gsmet removed the Changes suggested label Jun 9, 2018

marko-bekhta and others added 23 commits June 11, 2018 09:58

HV-1623 Build abstraction over reflection in ConstraintLocation and r…

53ea896

…elated code

HV-1623 Reintroduce field and getter constraint locations

af67fd7

HV-1623 Clean ups around cascadable properties in PropertyMetaData

556d277

HV-1623 Remove redundant check in ConfiguredConstraint

dbffeba

- based on the usage of the method we cannot receive methods into it, hence the check can be removed. And also the method can be named more specific to represent what it is really should be doing.

HV-1623 Clean up property metadata

3063210

- changed Constrainable to Property to better represent the intended objects to be processed by the builder - removed unused parameters, fields and methods

HV-1623 Add additional checks before casting

660b64c

HV-1623 Remove ConstrainableFormatter and push the logic to toString …

0b0835c

…implementations

HV-1623 Wrap ElementType with ConstraintLocationKind

fc0824a

HV-1623 More abstraction work around executables

9376899

HV-1623 Propagate ConstraintLocationKind as far as possible

f44ecb4

HV-1623 Encapsulate ConstraintLocationKind inside ConstraintLocation

418e010

HV-1623 Extract class-level metadata from property metadata case

8bb498e

HV-1623 Create different programmatic mapping contexts for getter and…

cace416

… field - create different implementations of PropertyConstraintMappingContextImpl for getter and field to separate the logic.

HV-1623 Make parameters more specific in programmatic API executable …

34e9db9

…mapping contexts

HV-1623 Deprecate property() in programmatic API and introduce field(…

cd64f25

…) and getter() instead - add two new methods field() and getter() to replace deprecated property() - update property constraint mapping implementations to use generics and get rid of casting objects

HV-1623 Replace empty check in Contracts with null and empty check at…

49c11c6

… the same time - based on our usage of not empty check passed parameters should not be null as well hence the implementation of the check can use StringHelper.isNullOrEmptyString utility method.

HV-1623 Introduce getter as a constraint location kind

7475853

HV-1623 Introduce ConstrainedElementKind.GETTER

e5b7c8d

HV-1623 Stop the propagation of JavaBeanField/Getter

3a1944c

We don't need the elements to be typed so let's be looser to open the gate for JSON properties (which will probably simply be treated as fields).

HV-1623 Rename a couple of classes to Abstract*

ce6f342

HV-1623 Introduce Field and Getter as subclasses of Property

6d7b067

HV-1623 Throw exception in case of unexpected constrainedElement clas…

edb5efd

…s in property metadata - need to throw an exception rather than just create it - removed the usage of Optional as it isn't needed anymore

HV-1623 Make the fields/getters accessible lazily

0332e6c

gsmet force-pushed the HV-1363-abstraction branch from 8f68c33 to 0332e6c Compare June 11, 2018 08:06

HV-1623 Reduce the memory footprint of the new reflection abstraction

be6861c

gunnarmorling reviewed Jun 11, 2018

View reviewed changes

gsmet closed this Jun 11, 2018

		this.cascadingMetaData = cascadingMetaData;
		this.constraintLocationKind = property instanceof JavaBeanField ? ConstraintLocationKind.PROPERTY : ConstraintLocationKind.METHOD;


		TypeVariable<?>[] getTypeParameters();

		<A extends Annotation> A getAnnotation(Class<A> annotationClass);

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

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

Conversation

marko-bekhta commented Jun 1, 2018 • edited by gsmet Loading

marko-bekhta commented Jun 1, 2018

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marko-bekhta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marko-bekhta commented Jun 4, 2018

gsmet commented Jun 4, 2018

gsmet commented Jun 4, 2018

gunnarmorling commented Jun 4, 2018 via email

gsmet commented Jun 4, 2018

gunnarmorling commented Jun 4, 2018 via email

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marko-bekhta commented Jun 10, 2018

gunnarmorling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marko-bekhta commented Jun 1, 2018 •

edited by gsmet

Loading

gsmet Jun 4, 2018 •

edited

Loading

gsmet Jun 11, 2018 •

edited

Loading

gsmet Jun 11, 2018 •

edited

Loading