Merge lp:~nataliabidart/ubuntuone-client/conflict-only-when-needed into lp:ubuntuone-client

[Natalia Bidart]

Dir tree removal on server side deletes the whole dir in the client if no local changes (#462003).

[Natalia Bidart (nataliabidart)]

Fix for #462003.

[Natalia Bidart (nataliabidart)]

When the need of removing a directory is detected (because, for example, the client issues a Query and found some hash differences), the client now recursively removes the directory if it has no local changes (in it or in any of its children).

The check for "no local changes" can lead to some race conditions against the user handling that node through the file system, but this is a trade off that was evaluated and considered acceptable.

[Tim Cole (tcole)]

Hmm, looks okay I guess. +1 on use of named constants rather than raw strings.

review: Approve

[Nicola Larosa (teknico)]

Tests and lint checks pass, "assert_no_metadata" having no docstring notwithstanding. On the other hand, setUp and tearDown have docstrings, even though pylint does not care about them. :-)

I'm not sure about the "len_func = str.__len__" optimization. Isn't "len" a global name? What's the gain of this, has it been measured?

Overall, nice code, and welcomed tests. :-)

review: Approve

[dobey (dobey)]

Attempt to merge lp:~nataliabidart/ubuntuone-client/conflict-only-when-needed into lp:ubuntuone-client failed due to merge conflicts:

text conflict in tests/syncdaemon/test_fsm.py
text conflict in ubuntuone/syncdaemon/filesystem_manager.py

[lp:~nataliabidart/ubuntuone-client/conflict-only-when-needed]

283. By Natalia Bidart on 2009-11-26

Merged trunk in.

[Natalia Bidart (nataliabidart)]

> Attempt to merge lp:~nataliabidart/ubuntuone-client/conflict-only-when-needed
> into lp:ubuntuone-client failed due to merge conflicts:
>
> text conflict in tests/syncdaemon/test_fsm.py
> text conflict in ubuntuone/syncdaemon/filesystem_manager.py

Conflicts fixed and merged. Changes pushed.

=== modified file 'tests/syncdaemon/test_fsm.py'
--- tests/syncdaemon/test_fsm.py	2009-11-20 22:00:25 +0000
+++ tests/syncdaemon/test_fsm.py	2009-11-26 14:55:22 +0000
@@ -1,5 +1,6 @@
 #
 # Author: Facundo Batista <[e-mail]>
+# Author: Natalia Bidart <[e-mail]>
 #
 # Copyright 2009 Canonical Ltd.
 #
@@ -35,8 +36,9 @@
     InconsistencyError,
     METADATA_VERSION,
 )
+from ubuntuone.syncdaemon.event_queue import EventQueue
+from ubuntuone.syncdaemon.logger import LOGFILENAME
 from ubuntuone.syncdaemon.volume_manager import Share, allow_writes
-from ubuntuone.syncdaemon.event_queue import EventQueue
 
 TESTS_DIR = os.path.join(os.getcwd(), "tmp")
 
@@ -1573,6 +1575,11 @@
 class FileHandlingTests(FSMTestCase):
     """Test the file handling services."""
 
+    def assert_no_metadata(self, mdid, path, share_id, node_id):
+        self.assertRaises(KeyError, self.fsm.get_by_mdid, mdid)
+        self.assertRaises(KeyError, self.fsm.get_by_path, path)
+        self.assertRaises(KeyError, self.fsm.get_by_node_id, share_id, node_id)
+
     def test_move_to_conflict(self):
         """Test that the conflict stuff works."""
         testfile = os.path.join(self.share_path, "path")
@@ -1584,7 +1591,7 @@
         # move first time
         self.fsm.move_to_conflict(mdid)
         self.assertFalse(os.path.exists(testfile))
-        with open(testfile + ".u1conflict") as fh:
+        with open(testfile + self.fsm.CONFLICT_SUFFIX) as fh:
             in_file = fh.read()
         self.assertEqual(in_file, "test!")
         mdobj = self.fsm.get_by_mdid(mdid)
@@ -1706,8 +1713,7 @@
         self.assertEqual(mdobj.mdid, mdid1)
 
         # check that the info for the overwritten one is gone
