-
-
Notifications
You must be signed in to change notification settings - Fork 786
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
Use the async session.asave method if it exists #2092
base: main
Are you sure you want to change the base?
Conversation
6b33357
to
0bcd7a4
Compare
if django.VERSION >= (5, 1): | ||
await self.scope["session"].asave() | ||
else: | ||
await database_sync_to_async(self.scope["session"].save)() | ||
except UpdateError: | ||
raise SuspiciousOperation( | ||
"The request's session was deleted before the " |
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'm not actually sure how to trigger this logic, because based on reading the Django source code I can't figure out where it would be raised when calling save()
without must_create=True
0bcd7a4
to
545b4e1
Compare
@carltongibson: I've added the missing test coverage (such as I could, see inline comment) and fixed the thing you noted. I'm not sure this should be merged before #2090 is resolved since sessions can use the Django ORM under the hood. |
545b4e1
to
2ac042f
Compare
Yes, I too was thinking we need to settle that first 👍 |
Support was added in django/django#17372 (slated for release in 5.1)
We can use this API if it exists, otherwise, we fallback to a
sync_to_async
bridge.This does break backwards compatibility, which I'm not terribly happy about but I'm not sure how best to fix it.
If users overrode
save_session
in a subclass they would expect that to still work, so I thought the best way to do this was to make it async in the base class and thenawait
it, so at least it will be obvious that it doesn't work (python will raise an exception aboutawait
ing a non-awaitable), but I'm open to other solutions.