Fix #665 and #714, add cleanup in middleware #798
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #798 +/- ##
==========================================
+ Coverage 87.26% 87.32% +0.05%
==========================================
Files 53 53
Lines 2160 2169 +9
==========================================
+ Hits 1885 1894 +9
Misses 275 275 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
There should be some test cases here to test the logic for non-decodable strings (https://github.com/albertyw/django-silk/blob/master/project/tests/test_execute_sql.py) and that the interception can be added/removed (https://github.com/albertyw/django-silk/blob/master/project/tests/test_silky_middleware.py)
|
Thanks for the feedback! I'll try to find time to address it this week. |
|
Hi @albertyw, I believe I've addressed the comments at this time. Any chance of getting this re-reviewed soon? |
| @@ -77,7 +83,7 @@ def execute_sql(self, *args, **kwargs): | |||
| return iter([]) | |||
| else: | |||
| return | |||
| sql_query = q % tuple(force_str(param) for param in params) | |||
There was a problem hiding this comment.
See #617 . Let's keep this as is.
There was a problem hiding this comment.
Okay, I'll need to re-add the try/except logic here then to catch the case where force_str raises a UnicodeDecodeError.
There was a problem hiding this comment.
Or alternately, should I go with an approach like #658?
There was a problem hiding this comment.
I think #658 makes sense. It's possible it might result in invalid SQL but given this SQL is meant to be shown for debugging, I think a human can make that decision rather than just triggering an exception.
|
Latest diff changes the approach to add a |
|
Guys... this Broke Django 4.2 with JSONField.. ---> force_str_with_fallback() |
|
@frafrafrafra I can't seem to reproduce your issue. Do you have reproduction steps? |
Same for me... |
|
Let's continue this in #806 |
…azzband#798)" This reverts commit e00b93c.
…azzband#798)" This partially reverts commit e00b93c. The fixes in the middleware to restore the original `execute_sql` function remain in place.
Fix #665 and fix #714 by adding try/except logic for the case where the SQL query params contain non-unicode binary data. The observed behavior with this fix is that queries involving such data will not render "explain" content in django-silk, which I think is an acceptable compromise for now versus trying to actually fix the logic here to correctly handle binary data.
I noticed as a side effect of the above bug that (with a long lived server process), django-silk's wrapped
execute_sqlfunction was being applied to requests that shouldn't be processed by django-silk (i.e._should_intercept()returns False). This is because the wrapper wasn't being removed after processing a request, so all subsequent requests received by the process after any one_should_intercept() == Truerequest were still getting the wrapper code. I've fixed this as well by adding appropriate logic inprocess_response()to revert the function wrapper when done with a request.