ConfigValue API

[radcortez]

Here is the initial proposal to support the ConfigValue API in MP Config.

Most likely, we need more detail in the spec, but I didn't want to be really extensive, since all of this is subject to change during revision.

Fixes issues:
#43
#312

[eclipse-microprofile-bot]

Can one of the admins verify this patch?

[Emily-Jiang]

From this PR, I don't see why the interceptor and interceptor context should be in the spec. It is implementation details.

[radcortez]

You mean the ConfigSourceInterceptor? What is your proposal then?

[radcortez]

Per our discussions in the meeting on 16/04, I've done the following changes:

• Removed the Interceptor API to focus only on the ConfigValue API (Interceptor API will go into a separate PR)
• Deprecated ConfigSource in favour of ConfigValueSource

If we are happy with this, I'll proceed to complete the Spec and enhance the TCK.

[radcortez]

Depends on #553.

[jbee]

Emily made most of the points I was about to make on the detail level.

On the strategic evolution of the API I generally see the need for a more capable and convenient API.

Use cases I encounter as "unsolved":

• null-safe and exception-less resolution of properties with fluent API and default values (convenience)
• access to conversion capabilities that are otherwise not accessible (convenience and added capability)
• conversion to lists and sets of elements (convenience and more clearly standardised)

Use cases I see solved by the PR:

• provide more information on the source of a property values for logging, debugging or presentation purposes

Given the above I can see how this PR can be the basis of an evolution towards the use cases I encounter as "unsolved" but it does not solve any of them yet. Thinking this forward I find the proposal made in this PR somewhat problematic. If sources already operate on the level of property value objects this moves conversion into sources or forces conversion to be bolted on through some form of wrapping or callback which I would find quite unsatisfactory as it either exposes "internals" in the public API or requires to extend the source implementation beyond the public API to implement the collaboration between the Config implementation and its sources. A smaller issue I see is the lack of a type parameter for the ConfigValue capturing the result type the user wants to have. While this could be added later on it should be in the same release in that case.

My suggestion is to keep ConfigSource as is returning String and to extend the interface with an additional method to retrieve additional source information (as far as I can see in this PR this would only affect the line number). Given such an additional method elevating the String returned by a source to a ConfigValue can be done by the Config. This keeps the conversion and generally control outside of sources and solely within the Config implementation. The ConfigValue should only be defined in terms of its capabilities (interface), not its implementation limiting ways to implement features like conversion and default fallbacks.

[dmlloyd]

Emily made most of the points I was about to make on the detail level.

On the strategic evolution of the API I generally see the need for a more capable and convenient API.

Use cases I encounter as "unsolved":

• null-safe and exception-less resolution of properties with fluent API and default values (convenience)
• access to conversion capabilities that are otherwise not accessible (convenience and added capability)
• conversion to lists and sets of elements (convenience and more clearly standardised)

None of these are use cases though the last directly implies a use case which is addressed by a different issue.

Null-safe resolution already should be a reality. What do you see as missing here? What do you mean by "exception-less"?

What conversion capabilities are you alluding to? What is the use case (i.e. what are you trying to do, as opposed to how do you imagine it being done)?

Use cases I see solved by the PR:

• provide more information on the source of a property values for logging, debugging or presentation purposes

Given the above I can see how this PR can be the basis of an evolution towards the use cases I encounter as "unsolved" but it does not solve any of them yet. Thinking this forward I find the proposal made in this PR somewhat problematic. If sources already operate on the level of property value objects this moves conversion into sources or forces conversion to be bolted on through some form of wrapping or callback which I would find quite unsatisfactory as it either exposes "internals" in the public API or requires to extend the source implementation beyond the public API to implement the collaboration between the Config implementation and its sources. A smaller issue I see is the lack of a type parameter for the ConfigValue capturing the result type the user wants to have. While this could be added later on it should be in the same release in that case.

Don't worry, we're not in a rush to release. We have consciously tried to make this PR be incremental for the purpose of segmenting debate and preventing a situation where conversation is confused.

My suggestion is to keep ConfigSource as is returning String and to extend the interface with an additional method to retrieve additional source information (as far as I can see in this PR this would only affect the line number). Given such an additional method elevating the String returned by a source to a ConfigValue can be done by the Config. This keeps the conversion and generally control outside of sources and solely within the Config implementation. The ConfigValue should only be defined in terms of its capabilities (interface), not its implementation limiting ways to implement features like conversion and default fallbacks.

We discussed this already. The main reason ConfigValue is being added is to support multiple other enhancements to the API to meet a complex, interleaving set of use cases. It exists for SPIs just as mach as for APIs, if not more so. The SPIs are still being introduced. It's a chicken and egg problem: you can't bring in the more complex changes without their predecessors, and the predecessors cannot support the more complex changes if you design and integrate them in a short-term way.

Note that conversion is not (yet) part of the ConfigValue story.

[Emily-Jiang]

My suggestion is to keep ConfigSource as is returning String and to extend the interface with an additional method to retrieve additional source information (as far as I can see in this PR this would only affect the line number). Given such an additional method elevating the String returned by a source to a ConfigValue can be done by the Config. This keeps the conversion and generally control outside of sources and solely within the Config implementation. The ConfigValue should only be defined in terms of its capabilities (interface), not its implementation limiting ways to implement features like conversion and default fallbacks.

I was in the last week's discussion. After I reviewed the updated PR and had a further thought on this, I changed my mind and agreed with @jbee before I even read this above comment. My main concern is that it creates a lot of confusions with two classes live next to each other. Besides, it is ok to add methods to ConfigSource and update as we wish. We can just increate the major version for this package.

[Emily-Jiang]

@jbee when @radcortez adds another method withDefault() on ConfigValue, your use case of setting default will be addressed.

[dmlloyd]

I think it is probably better to break compatibility and make ConfigSource use ConfigValue in that case.

[jbee]

adds another method withDefault() on ConfigValue, your use case of setting default will be addressed.

@Emily-Jiang only partly. Default in combination with conversion is the use case. The one arising from @ConfigProperty requires to be able to set the default as String but then resolving the value as converted value. This can be e.g. Integer (use Converter<Integer>) or Integer[] (split string first and use use Converter<Integer> on each element). So this is much closer to the https://github.com/struberg/microprofile-config/blob/ConfigAccessor/api/src/main/java/org/eclipse/microprofile/config/ConfigAccessor.java @struberg sketched. With converters exposed this could be done by the user of the API but it is very inconvenient to let the user go through this each time.

[jbee]

My main concern is that it creates a lot of confusions with two classes live next to each other. Besides, it is ok to add methods to ConfigSource and update as we wish

Agree, adding methods to ConfigSource would be a much smaller change with better long term backwards compatibility and an API that is easier to understand as responsibilities are more clear.

With conversion added to capabilities of ConfigValue it would further indicate a mismatches with making the creation of ConfigValue a responsibility of the sources. Conversion isn't their responsibility. If it remains in Config's responsibility but sources create the ConfigValues there are two places which have to be involved which I find a path to a confusing solution that has to jump through some hoops to work.

The issue I have with ConfigValue being a class, especially when originating from sources is that all data has to be filled all the time needed or not. Making it an interface originating from Config implementation the implementations has some options on how to implement it. It can defer lookup of line number data to the point the user requests this from the ConfigValue. It also narrows the ConfigValue API surface as no builder pattern is needed. The API would only offer methods to work with the value and receive information on the property all of which can be done eager or lazy. Making this a problem of the implementation also means the implementation can chose the solution that works best with other aspects used by the implementation like caching of values.

[radcortez]

Agree, adding methods to ConfigSource would be a much smaller change with better long term backwards compatibility and an API that is easier to understand as responsibilities are more clear.

I understand. On the other hand, we want implementors to implement the ConfigValue style methods, so if we add them as default methods, no one is going to implement them (or very few). If we added them as methods to be implemented we are breaking compatibility anyway, so that is why we added a separate interface.

With conversion added to capabilities of ConfigValue it would further indicate a mismatches with making the creation of ConfigValue a responsibility of the sources. Conversion isn't their responsibility. If it remains in Config's responsibility but sources create the ConfigValues there are two places which have to be involved which I find a path to a confusing solution that has to jump through some hoops to work.

I do think that ConfigSource and ConfigValue shouldn't have to deal with any conversion details. I look into ConfigSource has a raw data source and ConfigValue as metadata to my raw data. I would prefer that conversions is looked as a separate step, especially considering that we discussed a few times to separate Conversion and make a separate spec out of it, since other projects also require it.

