diff --git a/zerver/lib/request.py b/zerver/lib/request.py index 63b0c63c5a..33a975b6eb 100644 --- a/zerver/lib/request.py +++ b/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: diff --git a/zerver/lib/request.pyi b/zerver/lib/request.pyi index 040c1321a4..bf30005e7b 100644 --- a/zerver/lib/request.pyi +++ b/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]] diff --git a/zerver/tests/test_openapi.py b/zerver/tests/test_openapi.py index 1ecd51ec4a..ba80ea3a50 100644 --- a/zerver/tests/test_openapi.py +++ b/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[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)