Skip to content

Introduce ability to add to the Environment using properties of a bean #32209

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

Closed
rwinch opened this issue Feb 6, 2024 · 4 comments
Closed

Introduce ability to add to the Environment using properties of a bean #32209

rwinch opened this issue Feb 6, 2024 · 4 comments
Assignees
Labels
in: test Issues in the test module status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement

Comments

@rwinch
Copy link
Member

rwinch commented Feb 6, 2024

It would be nice to be able to add properties to the Environment based upon properties of a bean. Spring Boot Testjars does this using the code in org.springframework.experimental.boot.test.context.

For example, the following code will add a property named messages.url to the environment with the value of http://localhost:1234 assuming that the messagesApiServer bean has a property named port that returns 1234.

The value of the annotation is a SpEL expression with the bean as the root of the expression.

@Bean
@DynamicProperty(name = "messages.url", value = "'http://localhost:' + port")
static CommonsExecWebServerFactoryBean messagesApiServer() {
  // ...
}

Composed annotations are also supported.

The following code allows @MessageUrl to be used in place of the @DynamicProperty annotation above.

@Retention(RetentionPolicy.RUNTIME)
@DynamicProperty(name = "message.url", value = "'http://localhost:' + port")
public @interface MessageUrl {
}

Attributes in the composed annotation are also supported:

@Retention(RetentionPolicy.RUNTIME)
@DynamicProperty(name = "${prefix}.url", value = "'http://localhost:' + port")
public @interface MessageUrl {
  String prefix() default  "messages";
}

The default property name is now messages.url. However, if a user provides the annotation @MessageUrl(prefix="foo") the property name would be foo.url.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 6, 2024
@jhoeller jhoeller added the in: core Issues in core modules (aop, beans, core, context, expression) label Feb 6, 2024
@sbrannen sbrannen changed the title Add Ability to Add to Environment using Properties on a Bean Introduce ability to add to the Environment using properties of a bean Feb 7, 2024
@jhoeller jhoeller added in: test Issues in the test module and removed in: core Issues in core modules (aop, beans, core, context, expression) labels Feb 7, 2024
@sbrannen sbrannen self-assigned this Feb 7, 2024
@sbrannen
Copy link
Member

sbrannen commented Feb 7, 2024

Hi @rwinch,

Thanks for the proposal! 👍

We discussed this topic within the team, and we are hesitant to introduce such a feature that is declarative and relies on SpEL expressions.

However, we came up with a counterproposal: register a DynamicPropertyRegistry as a singleton bean in the container which can then be injected into @Configuration classes or directly in @Bean methods.

I put together a proof of concept in the following feature branch.

main...sbrannen:spring-framework:issues/gh-32209-register-DynamicPropertyRegistry-as-bean

Take a look at DynamicPropertyRegistryIntegrationTests to see it in use.

The basic idea can be seen in this @Bean method:

@Bean
ApiServer apiServer(DynamicPropertyRegistry registry) {
	ApiServer apiServer = new ApiServer();
	registry.add("api.url", apiServer::getUrl);
	return apiServer;
}

That follows the same programming model as a @DynamicPropertySource method within a test class.

The @Test methods in DynamicPropertyRegistryIntegrationTests demonstrate what's possible with this approach and what's not.

The Environment contains a DynamicValuesPropertySource which makes the dynamic properties available, and the singleton DynamicPropertyRegistry bean is tied to that DynamicValuesPropertySource.

Thus, the Environment can be injected into EnvironmentInjectedService which lazily retrieves dynamic properties from the environment.

DependentService demonstrates another use case which works: it has an @Autowired setter method that retrieves a dynamic property via @Value. To ensure the dynamic property is available, the @Bean method for DependentService is annotated with @DependsOn("apiServer").

However, if we don't make use of @DependsOn we see that a bean cannot reliably retrieve a dynamic property. This is showcased in ValueInjectedService and the associated test method.

I suppose your proposal for Spring Boot Testjars exhibits the same behavior, but if that's not the case, please let us know.

In any case, please let us know what you think of our counterproposal.

Cheers,

Sam

p.s. @philwebb, you may also be interested in this, especially since I took some inspiration from Boot's TestcontainersPropertySource.

@sbrannen sbrannen added the status: waiting-for-feedback We need additional information before we can continue label Feb 7, 2024
@rwinch
Copy link
Member Author

rwinch commented Feb 7, 2024

Thank you @sbrannen!

We discussed this topic within the team, and we are hesitant to introduce such a feature that is declarative and relies on SpEL expressions.

