Introduce DynamicPropertyRegistrar as a replacement for DynamicPropertyRegistry bean support

[philwebb]

#32271 introduced the automatic registration of DynamicPropertyRegistry as a bean that can be used in tests, along with support for @DynamicPropertySource on bean methods. This unfortunately caused spring-projects/spring-boot#41839 due to the fact that Spring Boot already registers a DynamicPropertyRegistry.

In hindsight, I think Spring Boot should not have directly used DynamicPropertyRegistry for Testcontainers support, but instead should have created a new interface that deals with the subtle lifecycle issues we're trying to overcome by triggering container events. I would like to change this in Spring Boot 3.4, but it's hard to do in a back compatible way because DynamicPropertyRegistry is not a Spring Boot interface.

Ideally, in Spring Boot 3.4 we would create a new interface and also create a DynamicPropertyRegistry bean that logs a warning telling folks to migrate. With Spring Framework also adding DynamicPropertyRegistry bean support, this is not possible.

Given that there can be very subtle lifecycle issues with using DynamicPropertyRegistry in a bean (you need to ensure that the bean is created before something else accesses the property). I wonder if we can reconsider the way that Spring Framework will handle this. With the current design, it's quite easy to miss that you should annotate your @Bean method with @DynamicPropertySource to ensure that it gets created early enough. It's also quite easy to miss that the registry is updated as a side-effect of creating the bean. One final problem is that it's a little hard to see how you can update a DynamicPropertyRegistry for an existing bean (rather than one you're creating).

I wonder if we might have time consider a dedicated DynamicPropertyRegistrar interface instead? The interface would accept the DynamicPropertyRegistry to be updated.

In code, I anticipate something like this:

@⁠Bean
DynamicPropertyRegistrar apiServerProperties(ApiServer apiServer) {
    return (registry) -> registry.add("api.url", apiServer::getUrl);
}

I'm not 100% sure if this will work, but if it does it would free up the DynamicPropertyRegistrar bean for Spring Boot to create and use to log migration tips. It would also make it impossible to use DynamicPropertyRegistrar and forget to add @DynamicPropertySource. Finally, it would give a good place to document how a DynamicPropertyRegistrar is created early and to be aware of lifecycle issues. Typically, Testcontainer properties aren't a big problem because the Container beans are distinct from the application. For regular app, the lifecycle issues could be very hard to track down.

[wilkinsona]

See spring-projects/spring-boot#41276 for an example of the sort of lifecycle problem that's created by DynamicPropertyRegistry being a bean and configuring properties as a side-effect of its injection into a @Bean method.

[sbrannen]

Thanks for raising your concerns, @philwebb.

In hindsight, I think Spring Boot should not have directly used DynamicPropertyRegistry for Testcontainers support, but instead should have created a new interface that deals with the subtle lifecycle issues we're trying to overcome by triggering container events.

Yes, I agree with that.

With the current design, it's quite easy to miss that you should annotate your @Bean method with @DynamicPropertySource to ensure that it gets created early enough.

The decision to use @DynamicPropertySource like that was intentional, and it's well documented in Javadoc and the reference manual. We wanted to allow people to opt into eager initialization of such beans. Though, I can understand that the inverse might be preferable in most scenarios.

It's also quite easy to miss that the registry is updated as a side-effect of creating the bean.

I don't see how that would be the case. The code has to invoke registry.add(...) which makes it quite clear what is going on.

Or are you referring to a different scenario/perspective?

One final problem is that it's a little hard to see how you can update a DynamicPropertyRegistry for an existing bean (rather than one you're creating).

Indeed. That's currently possible but not exactly straightforward.

I wonder if we might have time consider a dedicated DynamicPropertyRegistrar interface instead? The interface would accept the DynamicPropertyRegistry to be updated.

In code, I anticipate something like this:

@⁠Bean
DynamicPropertyRegistrar apiServerProperties(ApiServer apiServer) {
    return (registry) -> registry.add("api.url", apiServer::getUrl);
}

I can see the advantages of implementing the feature that way, as it addresses some of the concerns you have raised.

The only downside I can foresee is that any bean injected into such a DynamicPropertyRegistrar @Bean factory method would always be eagerly initialized. Though, as I mentioned above, that might actually be the preferred behavior for most common use cases of the feature. If we go this route, we could always consider allowing such factory methods to be @Lazy, but then again, if nobody needs that "lazy" behavior, we could just leave it as-is (always eager).

I'm not 100% sure if this will work, but if it does it would free up the DynamicPropertyRegistrar bean for Spring Boot to create and use to log migration tips.

Assuming you meant the DynamicPropertyRegistry bean, yes, that could allow Boot to continue creating that bean instead of Framework.

But then Framework would be responsible for processing all DynamicPropertyRegistrar beans in the context to populate the DynamicValuesPropertySource, and Boot would have to avoid interacting directly with DynamicPropertyRegistrar beans.

It would also make it impossible to use DynamicPropertyRegistrar and forget to add @DynamicPropertySource.

