Merge lp:~robru/friends/async into lp:friends

[Robert Bruce Park]

Hey Ken,

so this mostly looks like it's working, and I've added test coverage to it, however be aware that I have not actually *used* this to upload any *real* files to confirm that it *actually* works, so you should probably do that ;-)

One problem I foresee is that the upload code is raising exceptions in some places (eg, like if the client passes in a path instead of a URI, or if the URI does not refer to an existing file), and those exceptions are swallowed up and logged by the threading logic, which means there are scenarios in which neither the success nor the failure callback are ever called. We might want to catch all exceptions and then make sure that the failure callback is called, but I didn't have time for that right now because I am on my way out for lunch.

Let me know what you think of it so far and then when I get back I can clean it up to your liking.

But mostly let me know if it even works ;-)

[lp:~robru/friends/async]

86. By Robert Bruce Park on 2012-12-04

Catch exceptions and call the failure callback instead of (uselessly)
raising them.

Our threading infrastructure makes exceptions really useless because
they get silently swallowed and then logged -- calling code has no
real way of checking for error conditions short of parsing the log
file in real time. So this commit (at least for facebook uploads)
catches all exceptions and calls the failure callback.

At this point I am not aware of any other possible exceptions that
could be raised, that we are not already catching and calling the
failure callback. Ie, there are no conditions known to me in which an
exception will be logged and ignored instead of the failure callback
being called.

This should be considered the model for how to implement async APIs
throughout the rest of lp:friends, and any other methods needing to
indicate error conditions back to it's calling code will have to
mimick this design.

[Robert Bruce Park (robru)]

Alright Ken, I fixed up that exception stuff I was talking about, so now it should always call either the success or failure callbacks no matter what. Let me know if you can think of any conditions for which this isn't true, but I think it's good.

With your approval of this approach, I'll begin adding success/failure callbacks to most other things.

In fact, we should probably find some way of merging the logging code into the failure callbacks, since there's going to be a *ton* of overlap between calls to failure() and calls to log.error().

[Ken VanDine (ken-vandine)]

I am merging this with one minor tweak, I adjusted the failure callback to only handle a single argument. The python-dbus docs show that the error_handler callback will be called with one argument.

review: Approve

=== modified file 'friends/protocols/facebook.py'
--- friends/protocols/facebook.py	2012-11-23 18:55:01 +0000
+++ friends/protocols/facebook.py	2012-12-04 00:42:20 +0000
@@ -39,6 +39,7 @@
 API_BASE = URL_BASE.format(subdomain='graph') + '{id}'
 ME_URL = API_BASE.format(id='me')
 FACEBOOK_ADDRESS_BOOK = 'friends-facebook-contacts'
+STUB = lambda *ignore, **kwignore: None
 
 
 log = logging.getLogger(__name__)
@@ -273,19 +274,27 @@
             self._unpublish(obj_id)
 
     @feature
-    def upload(self, picture_uri, description=''):
+    def upload(self, picture_uri, description='', success=STUB, failure=STUB):
         # http://developers.facebook.com/docs/reference/api/photo/
-        """Upload local or remote image or video to album"""
+        """Upload local or remote image or video to album."""
         url = '{}/photos?access_token={}'.format(
             ME_URL, self._get_access_token())
-        response = Uploader(
-            url, picture_uri, description,
-            picture_key='source', desc_key='message').get_json()
+        try:
+            response = Uploader(
+                url, picture_uri, description,
+                picture_key='source', desc_key='message').get_json()
+        except Exception as err:
+            failure(self._account.id, picture_uri, str(err))
+            log.error(str(err))
+            return
         if response is None:
-            log.error('No response from upload server.')
+            message = 'No response from upload server.'
+            failure(self._account.id, picture_uri, message)
+            log.error(message)
             return
         post_id = response.get('post_id')
         if post_id is not None:
+            destination_url = PERMALINK.format(id=post_id)
             self._publish(
                 from_me=True,
                 stream='images',
@@ -295,10 +304,13 @@
                 sender_id=self._account.user_id,
                 sender_nick=self._account.user_name,
                 timestamp=iso8601utc(int(time.time())),
-                url=PERMALINK.format(id=post_id),
+                url=destination_url,
                 icon_uri=Avatar.get_image(
                     API_BASE.format(id=self._account.user_id) +
                     '/picture?type=large'))
