diff --git a/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java b/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java index 020ee0bb52..4bb35ead3c 100644 --- a/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java +++ b/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 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 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); } diff --git a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java b/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java index f423379041..b632757279 100644 --- a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java +++ b/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 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 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 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 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) 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";