Merge lp:~verterok/ubuntuone-client/fix-583412-stable into lp:ubuntuone-client/stable-1-2

[Guillermo Gonzalez]

Fix dbus_interface error handling in Folders.delete (and Shares.delete_share) when the volume_id is invalid.

Fix syncdaemon dbus interface when Folders.delete (or Shares.delete_share) is called with an invalid udf id, now the correct error signal is fired.

[Rick McBride (rmcbride)]

+1

review: Approve

[Stuart Colville (muffinresearch)]

tests pass - looks good.

review: Approve

=== modified file 'tests/syncdaemon/test_dbus.py'
--- tests/syncdaemon/test_dbus.py	2010-05-17 22:23:56 +0000
+++ tests/syncdaemon/test_dbus.py	2010-06-02 18:23:20 +0000
@@ -37,7 +37,12 @@
     DBUS_IFACE_PUBLIC_FILES_NAME,
     EventListener,
 )
-from ubuntuone.syncdaemon.volume_manager import Share, Shared, UDF
+from ubuntuone.syncdaemon.volume_manager import (
+    Share,
+    Shared,
+    UDF,
+    VolumeDoesNotExist,
+)
 from ubuntuone.syncdaemon.tools import DBusClient
 from ubuntuone.syncdaemon import event_queue, states, main, config
 from contrib.testing.testcase import (
@@ -2036,7 +2041,7 @@
             """FolderDeleted handler."""
             self.assertRaises(KeyError, self.main.fs.get_by_path,
                               info['path'].decode('utf-8'))
-            self.assertRaises(KeyError, self.main.vm.get_volume, info['volume_id'])
+            self.assertRaises(VolumeDoesNotExist, self.main.vm.get_volume, info['volume_id'])
             d.callback(True)
         match = self.bus.add_signal_receiver(deleted_handler,
                                              signal_name='FolderDeleted')
@@ -2045,7 +2050,7 @@
             """the callback"""
             self.assertNotIn(udf.volume_id, self.main.vm.udfs)
             self.assertRaises(KeyError, self.main.fs.get_by_path, udf.path)
-            self.assertRaises(KeyError, self.main.vm.get_volume, udf.volume_id)
+            self.assertRaises(VolumeDoesNotExist, self.main.vm.get_volume, udf.volume_id)
         self.folders_client.call_method('delete', udf.volume_id,
                                         reply_handler=check_deleted,
                                         error_handler=self.error_handler)
@@ -2078,6 +2083,24 @@
         return d
 
     @defer.inlineCallbacks
+    def test_delete_error_signal_folder_id(self):
+        """Test for FolderDeleteError for a volume that doesn't exists."""
+        udf_id = 'foobar'
+        d = defer.Deferred()
+        def deleted_error_handler(info, error):
+            """FolderDeleteError handler"""
+            d.callback((info, error))
+        match = self.bus.add_signal_receiver(deleted_error_handler,
+                                             signal_name='FolderDeleteError')
+        self.signal_receivers.add(match)
+        self.folders_client.call_method('delete', udf_id,
+                                        reply_handler=lambda *args: None,
+                                        error_handler=d.errback)
+        info, error = yield d
+        self.assertEquals(info['volume_id'], udf_id)
+        self.assertEquals(error, "DOES_NOT_EXIST")
+
+    @defer.inlineCallbacks
     def test_subscribe(self):
         """Test for Folders.subscribe and that it fires a dbus signal."""
         suggested_path = u'~/ñoño'
@@ -2182,7 +2205,7 @@
             """FolderDeleted handler."""
             self.assertRaises(KeyError, self.main.fs.get_by_path,
                               info['path'].decode('utf-8'))
-            self.assertRaises(KeyError, self.main.vm.get_volume, info['volume_id'])
+            self.assertRaises(VolumeDoesNotExist, self.main.vm.get_volume, info['volume_id'])
             d.callback(True)
         match = self.bus.add_signal_receiver(deleted_handler,
                                              signal_name='ShareDeleted')
@@ -2191,7 +2214,7 @@
             """the callback"""
             self.assertNotIn(share.volume_id, self.main.vm.shares)
             self.assertRaises(KeyError, self.main.fs.get_by_path, share.path)
-            self.assertRaises(KeyError, self.main.vm.get_volume, share.volume_id)
+            self.assertRaises(VolumeDoesNotExist, self.main.vm.get_volume, share.volume_id)
         client = DBusClient(self.bus, '/shares', DBUS_IFACE_SHARES_NAME)
         client.call_method('delete_share', share.volume_id,
                            reply_handler=check_deleted,

=== modified file 'tests/syncdaemon/test_vm.py'
--- tests/syncdaemon/test_vm.py	2010-05-25 21:00:09 +0000
+++ tests/syncdaemon/test_vm.py	2010-06-02 18:23:20 +0000
@@ -46,6 +46,7 @@
     LegacyShareFileShelf,
     MetadataUpgrader,
     VMFileShelf,
+    VolumeDoesNotExist,
 )
 from twisted.internet import defer, reactor
 
@@ -1376,6 +1377,13 @@
             self.vm.delete_volume(udf.volume_id)
         yield d
 
+    def test_handle_AQ_DELETE_VOLUME_ERROR_missing_volume(self):
+        """Test for handle_AQ_DELETE_VOLUME_ERROR for a missing volume."""
+        volume_id = 'foobar'
+        self.assertRaises(VolumeDoesNotExist,
+                          self.vm.handle_AQ_DELETE_VOLUME_ERROR,
+                          volume_id=volume_id, error="ERROR!")
+
     def _test_handle_SV_VOLUME_CREATED(self, auto_subscribe):
         """Test for handle_SV_VOLUME_CREATED."""
         user_conf = config.get_user_config()

=== modified file 'ubuntuone/syncdaemon/dbus_interface.py'
--- ubuntuone/syncdaemon/dbus_interface.py	2010-05-17 22:23:56 +0000
+++ ubuntuone/syncdaemon/dbus_interface.py	2010-06-02 18:23:20 +0000
@@ -32,7 +32,7 @@
 from ubuntuone.syncdaemon.event_queue import EVENTS
 from ubuntuone.syncdaemon.interfaces import IMarker
 from ubuntuone.syncdaemon import config
-from ubuntuone.syncdaemon.volume_manager import Share, UDF
+from ubuntuone.syncdaemon.volume_manager import Share, UDF, VolumeDoesNotExist
 
 
 # Disable the "Invalid Name" check here, as we have lots of DBus style names
@@ -669,9 +669,9 @@
         self.handle_default('VM_VOLUME_DELETE_ERROR', volume_id, error)
         try:
             volume = self.dbus_iface.volume_manager.get_volume(volume_id)
-        except KeyError:
+        except VolumeDoesNotExist:
             logger.error("Unable to handle VM_VOLUME_DELETE_ERROR for "
-                         "volume_id=%r", volume_id)
+                         "volume_id=%r, no such volume.", volume_id)
         else:
             if isinstance(volume, Share):
                 self.dbus_iface.shares.emit_share_delete_error(volume, error)
