Tree node support. #711

tomas-langer · 2021-08-26T18:43:13Z

This PR shows how tree structure could be supported using MP config.
I have updated the proposal with a bit simplified version.

API changes

Config now has a method get(String name) to obtain a child node of the current node.
It also has several typed methods to get the current node as a typed value

SPI changes

Config source can now provide the full tree instead of a single node
Converter can convert from a node (object node, list node, value node) to a type

This PR was created as a result of discussion on Jakarta Config meeting, so we have a vehicle to discuss tree structure in configuration.

Some decisions that I made and explanation:

there are two new methods (isMutable and changeListener) on ConfigSource. Unfortunately the tree structure is connected with mutability, as if a source provides the full tree, it is no longer queried for each node separately, and so the existing approach of asking each source in sequence (and thus supporting some kind of "maybe mutability") is no longer possible. The new approach is allowing a config source to declare its support for mutations, and an event support to notify configuration of such a change. The configuration may or may not be interested in changes - if not, method changeListener is not called...
instead of creating three interfaces, I have added a method getType() and default methods to the config source that support each type. This is for backward compatibility (and also to make it easy for config source implementation - you just need to know about a single interface). This is a bit more complicated for config implementation, as it needs to check for source type, and then call the correct method to get values

eclipse-microprofile-bot · 2021-08-26T18:48:01Z

Can one of the admins verify this patch?

Emily-Jiang · 2021-08-31T15:05:06Z

api/src/main/java/org/eclipse/microprofile/config/Config.java

