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

Session cookie fix #2402

Closed
wants to merge 9 commits into from

Conversation

ToasterChicken
Copy link

Summary

This is regarding Issue #2019

It adds persistent session features, and also the option for refreshing the current session before it expires while it's still valid (refresh window) and if it's beyond that window the session is invalid.

The default behavior without the new features should also remain the same was it was prior to the changes, every single requests results in a new session cookie.

I feel like there's possibly a much better way of doing this like being able to handle it all with in the call before the send_wrapper. However this is my first time delving into the ASGI framework so I'm not sure if I am able to obtain the new session data and compare it to the old session data only within the call and not have to make one cookie in call and another one with in send_wraper to resolve the current cookie and compare it to the new data.

Haven't added any tests to test_session.py yet, since I think this is a pretty large change and I can go ahead and work on that if this appears to be something that maybe accepted. The existing test suite passes however, and manually testing individual behaviors from the update work as expected.

There's no single change here, its a pretty big overall change to the middleware.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • Passes existing tests.
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@ToasterChicken
Copy link
Author

I re-submitted thinking there was something else wrong. but no it's because I don't have the tests implemented for the new logic...

Anyways, I feel like discussing this before getting all into that maybe first step before going through all of that since it isn't an insignificant change to the middleware.

@ToasterChicken
Copy link
Author

Will resubmit with the change suggested in the discussion.

@ToasterChicken ToasterChicken deleted the SessionCookieFix branch January 20, 2024 13:52
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.

1 participant