AnnotationTypeMapping mirror sets do not resolve in openfeign

[philwebb]

Spring Cloud openfeign project is currently failing due to the following exception:

Caused by: org.springframework.core.annotation.AnnotationConfigurationException: Different @AliasFor mirror values for annotation [org.springframework.cloud.openfeign.FeignClient] declared on org.springframework.cloud.openfeign.invalid.FeignClientValidationTests$NameAndValueConfiguration$Client, attribute 'name' and its alias 'value' are declared with values of [bar] and [foo].
    at org.springframework.core.annotation.AnnotationTypeMapping$MirrorSets$MirrorSet.resolve(AnnotationTypeMapping.java:649)
    at org.springframework.core.annotation.AnnotationTypeMapping$MirrorSets.resolve(AnnotationTypeMapping.java:604)
    at org.springframework.core.annotation.TypeMappedAnnotation.<init>(TypeMappedAnnotation.java:137)
    at org.springframework.core.annotation.TypeMappedAnnotation.<init>(TypeMappedAnnotation.java:120)
    at org.springframework.core.annotation.TypeMappedAnnotation.of(TypeMappedAnnotation.java:633)
    at org.springframework.core.annotation.MergedAnnotation.of(MergedAnnotation.java:584)
    at org.springframework.core.type.classreading.MergedAnnotationReadingVisitor.visitEnd(MergedAnnotationReadingVisitor.java:96)
    at org.springframework.asm.ClassReader.readElementValues(ClassReader.java:2775)
    at org.springframework.asm.ClassReader.accept(ClassReader.java:572)
    at org.springframework.asm.ClassReader.accept(ClassReader.java:400)
    at org.springframework.core.type.classreading.SimpleMetadataReader.<init>(SimpleMetadataReader.java:50)
    at org.springframework.core.type.classreading.SimpleMetadataReaderFactory.getMetadataReader(SimpleMetadataReaderFactory.java:103)
    at org.springframework.core.type.classreading.CachingMetadataReaderFactory.getMetadataReader(CachingMetadataReaderFactory.java:123)
    at org.springframework.context.annotation.ClassPathScanningCandidateComponentProvider.scanCandidateComponents(ClassPathScanningCandidateComponentProvider.java:430)
    ... 45 more

Initial analysis looks like 3b145a5 might be to blame, but we didn't get any framework or boot test failures so we must have a scenario that we've not properly covered.

[philwebb]

See https://jenkins.spring.io/job/spring-cloud-openfeign-master-ci/

[philwebb]

It turns out the failure is actually due to #22884 but it's an interesting one and I'm not sure if we should treat it as a regression or not.

Here's a small repo that you can run against Spring 5.1 or 5.2:

public class Temp {

	public static void main(String[] args) throws IOException {
		SimpleMetadataReaderFactory factory = new SimpleMetadataReaderFactory();
		MetadataReader reader = factory.getMetadataReader(Client.class.getName());
		AnnotationMetadata metadata = reader.getAnnotationMetadata();
		System.out.println(metadata.getClassName());
		FeignClient annotation = AnnotatedElementUtils.getMergedAnnotation(Client.class,
				FeignClient.class);
		System.out.println(annotation);
	}

	@Target(ElementType.TYPE)
	@Retention(RetentionPolicy.RUNTIME)
	@Documented
	public @interface FeignClient {

		@AliasFor("name")
		String value() default "";

		@AliasFor("value")
		String name() default "";

	}

	@FeignClient(value = "foo", name = "bar")
	interface Client {

	}

}

When you run it against 5.2 you get:

Exception in thread "main" org.springframework.core.annotation.AnnotationConfigurationException: Different @AliasFor mirror values for annotation [scratch.Temp$FeignClient] declared on scratch.Temp$Client, attribute 'name' and its alias 'value' are declared with values of [bar] and [foo].
	at org.springframework.core.annotation.AnnotationTypeMapping$MirrorSets$MirrorSet.resolve(AnnotationTypeMapping.java:649)
	at org.springframework.core.annotation.AnnotationTypeMapping$MirrorSets.resolve(AnnotationTypeMapping.java:604)
	at org.springframework.core.annotation.TypeMappedAnnotation.<init>(TypeMappedAnnotation.java:137)
	at org.springframework.core.annotation.TypeMappedAnnotation.<init>(TypeMappedAnnotation.java:120)
	at org.springframework.core.annotation.TypeMappedAnnotation.of(TypeMappedAnnotation.java:633)
	at org.springframework.core.annotation.MergedAnnotation.of(MergedAnnotation.java:584)
	at org.springframework.core.type.classreading.MergedAnnotationReadingVisitor.visitEnd(MergedAnnotationReadingVisitor.java:96)
	at org.springframework.asm.ClassReader.readElementValues(ClassReader.java:2775)
	at org.springframework.asm.ClassReader.accept(ClassReader.java:572)
	at org.springframework.asm.ClassReader.accept(ClassReader.java:400)
	at org.springframework.core.type.classreading.SimpleMetadataReader.<init>(SimpleMetadataReader.java:50)
	at org.springframework.core.type.classreading.SimpleMetadataReaderFactory.getMetadataReader(SimpleMetadataReaderFactory.java:103)
	at org.springframework.core.type.classreading.SimpleMetadataReaderFactory.getMetadataReader(SimpleMetadataReaderFactory.java:81)
	at scratch.Temp.main(Temp.java:36)

