Merge lp:~nataliabidart/ubuntuone-control-panel/device-list-type-error into lp:ubuntuone-control-panel

[Natalia Bidart]

- Made the backend robust against possible None values (or any non basestring instance) sent from the API server (LP: #745790).

There is no good way of IRL test this since we would need to force a response from our API servers to send a null device name, for example.

If you insist, you should temporarly change the line 275 in backend.py and set something like:

local_device["name"] = None

and then run both the backend and UI from this branch.

[Eric Casteleijn (thisfred)]

Looks great!

review: Approve

[lp:~nataliabidart/ubuntuone-control-panel/device-list-type-error]

124. By Natalia Bidart on 2011-03-31

Typo.

[Martin Albisetti (beuno)]

review: Approve

=== modified file 'ubuntuone/controlpanel/backend.py'
--- ubuntuone/controlpanel/backend.py	2011-03-29 17:10:59 +0000
+++ ubuntuone/controlpanel/backend.py	2011-03-31 21:12:36 +0000
@@ -61,6 +61,16 @@
     return 'True' if value else ''
 
 
+def filter_field(info, field):
+    """Return a copy of 'info' where each item has 'field' hidden."""
+    result = []
+    for item in info:
+        item = item.copy()
+        item[field] = '<hidden>'
+        result.append(item)
+    return result
+
+
 class ControlBackend(object):
     """The control panel backend."""
 
@@ -178,6 +188,13 @@
             # di["available_services"] = ""
             # di["enabled_services"] = ""
 
+            # make a sanity check
+            for key, val in di.iteritems():
+                if not isinstance(val, basestring):
+                    logger.warning('_process_device_web_info: (key %r), '
+                                   'val %r is not a basestring.', key, val)
+                    di[key] = repr(val)
+
             if is_local:  # prepend the local device!
                 result.insert(0, di)
             else:
@@ -197,7 +214,6 @@
         dtype, did = self.type_n_id(device_id)
         local_token = yield self.get_token()
         is_local = (dtype == DEVICE_TYPE_COMPUTER and did == local_token)
-        logger.info('device_is_local: result %r, ', is_local)
         returnValue(is_local)
 
     @log_call(logger.debug)
@@ -238,6 +254,10 @@
             show_notifs = yield dbus_client.show_all_notifications_enabled()
             limits = yield dbus_client.get_throttling_limits()
 
+        logger.debug('devices_info: file sync enabled? %s limit_bw %s, limits '
+                     '%s, show_notifs %s',
+                     enabled, limit_bw, limits, show_notifs)
+
         try:
             devices = yield self.wc.call_api(DEVICES_API)
         except WebClientError:
@@ -247,6 +267,8 @@
                                                          limit_bw, limits,
                                                          show_notifs)
         if result is None:
+            logger.info('devices_info: result is None after calling '
+                        'devices/ API, building the local device.')
             credentials = yield dbus_client.get_credentials()
             local_device = {}
             local_device["type"] = DEVICE_TYPE_COMPUTER
@@ -264,6 +286,10 @@
                 local_device[UPLOAD_KEY] = str(upload)
                 local_device[DOWNLOAD_KEY] = str(download)
             result = [local_device]
+        else:
+            logger.info('devices_info: result is not None after calling '
+                        'devices/ API: %r',
+                        filter_field(result, field='device_id'))
 
         returnValue(result)
 
@@ -275,7 +301,7 @@
             return DEVICE_TYPE_PHONE, device_id[5:]
         return "No device", device_id
 
-    @log_call(logger.info)
+    @log_call(logger.info, with_args=False)
     @inlineCallbacks
     def change_device_settings(self, device_id, settings):
         """Change the settings for the given device."""
@@ -311,7 +337,7 @@
         # still pending: more work on the settings dict (LP: #673674)
         returnValue(device_id)
 
-    @log_call(logger.warning)
+    @log_call(logger.warning, with_args=False)
     @inlineCallbacks
     def remove_device(self, device_id):
         """Remove a device's tokens from the sso server."""
@@ -322,8 +348,8 @@
         yield self.wc.call_api(api)
 
         if is_local:
-            logger.warning('remove_device: device is local, id %r, '
-                           'clearing credentials.', device_id)
+            logger.warning('remove_device: device is local! removing and '
+                           'clearing credentials.')
             yield dbus_client.clear_credentials()
 
         returnValue(device_id)

=== modified file 'ubuntuone/controlpanel/dbus_client.py'
--- ubuntuone/controlpanel/dbus_client.py	2011-02-16 15:56:43 +0000
+++ ubuntuone/controlpanel/dbus_client.py	2011-03-31 21:12:36 +0000
@@ -64,7 +64,6 @@
 
     def found_credentials(app_name, creds):
         """Credentials have been found."""
-        logger.debug('credentials were found for app_name %r.', app_name)
         if app_name == APP_NAME:
             logger.info('credentials were found! (%r).', APP_NAME)
             d.callback(creds)

=== modified file 'ubuntuone/controlpanel/dbus_service.py'
--- ubuntuone/controlpanel/dbus_service.py	2011-03-28 17:25:21 +0000
+++ ubuntuone/controlpanel/dbus_service.py	2011-03-31 21:12:36 +0000
@@ -32,7 +32,8 @@
 from ubuntuone.controlpanel import (DBUS_BUS_NAME, DBUS_PREFERENCES_PATH,
     DBUS_PREFERENCES_IFACE)
 from ubuntuone.controlpanel.backend import (
-    ControlBackend, FILE_SYNC_DISABLED, FILE_SYNC_DISCONNECTED,
+    ControlBackend, filter_field,
+    FILE_SYNC_DISABLED, FILE_SYNC_DISCONNECTED,
     FILE_SYNC_ERROR, FILE_SYNC_IDLE, FILE_SYNC_STARTING, FILE_SYNC_STOPPED,
     FILE_SYNC_SYNCING,
     MSG_KEY, STATUS_KEY,
@@ -85,6 +86,7 @@
     """
     def inner(error, _=None):
         """Do the Failure transformation."""
+        logger.error('processing failure: %r', error.printTraceback())
         error_dict = error_handler(error)
         if _ is not None:
             result = f(_, error_dict)
@@ -149,10 +151,11 @@
         d.addCallback(self.DevicesInfoReady)
         d.addErrback(transform_failure(self.DevicesInfoError))
 
-    @log_call(logger.debug)
     @signal(dbus_interface=DBUS_PREFERENCES_IFACE, signature="aa{ss}")
     def DevicesInfoReady(self, info):
         """The info for the devices is available right now."""
+        logger.debug('DevicesInfoReady: args %r',
+                     filter_field(info, field='device_id'))
 
     @log_call(logger.error)
     @signal(dbus_interface=DBUS_PREFERENCES_IFACE, signature="a{ss}")
@@ -161,7 +164,7 @@
 
     #---
 
-    @log_call(logger.info)
+    @log_call(logger.info, with_args=False)
     @method(dbus_interface=DBUS_PREFERENCES_IFACE, in_signature="sa{ss}")
     def change_device_settings(self, device_id, settings):
         """Configure a given device."""
@@ -170,19 +173,19 @@
         d.addErrback(transform_failure(self.DeviceSettingsChangeError),
                      device_id)
 
-    @log_call(logger.info)
+    @log_call(logger.info, with_args=False)
     @signal(dbus_interface=DBUS_PREFERENCES_IFACE, signature="s")
     def DeviceSettingsChanged(self, device_id):
         """The settings for the device were changed."""
 
-    @log_call(logger.error)
+    @log_call(logger.error, with_args=False)
     @signal(dbus_interface=DBUS_PREFERENCES_IFACE, signature="sa{ss}")
     def DeviceSettingsChangeError(self, device_id, error):
         """Problem changing settings for the device."""
 
     #---
 
-    @log_call(logger.warning)
+    @log_call(logger.warning, with_args=False)
     @method(dbus_interface=DBUS_PREFERENCES_IFACE, in_signature="s")
     def remove_device(self, device_id):
         """Remove a given device."""
@@ -190,12 +193,12 @@
         d.addCallback(self.DeviceRemoved)
         d.addErrback(transform_failure(self.DeviceRemovalError), device_id)
 
-    @log_call(logger.warning)
+    @log_call(logger.warning, with_args=False)
     @signal(dbus_interface=DBUS_PREFERENCES_IFACE, signature="s")
     def DeviceRemoved(self, device_id):
         """The removal for the device was completed."""
 
-    @log_call(logger.error)
+    @log_call(logger.error, with_args=False)
     @signal(dbus_interface=DBUS_PREFERENCES_IFACE, signature="sa{ss}")
     def DeviceRemovalError(self, device_id, error):
         """Problem removing the device."""

=== modified file 'ubuntuone/controlpanel/tests/__init__.py'
--- ubuntuone/controlpanel/tests/__init__.py	2011-03-29 11:38:39 +0000
+++ ubuntuone/controlpanel/tests/__init__.py	2011-03-31 21:12:36 +0000
@@ -110,6 +110,16 @@
 ]
 """
 
+EMPTY_DESCRIPTION_JSON = """
+[
+    {
+        "token": "ABCDEF01234token",
+        "description": null,
+        "kind": "Computer"
+    }
+]
+"""
+
 LOCAL_DEVICE = {
     'is_local': 'True',
     'configurable': 'True',

=== modified file 'ubuntuone/controlpanel/tests/test_backend.py'
--- ubuntuone/controlpanel/tests/test_backend.py	2011-03-29 14:48:28 +0000
+++ ubuntuone/controlpanel/tests/test_backend.py	2011-03-31 21:12:36 +0000
@@ -30,6 +30,7 @@
 from ubuntuone.controlpanel import backend, replication_client
 from ubuntuone.controlpanel.backend import (bool_str,
     ACCOUNT_API, DEVICES_API, DEVICE_REMOVE_API, QUOTA_API,
+    DEVICE_TYPE_COMPUTER,
     FILE_SYNC_DISABLED,
     FILE_SYNC_DISCONNECTED,
     FILE_SYNC_ERROR,
@@ -41,6 +42,7 @@
     MSG_KEY, STATUS_KEY,
 )
 from ubuntuone.controlpanel.tests import (TestCase,
+    EMPTY_DESCRIPTION_JSON,
     EXPECTED_ACCOUNT_INFO,
     EXPECTED_ACCOUNT_INFO_WITH_CURRENT_PLAN,
     EXPECTED_DEVICES_INFO,
@@ -251,7 +253,7 @@
         self.patch(backend, "WebClient", MockWebClient)
         self.patch(backend, "dbus_client", MockDBusClient())
         self.patch(backend, "replication_client", MockReplicationClient())
-        self.local_token = "Computer" + TOKEN["token"]
+        self.local_token = DEVICE_TYPE_COMPUTER + TOKEN["token"]
         self.be = backend.ControlBackend()
 
         self.memento = MementoHandler()
@@ -380,9 +382,35 @@
         self.assertEqual(result, expected)
 
     @inlineCallbacks
+    def test_devices_info_when_token_name_is_empty(self):
+        """The devices_info can handle empty token names."""
+        # pylint: disable=E1101
+        self.be.wc.results[DEVICES_API] = EMPTY_DESCRIPTION_JSON
+        result = yield self.be.devices_info()
+        expected = {'configurable': '',
+                    'device_id': 'ComputerABCDEF01234token',
+                    'is_local': '', 'name': 'None',
+                    'type': DEVICE_TYPE_COMPUTER}
+        self.assertEqual(result, [expected])
+        self.assertTrue(self.memento.check_warning('name', 'None'))
+
+    @inlineCallbacks
+    def test_devices_info_does_not_log_device_id(self):
+        """The devices_info does not log the device_id."""
+        # pylint: disable=E1101
+        self.be.wc.results[DEVICES_API] = SAMPLE_DEVICES_JSON
+        yield self.be.devices_info()
+
+        dids = (d['device_id'] for d in EXPECTED_DEVICES_INFO)
+        device_id_logged = all(all(did not in r.getMessage()
+                                   for r in self.memento.records)
+                               for did in dids)
+        self.assertTrue(device_id_logged)
+
+    @inlineCallbacks
     def test_remove_device(self):
         """The remove_device method calls the right api."""
-        dtype, did = "Computer", "SAMPLE-TOKEN"
+        dtype, did = DEVICE_TYPE_COMPUTER, "SAMPLE-TOKEN"
         device_id = dtype + did
         apiurl = DEVICE_REMOVE_API % (dtype.lower(), did)
         # pylint: disable=E1101
@@ -411,6 +439,16 @@
                                  WebClientError)
 
     @inlineCallbacks
+    def test_remove_device_does_not_log_device_id(self):
+        """The remove_device does not log the device_id."""
+        device_id = DEVICE_TYPE_COMPUTER + TOKEN['token']
+        yield self.be.remove_device(device_id)
+
+        device_id_logged = all(device_id not in r.getMessage()
+                               for r in self.memento.records)
+        self.assertTrue(device_id_logged)
+
+    @inlineCallbacks
     def test_change_show_all_notifications(self):
         """The device settings are updated."""
         backend.dbus_client.show_all_notifications = False
@@ -465,6 +503,16 @@
         self.assertEqual(backend.dbus_client.limits["upload"], -1)
         self.assertEqual(backend.dbus_client.limits["download"], -1)
 
+    @inlineCallbacks
+    def test_changing_settings_does_not_log_device_id(self):
+        """The change_device_settings does not log the device_id."""
+        device_id = 'yadda-yadda'
+        yield self.be.change_device_settings(device_id, {})
+
+        device_id_logged = all(device_id not in r.getMessage()
+                               for r in self.memento.records)
+        self.assertTrue(device_id_logged)
+
 
 class BackendVolumesTestCase(BackendBasicTestCase):
     """Volumes tests for the backend."""

=== modified file 'ubuntuone/controlpanel/webclient.py'
--- ubuntuone/controlpanel/webclient.py	2010-12-18 16:21:36 +0000
+++ ubuntuone/controlpanel/webclient.py	2011-03-31 21:12:36 +0000
@@ -48,7 +48,7 @@
 
     def _handler(self, session, msg, d):
         """Handle the result of an http message."""
-        logger.debug("WebClient: got http response %d for uri %r",
+        logger.debug("got http response %d for uri %r",
                      msg.status_code, msg.get_uri().to_string(False))
         data = msg.response_body.data
         if msg.status_code == 200:
@@ -62,7 +62,7 @@
         """Get a given url from the webservice with credentials."""
         url = (self.base_url + api_name).encode('utf-8')
         method = "GET"
-        logger.debug("WebClient: getting url: %s, %s", method, url)
+        logger.debug("getting url: %s, %s", method, url)
         msg = Soup.Message.new(method, url)
         add_oauth_headers(msg.request_headers.append, method, url, credentials)
         d = defer.Deferred()
@@ -71,7 +71,8 @@
 
     def call_api(self, api_name):
         """Get a given url from the webservice."""
-        logger.debug("WebClient: calling api: %s", api_name)
+        # this may log device ID's, but only for removals, which is OK
+        logger.debug("calling api: %s", api_name)
         d = self.get_credentials()
         d.addCallback(self._call_api_with_creds, api_name)
         return d