monads and ugly code

[Vladimir Bychkovsky]

I've am trying to implement basic account management system (register/login/logout/etc) using happstack. This experience is challenging my understanding of monads and monad transformers. Below is a simple function that checks if a given session is valid. 

Session stores user's id (email), client IP/port. 'checkSession' verifies if this information is valid. What I can't figure out how to make this code cleaner. It seems like this is a classic example for Maybe monad, but because of 'getSession' (~StateT) and 'query' (MonadIO) I can't seem to figure out a way to clean this up. [ I've been thinking that I should put 'acid' into a ReaderT, but that's only going to make this more complicated here]

Any ideas on how I can clean this up?

[ P.S. I know this check is not secure from replay attacks from the same IP; but that's another story ]   

-- this code is ugly, I wonder if I could use a Maybe monad here 

checkSession acid = do

    session <- getSession 

    let uMail=userEmail session

    if uMail == ""

        then return (Nothing)

        else  do

            req <- askRq

            let peer=rqPeer req  

            if peer /= (userIPPort session)

                then return (Nothing)

                else do

                    u <- query' acid (FindUser uMail)

                    case u of 

                        Nothing -> return (Nothing)

                        Just user -> return (Just session)

homePage :: DB -> App Response 

homePage acid = do

    res <- checkSession acid 

    case res of 

        Nothing -> loginPage 

        Just session -> userPage session

[thomas hartman]

First of all, it would be easier to answer your question if you included the typesig of checksession in your post.

Somewhat guessing here, but I think you might want MaybeT here.

Another possibility is ErrorT, and you'd be producing error values instead of Nothing if the user can't authenticate.

http://en.wikibooks.org/wiki/Haskell/Monad_transformers

In my experience, using transformers gets your code more concise, but at the cost of potentially making the code less readable to your colleagues (or yourself!). 

So you might just want "if it ain't broke don't mess with it" approach.

But I know you'll have fun with transformers :)

[Vladimir Bychkovsky]

Thank you, Thomas. I've read the link, but I could not make that code for me.

I was not sure about the types, so I let GHC infer them. Here are the types I've extracted:

checkSession  :: (ServerMonad m, MonadIO m, MonadClientSession SessionData m) =>DB -> m (Maybe SessionData)

What I want, I think ends with  DB -> MaybeT (m SessionData), but after fighting with MaybeT for a while, I've decided to follow your advice and not "fix it". Maybe the solution will dawn on me later.

Vlad.

[thomas hartman]

> MaybeT (m SessionData)

I think you want 

MaybeT m SessionData -- no parens

MaybeT (and monad transformers usually) takes 2 type arguments.

Anwyay... don't feel bad not messing with it. 

You don't have to use every shiny feature of Happstack to get useful work done.

[Vladimir Bychkovsky]

Actually, I think I got it!

I wanted to be able to use MaybeT guards. The trick was to use runMaybeT. This looks a lot cleaner to me:

checkSession acid = runMaybeT $ do

    -- check if session is empty

    session <- lift getSession 

    let email=userEmail session

    guard $ email /= ""

    

    -- check if peer IP/port match the session

    req <- lift askRq

    guard $ (rqPeer req) == (userIPPort session)

    -- check if the user exists in DB

    u <- liftIO $ query' acid (FindUser email)

    guard $ isJust u

    return session

[Vladimir Bychkovsky]

This is probably going overboard, but using RecordWildCards and the fact that pattern match in MaybeT returns Nothing, we can get fewer lines

( it is not clear if this code is more readable though). Anyway, I'll stop here :-)

checkSession acid = runMaybeT $ do

    -- check if session is empty

    session@SessionData{..} <- lift getSession 

    guard $ userEmail /= ""

    

    -- check if peer IP/port match the session

    Request{..} <- lift askRq

    guard $ rqPeer == userIPPort

    -- check if the user exists in DB

    Just _ <- liftIO $ query' acid (FindUser userEmail)

    return session

[dag.od...@gmail.com]

That liftIO is redundant.  Also maybe try MonadComprensions if you're using GHC 7.4 or later, something like:

    checkSession acid = runMaybeT
        [ session | session@SessionData{..} <- lift getSession
                  , userEmail /= ""
                  , Request{..} <- lift askRq
                  , rqPeer == userIPPort

                  , Just _ <- query' acid (FindUser userEmail) ]

--
You received this message because you are subscribed to the Google Groups "HAppS" group.
To post to this group, send email to ha...@googlegroups.com.
To unsubscribe from this group, send email to happs+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/happs?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Vladimir Bychkovsky]

Thanks, Dag! I've looked up MonadComprehensions and they seem pretty sweet [as in, very sugary syntax :-) ]

re: redundant liftIO

Thank you for pointing this out. So, liftIO isn't needed because our monad transformer stack inside the comprehension ends with IO a, or more specifically MaybeT (AppT IO) SessionData is the type inside the comprehension? So, effectively, the return type is IO SessionData (that gets transformed into IO (Maybe SessionData) by runMaybeT later)? And we still need to lift getSession and askReq, because ClientSession and ServerMonad sit above IO in the monad stack?

I've also dropped userEmail /= "" constraint because userIPPort would be empty for an empty session (rqPeer should never be empty unless there is some major bug in Happstack). Also, an entry with an empty userID will not be found in the database. The final code seems very clean, so it does not seem to need comments (though, maybe I should rename userIPPort into sessionIPPort, and userEmail to sessionEmail):

checkSession acid = runMaybeT 

    [ session | session@SessionData{..} <- lift getSession

              , Request{..} <- lift askRq

              , rqPeer == userIPPort

              , Just _ <- query' acid (FindUser userEmail)

    ]

[Arseniy Alekseyev]

Another thing that went unnoticed: the pattern-match of the result of
query' is not necessary here because it's exactly what >>= of MaybeT
is designed to do for you. Combined with the fact that the fail method
of Monad typeclass is generally considered bad, the following solution
looks nicer to me:

checkSession acid = runMaybeT

[ session | session@SessionData{..} <- lift getSession
, userEmail /= ""
, Request{..} <- lift askRq
, rqPeer == userIPPort

, MaybeT $ query' acid (FindUser userEmail) ]

Also, I can't seem to figure out why monad comprehensions are useful
here. They don't bring anything other than the necessity to
comma-separate statements here, do they?

Cheers!
Arseniy


On 26 January 2013 14:43, Vladimir Bychkovsky

[Arseniy Alekseyev]

Oh, sorry, I didn't notice they eliminated "guard"s. My bad.

[dag.od...@gmail.com]

liftIO is redundant because query' is already lifted to IO.

[dag.od...@gmail.com]

Also the "return".  But I'm not sure you can use a comprehension with the MaybeT constructor like that, you'd probably have to make it:

    _ <- MaybeT (query' ...)

and then I'm not sure it's an improvement.  But nice in deed if you're not using comprehensions.

--
You received this message because you are subscribed to the Google Groups "HAppS" group.

To unsubscribe from this group and stop receiving emails from it, send an email to happs+un...@googlegroups.com.

To post to this group, send email to ha...@googlegroups.com.

[Vladimir Bychkovsky]

Oh! That makes more sense and it is a lot simpler. I was pretty confused with all the monad stacking. 

I was using query' following the acid-state example (blindly). According to the docs is it lifted indeed: 

query' acidState event = liftIO (query acidState event)

Thanks again, Dag!

[Vladimir Bychkovsky]

I could not quite make the suggested syntax work, but in the spirt of not using 'fail', the following seems to do the trick

checkSession acid = runMaybeT

    [ session | session@SessionData{..} <- lift getSession

              , Request{..} <- lift askRq

              , rqPeer == userIPPort

              , u <- query' acid (FindUser userEmail)

              , isJust u

    ]   

[dag.od...@gmail.com]

FWIW I like "fail" in monads like Maybe.  The main issue with "fail" is that it's in Monad and not a separate class, which means we can't statically disallow it for certain monads and are forced to use runtime exceptions.  That's not a problem with the Maybe monad.

[Vladimir Bychkovsky]

At the risk of beating this horse to death, here is the version without MonadComprehensions that uses the suggested MaybeT:

checkSession acid = runMaybeT $ do

    session@SessionData{..} <- lift getSession

    Request{..} <- lift askRq

    guard $ rqPeer == userIPPort

    runMaybeT $ query' acid (FindUser userEmail)

    return session

I think the earlier version (shown below) is still my favorite as far as readability goes, but that's of course subjective. Luckily, nobody else has to read this code (yet). 

checkSession acid = runMaybeT 

    [ session | session@SessionData{..} <- lift getSession

              , Request{..} <- lift askRq

              , rqPeer == userIPPort

              , Just _ <- query' acid (FindUser userEmail)

    ]

In the light of Dag's explaination of reasons for caution around Monad's fail, I think I'll go with the last version. Thanks again, everyone!