From 3c634de56666cfadf19b813dd9de4e7e66b6ac65 Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Mon, 11 May 2015 09:48:53 -0700 Subject: Properly raise HTTP 405 (and specify Allow headers) for RestController Change-Id: Id790efc75c8207eb61d74e9b2242b310ccd62ab1 Depends-On: Ieffa3fddc3c8d3152742455ca46d69bcc7208d69 Closes-bug: #1334690 Closes-bug: #1450109 --- pecan/rest.py | 33 +++++++--- pecan/routing.py | 9 ++- pecan/tests/test_no_thread_locals.py | 4 +- pecan/tests/test_rest.py | 116 +++++++++++++++++++++++++++-------- 4 files changed, 126 insertions(+), 36 deletions(-) diff --git a/pecan/rest.py b/pecan/rest.py index ef5d753..4807bf0 100644 --- a/pecan/rest.py +++ b/pecan/rest.py @@ -116,14 +116,33 @@ class RestController(object): _lookup_result = self._handle_lookup(args, request) if _lookup_result: return _lookup_result - except (exc.HTTPClientError, exc.HTTPNotFound): + except (exc.HTTPClientError, exc.HTTPNotFound, + exc.HTTPMethodNotAllowed) as e: # - # If the matching handler results in a 400 or 404, attempt to + # If the matching handler results in a 400, 404, or 405, attempt to # handle a _lookup method (if it exists) # _lookup_result = self._handle_lookup(args, request) if _lookup_result: return _lookup_result + + # Build a correct Allow: header + if isinstance(e, exc.HTTPMethodNotAllowed): + + def method_iter(): + for func in ('get', 'get_one', 'get_all', 'new', 'edit', + 'get_delete'): + if self._find_controller(func): + yield 'GET' + break + for method in ('HEAD', 'POST', 'PUT', 'DELETE', 'TRACE', + 'PATCH'): + func = method.lower() + if self._find_controller(func): + yield method + + e.allow = sorted(method_iter()) + raise # return the result @@ -217,7 +236,7 @@ class RestController(object): return lookup_controller(sub_controller, remainder[1:], request) - abort(404) + abort(405) def _handle_get(self, method, remainder, request=None): ''' @@ -233,7 +252,7 @@ class RestController(object): if controller: self._handle_bad_rest_arguments(controller, remainder, request) return controller, [] - abort(404) + abort(405) method_name = remainder[-1] # check for new/edit/delete GET requests @@ -258,7 +277,7 @@ class RestController(object): self._handle_bad_rest_arguments(controller, remainder, request) return controller, remainder - abort(404) + abort(405) def _handle_delete(self, method, remainder, request=None): ''' @@ -291,7 +310,7 @@ class RestController(object): return lookup_controller(sub_controller, remainder[1:], request) - abort(404) + abort(405) def _handle_post(self, method, remainder, request=None): ''' @@ -315,7 +334,7 @@ class RestController(object): if controller: return controller, remainder - abort(404) + abort(405) def _handle_put(self, method, remainder, request=None): return self._handle_post(method, remainder, request) diff --git a/pecan/routing.py b/pecan/routing.py index 1d534aa..91c9fe3 100644 --- a/pecan/routing.py +++ b/pecan/routing.py @@ -49,7 +49,10 @@ def lookup_controller(obj, remainder, request=None): request) handle_security(obj) return obj, remainder - except (exc.HTTPNotFound, PecanNotFound): + except (exc.HTTPNotFound, exc.HTTPMethodNotAllowed, + PecanNotFound) as e: + if isinstance(e, PecanNotFound): + e = exc.HTTPNotFound() while notfound_handlers: name, obj, remainder = notfound_handlers.pop() if name == '_default': @@ -67,11 +70,11 @@ def lookup_controller(obj, remainder, request=None): remainder == [''] and len(obj._pecan['argspec'].args) > 1 ): - raise exc.HTTPNotFound + raise e obj_, remainder_ = result return lookup_controller(obj_, remainder_, request) else: - raise exc.HTTPNotFound + raise e def handle_lookup_traversal(obj, args): diff --git a/pecan/tests/test_no_thread_locals.py b/pecan/tests/test_no_thread_locals.py index 80aa88f..1ca418e 100644 --- a/pecan/tests/test_no_thread_locals.py +++ b/pecan/tests/test_no_thread_locals.py @@ -1002,8 +1002,8 @@ class TestRestController(PecanTestCase): assert r.status_int == 302 def test_invalid_custom_action(self): - r = self.app_.get('/things?_method=BAD', status=404) - assert r.status_int == 404 + r = self.app_.get('/things?_method=BAD', status=405) + assert r.status_int == 405 def test_named_action(self): # test custom "GET" request "length" diff --git a/pecan/tests/test_rest.py b/pecan/tests/test_rest.py index 433e424..96e3d84 100644 --- a/pecan/tests/test_rest.py +++ b/pecan/tests/test_rest.py @@ -250,8 +250,8 @@ class TestRestController(PecanTestCase): assert r.body == b_('OTHERS') # test an invalid custom action - r = app.get('/things?_method=BAD', status=404) - assert r.status_int == 404 + r = app.get('/things?_method=BAD', status=405) + assert r.status_int == 405 # test custom "GET" request "count" r = app.get('/things/count') @@ -299,7 +299,7 @@ class TestRestController(PecanTestCase): assert r.status_int == 200 assert r.body == b_(dumps(dict(items=ThingsController.data))) - def test_404_with_lookup(self): + def test_405_with_lookup(self): class LookupController(RestController): @@ -322,10 +322,10 @@ class TestRestController(PecanTestCase): # create the app app = TestApp(make_app(RootController())) - # these should 404 + # these should 405 for path in ('/things', '/things/'): r = app.get(path, expect_errors=True) - assert r.status_int == 404 + assert r.status_int == 405 r = app.get('/things/foo') assert r.status_int == 200 @@ -813,53 +813,53 @@ class TestRestController(PecanTestCase): app = TestApp(make_app(RootController())) # test get_all - r = app.get('/things', status=404) - assert r.status_int == 404 + r = app.get('/things', status=405) + assert r.status_int == 405 # test get_one - r = app.get('/things/1', status=404) - assert r.status_int == 404 + r = app.get('/things/1', status=405) + assert r.status_int == 405 # test post - r = app.post('/things', {'value': 'one'}, status=404) - assert r.status_int == 404 + r = app.post('/things', {'value': 'one'}, status=405) + assert r.status_int == 405 # test edit - r = app.get('/things/1/edit', status=404) - assert r.status_int == 404 + r = app.get('/things/1/edit', status=405) + assert r.status_int == 405 # test put - r = app.put('/things/1', {'value': 'ONE'}, status=404) + r = app.put('/things/1', {'value': 'ONE'}, status=405) # test put with _method parameter and GET r = app.get('/things/1?_method=put', {'value': 'ONE!'}, status=405) assert r.status_int == 405 # test put with _method parameter and POST - r = app.post('/things/1?_method=put', {'value': 'ONE!'}, status=404) - assert r.status_int == 404 + r = app.post('/things/1?_method=put', {'value': 'ONE!'}, status=405) + assert r.status_int == 405 # test get delete - r = app.get('/things/1/delete', status=404) - assert r.status_int == 404 + r = app.get('/things/1/delete', status=405) + assert r.status_int == 405 # test delete - r = app.delete('/things/1', status=404) - assert r.status_int == 404 + r = app.delete('/things/1', status=405) + assert r.status_int == 405 # test delete with _method parameter and GET r = app.get('/things/1?_method=DELETE', status=405) assert r.status_int == 405 # test delete with _method parameter and POST - r = app.post('/things/1?_method=DELETE', status=404) - assert r.status_int == 404 + r = app.post('/things/1?_method=DELETE', status=405) + assert r.status_int == 405 # test "RESET" custom action with warnings.catch_warnings(): warnings.simplefilter("ignore") - r = app.request('/things', method='RESET', status=404) - assert r.status_int == 404 + r = app.request('/things', method='RESET', status=405) + assert r.status_int == 405 def test_nested_rest_with_missing_intermediate_id(self): @@ -1490,6 +1490,74 @@ class TestRestController(PecanTestCase): assert r.status_int == 200 assert r.body == b_('DELETE_BAR') + def test_method_not_allowed_get(self): + class ThingsController(RestController): + + @expose() + def put(self, id_, value): + response.status = 200 + + @expose() + def delete(self, id_): + response.status = 200 + + app = TestApp(make_app(ThingsController())) + r = app.get('/', status=405) + assert r.status_int == 405 + assert r.headers['Allow'] == 'DELETE, PUT' + + def test_method_not_allowed_post(self): + class ThingsController(RestController): + + @expose() + def get_one(self): + return dict() + + app = TestApp(make_app(ThingsController())) + r = app.post('/', {'foo': 'bar'}, status=405) + assert r.status_int == 405 + assert r.headers['Allow'] == 'GET' + + def test_method_not_allowed_put(self): + class ThingsController(RestController): + + @expose() + def get_one(self): + return dict() + + app = TestApp(make_app(ThingsController())) + r = app.put('/123', status=405) + assert r.status_int == 405 + assert r.headers['Allow'] == 'GET' + + def test_method_not_allowed_delete(self): + class ThingsController(RestController): + + @expose() + def get_one(self): + return dict() + + app = TestApp(make_app(ThingsController())) + r = app.delete('/123', status=405) + assert r.status_int == 405 + assert r.headers['Allow'] == 'GET' + + def test_proper_allow_header_multiple_gets(self): + class ThingsController(RestController): + + @expose() + def get_all(self): + return dict() + + @expose() + def get(self): + return dict() + + app = TestApp(make_app(ThingsController())) + r = app.put('/123', status=405) + assert r.status_int == 405 + assert r.headers['Allow'] == 'GET' + def test_rest_with_utf8_uri(self): class FooController(RestController): -- cgit v1.2.1