-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Make show toolbar callback function async/sync compatible. #2066
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
Merged
Merged
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
6cb6396
ASGI check approach with added test and docs
salty-ivy 830c964
revert changes
salty-ivy e355468
Merge branch 'main' of https://github.com/salty-ivy/django-debug-toolbar
salty-ivy b5bf3df
Merge branch 'main' of https://github.com/salty-ivy/django-debug-toolbar
salty-ivy 6dfd180
Merge branch 'main' of https://github.com/salty-ivy/django-debug-toolbar
salty-ivy 5b218d7
Merge branch 'main' of https://github.com/salty-ivy/django-debug-toolbar
salty-ivy 7171dbb
Make show toolbar callback function async/sync compatible.
tim-schilling ec3308a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 91eaca0
Merge branch 'main' of https://github.com/salty-ivy/django-debug-toolbar
salty-ivy 8cb21b7
Merge branch 'main' of https://github.com/salty-ivy/django-debug-toolbar
salty-ivy e183012
async compatible require_toolbar and tests
salty-ivy d5613df
Merge branch 'main' of https://github.com/jazzband/django-debug-toolb…
salty-ivy 1c90278
Merge branch 'django-commons:main' into main
salty-ivy 0957fc3
Merge branch 'main' of https://github.com/salty-ivy/django-debug-tool…
salty-ivy 588a71b
changes.rst
salty-ivy fc2130f
add docs for async require_toolbar
salty-ivy 42b5bbf
Merge branch 'main' into show-toolbar-compat
tim-schilling File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
import asyncio | ||
from unittest.mock import patch | ||
|
||
from django.contrib.auth.models import User | ||
from django.http import HttpResponse | ||
from django.test import AsyncRequestFactory, RequestFactory, TestCase, override_settings | ||
|
||
from debug_toolbar.middleware import DebugToolbarMiddleware | ||
|
||
|
||
def show_toolbar_if_staff(request): | ||
# Hit the database, but always return True | ||
return User.objects.exists() or True | ||
|
||
|
||
async def ashow_toolbar_if_staff(request): | ||
# Hit the database, but always return True | ||
has_users = await User.objects.afirst() | ||
return has_users or True | ||
|
||
|
||
class MiddlewareSyncAsyncCompatibilityTestCase(TestCase): | ||
def setUp(self): | ||
self.factory = RequestFactory() | ||
self.async_factory = AsyncRequestFactory() | ||
|
||
@override_settings(DEBUG=True) | ||
def test_sync_mode(self): | ||
""" | ||
test middleware switches to sync (__call__) based on get_response type | ||
""" | ||
|
||
request = self.factory.get("/") | ||
middleware = DebugToolbarMiddleware( | ||
lambda x: HttpResponse("<html><body>Test app</body></html>") | ||
) | ||
|
||
self.assertFalse(asyncio.iscoroutinefunction(middleware)) | ||
|
||
response = middleware(request) | ||
self.assertEqual(response.status_code, 200) | ||
self.assertIn(b"djdt", response.content) | ||
|
||
@override_settings(DEBUG=True) | ||
async def test_async_mode(self): | ||
""" | ||
test middleware switches to async (__acall__) based on get_response type | ||
and returns a coroutine | ||
""" | ||
|
||
async def get_response(request): | ||
return HttpResponse("<html><body>Test app</body></html>") | ||
|
||
middleware = DebugToolbarMiddleware(get_response) | ||
request = self.async_factory.get("/") | ||
|
||
self.assertTrue(asyncio.iscoroutinefunction(middleware)) | ||
|
||
response = await middleware(request) | ||
self.assertEqual(response.status_code, 200) | ||
self.assertIn(b"djdt", response.content) | ||
|
||
@override_settings(DEBUG=True) | ||
@patch( | ||
"debug_toolbar.middleware.show_toolbar_func_or_path", | ||
return_value=ashow_toolbar_if_staff, | ||
) | ||
def test_async_show_toolbar_callback_sync_middleware(self, mocked_show): | ||
def get_response(request): | ||
return HttpResponse("<html><body>Hello world</body></html>") | ||
|
||
middleware = DebugToolbarMiddleware(get_response) | ||
|
||
request = self.factory.get("/") | ||
response = middleware(request) | ||
self.assertEqual(response.status_code, 200) | ||
self.assertIn(b"djdt", response.content) | ||
|
||
@override_settings(DEBUG=True) | ||
@patch( | ||
"debug_toolbar.middleware.show_toolbar_func_or_path", | ||
return_value=show_toolbar_if_staff, | ||
) | ||
async def test_sync_show_toolbar_callback_async_middleware(self, mocked_show): | ||
async def get_response(request): | ||
return HttpResponse("<html><body>Hello world</body></html>") | ||
|
||
middleware = DebugToolbarMiddleware(get_response) | ||
|
||
request = self.async_factory.get("/") | ||
response = await middleware(request) | ||
self.assertEqual(response.status_code, 200) | ||
self.assertIn(b"djdt", response.content) |
This file was deleted.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we pass
async_mode
based on the type of requestWSGIRequest
orASGIRequest
here ? I mean I am not sure whetherrequire_show_toolbar
would ever be exposed to a customasync view
in async context or vice versa but this would save the conversion as it slows down the overall process. thoughts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping
require_show_toolbar
was an undocumented API, but it is documented. Yes, we need to pass in a better value than alwaysFalse
. Good catch @salty-ivy!It feels like we'd want
async_mode
defined by whetherview
is a coroutine though rather than based on WSGIRequest vs ASGIRequest. Not 100% sure though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can possible break in a situation where everything is in async context
including
show_toolbar
call back and if I userequire_show_toolbar
on anasync view
it will forcefully convert it into a sync func and we would get into similar error given thatrequire_show_toolbar
is exposed to such conditions :DThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be checking both but it seems like that would cause a rabbit hole : (
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. I totally missed that this decorator needs to be async friendly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct, we should check whether view is a coroutine or not since django also converts the request object based on that, just checked : D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah missed that too and yeah a iscoroutinefunction is the way other django decorator work.