Consider not overriding meta-annotation attributes if empty [SPR-11709]

[spring-projects-issues]

Gary Russell opened SPR-11709 and commented

In AnnotationReadingVisitorUtils.getMergedAnnotationAttributes(), the attributes closer to the annotated element override those in the hierarchy.

It would be useful if this only occurred if the closer attribute was not-empty.

For example:

@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
@MessagingGateway(defaultRequestChannel = "gatewayChannel",
		defaultHeaders = @GatewayHeader(name = "foo", value = "FOO"))
public static @interface TestMessagingGateway {

	String defaultRequestChannel() default "";
}

This allows the user to override the "default" setting in the meta-annotation, but the current logic replaces the default with "" if the user does not specify a value when using the annotation.

The same applies to attributes with array values; consider preventing the override of a higher-up array with an empty array closer to the declaration.

---

Affects: 4.0.3

Issue Links:

• Support meta-annotation attribute overrides in ASM-based annotation processing [SPR-11574] #16198 Support meta-annotation attribute overrides in ASM-based annotation processing
• AnnotationReadingVisitorUtils.getMergedAnnotationAttributes() overrides values it should not [SPR-11649] #16272 AnnotationReadingVisitorUtils.getMergedAnnotationAttributes() overrides values it should not
• AnnotatedElementUtils fails to find annotations on abstract, bridge, or interface methods [SPR-12738] #17335 AnnotatedElementUtils fails to find annotations on abstract, bridge, or interface methods
• INT-3376 Merge Messaging Meta-Annotation Attributes

0 votes, 5 watchers

[spring-projects-issues]

Juergen Hoeller commented

This is an interesting one, and I can definitely see your argument here. The question is basically how to design a custom optional argument that only overrides if it's actually set, with the original default value from the overridden annotation applying otherwise.

However, there are also cases where you may want to explicitly override with an empty value, resetting an original default value through an override with "" or an empty array... We could theoretically differentiate between an explicitly specified empty value and a defaulted empty value in the annotation declaration, but that's not very clean - neither conceptually nor implementation-wise - and should therefore be a last resort only.

This might be a case where an explicit attribute-level annotation would help, clearly indicating a specific attribute's override intention. I have been asked before whether we would support such an annotation for general validation reasons, in a JDK @Override / @FunctionalInterface style where the annotation is optional on top of a convention... with the annotation just used to enforce validation at the tooling level. Attribute defaulting could be clearly indicated there, without us having to do convention-based guessing.

Juergen

[spring-projects-issues]

Gary Russell commented

Thanks; it would be great if we can come up with a scheme to conditionally determine whether an empty value should override that on the meta-annotation.

[spring-projects-issues]

Sam Brannen commented

Hi guys,

The @TestMessagingGateway example you have provided is actually an example of how not to define a custom composed annotation with attribute overrides. Specifically, declaring defaultRequestChannel both via the original annotation (as a meta-annotation) and as an attribute override will never work, neither for reflection-based nor ASM-based annotation processing.

(note: when I say "never", I obviously mean for the status quo)

Without some new mechanism in place, the best practice is to:

1. Declare non-overridable attributes via the meta-annotation on the composed annotation.
2. Redeclare overridable attributes via the composed annotation, copying the defaults verbatim from the target meta-annotation.

This best practice can be seen in Spring Boot's @SpringApplicationConfiguration.

Specifically, in @SpringApplicationConfiguration, the loader attribute is not meant to be overridden and is therefore configured via @ContextConfiguration as a meta-annotation. Consequently, the loader attribute is not redeclared as an attribute of @SpringApplicationConfiguration. Furthermore, @SpringApplicationConfiguration redeclares all other attributes of @ContextConfiguration verbatim (i.e., via copy-n-paste).

Now... I can understand that the copy-n-paste approach is perhaps not very desirable for annotations that have several attributes, but it is currently the only way to achieve your goal.

Note as well that redeclared attributes in composed annotations can optionally change the defaults of the target meta-annotation while still allowing for attribute overriding.

