Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

mgaligniana
Copy link
Member

@mgaligniana mgaligniana commented Jul 27, 2023

Hello!

In this PR I try fix the issue #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
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.07%. Comparing base (75cbbcf) to head (8bb96b5).
Report is 39 commits behind head on master.

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

silk/middleware.py Outdated Show resolved Hide resolved
@yoonthegoon yoonthegoon requested a review from SebCorbin July 28, 2023 19:12
@yoonthegoon
Copy link

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.

project/tests/test_silky_middleware.py Show resolved Hide resolved
project/tests/test_view_requests.py Outdated Show resolved Hide resolved
project/tests/test_view_requests.py Outdated Show resolved Hide resolved
@@ -130,7 +130,7 @@ def _get_objects(self, show=None, order_by=None, order_dir=None, path=None, filt
return query_set[:show]

def _create_context(self, request):
raw_filters = request.session.get(self.session_key_request_filters, {}).copy()
raw_filters = request.session.get(self.session_key_request_filters, {}).copy() if hasattr(request, 'session') else {}
Copy link
Contributor

@SebCorbin SebCorbin Jul 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -54,7 +54,7 @@ def _num_queries_by_view(self, filters):
return sorted(requests, key=lambda item: item.t, reverse=True)

def _create_context(self, request):
raw_filters = request.session.get(self.filters_key, {})
raw_filters = request.session.get(self.filters_key, {}) if hasattr(request, 'session') else {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also a fallback to request.GET or request.POST here

@mgaligniana mgaligniana force-pushed the ticket_347 branch 3 times, most recently from 014bbf8 to 753ced8 Compare August 14, 2023 10:31
@mgaligniana
Copy link
Member Author

mgaligniana commented Aug 14, 2023

@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 mgaligniana requested a review from SebCorbin August 14, 2023 12:43
@mgaligniana mgaligniana requested a review from albertyw October 3, 2023 13:23
@mgaligniana mgaligniana self-assigned this Oct 18, 2023
@mgaligniana
Copy link
Member Author

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
Copy link

MattyCZ commented Jul 17, 2024

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
Copy link
Member Author

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!

@mgaligniana
Copy link
Member Author

Hi @albertyw! I just want to let you know that this PR was marked as part of the release 5.1.0 but was never merged! https://github.com/jazzband/django-silk/releases/tag/5.1.0

@albertyw albertyw merged commit 28b1184 into jazzband:master Aug 15, 2024
@albertyw albertyw mentioned this pull request Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants