From 0d4fefa2b64a6c8e2949a00ed509a28d82763919 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Tue, 23 Jun 2020 13:53:08 -0700 Subject: [PATCH] logging_handlers: Fix type: ignore issues. Signed-off-by: Anders Kaseorg --- zerver/logging_handlers.py | 9 ++++++-- zerver/tests/test_logging_handlers.py | 30 +++++++++++++++++---------- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/zerver/logging_handlers.py b/zerver/logging_handlers.py index 7c22968821..a523cd5ebb 100644 --- a/zerver/logging_handlers.py +++ b/zerver/logging_handlers.py @@ -10,6 +10,7 @@ from urllib.parse import SplitResult from django.conf import settings from django.http import HttpRequest from django.views.debug import get_exception_reporter_filter +from typing_extensions import Protocol, runtime_checkable from version import ZULIP_VERSION from zerver.lib.logging_util import find_log_caller_module @@ -67,6 +68,10 @@ def add_request_metadata(report: Dict[str, Any], request: HttpRequest) -> None: # exception if the host is invalid report['host'] = platform.node() +@runtime_checkable +class HasRequest(Protocol): + request: HttpRequest + class AdminNotifyHandler(logging.Handler): """An logging handler that sends the log/exception to the queue to be turned into an email and/or a Zulip message for the server admins. @@ -123,8 +128,8 @@ class AdminNotifyHandler(logging.Handler): report['log_module'] = find_log_caller_module(record) report['log_lineno'] = record.lineno - if hasattr(record, "request"): - add_request_metadata(report, record.request) # type: ignore[attr-defined] # record.request is added dynamically + if isinstance(record, HasRequest): + add_request_metadata(report, record.request) except Exception: report['message'] = "Exception in preparing exception report!" diff --git a/zerver/tests/test_logging_handlers.py b/zerver/tests/test_logging_handlers.py index d3a266b7ac..76b6d430ef 100644 --- a/zerver/tests/test_logging_handlers.py +++ b/zerver/tests/test_logging_handlers.py @@ -13,7 +13,7 @@ from django.utils.log import AdminEmailHandler from zerver.lib.test_classes import ZulipTestCase from zerver.lib.types import ViewFuncT -from zerver.logging_handlers import AdminNotifyHandler +from zerver.logging_handlers import AdminNotifyHandler, HasRequest captured_request: Optional[HttpRequest] = None captured_exc_info: Tuple[Optional[Type[BaseException]], Optional[BaseException], Optional[TracebackType]] = None @@ -78,9 +78,16 @@ class AdminNotifyHandlerTest(ZulipTestCase): self.assert_json_error(result, "Internal server error", status_code=500) rate_limit_patch.assert_called_once() - record = self.logger.makeRecord('name', logging.ERROR, 'function', 15, - 'message', {}, captured_exc_info) - record.request = captured_request # type: ignore[attr-defined] # this field is dynamically added + record = self.logger.makeRecord( + 'name', + logging.ERROR, + 'function', + 15, + 'message', + {}, + captured_exc_info, + extra={"request": captured_request}, + ) return record def run_handler(self, record: logging.LogRecord) -> Dict[str, Any]: @@ -109,6 +116,7 @@ class AdminNotifyHandlerTest(ZulipTestCase): mock_function.return_value = None """A normal request is handled properly""" record = self.simulate_error() + assert isinstance(record, HasRequest) report = self.run_handler(record) self.assertIn("user_email", report) @@ -125,7 +133,7 @@ class AdminNotifyHandlerTest(ZulipTestCase): self.assertEqual(report["stack_trace"], "See /var/log/zulip/errors.log") # Check anonymous user is handled correctly - record.request.user = AnonymousUser() # type: ignore[attr-defined] # this field is dynamically added + record.request.user = AnonymousUser() report = self.run_handler(record) self.assertIn("host", report) self.assertIn("user_email", report) @@ -135,10 +143,10 @@ class AdminNotifyHandlerTest(ZulipTestCase): # Now simulate a DisallowedHost exception def get_host_error() -> None: raise Exception("Get Host Failure!") - orig_get_host = record.request.get_host # type: ignore[attr-defined] # this field is dynamically added - record.request.get_host = get_host_error # type: ignore[attr-defined] # this field is dynamically added + orig_get_host = record.request.get_host + record.request.get_host = get_host_error report = self.run_handler(record) - record.request.get_host = orig_get_host # type: ignore[attr-defined] # this field is dynamically added + record.request.get_host = orig_get_host self.assertIn("host", report) self.assertIn("user_email", report) self.assertIn("message", report) @@ -147,9 +155,9 @@ class AdminNotifyHandlerTest(ZulipTestCase): # Test an exception_filter exception with patch("zerver.logging_handlers.get_exception_reporter_filter", return_value=15): - record.request.method = "POST" # type: ignore[attr-defined] # this field is dynamically added + record.request.method = "POST" report = self.run_handler(record) - record.request.method = "GET" # type: ignore[attr-defined] # this field is dynamically added + record.request.method = "GET" self.assertIn("host", report) self.assertIn("user_email", report) self.assertIn("message", report) @@ -173,7 +181,7 @@ class AdminNotifyHandlerTest(ZulipTestCase): self.assertEqual(report["stack_trace"], 'No stack trace available') # Test arbitrary exceptions from request.user - record.request.user = None # type: ignore[attr-defined] # this field is dynamically added + record.request.user = None with patch("zerver.logging_handlers.traceback.print_exc"): report = self.run_handler(record) self.assertIn("host", report)