Merge lp:~facundo/ubuntuone-client/fix-svfilenew-conflict into lp:ubuntuone-client

[Facundo Batista]

Support SV_FILE_NEW when having a node for that path (LP: #711389)

Support SV_FILE_NEW when having a node for that path

This is because _handle_SV_FILE_NEW is called when there's no match by node_id, but then it searches the node by path, and sometimes there is a node with that path.

Three cases arise:

- if the node still has no node_id: it's just waiting for it, so we go and set it.

- if the node has a node_id but other one: it's a case where the file was overwritten by a move operation; just delete the file and will get the server one.

- if the node has the same node_id: we broke something, because we shouldn't have called _handle_SV_FILE_NEW at all.

Tests included for everything.

[Lucio Torre (lucio.torre)]

review: Approve

[Guillermo Gonzalez (verterok)]

+1

review: Approve

=== modified file 'contrib/dump_metadata.py'
--- contrib/dump_metadata.py	2011-02-21 17:15:15 +0000
+++ contrib/dump_metadata.py	2011-04-04 17:52:08 +0000
@@ -97,10 +97,11 @@
 
     print "\nShowing trash:"
     something = False
-    for (vol_id, node_id), (mdid, parent_id, path) in fsm.trash.iteritems():
+    for (vol_id, node_id), (mdid, parent_id, path, is_dir) in \
+                                                        fsm.trash.iteritems():
         something = True
-        print "  mdid=%r  volume_id=%r  node_id=%r  parent_id=%r  path=%r" % (
-            mdid, share_id, node_id, parent_id, path)
+        print ("  mdid=%r  volume_id=%r  node_id=%r  parent_id=%r  path=%r "
+               "is_dir=%r" % (mdid, share_id, node_id, parent_id, path, is_dir))
     if not something:
         print "  (empty)"
 
 
=== modified file 'tests/syncdaemon/test_sync.py'
--- tests/syncdaemon/test_sync.py	2011-03-31 00:36:23 +0000
+++ tests/syncdaemon/test_sync.py	2011-04-04 17:52:08 +0000
@@ -255,6 +255,7 @@
         BaseSync.setUp(self)
         self.sync = Sync(main=self.main)
         self.fsm = self.main.fs
+        self.handler.setLevel(logging.DEBUG)
 
     def test_deleting_open_files_is_no_cause_for_despair(self):
         """test_deleting_open_files_is_no_cause_for_despair."""
@@ -621,6 +622,76 @@
                                        'new_parent_id', 'new_name', 'error')
         self.assertEqual(called[1:], ['AQ_MOVE_ERROR', {}, '', 'node_id'])
 
