Merge pull request #541 from sirosen/errorhandler-must-reraise

[7.x] ValueError if error handler does not reraise

Author: sirosen

CHANGELOG.rst

@@ -45,6 +45,12 @@ Usages are varied, but include
 
     parser = MyParser()
 
+Changes:
+
+* Registered `error_handler` callbacks are required to raise an exception.
+  If a handler is invoked and no exception is raised, `webargs` will raise
+  a `ValueError` (:issue:`527`)
+
 6.1.1 (2020-09-08)
 ******************
 

src/webargs/core.py

@@ -283,10 +283,9 @@ def parse(
                 error_status_code=error_status_code,
                 error_headers=error_headers,
             )
-            warnings.warn(
-                "_on_validation_error hook did not raise an exception and flow "
-                "of control returned to parse(). You may get unexpected results"
-            )
+            raise ValueError(
+                "_on_validation_error hook did not raise an exception"
+            ) from error
         return data
 
     def get_default_request(self):

tests/test_core.py

@@ -289,14 +289,19 @@ def test_value_error_raised_if_parse_called_with_invalid_location(parser, web_re
 
 @mock.patch("webargs.core.Parser.handle_error")
 def test_handle_error_called_when_parsing_raises_error(handle_error, web_request):
+    # handle_error must raise an error to be valid
+    handle_error.side_effect = ValidationError("parsing failed")
+
     def always_fail(*args, **kwargs):
         raise ValidationError("error occurred")
 
     p = Parser()
     assert handle_error.call_count == 0
-    p.parse({"foo": fields.Field()}, web_request, validate=always_fail)
+    with pytest.raises(ValidationError):
+        p.parse({"foo": fields.Field()}, web_request, validate=always_fail)
     assert handle_error.call_count == 1
-    p.parse({"foo": fields.Field()}, web_request, validate=always_fail)
+    with pytest.raises(ValidationError):
+        p.parse({"foo": fields.Field()}, web_request, validate=always_fail)
     assert handle_error.call_count == 2
 
 
@@ -360,6 +365,25 @@ def handle_error(error, req, schema, *, error_status_code, error_headers):
         parser.parse(mock_schema, web_request)
 
 
+def test_custom_error_handler_must_reraise(web_request):
+    class CustomError(Exception):
+        pass
+
+    mock_schema = mock.Mock(spec=Schema)
+    mock_schema.strict = True
+    mock_schema.load.side_effect = ValidationError("parsing json failed")
+    parser = Parser()
+
+    @parser.error_handler
+    def handle_error(error, req, schema, *, error_status_code, error_headers):
+        pass
+
+    # because the handler above does not raise a new error, the parser should
+    # raise a ValueError -- indicating a programming error
+    with pytest.raises(ValueError):
+        parser.parse(mock_schema, web_request)
+
+
 def test_custom_location_loader(web_request):
     web_request.data = {"foo": 42}
 

tests/test_flaskparser.py

@@ -1,4 +1,4 @@
-from werkzeug.exceptions import HTTPException
+from werkzeug.exceptions import HTTPException, BadRequest
 import pytest
 
 from flask import Flask
@@ -85,6 +85,9 @@ def test_parsing_unexpected_headers_when_raising(self, testapp):
 
 @mock.patch("webargs.flaskparser.abort")
 def test_abort_called_on_validation_error(mock_abort):
+    # error handling must raise an error to be valid
+    mock_abort.side_effect = BadRequest("foo")
+
     app = Flask("testapp")
 
     def validate(x):
@@ -97,7 +100,8 @@ def validate(x):
         data=json.dumps({"value": 41}),
         content_type="application/json",
     ):
-        parser.parse(argmap)
+        with pytest.raises(HTTPException):
+            parser.parse(argmap)
     mock_abort.assert_called()
     abort_args, abort_kwargs = mock_abort.call_args
     assert abort_args[0] == 422