From b12d3d54c6a5839dfa6fa222e5fac9c13da024a3 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Mon, 21 Oct 2019 16:43:54 -0700 Subject: [PATCH] events: Fix documentation testing for /events. Most of the failures were due to parameters that are not intended to be used by third-party code, so the correct fix for those was the set intentionally_undocumented=True. Fixes #12969. --- zerver/lib/request.py | 6 ------ zerver/tests/test_openapi.py | 20 +++++++++++++++----- zerver/tornado/views.py | 31 ++++++++++++++++++++++--------- 3 files changed, 37 insertions(+), 20 deletions(-) diff --git a/zerver/lib/request.py b/zerver/lib/request.py index 5cc0644489..951d1225e2 100644 --- a/zerver/lib/request.py +++ b/zerver/lib/request.py @@ -188,12 +188,6 @@ def has_request_variables(view_func: ViewFuncT) -> ViewFuncT: post_params = [] view_func_full_name = '.'.join([view_func.__module__, view_func.__name__]) - if view_func_full_name == 'zerver.tornado.views.get_events_backend': - # Hack to help test_openapi find the has_request_variables - # data for get_events. This is complex because of how - # get_events_backend, which parses the parameters, is wrapped - # by multiple callers that don't parse the parameters. - view_func_full_name = 'zerver.tornado.views.get_events' for (name, value) in zip(default_param_names, default_param_values): if isinstance(value, _REQ): diff --git a/zerver/tests/test_openapi.py b/zerver/tests/test_openapi.py index 46e1d4b69c..b59ec2adc2 100644 --- a/zerver/tests/test_openapi.py +++ b/zerver/tests/test_openapi.py @@ -232,11 +232,11 @@ class OpenAPIArgumentsTest(ZulipTestCase): '/user_groups/{group_id}', # Equivalent of what's in urls.py '/user_groups/{user_group_id}', # What's in the OpenAPI docs ]) - # TODO: These endpoints have a mismatch between the - # documentation and the actual API and need to be fixed: + + # Endpoints where the documentation is currently failing our + # consistency tests. We aim to keep this list empty. buggy_documentation_endpoints = set([ - '/events', - ]) + ]) # type: Set[str] def convert_regex_to_url_pattern(self, regex_pattern: str) -> str: """ Convert regular expressions style URL patterns to their @@ -481,6 +481,16 @@ do not match the types declared in the implementation of {}.\n""".format(functio else: function_name, tags = value + if function_name == 'zerver.tornado.views.get_events': + # Work around the fact that the registered + # get_events view function isn't where we do + # @has_request_variables. + # + # TODO: Make this configurable via an optional argument + # to has_request_variables, e.g. + # @has_request_variables(view_func_name="zerver.tornado.views.get_events") + function_name = 'zerver.tornado.views.get_events_backend' + lookup_parts = function_name.split('.') module = __import__('.'.join(lookup_parts[:-1]), {}, {}, ['']) function = getattr(module, lookup_parts[-1]) @@ -540,7 +550,7 @@ so maybe we shouldn't include it in pending_endpoints. [parameter['name'] for parameter in openapi_parameters] ) - if len(accepted_arguments - openapi_parameter_names) > 0: + if len(accepted_arguments - openapi_parameter_names) > 0: # nocoverage print("Undocumented parameters for", url_pattern, method, function_name) print(" +", openapi_parameter_names) diff --git a/zerver/tornado/views.py b/zerver/tornado/views.py index 6bf2c6a8e6..66c12f1b60 100644 --- a/zerver/tornado/views.py +++ b/zerver/tornado/views.py @@ -1,5 +1,5 @@ import time -from typing import Iterable, List, Optional, Sequence, Union +from typing import Iterable, Optional, Sequence, Union import ujson from django.core.handlers.base import BaseHandler @@ -50,16 +50,29 @@ def get_events(request: HttpRequest, user_profile: UserProfile, @has_request_variables def get_events_backend(request: HttpRequest, user_profile: UserProfile, handler: BaseHandler, - user_client: Optional[Client]=REQ(converter=get_client, default=None), + # user_client is intended only for internal Django=>Tornado requests + # and thus shouldn't be documented for external use. + user_client: Optional[Client]=REQ(converter=get_client, default=None, + intentionally_undocumented=True), last_event_id: Optional[int]=REQ(converter=int, default=None), - queue_id: Optional[List[str]]=REQ(default=None), - apply_markdown: bool=REQ(default=False, validator=check_bool), - client_gravatar: bool=REQ(default=False, validator=check_bool), - all_public_streams: bool=REQ(default=False, validator=check_bool), - event_types: Optional[str]=REQ(default=None, validator=check_list(check_string)), + queue_id: Optional[str]=REQ(default=None), + # apply_markdown, client_gravatar, all_public_streams, and various + # other parameters are only used when registering a new queue via this + # endpoint. This is a feature used primarily by get_events_internal + # and not expected to be used by third-party clients. + apply_markdown: bool=REQ(default=False, validator=check_bool, + intentionally_undocumented=True), + client_gravatar: bool=REQ(default=False, validator=check_bool, + intentionally_undocumented=True), + all_public_streams: bool=REQ(default=False, validator=check_bool, + intentionally_undocumented=True), + event_types: Optional[str]=REQ(default=None, validator=check_list(check_string), + intentionally_undocumented=True), dont_block: bool=REQ(default=False, validator=check_bool), - narrow: Iterable[Sequence[str]]=REQ(default=[], validator=check_list(None)), - lifespan_secs: int=REQ(default=0, converter=to_non_negative_int) + narrow: Iterable[Sequence[str]]=REQ(default=[], validator=check_list(None), + intentionally_undocumented=True), + lifespan_secs: int=REQ(default=0, converter=to_non_negative_int, + intentionally_undocumented=True) ) -> Union[HttpResponse, _RespondAsynchronously]: if user_client is None: valid_user_client = request.client