Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsafe invocation of .value() on annotations in SynthesizedMergedAnnotationInvocationHandler prevents backwards-compatible additions to @annotation interfaces #24029

Closed
olaf-otto opened this issue Nov 19, 2019 · 3 comments
Assignees
Milestone

Comments

@olaf-otto
Copy link

@olaf-otto olaf-otto commented Nov 19, 2019

Context

I have a scenario where I am extending an annotation API from

@interface MyAnnotation {
    String[] old();
}

to

@interface MyAnnotation {
    String[] value();

   @deprecated
    String[] old() default {}
}

Here, old libraries compiled with the old annotation are still supported as the change is binary compatible. It is only that I as a framework author need to handle the case of an invocation of value() throwing an IncompleteAnnotationException.

I believe allowing this is crucial to evolution of annotations without forcing all implementors to re-compile.

Issue in Spring Core

When calling org.springframework.beans.factory.ListableBeanFactory#findAnnotationOnBean, Spring tries to build a proxy for the found annotation. In the process, spring attempts to load the value of all annotation attributes via org.springframework.core.annotation.SynthesizedMergedAnnotationInvocationHandler#getAttributeValue. This fails for the newly added annotation attribute as mentioned above:

java.lang.annotation.IncompleteAnnotationException: MyAnnotation missing element value
	at sun.reflect.annotation.AnnotationInvocationHandler.invoke(AnnotationInvocationHandler.java:81)
	at com.sun.proxy.$Proxy180.value(Unknown Source)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.springframework.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:279) [org.apache.servicemix.bundles.spring-core:5.2.0.RELEASE_1]
	at org.springframework.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:263) [org.apache.servicemix.bundles.spring-core:5.2.0.RELEASE_1]
	at org.springframework.core.annotation.TypeMappedAnnotation.getValue(TypeMappedAnnotation.java:429) [org.apache.servicemix.bundles.spring-core:5.2.0.RELEASE_1]
	at org.springframework.core.annotation.TypeMappedAnnotation.getValue(TypeMappedAnnotation.java:399) [org.apache.servicemix.bundles.spring-core:5.2.0.RELEASE_1]
	at org.springframework.core.annotation.TypeMappedAnnotation.getAttributeValue(TypeMappedAnnotation.java:384) [org.apache.servicemix.bundles.spring-core:5.2.0.RELEASE_1]
	at org.springframework.core.annotation.AbstractMergedAnnotation.getValue(AbstractMergedAnnotation.java:178) [org.apache.servicemix.bundles.spring-core:5.2.0.RELEASE_1]
	at org.springframework.core.annotation.SynthesizedMergedAnnotationInvocationHandler.getAttributeValue(SynthesizedMergedAnnotationInvocationHandler.java:176) [org.apache.servicemix.bundles.spring-core:5.2.0.RELEASE_1]
	at org.springframework.core.annotation.SynthesizedMergedAnnotationInvocationHandler.<init>(SynthesizedMergedAnnotationInvocationHandler.java:66) [org.apache.servicemix.bundles.spring-core:5.2.0.RELEASE_1]
	at org.springframework.core.annotation.SynthesizedMergedAnnotationInvocationHandler.createProxy(SynthesizedMergedAnnotationInvocationHandler.java:184) [org.apache.servicemix.bundles.spring-core:5.2.0.RELEASE_1]
	at org.springframework.core.annotation.TypeMappedAnnotation.createSynthesized(TypeMappedAnnotation.java:335) [org.apache.servicemix.bundles.spring-core:5.2.0.RELEASE_1]
	at org.springframework.core.annotation.AbstractMergedAnnotation.synthesize(AbstractMergedAnnotation.java:210) [org.apache.servicemix.bundles.spring-core:5.2.0.RELEASE_1]
	at org.springframework.core.annotation.AbstractMergedAnnotation.synthesize(AbstractMergedAnnotation.java:200) [org.apache.servicemix.bundles.spring-core:5.2.0.RELEASE_1]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.findAnnotationOnBean(DefaultListableBeanFactory.java:680) [io.neba.spring-beans:5.2.0.RELEASE_1]

Here, I believe the implementation should not expected values to be complete. I am not even sure as to why org.springframework.core.annotation.SynthesizedMergedAnnotationInvocationHandler#SynthesizedMergedAnnotationInvocationHandler loads the values in the first place.

@olaf-otto

This comment has been minimized.

Copy link
Author

@olaf-otto olaf-otto commented Nov 19, 2019

Note: This affects Spring 5.2.0.GA, and likely others.

@jhoeller

This comment has been minimized.

Copy link
Contributor

@jhoeller jhoeller commented Nov 19, 2019

This pre-loading values only really happens for early validation of value mismatches in the case of aliased attributes or Map-populated attributes. Since even those cases should be fine with late validation on actual attribute invocation, I've dropped that pre-loading iteration completely.

Please give the upcoming 5.2.2.BUILD-SNAPSHOT a try, I hope your scenarios is covered now.

@olaf-otto

This comment has been minimized.

Copy link
Author

@olaf-otto olaf-otto commented Nov 21, 2019

Thanks @jhoeller for the prompt and excellent resolution. Unfortunately I am unable to fully verify my scenario as it is a complex OSGi app and I am depending on the ASF's ServiceMix team to translate spring releases into OSGi bundles. I'll re-test with the next release and will use a workaround for the time being.

olaf-otto added a commit to unic/neba that referenced this issue Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.