initial sketch for session.regenerate() support
#6
Conversation
This changes how sessions are provided by the middleware in order to support the ability to use session methods, like `regenerate`. Crucially sessions are no longer cloned and instead are stored via a handle. This handle is defined as `Arc<RwLock<async_session::Session>>` which allows us to clone the `Arc` without cloning the contained session directly. In order to support better ergonomics we also introduce two new extractors: `ReadableSession` and `WritableSession`. These respectively enable reading and writing of sessions. Internally they acquire the lock and expose the session for manipulation. One downside to this approach is that library consumers now need to use two separate types as opposed to using `async_session::Session` via `Extension` directly. However the API here now no longer requires `Extension` and enforces explicit usage of reading and writing via the public type interface. Fixes: #5.
|
@Ptrskay3 here's what I've put together pursuing the interior mutability option. I'd love your feedback. |
| // NB: This DOES NOT update the store, meaning that both sessions will still be | ||
| // found. |
There was a problem hiding this comment.
Nit: Technically, async_session doesn't promise to purge the old session from the storage. In most applications it's probably fine, but depends on what type of data you store.
I tried to add the purging functionality to this implementation, but unfortunately the destroy_session expects an async_session::Session as argument, so even though in the middleware we are able to inspect and compare the id of the session before and after the handler is called, there's no easy way to purge it.
There was a problem hiding this comment.
I think it's a little orthogonal to our patch here, but perhaps there's a missing "refresh" method from async_sessions itself? I think I may have seen something about that browsing through their repo issues.
src/extractors.rs
Outdated
| where | ||
| B: Send, | ||
| { | ||
| type Rejection = http::StatusCode; |
There was a problem hiding this comment.
There is no way to return http::StatusCode (or any other type of rejection) from the implementation, so I think this should be std::convert::Infallible. If the session layer is not added, let the thread panic, that's definitely a bug, so I think we can leave the expect.
src/session.rs
Outdated
| Ok(None) => {} | ||
|
|
||
| Ok(None) => { | ||
| tracing::warn!("The cookie value is missing; no cookie will be set!"); |
There was a problem hiding this comment.
In my experience, this makes a lot of unnecessary noise in the logs. If you insist on keeping this, I think this should be info level at max.
There was a problem hiding this comment.
Thanks for calling this out. I was initially assuming this was a case that we'd not really hit, but in practice you're absolutely right. I'll take it out for the time being.
maxcountryman
force-pushed
the
feat/support-regenerate
branch
from
c3dfb41 to
c22f22d
Compare
|
I'll go ahead and merge this. Thank you for your great feedback! |
This changes how sessions are provided by the middleware in order to
support the ability to use session methods, like
regenerate. Cruciallysessions are no longer cloned and instead are stored via a handle. This
handle is defined as
Arc<RwLock<async_session::Session>>which allowsus to clone the
Arcwithout cloning the contained session directly.In order to support better ergonomics we also introduce two new
extractors:
ReadableSessionandWritableSession. These respectivelyenable reading and writing of sessions. Internally they acquire the lock
and expose the session for manipulation.
One downside to this approach is that library consumers now need to use
two separate types as opposed to using
async_session::SessionviaExtensiondirectly. However the API here now no longer requiresExtensionand enforces explicit usage of reading and writing via thepublic type interface.
Fixes: #5.