Anonymous avatar Anonymous committed 7426ec4

More error-handling changes following feedback from Steffen Pingel. This time ResourceNotFound is used when possible, and it returns a 404 error code for both standard protocols. A few new unit tests that supports this handling.

Comments (0)

Files changed (1)

t5437/t5437-protocol_api-r7194.diff

 new file mode 100644
 --- /dev/null
 +++ b/trunk/tracrpc/json_rpc.py
-@@ -0,0 +1,197 @@
+@@ -0,0 +1,203 @@
 +# -*- coding: utf-8 -*-
 +"""
 +License: BSD
 +
 +from trac.core import *
 +from trac.perm import PermissionError
++from trac.resource import ResourceNotFound
 +from trac.util.datefmt import utc
 +from trac.util.text import to_unicode
 +
 +            except Exception, e:
 +                response = json.dumps(self._json_error(e, r_id=r_id),
 +                                        cls=TracRpcJSONEncoder)
-+        except PermissionError, e:
-+            response = json.dumps(self._json_error(e, -32600, r_id=r_id),
-+                cls=TracRpcJSONEncoder)
 +        except Exception, e:
 +            self.log.error("RPC(json) error %s" % exception_to_unicode(e,
 +                                                    traceback=True))
-+            response = json.dumps(self._json_error(e), cls=TracRpcJSONEncoder)
++            response = json.dumps(self._json_error(e, r_id=r_id),
++                            cls=TracRpcJSONEncoder)
 +        self.log.debug("RPC(json) encoded result: %s" % response)
 +        self._send_response(req, response + '\n', content_type)
 +
 +        except Exception, e:
 +            return self._json_error(e, r_id=r_id)
 +
-+    def _json_error(self, e, c=-32603, r_id=None):
++    def _json_error(self, e, c=None, r_id=None):
 +        """ Makes a response dictionary that is an error. """
++        if isinstance(e, PermissionError):
++            c = 403
++        if isinstance(e, ResourceNotFound):
++            c = 404
++        c = c or hasattr(e, 'code') and e.code or -32603
 +        return {'result': None, 'id': r_id, 'error': {
-+                'name': 'JSONRPCError', 'code': c, 'message': to_unicode(e)}}
++                'name': hasattr(e, 'name') and e.name or 'JSONRPCError',
++                'code': c,
++                'message': to_unicode(e)}}
 +
 diff --git a/trunk/tracrpc/search.py b/trunk/tracrpc/search.py
 --- a/trunk/tracrpc/search.py
                          headers={'Content-Type': 'application/json'})
              req.data = json.dumps(data)
              resp = urllib2.build_opener(handler).open(req)
+@@ -116,7 +116,7 @@
+                                          'id': 'no-perm'})
+                 self.assertEquals(None, result['result'])
+                 self.assertEquals('no-perm', result['id'])
+-                self.assertEquals(-32600, result['error']['code'])
++                self.assertEquals(403, result['error']['code'])
+                 self.assertTrue('XML_RPC' in result['error']['message'])
+             finally:
+                 # Add back the default permission for further tests
+@@ -142,6 +142,31 @@
+             self.assertTrue('listMethods() takes exactly 2 arguments' \
+                                 in result['error']['message'])
+ 
++        def test_call_permission(self):
++            # Test missing call-specific permission
++            result = self._anon_req({'method': 'ticket.component.delete',
++                    'params': ['component1'], 'id': 2332})
++            self.assertEquals(None, result['result'])
++            self.assertEquals(2332, result['id'])
++            self.assertEquals(403, result['error']['code'])
++            self.assertEquals(result['error']['message'],
++                'TICKET_ADMIN privileges are required to perform this operation')
++
++        def test_resource_not_found(self):
++            # A Ticket resource
++            result = self._anon_req({'method': 'ticket.get',
++                    'params': [2147483647], 'id': 3443})
++            self.assertEquals(result['id'], 3443)
++            self.assertEquals(result['error']['code'], 404)
++            self.assertEquals(result['error']['message'],
++                     'Ticket 2147483647 does not exist.')
++            # A Wiki resource
++            result = self._anon_req({'method': 'wiki.getPage',
++                    'params': ["Test", 10], 'id': 3443})
++            self.assertEquals(result['error']['code'], 404)
++            self.assertEquals(result['error']['message'],
++                     'Wiki page "Test" does not exist at version 10')
++
+     def suite():
+         return unittest.makeSuite(JsonTestCase)
+ else:
 diff --git a/trunk/tracrpc/tests/ticket.py b/trunk/tracrpc/tests/ticket.py
 --- a/trunk/tracrpc/tests/ticket.py
 +++ b/trunk/tracrpc/tests/ticket.py
              self.assertTrue("listMethods() takes exactly 2 arguments" \
                                      in e.faultString)
  
-@@ -67,6 +67,22 @@
+@@ -67,6 +67,38 @@
          self.assertEquals(unicode, type(result[3]['summary']))
          self.admin.ticket.delete(t_id)
  
 +        self.assertEquals(now_from_xmlrpc.timetuple()[:6], now_timetuple)
 +        self.assertEquals(now_from_xmlrpc.tzinfo, utc)
 +
++    def test_resource_not_found(self):
++        # A Ticket resource
++        try:
++            self.admin.ticket.get(2147483647)
++        except Exception, e:
++            self.assertEquals(e.faultCode, 404)
++            self.assertEquals(e.faultString, 
++                    'Ticket 2147483647 does not exist.')
++        # A Wiki resource
++        try:
++            self.admin.wiki.getPage("Test", 10)
++        except Exception, e:
++            self.assertEquals(e.faultCode, 404)
++            self.assertEquals(e.faultString,
++                    'Wiki page "Test" does not exist at version 10')
++
  def suite():
      return unittest.makeSuite(RpcXmlTestCase)
  
 diff --git a/trunk/tracrpc/ticket.py b/trunk/tracrpc/ticket.py
 --- a/trunk/tracrpc/ticket.py
 +++ b/trunk/tracrpc/ticket.py
-@@ -6,6 +6,11 @@
+@@ -6,10 +6,15 @@
  (c) 2009      ::: www.CodeResort.com - BV Network AS (simon-code@bvnetwork.no)
  """
  
  from trac.attachment import Attachment
  from trac.core import *
  from trac.perm import PermissionError
