jazzband / django-silk

Silky smooth profiling for Django
MIT License
4.35k stars 333 forks source link

Fix when Session, Authentication or Message middlewares are not present #667

Open mgaligniana opened 12 months ago

mgaligniana commented 12 months ago

Hello!

In this PR I try fix the issue https://github.com/jazzband/django-silk/issues/347

Doesn't fail when one of these middlewares are not present:

'django.contrib.sessions.middleware.SessionMiddleware',
'django.contrib.auth.middleware.AuthenticationMiddleware',
'django.contrib.messages.middleware.MessageMiddleware'

and if SILKY_AUTHENTICATION is True, an exception is raised: SILKY_AUTHENTICATION can not be enabled without Session, Authentication or Message Django's middlewares

Let me know any change required!

Thank you!

codecov[bot] commented 12 months ago

Codecov Report

Merging #667 (8bb96b5) into master (75cbbcf) will increase coverage by 0.56%. Report is 2 commits behind head on master. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #667      +/-   ##
==========================================
+ Coverage   86.51%   87.07%   +0.56%     
==========================================
  Files          52       52              
  Lines        2091     2113      +22     
==========================================
+ Hits         1809     1840      +31     
+ Misses        282      273       -9     
Files Changed Coverage Δ
silk/middleware.py 90.00% <100.00%> (+0.67%) :arrow_up:
silk/request_filters.py 84.70% <100.00%> (+1.68%) :arrow_up:
silk/views/profiling.py 92.85% <100.00%> (+5.35%) :arrow_up:
silk/views/requests.py 98.86% <100.00%> (+0.01%) :arrow_up:
silk/views/summary.py 96.82% <100.00%> (+4.88%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

yoonthegoon commented 12 months ago

profiling.py, requests.py, and summary.py look good to me. The tests look good at a glance.

Appears to resolve the issue on the ticket. Hopefully Seb or someone else comes in and checks it too.

mgaligniana commented 11 months ago

@SebCorbin First, thank you for your quick responses and the pacience 🙏

Regarding the review:

Thanks for your work, it's a great beginning, but if I'm not mistaking, the filters are not working when selecting them. They should work if you fall back to request.GET or request.POST here

I followed the session middleware variable approach and defined a new one for the silk's filters: silk_filters as a fallback.

With that + the custom filter manager we can get filters from session or silk_filters depending on if the first one is not defined.

What do you think?

Other question: would it be worth adding selenium tests?

mgaligniana commented 7 months ago

Hi @albertyw! Sorry to bother you, just wondering if there is something I can do to push forward the pull request. As I mentioned in my last comment, perhaps add selenium tests could give more confident to the change? Thank you!

MattyCZ commented 1 week ago

Hi!

Sorry to bother you, but any progress on the review? We would really appreciate this feature, since it would allow us to use silk without the middleware needed. Thank you very much

mgaligniana commented 1 week ago

Hi @MattyCZ!

This pull request was marked as part of the last release but never merged. I think we should wait.

I'm available in case of changes required!