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

DynamicPropertyRegistry in @Bean-method fails with spring-boot-testcontainers dependency in classpath (3.4.0-M1) #41839

Closed
nilshartmann opened this issue Aug 13, 2024 · 5 comments
Labels
type: regression A regression from a previous release
Milestone

Comments

@nilshartmann
Copy link
Contributor

nilshartmann commented Aug 13, 2024

In Spring Boot until 3.3.x I was able to inject an instance of DynamicPropertyRegistry into a @Bean configuration in my Test classes like this:

@TestConfiguration
public class DemoTestConfig {
    @Bean
    DummyService dummyService(DynamicPropertyRegistry registry) {
        registry.add("demo.test.prop", () -> "Hello World");

        return new DummyService();
    }
}

In Spring Boot 3.4.0-M1 this also does work, but only as long as I do not have spring-boot-testcontainers in classpath/pom.xml. With Testcontainers in classpath, Spring Boots complains that two instances of DynamicPropertyRegistry are found:

Parameter 0 of method dummyService in com.example.demo.DemoTestConfig required a single bean, but 2 were found:
	- dynamicPropertyRegistry: defined by method 'dynamicPropertyRegistry' in class path resource [org/springframework/boot/testcontainers/properties/TestcontainersPropertySourceAutoConfiguration.class]
	- org.springframework.test.context.support.DynamicPropertiesContextCustomizer.dynamicPropertyRegistry: a programmatically registered singleton

In 3.3.x it works with and without spring-boot-testcontainers in pom.xml.

I created an example project that shows the behaviour with Spring Boot 3.3.2 and 3.4.0-M1 here: https://github.com/nilshartmann/spring-boot-3-4-dynamic_properties. See README.md there how to run the test in 3.3.2 and the failing one in 3.4.0 using Maven.

In Spring 6.2.0-M1 I saw this addition recently spring-projects/spring-framework#32271, maybe that is related?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 13, 2024
@wilkinsona
Copy link
Member

Thanks for the report.

In Spring 6.2.0-M1 I saw this addition recently spring-projects/spring-framework#32271, maybe that is related?

Yes, that's the underlying cause of the regression. Hopefully we can adapt to it in Boot, but we may need a Framework change.

@wilkinsona wilkinsona added type: regression A regression from a previous release and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 13, 2024
@wilkinsona wilkinsona added this to the 3.4.x milestone Aug 13, 2024
@wilkinsona
Copy link
Member

FYI, @sbrannen. It's not immediately clear to me how to address this one as there's no easy way to compose DynamicPropertyRegistry implementations and the event publishing that we require for Testcontainers isn't supported by the Framework implementation.

philwebb added a commit that referenced this issue Aug 22, 2024
Remove the Spring Framework registered `DynamicPropertyRegistry` when
using Testcontainers.

See gh-41839
@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Aug 22, 2024
@philwebb
Copy link
Member

I've put a work-around in for M2, but I think we might need a better long-term solution. In hindsight, we should probably not have used DynamicPropertyRegistry but instead have our own interface.

@wilkinsona
Copy link
Member

I've opened #41996 to follow up on a better long-term solution so that we can close this one and have it appear in the changelog.

@wilkinsona wilkinsona modified the milestones: 3.4.x, 3.4.0-M2 Aug 22, 2024
@wilkinsona wilkinsona removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Aug 22, 2024
@nilshartmann
Copy link
Contributor Author

@wilkinsona @philwebb thanks a lot for the fix! Will try the workaround when M2 is available. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression A regression from a previous release
Projects
None yet
Development

No branches or pull requests

4 participants