From ca1d9603cb1961f39b1ec1dfc0b705d53a4e83c4 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Mon, 22 Jun 2020 19:30:55 -0700 Subject: [PATCH] decorator: Fix type of signature-changing decorators. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In a decorator annotated with generic type (ViewFuncT) -> ViewFuncT, the type variable ViewFuncT = TypeVar(…) must be instantiated to the *same* type in both places. This amounts to a claim that the decorator preserves the signature of the view function, which is not the case for decorators that add a user_profile parameter. The corrected annotations enforce no particular relationship between the input and output signatures, which is not the ideal type we might get if mypy supported variadic generics, but is better than enforcing a relationship that is guaranteed to be wrong. This removes a bunch of ‘# type: ignore[call-arg] # mypy doesn't seem to apply the decorator’ annotations. Mypy does apply the decorator, but the decorator’s incorrect annotation as signature-preserving made it appear as if it didn’t. Signed-off-by: Anders Kaseorg --- zerver/decorator.py | 37 +++++++++++++++++----------- zerver/tests/test_decorators.py | 36 +++++++++++++-------------- zerver/tests/test_webhooks_common.py | 4 +-- 3 files changed, 43 insertions(+), 34 deletions(-) diff --git a/zerver/decorator.py b/zerver/decorator.py index 72e3dc1db3655..701f85c38abb7 100644 --- a/zerver/decorator.py +++ b/zerver/decorator.py @@ -328,11 +328,10 @@ def full_webhook_client_name(raw_client_name: Optional[str]=None) -> Optional[st def api_key_only_webhook_view( webhook_client_name: str, notify_bot_owner_on_invalid_json: bool=True, -) -> Callable[[ViewFuncT], ViewFuncT]: +) -> Callable[[Callable[..., HttpResponse]], Callable[..., HttpResponse]]: # TODO The typing here could be improved by using the Extended Callable types: # https://mypy.readthedocs.io/en/latest/kinds_of_types.html#extended-callable-types - - def _wrapped_view_func(view_func: ViewFuncT) -> ViewFuncT: + def _wrapped_view_func(view_func: Callable[..., HttpResponse]) -> Callable[..., HttpResponse]: @csrf_exempt @has_request_variables @wraps(view_func) @@ -533,8 +532,10 @@ def _wrapped_view_func(request: HttpRequest, user_profile: UserProfile, # This API endpoint is used only for the mobile apps. It is part of a # workaround for the fact that React Native doesn't support setting # HTTP basic authentication headers. -def authenticated_uploads_api_view(skip_rate_limiting: bool=False) -> Callable[[ViewFuncT], ViewFuncT]: - def _wrapped_view_func(view_func: ViewFuncT) -> ViewFuncT: +def authenticated_uploads_api_view( + skip_rate_limiting: bool = False, +) -> Callable[[Callable[..., HttpResponse]], Callable[..., HttpResponse]]: + def _wrapped_view_func(view_func: Callable[..., HttpResponse]) -> Callable[..., HttpResponse]: @csrf_exempt @has_request_variables @wraps(view_func) @@ -555,10 +556,13 @@ def _wrapped_func_arguments(request: HttpRequest, # # If webhook_client_name is specific, the request is a webhook view # with that string as the basis for the client string. -def authenticated_rest_api_view(*, webhook_client_name: Optional[str]=None, - is_webhook: bool=False, - skip_rate_limiting: bool=False) -> Callable[[ViewFuncT], ViewFuncT]: - def _wrapped_view_func(view_func: ViewFuncT) -> ViewFuncT: +def authenticated_rest_api_view( + *, + webhook_client_name: Optional[str] = None, + is_webhook: bool = False, + skip_rate_limiting: bool = False, +) -> Callable[[Callable[..., HttpResponse]], Callable[..., HttpResponse]]: + def _wrapped_view_func(view_func: Callable[..., HttpResponse]) -> Callable[..., HttpResponse]: @csrf_exempt @wraps(view_func) def _wrapped_func_arguments(request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponse: @@ -671,24 +675,29 @@ def authenticate_log_and_execute_json(request: HttpRequest, # Checks if the request is a POST request and that the user is logged # in. If not, return an error (the @login_required behavior of # redirecting to a login page doesn't make sense for json views) -def authenticated_json_post_view(view_func: ViewFuncT) -> ViewFuncT: +def authenticated_json_post_view( + view_func: Callable[..., HttpResponse], +) -> Callable[..., HttpResponse]: @require_post @has_request_variables @wraps(view_func) def _wrapped_view_func(request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponse: return authenticate_log_and_execute_json(request, view_func, *args, **kwargs) - return _wrapped_view_func # type: ignore[return-value] # https://github.com/python/mypy/issues/1927 + return _wrapped_view_func -def authenticated_json_view(view_func: ViewFuncT, skip_rate_limiting: bool=False, - allow_unauthenticated: bool=False) -> ViewFuncT: +def authenticated_json_view( + view_func: Callable[..., HttpResponse], + skip_rate_limiting: bool = False, + allow_unauthenticated: bool = False, +) -> Callable[..., HttpResponse]: @wraps(view_func) def _wrapped_view_func(request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponse: kwargs["skip_rate_limiting"] = skip_rate_limiting kwargs["allow_unauthenticated"] = allow_unauthenticated return authenticate_log_and_execute_json(request, view_func, *args, **kwargs) - return _wrapped_view_func # type: ignore[return-value] # https://github.com/python/mypy/issues/1927 + return _wrapped_view_func def is_local_addr(addr: str) -> bool: return addr in ('127.0.0.1', '::1') diff --git a/zerver/tests/test_decorators.py b/zerver/tests/test_decorators.py index ae2edb9cf258b..adda952e16b0d 100644 --- a/zerver/tests/test_decorators.py +++ b/zerver/tests/test_decorators.py @@ -291,7 +291,7 @@ def my_webhook_raises_exception_unexpected_event( request.POST['api_key'] = 'X'*32 with self.assertRaisesRegex(JsonableError, "Invalid API key"): - my_webhook(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + my_webhook(request) # Start a valid request here request.POST['api_key'] = webhook_bot_api_key @@ -299,7 +299,7 @@ def my_webhook_raises_exception_unexpected_event( with mock.patch('logging.warning') as mock_warning: with self.assertRaisesRegex(JsonableError, "Account is not associated with this subdomain"): - api_result = my_webhook(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + api_result = my_webhook(request) mock_warning.assert_called_with( "User %s (%s) attempted to access API on wrong subdomain (%s)", @@ -310,7 +310,7 @@ def my_webhook_raises_exception_unexpected_event( with self.assertRaisesRegex(JsonableError, "Account is not associated with this subdomain"): request.host = "acme." + settings.EXTERNAL_HOST - api_result = my_webhook(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + api_result = my_webhook(request) mock_warning.assert_called_with( "User %s (%s) attempted to access API on wrong subdomain (%s)", @@ -325,7 +325,7 @@ def my_webhook_raises_exception_unexpected_event( with self.assertRaisesRegex(Exception, "raised by webhook function"): request.body = "{}" request.content_type = 'application/json' - my_webhook_raises_exception(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + my_webhook_raises_exception(request) # Test when content_type is not application/json; exception raised # in the webhook function should be re-raised @@ -333,7 +333,7 @@ def my_webhook_raises_exception_unexpected_event( with self.assertRaisesRegex(Exception, "raised by webhook function"): request.body = "notjson" request.content_type = 'text/plain' - my_webhook_raises_exception(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + my_webhook_raises_exception(request) # Test when content_type is application/json but request.body # is not valid JSON; invalid JSON should be logged and the @@ -343,7 +343,7 @@ def my_webhook_raises_exception_unexpected_event( request.body = "invalidjson" request.content_type = 'application/json' request.META['HTTP_X_CUSTOM_HEADER'] = 'custom_value' - my_webhook_raises_exception(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + my_webhook_raises_exception(request) message = """ user: {email} ({realm}) @@ -374,7 +374,7 @@ def my_webhook_raises_exception_unexpected_event( request.body = "invalidjson" request.content_type = 'application/json' request.META['HTTP_X_CUSTOM_HEADER'] = 'custom_value' - my_webhook_raises_exception_unexpected_event(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + my_webhook_raises_exception_unexpected_event(request) message = """ user: {email} ({realm}) @@ -400,7 +400,7 @@ def my_webhook_raises_exception_unexpected_event( with self.settings(RATE_LIMITING=True): with mock.patch('zerver.decorator.rate_limit_user') as rate_limit_mock: - api_result = my_webhook(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + api_result = my_webhook(request) # Verify rate limiting was attempted. self.assertTrue(rate_limit_mock.called) @@ -414,7 +414,7 @@ def my_webhook_raises_exception_unexpected_event( webhook_bot.is_active = False webhook_bot.save() with self.assertRaisesRegex(JsonableError, "Account is deactivated"): - my_webhook(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + my_webhook(request) # Reactive the user, but deactivate their realm. webhook_bot.is_active = True @@ -422,7 +422,7 @@ def my_webhook_raises_exception_unexpected_event( webhook_bot.realm.deactivated = True webhook_bot.realm.save() with self.assertRaisesRegex(JsonableError, "This organization has been deactivated"): - my_webhook(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + my_webhook(request) class SkipRateLimitingTest(ZulipTestCase): def test_authenticated_rest_api_view(self) -> None: @@ -439,12 +439,12 @@ def my_unlimited_view(request: HttpRequest, user_profile: UserProfile) -> str: request.method = 'POST' with mock.patch('zerver.decorator.rate_limit') as rate_limit_mock: - result = my_unlimited_view(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + result = my_unlimited_view(request) self.assert_json_success(result) self.assertFalse(rate_limit_mock.called) with mock.patch('zerver.decorator.rate_limit') as rate_limit_mock: - result = my_rate_limited_view(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + result = my_rate_limited_view(request) # Don't assert json_success, since it'll be the rate_limit mock object self.assertTrue(rate_limit_mock.called) @@ -462,12 +462,12 @@ def my_unlimited_view(request: HttpRequest, user_profile: UserProfile) -> str: request.POST['api_key'] = get_api_key(self.example_user("hamlet")) with mock.patch('zerver.decorator.rate_limit') as rate_limit_mock: - result = my_unlimited_view(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + result = my_unlimited_view(request) self.assert_json_success(result) self.assertFalse(rate_limit_mock.called) with mock.patch('zerver.decorator.rate_limit') as rate_limit_mock: - result = my_rate_limited_view(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + result = my_rate_limited_view(request) # Don't assert json_success, since it'll be the rate_limit mock object self.assertTrue(rate_limit_mock.called) @@ -484,12 +484,12 @@ def my_view(request: HttpRequest, user_profile: UserProfile) -> str: request.user = self.example_user("hamlet") with mock.patch('zerver.decorator.rate_limit') as rate_limit_mock: - result = my_unlimited_view(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + result = my_unlimited_view(request) self.assert_json_success(result) self.assertFalse(rate_limit_mock.called) with mock.patch('zerver.decorator.rate_limit') as rate_limit_mock: - result = my_rate_limited_view(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + result = my_rate_limited_view(request) # Don't assert json_success, since it'll be the rate_limit mock object self.assertTrue(rate_limit_mock.called) @@ -513,7 +513,7 @@ def my_webhook_raises_exception(request: HttpRequest, user_profile: UserProfile) with mock.patch('zerver.decorator.webhook_logger.exception') as mock_exception: with self.assertRaisesRegex(Exception, "raised by webhook function"): - my_webhook_raises_exception(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + my_webhook_raises_exception(request) message = """ user: {email} ({realm}) @@ -557,7 +557,7 @@ def my_webhook_raises_exception(request: HttpRequest, user_profile: UserProfile) with mock.patch('zerver.decorator.webhook_unexpected_events_logger.exception') as mock_exception: exception_msg = "The 'test_event' event isn't currently supported by the helloworld webhook" with self.assertRaisesRegex(UnexpectedWebhookEventType, exception_msg): - my_webhook_raises_exception(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + my_webhook_raises_exception(request) message = """ user: {email} ({realm}) diff --git a/zerver/tests/test_webhooks_common.py b/zerver/tests/test_webhooks_common.py index a842b230b1e74..b82441b533bae 100644 --- a/zerver/tests/test_webhooks_common.py +++ b/zerver/tests/test_webhooks_common.py @@ -77,7 +77,7 @@ def my_webhook_notify(request: HttpRequest, user_profile: UserProfile) -> None: last_message_id = self.get_last_message().id with self.assertRaisesRegex(JsonableError, "Malformed JSON"): - my_webhook_no_notify(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + my_webhook_no_notify(request) # First verify that without the setting, it doesn't send a PM to bot owner. msg = self.get_last_message() @@ -86,7 +86,7 @@ def my_webhook_notify(request: HttpRequest, user_profile: UserProfile) -> None: # Then verify that with the setting, it does send such a message. with self.assertRaisesRegex(JsonableError, "Malformed JSON"): - my_webhook_notify(request) # type: ignore[call-arg] # mypy doesn't seem to apply the decorator + my_webhook_notify(request) msg = self.get_last_message() self.assertNotEqual(msg.id, last_message_id) self.assertEqual(msg.sender.email, self.notification_bot().email)