@@ -886,10 +886,12 @@
         try:
             self.vm.delete_volume(str(share_id))
             reply_handler()
-        except KeyError:
-            error_handler(ValueError("The volume with id: %s don't exists" % \
-                                     str(share_id)))
-
+        except VolumeDoesNotExist, e:
+            self.ShareDeleteError({'volume_id':share_id}, str(e))
+        except Exception, e:
+            error_handler(e)
+            logger.exception('Error while deleting share: %r', share_id)
+            self.ShareDeleteError({'volume_id':share_id}, str(e))
 
     @dbus.service.signal(DBUS_IFACE_SHARES_NAME,
                          signature='a{ss}')
@@ -1269,9 +1271,11 @@
         logger.debug('Folders.delete: %r', folder_id)
         try:
             self.vm.delete_volume(folder_id)
+        except VolumeDoesNotExist, e:
+            self.FolderDeleteError({'volume_id':folder_id}, str(e))
         except Exception, e:
             logger.exception('Error while deleting volume: %r', folder_id)
-            self.emit_folder_delete_error({'id':folder_id}, str(e))
+            self.FolderDeleteError({'volume_id':folder_id}, str(e))
 
     @dbus.service.method(DBUS_IFACE_FOLDERS_NAME, out_signature='aa{ss}')
     def get_folders(self):

=== modified file 'ubuntuone/syncdaemon/filesystem_manager.py'
--- ubuntuone/syncdaemon/filesystem_manager.py	2010-03-26 20:24:15 +0000
+++ ubuntuone/syncdaemon/filesystem_manager.py	2010-06-02 18:23:20 +0000
@@ -29,6 +29,7 @@
 import stat
 
 from ubuntuone.syncdaemon import file_shelf
+from ubuntuone.syncdaemon.volume_manager import VolumeDoesNotExist
 import uuid
 
 METADATA_VERSION = "4"
@@ -277,7 +278,7 @@
                 # check if the share exists
                 try:
                     self._get_share(mdobj["share_id"])
-                except KeyError:
+                except VolumeDoesNotExist:
                     # oops, the share is gone!, invalidate this mdid
                     log_warning('Share %r disappeared! deleting mdid: %s', mdobj['share_id'], mdid)
                     del self.fs[mdid]

=== modified file 'ubuntuone/syncdaemon/volume_manager.py'
--- ubuntuone/syncdaemon/volume_manager.py	2010-05-25 21:00:09 +0000
+++ ubuntuone/syncdaemon/volume_manager.py	2010-06-02 18:23:20 +0000
@@ -276,6 +276,20 @@
         return result
 
 
+class VolumeDoesNotExist(Exception):
+    """Exception for non existing volumes."""
+
+    msg = 'DOES_NOT_EXIST'
+
+    def __init__(self, volume_id):
+        """Create the instance."""
+        super(VolumeDoesNotExist, self).__init__(self.msg, volume_id)
+
+    def __str__(self):
+        """The error message."""
+        return self.msg
+
+
 class VolumeManager(object):
     """Manages shares and mount points."""
 
@@ -807,13 +821,13 @@
         del self.udfs[udf_id]
         self.m.event_q.push('VM_VOLUME_DELETED', udf)
 
-    def get_volume(self, id):
+    def get_volume(self, volume_id):
         """Returns the Share or UDF with the matching id."""
-        volume = self.shares.get(id, None)
-        if volume is None:
-            volume = self.udfs.get(id, None)
-        if volume is None:
-            raise KeyError(id)
+        volume = self.shares.get(volume_id, None)
+        if volume is None:
+            volume = self.udfs.get(volume_id, None)
+        if volume is None:
+            raise VolumeDoesNotExist(volume_id)
         return volume
 
     def get_volumes(self, all_volumes=False):
@@ -920,13 +934,8 @@
 
         """
         self.log.info('delete_volume: %r', volume_id)
-        try:
-            volume = self.get_volume(volume_id)
-        except KeyError:
-            self.m.event_q.push('VM_VOLUME_DELETE_ERROR', volume_id,
-                                "DOES_NOT_EXIST")
-        else:
-            self.m.action_q.delete_volume(volume.id)
+        volume = self.get_volume(volume_id)
+        self.m.action_q.delete_volume(volume.id)
 
     def subscribe_udf(self, udf_id):
         """Mark the UDF with id as subscribed.