AnnotationsScanner throws NPE due to because AnnotatedElement proxy is not cloning arrays

[schauder]

This affects 5.2.0BUILD-SNAPSHOT, specifically 5.2.0-BUILD-20190325.085048

Spring Data JDBC integration tests started failing with the stack trace below.

The reason is that this line: https://github.com/spring-projects/spring-framework/blob/master/spring-core/src/main/java/org/springframework/core/annotation/AnnotationsScanner.java#L448

throws an NPE because the array annotations contains a null element.

The issue can be reproduced by running mvn clean install on master of spring-data-jdbc.
Sorry, couldn't pin the issue down more precisely so far.

CI builds show that it worked last Friday. (20190322...)

java.lang.IllegalStateException: Failed to load ApplicationContext
	at org.springframework.test.context.cache.DefaultCacheAwareContextLoaderDelegate.loadContext(DefaultCacheAwareContextLoaderDelegate.java:125)
	at org.springframework.test.context.support.DefaultTestContext.getApplicationContext(DefaultTestContext.java:108)
	at org.springframework.test.context.support.DependencyInjectionTestExecutionListener.injectDependencies(DependencyInjectionTestExecutionListener.java:118)
	at org.springframework.test.context.support.DependencyInjectionTestExecutionListener.prepareTestInstance(DependencyInjectionTestExecutionListener.java:83)
	at org.springframework.test.context.TestContextManager.prepareTestInstance(TestContextManager.java:244)
	at org.springframework.test.context.junit4.statements.RunPrepareTestInstanceCallbacks.evaluate(RunPrepareTestInstanceCallbacks.java:63)
	at org.springframework.test.context.junit4.statements.SpringRepeat.evaluate(SpringRepeat.java:84)
	at org.springframework.test.context.junit4.statements.SpringFailOnTimeout.evaluate(SpringFailOnTimeout.java:87)
	at org.springframework.test.context.junit4.statements.ProfileValueChecker.evaluate(ProfileValueChecker.java:103)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.springframework.test.context.junit4.statements.RunBeforeTestClassCallbacks.evaluate(RunBeforeTestClassCallbacks.java:61)
	at org.springframework.test.context.junit4.statements.RunAfterTestClassCallbacks.evaluate(RunAfterTestClassCallbacks.java:70)
	at org.springframework.test.context.junit4.statements.ProfileValueChecker.evaluate(ProfileValueChecker.java:103)
	at org.springframework.test.context.junit4.rules.SpringClassRule$TestContextManagerCacheEvictor.evaluate(SpringClassRule.java:190)
	at org.junit.rules.RunRules.evaluate(RunRules.java:20)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
Caused by: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'jdbcRepositoryManipulateDbActionsIntegrationTests.DummyEntityRepository': Injection of autowired dependencies failed; nested exception is java.lang.NullPointerException
	at org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor.postProcessProperties(AutowiredAnnotationBeanPostProcessor.java:391)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.populateBean(AbstractAutowireCapableBeanFactory.java:1415)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:592)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:515)
	at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:320)
	at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:222)
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:318)
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:199)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:840)
	at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:877)
	at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:549)
	at org.springframework.test.context.support.AbstractGenericContextLoader.loadContext(AbstractGenericContextLoader.java:128)
	at org.springframework.test.context.support.AbstractGenericContextLoader.loadContext(AbstractGenericContextLoader.java:60)
	at org.springframework.test.context.support.AbstractDelegatingSmartContextLoader.delegateLoading(AbstractDelegatingSmartContextLoader.java:275)
	at org.springframework.test.context.support.AbstractDelegatingSmartContextLoader.loadContext(AbstractDelegatingSmartContextLoader.java:243)
	at org.springframework.test.context.cache.DefaultCacheAwareContextLoaderDelegate.loadContextInternal(DefaultCacheAwareContextLoaderDelegate.java:99)
	at org.springframework.test.context.cache.DefaultCacheAwareContextLoaderDelegate.loadContext(DefaultCacheAwareContextLoaderDelegate.java:117)
	... 27 common frames omitted
Caused by: java.lang.NullPointerException: null
	at org.springframework.core.annotation.AnnotationsScanner.getDeclaredAnnotations(AnnotationsScanner.java:448)
	at org.springframework.core.annotation.AnnotationsScanner.getDeclaredAnnotations(AnnotationsScanner.java:421)
	at org.springframework.core.annotation.AnnotationsScanner.processElement(AnnotationsScanner.java:409)
	at org.springframework.core.annotation.AnnotationsScanner.process(AnnotationsScanner.java:111)
	at org.springframework.core.annotation.AnnotationsScanner.scan(AnnotationsScanner.java:96)
	at org.springframework.core.annotation.AnnotationsScanner.scan(AnnotationsScanner.java:77)
	at org.springframework.core.annotation.TypeMappedAnnotations.scan(TypeMappedAnnotations.java:246)
	at org.springframework.core.annotation.TypeMappedAnnotations.get(TypeMappedAnnotations.java:148)
	at org.springframework.core.annotation.AnnotatedElementUtils.getMergedAnnotationAttributes(AnnotatedElementUtils.java:258)
	at org.springframework.beans.factory.annotation.QualifierAnnotationAutowireCandidateResolver.findValue(QualifierAnnotationAutowireCandidateResolver.java:366)
	at org.springframework.beans.factory.annotation.QualifierAnnotationAutowireCandidateResolver.getSuggestedValue(QualifierAnnotationAutowireCandidateResolver.java:354)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1195)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveDependency(DefaultListableBeanFactory.java:1177)
	at org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor$AutowiredMethodElement.inject(AutowiredAnnotationBeanPostProcessor.java:697)
	at org.springframework.beans.factory.annotation.InjectionMetadata.inject(InjectionMetadata.java:116)
	at org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor.postProcessProperties(AutowiredAnnotationBeanPostProcessor.java:385)
	... 43 common frames omitted

[sbrannen]

@philwebb, do you have time to look into this one as well?

[sbrannen]

Similar issued reported by another user:

Caused by: java.lang.NullPointerException
    at org.springframework.beans.factory.annotation.QualifierAnnotationAutowireCandidateResolver.checkQualifiers(QualifierAnnotationAutowireCandidateResolver.java:172)
    at org.springframework.beans.factory.annotation.QualifierAnnotationAutowireCandidateResolver.isAutowireCandidate(QualifierAnnotationAutowireCandidateResolver.java:149)
    at org.springframework.beans.factory.support.DefaultListableBeanFactory.isAutowireCandidate(DefaultListableBeanFactory.java:774)
    at org.springframework.beans.factory.support.DefaultListableBeanFactory.isAutowireCandidate(DefaultListableBeanFactory.java:737)
    at org.springframework.beans.factory.support.DefaultListableBeanFactory.isAutowireCandidate(DefaultListableBeanFactory.java:721)
    at org.springframework.beans.factory.support.DefaultListableBeanFactory.findAutowireCandidates(DefaultListableBeanFactory.java:1436)

[jhoeller]

This has been bugging me in my review of the new arrangement already: Ignored annotations are nulled out in the annotations array. I left it in place for the time being but really prefer this being replaced with a correspondingly shrinked new array which never contains null entries. @philwebb, unless there was some specific reasoning behind reusing the original arrays, a shrinked array seems much safer, in particular for externally exposed annotation arrays?

[philwebb]

@jhoeller The main reason was to reduce GC pressure and improve performance. I figured it would be best not to need the array copy (but I admit I've not done testing with a copy in place).

[jhoeller]

@philwebb On a related note, in my immediate revision that came with the merge, I've changed the rule for ignoring annotations to include java meta-annotations in general, not just FunctionalInterface as it originally said. This might lead to further null entries exposed down the line which haven't been seen before? In any case, for public exposure of annotation arrays, we should never expose null entries to be on the safe side. For internal arrays, iterating them and leniently skipping them might be worthwhile.

[philwebb]

Indeed, the null entries should never escape in internal code.

[philwebb]

Actually, this one is a little odd. The array in question isn't ours, it's a result of calling source.getDeclaredAnnotations() (where source is an AnnotatedElement). I wasn't aware that could return null elements.

[philwebb]

I've pushed a fix for master. Although it doesn't surface an NPE, the issue also exists in previous branches so I'll reopen for @jhoeller to consider a backport.

[jhoeller]

Since the user-level recommendation is to not modify those arrays and we're not defensively copying them in DependendyDescriptor and co either, I'd rather leave this as-is for our purposes in the older branches. After all, it avoids extra array creation within QualifierAnnotationAutowireCandidateResolver where we know that the annotation array input comes from a DependendyDescriptor anyway.

That said, generally speaking for an AnnotatedElement implementation, the defensive copy is indeed correct there, and as of 5.2 we're relying on this behavior ourselves now.