+    def test_SV_FILE_NEW_no_node(self):
+        """Handle SV_FILE_NEW not having a node."""
+        # fake method
+        called = []
+        orig = SyncStateMachineRunner.new_file
+        self.patch(SyncStateMachineRunner, 'new_file',
+                   lambda *a: called.extend(a) or orig(*a))
+
+        # create the parent
+        parentpath = os.path.join(self.root, 'somepath')
+        self.fsm.create(parentpath, '', node_id='parent_id')
+
+        # call and check
+        r = self.sync._handle_SV_FILE_NEW('', 'node_id', 'parent_id', 'name')
+        self.assertEqual(called[1:], ['SV_FILE_NEW', {}, '', 'node_id',
+                                      'parent_id', 'name'])
+        self.assertEqual(r.share_id, '')
+        self.assertEqual(r.node_id, 'node_id')
+        self.assertEqual(r.path, 'somepath/name')
+
+    def test_SV_FILE_NEW_node_no_id(self):
+        """Handle SV_FILE_NEW having a node without node_id."""
+        # fake method
+        called = []
+        orig = SyncStateMachineRunner.new_server_file_having_local
+        self.patch(SyncStateMachineRunner, 'new_server_file_having_local',
+                   lambda *a: called.extend(a) or orig(*a))
+
+        # create the node and its parent, to match per path
+        parentpath = os.path.join(self.root, 'somepath')
+        self.fsm.create(parentpath, '', node_id='parent_id')
+        childpath = os.path.join(parentpath, 'name')
+        self.fsm.create(childpath, '')
+
+        # call and check
+        self.sync._handle_SV_FILE_NEW('', 'node_id', 'parent_id', 'name')
+        self.assertEqual(called[1:], ['SV_FILE_NEW', {}, '', 'node_id',
+                                      'parent_id', 'name'])
+
+    def test_SV_FILE_NEW_node_same_id(self):
+        """Handle SV_FILE_NEW having a node with the same node_id."""
+        parentpath = os.path.join(self.root, 'somepath')
+        self.fsm.create(parentpath, '', node_id='parent_id')
+        childpath = os.path.join(parentpath, 'name')
+        self.fsm.create(childpath, '', node_id='node_id')
+        r = self.assertRaises(ValueError, self.sync._handle_SV_FILE_NEW,
+                              '', 'node_id', 'parent_id', 'name')
+        self.assertTrue("same node_id in handle_SV_FILE_NEW" in str(r))
+
+    def test_SV_FILE_NEW_node_different_id(self):
+        """Handle SV_FILE_NEW having a node with different node_id."""
+        # create the node and its parent, to match per path
+        parentpath = os.path.join(self.root, 'somepath')
+        self.fsm.create(parentpath, '', node_id='parent_id')
+        childpath = os.path.join(parentpath, 'name')
+        self.fsm.create(childpath, '', node_id='other_id')
+
+        # fake method
+        called = []
+        orig = self.fsm.delete_file
+        self.patch(self.fsm, 'delete_file',
+                   lambda path: called.append(path) or orig(path))
+
+        # call and check
+        self.sync._handle_SV_FILE_NEW('', 'node_id', 'parent_id', 'name')
+        self.assertEqual(called, [childpath])
+        self.handler.debug  = True
+        self.assertTrue(self.handler.check_debug("Wanted to apply SV_FILE_NEW",
+                                                 "found it with other id"))
+
 
 class SyncStateMachineRunnerTestCase(BaseSync):
     """Tests for the SyncStateMachineRunner."""
@@ -1115,6 +1186,24 @@
         # check the create and the make_dir
         self.assertEqual(called, [(new_path, '', 'node_id', True), 'mdid'])
 
+    def test_new_new_server_file_having_local(self):
+        """Set the node_id to the node."""
+        # create context
+        somepath = os.path.join(self.root, 'foo')
+        self.fsm.create(somepath, '')
+        key = FSKey(self.main.fs, path=somepath)
+        ssmr = SyncStateMachineRunner(fsm=self.fsm, main=self.main,
+                                      key=key, logger=None)
+
+        # log the call to fsm
+        called = []
+        self.fsm.set_node_id = lambda *a: called.append(a)
+
+        # call the tested method and check
+        ssmr.new_server_file_having_local('event', {}, '', 'node_id',
+                                          'parent_id', 'name')
+        self.assertEqual(called, [(somepath, 'node_id')])
+
 
 class FakedState(object):
     """A faked state."""
 
=== modified file 'ubuntuone/syncdaemon/sync.py'
--- ubuntuone/syncdaemon/sync.py	2011-03-31 00:36:23 +0000
+++ ubuntuone/syncdaemon/sync.py	2011-04-04 17:52:08 +0000
@@ -545,6 +545,11 @@
         self.m.action_q.uuid_map.set(marker, new_id)
         self.m.fs.set_node_id(self.key['path'], new_id)
 
+    def new_server_file_having_local(self, event, parms, share_id, node_id,
+                                     parent_id, name):
+        """Got new file from server, we have it local but without id yet."""
+        self.m.fs.set_node_id(self.key['path'], node_id)
+
     def new_local_dir(self, event, parms, path):
         """a new local dir was created"""
         parent_path = os.path.dirname(path)