With 5.1 you get:

scratch.Temp$Client
Exception in thread "main" org.springframework.core.annotation.AnnotationConfigurationException: In annotation [scratch.Temp$FeignClient] declared on interface scratch.Temp$Client and synthesized from [@scratch.Temp$FeignClient(name=bar, value=foo)], attribute 'name' and its alias 'value' are present with values of [bar] and [foo], but only one is permitted.
	at org.springframework.core.annotation.AbstractAliasAwareAnnotationAttributeExtractor.getAttributeValue(AbstractAliasAwareAnnotationAttributeExtractor.java:103)
	at org.springframework.core.annotation.SynthesizedAnnotationInvocationHandler.getAttributeValue(SynthesizedAnnotationInvocationHandler.java:91)
	at org.springframework.core.annotation.SynthesizedAnnotationInvocationHandler.annotationToString(SynthesizedAnnotationInvocationHandler.java:244)
	at org.springframework.core.annotation.SynthesizedAnnotationInvocationHandler.invoke(SynthesizedAnnotationInvocationHandler.java:71)
	at com.sun.proxy.$Proxy5.toString(Unknown Source)
	at java.lang.String.valueOf(String.java:2994)
	at java.io.PrintStream.println(PrintStream.java:821)
	at scratch.Temp.main(Temp.java:44)

The main problem is that this annotation is not valid because it contains conflicting alias values. In Spring 5.1 you can read class meta-data and the exception is thrown only when you access the annotations. In Spring 5.2 the exception is thrown when the metadata is read.

[philwebb]

I have fix available here which I think solves the issues. Since it adds a little more complexity, and you could argue that throwing the AnnotationConfigurationException is more robust, I'll leave it for @jhoeller to decide what to do.

One other possible advantage of the fix is it might mean less work needs to happen if class data is read but annotation data is not (but I've not profiled anything).

[philwebb]

Patched locally:

[INFO] --- maven-install-plugin:2.4:install (default-install) @ spring-cloud-openfeign-docs ---
[INFO] Installing /Users/pwebb/projects/spring-cloud/code/spring-cloud-openfeign/docs/pom.xml to /Users/pwebb/.m2/repository/org/springframework/cloud/spring-cloud-openfeign-docs/2.2.0.BUILD-SNAPSHOT/spring-cloud-openfeign-docs-2.2.0.BUILD-SNAPSHOT.pom
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO] 
[INFO] spring-cloud-openfeign-dependencies ................ SUCCESS [  0.140 s]
[INFO] Spring Cloud OpenFeign 2.2.0.BUILD-SNAPSHOT ........ SUCCESS [  2.003 s]
[INFO] Spring Cloud OpenFeign Core ........................ SUCCESS [01:05 min]
[INFO] Spring Cloud Starter OpenFeign ..................... SUCCESS [  0.291 s]
[INFO] Spring Cloud OpenFeign Docs 2.2.0.BUILD-SNAPSHOT ... SUCCESS [  0.195 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:08 min
[INFO] Finished at: 2019-05-13T21:25:09-07:00
[INFO] ------------------------------------------------------------------------

[ryanjbaxter]

@philwebb regarding this annotation

We expected an error for this as it is not a valid configuration for the annotation
https://github.com/spring-cloud/spring-cloud-openfeign/blob/50c799a4a00a668c05b910e233d24496c93eec33/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/invalid/FeignClientValidationTests.java#L51

However I guess the change just changed when or how that error was thrown (earlier on when the application starts).

[philwebb]

@ryanjbaxter Correct. I basically made a couple of changes to the meta-data code. The first exposes the new MergedAnnotations API and the second creates immutable instances when using ASM to read the bytecode.

The ASM change means I construct the MergedAnnotations during read, and that causes the exception to be thrown earlier. Previously, you could read the bytecode and it wouldn't fail until actually tried to access the annotation data.

I personally think the new approach is better since the errors are thrown earlier, but it could be considered a breaking change. That's why I didn't just push https://github.com/philwebb/spring-framework/tree/gh-22969, because I think it's a call someone on the framework team need to make the decision about what's best.

[philwebb]

Although there's a difference in behavior, we feel it's better to throw the exception early.