Support a fixed list of Map keys statically @WithKeys

[radcortez]

Add a new annotation @WithKeys to be used with Map in @ConfigMapping.

The purpose is to provide a list of keys to populate the Map instead of relying on the list of property names (which may not include all the defined keys in the configuration).

Inspired by

• Improve compatibility of the REST Client configuration quarkusio/quarkus#42932
• Move REST Client configuration to use @ConfigMapping quarkusio/quarkus#42106
• OpenTelemetry traces and metrics config fallback to base quarkusio/quarkus#42814

[radcortez]

//cc @gsmet @yrodiere would love your feedback on this one

[dmlloyd]

It should only be TYPE_USE IMO. The other cases where we have both are historical (IIRC) where it was wrongly METHOD but we actually intended it to be TYPE_USE. Having both means we have to check for it twice in very different ways, which is sometimes quite painful.

[radcortez]

Sure, this was to make it consistent with the other annotations.

[yrodiere]

//cc @gsmet @yrodiere would love your feedback on this one

So from what I understand this is how it's supposed to be used:

    @ConfigMapping(prefix = "map")
    interface KnownMapKeys {
        @WithKeys({ "one", "two" })
        Map<String, String> values();
    }

I am unsure what the use case is, since it seems to me that removing @WithKeys (and maybe adding @WithDefaults) would get you the same result in practice... ?

    @ConfigMapping(prefix = "map")
    interface KnownMapKeys {
        @WithDefaults
        Map<String, String> values();
    }

Or is it just to mandate the keys one/two to be present? But then it's just static keys, and you could just use objects:

    interface MyStaticMap {
        String one();
        String two();
    }
    @ConfigMapping(prefix = "map")
    interface KnownMapKeys {
        MyStaticMap values();
    }

Anyway, I don't have anything against it in principle, I think I'm just missing the point.

[radcortez]

Anyway, I don't have anything against it in principle, I think I'm just missing the point.

Yes, sorry, I was not very clear in the goal.

One motivation is to provide the static set of keys during build time for runtime. Of course, this cannot be done by a plain array of keys in the annotation, so that is why we have a class provider https://github.com/smallrye/smallrye-config/pull/1220/files#diff-8f82c482208ee092deabd66a10cfc069c895fc0b95a2d8ca92d5b0751e3b5e52R23.

The intent is that this provider can be generated during build time for cases where we can determine the list of keys, but at the same time, the keys are dynamic enough that they cannot be typed statically in the mapping. As an example, check the REST Client configuration:

https://github.com/quarkusio/quarkus/blob/563d9834e7e223e07c35a8ce1557112cc853efa1/extensions/resteasy-classic/rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/RestClientsConfig.java#L26-L36

We need to map the REST Client configuration as a Map because we don't know the REST Client names, but we do know them during the build. In this case we could generate this provided key class, that the config would use to populate the Map, with the following benefits:

• These keys are now required, and we can validate their presence at the config level and not in the extension: https://github.com/quarkusio/quarkus/blob/39d5ba15c056d67968c30a7cc5f050708895a860/extensions/resteasy-classic/resteasy-client/runtime/src/main/java/io/quarkus/restclient/runtime/RestClientBase.java#L305-L320
• We can also report as unused / unknown keys that are not part of the list
• Sources that do not provide a list of property names would work, since we have to rely on the list of names to find the keys

I think this is definetely useful for the REST Client case and also in another similar case with OTel. I was wondering if you would find this useful for datasources or hibernate.

[yrodiere]

Thanks, that's clearer. I agree this makes a lot of sense.

These keys are now required, and we can validate their presence at the config level and not in the extension: https://github.com/quarkusio/quarkus/blob/39d5ba15c056d67968c30a7cc5f050708895a860/extensions/resteasy-classic/resteasy-client/runtime/src/main/java/io/quarkus/restclient/runtime/RestClientBase.java#L305-L320

This example seems a bit weird though... it seems it allows two different keys (class name and client name) to configure the same client. And I think that is the reason for urlReload being Optional... otherwise it seems to me urlReload could have been non-Optional and Smallrye config would have executed the check when mapping the runtime configuration?

I think to solve the problem in the rest client, what you'd need instead (or on top) of this PR would be some concept of "synonym" keys? Then Smallrye Config would be able to merge configs using keys that refer to the same client (and/or to fail on conflicts), and the Rest Client extension would retrieve the config more naturally, without having to check for mandatory properties.

I was wondering if you would find this useful for datasources or hibernate.

For "required checks" I'm not sure, first because I don't see how it helps (see ^), and second because required config is quite rare.

To "report as unused / unknown keys that are not part of the list", definitely -- though I'm not sure this will be a high-priority change in the Hibernate extensions, sorry -- our todo list is long :) It's more of a "nice-to-have".

For "sources that do not provide a list of property names", I've never faced the issue personally, but it makes sense -- it will probably help for Hibernate/datasources as well.

[radcortez]

This example seems a bit weird though... it seems it allows two different keys (class name and client name) to configure the same client. And I think that is the reason for urlReload being Optional... otherwise it seems to me urlReload could have been non-Optional and Smallrye config would have executed the check when mapping the runtime configuration?

Yes, the REST Client config is a bit of a mess. We added the convention where we use the FQN of the class, simple name, or RegisterRestClient#configKey. There are too many combinations that we need to figure out. The reload is a workaround to support ${quarkus.http.port} when the port is random. The issue is that users use that expression in the endpoint URL, but the configuration is evaluated at the start before we get the real port from Vert.x. We need to solve it with quarkusio/quarkus#42986, which will to help with some other issues.

I think to solve the problem in the rest client, what you'd need instead (or on top) of this PR would be some concept of "synonym" keys? Then Smallrye Config would be able to merge configs using keys that refer to the same client (and/or to fail on conflicts), and the Rest Client extension would retrieve the config more naturally, without having to check for mandatory properties.

We already do that with fallbacks

For "required checks" I'm not sure, first because I don't see how it helps (see ^), and second because required config is quite rare.

The Map config is always optional. In the case of the REST Client, we know that if there is a service foo, we require a `foo..url' so that can be validated at the config level and not at the extension level.

Thanks for the feedback. I think we agree that this is useful, so I'll complete the missing pieces.

[yrodiere]

I think to solve the problem in the rest client, what you'd need instead (or on top) of this PR would be some concept of "synonym" keys? Then Smallrye Config would be able to merge configs using keys that refer to the same client (and/or to fail on conflicts), and the Rest Client extension would retrieve the config more naturally, without having to check for mandatory properties.

We already do that with fallbacks

If by "fallbacks" you mean what's implemented in the REST client, well... it's not quite the same, as it's the extension doing all the heavy lifting. And it prevents one from defining mandatory properties.

What I meant is this:

    @ConfigMapping(prefix = "some-config")
    interface SomeConfig {
        @WithDefaults
        @WithFallbackKeys(provider = MyProvider.class)
        Map<String, String> values();
    }

    class MyProvider implements FallbackKeyProvider {
        public List<String> getFallbackKeys(String requestedKey) {
             return switch(requestedKey) {
                   case "myClient1": yield List.of("com.acme.ClientInterface1");
                   case "myClient2": yield List.of("com.acme.ClientInterface2");
             };
        }
    }

Which would lead to this:

SomeConfig config = ...;
config.get("myClient1"); // returns config set with key "myClient1" OR "com.acme.ClientInterface1", merged.

For "required checks" I'm not sure, first because I don't see how it helps (see ^), and second because required config is quite rare.

The Map config is always optional. In the case of the REST Client, we know that if there is a service foo, we require a `foo..url' so that can be validated at the config level and not at the extension level.

The Map itself may be optional, but not the properties within the nested values. From what I've seen, as soon as the extension retrieves a map entry:

1. Either the extension is not using @WithDefaults, and the entry's value may be null (no config at all), and then yes the extension will have to be careful about that. But then why wouldn't it be using @WithDefaults?
2. Or the entry's value will not be null (some config present), and then nested config will get validated, and if Smallrye config detects a missing value it will report it.

The only reason you're not getting validation in the "rest client" case is that urlReload was marked as Optional. And I think that's because configuration for a single client could be spread across multiple different keys. Which this PR won't solve.

I think we agree that this is useful, so I'll complete the missing pieces.

Fine by me, but before going further I'd warmly recommend you write an actual test to check that this solves the Rest Client problem (the "making keys required" problem you mentioned when you linked to https://github.com/quarkusio/quarkus/blob/39d5ba15c056d67968c30a7cc5f050708895a860/extensions/resteasy-classic/resteasy-client/runtime/src/main/java/io/quarkus/restclient/runtime/RestClientBase.java#L305-L320).

Because it seems to me this won't solve it.

Or, possibly, I'm completely misunderstanding what's going on :)

[radcortez]

If by "fallbacks" you mean what's implemented in the REST client, well... it's not quite the same, as it's the extension doing all the heavy lifting. And it prevents one from defining mandatory properties.

No, it is done in config directly:
https://github.com/quarkusio/quarkus/blob/39d5ba15c056d67968c30a7cc5f050708895a860/extensions/resteasy-classic/rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/RestClientNameFallbackInterceptor.java#L31

All the oneOf Optional are not required anymore. I only recently moved to @ConfigMapping in REST Client (so we uncovered some of the issues), and I didn't want to do that change:

• Move REST Client configuration to use @ConfigMapping quarkusio/quarkus#42106
• Improve compatibility of the REST Client configuration quarkusio/quarkus#42932

The only reason you're not getting validation in the "rest client" case is that urlReload was marked as Optional. And I think that's because configuration for a single client could be spread across multiple different keys. Which this PR won't solve.

In practice a Map mapping is always optional, because it does not force you to have keys configured, but when a Map key is available in the configuration, any nested elements will follow the expected rules: optionals are not required, non-optionals are required.

That makes sense when you don't know how to populate the Map (for instance when configuring keystores). In the case of the REST Client, we know the keys, but we don't know them statically to reflect that in the mapping, so it is possible to have a REST Client service, with no configuration whatsoever that config would not complain.

Fine by me, but before going further I'd warmly recommend you write an actual test to check that this solves the Rest Client problem (the "making keys required" problem you mentioned when you linked to

For sure, I wouldn't be doing it in any other way :)

[yrodiere]

All the oneOf Optional are not required anymore. I only recently moved to @ConfigMapping in REST Client (so we uncovered some of the issues), and I didn't want to do that change:

Okay, that... was unexpected. Thanks for explaining.

so it is possible to have a REST Client service, with no configuration whatsoever that config would not complain.

Would it still be possible when using @WithDefaults on the map? Because in that case, the config object for a given rest service would be non-null, and thus would need to be instantiated, and thus would need a value for every non-Optional, non-defaulted property... ?

[radcortez]

Would it still be possible when using @WithDefaults on the map? Because in that case, the config object for a given rest service would be non-null, and thus would need to be instantiated, and thus would need a value for every non-Optional, non-defaulted property... ?

Correct. That is something we can add. When I made the initial move, it was not only a plain root -> mapping migration. The REST Client config had its own implementation to retrieve values, disregarding the root completely (only used for documentation). I've tried to be conservative with the changes, and we still had compatibility issues. Things should be more stable now, and I think we can be more confident with further code changes by getting rid of things we don't need, optimizing the lookups, and adequately setting up the mapping structure.

Thank you for all the feedback. It is very useful :)

[radcortez]

This is how the REST Client changes look like:

quarkusio/quarkus#43345

There are still two gaps not related to this new feature (that I plan to address later):

• Automatic lookups for quoted / unquoted keys. A multiple-segment key always requires quotes, and a single-segment key does not, but some users add the quotes anyway.
• Fallback from the Map#value to a parent configuration.