Add RequiresBean and RequiresProperty Annotations

[SentryMan]

• adds RequiresBean and RequiresProperty
• modify bean/method readers to get the prisms and write the conditional statements
• bean types are automatically added to auto requires (to ensure module order is correct)
• modify builder and proxy scope to add contains methods
• no longer register null/Optional.empty() beans

I'm thinking this is a good idea. Will not try to wire the bean if the condition is not met. This used in conjunction with @Secondary could be handy.

@Factory
@RequiresProperty(value="wire.configs", notEqualTo="false")
public class ConfigFactory {

  @Bean
  @RequiresBean(qualifiers="proxy") // some external bean we may provide manually or from another module 
  ProxyConfig proxyConfig() {

    return new ProxyConfig();
  }

  @Bean
  @Secondary // will default to this if not matched
  RealConfig config() {

    return new RealConfig();
  }
}

Generated proxyConfig factory method

  public static void build_proxyConfig(io.avaje.inject.spi.Builder builder) {
    if (!builder.contains("proxy"))
     return;

    if (builder.isAddBeanFor(ProxyConfig.class, Config.class)) {
      var factory = builder.getNullable(ConfigFactory.class); 
      if(factory == null) return;
      var bean = factory.proxyConfig();
      builder.register(bean);
    }
  }

[rbygrave]

@ConditionalOnBean(name="proxy")

name = "proxy" to me does not look like a realistic example in that internally the "name" matches against the keys in DBeanMap which are the type name. So to me this example isn't realistic? Did I misunderstand what name is here? Maybe there should be a test here that shows the intended "name" values.

[rbygrave]

To me I'm almost certain this is a copy of the equivalent spring feature and their string name matches to a full type name ... and we need to use the string rather than Foo.class as Foo.class isn't necessarily on the classpath (so we use the string literal like "org.example.Foo" rather than Foo.class). I suspect this is the same feature but then the "proxy" literal in the example doesn't look right so maybe I don't understand this yet.

[rbygrave]

As some background, as I see it IF the "conditional" in practice translates to ... "wire these 10 beans based on this 1 condition" then custom scopes are a good fit. IF the "conditional" in practice translates to ... "wire this 1 bean based in this 1 condition" then yes @ConditionalOnBean would be a much better fit than a custom scope.

For me, the custom scopes case is the common one I have seen in practice although the good alternative would have been to use a @ConditionalOnBean on the Factory itself rather than each factory method meaning - all the beans this factory creates require that condition.

[SentryMan]

It was pretty late when I wrote this so I guess I was unclear.

their string name matches a full type name ... and we need to use the string rather than Foo.class as Foo.class isn't necessarily on the classpath (so we use the string literal like "org.example.Foo" rather than Foo.class). I suspect this is the same feature but then the "proxy" literal in the example doesn't look right so maybe I don't understand this yet.

well looks like I goofed, I somehow totally misinterpreted how the beanScope contains(String type) worked, and assumed that it worked based on the qualifier name (that is, @Named or @Qualifier annotations).

the good alternative would have been to use a @ConditionalOnBean on the Factory itself rather than each factory method meaning - all the beans this factory creates require that condition.

Oh, this PR does that too, it's why I changed the factory methods to use getNullable.

      var factory = builder.getNullable(ConfigFactory.class); 
      if(factory == null) return; // return if factory is not in scope (only possible when Factory has a conditionOnBean that doesn't match)

[rbygrave]

misinterpreted

I think this "name" area is confusing because Spring DI really does give beans a "name" (not just a qualifier) and that "name" is based on the class short name (TLDR Today this looks like a terrible idea). This was maybe a good idea way back in the day with xml wiring configuration and only a few beans being wired but imo its a terrible idea when we might be wiring hundreds of beans in a large multi-module application because using the class short name is prone to not being unique - its easy for a bean with a completely different type to override another.

Both Micronaut and avaje-inject both do NOT follow Spring DI here with respect to "bean name" and both only using Qualifier + Type.

