FELIX-6862 Only handle configuration files this installer handles

fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java

@@ -67,7 +67,10 @@ public class ConfigInstaller implements ArtifactInstaller, ConfigurationListener
     private final BundleContext context;
     private final ConfigurationAdmin configAdmin;
     private final FileInstall fileInstall;
-    private final Map<String, String> pidToFile = new HashMap<>();
+    // Package-private, because ConfigInstallerTest asserts what init() adopts. Two sites filter
+    // this map, and either one keeps a foreign file alive, so a test that reads the file cannot
+    // fail on one site alone.
+    final Map<String, String> pidToFile = new HashMap<>();
     private final Method getFactoryConfigurationMethod;
     private final Method addAttributesMethod;
     private final Method getAttributesMethod;
@@ -163,7 +166,13 @@ public void init()
                     for (Configuration config : configs) {
                         Dictionary<?, ?> dict = config.getProperties();
                         String fileName = dict != null ? (String) dict.get(DirectoryWatcher.FILENAME) : null;
-                        if (fileName != null) {
+                        // This installer owns .cfg and .config files, and no other format.
+                        // Another ArtifactInstaller records felix.fileinstall.filename for a format of its own.
+                        // A pid adopted here is deleted with its file on CM_DELETED, so the filter runs first.
+                        // canHandle reads the file name only, so new File is enough here.
+                        // fromConfigKey would call URI.create, which throws on a value that is not a URI.
+                        // The catch around this loop would then leave pidToFile half-built.
+                        if (fileName != null && canHandle(new File(fileName))) {
                             pidToFile.put(config.getPid(), fileName);
                         }
                     }
@@ -308,7 +317,9 @@ public void doConfigurationEvent(ConfigurationEvent configurationEvent)
             try {
                 String fileName = pidToFile.remove(configurationEvent.getPid());
                 File file = fileName != null ? fromConfigKey(fileName) : null;
-                if (file != null && file.isFile()) {
+                // Deleting the file loses data, so this site checks ownership as well.
+                // Every writer of pidToFile filters already, and this check covers the next writer.
+                if (file != null && file.isFile() && canHandle(file)) {
                     if (!file.delete()) {
                         throw new IOException("Unable to delete file: " + file);
                     }

fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java

@@ -46,6 +46,9 @@
  */
 public class ConfigInstallerTest extends TestCase {
 
+    /** The pid of the one configuration initOverTheConfigurationThatRecords sets up. */
+    private static final String ADOPTED_PID = "test";
+
     BundleContext mockBundleContext;
     Bundle mockBundle;
     ConfigurationAdmin mockConfigurationAdmin;
@@ -278,6 +281,155 @@ public Dictionary<String, Object> answer() throws Throwable {
         assertFalse("Configuration file should be deleted", file.isFile());
     }
 
+    /**
+     * init() adopts a configuration into pidToFile, and CM_DELETED deletes the file that map names.
+     * A configuration written by another ArtifactInstaller also records felix.fileinstall.filename.
+     * Without a canHandle filter this installer therefore deletes a file it does not handle.
+     */
+    public void testCmDeletedKeepsAFileOfAnotherInstallersFormat() throws Exception
+    {
+        File file = File.createTempFile("test", ".yml");
+        try
+        {
+            deleteTheConfigurationThatRecords(file);
+            assertTrue("A .yml file belongs to another installer, so CM_DELETED keeps it", file.isFile());
+        }
+        finally
+        {
+            file.delete();
+        }
+    }
+
+    public void testCmDeletedStillRemovesAFileOfItsOwnFormat() throws Exception
+    {
+        File file = File.createTempFile("test", ".cfg");
+        try
+        {
+            deleteTheConfigurationThatRecords(file);
+            assertFalse("A .cfg file belongs to this installer, so CM_DELETED removes it", file.isFile());
+        }
+        finally
+        {
+            file.delete();
+        }
+    }
+
+    /**
+     * init() adopts a configuration into pidToFile, and both the adoption site and the deletion
+     * site filter on canHandle. Either filter keeps a foreign file alive, so a test that reads
+     * the file passes when one of the two filters is removed. This test reads the map instead,
+     * so it fails on the adoption site alone.
+     */
+    public void testInitLeavesAConfigurationOfAnotherInstallersFormatAlone() throws Exception
+    {
+        File file = File.createTempFile("test", ".yml");
+        try
+        {
+            ConfigInstaller ci = initOverTheConfigurationThatRecords(file);
+            assertTrue("A .yml file belongs to another installer, so init() does not adopt its pid",
+                       ci.pidToFile.isEmpty());
+        }
+        finally
+        {
+            file.delete();
+        }
+    }
+
+    public void testInitAdoptsAConfigurationOfItsOwnFormat() throws Exception
+    {
+        File file = File.createTempFile("test", ".cfg");
+        try
+        {
+            ConfigInstaller ci = initOverTheConfigurationThatRecords(file);
+            assertEquals("A .cfg file belongs to this installer, so init() adopts its pid",
+                         file.toURI().toString(), ci.pidToFile.get(ADOPTED_PID));
+        }
+        finally
+        {
+            file.delete();
+        }
+    }
+
+    /**
+     * A writer that does not filter puts a foreign file in pidToFile, which is the state the
+     * deletion site is there for. The map carries that state here, so this test fails on the
+     * deletion site alone.
+     */
+    public void testCmDeletedKeepsAFileTheMapNamesAndThisInstallerDoesNotHandle() throws Exception
+    {
+        File file = File.createTempFile("test", ".yml");
+        try
+        {
+            EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes();
+            EasyMock.expect(mockBundle.loadClass(ConfigurationAttribute.class.getName()))
+                    .andReturn((Class) ConfigurationAttribute.class).anyTimes();
+            EasyMock.expect(mockBundleContext.getProperty((String) EasyMock.anyObject()))
+                    .andReturn(null).anyTimes();
+
+            ServiceReference<ConfigurationAdmin> sr = EasyMock.createMock(ServiceReference.class);
+            EasyMock.replay(mockConfiguration, mockConfigurationAdmin, mockBundleContext, mockBundle, sr);
+
+            ConfigInstaller ci = new ConfigInstaller(mockBundleContext, mockConfigurationAdmin, new FileInstall());
+            ci.pidToFile.put(ADOPTED_PID, file.toURI().toString());
+
+            ci.doConfigurationEvent(new ConfigurationEvent(sr, ConfigurationEvent.CM_DELETED, null, ADOPTED_PID));
+
+            assertTrue("A .yml file belongs to another installer, so CM_DELETED keeps it", file.isFile());
+        }
+        finally
+        {
+            file.delete();
+        }
+    }
+
+    /**
+     * Run init() over one configuration that records the given file, then raise CM_DELETED for its pid.
+     * The caller asserts on the file, because whether the file survives is the whole behaviour.
+     */
+    private void deleteTheConfigurationThatRecords(File file) throws Exception
+    {
+        ConfigInstaller ci = initOverTheConfigurationThatRecords(file);
+
+        ServiceReference<ConfigurationAdmin> sr = EasyMock.createMock(ServiceReference.class);
+        EasyMock.replay(sr);
+
+        ci.doConfigurationEvent(new ConfigurationEvent(sr, ConfigurationEvent.CM_DELETED, null, ADOPTED_PID));
+    }
+
+    /**
+     * Run init() over one configuration that records the given file, and return the installer.
+     */
+    private ConfigInstaller initOverTheConfigurationThatRecords(File file) throws Exception
+    {
+        Dictionary<String, Object> props = new Hashtable<>();
+        props.put(DirectoryWatcher.FILENAME, file.toURI().toString());
+
+        EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes();
+        EasyMock.expect(mockBundle.loadClass(ConfigurationAttribute.class.getName()))
+                .andReturn((Class) ConfigurationAttribute.class).anyTimes();
+        EasyMock.expect(mockBundleContext.getProperty((String) EasyMock.anyObject()))
+                .andReturn(null).anyTimes();
+        EasyMock.expect(mockBundleContext.registerService((String[]) EasyMock.anyObject(),
+                                                          EasyMock.anyObject(),
+                                                          (Dictionary<String, ?>) EasyMock.anyObject()))
+                .andReturn(null);
+        EasyMock.expect(mockConfigurationAdmin.listConfigurations(null))
+                .andReturn(new Configuration[] { mockConfiguration });
+        EasyMock.expect(mockConfiguration.getProperties()).andReturn(props).anyTimes();
+        EasyMock.expect(mockConfiguration.getPid()).andReturn(ADOPTED_PID).anyTimes();
+
+        EasyMock.replay(mockConfiguration, mockConfigurationAdmin, mockBundleContext, mockBundle);
+
+        ConfigInstaller ci = new ConfigInstaller(mockBundleContext, mockConfigurationAdmin, new FileInstall());
+        ci.init();
+
+        // init() must have read the configurations. Without this call the test passes even when
+        // init() throws before its loop, because ConfigInstaller logs that failure and swallows it.
+        EasyMock.verify(mockConfigurationAdmin, mockBundleContext);
+
+        return ci;
+    }
+
     public void testUseExistingConfigAndObserveCMDeleted() throws Exception
     {
         String pid = "test";