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

Clarify that all named properties must match for @ConditionalOnProperty to match #40110

Closed
adase11 opened this issue Mar 26, 2024 · 2 comments
Closed
Assignees
Labels
type: documentation A documentation update
Milestone

Comments

@adase11
Copy link

adase11 commented Mar 26, 2024

I think it would be useful to enhance the documentation regarding the expected behavior regarding the name attribute of the ConditionalOnProperty annotation when there are multiple values passed in.

I found myself surprised at the way multiple values are handled in that I was expecting the condition to match if any of the values are present, however the actual behavior requires that all the values are present. (This can be confirmed in the tests ConditionalOnPropertyTests and by looking at OnPropertyCondition).


Separately, changing that behavior would definitely be possible and I can attempt to make a case for why it would be useful to support an any match instead of an all match. I completely understand that changing the behavior has the potential to be a major breaking change and so I could see hesitancy in wanting to fully change the logic, thus the actual intent of this ticket is to request a documentation change. I'm not sure if its a good idea but it could also be possible to support this particular use-case I'm describing with another attribute on ConditionalOnProperty that controls the "name matching style" - as in a boolean toggle to match either any or all of the name values).

Say that you're trying to use @ConditionalOnProperty in combination with some @ConfigurationProperties bean and you want the configuration annotated with @ConditionalOnProperty to be enabled if any of the properties from the @ConfigurationProperties bean are set (because perhaps some may or may not be required). This is not something that's possible to do. Further, you could not use the @Validated to catch the case where one of those properties are set but the other required properties are not, the configuration would just be fully disabled.

For example:

@EnableConfigurationProperties(MyProperties.class)
@ConditionalOnProperty(prefix = "my.props", name = {"first", "second"})
class MyConfiguration {
...
}

@Validated
@ConfigurationProperties(prefix = "my.props")
class MyProperties {

  private final @NotBlank String first;
  private final @NotBlank String second;

  public MyProperties(String first, String second) {
    this.first = first;
    this.second = second;
  }
  ...
}


class MyConfigurationTest {
  private final ApplicationContextRunner contextRunner = new ApplicationContextRunner()
    .withConfiguration(AutoConfigurations.of(MyConfiguration.class));

  @Test
  void configuration_active_property_not_set() {
    contextRunner.withPropertyValues("my.props.first=")
      .run(
        context -> assertThat(context).hasFailed()
          .getFailure()
          .hasCauseInstanceOf(IllegalStateException.class)
          .hasMessageContaining("Failed to bind properties under 'my.props'")
      );
  }
}

Here I'm trying to outline the case I was describing above where the configuration_active_property_not_set test shows that the property is misconfigured (my.props.first is blank and my.props.second is missing). My expectation was that the configuration would be active and that this would fail validation, however the application would actually just start successfully without MyConfiguration being active at all. However I think in this case it seems to me that the "user's" intention was to use MyConfiguration they just misused it (and therefore should be alerted to the mistake via the validation failure).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 26, 2024
@wilkinsona wilkinsona changed the title ConditionalOnProperty Documentation Should Clarify How name Attribute Is Handled With Multiple Values Clarify that all named properties must match for @ConditionalOnProperty to match Mar 27, 2024
@wilkinsona wilkinsona added type: documentation A documentation update and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 27, 2024
@wilkinsona wilkinsona added this to the 3.1.x milestone Mar 27, 2024
@wilkinsona
Copy link
Member

Thanks for the suggestion. We can certainly update the documentation.

We can't change the current behavior as it would be a significant breaking change. We've considered in the past allowing the matching behaviour (OR or AND) to be configured but this was rejected due the extra complexity that it would introduce. We also haven't seen a great deal of demand for it. As an alternative, you can use AnyNestedCondition with two or more nested @ConditionalOnProperty conditions that will then have OR semantics.

@adase11
Copy link
Author

adase11 commented Mar 27, 2024

Definitely makes sense to me. Thanks for considering

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation A documentation update
Projects
None yet
Development

No branches or pull requests

4 participants