Depending on who we talk to "name" can be a confusing term and as you say we note @Named is really "Qualifier name".

For myself, I'm thinking I should try and just talk about "Qualifier" and Type (and not use "name" anywhere - the @ConditionalOnBean annotation should use type() and not name()).

Note that Micronaut uses @Requires rather than the multitude of @ConditionOn___ that Spring has. I preferred the @Requires when I last looked at this but I'll have another look and review.

[SentryMan]

ok I can rename

[SentryMan]

ah hang on, I'm inspired now

[SentryMan]

I think I'll split requires into two, one for beans and another for properties. The Micronaut one that is

[SentryMan]

I'm also going to use System.getProperty for the property check, but will use avaje config if available

[SentryMan]

Okay, @rbygrave how's this?

[SentryMan]

@Factory
@RequiresProperty("useHttp")
public class HttpClientFactory {

  @Bean
  @RequiresBean(qualifiers = "someQualifier")
  Builder builder(
      Jsonb jsonb) {
...
  }

  @Bean
  @RequiresBean(missingBeans = ApiClient.class)
  ApiClient client(Builder b) {
...
  }
}

Generated class

@Generated("io.avaje.inject.generator")
public final class HttpClientFactory$DI  {

  /**
   * Create and register HttpClientFactory.
   */
  public static void build(io.avaje.inject.spi.Builder builder) {
    if (Config.getOptional("useHttp")).isEmpty())
     return;

    if (builder.isAddBeanFor(HttpClientFactory.class)) {
      var bean = new HttpClientFactory();
      builder.register(bean);
    }
  }

  /**
   * Create and register Builder via factory bean method HttpClientFactory#builder().
   */
  public static void build_builder(io.avaje.inject.spi.Builder builder) {
    if (!builder.containsQualifier("someQualifier"))
     return;

    if (builder.isAddBeanFor(Builder.class)) {
      var factory = builder.getNullable(HttpClientFactory.class); 
      if(factory == null) return;
      var bean = factory.builder(builder.get(Jsonb.class,"!jsonb"));
      builder.register(bean);
    }
  }

  /**
   * Create and register ApiClient via factory bean method HttpClientFactory#client().
   */
  public static void build_client(io.avaje.inject.spi.Builder builder) {
    if (builder.contains(ApiClient.class))
     return;

    if (builder.isAddBeanFor(ApiClient.class)) {
      var factory = builder.getNullable(HttpClientFactory.class); 
      if(factory == null) return;
      var bean = factory.client(builder.get(Builder.class,"!b"));
      builder.register(bean);
    }
  }

}

[SentryMan]

Hmm, using System.getProperty or avaje config feels like it would be inconsistent in practice. Do you think I should commit to using avaje config and throw illegalStateException when avaje config isn't found while trying to use@RequiresProperty?

[SentryMan]

As some background, as I see it IF the "conditional" in practice translates to ... "wire these 10 beans based on this 1 condition" then custom scopes are a good fit. IF the "conditional" in practice translates to ... "wire this 1 bean based in this 1 condition" then yes @ConditionalOnBean would be a much better fit than a custom scope.

I haven't used custom scopes all that much, but say you're building an auto-configuration library using avaje, with a bunch of beans only supposed to be wired based on a bunch of separate conditions for multiple beans, I'm thinking using custom scopes would become a little unwieldy.

[SentryMan]

In any case, I think I got it working how I want. Please review when you get time.

[rbygrave]

I'll have a look at these points but looking really good.

Maybe the main thing is that I think we should add a new interface like RequiredPropertiesPlugin that has our 3 property checking methods and can implement that via Config or via System properties (or someone can put in their own implementation).

[rbygrave]

The follow up PR is - #315

[SentryMan]

Nice, don't have access to modify that one so I'll pull it over here and add on to it

[rbygrave]

Groovy. I think we merge this in and tweak in master (add javadoc etc)