From 0226580773dc8c8b3f4554f5e42d818d96544b25 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Tue, 4 Jul 2023 15:58:30 +0200 Subject: [PATCH 1/2] Refresh cached value after unexpected mismatch (e.g. null vs non-null) In addition to the previously addressed removal of bean definitions, this is able to deal with prototype factory methods returning non-null after null or also null after non-null. Stale cached values are getting refreshed rather than bypassed. Closes gh-30794 --- .../AutowiredAnnotationBeanPostProcessor.java | 39 ++++++----- ...wiredAnnotationBeanPostProcessorTests.java | 67 +++++++++++++++++++ 2 files changed, 90 insertions(+), 16 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessor.java b/spring-beans/src/main/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessor.java index a9c716684ab..c948f9438d2 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessor.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessor.java @@ -638,7 +638,7 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA * Resolve the specified cached method argument or field value. */ @Nullable - private Object resolvedCachedArgument(@Nullable String beanName, @Nullable Object cachedArgument) { + private Object resolveCachedArgument(@Nullable String beanName, @Nullable Object cachedArgument) { if (cachedArgument instanceof DependencyDescriptor descriptor) { Assert.state(this.beanFactory != null, "No BeanFactory available"); return this.beanFactory.resolveDependency(descriptor, beanName, null, null); @@ -683,10 +683,12 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA Object value; if (this.cached) { try { - value = resolvedCachedArgument(beanName, this.cachedFieldValue); + value = resolveCachedArgument(beanName, this.cachedFieldValue); } - catch (NoSuchBeanDefinitionException ex) { - // Unexpected removal of target bean for cached argument -> re-resolve + catch (BeansException ex) { + // Unexpected target bean mismatch for cached argument -> re-resolve + this.cached = false; + logger.debug("Failed to resolve cached argument", ex); value = resolveFieldValue(field, bean, beanName); } } @@ -715,9 +717,8 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA } synchronized (this) { if (!this.cached) { - Object cachedFieldValue = null; if (value != null || this.required) { - cachedFieldValue = desc; + Object cachedFieldValue = desc; registerDependentBeans(beanName, autowiredBeanNames); if (value != null && autowiredBeanNames.size() == 1) { String autowiredBeanName = autowiredBeanNames.iterator().next(); @@ -727,9 +728,13 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA desc, autowiredBeanName, field.getType()); } } + this.cachedFieldValue = cachedFieldValue; + this.cached = true; + } + else { + this.cachedFieldValue = null; + // cached flag remains false } - this.cachedFieldValue = cachedFieldValue; - this.cached = true; } } return value; @@ -760,10 +765,12 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA Object[] arguments; if (this.cached) { try { - arguments = resolveCachedArguments(beanName); + arguments = resolveCachedArguments(beanName, this.cachedMethodArguments); } - catch (NoSuchBeanDefinitionException ex) { - // Unexpected removal of target bean for cached argument -> re-resolve + catch (BeansException ex) { + // Unexpected target bean mismatch for cached argument -> re-resolve + this.cached = false; + logger.debug("Failed to resolve cached argument", ex); arguments = resolveMethodArguments(method, bean, beanName); } } @@ -782,14 +789,13 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA } @Nullable - private Object[] resolveCachedArguments(@Nullable String beanName) { - Object[] cachedMethodArguments = this.cachedMethodArguments; + private Object[] resolveCachedArguments(@Nullable String beanName, @Nullable Object[] cachedMethodArguments) { if (cachedMethodArguments == null) { return null; } Object[] arguments = new Object[cachedMethodArguments.length]; for (int i = 0; i < arguments.length; i++) { - arguments[i] = resolvedCachedArgument(beanName, cachedMethodArguments[i]); + arguments[i] = resolveCachedArgument(beanName, cachedMethodArguments[i]); } return arguments; } @@ -822,7 +828,7 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA synchronized (this) { if (!this.cached) { if (arguments != null) { - DependencyDescriptor[] cachedMethodArguments = Arrays.copyOf(descriptors, arguments.length); + DependencyDescriptor[] cachedMethodArguments = Arrays.copyOf(descriptors, argumentCount); registerDependentBeans(beanName, autowiredBeans); if (autowiredBeans.size() == argumentCount) { Iterator it = autowiredBeans.iterator(); @@ -837,11 +843,12 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA } } this.cachedMethodArguments = cachedMethodArguments; + this.cached = true; } else { this.cachedMethodArguments = null; + // cached flag remains false } - this.cached = true; } } return arguments; diff --git a/spring-beans/src/test/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessorTests.java b/spring-beans/src/test/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessorTests.java index 53b36cddb1b..5cd98cf8cae 100644 --- a/spring-beans/src/test/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessorTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessorTests.java @@ -151,6 +151,59 @@ public class AutowiredAnnotationBeanPostProcessorTests { assertThat(bean.getTestBean3()).isNull(); } + @Test + void resourceInjectionWithSometimesNullBean() { + RootBeanDefinition bd = new RootBeanDefinition(OptionalResourceInjectionBean.class); + bd.setScope(BeanDefinition.SCOPE_PROTOTYPE); + bf.registerBeanDefinition("annotatedBean", bd); + RootBeanDefinition tb = new RootBeanDefinition(SometimesNullFactoryMethods.class); + tb.setFactoryMethodName("createTestBean"); + tb.setScope(BeanDefinition.SCOPE_PROTOTYPE); + bf.registerBeanDefinition("testBean", tb); + + SometimesNullFactoryMethods.active = false; + OptionalResourceInjectionBean bean = (OptionalResourceInjectionBean) bf.getBean("annotatedBean"); + assertThat(bean.getTestBean()).isNull(); + assertThat(bean.getTestBean2()).isNull(); + assertThat(bean.getTestBean3()).isNull(); + + SometimesNullFactoryMethods.active = true; + bean = (OptionalResourceInjectionBean) bf.getBean("annotatedBean"); + assertThat(bean.getTestBean()).isNotNull(); + assertThat(bean.getTestBean2()).isNotNull(); + assertThat(bean.getTestBean3()).isNotNull(); + + SometimesNullFactoryMethods.active = false; + bean = (OptionalResourceInjectionBean) bf.getBean("annotatedBean"); + assertThat(bean.getTestBean()).isNull(); + assertThat(bean.getTestBean2()).isNull(); + assertThat(bean.getTestBean3()).isNull(); + + SometimesNullFactoryMethods.active = false; + bean = (OptionalResourceInjectionBean) bf.getBean("annotatedBean"); + assertThat(bean.getTestBean()).isNull(); + assertThat(bean.getTestBean2()).isNull(); + assertThat(bean.getTestBean3()).isNull(); + + SometimesNullFactoryMethods.active = true; + bean = (OptionalResourceInjectionBean) bf.getBean("annotatedBean"); + assertThat(bean.getTestBean()).isNotNull(); + assertThat(bean.getTestBean2()).isNotNull(); + assertThat(bean.getTestBean3()).isNotNull(); + + SometimesNullFactoryMethods.active = true; + bean = (OptionalResourceInjectionBean) bf.getBean("annotatedBean"); + assertThat(bean.getTestBean()).isNotNull(); + assertThat(bean.getTestBean2()).isNotNull(); + assertThat(bean.getTestBean3()).isNotNull(); + + SometimesNullFactoryMethods.active = false; + bean = (OptionalResourceInjectionBean) bf.getBean("annotatedBean"); + assertThat(bean.getTestBean()).isNull(); + assertThat(bean.getTestBean2()).isNull(); + assertThat(bean.getTestBean3()).isNull(); + } + @Test void extendedResourceInjection() { RootBeanDefinition bd = new RootBeanDefinition(TypedExtendedResourceInjectionBean.class); @@ -3881,6 +3934,20 @@ public class AutowiredAnnotationBeanPostProcessorTests { } + public static class SometimesNullFactoryMethods { + + public static boolean active = false; + + public static TestBean createTestBean() { + return (active ? new TestBean() : null); + } + + public static NestedTestBean createNestedTestBean() { + return (active ? new NestedTestBean() : null); + } + } + + public static class ProvidedArgumentBean { public ProvidedArgumentBean(String[] args) { From 1dc9dffc70308252a00bb56b1b37c67b52ba1ed2 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Tue, 4 Jul 2023 15:58:46 +0200 Subject: [PATCH 2/2] Restore full representation of rejected value in FieldError.toString() We would preferably use ObjectUtils.nullSafeConciseToString(rejectedValue) here but revert to the full nullSafeToString representation for strict backwards compatibility (programmatic toString calls as well as exception messages). Closes gh-30799 --- .../main/java/org/springframework/validation/FieldError.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spring-context/src/main/java/org/springframework/validation/FieldError.java b/spring-context/src/main/java/org/springframework/validation/FieldError.java index 774f38cb9fb..e8b03717cf1 100644 --- a/spring-context/src/main/java/org/springframework/validation/FieldError.java +++ b/spring-context/src/main/java/org/springframework/validation/FieldError.java @@ -124,8 +124,10 @@ public class FieldError extends ObjectError { @Override public String toString() { + // We would preferably use ObjectUtils.nullSafeConciseToString(rejectedValue) here but + // keep including the full nullSafeToString representation for backwards compatibility. return "Field error in object '" + getObjectName() + "' on field '" + this.field + - "': rejected value [" + ObjectUtils.nullSafeConciseToString(this.rejectedValue) + "]; " + + "': rejected value [" + ObjectUtils.nullSafeToString(this.rejectedValue) + "]; " + resolvableToString(); }