jazzband
Async compatible debug-toolbar middleware
Description
GSoC 2024
This PR aims to add async compatibility in django-debug-toolbar middleware
Fixes # (issue)
Checklist:
- [x] I have added the relevant tests for this change.
- [ ] I have added an item to the Pending section of
docs/changes.rst.
LGTM. I wonder how the panels can differentiate in
process_requestbetween the sync and async version.
panels would receive either WSGIRequest or ASGIRequest ( not async ) instance which are subclasses of HttpRequest.
Also, awaiting def process_request (NOT async def process_request) looks a bit strange, but seems fine to me after thinking about it a bit.
process_request call of panels would ultimately reach down to view via chain of calls of both panels and then middlewares, the response will always be adapted to async by adapt_method_mode that has to be awaited.
https://github.com/django/django/blob/e56a32b89bb7fadffdfaa2cdf12b4863ccd5af9b/django/core/handlers/base.py#L104
it even adapts the view if its not async def the response will be adapted to async coroutines if the receiving middleware is async and we are running in async context ( if both conditions are true ).
https://github.com/django/django/blob/e56a32b89bb7fadffdfaa2cdf12b4863ccd5af9b/django/core/handlers/base.py#L54
this is my overall understanding so far.
- Change
Panel.is_asyncto a@propertymethod with a doc string, then update https://github.com/jazzband/django-debug-toolbar/blob/main/docs/panels.rst?plain=1#L332 to
Sorry for the nit pick question but should we use @classproperty or @property with class level private attribute _is_async? my main idea is that is should be class level we might need to access it in future directly via class and makes subclassing easy
Not a nitpick at all. It's a good question @salty-ivy! My instinct is to go with @property to be consistent with the others. @matthiask thoughts?
Autogenerating documentation from code is always a workaround for lazyness (I like it a lot!) but I'm not sure we should uglify the code just for that.
I thought that adding a #: comment above a property would also act as a docstring by the way, so that we could keep the attribute?
TIL, then let's go with what Matthias suggested. Thank you for pointing that out
If you search this page for #: you'll find examples:
https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#directive-autoproperty
So, if the autodoc support is the only reason for converting the attribute to a property I think I'm -0 on it. No strong opposition certainly, I just don't see the need for it.
I wanted the autodoc and then thought it would bring consistency to the API. Though you and @salty-ivy both questioning it is enough for me to agree to having it as an attribute.
- Update the architecture document here: https://github.com/jazzband/django-debug-toolbar/blob/9bcd6cac17fd1721d60c2b1b8218884caf0ee45e/docs/architecture.rst?plain=1#L82-L84
So we are left with this only while keeping is_async a class level attribute for now.
Not sure, may be something related to sphinx missing in the updated docs to throw the lint error?
@salty-ivy it can be hard to read the docs failures:
architecture.rst:84: : Spell check: ASGI: are disabled by default when running in async/ASGI enviroment..
architecture.rst:84: : Spell check: enviroment: are disabled by default when running in async/ASGI enviroment..
You need to change enviroment to environment
You need to change
enviromenttoenvironment
ahh, how can I miss this, thank you so much. Interestingly It even fails on ASGI keyword.
See! I didn't even catch that other one. I thought it was two lines because the word was used twice on that line :grin:
I squashed the commits down to one, cleaned up the commit message, and rebased on main. After tests pass, I'll merge it in