From ff46de305a90c76721112f547c93bf7eec6ae0c1 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Tue, 11 Aug 2020 19:54:48 -0700 Subject: [PATCH] openapi: Use reasonable variable names. Signed-off-by: Anders Kaseorg --- zerver/openapi/markdown_extension.py | 4 +- zerver/openapi/openapi.py | 202 ++++++++++++++------------- zerver/tests/test_openapi.py | 80 +++++------ 3 files changed, 144 insertions(+), 142 deletions(-) diff --git a/zerver/openapi/markdown_extension.py b/zerver/openapi/markdown_extension.py index acf0bdc8e7..467c2b8cc6 100644 --- a/zerver/openapi/markdown_extension.py +++ b/zerver/openapi/markdown_extension.py @@ -229,8 +229,8 @@ def generate_curl_example(endpoint: str, method: str, lines = ["```curl"] operation = endpoint + ":" + method.lower() - operation_entry = openapi_spec.spec()['paths'][endpoint][method.lower()] - global_security = openapi_spec.spec()['security'] + operation_entry = openapi_spec.openapi()['paths'][endpoint][method.lower()] + global_security = openapi_spec.openapi()['security'] operation_params = operation_entry.get("parameters", []) operation_request_body = operation_entry.get("requestBody", None) diff --git a/zerver/openapi/openapi.py b/zerver/openapi/openapi.py index d9d41676fa..6730c137c9 100644 --- a/zerver/openapi/openapi.py +++ b/zerver/openapi/openapi.py @@ -15,17 +15,23 @@ OPENAPI_SPEC_PATH = os.path.abspath(os.path.join( # A list of endpoint-methods such that the endpoint # has documentation but not with this particular method. -EXCLUDE_UNDOCUMENTED_ENDPOINTS = {"/realm/emoji/{emoji_name}:delete", "/users:patch"} +EXCLUDE_UNDOCUMENTED_ENDPOINTS = { + ("/realm/emoji/{emoji_name}", "delete"), + ("/users", "patch"), +} # Consists of endpoints with some documentation remaining. # These are skipped but return true as the validator cannot exclude objects -EXCLUDE_DOCUMENTED_ENDPOINTS = {"/settings/notifications:patch"} +EXCLUDE_DOCUMENTED_ENDPOINTS = { + ("/settings/notifications", "patch"), +} + class OpenAPISpec(): - def __init__(self, path: str) -> None: - self.path = path - self.last_update: Optional[float] = None - self.data: Dict[str, Any] = {} - self.regex_dict: Dict[str, str] = {} - self.core_data: Any = None + def __init__(self, openapi_path: str) -> None: + self.openapi_path = openapi_path + self.mtime: Optional[float] = None + self._openapi: Dict[str, Any] = {} + self._endpoints_dict: Dict[str, str] = {} + self._request_validator: Optional[RequestValidator] = None def check_reload(self) -> None: # Because importing yamole (and in turn, yaml) takes @@ -40,21 +46,21 @@ class OpenAPISpec(): # corruption. from yamole import YamoleParser - last_modified = os.path.getmtime(self.path) + mtime = os.path.getmtime(self.openapi_path) # Using == rather than >= to cover the corner case of users placing an # earlier version than the current one - if self.last_update == last_modified: + if self.mtime == mtime: return - with open(self.path) as f: - yaml_parser = YamoleParser(f) - self.data = yaml_parser.data - validator_spec = create_spec(self.data) - self.core_data = RequestValidator(validator_spec) - self.create_regex_dict() - self.last_update = last_modified + with open(self.openapi_path) as f: + yamole_parser = YamoleParser(f) + self._openapi = yamole_parser.data + spec = create_spec(self._openapi) + self._request_validator = RequestValidator(spec) + self.create_endpoints_dict() + self.mtime = mtime - def create_regex_dict(self) -> None: + def create_endpoints_dict(self) -> None: # Algorithm description: # We have 2 types of endpoints # 1.with path arguments 2. without path arguments @@ -74,115 +80,112 @@ class OpenAPISpec(): # such as email they must be substituted with proper regex. email_regex = r'([a-zA-Z0-9_\-\.]+)@([a-zA-Z0-9_\-\.]+)\.([a-zA-Z]{2,5})' - self.regex_dict = {} - for key in self.data['paths']: - if '{' not in key: + self._endpoints_dict = {} + for endpoint in self._openapi['paths']: + if '{' not in endpoint: continue - regex_key = '^' + key + '$' + path_regex = '^' + endpoint + '$' # Numeric arguments have id at their end # so find such arguments and replace them with numeric # regex - regex_key = re.sub(r'{[^}]*id}', r'[0-9]*', regex_key) + path_regex = re.sub(r'{[^}]*id}', r'[0-9]*', path_regex) # Email arguments end with email - regex_key = re.sub(r'{[^}]*email}', email_regex, regex_key) + path_regex = re.sub(r'{[^}]*email}', email_regex, path_regex) # All other types of arguments are supposed to be # all-encompassing string. - regex_key = re.sub(r'{[^}]*}', r'[^\/]*', regex_key) - regex_key = regex_key.replace(r'/', r'\/') - regex_key = fr'{regex_key}' - self.regex_dict[regex_key] = key + path_regex = re.sub(r'{[^}]*}', r'[^\/]*', path_regex) + path_regex = path_regex.replace(r'/', r'\/') + self._endpoints_dict[path_regex] = endpoint - def spec(self) -> Dict[str, Any]: + def openapi(self) -> Dict[str, Any]: """Reload the OpenAPI file if it has been modified after the last time it was read, and then return the parsed data. """ self.check_reload() - assert(len(self.data) > 0) - return self.data + assert(len(self._openapi) > 0) + return self._openapi - def regex_keys(self) -> Dict[str, str]: + def endpoints_dict(self) -> Dict[str, str]: """Reload the OpenAPI file if it has been modified after the last time it was read, and then return the parsed data. """ self.check_reload() - assert(len(self.regex_dict) > 0) - return self.regex_dict + assert(len(self._endpoints_dict) > 0) + return self._endpoints_dict - def core_validator(self) -> Any: + def request_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() - return self.core_data + assert self._request_validator is not None + return self._request_validator class SchemaError(Exception): pass openapi_spec = OpenAPISpec(OPENAPI_SPEC_PATH) -def get_schema(endpoint: str, method: str, response: str) -> Dict[str, Any]: - if len(response) == 3 and ('oneOf' in (openapi_spec.spec())['paths'][endpoint] - [method.lower()]['responses'][response]['content'] - ['application/json']['schema']): +def get_schema(endpoint: str, method: str, status_code: str) -> Dict[str, Any]: + if len(status_code) == 3 and ('oneOf' in openapi_spec.openapi()['paths'][endpoint] + [method.lower()]['responses'][status_code]['content'] + ['application/json']['schema']): # Currently at places where multiple schemas are defined they only # differ in example so either can be used. - response += '_0' - if len(response) == 3: - schema = (openapi_spec.spec()['paths'][endpoint][method.lower()]['responses'] - [response]['content']['application/json']['schema']) + status_code += '_0' + if len(status_code) == 3: + schema = (openapi_spec.openapi()['paths'][endpoint][method.lower()]['responses'] + [status_code]['content']['application/json']['schema']) return schema else: - resp_code = int(response[4]) - response = response[0:3] - schema = (openapi_spec.spec()['paths'][endpoint][method.lower()]['responses'] - [response]['content']['application/json']['schema']["oneOf"][resp_code]) + subschema_index = int(status_code[4]) + status_code = status_code[0:3] + schema = (openapi_spec.openapi()['paths'][endpoint][method.lower()]['responses'] + [status_code]['content']['application/json']['schema']["oneOf"][subschema_index]) return schema def get_openapi_fixture(endpoint: str, method: str, - response: str='200') -> Dict[str, Any]: + status_code: str='200') -> Dict[str, Any]: """Fetch a fixture from the full spec object. """ - return (get_schema(endpoint, method, response)['example']) + return get_schema(endpoint, method, status_code)['example'] def get_openapi_description(endpoint: str, method: str) -> str: """Fetch a description from the full spec object. """ - description = openapi_spec.spec()['paths'][endpoint][method.lower()]['description'] - return description + return openapi_spec.openapi()['paths'][endpoint][method.lower()]['description'] def get_openapi_paths() -> Set[str]: - return set(openapi_spec.spec()['paths'].keys()) + return set(openapi_spec.openapi()['paths'].keys()) def get_openapi_parameters(endpoint: str, method: str, include_url_parameters: bool=True) -> List[Dict[str, Any]]: - openapi_endpoint = openapi_spec.spec()['paths'][endpoint][method.lower()] + operation = openapi_spec.openapi()['paths'][endpoint][method.lower()] # We do a `.get()` for this last bit to distinguish documented # endpoints with no parameters (empty list) from undocumented # endpoints (KeyError exception). - parameters = openapi_endpoint.get('parameters', []) + parameters = operation.get('parameters', []) # Also, we skip parameters defined in the URL. if not include_url_parameters: parameters = [parameter for parameter in parameters if parameter['in'] != 'path'] return parameters -def get_openapi_return_values(endpoint: str, method: str, - include_url_parameters: bool=True) -> List[Dict[str, Any]]: - openapi_endpoint = openapi_spec.spec()['paths'][endpoint][method.lower()] - response = openapi_endpoint['responses']['200']['content']['application/json']['schema'] +def get_openapi_return_values(endpoint: str, method: str) -> List[Dict[str, Any]]: + operation = openapi_spec.openapi()['paths'][endpoint][method.lower()] + schema = operation['responses']['200']['content']['application/json']['schema'] # In cases where we have used oneOf, the schemas only differ in examples # So we can choose any. - if 'oneOf' in response: - response = response['oneOf'][0] - response = response['properties'] - return response + if 'oneOf' in schema: + schema = schema['oneOf'][0] + return schema['properties'] -def match_against_openapi_regex(endpoint: str) -> Optional[str]: - for key in openapi_spec.regex_keys(): - matches = re.match(fr'{key}', endpoint) +def find_openapi_endpoint(path: str) -> Optional[str]: + for path_regex, endpoint in openapi_spec.endpoints_dict().items(): + matches = re.match(path_regex, path) if matches: - return openapi_spec.regex_keys()[key] + return endpoint return None def get_event_type(event: Dict[str, Any]) -> str: @@ -198,8 +201,8 @@ def fix_events(content: Dict[str, Any]) -> None: for event in content['events']: event.pop('user', None) -def validate_against_openapi_schema(content: Dict[str, Any], endpoint: str, - method: str, response: str) -> bool: +def validate_against_openapi_schema(content: Dict[str, Any], path: str, + method: str, status_code: str) -> bool: """Compare a "content" dict with the defined schema for a specific method in an endpoint. Return true if validated and false if skipped. """ @@ -208,26 +211,27 @@ def validate_against_openapi_schema(content: Dict[str, Any], endpoint: str, # hope to eliminate over time as we improve our API documentation. # No 500 responses have been documented, so skip them - if response.startswith('5'): + if status_code.startswith('5'): return False - if endpoint not in openapi_spec.spec()['paths'].keys(): - match = match_against_openapi_regex(endpoint) + if path not in openapi_spec.openapi()['paths'].keys(): + endpoint = find_openapi_endpoint(path) # If it doesn't match it hasn't been documented yet. - if match is None: + if endpoint is None: return False - endpoint = match + else: + endpoint = path # Excluded endpoint/methods - if endpoint + ':' + method in EXCLUDE_UNDOCUMENTED_ENDPOINTS: + if (endpoint, method) in EXCLUDE_UNDOCUMENTED_ENDPOINTS: return False # Return true for endpoints with only response documentation remaining - if endpoint + ':' + method in EXCLUDE_DOCUMENTED_ENDPOINTS: + if (endpoint, method) in EXCLUDE_DOCUMENTED_ENDPOINTS: return True # Check if the response matches its code - if response.startswith('2') and (content.get('result', 'success').lower() != 'success'): + if status_code.startswith('2') and (content.get('result', 'success').lower() != 'success'): raise SchemaError("Response is not 200 but is validating against 200 schema") # Code is not declared but appears in various 400 responses. If # common, it can be added to 400 response schema - if response.startswith('4'): + if status_code.startswith('4'): # This return statement should ideally be not here. But since # we have not defined 400 responses for various paths this has # been added as all 400 have the same schema. When all 400 @@ -235,7 +239,7 @@ def validate_against_openapi_schema(content: Dict[str, Any], endpoint: str, 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, response) + 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. @@ -250,11 +254,11 @@ def validate_schema_array(schema: Dict[str, Any]) -> None: Helper function for validate_schema """ if 'oneOf' in schema['items']: - for oneof_schema in schema['items']['oneOf']: - if oneof_schema['type'] == 'array': - validate_schema_array(oneof_schema) - elif oneof_schema['type'] == 'object': - validate_schema(oneof_schema) + for subschema in schema['items']['oneOf']: + if subschema['type'] == 'array': + validate_schema_array(subschema) + elif subschema['type'] == 'object': + validate_schema(subschema) else: if schema['items']['type'] == 'array': validate_schema_array(schema['items']) @@ -272,18 +276,18 @@ def validate_schema(schema: Dict[str, Any]) -> None: if 'additionalProperties' not in schema: raise SchemaError('additionalProperties needs to be defined for objects to make' + 'sure they have no additional properties left to be documented.') - for key in schema.get('properties', dict()): - if 'oneOf' in schema['properties'][key]: - for types in schema['properties'][key]['oneOf']: - if types['type'] == 'object': - validate_schema(types) - elif types['type'] == 'array': - validate_schema_array(types) + for property_schema in schema.get('properties', dict()).values(): + if 'oneOf' in property_schema: + for subschema in property_schema['oneOf']: + if subschema['type'] == 'object': + validate_schema(subschema) + elif subschema['type'] == 'array': + validate_schema_array(subschema) else: - if schema['properties'][key]['type'] == 'object': - validate_schema(schema['properties'][key]) - elif schema['properties'][key]['type'] == 'array': - validate_schema_array(schema['properties'][key]) + if property_schema['type'] == 'object': + validate_schema(property_schema) + elif property_schema['type'] == 'array': + validate_schema_array(property_schema) if schema['additionalProperties']: if schema['additionalProperties']['type'] == 'array': validate_schema_array(schema['additionalProperties']) @@ -314,14 +318,16 @@ def likely_deprecated_parameter(parameter_description: str) -> bool: # Skip those JSON endpoints whose query parameters are different from # their `/api/v1` counterpart. This is a legacy code issue that we # plan to fix by changing the implementation. -SKIP_JSON = {'/fetch_api_key:post'} +SKIP_JSON = { + ('/fetch_api_key', 'post'), +} def validate_request(url: str, method: str, data: Dict[str, Any], http_headers: Dict[str, Any], json_url: bool, status_code: str, intentionally_undocumented: bool=False) -> None: # Some JSON endpoints have different parameters compared to # their `/api/v1` counterparts. - if json_url and url + ':' + method in SKIP_JSON: + if json_url and (url, method) in SKIP_JSON: return # TODO: Add support for file upload endpoints that lack the /json/ @@ -333,7 +339,7 @@ def validate_request(url: str, method: str, data: Dict[str, Any], # against the OpenAPI documentation. mock_request = MockRequest('http://localhost:9991/', method, '/api/v1' + url, headers=http_headers, args=data) - result = openapi_spec.core_validator().validate(mock_request) + result = openapi_spec.request_validator().validate(mock_request) if len(result.errors) != 0: # Requests that do not validate against the OpenAPI spec must either: # * Have returned a 400 (bad request) error diff --git a/zerver/tests/test_openapi.py b/zerver/tests/test_openapi.py index 487b7d6637..ebd5bd5fc6 100644 --- a/zerver/tests/test_openapi.py +++ b/zerver/tests/test_openapi.py @@ -16,7 +16,6 @@ from typing import ( Tuple, Union, ) -from unittest import mock from unittest.mock import MagicMock, patch import yaml @@ -34,10 +33,10 @@ from zerver.openapi.openapi import ( OPENAPI_SPEC_PATH, OpenAPISpec, SchemaError, + find_openapi_endpoint, get_openapi_fixture, get_openapi_parameters, get_openapi_paths, - match_against_openapi_regex, openapi_spec, to_python_type, validate_against_openapi_schema, @@ -156,7 +155,7 @@ class OpenAPIToolsTest(ZulipTestCase): with open(os.path.join(os.path.dirname(OPENAPI_SPEC_PATH), "testing.yaml")) as test_file: test_dict = yaml.safe_load(test_file) - openapi_spec.spec()['paths']['testing'] = test_dict + openapi_spec.openapi()['paths']['testing'] = test_dict try: validate_against_openapi_schema((test_dict['test1']['responses']['200']['content'] ['application/json']['example']), @@ -170,7 +169,7 @@ class OpenAPIToolsTest(ZulipTestCase): validate_schema((test_dict['test3']['responses']['200'] ['content']['application/json']['schema'])) finally: - openapi_spec.spec()['paths'].pop('testing', None) + openapi_spec.openapi()['paths'].pop('testing', None) def test_to_python_type(self) -> None: TYPES = { @@ -188,18 +187,18 @@ class OpenAPIToolsTest(ZulipTestCase): def test_live_reload(self) -> None: # Force the reload by making the last update date < the file's last # modified date - openapi_spec.last_update = 0 + openapi_spec.mtime = 0 get_openapi_fixture(TEST_ENDPOINT, TEST_METHOD) # Check that the file has been reloaded by verifying that the last # update date isn't zero anymore - self.assertNotEqual(openapi_spec.last_update, 0) + self.assertNotEqual(openapi_spec.mtime, 0) # Now verify calling it again doesn't call reload - old_spec_dict = openapi_spec.spec() + old_openapi = openapi_spec.openapi() get_openapi_fixture(TEST_ENDPOINT, TEST_METHOD) - new_spec_dict = openapi_spec.spec() - self.assertIs(old_spec_dict, new_spec_dict) + new_openapi = openapi_spec.openapi() + self.assertIs(old_openapi, new_openapi) class OpenAPIArgumentsTest(ZulipTestCase): # This will be filled during test_openapi_arguments: @@ -908,7 +907,7 @@ class TestCurlExampleGeneration(ZulipTestCase): ] self.assertEqual(generated_curl_example, expected_curl_example) - @patch("zerver.openapi.openapi.OpenAPISpec.spec") + @patch("zerver.openapi.openapi.OpenAPISpec.openapi") def test_generate_and_render_curl_with_default_examples(self, spec_mock: MagicMock) -> None: spec_mock.return_value = self.spec_mock_without_examples generated_curl_example = self.curl_example("/mark_stream_as_read", "POST") @@ -922,7 +921,7 @@ class TestCurlExampleGeneration(ZulipTestCase): ] self.assertEqual(generated_curl_example, expected_curl_example) - @patch("zerver.openapi.openapi.OpenAPISpec.spec") + @patch("zerver.openapi.openapi.OpenAPISpec.openapi") def test_generate_and_render_curl_with_invalid_method(self, spec_mock: MagicMock) -> None: spec_mock.return_value = self.spec_mock_with_invalid_method with self.assertRaises(ValueError): @@ -945,7 +944,7 @@ class TestCurlExampleGeneration(ZulipTestCase): ] self.assertEqual(generated_curl_example, expected_curl_example) - @patch("zerver.openapi.openapi.OpenAPISpec.spec") + @patch("zerver.openapi.openapi.OpenAPISpec.openapi") def test_generate_and_render_curl_with_object(self, spec_mock: MagicMock) -> None: spec_mock.return_value = self.spec_mock_using_object generated_curl_example = self.curl_example("/endpoint", "GET") @@ -958,19 +957,19 @@ class TestCurlExampleGeneration(ZulipTestCase): ] self.assertEqual(generated_curl_example, expected_curl_example) - @patch("zerver.openapi.openapi.OpenAPISpec.spec") + @patch("zerver.openapi.openapi.OpenAPISpec.openapi") def test_generate_and_render_curl_with_object_without_example(self, spec_mock: MagicMock) -> None: spec_mock.return_value = self.spec_mock_using_object_without_example with self.assertRaises(ValueError): self.curl_example("/endpoint", "GET") - @patch("zerver.openapi.openapi.OpenAPISpec.spec") + @patch("zerver.openapi.openapi.OpenAPISpec.openapi") def test_generate_and_render_curl_with_array_without_example(self, spec_mock: MagicMock) -> None: spec_mock.return_value = self.spec_mock_using_array_without_example with self.assertRaises(ValueError): self.curl_example("/endpoint", "GET") - @patch("zerver.openapi.openapi.OpenAPISpec.spec") + @patch("zerver.openapi.openapi.OpenAPISpec.openapi") def test_generate_and_render_curl_with_param_in_path(self, spec_mock: MagicMock) -> None: spec_mock.return_value = self.spec_mock_using_param_in_path generated_curl_example = self.curl_example("/endpoint/{param1}", "GET") @@ -1023,30 +1022,27 @@ class OpenAPIAttributesTest(ZulipTestCase): VALID_TAGS = ["users", "server_and_organizations", "authentication", "real_time_events", "streams", "messages", "users", "webhooks"] - openapi_spec = OpenAPISpec(OPENAPI_SPEC_PATH).spec()["paths"] - for path in openapi_spec: + paths = OpenAPISpec(OPENAPI_SPEC_PATH).openapi()["paths"] + for path, path_item in paths.items(): if path in EXCLUDE: continue - for method in openapi_spec[path]: + for method, operation in path_item.items(): # Check if every file has an operationId - assert("operationId" in openapi_spec[path][method]) - assert("tags" in openapi_spec[path][method]) - tag = openapi_spec[path][method]["tags"][0] + assert("operationId" in operation) + assert("tags" in operation) + tag = operation["tags"][0] assert(tag in VALID_TAGS) - for response in openapi_spec[path][method]['responses']: - response_schema = (openapi_spec[path][method]['responses'][response] - ['content']['application/json']['schema']) - if 'oneOf' in response_schema: - cnt = 0 - for entry in response_schema['oneOf']: - validate_schema(entry) - assert(validate_against_openapi_schema(entry['example'], path, - method, response + '_' + str(cnt))) - cnt += 1 + for status_code, response in operation['responses'].items(): + schema = response['content']['application/json']['schema'] + if 'oneOf' in schema: + for subschema_index, subschema in enumerate(schema['oneOf']): + validate_schema(subschema) + assert(validate_against_openapi_schema(subschema['example'], path, + method, status_code + '_' + str(subschema_index))) continue - validate_schema(response_schema) - assert(validate_against_openapi_schema(response_schema['example'], path, - method, response)) + validate_schema(schema) + assert(validate_against_openapi_schema(schema['example'], path, + method, status_code)) class OpenAPIRegexTest(ZulipTestCase): def test_regex(self) -> None: @@ -1056,18 +1052,18 @@ class OpenAPIRegexTest(ZulipTestCase): """ # Some of the undocumentd endpoints which are very similar to # some of the documented endpoints. - assert(match_against_openapi_regex('/users/me/presence') is None) - assert(match_against_openapi_regex('/users/me/subscriptions/23') is None) - assert(match_against_openapi_regex('/users/iago/subscriptions/23') is None) - assert(match_against_openapi_regex('/messages/matches_narrow') is None) + assert(find_openapi_endpoint('/users/me/presence') is None) + assert(find_openapi_endpoint('/users/me/subscriptions/23') is None) + assert(find_openapi_endpoint('/users/iago/subscriptions/23') is None) + assert(find_openapi_endpoint('/messages/matches_narrow') is None) # Making sure documented endpoints are matched correctly. - assert(match_against_openapi_regex('/users/23/subscriptions/21') == + assert(find_openapi_endpoint('/users/23/subscriptions/21') == '/users/{user_id}/subscriptions/{stream_id}') - assert(match_against_openapi_regex('/users/iago@zulip.com/presence') == + assert(find_openapi_endpoint('/users/iago@zulip.com/presence') == '/users/{email}/presence') - assert(match_against_openapi_regex('/messages/23') == + assert(find_openapi_endpoint('/messages/23') == '/messages/{message_id}') - assert(match_against_openapi_regex('/realm/emoji/realm_emoji_1') == + assert(find_openapi_endpoint('/realm/emoji/realm_emoji_1') == '/realm/emoji/{emoji_name}') class OpenAPIRequestValidatorTest(ZulipTestCase):