openapi: Add validation of parameter lists against actual code.

This validation is incomplete, in large part because of the long list
of TODOs in this code.  But this test should provide a ton of support
for us in avoiding regressions as we work towards having complete API
documentation.

See https://github.com/zulip/zulip/issues/12521 for a bunch of
follow-up improvements.

zerver/lib/request.py

@@ -5,6 +5,7 @@
 #
 # Because request.pyi exists, the type annotations in this file are
 # mostly not processed by mypy.
+from collections import defaultdict
 from functools import wraps
 import ujson
 
@@ -16,7 +17,7 @@ from zerver.lib.types import ViewFuncT
 
 from django.http import HttpRequest, HttpResponse
 
-from typing import Any, Callable, List, Optional, Type
+from typing import Any, Callable, Dict, List, Optional, Type
 
 class RequestConfusingParmsError(JsonableError):
     code = ErrorCode.REQUEST_CONFUSING_VAR
@@ -110,6 +111,8 @@ class REQ:
             # Not user-facing, so shouldn't be tagged for translation
             raise AssertionError('validator and str_validator are mutually exclusive')
 
+arguments_map = defaultdict(list)  # type: Dict[str, List[str]]
+
 # Extracts variables from the request object and passes them as
 # named function arguments.  The request object must be the first
 # argument to the function.
@@ -139,12 +142,15 @@ def has_request_variables(view_func: ViewFuncT) -> ViewFuncT:
 
     post_params = []
 
+    view_func_full_name = '.'.join([view_func.__module__, view_func.__name__])
+
     for (name, value) in zip(default_param_names, default_param_values):
         if isinstance(value, REQ):
             value.func_var_name = name
             if value.post_var_name is None:
                 value.post_var_name = name
             post_params.append(value)
