Regression: default values for implicit aliases no longer honored in MergedAnnotations

[iiiron]

A single low-level annotation attribute can change a high-level attribute via @AliasFor, but when some low-level attribute is mirrored and is the default value, the high-level attribute will not be changed.

I think this is unreasonable.

[pivotal-issuemaster]

@iiiron Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@iiiron Thank you for signing the Contributor License Agreement!

[sbrannen]

@iiiron, thanks for submitting the PR.

Do you have a use case that used to work but now fails with Spring Framework 5.2.x?

[iiiron]

No I didn't，I just find it by reading the source code.

[sbrannen]

No I didn't，I just find it by reading the source code.

Thanks for providing feedback.

Since the proposed change does not address a regression, I would like to wait on input from @philwebb.

In addition, after reviewing the affected code, it appears to me that leaving the result set to -1 in such cases does not lead to any negative side effects.

[philwebb]

I'm not sure I totally understand the issue. @iiiron can you clarify what you mean by "when some low-level attribute is mirrored and is the default value, the high-level attribute will not be changed"? Perhaps you can provide some sample annotations that show what you mean?

[iiiron]

think that. ```java @TestB @retention(RetentionPolicy.RUNTIME) @target({ElementType.TYPE}) public @interface TestA { @AliasFor(value = "b1", annotation = TestB.class) String a1() default "testA"; } @testc @retention(RetentionPolicy.RUNTIME) @target({ElementType.TYPE}) public @interface TestB { @AliasFor(value = "b2") String b1() default "testB"; @AliasFor(value = "b1") String b2() default "testB"; } @testa public class Test { public static void main(String[] args) { // you will get an AnnotationAttributes what key b1 set to "testA" and key b2 set to "testA" Object b = AnnotatedElementUtils.findMergedAnnotationAttributes(Test.class, TestB.class, false, true); } } ``` but when you make `TestA` like that ```java @TestB @retention(RetentionPolicy.RUNTIME) @target({ElementType.TYPE}) public @interface TestA { @AliasFor(value = "b1", annotation = TestB.class) String a1() default "testA"; @AliasFor(value = "b2", annotation = TestB.class) String a2() default "testA"; } @testa public class Test { public static void main(String[] args) { // you will get an AnnotationAttributes what key b1 set to "testB" and key b2 set to "testB" Object b = AnnotatedElementUtils.findMergedAnnotationAttributes(Test.class, TestB.class, false, true); } } ``` I think it's unreasonable, when the attributes in `TestA` become to mirrors, it will not effect `TestB`. The low-level annotation attributes should have priority over high-level annotation attributes, like subclass can overwrite the methods in super-class.

[sbrannen]

Thanks for providing the example.

In terms of executable tests, the following passes against master.

class TestCase {

	@Test
	void findMergedAnnotation() {
		MyAnnotation myAnnotation = AnnotatedElementUtils.findMergedAnnotation(AnnotatedClass.class,
			MyAnnotation.class);
		assertThat(myAnnotation).isNotNull();
		assertThat(myAnnotation.b1()).isEqualTo("testA");
		assertThat(myAnnotation.b2()).isEqualTo("testA");
	}

	@Test
	void findMergedAnnotationAttributes() {
		AnnotationAttributes attributes = AnnotatedElementUtils.findMergedAnnotationAttributes(AnnotatedClass.class,
			MyAnnotation.class, false, true);

		assertThat(attributes).isNotNull();
		assertThat(attributes).size().isEqualTo(2);
		assertThat(attributes.get("b1")).isEqualTo("testA");
		assertThat(attributes.get("b2")).isEqualTo("testA");
	}
}

@Retention(RetentionPolicy.RUNTIME)
@interface MyAnnotation {

	@AliasFor("b2")
	String b1() default "testB";

	@AliasFor("b1")
	String b2() default "testB";
}

@MyAnnotation
@Retention(RetentionPolicy.RUNTIME)
@interface ComposedAnnotation {

	@AliasFor(attribute = "b1", annotation = MyAnnotation.class)
	String a1() default "testA";

}

@ComposedAnnotation
class AnnotatedClass {
}

[sbrannen]

In addition, the following JUnit 4 based tests pass against 5.1.x.

public class TestCase {

	@Test
	public void findMergedAnnotation() {
		MyAnnotation myAnnotation = AnnotatedElementUtils.findMergedAnnotation(AnnotatedClass.class,
			MyAnnotation.class);
		assertNotNull(myAnnotation);
		assertEquals("testA", myAnnotation.b1());
		assertEquals("testA", myAnnotation.b2());
	}

	@Test
	public void findMergedAnnotationAttributes() {
		AnnotationAttributes attributes = AnnotatedElementUtils.findMergedAnnotationAttributes(AnnotatedClass.class,
			MyAnnotation.class, false, true);

		assertNotNull(attributes);
		assertEquals(2, attributes.size());
		assertEquals("testA", attributes.get("b1"));
		assertEquals("testA", attributes.get("b2"));
	}
}

@Retention(RetentionPolicy.RUNTIME)
@interface MyAnnotation {

	@AliasFor("b2")
	String b1() default "testB";

	@AliasFor("b1")
	String b2() default "testB";
}

@MyAnnotation
@Retention(RetentionPolicy.RUNTIME)
@interface ComposedAnnotation {

	@AliasFor(attribute = "b1", annotation = MyAnnotation.class)
	String a1() default "testA";

}

@ComposedAnnotation
class AnnotatedClass {
}

Thus, there is not a regression in 5.2.x for this behavior.

[sbrannen]

The low-level annotation attributes should have priority over high-level annotation attributes, like subclass can overwrite the methods in super-class.

That is exactly the behavior that is supported.

In the tests I provided, @ComposedAnnotation is like a subclass of @MyAnnotation, and the default value for a1 overrides the default values for b1 and b2.

In light of that, I am closing this PR as "works as designed".

For further information on Spring's support for composed annotations and @AliasFor, consult the Spring Annotation Programming Model wiki page.

[iiiron]

thank you for your feedback! but do you test that case: ```java @MyAnnotation @retention(RetentionPolicy.RUNTIME) @interface ComposedAnnotation { @AliasFor(attribute = "b1", annotation = MyAnnotation.class) String a1() default "testA"; @AliasFor(attribute = "b2", annotation = MyAnnotation.class) String a2() default "testA";} ```

[sbrannen]

The tests still pass against 5.1.x with only the following changes.

@ComposedAnnotation2
class AnnotatedClass {
}

@MyAnnotation
@Retention(RetentionPolicy.RUNTIME)
@interface ComposedAnnotation2 {

	@AliasFor(attribute = "b1", annotation = MyAnnotation.class)
	String a1() default "testA";

	@AliasFor(attribute = "b2", annotation = MyAnnotation.class)
	String a2() default "testA";
}

However, the tests in fact fail against master with the same changes.

Thus I am reopening this issue since it is in fact a regression.

@iiiron, thank you very much bringing this to our attention!

[iiiron]

thank you, it's my pleasure!

[sbrannen]

The proposed code change in the PR appears to fix the regression.

So I've scheduled this for 5.2.3.

[sbrannen]

This has been merged into master in 6f15f32 with an additional integration test in fb13f6f.

Thanks again for the PR! 👏

[philwebb]

Thanks @iiiron and @sbrannen !