mirror of
https://github.com/zulip/zulip.git
synced 2025-11-14 10:57:58 +00:00
openapi: Refactor OpenAPIArgumentsTest.
The main things targeted by the refactor are the usage of comments and moving the top-level variables to the scope of the class. The movement of variables was to facilitate allowing us to perform a reverse mapping test from OpenAPI URLs -> Code defined URLs.
This commit is contained in:
committed by
Tim Abbott
parent
74a72fc422
commit
718744c22d
@@ -144,114 +144,112 @@ class OpenAPIToolsTest(ZulipTestCase):
|
|||||||
self.assertFalse(mock_reload.called)
|
self.assertFalse(mock_reload.called)
|
||||||
|
|
||||||
class OpenAPIArgumentsTest(ZulipTestCase):
|
class OpenAPIArgumentsTest(ZulipTestCase):
|
||||||
|
# This will be filled during test_openapi_arguments:
|
||||||
|
checked_endpoints = set() # type: Set[str]
|
||||||
|
# TODO: These endpoints need to be documented:
|
||||||
|
pending_endpoints = set([
|
||||||
|
'/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',
|
||||||
|
# Regex based urls
|
||||||
|
'/realm/domains/<domain>',
|
||||||
|
'/realm/filters/<filter_id>',
|
||||||
|
'/realm/profile_fields/<field_id>',
|
||||||
|
'/users/<user_id>/reactivate',
|
||||||
|
'/users/<user_id>',
|
||||||
|
'/bots/<bot_id>/api_key/regenerate',
|
||||||
|
'/bots/<bot_id>',
|
||||||
|
'/invites/<prereg_id>',
|
||||||
|
'/invites/<prereg_id>/resend',
|
||||||
|
'/invites/multiuse/<invite_id>',
|
||||||
|
'/messages/<message_id>',
|
||||||
|
'/messages/<message_id>/history',
|
||||||
|
'/users/me/subscriptions/<stream_id>',
|
||||||
|
'/messages/<message_id>/reactions',
|
||||||
|
'/messages/<message_id>/emoji_reactions/<emoji_name>',
|
||||||
|
'/attachments/<attachment_id>',
|
||||||
|
'/user_groups/<user_group_id>',
|
||||||
|
'/user_groups/<user_group_id>/members',
|
||||||
|
'/users/me/<stream_id>/topics',
|
||||||
|
'/streams/<stream_id>/members',
|
||||||
|
'/streams/<stream_id>',
|
||||||
|
'/streams/<stream_id>/delete_topic',
|
||||||
|
'/default_stream_groups/<group_id>',
|
||||||
|
'/default_stream_groups/<group_id>/streams',
|
||||||
|
# Regex with an unnamed capturing group.
|
||||||
|
'/users/(?!me/)(?P<email>[^/]*)/presence',
|
||||||
|
])
|
||||||
|
# TODO: These endpoints have a mismatch between the
|
||||||
|
# documentation and the actual API and need to be fixed:
|
||||||
|
buggy_documentation_endpoints = set([
|
||||||
|
'/events',
|
||||||
|
'/users/me/subscriptions/muted_topics',
|
||||||
|
])
|
||||||
|
|
||||||
def test_openapi_arguments(self) -> None:
|
def test_openapi_arguments(self) -> None:
|
||||||
# Verifies that every REQ-defined argument appears in our API
|
"""This end-to-end API documentation test compares the arguments
|
||||||
# documentation for the target endpoint where possible.
|
defined in the actual code using @has_request_variables and
|
||||||
|
REQ(), with the arguments declared in our API documentation
|
||||||
|
for every API endpoint in Zulip.
|
||||||
|
|
||||||
# These should have docs added
|
First, we import the fancy-Django version of zproject/urls.py
|
||||||
PENDING_ENDPOINTS = set([
|
by doing this, each has_request_variables wrapper around each
|
||||||
'/users/me/avatar',
|
imported view function gets called to generate the wrapped
|
||||||
'/user_uploads',
|
view function and thus filling the global arguments_map variable.
|
||||||
'/settings/display',
|
Basically, we're exploiting code execution during import.
|
||||||
'/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',
|
|
||||||
# Regex based urls
|
|
||||||
'/realm/domains/<domain>',
|
|
||||||
'/realm/filters/<filter_id>',
|
|
||||||
'/realm/profile_fields/<field_id>',
|
|
||||||
'/users/<user_id>/reactivate',
|
|
||||||
'/users/<user_id>',
|
|
||||||
'/bots/<bot_id>/api_key/regenerate',
|
|
||||||
'/bots/<bot_id>',
|
|
||||||
'/invites/<prereg_id>',
|
|
||||||
'/invites/<prereg_id>/resend',
|
|
||||||
'/invites/multiuse/<invite_id>',
|
|
||||||
'/messages/<message_id>',
|
|
||||||
'/messages/<message_id>',
|
|
||||||
'/messages/<message_id>',
|
|
||||||
'/messages/<message_id>/history',
|
|
||||||
'/users/me/subscriptions/<stream_id>',
|
|
||||||
'/messages/<message_id>/reactions',
|
|
||||||
'/messages/<message_id>/reactions',
|
|
||||||
'/messages/<message_id>/emoji_reactions/<emoji_name>',
|
|
||||||
'/messages/<message_id>/emoji_reactions/<emoji_name>',
|
|
||||||
'/attachments/<attachment_id>',
|
|
||||||
'/user_groups/<user_group_id>',
|
|
||||||
'/user_groups/<user_group_id>',
|
|
||||||
'/user_groups/<user_group_id>/members',
|
|
||||||
'/users/me/<stream_id>/topics',
|
|
||||||
'/streams/<stream_id>/members',
|
|
||||||
'/streams/<stream_id>',
|
|
||||||
'/streams/<stream_id>',
|
|
||||||
'/streams/<stream_id>/delete_topic',
|
|
||||||
'/default_stream_groups/<group_id>',
|
|
||||||
'/default_stream_groups/<group_id>',
|
|
||||||
'/default_stream_groups/<group_id>/streams',
|
|
||||||
# Regex with an unnamed capturing group.
|
|
||||||
'/users/(?!me/)(?P<email>[^/]*)/presence',
|
|
||||||
])
|
|
||||||
|
|
||||||
# These endpoints have a mismatch between the documentation
|
Then we need to import some view modules not already imported in
|
||||||
# and the actual API. There are situations where we may want
|
urls.py. We use this different syntax because of the linters complaining
|
||||||
# to have undocumented parameters for e.g. backwards
|
of an unused import (which is correct, but we do this for triggering the
|
||||||
# compatibility, which could be the situation for some of
|
has_request_variables decorator).
|
||||||
# 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',
|
|
||||||
'/users/me/subscriptions/muted_topics',
|
|
||||||
])
|
|
||||||
|
|
||||||
# First, we import the fancy-Django version of zproject/urls.py
|
|
||||||
urlconf = __import__(getattr(settings, "ROOT_URLCONF"), {}, {}, [''])
|
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.typing')
|
||||||
__import__('zerver.views.events_register')
|
__import__('zerver.views.events_register')
|
||||||
__import__('zerver.views.realm_emoji')
|
__import__('zerver.views.realm_emoji')
|
||||||
|
|
||||||
# We loop through all the API patterns, looking in particular
|
# We loop through all the API patterns, looking in particular
|
||||||
# those using the rest_dispatch decorator; we then parse its
|
# for those using the rest_dispatch decorator; we then parse
|
||||||
# mapping of (HTTP_METHOD -> FUNCTION).
|
# its mapping of (HTTP_METHOD -> FUNCTION).
|
||||||
for p in urlconf.v1_api_and_json_patterns:
|
for p in urlconf.v1_api_and_json_patterns:
|
||||||
if p.lookup_str != 'zerver.lib.rest.rest_dispatch':
|
if p.lookup_str != 'zerver.lib.rest.rest_dispatch':
|
||||||
continue
|
continue
|
||||||
@@ -261,6 +259,7 @@ class OpenAPIArgumentsTest(ZulipTestCase):
|
|||||||
tags = set() # type: Set[str]
|
tags = set() # type: Set[str]
|
||||||
else:
|
else:
|
||||||
function, tags = value
|
function, tags = value
|
||||||
|
|
||||||
# Our accounting logic in the `has_request_variables()`
|
# Our accounting logic in the `has_request_variables()`
|
||||||
# code means we have the list of all arguments
|
# code means we have the list of all arguments
|
||||||
# accepted by every view function in arguments_map.
|
# accepted by every view function in arguments_map.
|
||||||
@@ -270,12 +269,11 @@ class OpenAPIArgumentsTest(ZulipTestCase):
|
|||||||
# verify those too!
|
# verify those too!
|
||||||
accepted_arguments = set(arguments_map[function])
|
accepted_arguments = set(arguments_map[function])
|
||||||
|
|
||||||
# The purpose of this block is to match our URL
|
# Convert regular expressions style URL patterns to their
|
||||||
# pattern regular expressions to the corresponding
|
# corresponding OpenAPI style formats.
|
||||||
# configuration in OpenAPI. The means matching
|
# E.G.
|
||||||
#
|
# /messages/{message_id} <-> r'^messages/(?P<message_id>[0-9]+)$'
|
||||||
# /messages/{message_id} <-> r'^messages/(?P<message_id>[0-9]+)$'
|
# /events <-> r'^events$'
|
||||||
# /events <-> r'^events$'
|
|
||||||
regex_pattern = p.regex.pattern
|
regex_pattern = p.regex.pattern
|
||||||
self.assertTrue(regex_pattern.startswith("^"))
|
self.assertTrue(regex_pattern.startswith("^"))
|
||||||
self.assertTrue(regex_pattern.endswith("$"))
|
self.assertTrue(regex_pattern.endswith("$"))
|
||||||
@@ -286,8 +284,8 @@ class OpenAPIArgumentsTest(ZulipTestCase):
|
|||||||
url_patterns = [re.sub(r"\(\?P<(\w+)>[^/]+\)", r"<\1>", url_pattern),
|
url_patterns = [re.sub(r"\(\?P<(\w+)>[^/]+\)", r"<\1>", url_pattern),
|
||||||
re.sub(r"\(\?P<(\w+)>[^/]+\)", r"{\1}", url_pattern)]
|
re.sub(r"\(\?P<(\w+)>[^/]+\)", r"{\1}", url_pattern)]
|
||||||
|
|
||||||
if any([url_patterns[0] in PENDING_ENDPOINTS,
|
if any([url_patterns[0] in self.pending_endpoints,
|
||||||
url_patterns[1] in PENDING_ENDPOINTS]):
|
url_patterns[1] in self.pending_endpoints]):
|
||||||
continue
|
continue
|
||||||
if "intentionally_undocumented" in tags:
|
if "intentionally_undocumented" in tags:
|
||||||
error = AssertionError("We found some OpenAPI \
|
error = AssertionError("We found some OpenAPI \
|
||||||
@@ -318,15 +316,16 @@ undocumented in the urls." % (method, url_patterns[0] + " or " + url_patterns[1]
|
|||||||
(method, url_patterns[0] + " or " + url_patterns[1]))
|
(method, url_patterns[0] + " or " + url_patterns[1]))
|
||||||
|
|
||||||
# We now have everything we need to understand the
|
# We now have everything we need to understand the
|
||||||
# function as defined in our urls.py
|
# function as defined in our urls.py:
|
||||||
#
|
#
|
||||||
# * method is the HTTP method, e.g. GET, POST, or PATCH
|
# * method is the HTTP method, e.g. GET, POST, or PATCH
|
||||||
#
|
#
|
||||||
# * p.regex.pattern is the URL pattern; might require
|
# * p.regex.pattern is the URL pattern; might require
|
||||||
# some processing to match with OpenAPI rules
|
# some processing to match with OpenAPI rules
|
||||||
#
|
#
|
||||||
# * accepted_arguments_list is the full set of arguments
|
# * accepted_arguments is the full set of arguments
|
||||||
# this method accepts.
|
# this method accepts (from the REQ declarations in
|
||||||
|
# code).
|
||||||
#
|
#
|
||||||
# * The documented parameters for the endpoint as recorded in our
|
# * The documented parameters for the endpoint as recorded in our
|
||||||
# OpenAPI data in zerver/openapi/zulip.yaml.
|
# OpenAPI data in zerver/openapi/zulip.yaml.
|
||||||
@@ -345,15 +344,15 @@ undocumented in the urls." % (method, url_patterns[0] + " or " + url_patterns[1]
|
|||||||
method, function)
|
method, function)
|
||||||
print(" +", openapi_parameter_names)
|
print(" +", openapi_parameter_names)
|
||||||
print(" -", accepted_arguments)
|
print(" -", accepted_arguments)
|
||||||
assert(any([url_patterns[0] in BUGGY_DOCUMENTATION_ENDPOINTS,
|
assert(any([url_patterns[0] in self.buggy_documentation_endpoints,
|
||||||
url_patterns[1] in BUGGY_DOCUMENTATION_ENDPOINTS]))
|
url_patterns[1] in self.buggy_documentation_endpoints]))
|
||||||
elif len(accepted_arguments - openapi_parameter_names) > 0:
|
elif len(accepted_arguments - openapi_parameter_names) > 0:
|
||||||
print("Documented invalid parameters for",
|
print("Documented invalid parameters for",
|
||||||
url_patterns[0] + " or " + url_patterns[1],
|
url_patterns[0] + " or " + url_patterns[1],
|
||||||
method, function)
|
method, function)
|
||||||
print(" -", openapi_parameter_names)
|
print(" -", openapi_parameter_names)
|
||||||
print(" +", accepted_arguments)
|
print(" +", accepted_arguments)
|
||||||
assert(any([url_patterns[0] in BUGGY_DOCUMENTATION_ENDPOINTS,
|
assert(any([url_patterns[0] in self.buggy_documentation_endpoints,
|
||||||
url_patterns[1] in BUGGY_DOCUMENTATION_ENDPOINTS]))
|
url_patterns[1] in self.buggy_documentation_endpoints]))
|
||||||
else:
|
else:
|
||||||
self.assertEqual(openapi_parameter_names, accepted_arguments)
|
self.assertEqual(openapi_parameter_names, accepted_arguments)
|
||||||
|
|||||||
Reference in New Issue
Block a user