From 2738b09044ef7e692cc818643b6d6b9cad29c72c Mon Sep 17 00:00:00 2001 From: "Hemanth V. Alluri" Date: Thu, 4 Jul 2019 11:45:10 +0530 Subject: [PATCH] openapi: Remove a few "buggy" endpoints in the validation test. By importing a few view modules in the validation test itself we can remove a few endpoints which were marked as buggy. What was happening was that the view functions weren't imported and hence the arguments map was not filled. Thus the test complained that there was documentation for request parameters that seemed to be missing in the code. Also, for the events register endpoint, we have renamed one of the documented request parameters from "stream" to "topic" (the API itself was not modified though). We add a new "documentation_pending" attribute to req variables so that any arguments not currently documented but should be documented can be properly accounted for. --- zerver/lib/request.py | 4 +++- zerver/lib/request.pyi | 1 + zerver/openapi/zulip.yaml | 2 +- zerver/tests/test_openapi.py | 11 ++++++----- zerver/views/events_register.py | 4 ++-- zerver/views/messages.py | 25 +++++++++++++++++-------- 6 files changed, 30 insertions(+), 17 deletions(-) diff --git a/zerver/lib/request.py b/zerver/lib/request.py index 53f8183d8f..f9480ef065 100644 --- a/zerver/lib/request.py +++ b/zerver/lib/request.py @@ -68,6 +68,7 @@ class REQ: str_validator: Callable[[Any], Any]=None, argument_type: str=None, type: Type=None, intentionally_undocumented=False, + documentation_pending=False, aliases: Optional[List[str]]=None) -> None: """whence: the name of the request variable that should be used for this parameter. Defaults to a request variable of the @@ -105,6 +106,7 @@ class REQ: self.argument_type = argument_type self.aliases = aliases self.intentionally_undocumented = intentionally_undocumented + self.documentation_pending = documentation_pending if converter and (validator or str_validator): # Not user-facing, so shouldn't be tagged for translation @@ -155,7 +157,7 @@ def has_request_variables(view_func: ViewFuncT) -> ViewFuncT: # Record arguments that should be documented so that our # automated OpenAPI docs tests can compare these against the code. - if not value.intentionally_undocumented: + if not value.intentionally_undocumented and not value.documentation_pending: arguments_map[view_func_full_name].append(value.post_var_name) @wraps(view_func) diff --git a/zerver/lib/request.pyi b/zerver/lib/request.pyi index 29267bf937..0785161366 100644 --- a/zerver/lib/request.pyi +++ b/zerver/lib/request.pyi @@ -28,6 +28,7 @@ def REQ(whence: Optional[str] = None, str_validator: Optional[Validator] = None, argument_type: Optional[str] = None, intentionally_undocumented: bool=False, + documentation_pending: bool=False, aliases: Optional[List[str]] = None) -> ResultT: ... def has_request_variables(view_func: ViewFuncT) -> ViewFuncT: ... diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 4fc4cd3477..465843adaa 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -499,7 +499,7 @@ paths: type: string example: aaron@zulip.com, iago@zulip.com required: true - - name: subject + - name: topic in: query description: The topic of the message. Only required if `type` is `stream`, ignored otherwise. Maximum length of 60 characters. diff --git a/zerver/tests/test_openapi.py b/zerver/tests/test_openapi.py index beebbca517..6fba96762d 100644 --- a/zerver/tests/test_openapi.py +++ b/zerver/tests/test_openapi.py @@ -201,14 +201,17 @@ class OpenAPIArgumentsTest(ZulipTestCase): # investigation. BUGGY_DOCUMENTATION_ENDPOINTS = set([ '/events', - '/register', - '/messages', - '/typing', '/users/me/subscriptions/muted_topics', ]) # First, we import the fancy-Django version of zproject/urls.py urlconf = __import__(getattr(settings, "ROOT_URLCONF"), {}, {}, ['']) + # Import some view modules not already imported in urls.py, we use + # this round about manner because of the linters complaining of an + # unused import (which is correct, but we do this for triggering the + # has_request_variables decorator). + __import__('zerver.views.typing') + __import__('zerver.views.events_register') # We loop through all the API patterns, looking in particular # those using the rest_dispatch decorator; we then parse its @@ -251,8 +254,6 @@ class OpenAPIArgumentsTest(ZulipTestCase): # we should remove this block and the associated List. continue if "intentionally_undocumented" in tags: - # Don't do any validation on endpoints with no API - # documentation by design. try: get_openapi_parameters(url_pattern, method) raise AssertionError("We found some OpenAPI \ diff --git a/zerver/views/events_register.py b/zerver/views/events_register.py index 6edac62567..004380472f 100644 --- a/zerver/views/events_register.py +++ b/zerver/views/events_register.py @@ -32,11 +32,11 @@ def events_register_backend( include_subscribers: bool=REQ(default=False, validator=check_bool), client_capabilities: Optional[Dict[str, bool]]=REQ(validator=check_dict([ ("notification_settings_null", check_bool), - ]), default=None), + ]), default=None, documentation_pending=True), event_types: Optional[Iterable[str]]=REQ(validator=check_list(check_string), default=None), fetch_event_types: Optional[Iterable[str]]=REQ(validator=check_list(check_string), default=None), narrow: NarrowT=REQ(validator=check_list(check_list(check_string, length=2)), default=[]), - queue_lifespan_secs: int=REQ(converter=int, default=0) + queue_lifespan_secs: int=REQ(converter=int, default=0, documentation_pending=True) ) -> HttpResponse: all_public_streams = _default_all_public_streams(user_profile, all_public_streams) narrow = _default_narrow(user_profile, narrow) diff --git a/zerver/views/messages.py b/zerver/views/messages.py index 3f05d8230c..6339aea809 100644 --- a/zerver/views/messages.py +++ b/zerver/views/messages.py @@ -1238,16 +1238,25 @@ def send_message_backend(request: HttpRequest, user_profile: UserProfile, message_type_name: str=REQ('type'), message_to: Union[Sequence[int], Sequence[str]]=REQ( 'to', converter=extract_recipients, default=[]), - forged: bool=REQ(default=False), + forged: bool=REQ(default=False, + documentation_pending=True), topic_name: Optional[str]=REQ_topic(), message_content: str=REQ('content'), - widget_content: Optional[str]=REQ(default=None), - realm_str: Optional[str]=REQ('realm_str', default=None), - local_id: Optional[str]=REQ(default=None), - queue_id: Optional[str]=REQ(default=None), - delivery_type: Optional[str]=REQ('delivery_type', default='send_now'), - defer_until: Optional[str]=REQ('deliver_at', default=None), - tz_guess: Optional[str]=REQ('tz_guess', default=None)) -> HttpResponse: + widget_content: Optional[str]=REQ(default=None, + documentation_pending=True), + realm_str: Optional[str]=REQ('realm_str', default=None, + documentation_pending=True), + local_id: Optional[str]=REQ(default=None, + documentation_pending=True), + queue_id: Optional[str]=REQ(default=None, + documentation_pending=True), + delivery_type: Optional[str]=REQ('delivery_type', default='send_now', + documentation_pending=True), + defer_until: Optional[str]=REQ('deliver_at', default=None, + documentation_pending=True), + tz_guess: Optional[str]=REQ('tz_guess', default=None, + documentation_pending=True) + ) -> HttpResponse: client = request.client is_super_user = request.user.is_api_super_user if forged and not is_super_user: