Reproduce issue with @WithUnnamedKey config group considered present even if unconfigured

[yrodiere]

Creating for discussion following https://quarkusio.zulipchat.com/#narrow/channel/187038-dev/topic/.40WithUnnamedKey.20and.20keySet/with/603369236

Running the tests and reading the "ISSUE" comments is probably is the easiest way to grasp what is going on.

Test results:

[radcortez]

This behavior is expected with the current implementation (not saying it's right or wrong). Consider:

@ConfigMapping(prefix = "mapping")
interfaceMapping {

  Group group();

  @WithDefaults
  Map<String, Group> map();

  interface Group {
    @WithDefault("value")
    String value();
  }

What the implementation does is to generate properties with the values and add them to our DefaultValuesConfigSource as:

mapping.group.value=value
mapping.*.value=value

The whole idea was to also be able to use the programmatic API to query for the name, so that the values you get from the mapping and the programmatic API are consistent.

When you add @WithUnnamedKey, this also generates a mapping.value=value, which gets picked by the Map, and returns the unnamed key in the Map keys. This pretty much works like the old pattern we used, when we had the default as a separate group and the map for the named keys. We never had a way to distinguish whether a group is only coming from defaults or had user input (doesn't matter if it is a group in a Map, a standalone group, or an optional group.

Should the unnamed key appear in the Map keys if it's only defaults?

Because we never required that distinction, everything we populate comes from sources, including defaults, so for the engine, it is just as if someone set mapping.value=value, and now you get the unnamed key. The default for all keys simply matches the mapping.* pattern and returns a single element on every Map#get. Of course, there is no way to include the keys in the Map keys, so this may seem inconsistent with how unnamed is handled.

We may require a concept that doesn't exist today: distinguishing defaults-only groups from user-configured ones. I'm looking into this from a standalone group perspective, because that was the motivation for us to implement @WithDefaults and @WithUnnnamedKey and to store everything in the Map to simplify the mapping pattern. While on a Map, you could express that using keys (or the absence of keys), how would we express it in a standalone group?

[yrodiere]

The whole idea was to also be able to use the programmatic API to query for the name, so that the values you get from the mapping and the programmatic API are consistent.

Makes sense, but as discussed on Zulip, you only get consistency for the unnamed key -- for other "named" keys, if they are absent from the user's configuration, you won't get any defaults through the programmatic API.

That being said, in my case I truly don't need programmatic retrieval to be consistent in any way when it comes to unnamed key -- in fact, if I currently relied on it, we'd have bugs because we don't get consistency for other "named" keys.

Should the unnamed key appear in the Map keys if it's only defaults?

IMO, no. If it's not configured, it should not appear in the map keys.

Of course, there is no way to include the keys in the Map keys, so this may seem inconsistent with how unnamed is handled.

Right, exactly.

While on a Map, you could express that using keys (or the absence of keys), how would we express it in a standalone group?

So, first, I want to stress I truly don't need this. I just need the map keys to behave 😁

But, if you want a hollistic approach and not a surgical fix... Optional<MyGroup> seems like an obvious answer to me, which would do the job. Or, if you want to allow defaults, some custom interface, e.g. Defaultable<MyGroup> with defaultable.get() returning the value or the defaults, and defaultable.isPresent()/isEmtpy() allowing to query it. Maybe defaultable.toOptional(), though that can easily be a default method.

And whatever the solution, IMO it's also fine to not have defaults available for that optional group when retrieving values programmatically. It's a very reasonsable limitation tied to a very specific feature.

[radcortez]

The whole idea was to also be able to use the programmatic API to query for the name, so that the values you get from the mapping and the programmatic API are consistent.

Makes sense, but as discussed on Zulip, you only get consistency for the unnamed key -- for other "named" keys, if they are absent from the user's configuration, you won't get any defaults through the programmatic API.

That being said, in my case I truly don't need programmatic retrieval to be consistent in any way when it comes to unnamed key -- in fact, if I currently relied on it, we'd have bugs because we don't get consistency for other "named" keys.

Actually, this does work for the programmatic API :)

So, first, I want to stress I truly don't need this. I just need the map keys to behave 😁

Ok, let me see what I can do.

[radcortez]

Check if this works for you.

The proposal is just to compare the unnamed group with a default group and omit the key in that case (all mappings include equals). Now, if the user explicitly sets a key, but it is a default, the unnamed key won't be included. Is this acceptable?

[yrodiere]

That looks perfect! Thank you.

[yrodiere]

Er... maybe I talked too fast. So now, if the user configured the unnamed key by setting a property to its default value, we will omit it from the map?

[radcortez]

Er... maybe I talked too fast. So now, if the user configured the unnamed key by setting a property to its default value, we will omit it from the map?

Yes. But does that matter?

[yrodiere]

Er... maybe I talked too fast. So now, if the user configured the unnamed key by setting a property to its default value, we will omit it from the map?

Yes. But does that matter?

Kind of, yes. The idea is to use configuration to extract a list of datasources (or persistence units, or...) that the user explicitly configured, because we know those need to be created for sure (unless they're explicitly disabled).

Before this patch, I was in trouble because the map could contain <default> even though the user didn't configure the default datasource.
After this patch, I'm still in trouble because the map might not contain <default> even though the user set, say, quarkus.datasource.jdbc=true (and only that).

In practice this is just one datapoint, and I'll try to also inspect injection points and other things. But still, it does mean we'll have a hole in our detection code...

[radcortez]

I did it this way because it was an easy fix that didn't require messing with the names or the defaults, which I wanted to avoid.

Let me think about this again.

[radcortez]

Ok, give it another try.

[yrodiere]

Thank you!