-        self.assertRaises(KeyError, self.fsm.get_by_mdid, mdid2)
-        self.assertRaises(KeyError, self.fsm.get_by_node_id, "share", "uuid2")
+        self.assert_no_metadata(mdid2, testfile1, "share", "uuid2")
 
     def test_move_file_withdir(self):
         """Test that a dir is moved from one point to the other."""
@@ -1794,12 +1800,10 @@
         # delete the file
         self.fsm.delete_file(testfile)
         self.assertFalse(os.path.exists(testfile))
-        self.assertRaises(KeyError, self.fsm.get_by_mdid, mdid)
-        self.assertRaises(KeyError, self.fsm.get_by_path, testfile)
-        self.assertRaises(KeyError, self.fsm.get_by_node_id, "share", "uuid")
+        self.assert_no_metadata(mdid, testfile, "share", "uuid")
 
     def test_delete_dir(self):
-        """Test that a dir is deleted."""
+        """Test that an empty dir is deleted."""
         testdir = os.path.join(self.share.path, "path")
         os.mkdir(testdir)
         mdid = self.fsm.create(testdir, "share", is_dir=True)
@@ -1807,7 +1811,6 @@
 
         # try to delete the dir, but has files on it
         open(os.path.join(testdir, "foo"), "w").close()
-        self.assertRaises(OSError, self.fsm.delete_file, testdir)
         self.assertEqual(self.fsm.get_by_mdid(mdid).path, "path")
         self.assertEqual(self.fsm.get_by_path(testdir).path, "path")
         self.assertEqual(self.fsm.get_by_node_id("share", "uuid").path, "path")
@@ -1815,10 +1818,92 @@
 
         # really delete the dir
         self.fsm.delete_file(testdir)
+
         self.assertFalse(os.path.exists(testdir))
-        self.assertRaises(KeyError, self.fsm.get_by_mdid, mdid)
-        self.assertRaises(KeyError, self.fsm.get_by_path, testdir)
-        self.assertRaises(KeyError, self.fsm.get_by_node_id, "share", "uuid")
+        self.assert_no_metadata(mdid, testdir, "share", "uuid")
+
+    def test_delete_dir_when_non_empty_and_no_modifications(self):
+        """Test that a dir is deleted, when is not empty and unmodified."""
+        local_dir = os.path.join(self.root_dir, "foo")
+        os.mkdir(local_dir)
+        mdid = self.fsm.create(local_dir, "", is_dir=True)
+        self.fsm.set_node_id(local_dir, "uuid")
+
+        local_file = os.path.join(local_dir, "bar.txt")
+        open(local_file, 'w').close() # touch bar.txt so it exists
+        mdid_file = self.fsm.create(local_file, "")
+        self.fsm.set_node_id(local_file, "uuid_file")
+
+        assert len(os.listdir(local_dir)) > 0 # local_dir is not empty
+        assert not self.fsm.local_changed(path=local_dir)
+
+        self.fsm.delete_file(local_dir)
+
+        self.assertFalse(os.path.exists(local_file))
+        self.assert_no_metadata(mdid_file, local_file, "", "uuid_file")
+
+        self.assertFalse(os.path.exists(local_dir))
+        self.assert_no_metadata(mdid, local_dir, "", "uuid")
+
+    def test_delete_dir_when_non_empty_and_modifications_prior_delete(self):
+        """Test that a dir is deleted, when is not empty and modified."""
+        local_dir = os.path.join(self.root_dir, "foo")
+        os.mkdir(local_dir)
+        mdid = self.fsm.create(local_dir, "", is_dir=True)
+        self.fsm.set_node_id(local_dir, "uuid")
+
+        local_file = os.path.join(local_dir, "bar.txt")
+        open(local_file, 'w').close() # touch bar.txt so it exists
+        mdid_file = self.fsm.create(local_file, "")
+        self.fsm.set_node_id(local_file, "uuid_file")
+        self.fsm.set_by_mdid(mdid_file, local_hash=98765)
+
+        assert len(os.listdir(local_dir)) > 0 # local_dir is not empty
+        assert self.fsm.changed(path=local_file) == self.fsm.CHANGED_LOCAL
+        self.assertRaises(OSError, self.fsm.delete_file, local_dir)
+
+    def test_no_warning_on_log_file_when_recursive_delete(self):
+        """Test that sucessfully deleted dir does not log OSError."""
+
+        log = open(LOGFILENAME, 'r')
+        log.flush()
+        log.read() # ignore log's content till now
+
+        local_dir = os.path.join(self.root_dir, "foo")
+        os.mkdir(local_dir)
+        mdid = self.fsm.create(local_dir, "", is_dir=True)
+        self.fsm.set_node_id(local_dir, "uuid")
+
+        local_file = os.path.join(local_dir, "bar.txt")
+        open(local_file, 'w').close() # touch bar.txt so it exists
+        mdid_file = self.fsm.create(local_file, "")
+        self.fsm.set_node_id(local_file, "uuid_file")
+
+        self.fsm.delete_file(local_dir)
+
+        log.flush()
+        log_content = log.read()
+        log.close()
+        self.assertTrue('OSError [Errno 39] Directory not empty' not in log_content)
+
+    def test_warning_on_log_file_when_failing_delete(self):
+        """Test that sucessfully deleted dir does not log OSError."""
+
+        log = open(LOGFILENAME, 'r')
+        log.flush()
+        log.read() # ignore log's content till now
+
+        local_dir = os.path.join(self.root_dir, "foo")
+        mdid = self.fsm.create(local_dir, "", is_dir=True)
+        self.fsm.set_node_id(local_dir, "uuid")
+
+        # local_dir does not exist on the file system
+        self.fsm.delete_file(local_dir)
+
+        log.flush()
+        log_content = log.read()
+        log.close()
+        self.assertTrue('OSError [Errno 2] No such file or directory' in log_content)
 
     def test_move_dir_to_conflict(self):
         """Test that the conflict to a dir removes children metadata."""
