HV-840 [follow-up] Annotation Processor doesn't detect errors in parameter constraints in inheritance hierarchies #584

gsmet · 2016-11-22T13:26:10Z

https://hibernate.atlassian.net/browse/HV-840
Follow-up of: HV-840 Annotation Processor doesn't detect errors in parameter constraints in inheritance hierarchies #524

So I did a couple of fixes and thought we might as well start with a clean review slate instead of having dozens of comments piling up.

This is all based on what you did. As you suspected it, the inheritance tree was what I had in mind. I fixed a couple of issues though, the most important one being that you ended the exploration when a superclass did not have the method, while you should also have explored the superclasses of this superclass.

I think we now have something working pretty well.

Sorry again about the delay, we had a few bad surprises with our upgrade in WildFly our it drained a lot of time and efforts.

Comments welcome!

--
Guillaume

…riding check

…ch goes through classes and checks all methods if they are correctly overridden

…cted how annotations mirrors are compared.

… is verified.

…ds inheritance tree.

marko-bekhta · 2016-11-22T18:47:14Z

...cessor/src/main/java/org/hibernate/validator/ap/classchecks/AbstractMethodOverrideCheck.java

+				collectOverriddenMethodsInInterfaces( overriddenMethod, interfaceTypeElement, methodInheritanceTreeBuilder );
+			}
+			else {
+				collectOverriddenMethodsInInterfaces( overridingMethod, interfaceTypeElement, methodInheritanceTreeBuilder );


it looks that collectOverriddenMethodsInInterfaces can be moved out of if else as it is called in both cases ?

There's a slight difference in the calls: one is using overriddenMethod and the other overridingMethod. I thought it was clearer this way than to declare another variable. Your question makes me think I might be wrong. WDYT?

OK, I fixed it, it seems to be clearer now.

ahhaa ! when I was looking at it these two lines looked the same to me :)

marko-bekhta · 2016-11-22T18:53:29Z

...cessor/src/main/java/org/hibernate/validator/ap/classchecks/AbstractMethodOverrideCheck.java

 			if ( !element.getKind().equals( ElementKind.METHOD ) ) {
 				continue;
 			}
