decorator: Fix type of signature-changing decorators.

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 <anders@zulip.com>

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 require_user_group_edit_permission(view_func: ViewFuncT) -> ViewFuncT:
 # 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 authenticated_uploads_api_view(skip_rate_limiting: bool=False) -> Callable[[
 #
 # 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')

zerver/tests/test_decorators.py

@@ -291,7 +291,7 @@ class DecoratorTestCase(TestCase):
         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 @@ class DecoratorTestCase(TestCase):
         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 @@ class DecoratorTestCase(TestCase):
             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 @@ class DecoratorTestCase(TestCase):
             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 @@ class DecoratorTestCase(TestCase):
             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 @@ class DecoratorTestCase(TestCase):
                 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 @@ body:
                 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 @@ body:
 
         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 @@ body:
         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 @@ body:
         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 @@ class SkipRateLimitingTest(ZulipTestCase):
         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 @@ class SkipRateLimitingTest(ZulipTestCase):
         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 @@ class SkipRateLimitingTest(ZulipTestCase):
         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 @@ class DecoratorLoggingTestCase(ZulipTestCase):
 
         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 @@ body:
         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})

zerver/tests/test_webhooks_common.py

@@ -77,7 +77,7 @@ class WebhooksCommonTestCase(ZulipTestCase):
 
         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 @@ class WebhooksCommonTestCase(ZulipTestCase):
 
         # 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)