From 362fe12704fd950d0d77db8b02b00e70d5175cb5 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Wed, 21 May 2025 14:44:12 +0200 Subject: [PATCH 1/3] Polishing --- ...onfigurationClassBeanDefinitionReader.java | 9 +- .../context/annotation/ImportResource.java | 26 +++-- .../configuration/ImportResourceTests.java | 107 +++++++++--------- ... => ImportNonXmlResourceConfig.properties} | 0 4 files changed, 73 insertions(+), 69 deletions(-) rename spring-context/src/test/resources/org/springframework/context/annotation/configuration/{ImportNonXmlResourceConfig-context.properties => ImportNonXmlResourceConfig.properties} (100%) diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java index 8385b9ef34..8f019f10ff 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java +++ b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java @@ -286,8 +286,8 @@ class ConfigurationClassBeanDefinitionReader { } if (logger.isTraceEnabled()) { - logger.trace(String.format("Registering bean definition for @Bean method %s.%s()", - configClass.getMetadata().getClassName(), beanName)); + logger.trace("Registering bean definition for @Bean method %s.%s()" + .formatted(configClass.getMetadata().getClassName(), beanName)); } this.registry.registerBeanDefinition(beanName, beanDefToRegister); } @@ -344,9 +344,8 @@ class ConfigurationClassBeanDefinitionReader { "@Bean definition illegally overridden by existing bean definition: " + existingBeanDef); } if (logger.isDebugEnabled()) { - logger.debug(String.format("Skipping bean definition for %s: a definition for bean '%s' " + - "already exists. This top-level bean definition is considered as an override.", - beanMethod, beanName)); + logger.debug("Skipping bean definition for %s: a definition for bean '%s' already exists. " + + "This top-level bean definition is considered as an override.".formatted(beanMethod, beanName)); } return true; } diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ImportResource.java b/spring-context/src/main/java/org/springframework/context/annotation/ImportResource.java index da1cb97b8e..3bdf24bac1 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ImportResource.java +++ b/spring-context/src/main/java/org/springframework/context/annotation/ImportResource.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -29,14 +29,15 @@ import org.springframework.core.annotation.AliasFor; * Indicates one or more resources containing bean definitions to import. * *

Like {@link Import @Import}, this annotation provides functionality similar to - * the {@code } element in Spring XML. It is typically used when designing - * {@link Configuration @Configuration} classes to be bootstrapped by an - * {@link AnnotationConfigApplicationContext}, but where some XML functionality such - * as namespaces is still necessary. + * the {@code } element in Spring XML configuration. It is typically used + * when designing {@link Configuration @Configuration} classes to be bootstrapped by + * an {@link AnnotationConfigApplicationContext}, but where some XML functionality + * such as namespaces is still necessary. * - *

