FELIX-6812 Follow-up: escape the pid in the service.pid filter

[jkevan]

A follow-up on FELIX-6812, and it is described in this comment. fileinstall-3.7.5 is not released yet, so the fix can still travel with the change it repairs. Tell me if you would rather see a new issue, and I will retitle this pull request.

What happens

FELIX-6812 replaced getConfiguration(pid, "?") with a listConfigurations call, and it builds the filter by concatenating the pid. A pid is not a filter-safe value. findExistingConfiguration escapes its own filter value through escapeFilterValue, nine lines below in the same class, and the new call site does not.

Two failures follow, and neither one is visible to the user.

• A pid holding an asterisk builds a substring filter. listConfigurations then returns configurations the event was not about, and ConfigInstaller writes the first of them back to that other configuration's file.
• A pid holding a parenthesis throws InvalidSyntaxException. The catch reports it as "Unable to save configuration", so the configuration is never written back to its file.

What changed

The pid is escaped through escapeFilterValue, which is the method the other call site already uses.

escapeFilterValue left the backslash alone, although the OSGi filter grammar reserves that character. The escaping added for the pid is incomplete without the backslash case, so this pull request covers both. The backslash is escaped first, or the escapes added after it would be escaped a second time.

The backslash defect predates FELIX-6812, and it affects findExistingConfiguration as well. Say the word and I will move that half to an issue of its own.

The method now matches literals rather than patterns. String.replaceAll compiles a pattern on each call, and escapeFilterValue compiled four of them. FELIX-6812 moved this method from findExistingConfiguration, called once per file install, to doConfigurationEvent, called for every configuration event.

The empty result of listConfigurations now logs at debug level. listConfigurations returns only the configurations the caller may see, and getConfiguration(pid, "?") did not filter that way. A configuration bound to another bundle's location therefore stops being written back to its file once a SecurityManager runs. See Configuration Admin 104.13.3. Neither early return left a trace before.

The two tests FELIX-6812 left green and empty

FELIX-6812 moved the CM_UPDATED path to listConfigurations, and two tests kept the expectations of the older call. Neither test calls EasyMock.verify, so an expectation nothing consumes fails nothing.

• testUseExistingConfigAndObserveCMDeleted expects listConfigurations for any argument and answers null. The handler returns before it reads anything, so the test asserts nothing at all while it passes green.
• testUseExistingConfigWithFileinstallFilenameAndObserveCMDeleted still expects getConfiguration and update. The CM_UPDATED path writes the file and calls no method on the configuration, and getConfiguration is reachable from setConfig only, which this test never calls.

Both tests verify their mocks now, and the dead expectations go. Adding the verify call before removing the expectations fails on Configuration.update, which is the measurement behind the commit.

This half is test hygiene rather than a defect in the shipped code, and it is the fallout of the same change. Tell me if you would rather see it in an issue of its own, and I will move the commit.

Tests

testTheConfigurationEventFilterEscapesAnAsteriskInThePid and testTheConfigurationEventFilterEscapesABackslashInThePid raise CM_UPDATED for such a pid and assert the filter the handler builds. Each test fails when its escape is removed.

mvn test on the fileinstall module passes on JDK 11, except DirectoryWatcherTest.testInvalidTempDir. That test also fails on an unmodified master on this machine, which is FELIX-6837.