Clarify the required behaviour of @ConfigProperties beans

[JHahnHRO]

The Spec does not explicitly state what kind of CDI Bean a class annotated with @ConfigProperties should give rise to. The way I understand it, it MUST be a dependent bean. My reasons are:

1. The class itself from the example is annotated with @Dependent
2. The prefix attribute in @ConfigProperties is @NonBinding. That means that the two example injection points

@Inject
@ConfigProperties
Details serverDetails;

and

@Inject
@ConfigProperties(prefix="client")
Details clientDetails;

are expected to resolve to the same bean. The only way the CDI spec allows for one bean to create two different instances depending on the injection point is for the bean to be @Dependent scoped.
3. Programmatic lookup with arbitrary prefixes only works if it is a @Dependent bean for the same reason.

HOWEVER: In the TCK there is a test with a @RequestScoped @ConfigProperties bean.
The only way for a normal scoped bean to give different instances for different injection points is to have the prefix attribute be binding again. And in fact, that is what smallrye does: It removes the @NonBinding annotation during BeforeBeanDiscovery and creates many, many synthetic beans from one @ConfigProperties class - one for each prefix that is discovered at injection points. And all of them are @Dependent scoped, even though the class specified @RequestScoped. This feels like cheating.

But that has another consequence: Programmatic lookup of @ConfigProperties no longer works as I would expect it. Again, the spec is not 100% clear here. The only example given is

Details details = CDI.current().select(Details.class, ConfigProperties.Literal.NO_PREFIX).get();

Does that mean that this is the only case of programmatic lookup that is required to work? Or is it required that programmatic lookup with arbitrary prefixes works? I would have expected the latter to be the case (and indeed I have filed this as a smallrye bug )

The same TCK case also contains the bean

@ConfigProperties(prefix = "cloud")
public static class BeanFour {
    @ConfigProperty(name = "a.host", defaultValue = "mycloud.org")
    private String host;
    public int port = 9080;
    public String getHost() {
        return host;
    }
    public Optional<String> location;
}

and the way it is used in the test case suggests that the field initializer replaces the need for a default value.
Where does this requirement come from? I cannot find it in the spec. And it is ... let's say weird ... from an implementation stand point. I tried to find the place where this is implementation in smallrye and could not find it. Maybe it is very well hidden, maybe smallrye just accidentally satisfies this testcase?

[radcortez]

Yes, I agree that @ConfigProperties#prefix should be binding. I've discussed that at some point in #629, but I ended up moving in the wrong direction :(

I also think that @ConfigProperties should not be @RequestScoped. While technically possible, I think it would only generate uncertainty and frustration because the config result could be very different (because MP Config, leaves the door open for dynamic configuration).

From the SmallRye Config side, we preferred to support something more like SmallRye Config @ConfigMapping. At the very least, use an interface instead of a class. Unfortunately, that was not possible. As you discovered, it creates weird and annoying edge cases (that interfaces do not have). Yes, @ConfigProperties can initialize a configuration via a field initializer because we cannot go against the language rules. In the case of SmallRye Config, we handle that piece here: https://github.com/smallrye/smallrye-config/blob/1483a2ed18ddbc7e61664f16a1f3952958b52724/implementation/src/main/java/io/smallrye/config/ConfigMappingGenerator.java#L282-L294

[JHahnHRO]

Yes, I agree that @ConfigProperties#prefix should be binding

That's not exactly what I said...

Yes, @ConfigProperties can initialize a configuration via a field initializer because we cannot go against the language rules

One doesn't have to. I mean it's not part of the spec. Why not simply delete that testcase from the TCK and make having field initializers undefined behaviour? Think about it this way: Except from the ability to use the same class with multiple prefixes, conceptually

@Dependent
@ConfigProperties(prefix="server")
class Server {
   String host;
   int port;
}

and

@Dependent
class Server {
   @ConfigProperty(name="server.host") String host;
   @ConfigProperty(name="server.port") int port;
}

should work more or less the same, right? In the second bean, you certainly could have field initializers. They just wouldn't do anything, because the field values would get overridden when injection happens. So why not ignore them altogether?

As for supporting only interfaces... I think I'm coming around to this point of view the longer I think about the edge cases of the class based approach. On the other hand: I'd very much like records to be supported in the future as well.

[JHahnHRO]

Looking at it again, the TCK testcase is fundamentally broken, even if we were of the opinion that @RequestScoped @ConfigProperties beans should be allowed. The assertion in line 145 will always fail for a request scoped bean, because the instance is only a client proxy, not the "real" bean instance, so the field will have its default value, not the injected value.
(I'm also unsure if the method call in line 144 would fail with a ContextNotActiveException, because the request scope does not seem to be active. But maybe that's just my inexperience with Arquillian.)

[radcortez]

should work more or less the same, right? In the second bean, you certainly could have field initializers. They just wouldn't do anything, because the field values would get overridden when injection happens. So why not ignore them altogether?

Sure, they could be ignored, but what if you have a field initialized, but there is no configuration for that property name? For configurations not found, we fail the config. Should we fail in this case? I see arguments for both cases, but I feel it would look weird failing because there is no value for that field, when it was initialized directly.

