KARAF-7098 JsonConfigInstaller ignores R7 factory configurations

config/src/main/java/org/apache/karaf/config/core/impl/JsonConfigInstaller.java

@@ -20,6 +20,7 @@
 import org.apache.felix.fileinstall.ArtifactInstaller;
 import org.apache.felix.fileinstall.internal.DirectoryWatcher;
 import org.apache.felix.utils.collections.DictionaryAsMap;
+import org.apache.karaf.util.config.ConfigurationPID;
 import org.osgi.framework.Constants;
 import org.osgi.service.cm.Configuration;
 import org.osgi.service.cm.ConfigurationAdmin;
@@ -37,12 +38,15 @@
 import java.net.URL;
 import java.util.Dictionary;
 import java.util.Hashtable;
+import java.util.Objects;
+
+import static org.apache.karaf.util.config.ConfigurationPID.parseFilename;
 
 public class JsonConfigInstaller implements ArtifactInstaller, ConfigurationListener {
 
     private final static Logger LOGGER = LoggerFactory.getLogger(JsonConfigInstaller.class);
 
-    private ConfigurationAdmin configurationAdmin;
+    private final ConfigurationAdmin configurationAdmin;
 
     public JsonConfigInstaller(ConfigurationAdmin configurationAdmin) {
         this.configurationAdmin = configurationAdmin;
@@ -69,9 +73,9 @@ public void uninstall(File artifact) throws Exception {
     }
 
     private void setConfig(File artifact) throws Exception {
-        String name = artifact.getName();
-        String pid[] = parsePid(name);
-        Configuration configuration = getConfiguration(toConfigKey(artifact), pid[0], pid[1]);
+        final String filename = artifact.getName();
+        final ConfigurationPID configurationPID = parseFilename(filename);
+        Configuration configuration = getConfiguration(toConfigKey(artifact), configurationPID);
         Dictionary<String, Object> props = configuration.getProperties();
         Hashtable<String, Object> old = props != null ? new Hashtable<>(new DictionaryAsMap<>(props)) : null;
         Hashtable<String, Object> properties = Configurations.buildReader().build(new FileReader(artifact)).readConfiguration();
@@ -84,20 +88,22 @@ private void setConfig(File artifact) throws Exception {
         if (old == null || !old.equals(properties)) {
             properties.put(DirectoryWatcher.FILENAME, toConfigKey(artifact));
             if (old == null) {
-                LOGGER.info("Creating configuration from " + pid[0] + (pid[1] == null ? "" : "-" + pid[1]) + ".json");
+                LOGGER.info("Creating configuration from {}", artifact.getName());
             } else {
-                LOGGER.info("Updating configuration from " + pid[0] + (pid[1] == null ? "" : "-" + pid[1]) + ".json");
+                LOGGER.info("Updating configuration from {}", artifact.getName());
             }
             configuration.update(properties);
         }
     }
 
-    boolean deleteConfig(File f) throws Exception {
-        String pid[] = parsePid(f.getName());
-        LOGGER.info("Deleting configuration from " + pid[0] + (pid[1] == null ? "" : "-" + pid[1]) + ".json");
-        Configuration config = getConfiguration(toConfigKey(f), pid[0], pid[1]);
-        config.delete();
-        return true;
+    void deleteConfig(File artifact) throws Exception {
+        Configuration config = findExistingConfiguration(toConfigKey(artifact));
+        if (Objects.nonNull(config)) {
+            config.delete();
+            LOGGER.info("Configuration for {} found and deleted", artifact.getName());
+        } else {
+            LOGGER.info("Configuration for {} not found, unable to delete", artifact.getName());
+        }
     }
 
     @Override
@@ -140,41 +146,33 @@ private File getCfgFileFromProperty(Object val) throws URISyntaxException, Malfo
         return null;
     }
 
-    String[] parsePid(String path) {
-        String pid = path.substring(0, path.lastIndexOf('.'));
-        int n = pid.indexOf('-');
-        if (n > 0) {
-            String factoryPid = pid.substring(n + 1);
-            pid = pid.substring(0, n);
-            return new String[]{pid, factoryPid};
-        } else {
-            return new String[]{pid, null};
-        }
-    }
-
     String toConfigKey(File f) {
         return f.getAbsoluteFile().toURI().toString();
     }
 
-    Configuration getConfiguration(String fileName, String pid, String factoryPid) throws Exception {
-        Configuration oldConfiguration = findExistingConfiguration(fileName);
+    Configuration getConfiguration(String configKey, ConfigurationPID configurationPID) throws Exception {
+        Configuration oldConfiguration = findExistingConfiguration(configKey);
         Configuration cachedConfiguration = oldConfiguration != null ?
                 configurationAdmin.getConfiguration(oldConfiguration.getPid(), null) : null;
         if (cachedConfiguration != null) {
             return cachedConfiguration;
         } else {
-            Configuration newConfiguration;
-            if (factoryPid != null) {
-                newConfiguration = configurationAdmin.createFactoryConfiguration(pid, "?");
+            final Configuration newConfiguration;
+            if (configurationPID.isFactory()) {
+                if (configurationPID.isR7()) { // use provided name for R7 Factory PIDs
+                    newConfiguration = configurationAdmin.getFactoryConfiguration(configurationPID.getFactoryPid(), configurationPID.getName(), "?");
+                } else { // let ConfigurationAdmin create a random string for name part (for backwards compatibility)
+                    newConfiguration = configurationAdmin.createFactoryConfiguration(configurationPID.getFactoryPid(), "?");
+                }
             } else {
-                newConfiguration = configurationAdmin.getConfiguration(pid, "?");
+                newConfiguration = configurationAdmin.getConfiguration(configurationPID.getPid(), "?");
             }
             return newConfiguration;
         }
     }
 
-    Configuration findExistingConfiguration(String fileName) throws Exception {
-        String filter = "(" + DirectoryWatcher.FILENAME + "=" + escapeFilterValue(fileName) + ")";
+    Configuration findExistingConfiguration(String configKey) throws Exception {
+        String filter = "(" + DirectoryWatcher.FILENAME + "=" + escapeFilterValue(configKey) + ")";
         Configuration[] configurations = configurationAdmin.listConfigurations(filter);
         if (configurations != null && configurations.length > 0) {
             return configurations[0];

util/pom.xml

@@ -74,6 +74,13 @@
             <artifactId>mail</artifactId>
             <scope>test</scope>
         </dependency>
+        <!-- nullability -->
+        <dependency>
+            <groupId>org.jetbrains</groupId>
+            <artifactId>annotations</artifactId>
+            <scope>provided</scope>
+            <version>20.1.0</version>
+        </dependency>
     </dependencies>
 
     <properties>

util/src/main/java/org/apache/karaf/util/config/ConfigurationPID.java

@@ -0,0 +1,122 @@
+/*
+ * 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.apache.karaf.util.config;
+
+import java.util.Objects;
+
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+public final class ConfigurationPID {
+
+    private final String pid;
+
+    private final String factoryPid;
+
+    private final String name;
+
+    private static final String TILDE = "~";
+
+    private static final String DASH = "-";
+
+    public ConfigurationPID(@NotNull final String pid) {
+        this.pid = pid;
+        this.factoryPid = null;
+        this.name = null;
+    }
+
+    public ConfigurationPID(@NotNull final String pid, @NotNull final String factoryPid, @Nullable final String name) {
+        this.pid = pid;
+        this.factoryPid = factoryPid;
+        this.name = name;
+    }
+
+    @NotNull
+    public String getPid() {
+        return pid;
+    }
+
+    @Nullable
+    public String getFactoryPid() {
+        return factoryPid;
+    }
+
+    @Nullable
+    public String getName() {
+        return name;
+    }
+
+    public boolean isFactory() {
+        return Objects.nonNull(factoryPid);
+    }
+
+    public boolean isR7() {
+        return pid.contains(TILDE);
+    }
+
+    /**
+     * @param pid the OSGi Persistent Identity (PID)
+     * @return the ConfigurationPID parsed from pid
+     */
+    @NotNull
+    public static ConfigurationPID parsePid(@NotNull final String pid) {
+        final int index = pid.contains(TILDE) ? pid.indexOf(TILDE) : pid.indexOf(DASH);
+        if (index > 0) {
+            final String factoryPid = pid.substring(0, index);
+            final String name = pid.substring(index + 1);
+            return new ConfigurationPID(pid, factoryPid, name);
+        } else {
+            return new ConfigurationPID(pid);
+        }
+    }
+
+    /**
+     * @param filename a filename of a Configuration with a single extension
+     * @return the ConfigurationPID parsed from filename
+     */
+    @NotNull
+    public static ConfigurationPID parseFilename(@NotNull final String filename) {
+        final String pid = filename.substring(0, filename.lastIndexOf('.'));
+        return parsePid(pid);
+    }
+
+    /**
+     * @param filename  a filename of a Configuration
+     * @param extension a filename extension without leading dot, e.g. <code>cfg</code>, <code>config</code>, <code>json</code>, <code>cfg.json</code> or empty string (no extension)
+     * @return the ConfigurationPID parsed from filename
+     * @throws IllegalArgumentException if filename does not end with given extension
+     */
+    @NotNull
+    public static ConfigurationPID parseFilename(@NotNull final String filename, @NotNull final String extension) throws IllegalArgumentException {
+        final String pid;
+        if (extension.isEmpty()) {
+            pid = filename;
+        } else {
+            final String ending = String.format(".%s", extension);
+            if (filename.endsWith(ending)) {
+                pid = filename.substring(0, filename.length() - ending.length());
+            } else {
+                final String message = String.format("Parsing filename failed. Filename '%s' does not have given extension '%s'.", filename, extension);
+                throw new IllegalArgumentException(message);
+            }
+        }
+        return parsePid(pid);
+    }
+
+}

util/src/test/java/org/apache/karaf/util/config/ConfigurationPIDTest.java

@@ -0,0 +1,103 @@
+/*
+ * 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.apache.karaf.util.config;
+
+import org.junit.Test;
+
+import static org.apache.karaf.util.config.ConfigurationPID.parseFilename;
+import static org.apache.karaf.util.config.ConfigurationPID.parsePid;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class ConfigurationPIDTest {
+
+    @Test
+    public void testR7FactoryPID() {
+        final String pid = "org.apache.felix.jaas.Configuration.factory~TokenLoginModule";
+        final ConfigurationPID configurationPID = parsePid(pid);
+        assertEquals(pid, configurationPID.getPid());
+        assertEquals("org.apache.felix.jaas.Configuration.factory", configurationPID.getFactoryPid());
+        assertEquals("TokenLoginModule", configurationPID.getName());
+        assertTrue(configurationPID.isFactory());
+        assertTrue(configurationPID.isR7());
+    }
+
+    @Test
+    public void testPreR7FactoryPID() {
+        final String pid = "org.apache.felix.jaas.Configuration.factory-TokenLoginModule";
+        final ConfigurationPID configurationPID = parsePid(pid);
+        assertEquals(pid, configurationPID.getPid());
+        assertEquals("org.apache.felix.jaas.Configuration.factory", configurationPID.getFactoryPid());
+        assertEquals("TokenLoginModule", configurationPID.getName());
+        assertTrue(configurationPID.isFactory());
+        assertFalse(configurationPID.isR7());
+    }
+
+    @Test
+    public void testR7FactoryPIDFromFilenameWithCfgExtension() {
+        final String filename = "org.apache.felix.jaas.Configuration.factory~TokenLoginModule.cfg";
+        final ConfigurationPID configurationPID = parseFilename(filename);
+        assertEquals("org.apache.felix.jaas.Configuration.factory", configurationPID.getFactoryPid());
+        assertEquals("TokenLoginModule", configurationPID.getName());
+        assertTrue(configurationPID.isFactory());
+        assertTrue(configurationPID.isR7());
+    }
+
+    @Test
+    public void testR7FactoryPIDFromFilenameWithConfigExtension() {
+        final String filename = "org.apache.felix.jaas.Configuration.factory~TokenLoginModule.config";
+        final ConfigurationPID configurationPID = parseFilename(filename);
+        assertEquals("org.apache.felix.jaas.Configuration.factory", configurationPID.getFactoryPid());
+        assertEquals("TokenLoginModule", configurationPID.getName());
+        assertTrue(configurationPID.isFactory());
+        assertTrue(configurationPID.isR7());
+    }
+
+    @Test
+    public void testR7FactoryPIDFromFilenameWithJsonExtension() {
+        final String filename = "org.apache.felix.jaas.Configuration.factory~TokenLoginModule.json";
+        final ConfigurationPID configurationPID = parseFilename(filename);
+        assertEquals("org.apache.felix.jaas.Configuration.factory", configurationPID.getFactoryPid());
+        assertEquals("TokenLoginModule", configurationPID.getName());
+        assertTrue(configurationPID.isFactory());
+        assertTrue(configurationPID.isR7());
+    }
+
+    @Test
+    public void testR7FactoryPIDFromFilenameWithCfgJsonExtension() {
+        final String filename = "org.apache.felix.jaas.Configuration.factory~TokenLoginModule.cfg.json";
+        final ConfigurationPID configurationPID = parseFilename(filename, "cfg.json");
+        assertEquals("org.apache.felix.jaas.Configuration.factory", configurationPID.getFactoryPid());
+        assertEquals("TokenLoginModule", configurationPID.getName());
+        assertTrue(configurationPID.isFactory());
+        assertTrue(configurationPID.isR7());
+    }
+
+    @Test
+    public void testR7FactoryPIDFromFilenameWithNoExtension() {
+        final String filename = "org.apache.felix.jaas.Configuration.factory~TokenLoginModule";
+        final ConfigurationPID configurationPID = parseFilename(filename, "");
+        assertEquals("org.apache.felix.jaas.Configuration.factory", configurationPID.getFactoryPid());
+        assertEquals("TokenLoginModule", configurationPID.getName());
+        assertTrue(configurationPID.isFactory());
+        assertTrue(configurationPID.isR7());
+    }
+
+}