Search for annotations on enclosing classes with MergedAnnotations API

[sbrannen]

Overview

The MergedAnnotations API currently supports four search strategies via the SearchStrategy enum; however, none of those support searches on enclosing classes.

In a typical Spring application, it is not necessary to search for an annotation on an enclosing class; however, in order to provide first-class support for Spring configuration in @Nested test class hierarchies in JUnit Jupiter, we need a mechanism for searching on enclosing classes as well (see gh-19930). There may be other use cases as well.

Proposal

Introduce a new SearchStrategy for the MergedAnnotations API that supports searching for annotations on "enclosing classes". This would likely be in addition to the semantics currently associated with the EXHAUSTIVE enum.

In one way, this would be EXHAUSTIVE++, but I don't know that we want to change the current semantics of EXHAUSTIVE. So, perhaps something like EXHAUSTIVE_PLUS_ENCLOSING_CLASSES would be more suitable.

Providing such support in MergedAnnotations could alleviate some of the challenges of providing custom workarounds as can be seen in the current work for gh-19930.

[philwebb]

On the surface this seems quite reasonable, but I think there's a few things we should consider before adding an extra strategy. The first thing I think we need to define is the ordering of annotations when enclosing classes are considered. Currently I think we we order like this:

• All direct annotations
• All meta-annotations
• Recursively for all supercalsses and interfaces

We will need to extended this to include the enclosing classes and define it in a way that makes sense.

For example:

@Example("animal")
public static class Animal {
}

@Example("cat")
public static class Cat extends Animal {

    @Example("garfield")
    public static class Garfield extends Animal {
    }

}

Should the order of annotations for Garfield be "garfield", "animal", "cat", "animal" or should it be "garfield", "cat", "animal", "animal"?

We probably also need to consider the case where an inner class extends the outer class so could end up both inheriting and contained by the same class.

Finally, I guess this strategy would only make sense for Class sources. We're not going to attempt to match methods in the enclosing class.

[sbrannen]

On the surface this seems quite reasonable, but I think there's a few things we should consider before adding an extra strategy. The first thing I think we need to define is the ordering of annotations when enclosing classes are considered. Currently I think we we order like this:

• All direct annotations
• All meta-annotations
• Recursively for all superclasses and interfaces

Yes, that's typically what we do.

Should the order of annotations for Garfield be "garfield", "animal", "cat", "animal" or should it be "garfield", "cat", "animal", "animal"?

Assuming you didn't mean to make the Garfield class static, I would expect the order to be "garfield", "animal", "cat", "animal".

Though I'm wondering if we should really revisit classes with constructs like that (i.e., when an enclosing class extends the same class that the enclosed member class extends). In other words, I'm wondering if the result shouldn't simply be "garfield", "animal", "cat". With this example, we only see one additional annotation/value duplicated, but if a superclass like Animal were annotated with multiple annotations (e.g., Spring test configuration) the entire set of annotations would show up twice in the MergedAnnotations, presumably with aggregateIndex equal to 1 and equal to 3.

Technically, I suppose the caller could apply uniqueness to the streamed annotations or filter out entire aggregate indexes with the same source (declaring class).

Long story, short: what do you think about "not revisiting classes"?

---

Although I didn't spell it out in this issue's description, I think this new search algorithm should apply to what people traditionally refer to as inner classes, where such classes are technically non-static member classes according to the JLS. See org.springframework.util.ClassUtils.isInnerClass(Class<?>). With that in mind, EXHAUSTIVE_PLUS_ENCLOSING_CLASSES might be misleading, since we really want to limit this to "enclosing classes for non-static member classes", which is a mouthful. Maybe EXHAUSTIVE_PLUS_ENCLOSING_CLASSES_FOR_INNER_CLASSES is better (even if rather long), or perhaps we stick with EXHAUSTIVE_PLUS_ENCLOSING_CLASSES and just document the scope.

In light of recent offline discussions, we may wish to rename EXHAUSTIVE to TYPE_HIERARCHY. Then TYPE_HIERARCHY_AND_ENCLOSING_CLASSES might be reasonable.

We probably also need to consider the case where an inner class extends the outer class so could end up both inheriting and contained by the same class.

I know that's technically possible, but I wonder how often people actually do that (or would do that for code on which they want Spring to find annotations). In any case, this comes back to the question I raised above: do we want to revisit classes?

Finally, I guess this strategy would only make sense for Class sources. We're not going to attempt to match methods in the enclosing class.

Yes, I agree: limit it to non-static member class sources.

[philwebb]

Long story, short: what do you think about "not revisiting classes"?

I remember considering this when implementing the search algorithm but decided against it at the time because adds some additional complication. I think I was also worried about the performance /garbage impact of needing to track visited class, but I must confess that I never actually measured that.

[sbrannen]

Reopening due to the following issue.

Summary: annotations on enclosing classes are not found if the "start" class is an inner class that is not annotated.

In AnnotationEnclosingClassSample, if you add NonAnnotatedEnclosedInnerInner to EnclosedInner as follows:

@EnclosedOne
public class AnnotationEnclosingClassSample {

	@EnclosedTwo
	public class EnclosedInner {

		@EnclosedThree
		public class EnclosedInnerInner {
		}

		public class NonAnnotatedEnclosedInnerInner {
		}
	}
}

Then, executing the following passes.

@Test
public void typeHierarchyWithEnclosedStrategyOnEnclosedInnerClassFindsAnnotations() {
	Class<?> source = AnnotationEnclosingClassSample.EnclosedInner.EnclosedInnerInner.class;
	List<String> list = MergedAnnotations.from(source,
		SearchStrategy.TYPE_HIERARCHY_AND_ENCLOSING_CLASSES).stream().map(
			mergedAnnotation -> mergedAnnotation.getType().getSimpleName()).collect(toList());

	assertThat(list).containsExactly("EnclosedThree", "EnclosedTwo", "EnclosedOne");
}

But executing the following fails with an empty list.

@Test
public void typeHierarchyWithEnclosedStrategyOnNonAnnotatedEnclosedInnerClassFindsAnnotations() {
	Class<?> source = AnnotationEnclosingClassSample.EnclosedInner.NonAnnotatedEnclosedInnerInner.class;
	List<String> list = MergedAnnotations.from(source,
		SearchStrategy.TYPE_HIERARCHY_AND_ENCLOSING_CLASSES).stream().map(
			mergedAnnotation -> mergedAnnotation.getType().getSimpleName()).collect(toList());

	assertThat(list).containsExactly("EnclosedTwo", "EnclosedOne");
}