Refactor settings in middleware to be loaded once rather than per request#75
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the RequestIDMiddleware from Django's legacy MiddlewareMixin pattern to the modern middleware pattern. The key improvement is loading Django settings once during middleware initialization rather than on every request, improving performance. The refactoring also eliminates the need for MiddlewareMixin, simplifying the code and making it compatible with modern Django versions (3.2+).
Changes:
- Converted
RequestIDMiddlewarefromMiddlewareMixinto modern Django middleware with__init__(get_response)and__call__(request)pattern - Moved settings retrieval from per-request methods to
__init__for performance optimization - Updated tests to invoke middleware via the standard call pattern instead of calling
process_requestandprocess_responseseparately
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| log_request_id/middleware.py | Refactored middleware class from MiddlewareMixin to modern pattern with settings loaded once in init; consolidated request/response processing into call method; modernized string formatting to use f-strings |
| log_request_id/tests.py | Updated test methods to call middleware through standard invocation pattern (instantiate and call) rather than calling process_request/process_response methods directly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dabapps-robot
left a comment
There was a problem hiding this comment.
Nice refactor overall 👍
High-level
This is a solid cleanup: moving to modern middleware shape, removing MiddlewareMixin, and caching settings in __init__ all make the flow clearer and should reduce per-request overhead.
The test updates to exercise the middleware via __call__ are also a good improvement in realism.
One thing worth tightening
I’d suggest wrapping the request handling in try/finally so local.request_id is always cleared, even if downstream raises:
def __call__(self, request):
local.request_id = request.id = request_id = self._get_request_id(request)
try:
response = self.get_response(request)
if self.request_id_response_header:
response[self.request_id_response_header] = request_id
if self.log_requests and "favicon" not in request.path:
logger.info(self.get_log_message(request, response))
return response
finally:
try:
del local.request_id
except AttributeError:
passNot a huge change, but it makes the middleware more robust around error paths.
Other than that, this looks good to me and is close to merge-ready.
This cleans up the request flow a little, and avoids the need to use
MiddlewareMixin.I also refactored the tests a little to use the entire middleware chain rather than calling the component pieces.