Merge lp:~nataliabidart/ubuntuone-client/tree-deletion-when-prior-conflict into lp:ubuntuone-client

[Natalia Bidart]

Fix for Bug #462003: when there is a server-side tree deletion and the user already have conflicts in the removed directory, the directory itself is moved to conflict.

  Added test case and fix for Bug #462003 (when there is a server-side tree deletion and the user already have conflicts in the removed directory, the directory itself is moved to conflict).

  Added simple fix to pseudo-related Bug #494469.

[lp:~nataliabidart/ubuntuone-client/tree-deletion-when-prior-conflict]

430. By Natalia Bidart on 2010-03-15

Fixing lint issues.

431. By Natalia Bidart on 2010-03-15

Using MementoHandler to assert over logs.

432. By Natalia Bidart on 2010-03-15

Restoring previous code to avoid diffs.

[Roman Yepishev (rye)]

I believe this code will fail in case the whole dir is renamed to u1conflict. In this case the folder itself will not be checked and it will be removed along with all the files inside.

Based on pydoc:
dirnames is a list of the names of the subdirectories in dirpath (excluding '.' and '..').
filenames is a list of the names of the non-directory files in dirpath.

review: Needs Fixing

[lp:~nataliabidart/ubuntuone-client/tree-deletion-when-prior-conflict]

433. By Natalia Bidart on 2010-03-15

Adding test case and code to correctly detect conflicted directories and avoid
tree deletion.

[Natalia Bidart (nataliabidart)]

> I believe this code will fail in case the whole dir is renamed to u1conflict.
> In this case the folder itself will not be checked and it will be removed
> along with all the files inside.
>
> Based on pydoc:
> dirnames is a list of the names of the subdirectories in dirpath (excluding
> '.' and '..').
> filenames is a list of the names of the non-directory files in dirpath.

Good catch! I added a test case and the proper code for this case. Let me know what you think.

[Roman Yepishev (rye)]

Tested with folders and dirs, files and directories in local-only state do not get removed on parent tree removal. The parent folder (i.e. which is asked to be removed by the server) is renamed to u1conflict.

I like this :)

review: Approve

[Guillermo Gonzalez (verterok)]

I'm not sure that using os.walk is the "right thing to do"(tm) here.
Because if the directory tree is *big* this will consume a considerable amount of memory, what do you think about using os.listdir() on path and each subdir?

We need to measure if this is a real problem, or at least track this issue with a Bug.

Could you file a bug about this?

Thanks!

review: Approve

[Natalia Bidart (nataliabidart)]

> I'm not sure that using os.walk is the "right thing to do"(tm) here.
> Because if the directory tree is *big* this will consume a considerable amount
> of memory, what do you think about using os.listdir() on path and each subdir?
>
> We need to measure if this is a real problem, or at least track this issue
> with a Bug.
>
> Could you file a bug about this?
>
> Thanks!

I reported Bug #539659 regarding this issue.

=== modified file 'contrib/testing/testcase.py'
--- contrib/testing/testcase.py	2010-03-05 17:41:17 +0000
+++ contrib/testing/testcase.py	2010-03-15 23:15:33 +0000
@@ -616,6 +616,30 @@
         self.remove_from_connection()
 
 
+class FakeLogger(object):
+    """Helper logging class."""
+    def __init__(self):
+        self.logged = dict(debug=[], warning=[], info=[])
+
+    def _log(self, log, txt, args):
+        """Really logs."""
+        if args:
+            txt = txt % args
+        log.append(txt)
+
+    def warning(self, txt, *args):
+        """WARNING logs."""
+        self._log(self.logged['warning'], txt, args)
+
+    def debug(self, txt, *args):
+        """DEBUG logs."""
+        self._log(self.logged['debug'], txt, args)
+
+    def info(self, txt, *args):
+        """INFO logs."""
+        self._log(self.logged['info'], txt, args)
+
+
 class Listener(object):
     """Helper class to gather events."""
 

