Fix merging of value nodes in config 1.x (#1490)

* Fix merging of value nodes in config

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2018 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 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.
@@ -122,9 +122,9 @@ private MergeableNode mergeWithObjectNode(ObjectNodeImpl node) {
         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);
+            builder.value(node.get());
+        } else if (this.hasValue()) {
+            builder.value(get());
         }
 
         return builder.build();

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2018 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 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.
@@ -16,6 +16,7 @@
 
 package io.helidon.config.internal;
 
+import java.util.HashMap;
 import java.util.Objects;
 
 import io.helidon.config.spi.ConfigNode.ValueNode;
@@ -70,16 +71,42 @@ static ValueNodeImpl wrap(ValueNode valueNode) {
     public MergeableNode merge(MergeableNode node) {
         switch (node.nodeType()) {
         case OBJECT:
-            return node.merge(this);
+            return mergeWithObjectNode((ObjectNodeImpl) node);
         case LIST:
-            return node.merge(this);
+            return mergeWithListNode((ListNodeImpl) node);
         case VALUE:
             return node;
         default:
             throw new IllegalArgumentException("Unsupported node type: " + node.getClass().getName());
         }
     }
 
+    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(new HashMap<>());
+
+        node.forEach((name, member) -> builder
+                .deepMerge(AbstractNodeBuilderImpl.MergingKey.of(name), AbstractNodeBuilderImpl.wrap(member)));
+
+        if (node.hasValue()) {
+            builder.value(node.get());
+        } else if (this.hasValue()) {
+            builder.value(get());
+        }
+
+        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<ConfigSource>... sources) {
+        return Config.builder(sources)
+                .disableEnvironmentVariablesSource()
+                .disableSystemPropertiesSource()
+                .build();
+    }
+}