From fbf647283be2a0bfede15b488e30c6dd1b48c21c Mon Sep 17 00:00:00 2001 From: orientor Date: Sat, 20 Jun 2020 22:55:32 +0530 Subject: [PATCH] openapi: Fix validate_against_openapi_schema nested object validation. We had a bug in `validate_against_openapi_schema` that prevented it from correctly inspecting nested arrays. Fix the bug and address all the exceptions, either via EXCLUDE_PROPERTIES or fixing them when simple. Also add a test case for nested verification. --- zerver/openapi/openapi.py | 96 ++++++++++++++++++------- zerver/openapi/testing.yaml | 136 +++++++++++++++++++++++++++++++++++ zerver/openapi/zulip.yaml | 41 ++++++++--- zerver/tests/test_openapi.py | 28 ++++++++ 4 files changed, 269 insertions(+), 32 deletions(-) create mode 100644 zerver/openapi/testing.yaml diff --git a/zerver/openapi/openapi.py b/zerver/openapi/openapi.py index d28eb1a66e..7d6ded4ab9 100644 --- a/zerver/openapi/openapi.py +++ b/zerver/openapi/openapi.py @@ -12,6 +12,18 @@ OPENAPI_SPEC_PATH = os.path.abspath(os.path.join( # The validator will ignore these keys when they appear in the "content" # passed. EXCLUDE_PROPERTIES = { + '/attachments': { + 'get': { + # messages is a small opaque object; should be easy to document + '200': ['messages'], + } + }, + '/events': { + 'get': { + # Array with opaque object + '200': ['events'] + } + }, '/register': { 'post': { '200': ['max_message_id', 'realm_emoji', 'pointer'], @@ -26,12 +38,22 @@ EXCLUDE_PROPERTIES = { 'realm_name_in_notifications', 'presence_enabled'], }, }, - + '/streams': { + 'get': { + # Some responses contain undocumented keys + '200': ['is_default'], + } + }, '/zulip-outgoing-webhook': { 'post': { '200': ['result', 'msg', 'message'], }, }, + '/users': { + 'get': { + '200': ['delivery_email'], + } + }, '/users/me': { 'get': { # Some responses contain undocumented keys @@ -55,11 +77,17 @@ EXCLUDE_PROPERTIES = { } }, '/messages': { + 'get': { + # Some responses contain undocumented keys and + # 'user' is opaque + '200': ['last_edit_timestamp', 'display_recipient', + 'match_content', 'match_subject', 'user'], + }, 'post': { # Extraneous '200': ['deliver_at'], } - } + }, } # A list of endpoint-methods such that the endpoint @@ -261,33 +289,41 @@ def validate_against_openapi_schema(content: Dict[str, Any], endpoint: str, def validate_array(content: List[Any], schema: Dict[str, Any], exclusion_list: List[str]) -> None: valid_types: List[type] = [] + object_schema: Optional[Dict[str, Any]] = None + array_schema: Optional[Dict[str, Any]] = None if 'oneOf' in schema['items']: - for valid_type in schema['items']['oneOf']: - valid_types.append(to_python_type(valid_type['type'])) + for oneof_schema in schema['items']['oneOf']: + if oneof_schema['type'] == 'array': + array_schema = oneof_schema + elif oneof_schema['type'] == 'object': + object_schema = oneof_schema + valid_types.append(to_python_type(oneof_schema['type'])) else: valid_types.append(to_python_type(schema['items']['type'])) + if schema['items']['type'] == 'array': + array_schema = schema['items'] + elif schema['items']['type'] == 'object': + object_schema = schema['items'] + for item in content: if type(item) not in valid_types: raise SchemaError('Wrong data type in array') # We can directly check for objects and arrays as # there are no mixed arrays consisting of objects # and arrays. - if 'object' in valid_types: - if 'oneOf' not in schema['items']: - if 'properties' not in schema['items']: - raise SchemaError('Opaque object in array') - validate_object(item, schema['items'], exclusion_list) - continue - # If the object was not an opaque object then - # the continue statement above should have - # been executed. - if type(item) is dict: - raise SchemaError('Opaque object in array') - if 'items' in schema['items']: - validate_array(item, schema['items'], exclusion_list) + if type(item) == dict: + assert object_schema is not None + if 'properties' not in object_schema: + raise SchemaError('Opaque object in array') + validate_object(item, object_schema, exclusion_list) + if type(item) == list: + assert(array_schema is not None) + validate_array(item, array_schema, exclusion_list) def validate_object(content: Dict[str, Any], schema: Dict[str, Any], exclusion_list: List[str]) -> None: for key, value in content.items(): + object_schema: Optional[Dict[str, Any]] = None + array_schema: Optional[Dict[str, Any]] = None if key in exclusion_list: continue # Check that the key is defined in the schema @@ -299,8 +335,17 @@ def validate_object(content: Dict[str, Any], schema: Dict[str, Any], exclusion_l if 'oneOf' in schema['properties'][key]: for types in schema['properties'][key]['oneOf']: expected_type.append(to_python_type(types['type'])) + if types['type'] == 'object': + object_schema = types + elif types['type'] == 'array': + array_schema = types else: expected_type.append(to_python_type(schema['properties'][key]['type'])) + if schema['properties'][key]['type'] == 'object': + object_schema = schema['properties'][key] + elif schema['properties'][key]['type'] == 'array': + array_schema = schema['properties'][key] + actual_type = type(value) # We have only define nullable property if it is nullable if value is None and 'nullable' in schema['properties'][key]: @@ -308,14 +353,17 @@ def validate_object(content: Dict[str, Any], schema: Dict[str, Any], exclusion_l if actual_type not in expected_type: raise SchemaError('Expected type {} for key "{}", but actually ' 'got {}'.format(expected_type, key, actual_type)) - if expected_type is list: - validate_array(value, schema['properties'][key], exclusion_list) - if 'properties' in schema['properties'][key]: - validate_object(value, schema['properties'][key], exclusion_list) - continue + if actual_type == list: + assert array_schema is not None + validate_array(value, array_schema, exclusion_list) + if actual_type == dict: + assert object_schema is not None + if 'properties' in object_schema: + validate_object(value, object_schema, exclusion_list) + continue if 'additionalProperties' in schema['properties'][key]: for child_keys in value: - if type(value[child_keys]) is list: + if type(value[child_keys]) == list: validate_array(value[child_keys], schema['properties'][key]['additionalProperties'], exclusion_list) continue @@ -324,7 +372,7 @@ def validate_object(content: Dict[str, Any], schema: Dict[str, Any], exclusion_l continue # If the object is not opaque then continue statements # will be executed above and this will be skipped - if expected_type is dict: + if actual_type == dict: raise SchemaError(f'Opaque object "{key}"') # Check that at least all the required keys are present if 'required' in schema: diff --git a/zerver/openapi/testing.yaml b/zerver/openapi/testing.yaml new file mode 100644 index 0000000000..beec44dbf4 --- /dev/null +++ b/zerver/openapi/testing.yaml @@ -0,0 +1,136 @@ +test1: + responses: + '200': + content: + application/json: + schema: + properties: + top_array: + type: array + items: + oneOf: + - type: object + properties: + obj: + oneOf: + - type: array + items: + type: string + - type: object + properties: + str3: + type: string + - type: array + items: + type: object + properties: + str1: + type: string + str2: + type: string + example: + { + "top_array": [ + { + "obj": { + "str3": "test" + } + }, + [ + { + "str1": "success", + "str2": "success" + } + ] + ] + } +test2: + responses: + '200': + content: + application/json: + schema: + properties: + top_array: + type: array + items: + oneOf: + - type: object + properties: + obj: + oneOf: + - type: array + items: + type: string + - type: object + properties: + str3: + type: string + - type: array + items: + type: object + properties: + str1: + type: string + str2: + type: string + example: + { + "top_array": [ + { + "obj": { + "str3": "test", + "str4": "extraneous" + } + }, + [ + { + "str1": "success", + "str2": "success" + } + ] + ] + } +test3: + responses: + '200': + content: + application/json: + schema: + properties: + top_array: + type: array + items: + oneOf: + - type: object + properties: + obj: + oneOf: + - type: array + items: + type: string + - type: object + - type: array + items: + type: object + properties: + str1: + type: string + str2: + type: string + example: + { + "top_array": [ + { + "obj": { + "str3": "test" + } + }, + [ + { + "str1": "success", + "str2": "success" + } + ] + ] + } diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index b923fe2ef3..2133333af7 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -386,9 +386,13 @@ paths: description: | Size of the file in bytes. create_time: - type: integer + # TODO: Make this value always return integers. + oneOf: + - type: integer + - type: number description: | - Time when the attachment was uploaded. + Time when the attachment was uploaded as a UNIX timestamp + multiplied by 1000 (matching the format of getTime() in JavaScript). messages: type: array description: | @@ -593,8 +597,12 @@ paths: properties: avatar_url: type: string + nullable: true description: | - The URL of the user's avatar. + The URL of the user's avatar. Can be null only if client_gravatar was passed, + which means that the user has not uploaded an avatar in Zulip, and the + client should compute the gravatar URL by hashing the + user's email address itself for this user. client: type: string description: | @@ -643,9 +651,15 @@ paths: type: object properties: emoji_code: - type: integer + oneOf: + - type: string + - type: integer description: | - An encoded version of the emoji's unicode codepoint. + A unique identifier, defining the specific emoji codepoint requested, + within the namespace of the `reaction_type`. + + For example, for `unicode_emoji`, this will be an encoding of the + unicode codepoint. emoji_name: type: string description: | @@ -653,8 +667,14 @@ paths: reaction_type: type: string description: | - If the reaction uses a [custom emoji](/help/add-custom-emoji), - `reaction_type` will be set to `realm_emoji`. + One of the following values: + + * `unicode_emoji`: Unicode emoji (`emoji_code` will be its unicode + codepoint). + * `realm_emoji`: [Custom emoji](/help/add-custom-emoji). + (`emoji_code` will be its ID). + * `zulip_extra_emoji`: Special emoji included with Zulip. Exists to + namespace the `zulip` emoji. user_id: type: integer description: | @@ -2425,6 +2445,8 @@ paths: Intended to help clients determine whether they need to display UI like the "more topics" widget that would suggest the stream has older history that can be accessed. + + Null is used for streams with no message history. stream_weekly_traffic: type: integer nullable: true @@ -3918,12 +3940,15 @@ paths: assumption, as we may change that behavior in the future. first_message_id: type: integer + nullable: true description: | The id of the first message in the stream. Intended to help clients determine whether they need to display UI like the "more topics" widget that would suggest the stream has older history that can be accessed. + + Null is used for streams with no message history. is_announcement_only: type: boolean description: | @@ -5053,7 +5078,7 @@ components: name: emoji_code in: query description: | - A unique ID, defining the specific emoji codepoint requested, + A unique identifier, defining the specific emoji codepoint requested, within the namespace of the `reaction_type`. For most API clients, you won't need this, but it's important diff --git a/zerver/tests/test_openapi.py b/zerver/tests/test_openapi.py index da64ecb30f..6fd05f3ea9 100644 --- a/zerver/tests/test_openapi.py +++ b/zerver/tests/test_openapi.py @@ -1,4 +1,5 @@ import inspect +import os import re import sys from collections import abc @@ -18,6 +19,7 @@ from typing import ( from unittest import mock from unittest.mock import MagicMock, patch +import yaml from django.http import HttpResponse from zerver.lib.request import _REQ, arguments_map @@ -163,6 +165,32 @@ class OpenAPIToolsTest(ZulipTestCase): TEST_RESPONSE_SUCCESS) finally: openapi.EXCLUDE_PROPERTIES = exclude_properties + test_dict: Dict[str, Any] = {} + + # Check that validate_against_openapi_schema correctly + # descends into 'deep' objects and arrays. Test 1 should + # pass, Test 2 has a 'deep' extraneous key and Test 3 has a + # 'deep' opaque object. Also the parameters are a heterogenous + # mix of arrays and objects to verify that our descent logic + # correctly gets to the the deeply nested objects. + 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 + try: + validate_against_openapi_schema((test_dict['test1']['responses']['200']['content'] + ['application/json']['example']), + 'testing', 'test1', '200') + with self.assertRaises(SchemaError, msg = 'Extraneous key "str4" in response\'s content'): + validate_against_openapi_schema((test_dict['test2']['responses']['200'] + ['content']['application/json']['example']), + 'testing', 'test2', '200') + with self.assertRaises(SchemaError, msg = 'Opaque object "obj"'): + validate_against_openapi_schema((test_dict['test3']['responses']['200'] + ['content']['application/json']['example']), + 'testing', 'test3', '200') + finally: + openapi_spec.spec()['paths'].pop('testing', None) def test_to_python_type(self) -> None: TYPES = {