=== modified file 'tests/syncdaemon/test_fsm.py'
--- tests/syncdaemon/test_fsm.py	2010-02-10 17:35:26 +0000
+++ tests/syncdaemon/test_fsm.py	2010-03-15 23:15:33 +0000
@@ -24,12 +24,11 @@
 import shutil
 import unittest
 import time
-import logging
 
 from contrib.testing.testcase import (
     FakeVolumeManager,
     FakeMain,
-    MementoHandler,
+    MementoHandler
 )
 
 from ubuntuone.syncdaemon.filesystem_manager import (
@@ -39,7 +38,6 @@
     METADATA_VERSION,
 )
 from ubuntuone.syncdaemon.event_queue import EventQueue
-from ubuntuone.syncdaemon.logger import LOGFILENAME
 from ubuntuone.syncdaemon import logger
 from ubuntuone.syncdaemon.volume_manager import Share, allow_writes
 
@@ -74,11 +72,19 @@
                                             self.fsm, self.shares_dir)
         self.share_path = self.share.path
 
+        # add a in-memory logger handler
+        self.memento = MementoHandler()
+        self.memento.setLevel(0)
+        logger.root_logger.addHandler(self.memento)
+
     def tearDown(self):
         """ Clean up the tests. """
         self.eq.shutdown()
         self.rmtree(TESTS_DIR)
 
+        # remove the handler
+        logger.root_logger.removeHandler(self.memento)
+
     @staticmethod
     def rmtree(path):
         """rmtree that handle ro childs."""
@@ -1866,13 +1872,48 @@
         assert self.fsm.changed(path=local_file) == self.fsm.CHANGED_LOCAL
         self.assertRaises(OSError, self.fsm.delete_file, local_dir)
 
+    def test_delete_dir_when_non_empty_and_prior_conflict_on_file(self):
+        """Test that a dir is not deleted, when there is a conflicted file."""
+        # local directory
+        local_dir = os.path.join(self.root_dir, "foo")
+        os.mkdir(local_dir)
+        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" + self.fsm.CONFLICT_SUFFIX)
+        open(local_file, 'w').close() # touch bar.txt.u1conflict
+
+        assert not local_file in self.fsm._idx_path
+        self.assertRaises(OSError, self.fsm.delete_file, local_dir)
+
+        infos = [record.message for record in self.memento.records
+                 if record.levelname == 'INFO']
+        self.assertTrue(len(infos) == 1)
+        self.assertTrue(local_file in infos[0])
+
+    def test_delete_dir_when_non_empty_and_prior_conflict_on_subdir(self):
+        """Test that a dir is not deleted, when there is a conflicted dir."""
+        # local directory
+        local_dir = os.path.join(self.root_dir, "foo")
+        os.mkdir(local_dir)
+        self.fsm.create(local_dir, "", is_dir=True)
+        self.fsm.set_node_id(local_dir, "uuid")
+
+        local_subdir = os.path.join(local_dir,
+                                    "subdir_bar" + self.fsm.CONFLICT_SUFFIX)
+        os.mkdir(local_subdir)
+
+        assert not local_subdir in self.fsm._idx_path
+        self.assertRaises(OSError, self.fsm.delete_file, local_dir)
+
+        infos = [record.message for record in self.memento.records
+                 if record.levelname == 'INFO']
+        self.assertTrue(len(infos) == 1)
+        self.assertTrue(local_subdir in infos[0])
+
     def test_no_warning_on_log_file_when_recursive_delete(self):
         """Test that sucessfully deleted dir does not log OSError."""
-        # add a in-memory handler
-        memento = MementoHandler()
-        memento.setLevel(logging.WARNING)
-        logger.root_logger.addHandler(memento)
-
         local_dir = os.path.join(self.root_dir, "foo")
         os.mkdir(local_dir)
         self.fsm.create(local_dir, "", is_dir=True)
@@ -1883,20 +1924,15 @@
         self.fsm.create(local_file, "")
         self.fsm.set_node_id(local_file, "uuid_file")
 
