From 4766c7c316857d660fa6cba60e26a4b7d8b8020e Mon Sep 17 00:00:00 2001 From: Kevan Date: Tue, 15 Sep 2026 16:30:38 +0200 Subject: [PATCH 1/2] FELIX-6862 Only handle configuration files this installer handles FELIX-5832 added a canHandle filter to the CM_UPDATED path, under the rule its subject states: only handle ConfigurationEvents for config objects managed by us. Three sites read or write pidToFile, and that change covered one of them. init() adopts every configuration that records felix.fileinstall.filename, whatever the format of the file. The CM_DELETED path then deletes the file that the resulting entry names. Another ArtifactInstaller records the same property for a format of its own, so after a restart this installer owns that installer's files and deletes one when its configuration is deleted. init() applies the filter now, so a file of another format no longer enters pidToFile. The CM_DELETED path applies it too, because deleting the file is the act that loses data and a filter at each writer makes the invariant depend on every future writer repeating it. canHandle reads the file name only, so new File is enough in init(). Passing fromConfigKey would call URI.create, which throws on a value that is not a URI, and the catch around that loop would then leave pidToFile half-built. Behaviour for .cfg and .config files is unchanged. Two tests cover the pair, and the first fails when both filters are removed. --- .../fileinstall/internal/ConfigInstaller.java | 12 +++- .../internal/ConfigInstallerTest.java | 69 +++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) 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..00e14e2ab6 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 @@ -163,7 +163,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 +314,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..1bb95dcb78 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 @@ -278,6 +278,75 @@ 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(); + } + } + + /** + * 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 + { + String pid = "test"; + 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(pid).anyTimes(); + + ServiceReference sr = EasyMock.createMock(ServiceReference.class); + EasyMock.replay(mockConfiguration, mockConfigurationAdmin, mockBundleContext, mockBundle, sr); + + ConfigInstaller ci = new ConfigInstaller(mockBundleContext, mockConfigurationAdmin, new FileInstall()); + ci.init(); + ci.doConfigurationEvent(new ConfigurationEvent(sr, ConfigurationEvent.CM_DELETED, null, pid)); + + // 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); + } + public void testUseExistingConfigAndObserveCMDeleted() throws Exception { String pid = "test"; From 244a4c8a57f346914389f1b5350e4fcbb589abd1 Mon Sep 17 00:00:00 2001 From: Kevan Date: Thu, 17 Sep 2026 11:19:27 +0200 Subject: [PATCH 2/2] FELIX-6862 Pin each filter with a test of its own A reviewer of the same change on a fork measured what the suite holds. Removing the init() filter alone left the suite green, removing the CM_DELETED filter alone left it green, and only removing both failed a test. Either filter keeps a foreign file alive, so a test that reads the file cannot fail on one site alone. pidToFile is package-private now, and three tests read it. ConfigInstaller already exposes shouldSaveConfig and getConfiguration to this test class for the same reason, and the comment on the field states why the map is not private. testInitLeavesAConfigurationOfAnotherInstallersFormatAlone runs init() over a configuration that records a .yml file, and asserts the map stays empty. testInitAdoptsAConfigurationOfItsOwnFormat does the same with a .cfg file and asserts the map names it. A third test puts a .yml file in the map itself, which is the state the deletion site is there for, and raises CM_DELETED. Each of the three fails when its own filter goes away. The two tests that read the file stay, because they state the behaviour a user sees. --- .../fileinstall/internal/ConfigInstaller.java | 5 +- .../internal/ConfigInstallerTest.java | 93 ++++++++++++++++++- 2 files changed, 92 insertions(+), 6 deletions(-) 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 00e14e2ab6..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; 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 1bb95dcb78..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; @@ -311,13 +314,93 @@ public void testCmDeletedStillRemovesAFileOfItsOwnFormat() throws Exception } } + /** + * 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 { - String pid = "test"; + 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()); @@ -333,18 +416,18 @@ private void deleteTheConfigurationThatRecords(File file) throws Exception EasyMock.expect(mockConfigurationAdmin.listConfigurations(null)) .andReturn(new Configuration[] { mockConfiguration }); EasyMock.expect(mockConfiguration.getProperties()).andReturn(props).anyTimes(); - EasyMock.expect(mockConfiguration.getPid()).andReturn(pid).anyTimes(); + EasyMock.expect(mockConfiguration.getPid()).andReturn(ADOPTED_PID).anyTimes(); - ServiceReference sr = EasyMock.createMock(ServiceReference.class); - EasyMock.replay(mockConfiguration, mockConfigurationAdmin, mockBundleContext, mockBundle, sr); + EasyMock.replay(mockConfiguration, mockConfigurationAdmin, mockBundleContext, mockBundle); ConfigInstaller ci = new ConfigInstaller(mockBundleContext, mockConfigurationAdmin, new FileInstall()); ci.init(); - ci.doConfigurationEvent(new ConfigurationEvent(sr, ConfigurationEvent.CM_DELETED, null, pid)); // 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