@@ -838,9 +843,31 @@
         key = FSKey(self.m.fs, path=path)
         log = FileLogger(self.logger, key)
         ssmr = SyncStateMachineRunner(self.fsm, self.m, key, log)
+
+        # check if the node found by path has other node_id (which will
+        # cause a conflict); note that we don't get the node_id from the
+        # 'key' as it lies to us with the marker
+        try:
+            mdid = key.get_mdid()
+        except KeyError:
+            pass  # no md at all, didn't find any node by the path
+        else:
+            mdobj = self.m.fs.get_by_mdid(mdid)
+            if mdobj.node_id is not None:
+                if mdobj.node_id == node_id:
+                    raise ValueError("Found same node_id in handle_SV_FILE_NEW"
+                                     " (node_id=%s path=%r)" % (node_id, path))
+                # have metadata with *other* node_id
+                log.debug("Wanted to apply SV_FILE_NEW with node_id %s to node"
+                          "with path %r, but found it with other id: %s",
+                          node_id, path, mdobj.node_id)
+                key.delete_file()
+                return None
+
         ssmr.on_event("SV_FILE_NEW", {}, share_id, node_id, parent_id, name)
         self.m.event_q.push('SV_FILE_NEW', volume_id=share_id,
                             node_id=node_id, parent_id=parent_id, name=name)
+        return self.m.fs.get_by_node_id(share_id, node_id)
 
     def _handle_SV_DIR_NEW(self, share_id, node_id, parent_id, name):
         """on SV_DIR_NEW"""
@@ -852,6 +879,7 @@
         ssmr.on_event("SV_DIR_NEW", {}, share_id, node_id, parent_id, name)
         self.m.event_q.push('SV_DIR_NEW', volume_id=share_id,
                             node_id=node_id, parent_id=parent_id, name=name)
+        return self.m.fs.get_by_node_id(share_id, node_id)
 
     def _handle_SV_FILE_DELETED(self, share_id, node_id, is_dir):
         """on SV_FILE_DELETED. Not called by EQ anymore."""
@@ -1106,13 +1134,14 @@
                         continue
 
                     # node not there, we must create it
+                    args = (dt.share_id, dt.node_id, dt.parent_id, dt_name)
                     if is_dir:
-                        self._handle_SV_DIR_NEW(dt.share_id, dt.node_id,
-                                                dt.parent_id, dt_name)
+                        node = self._handle_SV_DIR_NEW(*args)
                     else:
-                        self._handle_SV_FILE_NEW(dt.share_id, dt.node_id,
-                                                 dt.parent_id, dt_name)
-                    node = self.m.fs.get_by_node_id(dt.share_id, dt.node_id)
+                        node = self._handle_SV_FILE_NEW(*args)
+                    if node is None:
+                        # the node was not created!
+                        continue
 
                 # if the delta is older than the node, skip!
                 if node.generation > dt.generation:
 
=== modified file 'ubuntuone/syncdaemon/u1fsfsm.ods'
 Binary files ubuntuone/syncdaemon/u1fsfsm.ods	2011-03-08 20:25:00 +0000 and ubuntuone/syncdaemon/u1fsfsm.ods	2011-04-04 17:52:08 +0000 differ
=== modified file 'ubuntuone/syncdaemon/u1fsfsm.py'
--- ubuntuone/syncdaemon/u1fsfsm.py	2010-09-07 20:49:38 +0000
+++ ubuntuone/syncdaemon/u1fsfsm.py	2011-04-04 17:52:08 +0000
@@ -300,32 +300,6 @@
                                 'STATE_OUT': {u'changed': u'=',
                                               u'has_metadata': u'=',
                                               u'is_directory': u'='}}],
