Merge lp:~therve/landscape-client/load-old-persist-on-empty into lp:~landscape/landscape-client/trunk

[Thomas Herve]

I spotted the problem while auditing for #788605. Hopefully it will reduce the chances of it happening.

[Jerry Seutter (jseutter)]

+1 looks good

review: Approve

[Alberto Donato (ack)]

Looks good, +1

review: Approve

=== modified file 'landscape/lib/persist.py'
--- landscape/lib/persist.py	2011-06-23 23:45:58 +0000
+++ landscape/lib/persist.py	2011-07-12 09:38:36 +0000
@@ -106,14 +106,8 @@
 
     def load(self, filepath):
         """Load a persisted database."""
-        filepath = os.path.expanduser(filepath)
-        if not os.path.isfile(filepath):
-            raise PersistError("File not found: %s" % filepath)
-        if os.path.getsize(filepath) == 0:
-            return
-        try:
-            self._hardmap = self._backend.load(filepath)
-        except:
+
+        def load_old():
             filepathold = filepath + ".old"
             if (os.path.isfile(filepathold) and
                 os.path.getsize(filepathold) > 0):
@@ -124,9 +118,23 @@
                 except:
                     raise PersistError("Broken configuration file at %s" %
                                        filepathold)
-            else:
-                raise PersistError("Broken configuration file at %s" %
-                                   filepath)
+                return True
+            return False
+
+        filepath = os.path.expanduser(filepath)
+        if not os.path.isfile(filepath):
+            if load_old():
+                return
+            raise PersistError("File not found: %s" % filepath)
+        if os.path.getsize(filepath) == 0:
+            load_old()
+            return
+        try:
+            self._hardmap = self._backend.load(filepath)
+        except:
+            if load_old():
+                return
+            raise PersistError("Broken configuration file at %s" % filepath)
 
     def save(self, filepath=None):
         """Save the persist to the given C{filepath}.

=== modified file 'landscape/lib/tests/test_persist.py'
--- landscape/lib/tests/test_persist.py	2011-07-05 05:09:11 +0000
+++ landscape/lib/tests/test_persist.py	2011-07-12 09:38:36 +0000
@@ -397,6 +397,46 @@
         filename = self.makeFile("")
         self.persist.load(filename)
 
+    def test_load_empty_files_restore_backup(self):
+        """
+        If the current file is empty, it tries to load the old one if it
+        exists.
+        """
+        filename = self.makeFile("")
+        filename_old = filename + ".old"
+
+        self.persist.set("a", 1)
+        self.persist.save(filename_old)
+
+        persist = self.build_persist()
+        persist.load(filename)
+
+        self.assertEqual(persist.get("a"), 1)
+
+    def test_non_existing_raise_error(self):
+        """
+        Trying to load a file that doesn't exist result in a L{PersistError}.
+        """
+        persist = self.build_persist()
+        self.assertRaises(PersistError, persist.load, "/nonexistent")
+
+    def test_non_existing_restore_backup(self):
+        """
+        If the file doesn't exist, it tries to load the old one if present and
+        valid.
+        """
+        filename = self.makeFile("")
+        filename_old = filename + ".old"
+        os.unlink(filename)
+
+        self.persist.set("a", 1)
+        self.persist.save(filename_old)
+
+        persist = self.build_persist()
+        persist.load(filename)
+
+        self.assertEqual(persist.get("a"), 1)
+
 
 class PicklePersistTest(GeneralPersistTest, SaveLoadPersistTest):