HiveBrain v1.2.0
Get Started
← Back to all entries
patternrustMinor

Cookie, Session, and Flash middleware for Iron framework

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
middlewareflashcookieironforsessionandframework

Problem

Iron's 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 Flashable on 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 an Option> 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 a RwLock so 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

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 Box


is 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 Util


you'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 suspect

self.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 from

if 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 Util

Context

StackExchange Code Review Q#135203, answer score: 4

Revisions (0)

No revisions yet.