True. We would no longer need to support @DynamicPropertySource on @Bean methods.

Since Spring Framework 6.2 RC1 is scheduled for release in two days, we don't have much time left, but we will discuss our options within the team.

From my point of view, if we want to enable a migration path for Boot's use of DynamicPropertyRegistry, I think we have the following options:

1. Collaborate with the Boot team to revise Framework's handling of DynamicPropertyRegistry to ensure that both the TestContext framework and Boot's testing support have what they need in terms of features.
2. Completely revert the DynamicPropertyRegistry support introduced in Spring Framework 6.2 M2.
3. Introduce something along the lines of the proposed DynamicPropertyRegistrar as a replacement for the DynamicPropertyRegistry support introduced in Spring Framework 6.2 M2.

Though, to be honest, I'm not sure how plausible option 1 is.

Thoughts?

[sbrannen]

See spring-projects/spring-boot#41276 for an example of the sort of lifecycle problem that's created by DynamicPropertyRegistry being a bean and configuring properties as a side-effect of its injection into a @Bean method.

Just to clarify: Wouldn't annotating the lgtmContainer() method in that sample application with @DynamicPropertySource address that issue?

[wilkinsona]

As I understand it, it would help for Boot 3.4 (where we depend Framework 6.2), but not for earlier versions of Boot as DynamicPropertySourceBeanInitializer is new in Framework 6.2.

Even with Boot 3.4 and the possibility of using @DynamicPropertySource to trigger eager initialization, I share Phil's concern about the fragility of this approach and I linked to spring-projects/spring-boot#41276 as I think it's a good example of this. The explanation in that issue of why the problem exists is lengthy and I would prefer that users didn't have to understand things at that level to know whether or not @DynamicPropertySource is needed.

[sbrannen]

I pushed a proof of concept for DynamicPropertyRegistrar support to the following feature branch.

main...sbrannen:spring-framework:issues/gh-33501-DynamicPropertyRegistrar

As stated in the commit message:

It appears that most things work as expected; however, in this POC there are two instances of DefaultDynamicPropertyRegistry that prevent the use of @⁠DynamicPropertySource and DynamicPropertyRegistrar in the same ApplicationContext, since the two features end up writing to two different valueSuppliers maps. The commented-out "magic.word" assertions in DynamicPropertySourceIntegrationTests therefore currently fail.

@philwebb and @jhoeller, I'd appreciate it you could take a quick look to confirm this is the direction we want to go.

If so, I'll work out the aforementioned bug and update the Javadoc and reference manual accordingly.

[philwebb]

It's also quite easy to miss that the registry is updated as a side-effect of creating the bean.

I don't see how that would be the case. The code has to invoke registry.add(...) which makes it quite clear what is going on.

Certainly scanning the implementation will make it obvious, but for me it's a bit of an unusual pattern for a @Bean method. I typically use @Bean method parameters for things that are being injected, but this one is slightly special. It's injecting something that isn't used in the returned bean. It means that the method always does two things, create the bean instance and configure the DynamicPropertyRegistry.

The only downside I can foresee is that any bean injected into such a DynamicPropertyRegistrar @Bean factory method would always be eagerly initialized

Yes, that might be a problem. I do wonder how useful not doing early initialization will be for most users. I think typically, you want to configure something in the Environment that will be used by another bean. At least, that's typical for the Testcontainers use-case. I guess it's possible that you might be accessing the Environment directly in your own code and you know that the everything will be fully initialized by the time you actually read the property.

From my point of view, if we want to enable a migration path for Boot's use of DynamicPropertyRegistry, I think we have the following options:

I agree that option 1 seems quite difficult given the timescales. Even option 3 feels tight. I guess if we can live with delaying the feature until Spring Framework 7, that would be my preference. We still have time in Spring Boot to deprecate our use of DynamicPropertyRegistry which would at least free up that bean to be used however Framework wishes in 7.0

[sbrannen]

@philwebb and @wilkinsona, in the end we decided to go with the DynamicPropertyRegistrar approach for Spring Framework 6.2.

For details, see e7b52cf.

And, Phil, thanks again for the DynamicPropertyRegistrar proposal! We think it turned out better as a result, and we're glad to have gotten this sorted out before 6.2 GA. 👍

[wilkinsona]

Thanks for this @sbrannen.

I'm still exploring all of the different permutations, but I think there's a possibility that Boot may be able to use DynamicPropertyRegistrar with Testcontainers and we won't need a Testcontainers-specific alternative. For that to hold true, DynamicPropertyRegistrar would have to be available when using Testcontainers at development time. In that situation, there's a dependency on spring-test but the test context framework isn't being used.

While we're exploring things and evaluating our options, it would be useful to know if you'd be open to allowing Boot to bootstrap Framework's DynamicPropertyRegistrar support. Looking at the code, making DynamicPropertyRegistrarBeanInitializer public would be one way to do so as we could then define a bean for the initializer as DynamicPropertiesContextCustomizer does in the test context framework.