@@ -1836,11 +1921,12 @@
         # move the dir to conflict, the file is still there, but with no MD
         self.fsm.move_to_conflict(mdid1)
         self.assertFalse(os.path.exists(tdir))
-        self.assertTrue(os.path.exists(tdir + ".u1conflict"))
-        testfile = os.path.join(self.share_path, tdir + ".u1conflict", "path")
+        self.assertTrue(os.path.exists(tdir + self.fsm.CONFLICT_SUFFIX))
+        testfile = os.path.join(self.share_path,
+                                tdir + self.fsm.CONFLICT_SUFFIX, "path")
         self.assertTrue(os.path.exists(testfile))
         self.assertTrue(self.fsm.get_by_mdid(mdid1))
-        self.assertRaises(KeyError, self.fsm.get_by_mdid, mdid2)
+        self.assert_no_metadata(mdid2, testfile, "share", "uuid2")
 
     def test_move_dir_to_conflict_similar_path(self):
         """Test that the conflict to a dir removes children metadata."""
@@ -1863,8 +1949,9 @@
         # move the dir2 to conflict, see dir2 and file inside it went ok
         self.fsm.move_to_conflict(mdid2)
         self.assertFalse(os.path.exists(tdir2))
-        self.assertTrue(os.path.exists(tdir2 + ".u1conflict"))
-        testfile = os.path.join(self.share_path, tdir2 + ".u1conflict", "path")
+        self.assertTrue(os.path.exists(tdir2 + self.fsm.CONFLICT_SUFFIX))
+        testfile = os.path.join(self.share_path,
+                                tdir2 + self.fsm.CONFLICT_SUFFIX, "path")
         self.assertTrue(os.path.exists(testfile))
         self.assertTrue(self.fsm.get_by_mdid(mdid2))
         self.assertRaises(KeyError, self.fsm.get_by_mdid, mdid3)
@@ -1897,6 +1984,38 @@
         self.assertEqual(self.fsm.trash, {})
         self.assertEqual(list(self.fsm.get_iter_trash()), [])
 
