#6031: Declaration Manager is creating a .bak file before removing a …

…decl from the target path.
codereader · Aug 13, 2022 · 95b3470 · 95b3470
1 parent 322ffce
commit 95b3470
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 14 deletions.
diff --git a/libs/os/file.h b/libs/os/file.h
@@ -113,4 +113,28 @@ inline std::size_t getFileSize(const std::string& path)
 	}
 }
 
+// Move/rename the given file to a .bak file, overwriting any existing .bak file
+// If the current file doesn't exist, this does nothing.
+// Returns true on success, false on failure or if the file doesn't exist.
+inline bool moveToBackupFile(const fs::path& path)
+{
+    if (!fs::is_regular_file(path))
+    {
+        return false;
+    }
+
+    try
+    {
+        // Move the old target file to .bak (overwriting any existing .bak file)
+        fs::rename(path, path.string() + ".bak");
+        return true;
+    }
+    catch (fs::filesystem_error& e)
+    {
+        rError() << "Could not rename the existing file to .bak: " << path.string() << std::endl
+            << e.what() << std::endl;
+        return false;
+    }
+}
+
 } // namespace
diff --git a/libs/stream/ExportStream.h b/libs/stream/ExportStream.h
@@ -9,6 +9,7 @@
 
 #include "os/fs.h"
 #include "os/path.h"
+#include "os/file.h"
 
 namespace stream
 {
@@ -83,21 +84,10 @@ class ExportStream
         fs::path targetPath = _outputDirectory;
         targetPath /= _filename;
 
-        if (fs::exists(targetPath))
+        if (fs::exists(targetPath) && !os::moveToBackupFile(targetPath))
         {
-            try
-            {
-                // Move the old target file to .bak (overwriting any existing .bak file)
-                fs::rename(targetPath, targetPath.string() + ".bak");
-            }
-            catch (fs::filesystem_error& e)
-            {
-                rError() << "Could not rename the existing file to .bak: " << targetPath.string() << std::endl
-                    << e.what() << std::endl;
-
-                throw std::runtime_error(
-                    fmt::format(_("Could not rename the existing file to .bak: {0}"), targetPath.string()));
-            }
+            throw std::runtime_error(
+                fmt::format(_("Could not rename the existing file to .bak: {0}"), targetPath.string()));
         }
 
         try

diff --git a/radiantcore/decl/DeclarationManager.cpp b/radiantcore/decl/DeclarationManager.cpp
@@ -13,6 +13,7 @@
 #include "module/StaticModule.h"
 #include "string/trim.h"
 #include "os/path.h"
+#include "os/file.h"
 #include "fmt/format.h"
 #include "gamelib.h"
 #include "stream/TemporaryOutputStream.h"
@@ -434,6 +435,9 @@ void DeclarationManager::removeDeclarationFromFile(const IDeclaration::Ptr& decl
     auto syntaxTree = parser.parse();
     inheritStream.close();
 
+    // Move the old file to .bak before overwriting it
+    os::moveToBackupFile(fullPath);
+
     removeDeclarationFromSyntaxTree(syntaxTree, decl->getDeclName());
 
     // Export the modified syntax tree

diff --git a/test/DeclManager.cpp b/test/DeclManager.cpp
@@ -973,6 +973,53 @@ testdecl decl/removal/9
 */)")) << "The decl manager removed the wrong declaration";
 }
 
+// Before overwriting the existing file it will be moved to .bak
+TEST_F(DeclManagerTest, RemoveDeclarationCreatesBackupFile)
+{
+    GlobalDeclarationManager().registerDeclType("testdecl", std::make_shared<TestDeclarationCreator>());
+    GlobalDeclarationManager().registerDeclFolder(decl::Type::TestDecl, TEST_DECL_FOLDER, ".decl");
+
+    // Create a backup copy of the decl file we're going to manipulate
+    auto fullPath = _context.getTestProjectPath() + "testdecls/removal_tests.decl";
+    auto expectedBackupPath = fullPath + ".bak";
+
+    // Remove the bak file for this test
+    fs::remove(expectedBackupPath);
+
+    // We want to remove the .bak file at the end of the test
+    TemporaryFile expectedBakFile(expectedBackupPath);
+
+    // A backup copy to restore the decl file to its previous state
+    BackupCopy backup(fullPath);
+
+    // Check that both decls are there
+    EXPECT_TRUE(algorithm::fileContainsText(fullPath, "testDecl decl/removal/3a"));
+    EXPECT_TRUE(algorithm::fileContainsText(fullPath, "testdecl decl/removal/2"));
+    expectDeclIsPresent(decl::Type::TestDecl, "decl/removal/3a");
+    expectDeclIsPresent(decl::Type::TestDecl, "decl/removal/2");
+
+    GlobalDeclarationManager().removeDeclaration(decl::Type::TestDecl, "decl/removal/3a");
+
+    EXPECT_FALSE(algorithm::fileContainsText(fullPath, "testDecl decl/removal/3a"));
+    EXPECT_TRUE(algorithm::fileContainsText(fullPath, "testdecl decl/removal/2"));
+
+    // The backup file should be there and contain the old decl
+    EXPECT_TRUE(os::fileOrDirExists(expectedBackupPath)) << "Cannot find the backup file " << expectedBackupPath;
+    EXPECT_TRUE(algorithm::fileContainsText(expectedBackupPath, "testDecl decl/removal/3a"));
+    EXPECT_TRUE(algorithm::fileContainsText(expectedBackupPath, "testdecl decl/removal/2"));
+
+    // Now remove the second decl, it should overwrite the first .bak copy
+    GlobalDeclarationManager().removeDeclaration(decl::Type::TestDecl, "decl/removal/2");
+
+    EXPECT_FALSE(algorithm::fileContainsText(fullPath, "testDecl decl/removal/3a"));
+    EXPECT_FALSE(algorithm::fileContainsText(fullPath, "testdecl decl/removal/2"));
+    EXPECT_TRUE(os::fileOrDirExists(expectedBackupPath)) << "Cannot find the backup file " << expectedBackupPath;
+
+    // The decl that got removed first should be missing in this second backup version too
+    EXPECT_FALSE(algorithm::fileContainsText(expectedBackupPath, "testDecl decl/removal/3a"));
+    EXPECT_TRUE(algorithm::fileContainsText(expectedBackupPath, "testdecl decl/removal/2"));
+}
+
 TEST_F(DeclManagerTest, RemoveUnsavedDeclaration)
 {
     GlobalDeclarationManager().registerDeclType("testdecl", std::make_shared<TestDeclarationCreator>());