Can you help me to understand the reason that a declarative feature should not be using SpEL? I ask because we can use SpEL with @Value which seems to be quite similar. @DynamicProperty is just the value (@Value) of a specific property in the Environment.

However, we came up with a counterproposal

I think that the main piece that is missing with the counter proposal is an easy way for the framework to easily provide well known configurations. For example, to specify the issuer URI in configuration this proposal needs to be:

@Bean
static CommonsExecWebServerFactoryBean authorizationServer(DynamicPropertyRegistry registry) {
  CommonsExecWebServerFactoryBean result = ...
   // NOTE: getObject throws Exception which doesn't work with Supplier
  registry.add("spring.security.oauth2.client.provider.spring.issuer-uri", () -> "http://127.0.0.1:" + result.getObject().getPort());
  return result;
}

With the current solution, users can do this

@Bean
@OAuth2ClientProviderIssuerUri
static CommonsExecWebServerFactoryBean authorizationServer() {
  return CommonsExecWebServerFactoryBean.builder()
    // ...
}

They can also easily override the providerName (replace "spring" with a new value) using:

@Bean
@OAuth2ClientProviderIssuerUri(providerName = "myProviderName")
static CommonsExecWebServerFactoryBean authorizationServer() {
  return CommonsExecWebServerFactoryBean.builder()
    // ...
}

I suppose your proposal for Spring Boot Testjars exhibits the same behavior, but if that's not the case, please let us know.

This is not the case with my solution. I added the tests to a branch without the @DependsOn and it passes.

The reason that @DependsOn is not needed is because the support implements ImportBeanDefinitionRegistrar to preemptively process @DynamicProperty annotated beans.

I'm also not a fan of the need for @DependsOn with the counter-proposal.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 7, 2024
@bclozel bclozel changed the title Introduce ability to add to the Environment using properties of a bean Introduce ability to add to the Environment using properties of a bean Feb 14, 2024
@sbrannen sbrannen changed the title Introduce ability to add to the Environment using properties of a bean Introduce ability to add to the Environment using properties of a bean Feb 14, 2024
@sbrannen
Copy link
Member

sbrannen commented Mar 16, 2024

Hi @rwinch,

After further discussions within the team, we decided to register DynamicPropertyRegistry as a singleton in the test's ApplicationContext (see #32271).

Can you help me to understand the reason that a declarative feature should not be using SpEL?

We are not saying that a declarative feature should not use SpEL. Rather, we are saying that we do not want to introduce this feature into the core framework. Providing a general-purpose SpEL-based solution that suits all use cases for the community might not be trivial. One has to take into consideration what types of annotation attributes are supported, how they are converted/included in the SpEL expression, how nested annotations are handled, whether arrays are supported as annotation attributes, etc.

As an alternative to an annotation-based approach (using SpEL and property placeholders), one may choose a programmatic approach, providing a DSL/utility that works with DynamicPropertyRegistry and handles prefixes, suffixes, etc. transparently behind the scenes.

#32271 should therefore serve as a building block within the core framework that portfolio projects and third parties can build on, potentially using SpEL as you proposed in this issue.

Regarding the need for @DependsOn in the counterproposal, we plan to address that in #32271 as well.

In light of the above, I am closing this issue as:

And we encourage you and @philwebb to review the result of #32271 once it's on main and let us know if there is anything else we can do to simplify working with DynamicPropertyRegistry.

Cheers,

Sam

@sbrannen sbrannen closed this as not planned Won't fix, can't repro, duplicate, stale Mar 16, 2024
@sbrannen sbrannen added status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Mar 16, 2024
sbrannen added a commit to sbrannen/spring-framework that referenced this issue May 11, 2024
@sbrannen
Copy link
Member

Hi @rwinch and @philwebb,

I just wanted to let you know that #32271 has been resolved and was included in Spring Framework 6.2 M2.

In summary...

  • The DynamicPropertyRegistry is now registered as a singleton bean in a test's ApplicationContext which allows it to be injected into @Configuration classes and @Bean methods.
  • The DynamicValuesPropertySource is registered in the Environment eagerly whenever @DynamicPropertySource is used within a test class and otherwise lazily whenever a component actually invokes add() on the DynamicPropertyRegistry bean (see Lazily register DynamicValuesPropertySource in the TestContext framework #32871).
  • @DependsOn is no longer necessary like it was in the original counter-proposal.
  • @DynamicPropertySource can now optionally be applied to a @Bean method to signal that the corresponding bean should be eagerly initialized before singletons are instantiated.
  • Details and examples can be found in the reference manual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants