Properly raise HTTP 405 (and specify Allow headers) for RestController

Change-Id: Id790efc75c8207eb61d74e9b2242b310ccd62ab1
Depends-On: Ieffa3fddc3c8d3152742455ca46d69bcc7208d69
Closes-bug: #1334690
Closes-bug: #1450109

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):