Introduce dedicated annotation to deal with boolean property conditions

[philwebb]

As shown in #43641 we have inconsistencies with the way we use @ConditionalOnProperty for enabled annotations. Introducing a dedicated set of annotations will help remove the inconsistencies and allow us to identify more easily configurations which depend on .enabled properties.

[philwebb]

I'd like the rest of the team to review https://github.com/philwebb/spring-boot/tree/gh-43704 before we merge.

[quaff]

Nice work, one small suggestion, maybe the name of annotations should reflect that .enabled suffix are using, how about:

ConditionalOnNoOptOut -> ConditionalOnImplicitEnabled
ConditionalOnOptOut -> ConditionalOnNotEnabled or ConditionalOnExplicitNotEnabled
ConditionalOnOptIn -> ConditionalOnEnabled or ConditionalOnExplicitEnabled

[nosan]

Honestly, when I saw those annotations for the first time, I had no clue which one I should use and when until I read Javadoc.

My suggestion is to have two annotations:

@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.TYPE, ElementType.METHOD })
@Documented
@ConditionalOnProperty(name = "enabled", havingValue = "true")
public @interface ConditionalOnEnabledProperty {
	@AliasFor(annotation = ConditionalOnProperty.class, attribute = "prefix")
	String value();
	@AliasFor(annotation = ConditionalOnProperty.class, attribute = "prefix")
	String prefix();
	@AliasFor(annotation = ConditionalOnProperty.class, attribute = "matchIfMissing")
	boolean matchIfMissing () default false;
}

@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.TYPE, ElementType.METHOD })
@Documented
@ConditionalOnProperty(name = "enabled", havingValue = "false")
public @interface ConditionalOnNotEnabledProperty {
	@AliasFor(annotation = ConditionalOnProperty.class, attribute = "prefix")
	String value();
	@AliasFor(annotation = ConditionalOnProperty.class, attribute = "prefix")
	String prefix();
	@AliasFor(annotation = ConditionalOnProperty.class, attribute = "matchIfMissing")
	boolean matchIfMissing () default false;
}

In that case, it would be possible to cover all possible outcomes:

//spring.jmx.enabled property must be present and has 'true' value
@ConditionalOnEnabledProperty("spring.jmx")
static class JmxConfiguration{}

//spring.jmx.enabled property may be empty but if present must have 'true' value
@ConditionalOnEnabledProperty(prefix = "spring.jmx", matchIfMissing = true)
static class JmxConfiguration{}

//-------------------------------


//spring.jmx.enabled property must be present and has 'false' value
@ConditionalOnNotEnabledProperty("spring.jmx")
static class JmxConfiguration{}

//spring.jmx.enabled property may be empty but if present must have 'false' value
@ConditionalOnNotEnabledProperty(prefix = "spring.jmx", matchIfMissing = true)
static class JmxConfiguration{}

[wilkinsona]

+1 for a pair of annotations, although I'd maybe use Disabled rather than NotEnabled:

• @ConditionalOnEnabledProperty
• @ConditionalOnDisabledProperty

Not 100% sure on this. Does ConditionalOnDisabledProperty suggest that the property's name would be .disabled more so than ConditionalOnNotEnabledProperty suggests that it would be .not-enabled?

I wonder if the Property suffix is needed? Perhaps it is to avoid confusion with the other @ConditionalOnEnabled… annotations.

I also wonder about renaming matchIfMissing. Something like enabledByDefault and disabledByDefault instead? Does that make the attribute's purpose more clear?

[nosan]

I was thinking about @ConditionalOnDisabledProperty instead of ConditionalOnNotEnabledProperty but for me, the first one sounds like the name of the property should be disabled rather than enabled with havingValue = false.

Perhaps it is to avoid confusion with the other @ConditionalOnEnabled… annotations.

Exactly

I also wonder about renaming matchIfMissing. Something like enabledByDefault and disabledByDefault instead? Does that make the attribute's purpose more clear?

For me, both enabledByDefault and matchIfMissing sounds good. Maybe matchIfMissing is preferable because is a well-known attribute for a long time.

[philwebb]

Thanks for all the feedback!

My suggestion is to have two annotations:

I didn't think we could alias a matchIfMissing boolean attribute to a matchIfMissing String, but perhaps we can. If not, we could easily develop a new annotation and not use @AliasFor.

+1 for a pair of annotations, although I'd maybe use Disabled rather than NotEnabled:

I'm not so keen on Disabled because it makes me think there's a foo.disabled=true property rather than foo.enabled=false.

---

I'm starting to wonder if we should have a single dedicated annotation and always be explicit about what we want. It might be more verbose, but it will at least be clear. The annotations in my branch map to one of three options:

@ConditionalOnProperty(name = "enabled", matchIfMissing = true,  havingValue = "true")
@ConditionalOnProperty(name = "enabled", matchIfMissing = false, havingValue = "true")
@ConditionalOnProperty(name = "enabled", matchIfMissing = false, havingValue = "false")

I'm thinking perhaps an enum attribute value on a single annotation might work well:

@ConditionalOnEnabledProperty(prefix="spring.jmx", havingValue=Enabled.TRUE)
@ConditionalOnEnabledProperty(prefix="spring.jmx", havingValue=Enabled.TRUE_OR_MISSING)
@ConditionalOnEnabledProperty(prefix="spring.jmx", havingValue=Enabled.FALSE)

[nosan]

I am wondering if the following syntax could be used:

@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.TYPE, ElementType.METHOD })
@Documented
//custom condition (alias for `@ConditionalOnProperty.havingValue` does not work (type mismatch) )
@Conditional(OnEnabledPropertyCondition.class)
public @interface ConditionalOnEnabledProperty {
	String prefix();
	boolean matchIfMissing() default false;
	Enabled havingValue() default Enabled.TRUE;
	enum Enabled {
		FALSE, TRUE
	}
}

@ConditionalOnEnabledProperty(prefix = "management.tracing.baggage")
@ConditionalOnEnabledProperty(prefix = "server.servlet.encoding", matchIfMissing = true)
@ConditionalOnEnabledProperty(prefix = "management.tracing.baggage", havingValue = Enabled.FALSE)