overhaul this crate's interface #57
Conversation
jbr
force-pushed
the
overhaul-session-and-session-store
branch
5 times, most recently
from
7d22fff to
3636ef2
Compare
* Remove interior mutability inside of Session * store values in memory as serde_json::Values instead of strings * add Session::take_value * use dashmap in MemoryStore * add an Error associated type instead of using anyhow * remove reexported but unused libraries * make memory-store and cookie-store cargo features (currently enabled by default) * updates base64 * adds Session::from_parts and Session::data
jbr
force-pushed
the
overhaul-session-and-session-store
branch
from
3636ef2 to
a1a094e
Compare
|
Consider extracting the |
|
|
||
| /// An async session backend. | ||
| #[async_trait] | ||
| pub trait SessionStore { | ||
| /// The [`std::error::Error`] type that this store returns | ||
| type Error: std::error::Error; |
|
It would be nice if v4 could also address the issue of not being able to clone without blowing away the session value. |
jbr
force-pushed
the
overhaul-session-and-session-store
branch
from
af484fd to
493fa04
Compare
| @@ -63,19 +63,6 @@ pub struct Session { | |||
| destroy: bool, | |||
| } | |||
|
|
|||
| impl Clone for Session { | |||
There was a problem hiding this comment.
Thank you for this! 🙇♂️
|
@jbr do you have a sense for when this might be merged and released? |
| pub fn get<T: serde::de::DeserializeOwned>(&self, key: &str) -> Option<T> { | ||
| let data = self.data.read().unwrap(); | ||
| let string = data.get(key)?; | ||
| serde_json::from_str(string).ok() | ||
| self.get_value(key) | ||
| .map(serde_json::from_value) | ||
| .transpose() | ||
| .ok() | ||
| .flatten() | ||
| } |
There was a problem hiding this comment.
i feel like this should return a Result<Option<T>, Err> instead of just an option. i'm running into an issue where i'm trying to get a value that does exist in the session store, but there must be some error in deserialization because it returns None.
IMO, there should be three cases:
Ok(Some(val)): That key is associated with this valueOk(None): That key doesn't have any value in the session storeErr(...): there was an error fetching/deserializing the value stored in that key
thoughts? I'm not a rust expert by any means, so feel free to correct me if i'm missing something.
(see this issue i opened in another repo for reference)
|
Hi @jbr I hope you been well, I was just wondering if this is ready to be merged or is there anything that's blocking this from getting merged? |
|
If I can donate my time to help with the maintenance of this crate I'd be more than happy to. Let me know if that would be helpful, @jbr. |
I plan to leave this open for a few days and merge if there are no concerns