-
Notifications
You must be signed in to change notification settings - Fork 360
Fix gateway cookie handling #1558
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 gateway cookie handling #1558
Conversation
1cb64b9
to
0cc8523
Compare
aa72183
to
188b5ca
Compare
I'm sorry, but I don't understand the linting failure. Please advise. |
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.
Hi Kevin! I'm not that familiar with the jupyter_server
codebase but I took a quick look at the failing CI so I wanted to add some comments!
89d7e29
to
bf15cef
Compare
Hmm - bit of a catch-22 here as best as I can determine. This linting error suggests I move the import into a type-checking block:
But when I do that, the docs build fails with:
So I reverted the change, and am back to the lint error. With the change reverted, the type checks, linting, tests, and docs build pass on my dev machine. Could someone please let me know how I should proceed? Thank you. |
This has been driving me crazy. I think it's more important to keep the type defined (i.e., not use the TYPE_CHECKING hint). This enables the docs build to succeed. The type hint suggested by At least this should allow us to get past the |
Could I please get this reviewed. Since it's gateway related I requested @lresende but the general list of reviewers looks extremely old and I don't think it reflects the current team membership. Thanks. |
Thank you for taking care of this @kevin-bates |
Thank you for the merge @minrk! |
Fixes: #1557
This pull request addresses two issues:
response.headers.get("Set-Cookie")
, the cookies are returned in a comma-separated string. TheSimpleCookie.load()
method fails to parse this string, and the exception listed in GatewayClient doesn't correctly handle multiple cookies returned from gateway server #1557 is raised.expires
morsel with an tz offset (e.g.,+0000
) rather than the tz name (e.g.,GMT
) and, again,SimpleCookie.load()
did not parse that. The test still passed because the "expiring" cookie was not present, but it wasn't present due to the parsing issue and not because it was caught as expired.Issue 1 has been addressed by using
response.headers.get_list("Set-Cookie")
, where each element (cookie) in the list is loaded and processed.Issue 2 has been addressed by using a date formatter that produces an RFS 7231-compliant date string, and the use of the
email.utils
package has been removed. The test has also been refactored to be clearer when expiration testing is performed. This led to the removal of one of the test's arguments since it was rendered obsolete by the refactoring.Lastly, the call to the
gateway_client.update_cookies()
method has been updated to improve testability.