Skip to content

Instantly share code, notes, and snippets.

@tomas-langer
Created April 11, 2023 15:34
Show Gist options
  • Save tomas-langer/0e3e71c51aec1148cafaa29c451d1b2b to your computer and use it in GitHub Desktop.
Save tomas-langer/0e3e71c51aec1148cafaa29c451d1b2b to your computer and use it in GitHub Desktop.
Config API and implementation choices

Helidon Config

io.helidon.common.config -> "Config API" io.helidon.config -> "Config"

Helidon 1.x -> 3.x There is a single "Config" module, that contains both APIs, SPIs, and implementation

Helidon 4.x, current main For the purpose of minimizing size of dependencies for Pico, and to remove dependencies "into" config module from Pico and common, we have created a "Config API" module

  • there is a full duplication of all of the APIs in "Config" to have backward compatibility as much as possible

Option 0

This approach brings in a "dirty" state, where we duplicate APIs, we want to use the "Config API", but need "Config" in some places in Helidon We could use this with a single backward incompatible change (method asNodeList), not sure how big impact this single method would have on implementation and Helidon in general

Option 1

Revert back to a single "Config" module, that would be used by Pico and all other Helidon modules as we have in 3.x. We should remove dependency on helidon-logging-jul and jakarta-annotation as a runtime dependency of Config This is fully backward compatible with Helidon 3, would introduce the full config as a dependency of Pico (and helidon-common-configurable) This would not allow us to depend on Pico in Config, so if we wanted to create a Pico service for config, we would need to have a new module such as helidon-config-pico

Option 2

Separate API fully from "Config" into "Config API". This would require a change of all usages of Config, to work with the API rather than with the implementation, and to create config instances using the chosen approach (API or runtime). This approach would create a clean dependency tree, where Config runtime could actually use Pico as a dependency. Also module helidon-common-configurable would only depend on API

We would need to decide about naming - current (helidon-common-config; helidon-config) - possible (helidon-config-api; helidon-config-runtime) - this is practically worse than the choice above, as it requires pico and common to have a dependency into config. I would choose this option only if we decide to move all APIs and SPIs to API (including builder etc.)

This option has additional sub-options:

a) Move all APIs and SPIs to Config API b) Move Config.Builder API to Config API - requires a) c) Move only APIs, and leave SPIs as implementation specific (ConfigSource, Parser etc.) - mutually exclusive with b

This has a few impacts on the use of Config:

  1. Usages can be refactored to Config API, as that is the right way to access configuration data
  2. Creating a new config instance a) We can have Config.create() in "Config API" and use service loader to get the default config instance b) We can refactor current io.helidon.config.Config to HelidonConfig and add all the create() methods and builder() methods there e.g. we could have: (config API).Config config = (config API).Config.create(); (config API).Config config = (Config).HelidonConfig.create(myConfigSource); c) We can move all the create and builder methods to "Config API" and use service loader to discover a builder implementation (requires sub-options a, and b)
@trentjeff
Copy link

It should be noted that currently common-config is a sparse set of interfaces/contracts and methods from the config impl module. Option 2 assumes that we will always want a "full" config API definition instead of the minimal definition for what config is at its pure essence (Config and ConfigValue at the core of common). Whether common-config is sparse or full seems orthogonal to this discussion for the API signature, and a "clean" API design.

From the guiding principle perspective, I believe the following:

  1. A module should allow a clean separation between the interface/contracts and the runtime implementation. Just because we don't split them today doesn't mean we will not want to split them tomorrow. If we cannot split them today (because of asNodeList() as in this case) then we should fix it and make a clean API during a major version change.
  2. We should think of compile time vs runtime dependencies. This will be ever so true with Pico, but is also true with config as well. common-config has zero dependencies whereas full config has these dependencies (see below). If someone wants to just compile, or just mock/fake, or provide integration to their own config (as what we should expect with Spectra) then we should not be bringing in any more dependencies transitively than necessary.
[INFO] io.helidon.config:helidon-config:jar:4.0.0-SNAPSHOT
[INFO] +- jakarta.annotation:jakarta.annotation-api:jar:2.1.0:compile
[INFO] +- io.helidon.common:helidon-common-config:jar:4.0.0-SNAPSHOT:compile
[INFO] +- io.helidon.common:helidon-common:jar:4.0.0-SNAPSHOT:compile
[INFO] +- io.helidon.common:helidon-common-media-type:jar:4.0.0-SNAPSHOT:compile
[INFO] +- io.helidon.logging:helidon-logging-jul:jar:4.0.0-SNAPSHOT:compile
[INFO] |  +- io.helidon.common:helidon-common-context:jar:4.0.0-SNAPSHOT:compile
[INFO] |  \- io.helidon.logging:helidon-logging-common:jar:4.0.0-SNAPSHOT:compile
[INFO] +- io.helidon.common.features:helidon-common-features-api:jar:4.0.0-SNAPSHOT:provided (optional) 
[INFO] +- io.helidon.common.features:helidon-common-features-processor:jar:4.0.0-SNAPSHOT:provided (optional) 
  1. Our modules should strive to be backwards compatible as much as possible - even through a major version upgrade.

What we are mainly debating here is the tradeoffs between principle #1 and principle #3: Noting that the current asNodeList method will be 100% backward compatible if we leave it alone, but it will not be "clean".

Now more concretely for this case - here is my opinion/suggestion:

  1. Splitting the config from API and runtime/impl was a good thing, and we should do more of this separation of API from IMPL modules as it reveals itself over time in the Helidon code base. This, therefore, rules out option 1 for me. If the API is not clean because we can't split it into two says something about the quality of the API we have today.
  2. We mark the existing asNodeList as deprecated. This is a nod to option 0 but with a caveat to allow for backward compatibility per our principles.
  3. We introduce an overloaded asNodeList method that is following the right signature for the generics declaration, thereby fixing the API. This ultimately adheres to all of the principles that I shared above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment