Fix merging of value nodes in config 2.0 (#1488)

* #1182 issue unit test and fix.
* MP Reference test.

Signed-off-by: Tomas Langer <tomas.langer@oracle.com>

config/config/src/main/java/io/helidon/config/ConfigSourcesRuntime.java

@@ -216,5 +216,10 @@ final class ConfigSourcesRuntime {
         private Optional<ObjectNode> data() {
             return data;
         }
+
+        @Override
+        public String toString() {
+            return runtime.toString();
+        }
     }
 }

config/config/src/main/java/io/helidon/config/ObjectNodeImpl.java

@@ -121,11 +121,7 @@ public class ObjectNodeImpl extends AbstractMap<String, ConfigNode> implements O
         ObjectNodeBuilderImpl builder = ObjectNodeBuilderImpl.create(members, resolveTokenFunction);
         node.forEach((name, member) -> builder.deepMerge(MergingKey.of(name), AbstractNodeBuilderImpl.wrap(member)));
 
-        if (node.hasValue()) {
-            builder.value(node.value);
-        } else if (hasValue()) {
-            builder.value(value);
-        }
+        node.value().or(this::value).ifPresent(builder::value);
 
         return builder.build();
     }

config/config/src/main/java/io/helidon/config/ValueNodeImpl.java

@@ -76,8 +76,9 @@ public class ValueNodeImpl implements ValueNode, MergeableNode {
     public MergeableNode merge(MergeableNode node) {
         switch (node.nodeType()) {
         case OBJECT:
+            return mergeWithObjectNode((ObjectNodeImpl) node);
         case LIST:
-            return node.merge(this);
+            return mergeWithListNode((ListNodeImpl) node);
         case VALUE:
             return node;
         default:
@@ -85,6 +86,28 @@ public class ValueNodeImpl implements ValueNode, MergeableNode {
         }
     }
 
+    private MergeableNode mergeWithListNode(ListNodeImpl node) {
+        if (node.hasValue()) {
+            // will not merge, as the new node has priority over this node and we only have a value
+            return node;
+        }
+
+        // and this will work fine, as the list node does not have a value, so we just add a value from this node
+        return node.merge(this);
+    }
+
+    private MergeableNode mergeWithObjectNode(ObjectNodeImpl node) {
+        // merge this value node with an object node
+        ObjectNodeBuilderImpl builder = ObjectNodeBuilderImpl.create();
+
+        node.forEach((name, member) -> builder
+                .deepMerge(AbstractNodeBuilderImpl.MergingKey.of(name), AbstractNodeBuilderImpl.wrap(member)));
+
+        node.value().or(this::value).ifPresent(builder::value);
+
+        return builder.build();
+    }
+
     @Override
     public String toString() {
         return "\"" + value + "\"";

config/config/src/test/java/io/helidon/config/Gh1182Override.java

@@ -0,0 +1,86 @@
+/*
+ * Copyright (c) 2020 Oracle and/or its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.helidon.config;
+
+import java.util.Map;
+import java.util.function.Supplier;
+
+import io.helidon.config.spi.ConfigSource;
+
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertThat;
+
+/**
+ * Unit test to reproduce (and validate fix) of
+ * <a href="https://github.com/oracle/helidon/issues/1182">Github issue 1182</a>.
+ */
+public class Gh1182Override {
+    private static final Map<String, String> VALUE = Map.of("app1.node1.value", "false");
+    private static final Map<String, String> LIST = Map.of(
+            "app1.node1.value", "true",
+            "app1.node1.value.1", "14",
+            "app1.node1.value.2", "15",
+            "app1.node1.value.3", "16"
+    );
+
+    @Test
+    void testValue() {
+        Config config = buildConfig(ConfigSources.create(VALUE));
+
+        ConfigValue<String> value = config.get("app1.node1.value").asString();
+
+        assertThat(value, is(ConfigValues.simpleValue("false")));
+    }
+
+    @Test
+    void testList() {
+        Config config = buildConfig(ConfigSources.create(LIST));
+
+        ConfigValue<String> value = config.get("app1.node1.value").asString();
+
+        assertThat(value, is(ConfigValues.simpleValue("true")));
+    }
+
+    @Test
+    void testOverrideValueOverList() {
+        Config config = buildConfig(ConfigSources.create(VALUE),
+                                    ConfigSources.create(LIST));
+
+        ConfigValue<String> value = config.get("app1.node1.value").asString();
+
+        assertThat(value, is(ConfigValues.simpleValue("false")));
+    }
+
+    @Test
+    void testOverrideListOverValue() {
+        Config config = buildConfig(ConfigSources.create(LIST),
+                                    ConfigSources.create(VALUE));
+
+        ConfigValue<String> value = config.get("app1.node1.value").asString();
+
+        assertThat(value, is(ConfigValues.simpleValue("true")));
+    }
+
+    private Config buildConfig(Supplier<? extends ConfigSource>... sources) {
+        return Config.builder(sources)
+                .disableEnvironmentVariablesSource()
+                .disableSystemPropertiesSource()
+                .build();
+    }
+}

config/tests/pom.xml

@@ -58,5 +58,6 @@
         <module>test-mappers-2-complex</module>
         <module>test-meta-source</module>
         <module>test-parsers-1-complex</module>
+        <module>test-mp-reference</module>
     </modules>
 </project>

config/tests/test-mp-reference/pom.xml

@@ -0,0 +1,57 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  ~ Copyright (c) 2020 Oracle and/or its affiliates.
+  ~
+  ~ Licensed under the Apache License, Version 2.0 (the "License");
+  ~ you may not use this file except in compliance with the License.
+  ~ You may obtain a copy of the License at
+  ~
+  ~     http://www.apache.org/licenses/LICENSE-2.0
+  ~
+  ~ Unless required by applicable law or agreed to in writing, software
+  ~ distributed under the License is distributed on an "AS IS" BASIS,
+  ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  ~ See the License for the specific language governing permissions and
+  ~ limitations under the License.
+  -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <modelVersion>4.0.0</modelVersion>
+    <parent>
+        <groupId>io.helidon.config.tests</groupId>
+        <artifactId>helidon-config-tests-project</artifactId>
+        <version>2.0.0-SNAPSHOT</version>
+    </parent>
+
+    <artifactId>helidon-config-test-mp-reference</artifactId>
+    <name>Helidon Config Tests MP Reference</name>
+
+    <description>
+        Integration tests of reference in MP
+    </description>
+
+    <dependencies>
+        <dependency>
+            <groupId>org.eclipse.microprofile.config</groupId>
+            <artifactId>microprofile-config-api</artifactId>
+        </dependency>
+        <dependency>
+            <groupId>io.helidon.config</groupId>
+            <artifactId>helidon-config</artifactId>
+            <scope>runtime</scope>
+        </dependency>
+
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-api</artifactId>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.hamcrest</groupId>
+            <artifactId>hamcrest-all</artifactId>
+            <scope>test</scope>
+        </dependency>
+    </dependencies>
+</project>

config/tests/test-mp-reference/src/main/resources/META-INF/microprofile-config.properties

@@ -0,0 +1,29 @@
+#
+# Copyright (c) 2020 Oracle and/or its affiliates.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+value1=value
+
+referencing1-1=${value1}
+referencing1-2=${value1}-ref
+referencing1-3=ref-${value1}
+referencing1-4=ref-${value1}-ref
+
+referencing2-1=${value2}
+referencing2-2=${value2}-ref
+referencing2-3=ref-${value2}
+referencing2-4=ref-${value2}-ref
+
+referencing3-1=${value1}-${value2}

config/tests/test-mp-reference/src/test/java/io/helidon/config/tests/mpref/MpConfigReferenceTest.java

@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2020 Oracle and/or its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.helidon.config.tests.mpref;
+
+import org.eclipse.microprofile.config.Config;
+import org.eclipse.microprofile.config.ConfigProvider;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.notNullValue;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class MpConfigReferenceTest {
+    private static final String VALUE_1 = "value";
+    private static final String VALUE_2 = "hodnota";
+    
+    private static Config config;
+
+    @BeforeAll
+    static void initClass() {
+        System.setProperty("value2", VALUE_2);
+
+        config = ConfigProvider.getConfig();
+    }
+
+    @Test
+    void testValue1() {
+        test("1", VALUE_1);
+    }
+
+    @Test
+    void testValue2() {
+        test("2", VALUE_2);
+    }
+
+    @Test
+    void testBoth() {
+        test("3", "1", VALUE_1 + "-" + VALUE_2);
+    }
+
+    private void test(String prefix, String value) {
+        test(prefix, "1", value);
+        test(prefix, "2", value + "-ref");
+        test(prefix, "3", "ref-" + value);
+        test(prefix, "4", "ref-" + value + "-ref");
+    }
+
+    private void test(String prefix, String suffix, String value) {
+        String key = "referencing" + prefix + "-" + suffix;
+        String configured = config.getValue(key, String.class);
+
+        assertThat("Value for key " + key, configured, notNullValue());
+        assertThat("Value for key " + key, configured, is(value));
+    }
+}