8.2 KiB
Helidon Config API design changes proposal
Summary
Summary table:
| Area | Impact on API | Impact on behavior | Impact on SPI | Urgency |
|---|---|---|---|---|
| Too many methods | High | Low or None | Low | High |
| Source types | Low or None | High | Medium | Medium |
| Change support | Medium | Medium | High | High |
| Polling strategies | Medium | Low | Medium | Low |
| Debugging | None | None | None | Medium |
| FileDetector SPI | None | None | None | High |
| Java Beans | None | Low | None | High |
| No reflection as() | None | High | None | High |
| Remove ConfigMapper | Medium | None | Medium | High |
| Source is Supplier | Compatible | None | Low or None | Medium |
Too Many Methods
Too many methods are part of public API - this makes it very complicated to test, maintain and (sometimes) use
This is caused by:
- each primitive type has its own method + methods for any type and methods for map, list and nodes
- boolean
- int
- long
- Double
- String
- Map
- List
- Config
- Class
- supporting too many paradigms
- returning T, Optional, Supplier, Supplier<Optional>
- for each of these methods (except for Optional) supporting a method without and with a default value
- this means that we have 6 methods for each type (using boolean as an example):
- asBoolean()
- asBoolean(boolean default)
- asBooleanSupplier()
- asBooleanSupplier(boolean default)
- asOptionalBoolean()
- asOptionalBooleanSupplier()
Proposal
Create a typed config value ConfigValue<T>.
This would leave config with the following accessor methods:
- Required:
ConfigValue<T> as(Class<? extends T> type) throws ConfigMappingExceptionConfigValue<List<T>> asList(Class<? extends T> type) throws ConfigMappingExceptionConfigValue<Map<String, String>> asMap()
- Optional (shortcut):
2.
ConfigValue<Config> asNode()3.ConfigValue<List<Config>> asNodeList()4.ConfigValue<Boolean> asBoolean() throws ConfigMappingException5. other shortcut methods for primitive types
The ConfigValue interface would have the following methods to access typed value (as supported in original API):
Optional<T> asOptional()- to get the "real" Optional valueSupplier<T> asSupplier()- supplier of current value (if config changes)Supplier<T> asSupplier(T defaultValue)- supplier of current value with defaultSupplier<Optional<T>> asOptionalSupplier()- supplier of current value as an optionalT get() throws MissingValueException- same as in java.util.Optional, just throws a different exception- and all methods of java.util.Optional (unfortunatelly optional is a final class, so we have no choice but to copy the methods) - including the methods from java9+ (stream(), ifPresentOrElse(), or())
Example:
// unchanged for String, as Config implements Value<String>
config.get("client-id").value().ifPresent(this::clientId);
// current for primitive type
config.get("proxy-port").asOptionalInt().ifPresent(this::proxyPort);
// new for primitive type
config.get("proxy-port").as(Integer.class).ifPresent(this::proxyPort);
// current for type with a mapper
config.get("identity-uri").asOptional(URI.class).ifPresent(this::identityUri);
// new for type with a mapper
config.get("identity-uri").as(URI.class).ifPresent(this::identityUri);
// current for type with a factory method
config.get("oidc-config").asOptional(OidcConfig.class).ifPresent(this::oidcConfig);
// new for type with a factory method (part of "No reflection as()" problem
config.get("oidc-config").as(OidcConfig::create).ifPresent(this::oidcConfig);
// current using response value
int port = config.get("proxy-port").asInt(7001);
// new using response value (if we decide to have shortcuts for primitives)
int port = config.get("proxy-prot").asInt().get(7001);
// new otherwise
int port = config.get("proxy-port").as(Integer.class).get(7001);
Source types
Lazy config sources
We do not support config sources that require lazy access to values (using term "lazy sources" in the text)
This may be sources that cannot list the keys, or where listing the keys is not feasible (speed, memory consumption etc.)
To support such source types, we need to change approach to our configuration tree:
- Each node created from known keys has to keep reference to all sources
- Whenever a node value is requested, lazy sources must be queried for value (according to priority)
- If a value is provided, it is cached forever
Other changes:
- Notifications for changes can be provided only for cached keys
- Config Source SPI will have to be updated, so a source can mark itself as lazy
- Methods that traverse the tree (asMap, traverse, nodeList etc.) would only return
the known key
- We must refactor our own usages of config to use direct key access wherever possible
- We should look into integrations to see if we can support lazy loading of configuration properties
- Jersey
- Weld
- Behavior must be clearly documented
Mutable sources with no notification support
Some of our config source are mutable, yet do not support notifications. We should change all config sources to support notifications if so chosen by the user. If need be, these should be polled regularly and compared with previous version.
Currently known sources that should be refactored:
- System properties
Change support
Current change support is too complex and uses APIs not suitable for the purpose:
- Flow API should not be used in Config API
- method "onChange" should be "onChange(Consumer)" - current expects a function with undefined behavior for returned boolean
- Remove dependency on SubmissionPublisher (and on project Reactor transitively)
Polling strategies
- Check if these can be simplified, as current API and SPI is not easy to use.
- Make sure one thing can be achieved only one way - e.g why do we have polling and watching both available for File config sources?
Debugging
Provide better support for debugging:
- Keep information about a config source that provided a value of a node
- This may be accessible only in a debugger (e.g. no need to change API or SPI)
File Detector SPI
Currently the FileDetector service does not work consistently in all environments. Known problems:
- when running JDK9+ using maven exec plugin (test with yaml config)
- when running in some docker images (need to find the failing image)
Java Beans
Separate java beans support into a different module (including annotations @Value and @Transient). The current support can build instances from config using reflection. This is complicated part of the code that should not be part of SE Config by default. Add SPI to allow for such (more complex) config.as(AClass.class) transformation
No reflection as()
Do not use reflection in T Config.as(Class type) method. Currently there is a complicated
code that introspects the class to find suitable constructor or factory method.
Create a new method ConfigValue<T> as(Function<Config, T> factoryMethod).
We can then use:
config.as(OidcConfig::create);
config.as(SomeClass::new);
Remove ConfigMapper
Remove ConfigMapper interface, as it is in fact a Function<Config, T>.
Source is Supplier
Currently the ConfigSource interface extends Supplier and default implementation
of the get() method returns this.
Reason behind this (probably) is to have a single set of methods on Builder, that
only accept Supplier<ConfigSource> and you can send in either a Builder<? extends ConfigSource
or an actual instance of a ConfigSource.
This is confusing and it is hard to clearly understand the behavior of such methods.
We should introduce builder methods for ConfigSource instances and remove the Supplier from
ConfigSource interface.