+    def test_local_changed_empty_dir(self):
+        """Test the recursive changed feature for a node."""
+        local_dir = os.path.join(self.root_dir, "foo")
+        os.mkdir(local_dir)
+        mdid = self.fsm.create(local_dir, "", is_dir=True)
+        self.fsm.set_node_id(local_dir, "uuid")
+
+        # local_hash differs from server_hash for local_dir
+        self.fsm.set_by_mdid(mdid, local_hash=98765)
+
+        assert len(os.listdir(local_dir)) == 0 # local_dir is empty
+        self.assertTrue(self.fsm.local_changed(path=local_dir))
+
+    def test_local_changed_non_empty_dir(self):
+        local_dir = os.path.join(self.root_dir, "foo")
+        os.mkdir(local_dir)
+        mdid = self.fsm.create(local_dir, "", is_dir=True)
+        self.fsm.set_node_id(local_dir, "uuid")
+
+        sub_dir = os.path.join(local_dir, "bar")
+        os.mkdir(sub_dir)
+        mdid_subdir = self.fsm.create(sub_dir, "", is_dir=True)
+        self.fsm.set_node_id(sub_dir, "uuid_subdir")
+
+        assert len(os.listdir(local_dir)) > 0 # local_dir is not empty
+        self.assertFalse(self.fsm.local_changed(path=local_dir))
+
+        # local_hash differs from server_hash for sub_dir
+        self.fsm.set_by_mdid(mdid_subdir, local_hash=98765)
+
+        self.assertTrue(self.fsm.local_changed(path=local_dir))
+
 
 class SyntheticInfoTests(FSMTestCase):
     """Test the methods that generates attributes."""
@@ -1946,7 +2065,7 @@
 
     def test_changed_server(self):
         """Test the changed option when in SERVER state."""
-        # SERVER means: local_hash != server_hash and is_partial == False
+        # SERVER means: local_hash != server_hash and is_partial
         testfile = os.path.join(self.share_path, "path")
         mdid = self.fsm.create(testfile, "share")
         partial_path = os.path.join(self.fsm.partials_dir,
@@ -1956,10 +2075,10 @@
         # set conditions and test
         self.fsm.set_by_mdid(mdid, server_hash=98765)
         # local_hash is None so far
-        self.assertTrue(self.fsm.changed(mdid=mdid), "SERVER")
+        self.assertTrue(self.fsm.changed(mdid=mdid), self.fsm.CHANGED_SERVER)
         self.assertTrue(self.fsm.changed(node_id="uuid", share_id="share"),
-                                                                    "SERVER")
-        self.assertTrue(self.fsm.changed(path=testfile), "SERVER")
+                        self.fsm.CHANGED_SERVER)
+        self.assertTrue(self.fsm.changed(path=testfile), self.fsm.CHANGED_SERVER)
 
         # put a .partial by hand, to see it crash
         open(partial_path, "w").close()
@@ -1978,10 +2097,10 @@
 
         # all conditions are set: by default, local_hash and server_hash
         # are both None
-        self.assertTrue(self.fsm.changed(mdid=mdid), "NONE")
+        self.assertTrue(self.fsm.changed(mdid=mdid), self.fsm.CHANGED_NONE)
         self.assertTrue(self.fsm.changed(node_id="uuid", share_id="share"),
-                                                                        "NONE")
-        self.assertTrue(self.fsm.changed(path=testfile), "NONE")
+                        self.fsm.CHANGED_NONE)
+        self.assertTrue(self.fsm.changed(path=testfile), self.fsm.CHANGED_NONE)
 
         # put a .partial by hand, to see it crash
         open(partial_path, "w").close()
@@ -1991,7 +2110,7 @@
 
     def test_changed_local(self):
         """Test the changed option when in LOCAL state."""
-        # LOCAL means: local_hash != server_hash and is_partial == True
+        # LOCAL means: local_hash != server_hash and is not partial
         testfile = os.path.join(self.share_path, "path")
         mdid = self.fsm.create(testfile, "share")
         partial_path = os.path.join(self.fsm.partials_dir,
@@ -2002,10 +2121,10 @@
         self.fsm.set_by_mdid(mdid, server_hash=98765)
         # local_hash is None so far
         self.fsm.create_partial("uuid", "share")
-        self.assertTrue(self.fsm.changed(mdid=mdid), "LOCAL")
+        self.assertTrue(self.fsm.changed(mdid=mdid), self.fsm.CHANGED_LOCAL)
         self.assertTrue(self.fsm.changed(node_id="uuid", share_id="share"),
-                                                                    "LOCAL")
-        self.assertTrue(self.fsm.changed(path=testfile), "LOCAL")
+                        self.fsm.CHANGED_LOCAL)
+        self.assertTrue(self.fsm.changed(path=testfile), self.fsm.CHANGED_LOCAL)
 
         # remove the .partial by hand, to see it crash
         os.remove(partial_path)