As for supporting only interfaces... I think I'm coming around to this point of view the longer I think about the edge cases of the class based approach. On the other hand: I'd very much like records to be supported in the future as well.

We had that experience with Quarkus, since initially, we were also using the class base approach and it proved to be too troublesome, so we switched to the interface approach. Unfortunately, our experience was not enough to convince this group. It does seem that in Jakarta Config, the group came around and now supports the interface approach. I did write a discussion about this: jakartaee/config#122. We never even considered records, because of the language level in MP.

Looking at it again, the TCK testcase is fundamentally broken, even if we were of the opinion that @RequestScoped @ConfigProperties beans should be allowed. The assertion in line 145 will always fail for a request scoped bean, because the instance is only a client proxy, not the "real" bean instance, so the field will have its default value, not the injected value.
(I'm also unsure if the method call in line 144 would fail with a ContextNotActiveException, because the request scope does not seem to be active. But maybe that's just my inexperience with Arquillian.)

I think we should propose removing this particular test in the TCK.

[JHahnHRO]

but I feel it would look weird failing because there is no value for that field, when it was initialized directly.

Why? That's just how CDI-stuff works, isn't it? If annotate a field with @Inject I need a bean matching it, no matter if there is a field initializer or not.

I think we should propose removing this particular test in the TCK.

Great. Where do I sign?

[radcortez]

Why? That's just how CDI-stuff works, isn't it? If annotate a field with @Inject I need a bean matching it, no matter if there is a field initializer or not.

In one of the cases you posted:

@Dependent
@ConfigProperties(prefix="server")
class Server {
   String host;
   int port;
}

You don't need any @Inject of @ConfigProperty annotation. The value is automatically retrieved from the config. In this case, it would look weird in my opinion, but if people are ok with it, I'm not going to oppose changing it.

Great. Where do I sign?

Just submit a PR with the change :)

[JHahnHRO]

What I meant was: The @ConfigProperties bean and the bean with manual injection of the two field should behave the same for consistency's sake. It should be possible to use the two options interchangeably (if there only ever is one relevant prefix). Currently, they are slightly different with respect to default values. That one of the options has an undocumented and unspecified way of defining defaults that the other does not have, is a bug, not a feature in my mind.

Pull request is created.

[Emily-Jiang]

The Spec does not explicitly state what kind of CDI Bean a class annotated with @ConfigProperties should give rise to. The way I understand it, it MUST be a dependent bean. My reasons are:

The class itself from the example is annotated with @Dependent
The prefix attribute in @ConfigProperties is @NonBinding. That means that the two example injection points

Catching with this thread. First of all, the bean can be any scoped. RequestScoped is perfectly fine. The reason that the annotation @ConfigProperties is non binding is that the impl just create one proxy bean to then supply different values with the prefix. If it is binding, you need to have the bean ready to be resolved based on the prefix. Otherwise, your app won't start.

[JHahnHRO]

But that is against the CDI Spec as I understand it. To use the @ConfigProperties instance from the injection point (even with a proxy in between. That proxy needs to know where to delegate), one needs to access the InjectionPoint bean which is only allowed for @Dependent scoped beans.

[radcortez]

I think the spec is not clear about request-scoped beans (even for @ConfigProperty). What is the intent of a request-scoped bean for @ConfigProperties?

Should it reread all config values? This could lead to inconsistencies and expensive operations on each request to retrieve the Config. If it is just returning a new instance per request but with the same values, there is not much point.

[JHahnHRO]

Scopes other than @Dependent can be used to shorten

@ApplicationScoped
class Server {
   @ConfigProperty(name="server.host") String host;
   @ConfigProperty(name="server.port") int port;
   // ... more properties ...
}

to

@ApplicationScoped
@ConfigProperties(prefix="server")
class Server {
   String host;
   int port;
   // ... more properties ...
}

And that's certainly a valid use case to me. However, maintaining my view point above that the two beans should be indistinguishable unless there are multiple prefixes in play, I expect a @RequestScoped @ConfigProperties bean to re-read all relevant config values for each request that uses such a bean, because the non-shortened version does just that. An @ApplicationScoped @ConfigProperties bean on the other hand would not re-loading the config.

But it makes no sense to use such a bean with multiple prefixes. It's not even possible to implement that (without dropping the @NonBinding from prefix as SmallRye does), I believe. If arbitrary scopes are really desired, then the spec should at least be clear on that limitation and require a DefinitionException to be thrown if a @ConfigProperties bean with any scope other than @Dependent is detected together with injection points with any prefix other than ConfigProperties#UNCONFIGURED_PREFIX

[radcortez]

I agree that @NonBinding must be removed or the CDI spec needs clarification. When we were trying to run SmallRye Config with OpenWebeans, I and @jeanouii realized that the programmatic lookup with @NonBinding did not work (so we came out with that workaround).

We looked in the spec and even the TCK and were not able to find a clarification, so we believe that a programmatic CDI lookup to a @NonBinding dynamic parameter is unspecified in CDI. The CDI TCK has a test for a programmatic binding parameter, but not non-binding.