-			if ( elementUtils.overrides( currentMethod, (ExecutableElement) element, currentTypeElement ) ) {
+			if ( elementUtils.overrides( currentMethod, (ExecutableElement) element, getEnclosingTypeElement( currentMethod ) ) ) {


maybe get enclosing element before the loop ? or this operation is not very expensive ? Something like:

TypeElement methodEnclosingType = getEnclosingTypeElement( currentMethod ); for (...){ .... if ( elementUtils.overrides( currentMethod, (ExecutableElement) element, methodEnclosingType ) ) {

Right! I'll fix it, thanks for noticing.

marko-bekhta · 2016-11-22T19:04:23Z

...ation-processor/src/main/java/org/hibernate/validator/ap/classchecks/AbstractClassCheck.java

 * </p>
 *
 * @author Marko Bekhta
 */
 public abstract class AbstractClassCheck implements ClassCheck {

 	@Override
-	public Collection<ConstraintCheckIssue> checkMethod(ExecutableElement element) {
+	public Set<ConstraintCheckIssue> checkMethod(ExecutableElement element) {


just out of interest - did you changed to Set to be more consistent with the existing checks or is there any other reasons ? I was thinking that if we are not using any Set specific behavior a more general collection interface can be used (that's just an explanation why I used Collection in the first place)

Yes, it's just for the sake of consistency. I must admit I wouldn't have changed it if I didn't have other changes to do.

To be honest, if I had to do it from the ground up, I think I would have used ArrayList everywhere to have the result as predictable as possible. I don't think Sets are of much value here.

marko-bekhta · 2016-11-22T19:11:43Z

hi @gsmet !

All looks great ! You definitely are much better at naming methods and variables :) and a great improvement to MethodInheritanceTree as well.

Also I can see that it seems like I still need to do some changes to my IDE formatting as there still are some differences :)

As for the delays - it's all fine and I completely understand it. I myself got stuck with my work and wasn't able to spent much time on HV related items :)

Have a nice day,
Marko

gsmet · 2016-11-23T13:15:27Z

@marko-bekhta thanks for the review. I pushed a commit fixing your remarks. Is it better now? (I'll probably squash it before merging)

marko-bekhta · 2016-11-23T13:57:19Z

@gsmet yes ! looks great !

gunnarmorling

@gsmet I looked mainly at the tests, not the impl so much. The expected behavior in the tests look right. Two comments inline.

gunnarmorling · 2016-11-23T14:14:42Z

...on-processor/src/test/java/org/hibernate/validator/ap/ConstraintValidationProcessorTest.java

+		);
+
+		assertEquals( diagnostics.getDiagnostics().get( 0 ).getMessage( Locale.getDefault() ),
+				"Parameters of method \"doSomething((@javax.validation.constraints.NotNull :: java.lang.String))\" do not respect the inheritance rules. " +


The error will be shown in the context of the method. Do we need to repeat the entire signature? Shouldn't the compiler itself point it out?

gunnarmorling · 2016-11-23T14:15:36Z

...sor/src/test/java/org/hibernate/validator/ap/testmodel/overriding/MethodOverridingTests.java

+	public static class MethodOverridingTestCase2Sub extends MethodOverridingTestCase2 {
+
+		@Override
+		public void doSomething(@NotNull @NotBlank @Size(max = 10) String param) {


Is there a strong need to check for this? Technically, it doesn't represent any constraint strengthening.

Well, it does not respect the inheritance rules we defined. The rule is that you cannot define constraints in the method hierarchy if there are already one method defining constraints in the method hierarchy. That's what we implemented here.

And it's definitely easier to throw an error for this than trying to determine what is or what is not a constraint strengthening.

At runtime, we'd allow that one. See OverridingMethodMustNotAlterParameterConstraints and how it's calling !method.isEquallyParameterConstrained(). That's primarily due to the behavior of Weld proxy classes though which would copy the annotations from proxied classes verbatim to the method definition on the proxy.

I'd prefer behavior of the AP to closely mimic the runtime engine, but if it's too cumbersome to do, we can leave as you've proposed it. It's in the sense of the spec for sure.

gunnarmorling · 2016-11-23T14:42:32Z

annotation-processor/src/main/java/org/hibernate/validator/ap/AbstractElementVisitor.java

+import org.hibernate.validator.ap.util.MessagerAdapter;
+
+/**
+ * An abstract {@link javax.lang.model.element.ElementVisitor} that should be used for implementation


No biggie, but we can use simple names in JavaDocs (and import them), too. Reads a tad nicer.

processor documentation

diagnostic

gsmet · 2016-11-30T12:57:38Z

@marko-bekhta ... and merged! Thanks for your tenacity on this subject, we finally got this in!

marko-bekhta · 2016-11-30T13:11:57Z

@gsmet Great ! :)

marko-bekhta and others added 10 commits November 22, 2016 14:19

HV-840 - added test-utils to pom file - added test cases for the over…

00edd47

…riding check

HV-840 implemented method overriding checks - added a new visitor whi…

1a0edf2

…ch goes through classes and checks all methods if they are correctly overridden

HV-840 added one more test case with non constraint annotation. Corre…

eae6109

…cted how annotations mirrors are compared.

HV-840 Updated documentation

b170ce1

HV-840 Various minor fixes

9a24c18

HV-840 added few more test cases. Changed how parallel implementation…

7aa0aa4

… is verified.

HV-840 made diagnostic exceptions ordered in tests

6262509

HV-840 cleaned up some code duplications

dfaac3f

HV-840 Reimplemented checks for overridden methods

8251bcb

HV-840 Changed how overridden methods are found. Add overridden metho…

f6c7845

…ds inheritance tree.

gsmet mentioned this pull request Nov 22, 2016

HV-840 Annotation Processor doesn't detect errors in parameter constraints in inheritance hierarchies #524

Closed

gsmet added the Ready for review label Nov 22, 2016

marko-bekhta reviewed Nov 22, 2016

View reviewed changes

marko-bekhta approved these changes Nov 22, 2016

View reviewed changes

gunnarmorling reviewed Nov 23, 2016

View reviewed changes

gsmet added 3 commits November 30, 2016 12:19

HV-840 Various fixes to the logic and other improvements

9babf9c

HV-840 Fix minor preexisting formatting issue in the annotation

a3b68e6

processor documentation

HV-840 Simplify error messages as the context is already present in the

354916d

diagnostic

gsmet force-pushed the ap-overriding branch from be7e579 to 354916d Compare November 30, 2016 11:33

gsmet closed this Nov 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HV-840 [follow-up] Annotation Processor doesn't detect errors in parameter constraints in inheritance hierarchies #584

HV-840 [follow-up] Annotation Processor doesn't detect errors in parameter constraints in inheritance hierarchies #584

gsmet commented Nov 22, 2016

marko-bekhta Nov 22, 2016

gsmet Nov 23, 2016

gsmet Nov 23, 2016

marko-bekhta Nov 23, 2016

marko-bekhta Nov 22, 2016

gsmet Nov 23, 2016

marko-bekhta Nov 22, 2016

gsmet Nov 23, 2016

marko-bekhta commented Nov 22, 2016

gsmet commented Nov 23, 2016

marko-bekhta commented Nov 23, 2016

gunnarmorling left a comment

gunnarmorling Nov 23, 2016

gunnarmorling Nov 23, 2016

gsmet Nov 23, 2016

gunnarmorling Nov 30, 2016

gunnarmorling Nov 23, 2016

gsmet commented Nov 30, 2016

marko-bekhta commented Nov 30, 2016

HV-840 [follow-up] Annotation Processor doesn't detect errors in parameter constraints in inheritance hierarchies #584

HV-840 [follow-up] Annotation Processor doesn't detect errors in parameter constraints in inheritance hierarchies #584

Conversation

gsmet commented Nov 22, 2016

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 Nov 22, 2016

gsmet commented Nov 23, 2016

marko-bekhta commented Nov 23, 2016

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

gsmet commented Nov 30, 2016

marko-bekhta commented Nov 30, 2016