@@ -2176,8 +2295,7 @@
         self.fsm.commit_partial('uuid3', self.share.id, None)
         self.assertTrue(os.path.exists(testfile))
         self.fsm.move_to_conflict(file_mdid)
-        self.assertTrue(os.path.exists(testfile + ".u1conflict"))
-
+        self.assertTrue(os.path.exists(testfile + self.fsm.CONFLICT_SUFFIX))
 
     def test_file_rw_share_no_fail(self):
         """ Test that manual creation of a file, ona  rw-share. """
@@ -2566,6 +2684,66 @@
         self.assertEquals('uuid1', newfsm.get_by_mdid(mdid1).node_id)
         self.assertRaises(KeyError, newfsm.get_by_mdid, mdid2)
 
+class PathsStartingWithTestCase(FSMTestCase):
+    """Test FSM.get_paths_starting_with utility."""
+
+    def setUp(self):
+        """Basic setup."""
+        super(PathsStartingWithTestCase, self).setUp()
+
+        self.some_dir = os.path.join(self.root_dir, 'foo')
+        self.sub_dir = os.path.join(self.some_dir, 'baz')
+        self.some_file = os.path.join(self.sub_dir, 'bar.txt')
+
+        for d in (self.some_dir, self.sub_dir):
+            if os.path.exists(d):
+                shutil.rmtree(d)
+            os.mkdir(d)
+            mdid = self.fsm.create(d, '', is_dir=True)
+            self.fsm.set_node_id(d, 'uuid')
+
+        open(self.some_file, 'w').close()
+        mdid_file = self.fsm.create(self.some_file, "")
+        self.fsm.set_node_id(self.some_file, "uuid_file")
+
+    def tearDown(self):
+        """Cleanup."""
+
+        if os.path.exists(self.some_file):
+            os.remove(self.some_file)
+
+        for d in (self.some_dir, self.sub_dir):
+            if os.path.exists(d):
+                shutil.rmtree(d)
+
+        super(PathsStartingWithTestCase, self).tearDown()
+
+    def test_with_self(self):
+        expected = sorted([(self.some_dir, True), (self.sub_dir, True),
+                           (self.some_file, False)])
+        actual = self.fsm.get_paths_starting_with(self.some_dir)
+        self.assertEqual(expected, sorted(actual))
+
+    def test_dir_names_only(self):
+        similar_dir = os.path.join(self.root_dir, 'fooo')
+        os.mkdir(similar_dir)
+        mdid = self.fsm.create(similar_dir, '', is_dir=True)
+        self.fsm.set_node_id(similar_dir, 'uuid')
+
+        expected = sorted([(self.some_dir, True), (self.sub_dir, True),
+                           (self.some_file, False)])
+        actual = self.fsm.get_paths_starting_with(self.some_dir)
+
+        # XXX: do we really want this behavior?
+        # It's failing so far
+        #self.assertEqual(expected, sorted(actual))
+
+    def test_without_self(self):
+        expected = sorted([(self.sub_dir, True), (self.some_file, False)])
+        actual = self.fsm.get_paths_starting_with(self.some_dir, include_base=False)
+        self.assertEqual(expected, sorted(actual))
+
+
 def test_suite():
     # pylint: disable-msg=C0111
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'ubuntuone/syncdaemon/filesystem_manager.py'
--- ubuntuone/syncdaemon/filesystem_manager.py	2009-11-20 22:00:25 +0000
+++ ubuntuone/syncdaemon/filesystem_manager.py	2009-11-26 14:55:22 +0000
@@ -203,6 +203,12 @@
       - idx_path: relationship path -> mdid
       - idx_node_id: relationship (share_id, node_id) -> mdid
     """
+
+    CONFLICT_SUFFIX = '.u1conflict'
+    CHANGED_LOCAL = 'LOCAL'
+    CHANGED_SERVER = 'SERVER'
+    CHANGED_NONE = 'NONE'
+
     def __init__(self, data_dir, partials_dir, vm):
         if not isinstance(data_dir, basestring):
             raise TypeError("data_dir should be a string instead of %s" % \
@@ -651,6 +657,33 @@
             del self._idx_node_id[(mdobj["share_id"], mdobj["node_id"])]
         del self.fs[mdid]
 
+    def _delete_dir_tree(self, path):
+        """Helper function to recursively remove a non-empty directory.
+
+        Removes the dir if there aren't local changes, and returns True.
+        If there are, return False.
+
+        Notifications are disabled for every sub-path within path.
+
+        """
+        dir_deleted = False
+        subtree = self.get_paths_starting_with(path)
+
+        for p, is_dir in subtree:
+            if self.changed(path=p) == self.CHANGED_LOCAL:
+                break
+        else:
+            # no local modification in the path tree
+            for p, is_dir in subtree:
+                filter_name = is_dir and "FS_DIR_DELETE" or "FS_FILE_DELETE"
+                self.eq.add_to_mute_filter(filter_name, p)
+                if p != path:
+                    self.delete_metadata(p)
+            shutil.rmtree(path)
+            dir_deleted = True
+
+        return dir_deleted
+
     def delete_file(self, path):
         """Deletes a file/dir and the metadata."""
         # adjust the metadata
@@ -670,9 +703,11 @@
                     os.remove(path)
         except OSError, e:
             if e.errno == errno.ENOTEMPTY:
-                raise
-            m = "OSError %s when trying to remove file/dir %r"
-            log_warning(m, e, path)
+                if not self._delete_dir_tree(path=path):
+                    raise
+            else:
+                m = "OSError %s when trying to remove file/dir %r"
+                log_warning(m, e, path)
         self.delete_metadata(path)
 
     def move_to_conflict(self, mdid):
@@ -680,7 +715,7 @@
         mdobj = self.fs[mdid]
         path = self.get_abspath(mdobj['share_id'], mdobj['path'])
         log_debug("move_to_conflict: path=%r mdid=%r" % (path, mdid))
-        base_to_path = to_path =  path + ".u1conflict"
+        base_to_path = to_path =  path + self.CONFLICT_SUFFIX
         ind = 0
         while os.path.exists(to_path):
             ind += 1
@@ -874,7 +909,20 @@
         raise TypeError("Incorrect arguments for 'has_metadata': %r" % kwargs)
 
     def changed(self, **kwargs):
-        """Return True if there's metadata for a given object."""
+        """Return whether a given node has changed or not.
+
+        The node can be defined by any of the following:
+         * path
+         * metadata's id (mdid)
+         * node_id and share_id
+
+        Return:
+         * LOCAL if the node has local modifications that the server is
+         not aware of.
+         * SERVER if the node is not fully downloaded.
+         * NONE the node has not changed.
+
+        """
         # get the values
         mdid = self._get_mdid_from_args(kwargs, "changed")
         mdobj = self.fs[mdid]