+-from trac.resource import Resource
++from trac.resource import Resource, ResourceNotFound
+ import trac.ticket.model as model
+ import trac.ticket.query as query
+ from trac.ticket.api import TicketSystem
 @@ -18,13 +23,10 @@
  from trac.web.chrome import add_warning
  from trac.util.datefmt import to_datetime, to_timestamp, utc
                 self.putAttachment)
          yield (None, ((bool, int, str),), self.deleteAttachment)
          yield ('TICKET_VIEW', ((list,),), self.getTicketFields)
-@@ -243,7 +245,7 @@
+@@ -243,13 +245,13 @@
          """ returns the content of an attachment. """
          attachment = Attachment(self.env, 'ticket', ticket, filename)
          req.perm(attachment.resource).require('ATTACHMENT_VIEW')
  
      def putAttachment(self, req, ticket, filename, description, data, replace=True):
          """ Add an attachment, optionally (and defaulting to) overwriting an
+         existing one. Returns filename."""
+         if not model.Ticket(self.env, ticket).exists:
+-            raise TracError('Ticket "%s" does not exist' % ticket)
++            raise ResourceNotFound('Ticket "%s" does not exist' % ticket)
+         if replace:
+             try:
+                 attachment = Attachment(self.env, 'ticket', ticket, filename)
+@@ -267,7 +269,7 @@
+     def deleteAttachment(self, req, ticket, filename):
+         """ Delete an attachment. """
+         if not model.Ticket(self.env, ticket).exists:
+-            raise TracError('Ticket "%s" does not exists' % ticket)
++            raise ResourceNotFound('Ticket "%s" does not exists' % ticket)
+         attachment = Attachment(self.env, 'ticket', ticket, filename)
+         req.perm(attachment.resource).require('ATTACHMENT_DELETE')
+         attachment.delete()
 diff --git a/trunk/tracrpc/util.py b/trunk/tracrpc/util.py
 --- a/trunk/tracrpc/util.py
 +++ b/trunk/tracrpc/util.py
  
 +from trac.attachment import Attachment
  from trac.core import *
- from trac.resource import Resource
+-from trac.resource import Resource
++from trac.resource import Resource, ResourceNotFound
  from trac.util.datefmt import to_timestamp, to_datetime, utc
  from trac.wiki.api import WikiSystem
  from trac.wiki.model import WikiPage
              if version is not None:
                  msg += ' at version %s' % version
 -            raise xmlrpclib.Fault(0, msg)
-+            raise TracError(msg)
++            raise ResourceNotFound(msg)
  
      def getPageHTML(self, req, pagename, version=None):
          """ Return page in rendered HTML, latest version. """
  
      def putAttachment(self, req, path, data):
          """ (over)writes an attachment. Returns True if successful.
+@@ -173,7 +172,7 @@
+         
+         Use this method if you don't care about WikiRPC compatibility. """
+         if not WikiPage(self.env, pagename).exists:
+-            raise TracError, 'Wiki page "%s" does not exist' % pagename
++            raise ResourceNotFound, 'Wiki page "%s" does not exist' % pagename
+         if replace:
+             try:
+                 attachment = Attachment(self.env, 'wiki', pagename, filename)
+@@ -192,7 +191,7 @@
+         """ Delete an attachment. """
+         pagename, filename = os.path.split(path)
+         if not WikiPage(self.env, pagename).exists:
+-            raise TracError, 'Wiki page "%s" does not exist' % pagename
++            raise ResourceNotFound, 'Wiki page "%s" does not exist' % pagename
+         attachment = Attachment(self.env, 'wiki', pagename, filename)
+         req.perm(attachment.resource).require('ATTACHMENT_DELETE')
+         attachment.delete()
 diff --git a/trunk/tracrpc/xml_rpc.py b/trunk/tracrpc/xml_rpc.py
 new file mode 100644
 --- /dev/null
 +++ b/trunk/tracrpc/xml_rpc.py
-@@ -0,0 +1,177 @@
+@@ -0,0 +1,182 @@
 +# -*- coding: utf-8 -*-
 +"""
 +License: BSD
 +
 +from trac.core import *
 +from trac.perm import PermissionError
++from trac.resource import ResourceNotFound
 +from trac.util.datefmt import utc
 +from trac.util.text import to_unicode
 +
 +            self._send_response(req,
 +                    xmlrpclib.dumps(
 +                        xmlrpclib.Fault(403, to_unicode(e))), content_type)
++        except ResourceNotFound, e:
++            self._send_response(req,
++                    xmlrpclib.dumps(
++                        xmlrpclib.Fault(404, to_unicode(e))), content_type)
 +        except Exception, e:
 +            self.log.error(e)
 +            import traceback
Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.