Merge pull request #120 from globus/an/fix_error_description_field_be…

…havior

Omit input data from validation errors

changelog.d/20240123_134319_ada_fix_error_description_field_behavior.rst

@@ -0,0 +1,7 @@
+Changes
+-------
+
+- Error descriptions in responses are now always strings (previously they could also
+  be lists of strings or lists of dictionaries).
+- Input validation errors now use an HTTP response status code of 422.
+- Validation errors no longer return input data in their description.

globus_action_provider_tools/flask/exceptions.py

@@ -20,19 +20,13 @@
     InternalServerError,
     NotFound,
     Unauthorized,
+    UnprocessableEntity,
 )
 
 JSONType = t.Union[str, int, float, bool, None, t.Dict[str, t.Any], t.List[t.Any]]
 
 
 class ActionProviderToolsException(HTTPException):
-    # This is only required to update allow mypy to recognize the description
-    # can be any JSON-able structure
-    def __init__(self, description: t.Optional[JSONType] = None, *args, **kwargs):
-        if description is not None:
-            description = json.dumps(description)
-        super().__init__(description, *args, **kwargs)
-
     @property
     def name(self):
         return type(self).__name__
@@ -41,19 +35,13 @@ def get_body(self, *args):
         return json.dumps(
             {
                 "code": self.name,
-                "description": self.get_description(),
+                "description": self.description,
             }
         )
 
     def get_headers(self, *args):
         return [("Content-Type", "application/json")]
 
-    def get_description(self, *args):
-        try:
-            return json.loads(self.description)
-        except json.decoder.JSONDecodeError:
-            return self.description
-
 
 class ActionNotFound(ActionProviderToolsException, NotFound):
     pass
@@ -63,6 +51,13 @@ class BadActionRequest(BadRequest, ActionProviderToolsException):
     pass
 
 
+class RequestValidationError(UnprocessableEntity, BadActionRequest):
+    # TODO: This inherits from BadActionRequest to avoid breaking
+    # downstream code that expects to catch BadActionRequest when an error occurs
+    # during validation. Remove this inheritance in a future release.
+    pass
+
+
 class ActionConflict(ActionProviderToolsException, Conflict):
     pass
 

globus_action_provider_tools/flask/helpers.py

@@ -24,7 +24,7 @@
 from globus_action_provider_tools.flask.exceptions import (
     ActionProviderError,
     ActionProviderToolsException,
-    BadActionRequest,
+    RequestValidationError,
     UnauthorizedRequest,
 )
 from globus_action_provider_tools.flask.types import ActionCallbackReturn, ViewReturn
@@ -144,7 +144,8 @@ def validate_input(
         request_json = RequestObject.parse_obj(request_json).__root__
         action_request = ActionRequest(**request_json)
     except ValidationError as ve:
-        raise BadActionRequest(ve.errors())
+        messages = [f"Field '{'.'.join(e['loc'])}': {e['msg']}" for e in ve.errors()]
+        raise RequestValidationError("; ".join(messages))
 
     input_body_validator(action_request.body)
 
@@ -197,7 +198,7 @@ def json_schema_input_validation(
     """
     result = validate_data(action_input, validator)
     if result.errors:
-        raise BadActionRequest(result.errors)
+        raise RequestValidationError(result.error_msg)
 
 
 def pydantic_input_validation(
@@ -210,7 +211,8 @@ def pydantic_input_validation(
     try:
         validator(**action_input)
     except ValidationError as ve:
-        raise BadActionRequest(ve.errors())
+        messages = [f"Field '{'.'.join(e['loc'])}': {e['msg']}" for e in ve.errors()]
+        raise RequestValidationError("; ".join(messages))
 
 
 try:

globus_action_provider_tools/validation.py

@@ -45,17 +45,12 @@ def request_validator(request: ValidationRequest) -> ValidationResult:
 def validate_data(
     data: Dict[str, Any], validator: jsonschema.protocols.Validator
 ) -> ValidationResult:
-    error_messages = []
-    for error in validator.iter_errors(data):
-        if error.path:
-            # Elements of the error path may be integers or other non-string types,
-            # but we need strings for use with join()
-            error_path_for_message = ".".join([str(x) for x in error.path])
-            error_message = f"'{error_path_for_message}' invalid due to {error.message}"
-        else:
-            error_message = error.message
-        error_messages.append(error_message)
-
-    error_msg = "; ".join(error_messages) if error_messages else None
-    result = ValidationResult(errors=error_messages, error_msg=error_msg)
+    # TODO: If python-jsonschema introduces a means of returning error messages that
+    # do not include input data, modify this to return more specific error information.
+    if not validator.is_valid(data):
+        message = "Input failed schema validation"
+        result = ValidationResult(errors=[message], error_msg=message)
+    else:
+        result = ValidationResult(errors=[], error_msg=None)
+
     return result

tests/test_exceptions.py

@@ -7,6 +7,7 @@
     ActionNotFound,
     ActionProviderError,
     BadActionRequest,
+    RequestValidationError,
     UnauthorizedRequest,
 )
 
@@ -18,6 +19,7 @@
         ActionNotFound,
         ActionProviderError,
         BadActionRequest,
+        RequestValidationError,
         UnauthorizedRequest,
     ],
 )

tests/test_flask_helpers/test_validation_helpers.py

@@ -9,6 +9,7 @@
 from globus_action_provider_tools.flask.exceptions import (
     ActionProviderError,
     BadActionRequest,
+    RequestValidationError,
 )
 from globus_action_provider_tools.flask.helpers import (
     get_input_body_validator,
@@ -88,19 +89,18 @@ def test_validating_action_request():
 
 
 @pytest.mark.parametrize(
-    "document, type_, message",
+    "document, message",
     (
-        ("wrong object type", "type_error.dict", "value is not a valid dict"),
-        ({1: "wrong key type"}, "type_error.str", "str type expected"),
+        ("wrong object type", "Field '__root__': value is not a valid dict"),
+        ({1: "wrong key type"}, "Field '__root__.__key__': str type expected"),
     ),
 )
-def test_validate_input_typeerror(document, type_, message):
+def test_validate_input_typeerror(document, message):
     """Verify that the `request_json` argument types are validated."""
 
     ap_description.input_schema = json.dumps(action_provider_json_input_schema)
     validator = get_input_body_validator(ap_description)
     with pytest.raises(BadActionRequest) as catcher:
         validate_input(document, validator)
-    assert catcher.value.get_response().status_code == 400
-    assert catcher.value.get_description()[0]["msg"] == message
-    assert catcher.value.get_description()[0]["type"] == type_
+    assert catcher.value.get_response().status_code == 422
+    assert catcher.value.description == message