Spec of precedence for sources when using profiles seems vague

[poikilotherm]

Description

When activating profiles, the 3.0 spec says this about precedence of sources:

Conforming implementations are required to search for a configuration source with the highest ordinal (priority) that provides either the property name or the “profile-specific” property name. If the configuration source provides the “profile-specific” name, the value of the “profile-specific” property will be returned. If it doesn’t contain the “profile-specific” name, the value of the plain property will be returned.

Let's set up a context before we head to why this might be a non-complete definition.

We have 2 sources activated:

1. microprofile-config.properties, source ordinal = 100:

   my.foobar=test
   %test.my.foobar=notest

2. Environment variable, source ordinal = 300:

   MY_FOOBAR=BBQ

There are two ways to interpret the spec when activating the profile test:

A) Lookup of my.foobar should return notest. The higher ordinal source does not provide a profiled value (which can be set using _TEST_MY_FOOBAR=BBQ2), thus the value from the properties file wins (despite it's lower ordinal). B) Lookup of my.foobar should return BBQ because although the higher ordinal source has no profiled setting, it shall still win because of the higher ordinal.

When testing with SmallRye Config (@radcortez), it seems that variant B is implemented. When testing with Payara (@smillidge), it seems that variant A is implemented.

I tried to reach out to Payara in their forum, but never received an answer. It looked like this aspect was discussed when profiles were first added in the Github Issue.

May I kindly request clarification which way it was intended to be @Emily-Jiang ?

If it helps, I'm happy to create a reproducer to demonstrate the behaviour. Thank you very much y'all!

[radcortez]

Hi @poikilotherm, thanks for the report :)

The feature added to the spec was heavily inspired by SmallRye Config (and Quarkus), so with some bias, your option B should be the intended behavior.

Things are simple when you have a single property without the profile prefix: the highest ordinal source that contains the property wins, and you get that value.

When you mix in a profile property, if it would take precedence over a non-profile property always, regardless of the ordinality of the source, then it would be possible to define such properties with really low ordinal (think, for instance -Integer.MAX) and the only way to override them would be to use the profile property name, which may not be evident for the user. By taking into account the ordinal for profile property names, you keep a consistent behavior with regular properties.

[poikilotherm]

Hi @radcortez, thanks for the reply.

Would you agree that the spec is not well defined enough in this particular aspect? Should there be some TCK acceptance test for this part of the spec to avoid implementations drifting apart?

[poikilotherm]

@Emily-Jiang @emilyjiang my sincerest apologies, GitHub autocomplete offered the wrong user handle and I didn't notice. Fixed it in the description above.

[radcortez]

Hi @radcortez, thanks for the reply.

Would you agree that the spec is not well defined enough in this particular aspect? Should there be some TCK acceptance test for this part of the spec to avoid implementations drifting apart?

Sure. Would you like to contribute some tests? Thanks!

[poikilotherm]

Before I do that, maybe we should hear back from @Emily-Jiang and @smillidge or other folks like @OndroMih about their take on this... :see_no_evil:

[OndroMih]

Hi, it was a long time ago but I remember very well that everybody understood it back then as described in the B option. The idea is that the order of config sources is always preferred, and within the same config source, you can add profile-specific properties to override the normal ones. It's easier to manage and understand how things work in production. Sometimes it might be convenient to override everything with a single config source but that's still possible with a custom config source with a very high priority, although it requires some coding.

I personally think that the wording can be interpreted only in this way (as the option B described) but we can improve it if you think it's not clear enough. And the behavior should definitely be covered by TCK so that implementations that chose a different behavior (e.g. described in option A) would fail the TCK and would have to comply with the intended behavior.

[Emily-Jiang]

Sorry for the late reply as I was traveling last week! IIRC, we discussed the scope when we added the profile support. The profile is defined to overwrite config properties per config source. It should not override the config properties across the board, which should be done while overriding the exactly same config properties. You were right that the spec was not clear. Please go ahead to propose a PR for further improvement.

[poikilotherm]

Thank you @Emily-Jiang, @OndroMih and @radcortez for your insights!

I will go ahead and create a pull request for changing the spec wording and the TCK each. (Please let me know if I should create a combined PR)

[poikilotherm]

@Emily-Jiang I did try to add a TCK test in 99e0734 - do you want me to include it in this PR or shall I create a separate PR to keep reviews apart?

[Emily-Jiang]

@Emily-Jiang I did try to add a TCK test in 99e0734 - do you want me to include it in this PR or shall I create a separate PR to keep reviews apart?

One PR will be good.