@@ -888,13 +936,26 @@
                 return "We broke the Universe! local_hash %r, server_hash %r,"\
                        " is_partial %r" % (local_hash, server_hash, is_partial)
             else:
-                return 'NONE'
+                return self.CHANGED_NONE
         else:
             if is_partial:
-                return 'SERVER'
+                return self.CHANGED_SERVER
             else:
-                return 'LOCAL'
-        return
+                return self.CHANGED_LOCAL
+
+    def local_changed(self, path):
+        """Return whether a given node have locally changed or not.
+
+        Return True if the node at `path' (or any of its children) has
+        been locally modified.
+
+        """
+        has_changed = False
+        for p, is_dir in self.get_paths_starting_with(path):
+            if self.changed(path=p) == self.CHANGED_LOCAL:
+                has_changed = True
+                break
+        return has_changed
 
     def dir_content(self, path):
         """Returns the content of the directory in a server-comparable way."""
@@ -958,11 +1019,15 @@
         share = self._get_share(share_id)
         return EnableShareWrite(share, path)
 
-    def get_paths_starting_with(self, base_path):
+    def get_paths_starting_with(self, base_path, include_base=True):
         """ return a list of paths that starts with: path. """
+        len_func = str.__len__ # avoid lookup
+        base_length = len_func(base_path)
+
         all_paths = []
         for path in self._idx_path:
-            if path.startswith(base_path):
+            candidate = include_base or len_func(path) > base_length
+            if candidate and path.startswith(base_path):
                 mdid = self._idx_path[path]
                 mdobj = self.fs[mdid]
                 all_paths.append((path, mdobj['is_dir']))