From 031f4596ab1737a237b1c099e792fe627a937ff7 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Tue, 11 Jan 2022 18:08:52 -0800 Subject: [PATCH] openapi: Use openapi_core ResponseValidator to validate responses. Signed-off-by: Anders Kaseorg --- pyproject.toml | 1 - zerver/openapi/openapi.py | 97 +++++++++++++++++++++--------------- zerver/tests/test_openapi.py | 9 ++-- 3 files changed, 62 insertions(+), 45 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index f6d5611bee..d51824e903 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -79,7 +79,6 @@ module = [ "netifaces.*", "onelogin.*", "openapi_core.*", - "openapi_schema_validator.*", "premailer.*", "pyinotify.*", "pyoembed.*", diff --git a/zerver/openapi/openapi.py b/zerver/openapi/openapi.py index 7635674404..a217b3ac72 100644 --- a/zerver/openapi/openapi.py +++ b/zerver/openapi/openapi.py @@ -10,11 +10,13 @@ import os import re from typing import Any, Dict, List, Mapping, Optional, Set, Tuple, Union +import orjson from jsonschema.exceptions import ValidationError as JsonSchemaValidationError from openapi_core import create_spec -from openapi_core.testing import MockRequest +from openapi_core.testing import MockRequest, MockResponse +from openapi_core.unmarshalling.schemas.exceptions import InvalidSchemaValue from openapi_core.validation.request.validators import RequestValidator -from openapi_schema_validator import OAS30Validator +from openapi_core.validation.response.validators import ResponseValidator OPENAPI_SPEC_PATH = os.path.abspath( os.path.join(os.path.dirname(__file__), "../openapi/zulip.yaml") @@ -78,6 +80,7 @@ class OpenAPISpec: self._openapi: Dict[str, Any] = {} self._endpoints_dict: Dict[str, str] = {} self._request_validator: Optional[RequestValidator] = None + self._response_validator: Optional[ResponseValidator] = None def check_reload(self) -> None: # Because importing yaml takes significant time, and we only @@ -104,6 +107,7 @@ class OpenAPISpec: spec = create_spec(openapi) self._request_validator = RequestValidator(spec) + self._response_validator = ResponseValidator(spec) self._openapi = naively_merge_allOf_dict(JsonRef.replace_refs(openapi)) self.create_endpoints_dict() self.mtime = mtime @@ -170,6 +174,15 @@ class OpenAPISpec: assert self._request_validator is not None return self._request_validator + def response_validator(self) -> RequestValidator: + """Reload the OpenAPI file if it has been modified after the last time + it was read, and then return the openapi_core validator object. Similar + to preceding functions. Used for proper access to OpenAPI objects. + """ + self.check_reload() + assert self._response_validator is not None + return self._response_validator + class SchemaError(Exception): pass @@ -420,51 +433,57 @@ def validate_against_openapi_schema( # been added as all 400 have the same schema. When all 400 # response have been defined this should be removed. return True - # The actual work of validating that the response matches the - # schema is done via the third-party OAS30Validator. - schema = get_schema(endpoint, method, status_code) + if endpoint == "/events" and method == "get": # This a temporary function for checking only documented events # as all events haven't been documented yet. # TODO: Remove this after all events have been documented. fix_events(content) - validator = OAS30Validator(schema) + mock_request = MockRequest("http://localhost:9991/", method, "/api/v1" + path) + mock_response = MockResponse( + # TODO: Use original response content instead of re-serializing it. + orjson.dumps(content), + status_code=status_code, + ) + result = openapi_spec.response_validator().validate(mock_request, mock_response) try: - validator.validate(content) - except JsonSchemaValidationError as error: - if not display_brief_error: - raise error + result.raise_for_errors() + except InvalidSchemaValue as isv: + message = f"{len(isv.schema_errors)} response validation error(s) at {method} /api/v1{path} ({status_code}):" + for error in isv.schema_errors: + if display_brief_error: + # display_brief_error is designed to avoid printing 1000 lines + # of output when the schema to validate is extremely large + # (E.g. the several dozen format variants for individual + # events returned by GET /events) and instead just display the + # specific variant we expect to match the response. + brief_error_validator_value = [ + validator_value + for validator_value in error.validator_value + if not prune_schema_by_type(validator_value, error.instance["type"]) + ] + brief_error_display_schema = error.schema.copy() + if "oneOf" in brief_error_display_schema: + brief_error_display_schema["oneOf"] = [ + i_schema + for i_schema in error.schema["oneOf"] + if not prune_schema_by_type(i_schema, error.instance["type"]) + ] - # display_brief_error is designed to avoid printing 1000 lines - # of output when the schema to validate is extremely large - # (E.g. the several dozen format variants for individual - # events returned by GET /events) and instead just display the - # specific variant we expect to match the response. - brief_error_validator_value = [ - validator_value - for validator_value in error.validator_value - if not prune_schema_by_type(validator_value, error.instance["type"]) - ] - brief_error_display_schema = error.schema.copy() - if "oneOf" in brief_error_display_schema: - brief_error_display_schema["oneOf"] = [ - i_schema - for i_schema in error.schema["oneOf"] - if not prune_schema_by_type(i_schema, error.instance["type"]) - ] - - # Field list from https://python-jsonschema.readthedocs.io/en/stable/errors/ - raise JsonSchemaValidationError( - message=error.message, - validator=error.validator, - path=error.path, - instance=error.instance, - schema_path=error.schema_path, - schema=brief_error_display_schema, - validator_value=brief_error_validator_value, - cause=error.cause, - ) + # Field list from https://python-jsonschema.readthedocs.io/en/stable/errors/ + error = JsonSchemaValidationError( + message=error.message, + validator=error.validator, + path=error.path, + instance=error.instance, + schema_path=error.schema_path, + schema=brief_error_display_schema, + validator_value=brief_error_validator_value, + cause=error.cause, + ) + message += f"\n\n{type(error).__name__}: {error}" + raise SchemaError(message) from None return True diff --git a/zerver/tests/test_openapi.py b/zerver/tests/test_openapi.py index e5a91d1a4c..711a46084f 100644 --- a/zerver/tests/test_openapi.py +++ b/zerver/tests/test_openapi.py @@ -8,7 +8,6 @@ from unittest.mock import MagicMock, patch import yaml from django.http import HttpResponse from django.utils import regex_helper -from jsonschema.exceptions import ValidationError from zerver.lib.request import _REQ, arguments_map from zerver.lib.rest import rest_dispatch @@ -86,7 +85,7 @@ class OpenAPIToolsTest(ZulipTestCase): def test_validate_against_openapi_schema(self) -> None: with self.assertRaisesRegex( - ValidationError, r"Additional properties are not allowed \('foo' was unexpected\)" + SchemaError, r"Additional properties are not allowed \('foo' was unexpected\)" ): bad_content: Dict[str, object] = { "msg": "", @@ -97,7 +96,7 @@ class OpenAPIToolsTest(ZulipTestCase): bad_content, TEST_ENDPOINT, TEST_METHOD, TEST_RESPONSE_SUCCESS ) - with self.assertRaisesRegex(ValidationError, r"42 is not of type string"): + with self.assertRaisesRegex(SchemaError, r"42 is not of type string"): bad_content = { "msg": 42, "result": "success", @@ -106,7 +105,7 @@ class OpenAPIToolsTest(ZulipTestCase): bad_content, TEST_ENDPOINT, TEST_METHOD, TEST_RESPONSE_SUCCESS ) - with self.assertRaisesRegex(ValidationError, r"'msg' is a required property"): + with self.assertRaisesRegex(SchemaError, r"'msg' is a required property"): bad_content = { "result": "success", } @@ -148,7 +147,7 @@ class OpenAPIToolsTest(ZulipTestCase): "200", ) with self.assertRaisesRegex( - ValidationError, + SchemaError, r"\{'obj': \{'str3': 'test', 'str4': 'extraneous'\}\} is not valid under any of the given schemas", ): validate_against_openapi_schema(