The issue I have with ConfigValue being a class, especially when originating from sources is that all data has to be filled all the time needed or not. Making it an interface originating from Config implementation the implementations has some options on how to implement it. It can defer lookup of line number data to the point the user requests this from the ConfigValue. It also narrows the ConfigValue API surface as no builder pattern is needed. The API would only offer methods to work with the value and receive information on the property all of which can be done eager or lazy. Making this a problem of the implementation also means the implementation can chose the solution that works best with other aspects used by the implementation like caching of values.

I'm ok to change ConfigValue to an interface, but we do need to agree on the API.

[jbee]

I do think that ConfigSource and ConfigValue shouldn't have to deal with any conversion details.

Generally I support the thought of this being concepts that are dealing with another domain, the domain of configuration, and thus should not be involved with conversion. Without conversion though I'd also think the API is unreasonable inconvenient to use. While discussing conversion is a topic on its own I find it relevant here to the extend that it has to fit in with where values are coming from and what that implies for what you can do with them and what you can't or at least what gets tricky or dirty to implement. In that sense I feel the topic of conversion is relevant here.

... make a separate spec out of it, since other projects also require it.

In general this seems a reasonable thing to do. But given the very limited nature of the conversion MP config offers I'd think it makes for a rather poor general conversion standard. In context of configuration I can see how its limitations are a reasonable way but to lift this to a conversion standard would mean it has to become more capable at which point I'd wonder why such a standard would start from the conversion done in MP config as the hard parts are everything beyond the basic capabilities MP config has.

[radcortez]

In general this seems a reasonable thing to do. But given the very limited nature of the conversion MP config offers I'd think it makes for a rather poor general conversion standard. In context of configuration I can see how its limitations are a reasonable way but to lift this to a conversion standard would mean it has to become more capable at which point I'd wonder why such a standard would start from the conversion done in MP config as the hard parts are everything beyond the basic capabilities MP config has.

Yes, a Conversion spec must offer way more and the one in Config is very limited. I don't say that it needs to start with Config, but we definitely need something. Other projects in MP like JWT also require conversions, so it is a waste that this problem is being tackled individually in each spec instead of globally. Then you end up with Conversion API's for JPA, JAXB, JsonB, Config, JWT, etc. Probably being too optimistic thinking that we can solve this issue :)

Anyway, ideally, we should design our API's in a way that if we move forward with it, we are able to replace current Conversion mechanism with another one.

Or we can just forget about it and do what we think is better for Config in particular.

[Emily-Jiang]

I believe we only need a few TCK, right? Is there anything out of this that we want to add into the spec?

Both spec and TCKs need to be added.

[radcortez]

Ok, but what do we want to add in the spec? This doesn't really change anything spec related. just a couple of new API's.

[Emily-Jiang]

@radcortez This section needs to be updated to demonstrate the ConfigValue usage.
By the way, I think we should consider the injection of ConfigValue as well.
@Inject @ConfigProperty(name="client.name") ConfigValue client;

Thoughts?

[radcortez]

Never thought about injection of ConfigValue. I don't see any reasons to not do it, but lets wait for some more feedback.

[Emily-Jiang]

@radcortez as promised, I have another review and logged some minor comments. Overall, it looks good.

[radcortez]

Thank you. I think I've addressed all of your comments.

[jbee]

Thanks @radcortez for keeping this going. Just the wrong year to correct and this should be good to go.

[radcortez]

Just rebased and squashed to make it more clean.

[Emily-Jiang]

LGTM! Thanks @radcortez !

[Emily-Jiang]

@OndroMih @struberg please re-review asap and I'd like this PR merged this week if possible.

[Emily-Jiang]

@radcortez I suddenly realised the TCK should also ensure the winning property config source name and value etc are populated. Can you add another config source with a lower priority for the same config property to test this contract?

[radcortez]

They are tested here:
https://github.com/radcortez/microprofile-config/blob/535aca922394b69599726db0db8e85fc21347cea/tck/src/main/java/org/eclipse/microprofile/config/tck/ConfigValueTest.java#L86-L89

I can another source anyway.

[Emily-Jiang]

Thank you @radcortez! LGTM!

[Emily-Jiang]

@radcortez feel free to merge this PR!

[Emily-Jiang]

I believe @radcortez has address the feedback from @OndroMih and @struberg ! This PR has been open for a while without further feedback. I'm merging this PR because of @radcortez has not got permission to merge the PR as yet.

[OndroMih]

Hi, @Emily-Jiang, @radcortez, I didn’t have time to review before you merged this but for the record I approve now. I had a look and it really reflects all my comments, thank you!