Merge lp:~nataliabidart/ubuntuone-storage-protocol/map-those-errors into lp:ubuntuone-storage-protocol

[Natalia Bidart]

Added an internal mapping to correctly raise specific exceptions when an ERROR was received in a Request.

Added an internal mapping to correctly raise specific exceptions when an ERROR was received in a Request.

[dobey (dobey)]

Can you clarify the pylintrc change by also adding comments as per the other values for disable-msg= above the disable-msg= line, or revert the change, using localized disable-msg comments instead if necessary? I'd like to avoid globally disabling messages when possible, as doing so may mask warnings that we should fix.

And can you use the following coding annotation instead, so that it's handled better by different editors?
# -*- coding: utf-8 -*-

review: Needs Fixing

[Natalia Bidart (nataliabidart)]

> Can you clarify the pylintrc change by also adding comments as per the other
> values for disable-msg= above the disable-msg= line, or revert the change,
> using localized disable-msg comments instead if necessary? I'd like to avoid
> globally disabling messages when possible, as doing so may mask warnings that
> we should fix.
>
> And can you use the following coding annotation instead, so that it's handled
> better by different editors?
> # -*- coding: utf-8 -*-

All changes added in revision 98.

[lp:~nataliabidart/ubuntuone-storage-protocol/map-those-errors]

98. By Natalia Bidart on 2010-04-01

Adding details to pylint disable-msg. Unified coding defs.

[dobey (dobey)]

review: Approve