Regards,

Sam

[spring-projects-issues]

Sam Brannen commented

In other words, I would consider your configuration broken, and this is how you fix your annotation:

@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
@MessagingGateway(defaultHeaders = @GatewayHeader(name = "foo", value = "FOO"))
public @interface TestMessagingGateway {

	String defaultRequestChannel() default "gatewayChannel";
}

Regards,

Sam

[spring-projects-issues]

Sam Brannen commented

In AnnotationReadingVisitorUtils.getMergedAnnotationAttributes(), the attributes closer to the annotated element override those in the hierarchy.

Just to be clear... this is not specific to ASM-based annotation processing. The same is true for reflection-based processing with AnnotationUtils, etc.

[spring-projects-issues]

Sam Brannen commented

Juergen Hoeller,

The question is basically how to design a custom optional argument that only overrides if it's actually set, with the original default value from the overridden annotation applying otherwise.

I hope that my explanation above explains how to achieve this.

Of course, with the status quo, getting the "original default value from the overridden annotation applying otherwise" requires that one manually copy all such declarations from the overridden annotation. It works, but it's not ideal. However, as you suggested, introducing "an explicit attribute-level annotation" might be an elegant solution.

- Sam

[spring-projects-issues]

Gary Russell commented

Thanks, Sam; that makes sense; and I can see how we can achieve the desired behavior this way.

BTW, one thing I noticed while researching this is that AnnotationUtils.getAnnotation() doesn't traverse the meta-annotation hierarchy, it only looks at the immediate meta-annotation. This is obviously different to the asm-based AnnotationMetadataReadingVisitor.isAnnotated() which can detect meta-annotated meta-annotations, no matter how far away from the direct annotation.

I presume this is intentional but it makes it difficult to handle and I had to roll my own code to recurse up the chain; I'd be interested in your (and Juergen Hoeller's) comments as to whether this (proposed) implementation is naive...

https://github.com/garyrussell/spring-integration/blob/7e05baf183d34ac001c8b126d568486e631a7dc4/spring-integration-core/src/main/java/org/springframework/integration/config/annotation/MessagingAnnotationPostProcessor.java#L193

Note that it stops after it finds the first reference to the required annotation and does not deal with the case that the target annotation might exist multiple times in the hierarchy. This is fine for SI as we'll just document the limitation.

Any suggestions for improvements are appreciated.

Thanks.

[spring-projects-issues]

Sam Brannen commented

BTW, one thing I noticed while researching this is that AnnotationUtils.getAnnotation() doesn't traverse the meta-annotation hierarchy, it only looks at the immediate meta-annotation.

Please note that AnnotationUtils provides two distinct sets of methods: get\*() and find\*(). The former does not traverse class hierarchies or meta-annotations; whereas, the latter does.

Excerpt from the class-level Javadoc for AnnotationUtils:

You can still explicitly choose between a _get_ lookup on the given class level only (`getAnnotation(Method, Class)`) and a _find_ lookup in the entire inheritance hierarchy of the given method (`findAnnotation(Method, Class)`).

Thus if you are currently invoking getAnnotation() in your code, you should likely switch to findAnnotation().

Please let us know if this helps.

Thanks,

Sam

[spring-projects-issues]

Sam Brannen commented

I presume this is intentional...

It's only partially intentional.

We actually already have support for this in AnnotatedElementUtils.isAnnotated(); however, due to #16219 this functionality is disabled until Spring Framework 4.1 (i.e., AnnotatedElementUtils.isAnnotated() passes false to its internal process() method for the traverseClassHierarchy flag).

Instead of forcing you to roll your own code, we could consider introducing an overloaded isAnnotated() method in AnnotatedElementUtils that accepts a boolean traverseClassHierarchy flag. This could serve as a work-around until #16221 is addressed.

Thoughts?

Sam

[spring-projects-issues]

Sam Brannen commented

Gary Russell, if the information provided in my previous comments does not help you solve your issue regarding your custom algorithm for finding an annotation on a method (potentially as a nested meta-annotation), would you please open a separate JIRA issue to address that topic?

Thanks,

Sam

[spring-projects-issues]

Gary Russell commented

Hi Sam Brannen,

I am traveling at the moment; I'll take a look tonight or tomorrow, unless Artem Bilan beats me to it.

Gary

[spring-projects-issues]

Artem Bilan commented

Hi, Sam!

Thank you, your help is appreciated.
However our task to have entire annotations hierarchy and traverse their attributes.
E.g.

@interface A {
     String foo();
}

@A(foo = "bar")
@interface B {
     String foo() default "";
}

@B
@interface C {
}

AnnotatedElementUtils.isAnnotated(method, A.annotationType().getName()) says here us that the method is candidate to process (looks like AnnotationUtils.findAnnotation(Method, Class) doesn't take a look into meta-annotations), but further we build the top-bottom list of those annotations and use our method resolveAttribute(List<Annotation> annotations, String name, Class<T> requiredType) to get the value of framework attribute. We iterate those annotation hierarchy till attribute is empty (String or Array).

What is good here the expression AnnotationUtils.findAnnotation(annotation.annotationType(), annotationTypeToFind) does the stuff for meta-annotation search.

Take a look, please, to the polishing for our PR: spring-projects/spring-integration#1132.

Hope Gary will add some comments on the matter.

However I don't see now so much issues to disturb you.

[spring-projects-issues]

Gary Russell commented

Thanks, Sam, I think we have a way forward with your suggested use of overrides in your first and second comments.

For now, we are just not going to allow override to "" and document the limitation (it generally doesn't make sense in SI anyway).

If Spring Framework 4.1 (or later) provides some mechanism to support that, we can move to it at that time.

We'll just use our local work-around until then rather than trying to synch. up with a 4.0.x SF version.

Thanks again

Gary

[spring-projects-issues]

Sam Brannen commented

Artem Bilan,

Just to reiterate, the following is not the proper way to declare foo = "bar" in @B, since default "" overrides the declaration of foo = "bar" in @A(foo = "bar").

@A(foo = "bar")
@interface B {
     String foo() default "";
}

Instead, you have to declare B like this:

@A
@interface B {
     String foo() default "bar";
}

[spring-projects-issues]

Sam Brannen commented

Artem Bilan,

looks like AnnotationUtils.findAnnotation(Method, Class) doesn't take a look into meta-annotations

That was true when this issue was first raised; however, this problem has been fixed in #17534.

Regards,

Sam

[spring-projects-issues]

Sam Brannen commented

Gary Russell & Artem Bilan, you might be interested in knowing that AnnotatedElementUtils provides first-class support for finding annotations (and meta-annotations) on interfaces and within class hierarchies as of Spring Framework 4.2.

For details, please read my comments in SPR-12738.

Pay special attention to the new findAnnotationAttributes() method in AnnotatedElementUtils!

getAnnotationAttributes() has always had shortcomings and will remain so for backwards compatibility. So, findAnnotationAttributes() is probably what you want to use going forward.

Regards,

Sam

p.s. there is currently no equivalent of isAnnotated() with "find semantics" in AnnotatedElementUtils, but findAnnotationAttributes() != null is logically equivalent to "is annotated 'using find semantics'".

[spring-projects-issues]

Sam Brannen commented

I'm resolving this issue as "Works as Designed".

See previous comments detailing best practices for supporting annotation attribute overrides with required vs. default semantics.

If someone feels that Spring needs an "explicit attribute-level annotation ... that clearly indicates a specific attribute's override intention" (as suggested by Juergen Hoeller), feel free to open a separate JIRA issue.

Regards,

Sam

[spring-projects-issues]

Artem Bilan commented

Sam Brannen, thank you very much for such a comprehensive support and explanations!

Indeed, we must be consistent and follow with standard rules on the matter.

We'll take a look to our engine what should be changed in line of Spring Framework behavior.