-            u'AQ_DOWNLOAD_CANCELLED': [{'ACTION': u'md.remove_partial(uuid);',
-                                        'ACTION_FUNC': u'nothing',
-                                        'COMMENTS': u'Download cancelled, remove the partial file',
-                                        'PARAMETERS': {u'hash_eq_local_hash': u'*',
-                                                       u'hash_eq_server_hash': u'*',
-                                                       u'not_authorized': u'NA',
-                                                       u'not_available': u'NA'},
-                                        'STATE': {u'changed': u'*',
-                                                  u'has_metadata': u'T',
-                                                  u'is_directory': u'*'},
-                                        'STATE_OUT': {u'changed': u'=',
-                                                      u'has_metadata': u'=',
-                                                      u'is_directory': u'='}},
-                                       {'ACTION': u'pass',
-                                        'ACTION_FUNC': u'nothing',
-                                        'COMMENTS': u'',
-                                        'PARAMETERS': {u'hash_eq_local_hash': u'*',
-                                                       u'hash_eq_server_hash': u'*',
-                                                       u'not_authorized': u'NA',
-                                                       u'not_available': u'NA'},
-                                        'STATE': {u'changed': u'*',
-                                                  u'has_metadata': u'F',
-                                                  u'is_directory': u'*'},
-                                        'STATE_OUT': {u'changed': u'=',
-                                                      u'has_metadata': u'=',
-                                                      u'is_directory': u'='}}],
             u'AQ_DOWNLOAD_DOES_NOT_EXIST': [{'ACTION': u'',
                                              'ACTION_FUNC': u'delete_file',
                                              'COMMENTS': u"Directory doesn't exist anymore, remove it",
@@ -1015,8 +989,8 @@
                                  'STATE_OUT': {u'changed': u'=',
                                                u'has_metadata': u'=',
                                                u'is_directory': u'='}},
-                                {'ACTION': u'md.set(mdid, server_uuid=server_uuid)\nPANIC',
-                                 'ACTION_FUNC': u'DESPAIR',
+                                {'ACTION': u"Node got node_id with a SV_FILE_NEW and now it's uploading something",
+                                 'ACTION_FUNC': u'nothing',
                                  'COMMENTS': u'',
                                  'PARAMETERS': {u'hash_eq_local_hash': u'NA',
                                                 u'hash_eq_server_hash': u'NA',
@@ -1025,8 +999,8 @@
                                  'STATE': {u'changed': u'SERVER',
                                            u'has_metadata': u'T',
                                            u'is_directory': u'F'},
-                                 'STATE_OUT': {u'changed': u'NONE',
-                                               u'has_metadata': u'T',
+                                 'STATE_OUT': {u'changed': u'=',
+                                               u'has_metadata': u'=',
                                                u'is_directory': u'='}},
                                 {'ACTION': u'aq.uuid_map.set(marker, new_id)',
                                  'ACTION_FUNC': u'release_marker_ok',
@@ -2211,8 +2185,8 @@
                               'STATE_OUT': {u'changed': u'NONE',
                                             u'has_metadata': u'T',
                                             u'is_directory': u'F'}},
-                             {'ACTION': u'CONFLICT',
-                              'ACTION_FUNC': u'new_file_on_server_with_local',
+                             {'ACTION': u"Didn't find the node by node_id, but found it by path",
+                              'ACTION_FUNC': u'new_server_file_having_local',
                               'COMMENTS': u'',
                               'PARAMETERS': {u'hash_eq_local_hash': u'NA',
                                              u'hash_eq_server_hash': u'NA',
@@ -2221,9 +2195,9 @@
                               'STATE': {u'changed': u'*',
                                         u'has_metadata': u'T',
                                         u'is_directory': u'*'},
-                              'STATE_OUT': {u'changed': u'NONE',
-                                            u'has_metadata': u'T',
-                                            u'is_directory': u'F'}}],
+                              'STATE_OUT': {u'changed': u'=',
+                                            u'has_metadata': u'=',
+                                            u'is_directory': u'='}}],
             u'SV_HASH_NEW': [{'ACTION': u'NA',
                               'ACTION_FUNC': u'',
                               'COMMENTS': u'',