+        previous = self.memento.records
         self.fsm.delete_file(local_dir)
 
-        # remove the handler
-        logger.root_logger.removeHandler(memento)
-        self.assertFalse(memento.records, memento.records)
+        # no logs were added
+        self.assertEquals(previous, self.memento.records)
 
-    # FIXME: Bug #494469
-    def SKIP_test_warning_on_log_file_when_failing_delete(self):
+    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 content till now
-
         local_dir = os.path.join(self.root_dir, "foo")
         self.fsm.create(local_dir, "", is_dir=True)
         self.fsm.set_node_id(local_dir, "uuid")
@@ -1904,11 +1940,11 @@
         # local_dir does not exist on the file system
         self.fsm.delete_file(local_dir)
 
-        log.flush()
-        log_content = log.read()
-        log.close()
-        log_present = 'OSError [Errno 2] No such file or directory' in log_content
-        self.assertTrue(log_present)
+        log_msg = 'OSError [Errno 2] No such file or directory'
+        warnings = [record.message for record in self.memento.records
+                    if record.levelname == 'WARNING']
+        self.assertTrue(len(warnings) == 1)
+        self.assertTrue(log_msg in warnings[0])
 
     def test_move_dir_to_conflict(self):
         """Test that the conflict to a dir removes children metadata."""

=== modified file 'tests/syncdaemon/test_states.py'
--- tests/syncdaemon/test_states.py	2010-03-11 15:29:45 +0000
+++ tests/syncdaemon/test_states.py	2010-03-15 23:15:33 +0000
@@ -21,6 +21,7 @@
 from twisted.internet import defer, reactor
 from twisted.trial.unittest import TestCase as TwistedTestCase
 
+from contrib.testing.testcase import FakeLogger
 from ubuntuone.syncdaemon.states import (
     StateManager, ConnectionManager, QueueManager, Node
 )
@@ -121,27 +122,6 @@
             return object.__getattribute__(self, name)
 
 
-
-class FakeLogger(object):
-    """Helper logging class."""
-    def __init__(self):
-        self.logged = dict(debug=[], warning=[])
-
-    def _log(self, log, txt, args):
-        """Really logs."""
-        if args:
-            txt = txt % args
-        log.append(txt)
-
-    def warning(self, txt, *args):
-        """WARNING logs."""
-        self._log(self.logged['warning'], txt, args)
-
-    def debug(self, txt, *args):
-        """DEBUG logs."""
-        self._log(self.logged['debug'], txt, args)
-
-
 class Base(TwistedTestCase):
     """Base class for state tests."""
     def setUp(self):

=== modified file 'ubuntuone/syncdaemon/filesystem_manager.py'
--- ubuntuone/syncdaemon/filesystem_manager.py	2010-02-11 19:13:30 +0000
+++ ubuntuone/syncdaemon/filesystem_manager.py	2010-03-15 23:15:33 +0000
@@ -682,22 +682,28 @@
         Notifications are disabled for every sub-path within path.
 
         """
-        dir_deleted = False
+        # check metadata to see if any node in LOCAL
         subtree = self.get_paths_starting_with(path, include_base=False)
-
         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)
-                self.delete_metadata(p)
-            shutil.rmtree(path)
-            dir_deleted = True
-
-        return dir_deleted
+                logger("Conflicting dir on remove because %r is local", p)
+                return False
+
+        # check disk searching for previous conflicts
+        for (dirpath, dirnames, filenames) in os.walk(path):
+            for fname in filenames + dirnames:
+                if fname.endswith(self.CONFLICT_SUFFIX):
+                    logger("Conflicting dir on remove because of previous "
+                           "conflict on: %r", os.path.join(dirpath, fname))
+                    return False
+
+        # all green
+        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)
+            self.delete_metadata(p)
+        shutil.rmtree(path)
+        return True
 
     def delete_file(self, path):
         """Deletes a file/dir and the metadata."""