Issues/104 #105
Issues/104 #105
Conversation
|
|
||
| if i != last: | ||
| part = part + self.boundary | ||
|
|
||
| self.logger.log(self.log_level, part, logging_context) | ||
|
|
||
| def _log_resp(self, level, response, logging_context): | ||
| if re.match('^application/json', response.get('Content-Type', ''), re.I): | ||
| if re.match("^application/json", response.get("Content-Type", ""), re.I): |
There was a problem hiding this comment.
Reformatting everything really requires calling out the changes so I don't wind up questioning all our previous decisions :)
Is there any reason to use re.match here instead of a string find on the lower-cased content-type?
There was a problem hiding this comment.
Reformatting everything really requires calling out the changes so I don't wind up questioning all our previous decisions :)
Same problem here :D
Is there any reason to use re.match here instead of a string find on the lower-cased content-type?
I don't see any. IMHO, it would even deserve a more stringent test (if response.get("Content-Type", "").lower() == "application/json"), unless you want to support all JSON variants out there (boy there are quite a few!).
There was a problem hiding this comment.
Considering that application/json; charset=UTF-8 is a valid content type (albeit obsolete), the test should probably be: if response.get("Content-Type", "").lower().startswith("application/json").
| from django.conf import settings | ||
| from django.test import RequestFactory, override_settings | ||
| from django.http import HttpResponse, StreamingHttpResponse | ||
|
|
||
| import request_logging | ||
| from request_logging.middleware import LoggingMiddleware, DEFAULT_LOG_LEVEL, DEFAULT_COLORIZE, DEFAULT_MAX_BODY_LENGTH,\ | ||
| NO_LOGGING_MSG, DEFAULT_HTTP_4XX_LOG_LEVEL | ||
| from request_logging.middleware import ( |
There was a problem hiding this comment.
Are there any additional tests here to prove the new functionality?
There was a problem hiding this comment.
Will add some and commit.
|
Just added tests. Not sure I followed the right pattern though, let me know! |
|
Manual tests reveal that proposed changes break: submitting a form triggers the following error: Traceback (most recent call last):
File "*****/venv/lib/python3.8/site-packages/django/core/handlers/exception.py", line 52, in inner
response = get_response(request)
File "*****/venv/lib/python3.8/site-packages/request_logging/middleware.py", line 112, in __call__
self.process_request(request, response)
File "*****/venv/lib/python3.8/site-packages/request_logging/middleware.py", line 122, in process_request
return self._log_request(request, response)
File "*****/venv/lib/python3.8/site-packages/request_logging/middleware.py", line 177, in _log_request
self._log_request_body(request, logging_context, log_level)
File "*****/venv/lib/python3.8/site-packages/request_logging/middleware.py", line 190, in _log_request_body
if request.body:
File "*****/venv/lib/python3.8/site-packages/django/http/request.py", line 314, in body
raise RawPostDataException("You cannot access body after reading from request's data stream")Will investigate more next week. |
|
I see 2 possible refactorings of
Your choice! |
|
I think the first is simplest, let's try that. |
|
OK, done. I think this might be ready for review/merge. Testing on our current project right now. |
|
Let me know when you're done with your testing; things are pretty crazy here this week so I won't have much of a chance to look at it. |
|
OK, things look pretty smooth so far, let's just give it some time and use and I'll let you know by the end of the week :-) |
|
Manual testing revealed no issue during the week. I didn't have the opportunity to test with DRF though. |
|
Nevermind. Just tested on another project that uses DRF and it's all good (both 200 and >= 400 requests). |
|
Thanks for merging 👍 |
|
Sorry to revive this PR, just want to know if you have plans to package and deploy to Pypi an updated version including these changes. |
Fixes #104.