fix: remove deleted properties from property files #5340

Util/include/Poco/Util/PropertyFileConfiguration.h

@@ -124,8 +124,11 @@ class Util_API PropertyFileConfiguration: public MapConfiguration
 		/// includes), save preserves comments, blank lines, and
 		/// !include directives. Changed values are written back to the
 		/// file they were originally loaded from. New keys are appended
-		/// to the root file. If no provenance information is available,
-		/// the file is written as a flat list of key-value pairs.
+		/// to the root file. Source files of removed keys are also
+		/// rewritten so deleted properties do not linger on disk, even
+		/// when the removal empties an included file. If no provenance
+		/// information is available, the file is written as a flat list
+		/// of key-value pairs.
 
 	std::string getSourceFile(const std::string& key) const;
 		/// Returns the file path the given key was loaded from,
@@ -199,6 +202,7 @@ class Util_API PropertyFileConfiguration: public MapConfiguration
 		/// the parent (typically LayeredConfiguration) owns this
 		/// PropertyFileConfiguration, so the parent always outlives the child.
 	std::map<std::string, std::string> _sourceMap;
+	mutable std::set<std::string> _removedSourceFiles;
 	std::string _rootFile;
 };
 

Util/src/PropertyFileConfiguration.cpp

@@ -131,6 +131,7 @@ void PropertyFileConfiguration::save(const std::string& path) const
 {
 	// Snapshot state under the lock, then release before file I/O
 	std::map<std::string, std::map<std::string, std::string>> fileValues; // file -> {key -> value}
+	std::set<std::string> snapshotRemovedFiles;
 
 	{
 		AbstractConfiguration::ScopedLock lock(*this);
@@ -139,10 +140,17 @@ void PropertyFileConfiguration::save(const std::string& path) const
 		absPath.makeAbsolute();
 		std::string absPathStr = absPath.toString();
 
-		bool useProvenance = !_sourceMap.empty() && !_rootFile.empty() && absPathStr == _rootFile;
+		bool useProvenance = (!_sourceMap.empty() || !_removedSourceFiles.empty())
+			&& !_rootFile.empty()
+			&& absPathStr == _rootFile;
 
 		if (useProvenance)
 		{
+			snapshotRemovedFiles = _removedSourceFiles;
+
+			for (const auto& file : _removedSourceFiles)
+				fileValues[file];
+
 			// Group key-values by source file
 			for (const auto& [key, file] : _sourceMap)
 			{
@@ -176,6 +184,12 @@ void PropertyFileConfiguration::save(const std::string& path) const
 	{
 		saveToFile(file, values);
 	}
+
+	{
+		AbstractConfiguration::ScopedLock lock(*this);
+		for (const auto& file : snapshotRemovedFiles)
+			_removedSourceFiles.erase(file);
+	}
 }
 
 
@@ -311,13 +325,17 @@ void PropertyFileConfiguration::setSourceFile(const std::string& key, const std:
 void PropertyFileConfiguration::removeRaw(const std::string& key)
 {
 	MapConfiguration::removeRaw(key);
-	std::string prefix = key + '.';
+
+	bool removeAll = key.empty();
+	std::string prefix = removeAll ? std::string() : key + '.';
 	for (auto it = _sourceMap.begin(); it != _sourceMap.end(); )
 	{
-		if (it->first == key || it->first.compare(0, prefix.size(), prefix) == 0)
+		if (removeAll || it->first == key || it->first.compare(0, prefix.size(), prefix) == 0)
+		{
+			_removedSourceFiles.insert(it->second);
 			it = _sourceMap.erase(it);
-		else
-			++it;
+		}
+		else ++it;
 	}
 }
 
@@ -326,6 +344,7 @@ void PropertyFileConfiguration::clear()
 {
 	MapConfiguration::clear();
 	_sourceMap.clear();
+	_removedSourceFiles.clear();
 	_rootFile.clear();
 }
 

Util/testsuite/src/PropertyFileConfigurationTest.cpp

@@ -504,6 +504,147 @@ void PropertyFileConfigurationTest::testSavePreserving()
 }
 
 
+void PropertyFileConfigurationTest::testSaveRemovesLastIncludedKey()
+{
+	Poco::TemporaryFile extraFile;
+	{
+		Poco::FileOutputStream ostr(extraFile.path());
+		ostr << "extra.prop1 = true\n";
+	}
+
+	Poco::TemporaryFile rootFile;
+	{
+		Poco::FileOutputStream ostr(rootFile.path());
+		ostr << "root.key = rootValue\n";
+		ostr << "!include " << extraFile.path() << "\n";
+	}
+
+	AutoPtr<PropertyFileConfiguration> pConf = new PropertyFileConfiguration(rootFile.path());
+	assertTrue (pConf->getString("extra.prop1") == "true");
+
+	pConf->remove("extra.prop1");
+	pConf->save(rootFile.path());
+
+	{
+		Poco::FileInputStream istr(extraFile.path());
+		std::string content((std::istreambuf_iterator<char>(istr)), std::istreambuf_iterator<char>());
+		assertTrue (content.find("extra.prop1") == std::string::npos);
+	}
+
+	AutoPtr<PropertyFileConfiguration> pReloaded = new PropertyFileConfiguration(rootFile.path());
+	try
+	{
+		pReloaded->getString("extra.prop1");
+		fail("Expected NotFoundException");
+	}
+	catch (Poco::NotFoundException&) {}
+
+	Poco::TemporaryFile groupFile;
+	{
+		Poco::FileOutputStream ostr(groupFile.path());
+		ostr << "group.item.name = item1\n";
+		ostr << "group.item.channel = channel1\n";
+	}
+
+	Poco::TemporaryFile groupRootFile;
+	{
+		Poco::FileOutputStream ostr(groupRootFile.path());
+		ostr << "root.key = rootValue\n";
+		ostr << "!include " << groupFile.path() << "\n";
+	}
+
+	pConf = new PropertyFileConfiguration(groupRootFile.path());
+	pConf->remove("group");
+	pConf->save(groupRootFile.path());
+
+	{
+		Poco::FileInputStream istr(groupFile.path());
+		std::string content((std::istreambuf_iterator<char>(istr)), std::istreambuf_iterator<char>());
+		assertTrue (content.find("group.item.name") == std::string::npos);
+		assertTrue (content.find("group.item.channel") == std::string::npos);
+	}
+}
+
+
+void PropertyFileConfigurationTest::testSaveRemovesRootKey()
+{
+	Poco::TemporaryFile rootFile;
+	{
+		Poco::FileOutputStream ostr(rootFile.path());
+		ostr << "root.keep = keepValue\n";
+		ostr << "root.drop = dropValue\n";
+	}
+
+	AutoPtr<PropertyFileConfiguration> pConf = new PropertyFileConfiguration(rootFile.path());
+	pConf->remove("root.drop");
+	pConf->save(rootFile.path());
+
+	{
+		Poco::FileInputStream istr(rootFile.path());
+		std::string content((std::istreambuf_iterator<char>(istr)), std::istreambuf_iterator<char>());
+		assertTrue (content.find("root.drop") == std::string::npos);
+		assertTrue (content.find("root.keep") != std::string::npos);
+	}
+
+	AutoPtr<PropertyFileConfiguration> pReloaded = new PropertyFileConfiguration(rootFile.path());
+	assertTrue (pReloaded->getString("root.keep") == "keepValue");
+	try
+	{
+		pReloaded->getString("root.drop");
+		fail("Expected NotFoundException");
+	}
+	catch (Poco::NotFoundException&) {}
+}
+
+
+void PropertyFileConfigurationTest::testSaveRemovedFilesRetryAfterFailure()
+{
+	Poco::TemporaryFile extraFile;
+	{
+		Poco::FileOutputStream ostr(extraFile.path());
+		ostr << "extra.prop = extraValue\n";
+	}
+
+	Poco::TemporaryFile rootFile;
+	{
+		Poco::FileOutputStream ostr(rootFile.path());
+		ostr << "root.key = rootValue\n";
+		ostr << "!include " << extraFile.path() << "\n";
+	}
+
+	AutoPtr<PropertyFileConfiguration> pConf = new PropertyFileConfiguration(rootFile.path());
+	pConf->remove("extra.prop");
+
+	Poco::File extra(extraFile.path());
+	extra.setWriteable(false);
+
+	bool threw = false;
+	try
+	{
+		pConf->save(rootFile.path());
+	}
+	catch (Poco::Exception&)
+	{
+		threw = true;
+	}
+	extra.setWriteable(true);
+
+	// If the I/O layer didn't enforce read-only (e.g. running as root),
+	// skip the retry portion — the failure path can't be exercised.
+	if (!threw) return;
+
+	// Removed-file provenance must survive a failed save so the retry
+	// rewrites the included file and strips the removed key.
+	pConf->save(rootFile.path());
+
+	{
+		Poco::FileInputStream istr(extraFile.path());
+		std::string content((std::istreambuf_iterator<char>(istr)), std::istreambuf_iterator<char>());
+		assertTrue (content.find("extra.prop") == std::string::npos);
+	}
+}
+
+
 void PropertyFileConfigurationTest::testSavePreservingMultiLine()
 {
 	// Create a properties file with multi-line continuation values
@@ -854,6 +995,9 @@ CppUnit::Test* PropertyFileConfigurationTest::suite()
 	CppUnit_addTest(pSuite, PropertyFileConfigurationTest, testSave);
 	CppUnit_addTest(pSuite, PropertyFileConfigurationTest, testInclude);
 	CppUnit_addTest(pSuite, PropertyFileConfigurationTest, testSavePreserving);
+	CppUnit_addTest(pSuite, PropertyFileConfigurationTest, testSaveRemovesLastIncludedKey);
+	CppUnit_addTest(pSuite, PropertyFileConfigurationTest, testSaveRemovesRootKey);
+	CppUnit_addTest(pSuite, PropertyFileConfigurationTest, testSaveRemovedFilesRetryAfterFailure);
 	CppUnit_addTest(pSuite, PropertyFileConfigurationTest, testSavePreservingMultiLine);
 	CppUnit_addTest(pSuite, PropertyFileConfigurationTest, testClearResetsProvenance);
 	CppUnit_addTest(pSuite, PropertyFileConfigurationTest, testGetSourceFilesCoversAllKeys);

Util/testsuite/src/PropertyFileConfigurationTest.h

@@ -28,6 +28,9 @@ class PropertyFileConfigurationTest: public AbstractConfigurationTest
 	void testSave();
 	void testInclude();
 	void testSavePreserving();
+	void testSaveRemovesLastIncludedKey();
+	void testSaveRemovesRootKey();
+	void testSaveRemovedFilesRetryAfterFailure();
 	void testSavePreservingMultiLine();
 	void testClearResetsProvenance();
 	void testGetSourceFilesCoversAllKeys();