[John O'Brien (jdobrien)]

review: Approve

=== modified file 'pylintrc'
--- pylintrc	2009-03-26 15:47:10 +0000
+++ pylintrc	2010-04-01 18:11:31 +0000
@@ -24,7 +24,9 @@
 # :W0613: *Unused argument %r* (We get lots of these from interfaces)
 # :W0621: *Redefining name %r from outer scope (line %s)* (pylint does a poor evaluation)
 # :W0622: *Redefining built-in '%r'
-disable-msg=E0101,W0142,W0221,W0613,W0621,W0622
+# :W0201: *Attribute '%r' defined outside __init__*
+# :W0212: *Access to a protected member %r of a client class*
+disable-msg=E0101,W0142,W0221,W0613,W0621,W0622,W0201,W0212
 
 
 [REPORTS]

=== modified file 'tests/test_client.py'
--- tests/test_client.py	2010-01-04 18:21:24 +0000
+++ tests/test_client.py	2010-04-01 18:11:31 +0000
@@ -1,4 +1,4 @@
-# coding=utf-8
+# -*- coding: utf-8 -*-
 #
 # Author: Natalia B. Bidart <[e-mail]>
 #

=== added file 'tests/test_errors.py'
--- tests/test_errors.py	1970-01-01 00:00:00 +0000
+++ tests/test_errors.py	2010-04-01 18:11:31 +0000
@@ -0,0 +1,51 @@
+# -*- coding: utf-8 -*-
+#
+# Author: Natalia B. Bidart <[e-mail]>
+#
+# Copyright (C) 2010 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU Affero General Public License version 3,
+# as published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+"""Tests for errors module."""
+
+import unittest
+
+from ubuntuone.storageprotocol import errors, protocol_pb2
+
+REQ_ARGS = dict(request=None, message=protocol_pb2.Message())
+
+HIGH_LEVEL_ERRORS = {errors.StorageProtocolErrorSizeTooBig: dict(),
+                     errors.StorageProtocolProtocolError: dict(),
+                     errors.StorageRequestError: REQ_ARGS,
+                     errors.RequestCancelledError: dict()}
+
+
+class ErrorsTestCase(unittest.TestCase):
+    """Basic testing of errors mapping."""
+
+    def setUp(self):
+        """Init."""
+
+    def test_exceptions_are_storage_protocol_error(self):
+        """High level exceptions inherit from StorageProtocolError."""
+        for e, args in HIGH_LEVEL_ERRORS.iteritems():
+            self.assertTrue(isinstance(e(**args), errors.StorageProtocolError),
+                            "%r must inherit from StorageProtocolError" % e)
+
+    def test_mapping(self):
+        """Protocol's specific exceptions are correct."""
+        for code_error, proto_error in errors._error_mapping.iteritems():
+            self.assertTrue(isinstance(proto_error(**REQ_ARGS),
+                            errors.StorageRequestError),
+                            "%r must inherit from StorageRequestError" %
+                             proto_error)
+            self.assertEqual(proto_error, errors.error_to_exception(code_error))

=== modified file 'tests/test_request.py'
--- tests/test_request.py	2009-06-26 16:23:19 +0000
+++ tests/test_request.py	2010-04-01 18:11:31 +0000
@@ -2,16 +2,17 @@
 #     request class tests
 #
 # Author: Tim Cole <[e-mail]>
+# Author: Natalia Bidart <[e-mail]>
 #
 # Copyright (C) 2009 Canonical
 #
-# This program is free software: you can redistribute it and/or modify it 
+# This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU Affero General Public License version 3,
 # as published by the Free Software Foundation.
 #
-# This program is distributed in the hope that it will be useful, but 
-# WITHOUT ANY WARRANTY; without even the implied warranties of 
-# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR 
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
 # PURPOSE.  See the GNU Affero General Public License for more details.
 #
 # You should have received a copy of the GNU Affero General Public License
@@ -20,9 +21,13 @@
 
 from __future__ import with_statement
 
-from twisted.trial.unittest import TestCase
+import unittest
+
+from twisted.trial.unittest import TestCase as TwistedTestCase
 from twisted.internet.defer import inlineCallbacks
 from twisted.python.failure import Failure
+
+from ubuntuone.storageprotocol import errors, protocol_pb2
 from ubuntuone.storageprotocol.request import (
     RequestHandler, Request)
 
@@ -50,7 +55,7 @@
         self.producer = None
 
 
-class TestRequests(TestCase):
+class TestRequests(TwistedTestCase):
     """Tests for request handling."""
 
     @inlineCallbacks
@@ -74,3 +79,51 @@
             pass # passed
         else:
             self.fail("Expected to fail with the correct reason.")
+
+
+class TestRequest(unittest.TestCase):
+    """Tests for request object."""
+
+    def setUp(self):
+        """Init."""
+        self.request = MindlessRequest(protocol=None)
+        self.error = None
+        self.request.error = lambda error: setattr(self, 'error', error)
+
+    def tearDown(self):
+        """Clean up."""
+        self.error = None
+        self.request = None
+
+    def test_default_process_message_basic(self):
+        """_default_process_message maps errors to exceptions."""
+        message = protocol_pb2.Message()
+        self.request._default_process_message(message)
+
+        self.assertTrue(isinstance(self.error, errors.StorageRequestError))
+        self.assertEqual(self.request, self.error.request)
+        self.assertEqual(message, self.error.error_message)
+
+    def test_default_process_message_no_error_message(self):
+        """_default_process_message maps errors to exceptions."""
+        message = protocol_pb2.Message()
+        self.request._default_process_message(message)
+
+        self.assertTrue(self.error.__class__ is errors.StorageRequestError)
+        self.assertEqual(self.request, self.error.request)
+        self.assertEqual(message, self.error.error_message)
+
+    def test_default_process_message(self):
+        """_default_process_message maps errors to exceptions."""
+        for code_error, proto_error in errors._error_mapping.iteritems():
+            message = protocol_pb2.Message()
+            message.type = protocol_pb2.Message.ERROR
+            message.error.type = code_error
+            self.request._default_process_message(message)
+
+            self.assertTrue(isinstance(self.error, proto_error),
+                            "must be an instance of %r" % proto_error)
+            self.assertEqual(self.request, self.error.request)
+            self.assertEqual(message, self.error.error_message)
+
+            self.error = None

=== modified file 'tests/test_volumes.py'
--- tests/test_volumes.py	2009-12-23 16:29:02 +0000
+++ tests/test_volumes.py	2010-04-01 18:11:31 +0000
@@ -1,4 +1,4 @@
-# coding=utf-8
+# -*- coding: utf-8 -*-
 #
 # Author: Natalia B. Bidart <[e-mail]>
 #

=== modified file 'ubuntuone/storageprotocol/client.py'
--- ubuntuone/storageprotocol/client.py	2010-03-12 20:32:23 +0000
+++ ubuntuone/storageprotocol/client.py	2010-04-01 18:11:31 +0000
@@ -569,14 +569,12 @@
             if self.callback is None:
                 self.data = "".join(self.parts)
             self.done()
-        elif message.type == protocol_pb2.Message.ERROR:
-            self.error(request.StorageRequestError(self, message))
         elif message.type == protocol_pb2.Message.OK:
             self.done()
         elif message.type == protocol_pb2.Message.CANCELLED:
             self.error(request.RequestCancelledError("CANCELLED"))
         else:
-            self.error(request.StorageProtocolProtocolError(message))
+            self._default_process_message(message)
 
     def _cancel(self):
         """Cancel the current download."""
@@ -609,7 +607,7 @@
         elif message.type == protocol_pb2.Message.SHARES_END:
             self.done()
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
 
 
 class CreateShare(request.Request):
@@ -666,7 +664,7 @@
             # XXX: this is for PROTOCOL_VERSION=1 backward compatibility
             self.done()
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
 
 
 class AcceptShare(request.Request):
@@ -711,7 +709,7 @@
         if message.type == protocol_pb2.Message.OK:
             self.done()
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
 
 
 class DeleteShare(request.Request):
@@ -739,7 +737,7 @@
         if message.type == protocol_pb2.Message.OK:
             self.done()
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
 
 
 class CreateUDF(request.Request):
@@ -774,7 +772,7 @@
             self.node_id = message.volume_created.udf.node
             self.done()
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
 
 
 class ListVolumes(request.Request):
@@ -816,7 +814,7 @@
         elif message.type == protocol_pb2.Message.VOLUMES_END:
             self.done()
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
 
 
 class DeleteVolume(request.Request):
@@ -844,7 +842,7 @@
         if message.type == protocol_pb2.Message.OK:
             self.done()
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
 
 
 class Unlink(request.Request):
@@ -875,7 +873,7 @@
         if message.type == protocol_pb2.Message.OK:
             self.done()
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
 
 
 class Move(request.Request):
@@ -914,10 +912,8 @@
         if message.type == protocol_pb2.Message.OK:
             self.done()
         else:
-            # XXX:
-            # lucio.torre
-            # handle more error messages
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
+
 
 class MultiQuery(object):
     """Create a Request-like object that encapsulates many Query requests
@@ -954,6 +950,7 @@
         for q in self.queries:
             q.start()
 
+
 class Query(request.Request):
     """Query about the hash of a node_id.
 
@@ -1011,10 +1008,7 @@
         elif message.type == protocol_pb2.Message.QUERY_END:
             self.done()
         else:
-            # XXX:
-            # lucio.torre
-            # handle more error messages
-            self.error(message)
+            self._default_process_message(message)
 
 
 class BytesMessageProducer(object):
@@ -1122,7 +1116,7 @@
         elif message.type == protocol_pb2.Message.CANCELLED:
             self.error(request.RequestCancelledError("CANCELLED"))
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
 
     def _cancel(self):
         """Cancel the current upload."""
@@ -1183,7 +1177,8 @@
             self.new_name = message.new.name
             self.done()
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
+
 
 class MakeDir(MakeObject):
     """Extend MakeObject to make directories."""
@@ -1224,7 +1219,8 @@
             self.other_protocol_version = message.protocol.version
             self.done()
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
+
 
 class Authenticate(request.Request):
     """Request to authenticate the user."""
@@ -1253,12 +1249,8 @@
         """Handle messages."""
         if message.type == protocol_pb2.Message.AUTH_AUTHENTICATED:
             self.done()
-        elif message.type == protocol_pb2.Message.ERROR:
-            # as the error travels with the exception, we send all here
-            self.error(request.StorageRequestError(self, message))
         else:
-            self.error(request.StorageProtocolError(
-                        "Authentication Error:"+str(message)))
+            self._default_process_message(message)
 
 
 class QuerySetCaps(request.Request):
@@ -1304,7 +1296,7 @@
             self.redirect_srvrecord = message.accept_caps.redirect_srvrecord
             self.done()
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
 
 
 class FreeSpaceInquiry(request.Request):
@@ -1330,7 +1322,7 @@
             self.free_bytes = message.free_space_info.free_bytes
             self.done()
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
 
 
 class AccountInquiry(request.Request):
@@ -1350,7 +1342,7 @@
             self.purchased_bytes = message.account_info.purchased_bytes
             self.done()
         else:
-            self.error(request.StorageRequestError(self, message))
+            self._default_process_message(message)
 
 
 class ThrottlingStorageClient(StorageClient):

=== added file 'ubuntuone/storageprotocol/errors.py'
--- ubuntuone/storageprotocol/errors.py	1970-01-01 00:00:00 +0000
+++ ubuntuone/storageprotocol/errors.py	2010-04-01 18:11:31 +0000
@@ -0,0 +1,156 @@
+# -*- coding: utf-8 -*-
+#
+# Author: Natalia B. Bidart <[e-mail]>
+#
+# Copyright 2010 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU Affero General Public License version 3,
+# as published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+"""The errors abstraction."""
+
+from ubuntuone.storageprotocol import protocol_pb2
+
+
+class StorageProtocolError(Exception):
+    """Base class for all client/server exceptions."""
+
+
+class StorageProtocolErrorSizeTooBig(StorageProtocolError):
+    """The size we received was too big."""
+
+
+class StorageProtocolProtocolError(StorageProtocolError):
+    """A protocol error on the storage protocol."""
+
+
+class StorageRequestError(StorageProtocolError):
+    """An exception that keeps the request that generated it around."""
+
+    def __init__(self, request, message):
+        """Create a StorageRequestError.
+
+        @param request: the request that generated this error.
+        @param message: the message received that generated the error.
+        """
+        error_name = protocol_pb2.Error.DESCRIPTOR \
+                 .enum_types_by_name['ErrorType'] \
+                 .values_by_number[message.error.type].name
+        super(StorageRequestError, self).__init__(error_name)
+        #: the request that generated the error
+        self.request = request
+        #: the message received that generated the error
+        self.error_message = message
+
+
+class RequestCancelledError(StorageProtocolError):
+    """The request was cancelled."""
+
+
+# Request specific errors
+
+
+class UnsupportedVersionError(StorageRequestError):
+    """The version is not supported."""
+
+
+class AuthenticationFailedError(StorageRequestError):
+    """The authencation failed."""
+
+
+class InternalError(StorageRequestError):
+    """There was an internal error on the other side."""
+
+
+class AuthenticationRequiredError(StorageRequestError):
+    """The authentication is required and hasn't been established yet."""
+
+
+class NoPermissionError(StorageRequestError):
+    """Current permissions are not the required ones."""
+
+
+class AlreadyExistsError(StorageRequestError):
+    """The node already exists."""
+
+
+class DoesNotExistError(StorageRequestError):
+    """The node does not exists."""
+
+
+class NotADirectoryError(StorageRequestError):
+    """The node is not a directory."""
+
+
+class NotEmptyError(StorageRequestError):
+    """The node is not empty."""
+
+
+class NotAvailableError(StorageRequestError):
+    """The node is not available."""
+
+
+class UploadInProgressError(StorageRequestError):
+    """There is already an upload in progress."""
+
+
+class UploadCorruptError(StorageRequestError):
+    """The upload is corrupted."""
+
+
+class UploadCanceledError(StorageRequestError):
+    """There upload was canceled."""
+
+
+class ConflictError(StorageRequestError):
+    """The was a conflict."""
+
+
+class TryAgainError(StorageRequestError):
+    """Server answered to try again."""
+
+
+class ProtocolError(StorageRequestError):
+    """There was a protocol error."""
+
+
+class QuotaExceededError(StorageRequestError):
+    """The quota was exceeded."""
+
+
+class InvalidFilenameError(StorageRequestError):
+    """The filename is invalid."""
+
+
+_error_mapping = {
+      protocol_pb2.Error.UNSUPPORTED_VERSION: UnsupportedVersionError,
+      protocol_pb2.Error.AUTHENTICATION_FAILED: AuthenticationFailedError,
+      protocol_pb2.Error.INTERNAL_ERROR: InternalError,
+      protocol_pb2.Error.AUTHENTICATION_REQUIRED: AuthenticationRequiredError,
+      protocol_pb2.Error.NO_PERMISSION: NoPermissionError,
+      protocol_pb2.Error.ALREADY_EXISTS: AlreadyExistsError,
+      protocol_pb2.Error.DOES_NOT_EXIST: DoesNotExistError,
+      protocol_pb2.Error.NOT_A_DIRECTORY: NotADirectoryError,
+      protocol_pb2.Error.NOT_EMPTY: NotEmptyError,
+      protocol_pb2.Error.NOT_AVAILABLE: NotAvailableError,
+      protocol_pb2.Error.UPLOAD_IN_PROGRESS: UploadInProgressError,
+      protocol_pb2.Error.UPLOAD_CORRUPT: UploadCorruptError,
+      protocol_pb2.Error.UPLOAD_CANCELED: UploadCanceledError,
+      protocol_pb2.Error.CONFLICT: ConflictError,
+      protocol_pb2.Error.TRY_AGAIN: TryAgainError,
+      protocol_pb2.Error.PROTOCOL_ERROR: ProtocolError,
+      protocol_pb2.Error.QUOTA_EXCEEDED: QuotaExceededError,
+      protocol_pb2.Error.INVALID_FILENAME: InvalidFilenameError,
+}
+
+def error_to_exception(error_code):
+    """Map protocol errors to specific exceptions."""
+    return _error_mapping[error_code]

=== modified file 'ubuntuone/storageprotocol/request.py'
--- ubuntuone/storageprotocol/request.py	2010-01-04 18:21:24 +0000
+++ ubuntuone/storageprotocol/request.py	2010-04-01 18:11:31 +0000
@@ -36,6 +36,12 @@
 from zope.interface import implements
 
 from ubuntuone.storageprotocol import protocol_pb2, validators
+from ubuntuone.storageprotocol.errors import (
+    StorageProtocolError, StorageProtocolErrorSizeTooBig,
+    StorageProtocolProtocolError, StorageRequestError,
+    RequestCancelledError, error_to_exception
+)
+
 
 # the max possible packet size is 2**32 (32 bits for size)
 # although we will not allow packets that big for now
@@ -53,41 +59,6 @@
 # the referred is the own root node, and not any of the shares
 ROOT = ''
 
-class StorageProtocolError(Exception):
-    """base class for all client/server exceptions."""
-
-
-class StorageProtocolErrorSizeTooBig(StorageProtocolError):
-    """the size we received was too big."""
-
-
-class StorageProtocolProtocolError(StorageProtocolError):
-    """A protocol error on the storage protocol."""
-
-
-class StorageRequestError(StorageProtocolError):
-    """an exception that keeps the request that generated it around."""
-
-    def __init__(self, request, message):
-        """create a StorageRequestError.
-
-        @param request: the request that generated this error.
-        @param message: the message received that generated the error.
-        """
-        error_name = protocol_pb2.Error.DESCRIPTOR \
-                 .enum_types_by_name['ErrorType'] \
-                 .values_by_number[message.error.type].name
-        super(StorageRequestError, self).__init__(error_name)
-        #: the request that generated the error
-        self.request = request
-        #: the message received that generated the error
-        self.error_message = message
-
-
-class RequestCancelledError(Exception):
-    """The request is cancelled."""
-
-
 class RequestHandler(Protocol):
     """the base class for a network peer.
 
@@ -419,6 +390,14 @@
         """override this method to start the request."""
         raise NotImplementedError("request needs to do something")
 
+    def _default_process_message(self, message):
+        """Map ERROR message to a specific exception."""
+        if message.type == protocol_pb2.Message.ERROR:
+            error_class = error_to_exception(message.error.type)
+        else:
+            error_class = StorageRequestError
+        self.error(error_class(self, message))
+
     def processMessage(self, message):
         """handle an incoming message for this request. override this.
 

=== modified file 'ubuntuone/storageprotocol/volumes.py'
--- ubuntuone/storageprotocol/volumes.py	2009-12-22 19:25:50 +0000
+++ ubuntuone/storageprotocol/volumes.py	2010-04-01 18:11:31 +0000
@@ -1,3 +1,5 @@
+# -*- coding: utf-8 -*-
+#
 # Author: Natalia B. Bidart <[e-mail]>
 #
 # Copyright 2009 Canonical Ltd.