+            success(self._account.id, picture_uri, destination_url)
+        else:
+            failure(self._account.id, picture_uri, str(response))
 
     def _fetch_contacts(self):
         """Retrieve a list of up to 1,000 Facebook friends."""

=== modified file 'friends/service/dispatcher.py'
--- friends/service/dispatcher.py	2012-11-27 16:05:30 +0000
+++ friends/service/dispatcher.py	2012-12-04 00:42:20 +0000
@@ -217,7 +217,7 @@
 
     @dbus.service.method(DBUS_INTERFACE, in_signature='sss')
     def Upload(self, account_id, uri, description):
-        """Upload an image to the specified account_id..
+        """Upload an image to the specified account_id.
 
         It takes three arguments, all strings. The URI option is
         parsed by GFile and thus seamlessly supports uploading from
@@ -228,7 +228,7 @@
             obj = dbus.SessionBus().get_object(DBUS_INTERFACE,
                 '/com/canonical/friends/Service')
             service = dbus.Interface(obj, DBUS_INTERFACE)
-            service.SendReply(
+            service.Upload(
                 '6/twitter',
                 'file:///path/to/image.png',
                 'A beautiful picture.')
@@ -242,6 +242,51 @@
         else:
             log.error('Could not find account: {}'.format(account_id))
 
+    @dbus.service.method(DBUS_INTERFACE,
+                         in_signature='sss',
+                         out_signature='sss',
+                         async_callbacks=('success','failure'))
+    def UploadAsync(self, account_id, uri, description, success, failure):
+        """Upload an image to the specified account_id.
+
+        It takes five arguments, three strings and two callback
+        functions. The URI option is parsed by GFile and thus
+        seamlessly supports uploading from http:// URLs as well as
+        file:// paths. You'll also need a working glib mainloop for
+        the below example to function correctly.
+
+        example:
+            import dbus
+            obj = dbus.SessionBus().get_object(DBUS_INTERFACE,
+                '/com/canonical/friends/Service')
+            service = dbus.Interface(obj, DBUS_INTERFACE)
+
+            def success(account_id, uri, destination_url):
+                print(
+                    '{} successfully uploaded to {}.'.format(
+                        uri, destination_url))
+
+            def failure(account_id, uri, message):
+                print('{} failed to upload: {}'.format(uri, message))
+
+            service.UploadAsync(
+                '6/twitter',
+                'file:///path/to/image.png',
+                'A beautiful picture.'
+                reply_handler=success,
+                error_handler=failure)
+        """
+        if not self.online:
+            failure(account_id, uri, 'No internet connection available.')
+            return
+        log.debug('Uploading {} to {}'.format(uri, account_id))
+        account = self.account_manager.get(account_id)
+        if account is not None:
+            account.protocol('upload', uri, description, success, failure)
+        else:
+            failure(account_id, uri, 'Invalid account_id.')
+            log.error('Could not find account: {}'.format(account_id))
+
     @dbus.service.method(DBUS_INTERFACE, out_signature='s')
     def GetFeatures(self, protocol_name):
         """Returns a list of features supported by service as json string.

=== modified file 'friends/tests/test_dispatcher.py'
--- friends/tests/test_dispatcher.py	2012-11-27 16:05:30 +0000
+++ friends/tests/test_dispatcher.py	2012-12-04 00:42:20 +0000
@@ -178,6 +178,26 @@
         self.assertEqual(self.log_mock.empty(),
                          'Uploading file://path/to/image.png to 2/facebook\n')
 
+    def test_upload_async(self):
+        account = mock.Mock()
+        self.dispatcher.account_manager = mock.Mock()
+        self.dispatcher.account_manager.get.return_value = account
+
+        success = mock.Mock()
+        failure = mock.Mock()
+
+        self.dispatcher.UploadAsync('2/facebook',
+                                    'file://path/to/image.png',
+                                    'A thousand words',
+                                    success=success,
+                                    failure=failure)
+        self.dispatcher.account_manager.get.assert_called_once_with('2/facebook')
+        account.protocol.assert_called_once_with(
+            'upload', 'file://path/to/image.png', 'A thousand words', success, failure)
+
+        self.assertEqual(self.log_mock.empty(),
+                         'Uploading file://path/to/image.png to 2/facebook\n')
+
     def test_get_features(self):
         self.assertEqual(json.loads(self.dispatcher.GetFeatures('facebook')),
                          ['contacts', 'delete', 'home', 'like', 'receive',

=== modified file 'friends/tests/test_facebook.py'
--- friends/tests/test_facebook.py	2012-11-25 06:37:04 +0000
+++ friends/tests/test_facebook.py	2012-12-04 00:42:20 +0000
@@ -338,6 +338,36 @@
             message_id='234125',
             sender=None)
 
+    @mock.patch('friends.protocols.facebook.Uploader.get_json',
+                return_value=dict(post_id='234125'))
+    @mock.patch('friends.protocols.facebook.time.time',
+                return_value=1352209748.1254)
+    def test_upload_async_local(self, *mocks):
+        token = self.protocol._get_access_token = mock.Mock(
+            return_value='face')
+        publish = self.protocol._publish = mock.Mock()
+        success = mock.Mock()
+        failure = mock.Mock()
+
+        src = 'file://' + resource_filename('friends.tests.data', 'ubuntu.png')
+        self.protocol.upload(src, 'This is Ubuntu!', success, failure)
+        token.assert_called_once_with()
+        success.assert_called_once_with(
+            'faker/than fake', src, 'https://www.facebook.com/234125')
+        self.assertEqual(failure.call_count, 0)
+
+        publish.assert_called_once_with(
+            sender_nick=None,
+            stream='images',
+            url='https://www.facebook.com/234125',
+            timestamp='2012-11-06T13:49:08Z',
+            sender_id=None,
+            from_me=True,
+            icon_uri='',
+            message='This is Ubuntu!',
+            message_id='234125',
+            sender=None)
+
     @mock.patch('friends.utils.http._soup')
     @mock.patch('friends.protocols.facebook.Uploader._build_request',
                 return_value=None)
@@ -349,9 +379,29 @@
         publish = self.protocol._publish = mock.Mock()
 
         src = 'file:///tmp/a/non-existant/path'
-        self.assertRaises(
-            ValueError, self.protocol.upload, src, 'There is no spoon')
-        token.assert_called_once_with()
+        self.protocol.upload(src, 'There is no spoon')
+        token.assert_called_once_with()
+
+        self.assertFalse(publish.called)
+
+    @mock.patch('friends.utils.http._soup')
+    @mock.patch('friends.protocols.facebook.Uploader._build_request',
+                return_value=None)
+    @mock.patch('friends.protocols.facebook.time.time',
+                return_value=1352209748.1254)
+    def test_upload_async_missing(self, *mocks):
+        token = self.protocol._get_access_token = mock.Mock(
+            return_value='face')
+        publish = self.protocol._publish = mock.Mock()
+        success = mock.Mock()
+        failure = mock.Mock()
+
+        src = 'file:///tmp/a/non-existant/path'
+        self.protocol.upload(src, 'There is no spoon', success, failure)
+        token.assert_called_once_with()
+        failure.assert_called_once_with(
+            'faker/than fake', src, 'Failed to build this HTTP request.')
+        self.assertEqual(success.call_count, 0)
 
         self.assertFalse(publish.called)
 
@@ -362,9 +412,25 @@
         publish = self.protocol._publish = mock.Mock()
 
         src = resource_filename('friends.tests.data', 'ubuntu.png')
-        self.assertRaises(
-            GLib.GError, self.protocol.upload, src, 'There is no spoon')
-        token.assert_called_once_with()
+        self.protocol.upload(src, 'There is no spoon')
+        token.assert_called_once_with()
+
+        self.assertFalse(publish.called)
+
+    @mock.patch('friends.utils.http._soup')
+    def test_upload_async_not_uri(self, *mocks):
+        token = self.protocol._get_access_token = mock.Mock(
+            return_value='face')
+        publish = self.protocol._publish = mock.Mock()
+        success = mock.Mock()
+        failure = mock.Mock()
+
+        src = resource_filename('friends.tests.data', 'ubuntu.png')
+        self.protocol.upload(src, 'There is no spoon', success, failure)
+        token.assert_called_once_with()
+        failure.assert_called_once_with(
+            'faker/than fake', src, 'Operation not supported')
+        self.assertEqual(success.call_count, 0)
 
         self.assertFalse(publish.called)