+    /*
+     * Shortcut helper methods
+     */
+    default Optional<String> asString() {


This is only callable if the hasValue() returns true. I am wondering whether we layer this further and move some of the methods to LeafNode.

This is practically not true - there can be a converter from config node to string that could work even for list nodes (e.g. converting a list to a comma separated string)

Emily-Jiang · 2021-08-31T22:08:03Z

api/src/main/java/org/eclipse/microprofile/config/Config.java

+        /**
+         * Object node with named members and a possible direct value.
+         */
+        OBJECT,


I like the Type structure, but not the enum value names. I think it is pretty much: Parent, List, Leaf, Empty

Parent is not a very good name, as it can have a value (it is basically a tree node that may contain child nodes and a value).
Leaf is OK. Empty vs. Missing - I think missing is more spot on (as this config node is missing from configuration). Empty may mean something different (e.g. the node exists, but is empty).
The values should still be all upper case (best practice for naming enum values)

Renamed value to LEAF

…t a tree node config source without implementing this method.

Emily-Jiang · 2021-09-23T10:57:49Z

api/src/main/java/org/eclipse/microprofile/config/spi/ConfigSource.java

+     */
+    enum Type {
+        /**
+         * This is for backward compatibility - request each property separately.


Should this be FLAT to state all properties are visible with a flat structure?

The naming could be anything, I just wanted to have an option that is backward compatible. If FLAT sounds better, no problem.

Emily-Jiang · 2021-09-23T11:01:02Z

api/src/main/java/org/eclipse/microprofile/config/spi/Converter.java

@@ -150,4 +152,18 @@
     *             if the given value was {@code null}
     */
    T convert(String value) throws IllegalArgumentException, NullPointerException;
+
+    /**
+     * Convert the given config node (may be an object, list or value) to the specified type.


If this config node is a node instead of leaf, I assume an IllegalArugumentException should be thrown.

A converter may be capable of converting object nodes or list nodes as well. If the converter converts to a POJO for example, accepting an Object node is absolutely fine

Emily-Jiang · 2021-09-23T11:17:06Z

api/src/main/java/org/eclipse/microprofile/config/spi/ConfigSource.java

+     *
+     * @param changeRunnable runnable to call when this config source changes
+     */
+    default void changeListener(Runnable changeRunnable) {


I think onChange is a better name.

Emily-Jiang · 2021-09-23T11:19:40Z

api/src/main/java/org/eclipse/microprofile/config/spi/ConfigSource.java

+     *
+     * @return node content or empty optional if the underlying config source does not exist
+     */
+    default Optional<ConfigNode> load() {


I think we should remove this method if we update the following method load(String key) to state that if the key is null, it will load the whole tree. Otherwise, load the tree with the specified root. I would change the signature load(String root) while null means the whole root.

I am against using null as a valid parameter value (anywhere, anytime).
I can see two options here:

use empty string instead (as the root node does not have a name, so the key could be an empty string)

use a dedicated Key class that would have the concept of a root and use that

Emily-Jiang · 2021-09-23T11:24:03Z

api/src/main/java/org/eclipse/microprofile/config/spi/ConfigNode.java

+        /**
+         * An object (complex structure), optionally may have a value.
+         */
+        OBJECT,


I still find this confusing. LIST and VALUE are all OBJECT. How about STRUCTURE or COMPLEX to differentiate them.

This is paritally taken from JSON, where JsonObject is the specific type (something that contains other nodes) and JsonStructure is the superset.
I am using List instead of Array, as I don't like arrays much in general. But otherwise this copies JSON-P naming

JsonObject -> OBJECT
JsonArray -> LIST (because collections are better than arrays ;) )
JsonValue -> VALUE

I didn't make to the correlation with Json before you mentioned. I think if we prefix with Config, it might be less confusing, but refering to ConfigNode.OBJECT as ConfigNode.ConfigOBJECT is mouthful. It is not ideal, but I have not got a better name as yet:(. Maybe someone else has.

Emily-Jiang · 2021-09-23T11:27:24Z

api/src/main/java/org/eclipse/microprofile/config/Config.java

+    /**
+     * Config node type.
+     */
+    enum Type {


We have NodeType under ConfigNode and this Type again for config node type. Can we list one out and refer to it in both Config and ConfigNode?

Because these are different scopes and different meanings.
One is intended for SPI (ConfigNode.Type) that supports just three types.
The other is for API (Config.Type) that supports also MISSING.
User's do not see ConfigNode unless they implement ConfigSource; SPI does not use Config.
So separation of concerns.

Emily-Jiang · 2021-09-23T11:29:35Z

api/src/main/java/org/eclipse/microprofile/config/Config.java

+     *
+     * @return a detached config instance
+     */
+    Config detach();


I am not sure about the use case here. When do you need to do this? Is this purly about shorter the key string?

this is to create a config that "cuts" the prefix from all of its nodes. Use case is when you need to configure a component that expects a map (such as JSON-B, CDI).
If you would not have such a method, you would need to create a map, and then rebuild the map by doing a substring on each key

JHahnHRO · 2022-11-26T02:24:54Z

api/src/main/java/org/eclipse/microprofile/config/spi/ConfigNode.java

+import java.util.Optional;
+
+/**
+ * Marker interface identifying a config node implementation.


Doesn't "marker interface" usually refer to interfaces without methods?

JHahnHRO · 2022-11-26T02:27:25Z

api/src/main/java/org/eclipse/microprofile/config/spi/ConfigNode.java

+     *
+     * @return NodeType this node represents
+     */
+    NodeType nodeType();


Some time, in the far, far future, the spec will require a minimum of Java 17. Should we prepare for this day by declaring the type enum to be a temporary workaround until this interface can be sealed in a future version?

ederks85 approved these changes Aug 27, 2021

View reviewed changes

Emily-Jiang reviewed Aug 31, 2021

View reviewed changes

Simplified tree support.

234b274

tomas-langer force-pushed the tree-structure branch from c85bb8e to 234b274 Compare September 23, 2021 10:21

tomas-langer added 2 commits September 23, 2021 12:26

getValue must have default implementation as well, so we can implemen…

1543997

…t a tree node config source without implementing this method.

Renamed value type to leaf.

cecbccc

Emily-Jiang reviewed Sep 23, 2021

View reviewed changes

JHahnHRO reviewed Nov 26, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tree node support. #711

Tree node support. #711

tomas-langer commented Aug 26, 2021 •

edited

Loading

eclipse-microprofile-bot commented Aug 26, 2021

Emily-Jiang Aug 31, 2021

tomas-langer Sep 1, 2021

Emily-Jiang Aug 31, 2021

tomas-langer Sep 1, 2021

tomas-langer Sep 23, 2021

Emily-Jiang Sep 23, 2021

tomas-langer Sep 23, 2021

Emily-Jiang Sep 23, 2021

tomas-langer Sep 23, 2021

Emily-Jiang Sep 23, 2021

Emily-Jiang Sep 23, 2021 •

edited

Loading

tomas-langer Sep 23, 2021

Emily-Jiang Sep 23, 2021

tomas-langer Sep 23, 2021

Emily-Jiang Sep 23, 2021

Emily-Jiang Sep 23, 2021

tomas-langer Sep 23, 2021

Emily-Jiang Sep 23, 2021

tomas-langer Sep 23, 2021

JHahnHRO Nov 26, 2022

JHahnHRO Nov 26, 2022

Tree node support. #711

Are you sure you want to change the base?

Tree node support. #711

Conversation

tomas-langer commented Aug 26, 2021 • edited Loading

API changes

SPI changes

eclipse-microprofile-bot commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emily-Jiang Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomas-langer commented Aug 26, 2021 •

edited

Loading

Emily-Jiang Sep 23, 2021 •

edited

Loading