Add getRoot() to MergedAnnotation

[philwebb]

Currently MergedAnnotation has a getParent() method but I think it would also be useful to add a getRoot() method as well. Using the API, I've quite often wanted to show details of the actual user annotation and not the meta-annotation.

[sbrannen]

This proposal is similar to the feature set of MetaAnnotationUtils.AnnotationDescriptor from spring-test.

As such, I would also find it quite useful to have comparable features in MergedAnnotation.

I suppose the mappings would be as follows, but please correct me if my interpretation is wrong.

AnnotationDescriptor	MergedAnnotation
getRootDeclaringClass()	getSource()
?	getParent()
getDeclaringClass()	?
getComposedAnnotation	getRoot()
getAnnotation()	synthesize()

While looking at that comparison table, however, a few things come to mind.

1. Does synthesize() always synthesize the annotation? In other words, if there is no need to synthesize a MergedAnnotation that's backed by a real annotation, can't the real annotation just be returned without synthesizing it (or rather synthesizing only if it is in fact "synthesizable" (i.e., has aliased attributes))? (this might just be a documentation issue)
2. The current implementation of getParent() actually returns the immediate "child", since annotation hierarchies are top-down analogous to class hierarchies. So I think we need to rename this method.
3. I feel that getRoot() may need an additional bit of information to describe what it is the root of. For example, by getRoot() you might actually mean getLeafAnnotation() or something like that.

In general, I think we should take some time to rethink the names of these methods and what they actually return, considering root annotated elements (like a method or class), what it means to be a parent or child, considering that annotation hierarchies are top-down and can have multiple levels, etc.

Admittedly, I had a difficult time coming up with the names in MetaAnnotationUtils.AnnotationDescriptor, so maybe we can learn from that and come up with something even better (more consistent / easier to reason about) in MergedAnnotation.

[sbrannen]

Regarding the current implementation/naming of getParent(), I added the following test to MergedAnnotationsTests to verify the status quo.

@Test
public void getParent() {
	MergedAnnotations annotations = MergedAnnotations.from(ComposedTransactionalComponentClass.class);
	assertThat(annotations.get(TransactionalComponent.class).getParent().getType())
			.isEqualTo(ComposedTransactionalComponent.class);
}

That test passes as-is, but I would expect a call to getParent() for @TransactionalComponent to return a list/array containing @Transactional and @Component; whereas, I would expect a method named something like getChild() to return @ComposedTransactionalComponent (as getParent() does now).

Rationale: in terms of object oriented inheritance terminology, the @ComposedTransactionalComponent "is a" @TransactionalComponent, not the other way around.

[sbrannen]

FWIW, I realize now that AnnotationDescriptor.getRootDeclaringClass() should really have been called getLeafDeclaringClass() or something along those lines, but that ship sailed a long time ago.

In any case, let's learn from that and not make the same mistake again. 😉

[philwebb]

Taking the example from AnnotationDescriptor and extending it a little. Given:

@Transactional
@Retention(RetentionPolicy.RUNTIME)
public @interface MyTransactional { }

@MyTransactional
@ContextConfiguration({"/test-datasource.xml", "/repository-config.xml"})
@Retention(RetentionPolicy.RUNTIME)
public @interface RepositoryTests { }

@RepositoryTests
public class UserRepositoryTests { }

If we do the following:

MergedAnnotations<Transactional> annotation = MergedAnnotations.from(UserRepositoryTests.class).get(Transactional.class);

The the method results would be:

• annotation.getSource() = UserRepositoryTests.class (but it's return type is as an object because we want to support ASM sources as well in the future)
• annotation.getParent() = A MergedAnnotation<MyTransactional> instance
• annotation.getRoot() = A MergedAnnotation<RepositoryTests> instance
• annotation.synthesize() = A Transactional annotation instance

Does synthesize() always synthesize the annotation

Yes, currently it does. In earlier versions of the prototype it used to check if it could bypass the method but I dropped that because it didn't seem to make much difference to performance and it made the code a bit more complex. I was also hoping that people wouldn't call synthesize() all that often. We can probably reintroduce that bypass logic. If we do, I wonder if we should also revisit the SynthesizedAnnotation interface. It kind of bugged me that it would sometimes be there and sometimes not.

The current implementation of getParent() actually returns the immediate "child", since annotation hierarchies are top-down analogous to class hierarchies. So I think we need to rename this method.

We'll also need to change AnnotationTypeMappings so that the names are consistent everywhere, but I'm not totally convinced that using "child" works very well. It doesn't feel as natural as parent to me. I think it's because parent helps to imply that there's only one, where as child feels like there could be many.

I think most users of the API want to work with annotations and not really care if they are meta-annotations or not. I think of the parent relationship as breadcrumbs to get you back to the root of the data. I also like that parent maps nicely to depth.

So for example, I might be trying to handle @Transactional annotations. Mostly I just want to get the @Transactional can look at the attributes on it. Hopefully I don't really care if they come from a @RepositoryTests or @MyTransactional. If I do want a bit more context then the parent helps. I think of the parent as "how the hell did we get here". So in the above example we have MergedAnnotations<Transactional> which is three deep:

RepositoryTests (depth = 0, the root)
 ^
MyTransactional (depth = 1)
  ^
Transactional (depth = 2)

So to put it another way. When I think about @RepositoryTests, It feels natural to say it defines a child annotations @MyTransactional and @ContextConfiguration. Likewise @MyTransactional defines a child annotation @Transactional. If I think about that specific @Transactional in the context of a @RepositoryTests declaration then it feels right to say this specific @Transactional has a parent @MyTransactional which has a parent @RepositoryTests (the root).

Perhaps we should just move away from the parent/child terminology entirely.

[philwebb]

@sbrannen I've added the extra method as is for now. I think we should open another issue if we want to reconsider the method names.

[sbrannen]

@philwebb, thanks for providing the examples with the MergedAnnotations API. That definitely helps to better discuss the topic.

If I think about that specific @Transactional in the context of a @RepositoryTests declaration then it feels right to say this specific @Transactional has a parent @MyTransactional which has a parent @RepositoryTests (the root).

I disagree with that. We literally have a top-down inheritance hierarchy from @Transactional down to @RepositoryTests, and that is reflected in the attribute merging and overriding algorithm: children inherit and potentially override attributes from their ancestors, analogous to class hierarchies.

Perhaps we should just move away from the parent/child terminology entirely.

Yes, that is likely the right thing to do for these particular methods.

[sbrannen]

@sbrannen I've added the extra method as is for now. I think we should open another issue if we want to reconsider the method names.

OK. I raised #22946 as a follow up.