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

@@ -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']:
+        for oneof_schema in schema['items']['oneOf']:
-            valid_types.append(to_python_type(valid_type['type']))
+            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 type(item) == dict:
-            if 'oneOf' not in schema['items']:
+            assert object_schema is not None
-                if 'properties' not in schema['items']:
+            if 'properties' not in object_schema:
-                    raise SchemaError('Opaque object in array')
+                raise SchemaError('Opaque object in array')
-                validate_object(item, schema['items'], exclusion_list)
+            validate_object(item, object_schema, exclusion_list)
-            continue
+        if type(item) == list:
-        # If the object was not an opaque object then
+            assert(array_schema is not None)
-        # the continue statement above should have
+            validate_array(item, array_schema, exclusion_list)
-        # 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)
 
 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:
+        if actual_type == list:
-            validate_array(value, schema['properties'][key], exclusion_list)
+            assert array_schema is not None
-        if 'properties' in schema['properties'][key]:
+            validate_array(value, array_schema, exclusion_list)
-            validate_object(value, schema['properties'][key], exclusion_list)
+        if actual_type == dict:
-            continue
+            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:

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"
+                         }
+                     ]
+                 ]
+             }

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),
+                                    One of the following values:
-                                    `reaction_type` will be set to `realm_emoji`.
+
+                                    * `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

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 = {