Using @ConfigurationProperties methods in @Requires annotation

[GavrilovSV]

This is a possible solution to resolve #5350
I'd like someone to review this PR to find out whether this solution is ok, or some sort of fail fast validation is expected in case of mistyping the method name or providing method with wrong return type. Then I can proceed with the implementation of similar functionality in other annotations like @scheduled etc

[CLAassistant]

All committers have signed the CLA.

[dstepanov]

Micronaut doesn't use reflection, to avoid that you would need to have the method @Executable
But I don't think that is something that should be supported, having a method defined by its name as a string is not the best idea, and how is it better than using the property path?

I would suggest using Condition and @Requires(condition=MyCondition.class).
Or event creating custom annotations and use @SchedulingProperties.Requires and @SchedulingProperties.Scheduled

[graemerocher]

I guess the only advantage of this approach that could be is if some kind of compilation time checking is supported, but even then I don't think it should support methods but only properties. Something like:

@Requires(configuration=TestConfig.class, property="enabled") 

And if TestConfig doesn't define an enabled property then fail compilation. We would also probably want to support value like:

@Requires(configuration=TestConfig.class, property="enabled", value="true")

But there is lots of think about (what about nested properties "foo.bar", what about when property is not specified etc.)

[GavrilovSV]

@graemerocher
I think compilation time checking can be implemented through the usage of recently introduced BeanElementVisitor as a part of validation module

Since the original issue was about avoiding duplication, I would also suggest usage of @ConfigurationProperties prefixes to resolve full path to specified property. For example:

@ConfigurationProperties("outer")
public class OuterConfig {

     @ConfigurationProperties("inner")
     public static class InnerConfig {
            private String innerProperty; 
     }
}

then we could do something like:

@Requires(configProperties = OuterConfig.InnerConfig.class, configProperty = "inner-property")

instead of

@Requires(property = "outer.inner.inner-property")

The problem of this approach is handling default values of properties specified inside @ConfigurationProperties classes. For example:

@ConfigurationProperties("config")
public class Config {

     private String property = "defaultValue";

     public void setProperty(String property) {
          this.property = property;
     }
 
     public String getProperty() {
         return this.property;
     }
}

@Requires(configProperties = Config.class, configProperty = "property")
public class ConditionalBean

In this case I would expect the conditional bean to be created even if I don't explicitly specify the property in external configuration since the default value is already provided inside the class. The same problem arises if I want the configuration property value to equal/not equal certain value:

@Requires(configProperties = Config.class, configProperty = "property", notEquals = "defaultValue")

The only solution I can see is using bean introspection for all @ConfigurationProperties classes, not only those which are explicitly annotated with @Introspected or require constraint validations (as it works at the moment, I guess), but I'm not sure whether extra overhead of such approach worth introducing this feature

[graemerocher]

Yes I think it would have to refer to the non-normalized name of properties only like fooBar (camel case)

[GavrilovSV]

@graemerocher
I've added support for config properties in @Requires and implemented compile time validation. Could someone pls review this PR?

[graemerocher]

@GavrilovSV will do, I think it needs some consideration as it will essentially generate introspections for all configuration properties right? This could have memory implications

[graemerocher]

@dstepanov what do you think? A lot of work has gone into this PR (Thanks!) perhaps there is a way to generate an optimized resolution handle to get the value of the property without an introspection or perhaps generate an introspection only if needed

[GavrilovSV]

Yes, this implementation generates introspections for all configuration properties, even when those classes are not referenced in @requires anywhere. On the other hand it seems to me like the main advantage of configuration properties is validation support, which requires introspections anyway. But if a way to generate introspections only when needed (or avoiding them at all) could be proposed, that would be great

[dstepanov]

Looks good, I think this should also support checking if the configuration is implementing io.micronaut.core.util.Toggleable.
What is going to happen if the configuration is not introspected?

[GavrilovSV]

@dstepanov

I think this should also support checking if the configuration is implementing io.micronaut.core.util.Toggleable.

We can log warning on compilation if ConfigurationProperties class does not implement io.micronaut.core.util.Toggleable. But I think it should be a separate validator in validation module. In this case I would also suggest mentioning in documenation that ConfigurationProperties are expected to implement io.micronaut.core.util.Toggleable

What is going to happen if the configuration is not introspected?

If introspection for config properties class can not be loaded, the conditional bean referencing it will not be loaded as well

[dstepanov]

The idea behind io.micronaut.core.util.Toggleable is that the configuration can define an enabled property that will disable the configuration. So I would expect that the required condition will fail also if the configuration implements Toggleable and returns enabled false.

If introspection for config properties class can not be loaded, the conditional bean referencing it will not be loaded as well

This will be confusing. I would prefer an exception if the property cannot be accessed because of the missing introspection.
Maybe it will be possible to annotate the configuration by the annotation processor.

[graemerocher]

Also I am unsure we should be generating introspections for all configuration properties. A user can always use @Introspected(classes=..) to add any missing introspections

[GavrilovSV]

@dstepanov
I've considered your suggestion regarding Toggeable interface and I've got couple of issues with it:

1. I don't see a way of determining that ConfigurationProperties class implements Toggeable interface in runtime without using reflection (maybe I'm just not aware of the right way to do it)
2. I'm not sure, whether disabling conditional bean in case enabled property on referenced configuration is false would be expected since Toggeable presence doesn't seem to have such impact anywhere else at the moment (correct me if I'm wrong), so I think it may be confusing if it affects only ConfigurationProperties and only if it's referenced in Requires annotation. Anyway, user can just specify desired value for enabled property directy using value or notEquals

@graemerocher
Denis suggested throwing an exception in case introspection is not available for ConfigurationProperties class. I agree that it may be the preferred behaviour, so I think making a user responsible for providing introspections is okay in case it is well documented and a meaningful exception message is provided when introspection is missing

[dstepanov]

1. You can use instanceof after Object bean = beanContext.getBean(beanDefinition);
2. I think that's the idea behind Toggleable - An interface for components or configurations that can be toggled on or off.

[GavrilovSV]

@graemerocher @dstepanov
I've updated the branch, added support for Toggleable feature and refactored a bit. Now an exception is throws in case no introspection is available for configuration properties. I've also reverted changes in IntrospectedTypeElementVisitor, so in current implementation introspections are not created for all configuraiton properties by default. Tests for new features were added as well

[jameskleeh]

Given there isn't anything in this feature that actually requires @ConfigurationProperties, couldn't we reuse @Requires(beans = ConfigPropsBean, property = "someProp"). The implementation doesn't need to care if it's a config props or not.

Edit: I suppose the above example could be a breaking change if people are using the annotation incorrectly. Perhaps add a bean member.

[graemerocher]

One issue that we have to be careful about is that currently @Requires(beans=FooBar.class) doesn't actually materialize the bean it just checks that there is a bean like that in the context. Adding this feature and any variant that has property means that we have the materialize the bean inside of RequiresCondition this can be problematic as it means requires evaluation can result in bean initialization which can lead to circular bean reference problems.

If we isolate it to just configuration properties maybe those can be mitigated somewhat but I am not super keen on adding bean initialization to the @Requires logic 🤔

[jameskleeh]

We should be able to put together the property string from the annotations and convert it to a property check

[graemerocher]

What about @Requires(beans = ConfigPropsBean, property = "someProp", value="blah") ? In order to do the value evaluation you have to init the bean.

[graemerocher]

The current implementation is in fact already doing that

https://github.com/micronaut-projects/micronaut-core/pull/6173/files#diff-abfb539ae66f2056d09796347f04c8e9104dcc8a1a4841a6c712fe7080334953R689-R690

And since the resolution context is not passed it is likely to cause issues

[GavrilovSV]

@graemerocher @jameskleeh
Should anything else be done as part of this PR? The CI pipeline has failed but it seems like its not because of any issues with this PR

[graemerocher]

@jameskleeh ?

[GavrilovSV]

@graemerocher
Can this be merged?

[graemerocher]

Thanks for the contribution!