+            arguments_map[view_func_full_name].append(value.post_var_name)
 
     @wraps(view_func)
     def _wrapped_view_func(request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponse:

zerver/lib/request.pyi

@@ -6,7 +6,7 @@
 # scan the parameter list for REQ objects and patch the parameters as the true
 # types.
 
-from typing import Any, List, Callable, TypeVar, Optional, Union, Type
+from typing import Any, Dict, Callable, List, TypeVar, Optional, Union, Type
 from zerver.lib.types import ViewFuncT, Validator, ExtractRecipients
 from zerver.lib.exceptions import JsonableError as JsonableError
 
@@ -30,3 +30,4 @@ def REQ(whence: Optional[str] = None,
         aliases: Optional[List[str]] = None) -> ResultT: ...
 
 def has_request_variables(view_func: ViewFuncT) -> ViewFuncT: ...
+arguments_map = ...  # type: Dict[str, List[str]]

zerver/tests/test_openapi.py

@@ -3,12 +3,15 @@
 import mock
 from typing import Dict, Any
 
+from django.conf import settings
+
 import zerver.lib.openapi as openapi
 from zerver.lib.test_classes import ZulipTestCase
 from zerver.lib.openapi import (
     get_openapi_fixture, get_openapi_parameters,
     validate_against_openapi_schema, to_python_type, SchemaError, openapi_spec
 )
+from zerver.lib.request import arguments_map
 
 TEST_ENDPOINT = '/messages/{message_id}'
 TEST_METHOD = 'patch'
@@ -138,3 +141,176 @@ class OpenAPIToolsTest(ZulipTestCase):
         with mock.patch('zerver.lib.openapi.openapi_spec.reload') as mock_reload:
             get_openapi_fixture(TEST_ENDPOINT, TEST_METHOD)
             self.assertFalse(mock_reload.called)
+
+class OpenAPIArgumentsTest(ZulipTestCase):
+    def test_openapi_arguments(self) -> None:
+        # Verifies that every REQ-defined argument appears in our API
+        # documentation for the target endpoint where possible.
+
+        # TODO: Potentially this should move into configuration flags
+        # we attach to the endpoint in zproject/urls.py, to indicate
+        # it's an endpoint with no documentation by design.
+        NO_API_DOCS = set([
+            # These endpoints are for webapp-only use and will likely
+            # never have docs.
+            '/report/narrow_times',
+            '/report/send_times',
+            '/report/unnarrow_times',
+            '/report/error',
+            '/users/me/tutorial_status',
+            '/users/me/hotspots',
+            # These should probably be eliminated
+            '/users/me/enter-sends',
+            # These should have docs added
+            '/users/me/avatar',
+            '/user_uploads',
+            '/settings/display',
+            '/settings/notifications',
+            '/users/me/profile_data',
+            '/user_groups',
+            '/user_groups/create',
+            '/users/me/pointer',
+            '/users/me/presence',
+            '/users/me',
+            '/bot_storage',
+            '/users/me/api_key/regenerate',
+            '/default_streams',
+            '/default_stream_groups/create',
+            '/users/me/alert_words',
+            '/users/me/status',
+            '/users/me/subscriptions',
+            '/messages/matches_narrow',
+            '/settings',
+            '/submessage',
+            '/attachments',
+            '/calls/create',
+            '/export/realm',
+            '/mark_all_as_read',
+            '/zcommand',
+            '/realm',
+            '/realm/deactivate',
+            '/realm/domains',
+            '/realm/emoji',
+            '/realm/filters',
+            '/realm/icon',
+            '/realm/logo',
+            '/realm/presence',
+            '/realm/profile_fields',
+            '/queue_id',
+            '/invites',
+            '/invites/multiuse',
+            '/bots',
+            # Mobile-app only endpoints
+            '/users/me/android_gcm_reg_id',
+            '/users/me/apns_device_token',
+        ])
+
+        # These endpoints have a mismatch between the documentation
+        # and the actual API.  There are situations where we may want
+        # to have undocumented parameters for e.g. backwards
+        # compatibility, which could be the situation for some of
+        # these, in which case we may want a more clever exclude
+        # system.  This list can serve as a TODO list for such an
+        # investigation.
+        BUGGY_DOCUMENTATION_ENDPOINTS = set([
+            '/events',
+            '/register',
+            '/mark_stream_as_read',
+            '/mark_topic_as_read',
+            '/messages',
+            '/messages/flags',
+            '/messages/render',
+            '/typing',
+            '/users/me/subscriptions/muted_topics',
+        ])
+
+        # First, we import the fancy-Django version of zproject/urls.py
+        urlconf = __import__(getattr(settings, "ROOT_URLCONF"), {}, {}, [''])
+
+        # We loop through all the API patterns, looking in particular
+        # those using the rest_dispatch decorator; we then parse its
+        # mapping of (HTTP_METHOD -> FUNCTION).
+        for p in urlconf.v1_api_and_json_patterns:
+            if p.lookup_str != 'zerver.lib.rest.rest_dispatch':
+                continue
+            for method, value in p.default_args.items():
+                if isinstance(value, str):
+                    function = value
+                else:
+                    function = value[0]
+                # Our accounting logic in the `has_request_variables()`
+                # code means we have the list of all arguments
+                # accepted by every view function in arguments_map.
+                #
+                # TODO: Probably with a bit more work, we could get
+                # the types, too; `check_int` -> `int`, etc., and
+                # verify those too!
+                accepted_arguments = set(arguments_map[function])
+
+                # TODO: The purpose of this block is to match our URL
+                # pattern regular expressions to the corresponding
+                # configuration in OpenAPI.  The means matching
+                #
+                # /messages/{message_id} <-> r'^messages/(?P<message_id>[0-9]+)$'
+                # /events <-> r'^events$'
+                #
+                # The below is a giant hack that only handles the simple case.
+                regex_pattern = p.regex.pattern
+                self.assertTrue(regex_pattern.startswith("^"))
+                self.assertTrue(regex_pattern.endswith("$"))
+                # Obviously this is a huge hack and won't work for
+                # some URLs.
+                url_pattern = '/' + regex_pattern[1:][:-1]
+
+                if url_pattern in NO_API_DOCS:
+                    # Don't do any validation on endpoints with no API
+                    # documentation by design.
+                    continue
+                if "(?P<" in url_pattern:
+                    # See above TODO about our matching algorithm not
+                    # handling captures in the regular expressions.
+                    continue
+
+                try:
+                    openapi_parameters = get_openapi_parameters(url_pattern, method)
+                    # TODO: Record which entries in the OpenAPI file
+                    # found a match, and throw an error for any that
+                    # unexpectedly didn't.
+                except Exception:  # nocoverage
+                    raise AssertionError("Could not find OpenAPI docs for %s %s" %
+                                         (method, url_pattern))
+
+                # We now have everything we need to understand the
+                # function as defined in our urls.py
+                #
+                # * method is the HTTP method, e.g. GET, POST, or PATCH
+                #
+                # * p.regex.pattern is the URL pattern; might require
+                #   some processing to match with OpenAPI rules
+                #
+                # * accepted_arguments_list is the full set of arguments
+                #   this method accepts.
+                #
+                # * The documented parameters for the endpoint as recorded in our
+                #   OpenAPI data in zerver/openapi/zulip.yaml.
+                #
+                # We now compare these to confirm that the documented
+                # argument list matches what actually appears in the
+                # codebase.
+
+                openapi_parameter_names = set(
+                    [parameter['name'] for parameter in openapi_parameters]
+                )
+
+                if len(openapi_parameter_names - accepted_arguments) > 0:
+                    print("Undocumented parameters for", url_pattern, method, function)
+                    print(" +", openapi_parameter_names)
+                    print(" -", accepted_arguments)
+                    assert(url_pattern in BUGGY_DOCUMENTATION_ENDPOINTS)
+                elif len(accepted_arguments - openapi_parameter_names) > 0:
+                    print("Documented invalid parameters for", url_pattern, method, function)
+                    print(" -", openapi_parameter_names)
+                    print(" +", accepted_arguments)
+                    assert(url_pattern in BUGGY_DOCUMENTATION_ENDPOINTS)
+                else:
+                    self.assertEqual(openapi_parameter_names, accepted_arguments)