[PAXEXAM-929] Fix Karaf configuration option issues

containers/pax-exam-container-karaf/src/main/java/org/ops4j/pax/exam/karaf/container/internal/KarafCfgFile.java

@@ -44,6 +44,7 @@ public void load() throws IOException {
             return;
         }
         final FileInputStream fis = new FileInputStream(file);
+        properties.clear();
         properties.load(fis);
         fis.close();
     }

containers/pax-exam-container-karaf/src/main/java/org/ops4j/pax/exam/karaf/container/internal/KarafConfigurationFile.java

@@ -42,6 +42,7 @@ public boolean exists() {
     public void replace(final File source) {
         try {
             FileUtils.copyFile(source, file);
+            load();
         }
         catch (IOException e) {
             throw new IllegalStateException("Error occured while replacing file " + file, e);

containers/pax-exam-container-karaf/src/main/java/org/ops4j/pax/exam/karaf/container/internal/KarafTestContainer.java

@@ -43,6 +43,7 @@
 import java.util.HashMap;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Map;
 import java.util.Properties;
 import java.util.Queue;
 import java.util.Set;
@@ -363,79 +364,57 @@ private void appendVmSettingsFromSystem(ArrayList<String> opts, ExamSystem subsy
         }
     }
 
-    private void updateUserSetProperties(File karafHome,
-        List<KarafDistributionConfigurationFileOption> options) throws IOException {
-        HashMap<String, HashMap<String, List<KarafDistributionConfigurationFileOption>>> optionMap = new HashMap<>();
-        for (KarafDistributionConfigurationFileOption option : options) {
-            if (!optionMap.containsKey(option.getConfigurationFilePath())) {
-                optionMap.put(option.getConfigurationFilePath(),
-                    new HashMap<String, List<KarafDistributionConfigurationFileOption>>());
+    private KarafConfigurationFile getKarafConfigurationFile(File karafHome, String configFile) {
+        KarafConfigurationFile karafConfigurationFile = KarafConfigurationFileFactory.create(karafHome, configFile);
+        if (!karafConfigurationFile.exists()) {
+            // some property options will come from Pax-Exam and use the default data/etc locations,
+            // in those cases when the property file doesn't exist and we have custom data/etc paths
+            // we need to consider the custom location and use that - but only if it matches+exists
+            String karafData = framework.getKarafData();
+            String karafEtc = framework.getKarafEtc();
+            KarafConfigurationFile customConfigurationFile = null;
+            if (configFile.startsWith("data/") && !configFile.startsWith(karafData)) {
+                customConfigurationFile = KarafConfigurationFileFactory.create(karafHome, karafData + configFile.substring(4));
             }
-            HashMap<String, List<KarafDistributionConfigurationFileOption>> optionEntries = optionMap
-                .get(option.getConfigurationFilePath());
-            if (!optionEntries.containsKey(option.getKey())) {
-                optionEntries.put(option.getKey(),
-                    new ArrayList<KarafDistributionConfigurationFileOption>());
+            if (configFile.startsWith("etc/") && !configFile.startsWith(karafEtc)) {
+                customConfigurationFile = KarafConfigurationFileFactory.create(karafHome, karafEtc + configFile.substring(3));
             }
-            else {
-                // if special file warn, replace and continue
-                if (!option.getConfigurationFilePath().equals(FeaturesCfg.FILE_PATH)) {
-                    LOGGER
-                        .warn("you're trying to add an additional value to a config file; you're current "
-                            + "value will be replaced.");
-                    optionEntries.put(option.getKey(),
-                        new ArrayList<KarafDistributionConfigurationFileOption>());
-                }
+            if (customConfigurationFile != null && customConfigurationFile.exists()) {
+                karafConfigurationFile = customConfigurationFile;
             }
-            optionEntries.get(option.getKey()).add(option);
-        }
-        String karafData = framework.getKarafData();
-        String karafEtc = framework.getKarafEtc();
-        Set<String> configFiles = optionMap.keySet();
-        for (String configFile : configFiles) {
-            KarafConfigurationFile karafConfigurationFile = KarafConfigurationFileFactory.create(karafHome, configFile);
-            if (!karafConfigurationFile.exists()) {
-                // some property options will come from Pax-Exam and use the default data/etc locations,
-                // in those cases when the property file doesn't exist and we have custom data/etc paths
-                // we need to consider the custom location and use that - but only if it matches+exists
-                KarafConfigurationFile customConfigurationFile = null;
-                if (configFile.startsWith("data/") && !configFile.startsWith(karafData)) {
-                    customConfigurationFile = KarafConfigurationFileFactory.create(karafHome, karafData + configFile.substring(4));
-                }
-                if (configFile.startsWith("etc/") && !configFile.startsWith(karafEtc)) {
-                    customConfigurationFile = KarafConfigurationFileFactory.create(karafHome, karafEtc + configFile.substring(3));
-                }
-                if (customConfigurationFile != null && customConfigurationFile.exists()) {
-                    karafConfigurationFile = customConfigurationFile;
+        }
+        return karafConfigurationFile;
+    }
+
+    private void updateUserSetProperties(File karafHome,
+        List<KarafDistributionConfigurationFileOption> options) throws IOException {
+        // group the options for each target config file separately
+        Map<String, List<KarafDistributionConfigurationFileOption>> optionMap = new HashMap<>();
+        for (KarafDistributionConfigurationFileOption option : options) {
+            optionMap.computeIfAbsent(option.getConfigurationFilePath(), k -> new ArrayList<>()).add(option);
+        }
+
+        // apply the options (in the order they appear) to each config file
+        for (String file : optionMap.keySet()) {
+            KarafConfigurationFile config = getKarafConfigurationFile(karafHome, file);
+            config.load();
+            boolean dirty = false;
+            for (KarafDistributionConfigurationFileOption option : optionMap.get(file)) {
+                if (option instanceof KarafDistributionConfigurationFilePutOption) {
+                    config.put(option.getKey(), option.getValue());
+                    dirty = true;
                 }
-            }
-            karafConfigurationFile.load();
-            Collection<List<KarafDistributionConfigurationFileOption>> optionsToApply = optionMap
-                .get(configFile).values();
-            boolean store = true;
-            for (List<KarafDistributionConfigurationFileOption> optionListToApply : optionsToApply) {
-                for (KarafDistributionConfigurationFileOption optionToApply : optionListToApply) {
-                    if (optionToApply instanceof KarafDistributionConfigurationFilePutOption) {
-                        karafConfigurationFile.put(optionToApply.getKey(), optionToApply.getValue());
-                    }
-                    else if (optionToApply instanceof KarafDistributionConfigurationFileReplacementOption) {
-                        karafConfigurationFile
-                            .replace(((KarafDistributionConfigurationFileReplacementOption) optionToApply)
-                                .getSource());
-                        store = false;
-                        break;
-                    }
-                    else {
-                        karafConfigurationFile
-                            .extend(optionToApply.getKey(), optionToApply.getValue());
-                    }
+                else if (option instanceof KarafDistributionConfigurationFileExtendOption) {
+                    config.extend(option.getKey(), option.getValue());
+                    dirty = true;
                 }
-                if (!store) {
-                    break;
+                else if (option instanceof KarafDistributionConfigurationFileReplacementOption) {
+                    config.replace(((KarafDistributionConfigurationFileReplacementOption) option).getSource());
+                    dirty = false;
                 }
             }
-            if (store) {
-                karafConfigurationFile.store();
+            if (dirty) {
+                config.store();
             }
         }
     }

containers/pax-exam-container-karaf/src/test/java/org/ops4j/pax/exam/karaf/container/ValidateConfigurationOptions.java

@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 org.ops4j.pax.exam.karaf.container;
+
+import static org.junit.Assert.assertEquals;
+import static org.ops4j.pax.exam.CoreOptions.composite;
+import static org.ops4j.pax.exam.karaf.options.KarafDistributionOption.editConfigurationFileExtend;
+import static org.ops4j.pax.exam.karaf.options.KarafDistributionOption.editConfigurationFilePut;
+import static org.ops4j.pax.exam.karaf.options.KarafDistributionOption.replaceConfigurationFile;
+import java.io.File;
+import java.io.IOException;
+import java.util.Dictionary;
+import javax.inject.Inject;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.ops4j.pax.exam.Configuration;
+import org.ops4j.pax.exam.Option;
+import org.ops4j.pax.exam.junit.PaxExam;
+import org.osgi.service.cm.ConfigurationAdmin;
+
+@RunWith(PaxExam.class)
+public class ValidateConfigurationOptions extends Karaf4TestContainerITest {
+
+    @Inject
+    ConfigurationAdmin configurationAdmin;
+
+    @Override
+    @Configuration
+    public Option[] config() {
+        File source1 = new File("target/test-classes/etc/source1.cfg");
+        File source2 = new File("target/test-classes/etc/source2.cfg");
+        return new Option[] {
+            composite(super.config()),
+            // put new
+            editConfigurationFilePut("etc/test.put.cfg", "param1", "value1"),
+            // put over existing
+            editConfigurationFilePut("etc/test.put.cfg", "param2", "value1"),
+            editConfigurationFilePut("etc/test.put.cfg", "param2", "value2"),
+            // extend new
+            editConfigurationFileExtend("etc/test.put.cfg", "params1", "value1"),
+            // extend existing
+            editConfigurationFilePut("etc/test.put.cfg", "params2", "value1"),
+            editConfigurationFileExtend("etc/test.put.cfg", "params2", "value2"),
+            // put new source file
+            composite(editConfigurationFilePut("etc/test.put.file.cfg", source1)),
+            // put and then replace file
+            composite(editConfigurationFilePut("etc/test.replace.cfg", source1)),
+            replaceConfigurationFile("etc/test.replace.cfg", source2),
+            // put over replaced file
+            editConfigurationFilePut("etc/test.replace.cfg", "param6", "value6"),
+            // extend over replaced file
+            editConfigurationFileExtend("etc/test.replace.cfg", "param4", "value5"),
+            // put null over replaced file
+            editConfigurationFilePut("etc/test.replace.cfg", "param5", null)
+        };
+    }
+
+    private void assertConfig(String pid, String property, Object expected) throws IOException {
+        org.osgi.service.cm.Configuration conf = configurationAdmin.getConfiguration(pid);
+        Dictionary<String, Object> properties = conf.getProperties();
+        Object actual = properties.get(property);
+        assertEquals(expected, actual);
+    }
+
+    @Test
+    public void testConfiguration() throws IOException {
+        assertConfig("test.put", "param1", "value1");
+        assertConfig("test.put", "param2", "value2");
+        assertConfig("test.put", "params1", "value1");
+        assertConfig("test.put", "params2", "value1,value2");
+        assertConfig("test.put.file", "param3", "value3");
+        assertConfig("test.replace", "param1", "value11");
+        assertConfig("test.replace", "param6", "value6");
+        assertConfig("test.replace", "param4", "value4,value5");
+    }
+}

containers/pax-exam-container-karaf/src/test/java/org/ops4j/pax/exam/karaf/container/internal/KarafCfgFileTest.java

@@ -0,0 +1,31 @@
+/*
+ * 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 org.ops4j.pax.exam.karaf.container.internal;
+
+import java.io.File;
+
+public class KarafCfgFileTest extends KarafConfigurationFileTest {
+
+    @Override
+    protected KarafConfigurationFile newKarafConfigurationFile(File karafHome, String location) {
+        return new KarafCfgFile(karafHome, location + ".cfg");
+    }
+
+    @Override
+    protected String[] toStrings(Object strings) {
+        return ((String)strings).split(",");
+    }
+}

containers/pax-exam-container-karaf/src/test/java/org/ops4j/pax/exam/karaf/container/internal/KarafConfigFileTest.java

@@ -17,75 +17,50 @@
  */
 package org.ops4j.pax.exam.karaf.container.internal;
 
+import static org.junit.Assert.assertArrayEquals;
 import java.io.File;
-import java.io.FileOutputStream;
-import java.util.Dictionary;
-import java.util.Hashtable;
-
-import org.apache.felix.cm.file.ConfigurationHandler;
 import org.junit.Test;
 
-import static junit.framework.TestCase.assertEquals;
-import static org.junit.Assert.assertArrayEquals;
-
-public class KarafConfigFileTest {
+public class KarafConfigFileTest extends KarafConfigurationFileTest {
 
-    final static private File KARAF_ETC = new File("src/test/resources");
+    @Override
+    protected KarafConfigurationFile newKarafConfigurationFile(File karafHome, String location) {
+        return new KarafConfigFile(karafHome, location + ".config");
+    }
 
-    @Test
-    public void store() throws Exception {
-        final int[] ints = {1, 2, 3, 4};
-        final Dictionary<String, Object> configuration = new Hashtable<>();
-        configuration.put("ints", ints);
-        final FileOutputStream fos = new FileOutputStream("target/store.config");
-        ConfigurationHandler.write(fos, configuration);
-        fos.close();
+    @Override
+    protected String[] toStrings(Object strings) {
+        return (String[])strings;
     }
 
     @Test
-    public void load() throws Exception {
+    public void loadInts() throws Exception {
         final int[] expected = {1, 2, 3, 4};
-        final KarafConfigFile karafConfigFile = new KarafConfigFile(KARAF_ETC, "etc/ints_4.config");
+        final KarafConfigurationFile karafConfigFile = newKarafConfigurationFile(KARAF_ETC, "etc/ints_4");
         karafConfigFile.load();
         final int[] ints = (int[]) karafConfigFile.get("ints");
         assertArrayEquals(expected, ints);
     }
 
     @Test
-    public void put() throws Exception {
-        final Double d = 2D;
-        final KarafConfigFile karafConfigFile = new KarafConfigFile(KARAF_ETC, "etc/na.config");
-        karafConfigFile.put("Double", d);
-    }
-
-    @Test
-    public void extendEmpty() throws Exception {
+    public void extendEmptyInts() throws Exception {
         final Integer[] expected = {1};
         final int i = 1;
-        final KarafConfigFile karafConfigFile = new KarafConfigFile(KARAF_ETC, "etc/na.config");
+        final KarafConfigurationFile karafConfigFile = newKarafConfigurationFile(KARAF_ETC, "etc/na");
         karafConfigFile.load();
         karafConfigFile.extend("ints", i);
         final Integer[] ints = (Integer[]) karafConfigFile.get("ints");
         assertArrayEquals(expected, ints);
     }
 
     @Test
-    public void extendExisting() throws Exception {
+    public void extendExistingInts() throws Exception {
         final int[] expected = {1, 2, 3, 4, 5};
-        final KarafConfigFile karafConfigFile = new KarafConfigFile(KARAF_ETC, "etc/ints_4.config");
+        final KarafConfigurationFile karafConfigFile = newKarafConfigurationFile(KARAF_ETC, "etc/ints_4");
         karafConfigFile.load();
         karafConfigFile.extend("ints", 5);
         final int[] ints = (int[]) karafConfigFile.get("ints");
         assertArrayEquals(expected, ints);
     }
 
-    @Test
-    public void get() throws Exception {
-        final String in = "value";
-        final KarafConfigFile karafConfigFile = new KarafConfigFile(KARAF_ETC, "etc/na.config");
-        karafConfigFile.put("key", in);
-        final Object out = karafConfigFile.get("key");
-        assertEquals(in, out);
-    }
-
-}
+}