Give 404 or 400 instead of 500 for unexpected HTTP methods. (#1264)

* Give 404 or 400 instead of 500 for unexpected HTTP methods.

* GET should remain a 500 if subclasses dont override it.

* addressed review comments

framework/basehandlers.py

@@ -46,16 +46,16 @@ class BaseHandler(flask.views.MethodView):
   def request(self):
     return flask.request
 
-  def abort(self, status, msg=None):
+  def abort(self, status, msg=None, **kwargs):
     """Support webapp2-style, e.g., self.abort(400)."""
     if msg:
       if status == 500:
         logging.error('ISE: %s' % msg)
       else:
         logging.info('Abort %r: %s' % (status, msg))
-      flask.abort(status, msg)
+      flask.abort(status, description=msg, **kwargs)
     else:
-      flask.abort(status)
+      flask.abort(status, **kwargs)
 
   def redirect(self, url):
     """Support webapp2-style, e.g., return self.redirect(url)."""
@@ -153,21 +153,33 @@ class APIHandler(BaseHandler):
     handler_data = self.do_delete(*args, **kwargs)
     return flask.jsonify(handler_data), headers
 
-  def do_get(self):
+  def _get_valid_methods(self):
+    """For 405 responses, list methods the concrete handler implements."""
+    valid_methods = ['GET']
+    if self.do_post.__code__ is not APIHandler.do_post.__code__:
+      valid_methods.append('POST')
+    if self.do_patch.__code__ is not APIHandler.do_patch.__code__:
+      valid_methods.append('PATCH')
+    if self.do_delete.__code__ is not APIHandler.do_delete.__code__:
+      valid_methods.append('DELETE')
+    return valid_methods
+
+  def do_get(self, **kwargs):
     """Subclasses should implement this method to handle a GET request."""
+    # Every API handler must handle GET.
     raise NotImplementedError()
 
-  def do_post(self):
+  def do_post(self, **kwargs):
     """Subclasses should implement this method to handle a POST request."""
-    raise NotImplementedError()
+    self.abort(405, valid_methods=self._get_valid_methods())
 
-  def do_patch(self):
+  def do_patch(self, **kwargs):
     """Subclasses should implement this method to handle a PATCH request."""
-    raise NotImplementedError()
+    self.abort(405, valid_methods=self._get_valid_methods())
 
-  def do_delete(self):
+  def do_delete(self, **kwargs):
     """Subclasses should implement this method to handle a DELETE request."""
-    raise NotImplementedError()
+    self.abort(405, valid_methods=self._get_valid_methods())
 
 
 class FlaskHandler(BaseHandler):
@@ -212,7 +224,7 @@ class FlaskHandler(BaseHandler):
 
   def process_post_data(self):
     """Subclasses should implement this method to handle a POST request."""
-    raise NotImplementedError()
+    self.abort(405, msg='Unexpected HTTP method', valid_methods=['GET'])
 
   def get_common_data(self, path=None):
     """Return template data used on all pages, e.g., sign-in info."""

framework/basehandlers_test.py

@@ -31,7 +31,7 @@ import settings
 
 
 
-class BaseHandlertests(unittest.TestCase):
+class BaseHandlerTests(unittest.TestCase):
 
   def setUp(self):
     self.handler = basehandlers.BaseHandler()
@@ -53,7 +53,7 @@ class BaseHandlertests(unittest.TestCase):
   def test_abort__with_msg(self, mock_abort, mock_info):
     """We can abort request handling."""
     self.handler.abort(400, msg='You messed up')
-    mock_abort.assert_called_once_with(400, 'You messed up')
+    mock_abort.assert_called_once_with(400, description='You messed up')
     mock_info.assert_called_once()
 
   @mock.patch('logging.error')
@@ -61,7 +61,7 @@ class BaseHandlertests(unittest.TestCase):
   def test_abort__with_500_msg(self, mock_abort, mock_error):
     """We can abort request handling."""
     self.handler.abort(500, msg='We messed up')
-    mock_abort.assert_called_once_with(500, 'We messed up')
+    mock_abort.assert_called_once_with(500, description='We messed up')
     mock_error.assert_called_once()
 
   @mock.patch('flask.redirect')
@@ -296,6 +296,44 @@ class ConstHandlerTests(unittest.TestCase):
         actual_response.json)
 
 
+class APIHandlerTests(unittest.TestCase):
+
+  def setUp(self):
+    self.handler = basehandlers.APIHandler()
+
+  def test_do_get(self):
+    """If a subclass does not implement do_get(), raise NotImplementedError."""
+    with self.assertRaises(NotImplementedError):
+      self.handler.do_get()
+
+    with self.assertRaises(NotImplementedError):
+      self.handler.do_get(feature_id=1234)
+
+  @mock.patch('flask.abort')
+  def check_bad_HTTP_method(self, handler_method, mock_abort):
+    mock_abort.side_effect = werkzeug.exceptions.MethodNotAllowed
+
+    with self.assertRaises(mock_abort.side_effect):
+      handler_method()
+    mock_abort.assert_called_once_with(405, valid_methods=['GET'])
+
+    # Extra URL parameters do not crash the app.
+    with self.assertRaises(mock_abort.side_effect):
+      handler_method(feature_id=1234)
+
+  def test_do_post(self):
+    """If a subclass does not implement do_post(), return a 405."""
+    self.check_bad_HTTP_method(self.handler.do_post)
+
+  def test_do_patch(self):
+    """If a subclass does not implement do_patch(), return a 405."""
+    self.check_bad_HTTP_method(self.handler.do_patch)
+
+  def test_do_delete(self):
+    """If a subclass does not implement do_delete(), return a 405."""
+    self.check_bad_HTTP_method(self.handler.do_delete)
+
+
 class FlaskHandlerTests(unittest.TestCase):
 
   def setUp(self):
@@ -362,9 +400,9 @@ class FlaskHandlerTests(unittest.TestCase):
     self.assertEqual('special.html', actual)
 
   def test_process_post_data__missing(self):
-    """Every subsclass should overide process_post_data()."""
+    """Subsclasses that don't override process_post_data() give a 405."""
     self.handler = basehandlers.FlaskHandler()
-    with self.assertRaises(NotImplementedError):
+    with self.assertRaises(werkzeug.exceptions.MethodNotAllowed):
       self.handler.process_post_data()
 
   def test_get_common_data__signed_out(self):