By default, arguments to the {@link #value} attribute will be processed using a + *

By default, arguments to the {@link #locations() locations} or {@link #value() value} + * attribute will be processed using a * {@link org.springframework.beans.factory.groovy.GroovyBeanDefinitionReader GroovyBeanDefinitionReader} - * if ending in {@code ".groovy"}; otherwise, an + * for resource locations ending in {@code ".groovy"}; otherwise, an * {@link org.springframework.beans.factory.xml.XmlBeanDefinitionReader XmlBeanDefinitionReader} * will be used to parse Spring {@code } XML files. Optionally, the {@link #reader} * attribute may be declared, allowing the user to choose a custom {@link BeanDefinitionReader} @@ -77,12 +78,15 @@ public @interface ImportResource { /** * {@link BeanDefinitionReader} implementation to use when processing - * resources specified via the {@link #value} attribute. + * resources specified via the {@link #locations() locations} or + * {@link #value() value} attribute. *

By default, the reader will be adapted to the resource path specified: * {@code ".groovy"} files will be processed with a - * {@link org.springframework.beans.factory.groovy.GroovyBeanDefinitionReader GroovyBeanDefinitionReader}; - * whereas, all other resources will be processed with an - * {@link org.springframework.beans.factory.xml.XmlBeanDefinitionReader XmlBeanDefinitionReader}. + * {@link org.springframework.beans.factory.groovy.GroovyBeanDefinitionReader + * GroovyBeanDefinitionReader}; whereas, all other resources will be processed + * with an {@link org.springframework.beans.factory.xml.XmlBeanDefinitionReader + * XmlBeanDefinitionReader}. + * @see #locations * @see #value */ Class reader() default BeanDefinitionReader.class; diff --git a/spring-context/src/test/java/org/springframework/context/annotation/configuration/ImportResourceTests.java b/spring-context/src/test/java/org/springframework/context/annotation/configuration/ImportResourceTests.java index 565b58e9d8..b3811088ee 100644 --- a/spring-context/src/test/java/org/springframework/context/annotation/configuration/ImportResourceTests.java +++ b/spring-context/src/test/java/org/springframework/context/annotation/configuration/ImportResourceTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,8 +16,6 @@ package org.springframework.context.annotation.configuration; -import java.util.Collections; - import org.aspectj.lang.annotation.Aspect; import org.aspectj.lang.annotation.Before; import org.junit.jupiter.api.Test; @@ -30,13 +28,12 @@ import org.springframework.context.annotation.AnnotationConfigApplicationContext import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.ImportResource; -import org.springframework.core.env.MapPropertySource; -import org.springframework.core.env.PropertySource; +import org.springframework.core.testfixture.env.MockPropertySource; import static org.assertj.core.api.Assertions.assertThat; /** - * Integration tests for {@link ImportResource} support. + * Integration tests for {@link ImportResource @ImportResource} support. * * @author Chris Beams * @author Juergen Hoeller @@ -45,81 +42,81 @@ import static org.assertj.core.api.Assertions.assertThat; class ImportResourceTests { @Test - void importXml() { - AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ImportXmlConfig.class); - assertThat(ctx.containsBean("javaDeclaredBean")).as("did not contain java-declared bean").isTrue(); - assertThat(ctx.containsBean("xmlDeclaredBean")).as("did not contain xml-declared bean").isTrue(); - TestBean tb = ctx.getBean("javaDeclaredBean", TestBean.class); - assertThat(tb.getName()).isEqualTo("myName"); - ctx.close(); + void importResource() { + try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ImportXmlConfig.class)) { + assertThat(ctx.containsBean("javaDeclaredBean")).as("did not contain java-declared bean").isTrue(); + assertThat(ctx.containsBean("xmlDeclaredBean")).as("did not contain xml-declared bean").isTrue(); + TestBean tb = ctx.getBean("javaDeclaredBean", TestBean.class); + assertThat(tb.getName()).isEqualTo("myName"); + } } @Test - void importXmlIsInheritedFromSuperclassDeclarations() { - AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(FirstLevelSubConfig.class); - assertThat(ctx.containsBean("xmlDeclaredBean")).isTrue(); - ctx.close(); + void importResourceIsInheritedFromSuperclassDeclarations() { + try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(FirstLevelSubConfig.class)) { + assertThat(ctx.containsBean("xmlDeclaredBean")).isTrue(); + } } @Test - void importXmlIsMergedFromSuperclassDeclarations() { - AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(SecondLevelSubConfig.class); - assertThat(ctx.containsBean("secondLevelXmlDeclaredBean")).as("failed to pick up second-level-declared XML bean").isTrue(); - assertThat(ctx.containsBean("xmlDeclaredBean")).as("failed to pick up parent-declared XML bean").isTrue(); - ctx.close(); + void importResourceIsMergedFromSuperclassDeclarations() { + try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(SecondLevelSubConfig.class)) { + assertThat(ctx.containsBean("secondLevelXmlDeclaredBean")).as("failed to pick up second-level-declared XML bean").isTrue(); + assertThat(ctx.containsBean("xmlDeclaredBean")).as("failed to pick up parent-declared XML bean").isTrue(); + } } @Test - void importXmlWithNamespaceConfig() { - AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ImportXmlWithAopNamespaceConfig.class); - Object bean = ctx.getBean("proxiedXmlBean"); - assertThat(AopUtils.isAopProxy(bean)).isTrue(); - ctx.close(); + void importResourceWithNamespaceConfig() { + try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ImportXmlWithAopNamespaceConfig.class)) { + Object bean = ctx.getBean("proxiedXmlBean"); + assertThat(AopUtils.isAopProxy(bean)).isTrue(); + } } @Test - void importXmlWithOtherConfigurationClass() { - AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ImportXmlWithConfigurationClass.class); - assertThat(ctx.containsBean("javaDeclaredBean")).as("did not contain java-declared bean").isTrue(); - assertThat(ctx.containsBean("xmlDeclaredBean")).as("did not contain xml-declared bean").isTrue(); - TestBean tb = ctx.getBean("javaDeclaredBean", TestBean.class); - assertThat(tb.getName()).isEqualTo("myName"); - ctx.close(); + void importResourceWithOtherConfigurationClass() { + try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ImportXmlWithConfigurationClass.class)) { + assertThat(ctx.containsBean("javaDeclaredBean")).as("did not contain java-declared bean").isTrue(); + assertThat(ctx.containsBean("xmlDeclaredBean")).as("did not contain xml-declared bean").isTrue(); + TestBean tb = ctx.getBean("javaDeclaredBean", TestBean.class); + assertThat(tb.getName()).isEqualTo("myName"); + } } @Test void importWithPlaceholder() { - AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(); - PropertySource propertySource = new MapPropertySource("test", - Collections. singletonMap("test", "springframework")); - ctx.getEnvironment().getPropertySources().addFirst(propertySource); - ctx.register(ImportXmlConfig.class); - ctx.refresh(); - assertThat(ctx.containsBean("xmlDeclaredBean")).as("did not contain xml-declared bean").isTrue(); - ctx.close(); + try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext()) { + ctx.getEnvironment().getPropertySources().addFirst(new MockPropertySource("test").withProperty("test", "springframework")); + ctx.register(ImportXmlConfig.class); + ctx.refresh(); + assertThat(ctx.containsBean("xmlDeclaredBean")).as("did not contain xml-declared bean").isTrue(); + } } @Test - void importXmlWithAutowiredConfig() { - AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ImportXmlAutowiredConfig.class); - String name = ctx.getBean("xmlBeanName", String.class); - assertThat(name).isEqualTo("xml.declared"); - ctx.close(); + void importResourceWithAutowiredConfig() { + try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ImportXmlAutowiredConfig.class)) { + String name = ctx.getBean("xmlBeanName", String.class); + assertThat(name).isEqualTo("xml.declared"); + } } @Test void importNonXmlResource() { - AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ImportNonXmlResourceConfig.class); - assertThat(ctx.containsBean("propertiesDeclaredBean")).isTrue(); - ctx.close(); + try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ImportNonXmlResourceConfig.class)) { + assertThat(ctx.containsBean("propertiesDeclaredBean")).isTrue(); + } } @Configuration @ImportResource("classpath:org/springframework/context/annotation/configuration/ImportXmlConfig-context.xml") static class ImportXmlConfig { + @Value("${name}") private String name; + @Bean public TestBean javaDeclaredBean() { return new TestBean(this.name); } @@ -146,6 +143,7 @@ class ImportResourceTests { @Aspect static class AnAspect { + @Before("execution(* org.springframework.beans.testfixture.beans.TestBean.*(..))") public void advice() { } } @@ -158,16 +156,19 @@ class ImportResourceTests { @Configuration @ImportResource("classpath:org/springframework/context/annotation/configuration/ImportXmlConfig-context.xml") static class ImportXmlAutowiredConfig { - @Autowired TestBean xmlDeclaredBean; - @Bean public String xmlBeanName() { + @Autowired + TestBean xmlDeclaredBean; + + @Bean + public String xmlBeanName() { return xmlDeclaredBean.getName(); } } @SuppressWarnings("deprecation") @Configuration - @ImportResource(locations = "classpath:org/springframework/context/annotation/configuration/ImportNonXmlResourceConfig-context.properties", + @ImportResource(locations = "org/springframework/context/annotation/configuration/ImportNonXmlResourceConfig.properties", reader = org.springframework.beans.factory.support.PropertiesBeanDefinitionReader.class) static class ImportNonXmlResourceConfig { } diff --git a/spring-context/src/test/resources/org/springframework/context/annotation/configuration/ImportNonXmlResourceConfig-context.properties b/spring-context/src/test/resources/org/springframework/context/annotation/configuration/ImportNonXmlResourceConfig.properties similarity index 100% rename from spring-context/src/test/resources/org/springframework/context/annotation/configuration/ImportNonXmlResourceConfig-context.properties rename to spring-context/src/test/resources/org/springframework/context/annotation/configuration/ImportNonXmlResourceConfig.properties From 98cef503fb436633b7fee07fac423773f005b866 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Wed, 21 May 2025 14:53:19 +0200 Subject: [PATCH 2/3] Remove obsolete TODO See gh-10976 --- .../annotation/ConfigurationClassBeanDefinitionReader.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java index 8f019f10ff..f0dab35258 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java +++ b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -385,8 +385,6 @@ class ConfigurationClassBeanDefinitionReader { "Could not instantiate BeanDefinitionReader class [" + readerClass.getName() + "]"); } } - - // TODO SPR-6310: qualify relative path locations as done in AbstractContextLoader.modifyLocations reader.loadBeanDefinitions(resource); }); } From d890a38f3cdcbaf42573351b0ac86217dc0f4dce Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Wed, 21 May 2025 15:31:04 +0200 Subject: [PATCH 3/3] =?UTF-8?q?Support=20registration=20of=20non-public=20?= =?UTF-8?q?BeanDefinitionReader=20via=20@=E2=81=A0ImportResource?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this commit, a BeanDefinitionReader registered via @⁠ImportResource was required to be public and have a public constructor that accepts a single BeanDefinitionRegistry. However, the public visibility requirements are not necessary, and the requirements for the constructor's formal parameter list is not documented. To address those issues, this commit removes the public visibility restrictions and documents that a BeanDefinitionReader registered via @⁠ImportResource must declare a constructor that accepts a single BeanDefinitionRegistry. In addition, this commit includes the cause of the instantiation failure in case the registered BeanDefinitionReader cannot be instantiated. Closes gh-34928 --- ...onfigurationClassBeanDefinitionReader.java | 10 +++++--- .../context/annotation/ImportResource.java | 4 ++++ .../configuration/ImportResourceTests.java | 24 +++++++++++++++++++ 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java index f0dab35258..5c02223ba6 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java +++ b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java @@ -16,6 +16,7 @@ package org.springframework.context.annotation; +import java.lang.reflect.Constructor; import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Arrays; @@ -27,6 +28,7 @@ import java.util.Set; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.beans.BeanUtils; import org.springframework.beans.factory.BeanDefinitionStoreException; import org.springframework.beans.factory.annotation.AnnotatedBeanDefinition; import org.springframework.beans.factory.annotation.AnnotatedGenericBeanDefinition; @@ -371,9 +373,11 @@ class ConfigurationClassBeanDefinitionReader { BeanDefinitionReader reader = readerInstanceCache.get(readerClass); if (reader == null) { try { + Constructor constructor = + readerClass.getDeclaredConstructor(BeanDefinitionRegistry.class); // Instantiate the specified BeanDefinitionReader - reader = readerClass.getConstructor(BeanDefinitionRegistry.class).newInstance(this.registry); - // Delegate the current ResourceLoader to it if possible + reader = BeanUtils.instantiateClass(constructor, this.registry); + // Delegate the current ResourceLoader and Environment to it if possible if (reader instanceof AbstractBeanDefinitionReader abdr) { abdr.setResourceLoader(this.resourceLoader); abdr.setEnvironment(this.environment); @@ -382,7 +386,7 @@ class ConfigurationClassBeanDefinitionReader { } catch (Throwable ex) { throw new IllegalStateException( - "Could not instantiate BeanDefinitionReader class [" + readerClass.getName() + "]"); + "Could not instantiate BeanDefinitionReader class [" + readerClass.getName() + "]", ex); } } reader.loadBeanDefinitions(resource); diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ImportResource.java b/spring-context/src/main/java/org/springframework/context/annotation/ImportResource.java index 3bdf24bac1..dbb1c6047d 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ImportResource.java +++ b/spring-context/src/main/java/org/springframework/context/annotation/ImportResource.java @@ -80,6 +80,10 @@ public @interface ImportResource { * {@link BeanDefinitionReader} implementation to use when processing * resources specified via the {@link #locations() locations} or * {@link #value() value} attribute. + *

The configured {@code BeanDefinitionReader} type must declare a + * constructor that accepts a single + * {@link org.springframework.beans.factory.support.BeanDefinitionRegistry + * BeanDefinitionRegistry} argument. *

By default, the reader will be adapted to the resource path specified: * {@code ".groovy"} files will be processed with a * {@link org.springframework.beans.factory.groovy.GroovyBeanDefinitionReader diff --git a/spring-context/src/test/java/org/springframework/context/annotation/configuration/ImportResourceTests.java b/spring-context/src/test/java/org/springframework/context/annotation/configuration/ImportResourceTests.java index b3811088ee..7b717f2c5a 100644 --- a/spring-context/src/test/java/org/springframework/context/annotation/configuration/ImportResourceTests.java +++ b/spring-context/src/test/java/org/springframework/context/annotation/configuration/ImportResourceTests.java @@ -23,6 +23,7 @@ import org.junit.jupiter.api.Test; import org.springframework.aop.support.AopUtils; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; +import org.springframework.beans.factory.support.BeanDefinitionRegistry; import org.springframework.beans.testfixture.beans.TestBean; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; @@ -109,6 +110,13 @@ class ImportResourceTests { } } + @Test + void importResourceWithPrivateReader() { + try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ImportWithPrivateReaderConfig.class)) { + assertThat(ctx.containsBean("propertiesDeclaredBean")).isTrue(); + } + } + @Configuration @ImportResource("classpath:org/springframework/context/annotation/configuration/ImportXmlConfig-context.xml") @@ -173,4 +181,20 @@ class ImportResourceTests { static class ImportNonXmlResourceConfig { } + @SuppressWarnings("deprecation") + @Configuration + @ImportResource(locations = "org/springframework/context/annotation/configuration/ImportNonXmlResourceConfig.properties", + reader = PrivatePropertiesBeanDefinitionReader.class) + static class ImportWithPrivateReaderConfig { + } + + @SuppressWarnings("deprecation") + private static class PrivatePropertiesBeanDefinitionReader + extends org.springframework.beans.factory.support.PropertiesBeanDefinitionReader { + + PrivatePropertiesBeanDefinitionReader(BeanDefinitionRegistry registry) { + super(registry); + } + } + }