patternrustMinor
Cookie, Session, and Flash middleware for Iron framework
Viewed 0 times
middlewareflashcookieironforsessionandframework
Problem
Iron's
A couple specific questions/concerns:
```
extern crate iron;
extern crate cookie;
use iron::prelude::*;
use iron::{AroundMiddleware, Handler, typemap};
use iron::headers::{Cookie, SetCookie};
pub use cookie::CookieJar;
pub use cookie::Cookie as CookiePair;
pub struct Builder(&'static [u8]);
impl Builder {
pub fn new(key: &'static [u8]) -> Self { Builder(key) }
}
pub struct Util(&'static [u8], Option>);
impl Util {
pub fn jar(&mut self) -> Option> {
if self.1.is_none() {
self.1 = Some(CookieJar::new(self.0));
}
self.1.as_ref()
}
}
impl typemap::Key for Util { type Value = Self; }
impl AroundMiddleware for Builder {
fn around(self, handler: Box) -> Box {
let wrapper = Wrapper {
cookie and session middleware projects seem to be abandoned. I've put together implementations for them, along with a session flash provider. They seem to be working fine. They are all very small, so I'll paste them in their entirety below (each file is linked to a Github repo and each repository also contains a working example). I would appreciate any input!A couple specific questions/concerns:
- The library user brings their own session store and flash object implementations. I've never really programmed with generics before, so I don't know if any of this is reasonable. For example, I'm using an associated type for the flash object (which the user defines in implementing
Flashableon their session object) rather than a trait type parameter. I think this is the right choice, but I mostly made it to keep the signatures shorter...
- In
session_fe::Store::get, there is a possibly unnecessary clone. Would it be better to return anOption>to remove the clone? That's definitely doable, but might be slightly more unwieldy if the user needs to mutate the object before setting it back. Also, would it be good to wrap the inner value in aRwLockso that it could be swapped out without write-locking the whole map?
cookie_fe/src/lib.rs```
extern crate iron;
extern crate cookie;
use iron::prelude::*;
use iron::{AroundMiddleware, Handler, typemap};
use iron::headers::{Cookie, SetCookie};
pub use cookie::CookieJar;
pub use cookie::Cookie as CookiePair;
pub struct Builder(&'static [u8]);
impl Builder {
pub fn new(key: &'static [u8]) -> Self { Builder(key) }
}
pub struct Util(&'static [u8], Option>);
impl Util {
pub fn jar(&mut self) -> Option> {
if self.1.is_none() {
self.1 = Some(CookieJar::new(self.0));
}
self.1.as_ref()
}
}
impl typemap::Key for Util { type Value = Self; }
impl AroundMiddleware for Builder {
fn around(self, handler: Box) -> Box {
let wrapper = Wrapper {
Solution
I've never used these libraries so I only have surface-level comments, but I'm answering anyway because I feel bad you've been ignored. Luckily, I think the lack of answers is at least partly because your code look totally fine. It's much easier to critique worse code.
In particular, though you've "never really programmed with generics before", you're pretty fluent with them. Your choices seem good to me.
Rustaceans don't tend to write things on one line like this
Similarly some of your spacing elsewhere seems inconsistent or excessive. In particular, trailing spaces.
is fine as
and I'd make the whole function just
Traits are normally imported fully, so rather than
you'd import
is just
I don't like the name
Doesn't that say something?
For session-fe,
Not using auto-deref looks really odd.
You can just
for the
Then the
Writing
is clearer. If you want to remove the
to
In particular, though you've "never really programmed with generics before", you're pretty fluent with them. Your choices seem good to me.
Rustaceans don't tend to write things on one line like this
pub fn new(key: &'static [u8]) -> Self { Builder(key) }Similarly some of your spacing elsewhere seems inconsistent or excessive. In particular, trailing spaces.
Box::new(wrapper) as Boxis fine as
Box::new(wrapper)and I'd make the whole function just
Box::new(Wrapper {
builder: self,
handler: handler
})Traits are normally imported fully, so rather than
impl typemap::Key for Utilyou'd import
Key and impl Key for Util.if let Ok(&mut ref mut r) = res.as_mut()is just
if let Ok(r) = res.as_mut()I don't like the name
Builder; it's too generic. In fact, in your own usage you rename it:use cookie_fe::{Builder as CookieBuilder, Util as CookieUtil, CookiePair};Doesn't that say something?
For session-fe,
use std::convert::Into; is unused. There's a warning there. Also, learn auto-deref.(*g).get(key) → g.get(key)
(*lock).insert(key, value) → lock.insert(key, value)
(*lock).remove(key) → lock.remove(key)Not using auto-deref looks really odd.
You can just
Store(Default::default())for the
Store.Then the
iter here is suspectself.0.read().iter()
.filter_map(|g| g.get(key))
.cloned()
.next()Writing
self.0.read().ok()
.and_then(|g| g.get(key).cloned())is clearer. If you want to remove the
cloned, you're going to have to "return" a lock as well, since the read is only valid while the lock is held. This might be a good idea, but it's really up to you if the cost is worth it.::new is just T::new, and I believe rotate_out can be simplified fromif let Some(sess) = req.extensions.get::>() {
if let Some(ref next) = self.next {
if let Some(mut obj) = sess.get() {
obj.set_flash(Some(next.clone()));
sess.set(obj);
} else {
let mut obj = ::new();
obj.set_flash(Some(next.clone()));
sess.set(obj);
}
} else if let Some(mut obj) = sess.get() {
obj.set_flash(None);
sess.set(obj);
}
}to
if let Some(sess) = req.extensions.get::>() {
let set = |mut obj: T| {
obj.set_flash(self.next.clone());
sess.set(obj);
};
if let Some(obj) = sess.get() {
set(obj);
} else if self.next.is_some() {
set(T::new());
}
}Code Snippets
pub fn new(key: &'static [u8]) -> Self { Builder(key) }Box::new(wrapper) as Box<Handler>Box::new(wrapper)Box::new(Wrapper {
builder: self,
handler: handler
})impl typemap::Key for UtilContext
StackExchange Code Review Q#135203, answer score: 4
Revisions (0)
No revisions yet.