Scala async/callback code rewriting

Question

Simple code that should check user by pass, user is active and after that update last login datetime.

  def authenticate() = Action.async { implicit request => 
    loginForm.bindFromRequest.fold(
        errors => Future.successful(BadRequest(views.html.logon(errors))),
        usersData =>{
           val cursor =  this.collection.find(BSONDocument("name" -> usersData._1)).one[Account].map(_.filter(p=>p.password == hashedPass(usersData._2, usersData._1)))
           cursor.flatMap(p => p match {
               case None => Future.successful(BadRequest(views.html.logon(loginForm.withGlobalError("user/pass incorect!!!"))))
               case Some(user) => {
                 if(!user.active) 
                   Future.successful(BadRequest(views.html.logon(loginForm.withGlobalError("inactive!!!"))))
                 else collection.update(BSONDocument("_id" -> user.id), 
                          BSONDocument("$set" -> 
                          BSONDocument("lastLogin" -> BSONDateTime(new org.joda.time.DateTime().getMillis()))))
                          .flatMap(x => gotoLoginSucceeded(user.id.stringify))

               }
               })
            })
  }  

How to rewrite it to less flatMap/map spaghetti?

Another solution

def authenticate() = AsyncStack { implicit request => 
loginForm.bindFromRequest.fold(
    errors => Future.successful(BadRequest(views.html.logon(errors))),
    usersData =>{
      for{
        user <- this.collection.find(BSONDocument("name" -> usersData._1)).one[Account].map(_.filter(p=>p.password == hashedPass(usersData._2, usersData._1)))
        update <- {
         lazy val update = collection.update(BSONDocument("_id" -> user.get.id), 
         BSONDocument("$set" -> 
         BSONDocument("lastLogin" -> BSONDateTime(new org.joda.time.DateTime().getMillis()))))
         update
        }
        result <- {
         lazy val result = gotoLoginSucceeded(user.get.id.stringify)
         result
        } 
      } yield
        if(user.isEmpty) BadRequest(views.html.logon(loginForm.withGlobalError("login\pass mismatch")))
        else if(!user.get.active) BadRequest(views.html.logon(loginForm.withGlobalError("inactive")))
        else if(update.err.isEmpty) result
        else  InternalServerError(views.html.logon(loginForm.withGlobalError("server error")))
        })

}

Feisty

How about breaking it down into several smaller functions? – Wellread

That looks like perfectly good code to me. It could maybe benefit from refactoring some of those blocks into methods, as EECOLOR has done, but otherwise I can't see anything wrong with it. What is it that's bothering you about it – Virgenvirgie

Answer 1

I would probably refactor the code into something like this:

def authenticate() = Action.async { implicit request => 
  loginForm.bindFromRequest.fold(
     hasErrors = displayFormWithErrors,
     success = loginUser)
}  

private def displayFormWithErrors[T](errors:Form[T]) = 
  Future.successful(BadRequest(views.html.logon(errors)))

private def loginUser(userData:(String, String)) = {
  val (username, password) = userData

  findUser(username, password)
    .flatMap {
      case None => 
        showLoginFormWithError("user/pass incorect!!!")
      case Some(user) if (!user.active) =>
        showLoginFormWithError("inactive!!!")
      case Some(user) =>
        updateUserAndRedirect(user)
  }
}

private def findUser(username:String, password:String) =
  this.collection
    .find(BSONDocument("name" -> username))
    .one[Account]
    .map(_.filter(_.password == hashedPass(password, username)))

private def showLoginFormWithError(error:String) = 
  Future.successful(BadRequest(
    views.html.logon(loginForm.withGlobalError(error))))

private def updateUserAndRedirect(user:Account) = 
  updateLastLogin(user)
    .flatMap(_ => gotoLoginSucceeded(user.id.stringify))

private def updateLastLogin(user:Account) = 
  collection
    .update(BSONDocument("_id" -> user.id), 
              BSONDocument("$set" -> 
              BSONDocument("lastLogin" -> 
              BSONDateTime(new JodaDateTime().getMillis()))))

Pronty

Looks like my first solution. – Feisty

Answer 2

I prefer doing password & user validation in the form validation clauses -- would be something like this (untested, but you get the idea):

private val loginForm = Form(
  mapping(
    "name" -> nonEmptyText,
    "password" -> nonEmptyText
  ){
    (name, password) => (
        this.collection.find(BSONDocument("name" -> name)).one[Account],
        password)
  }{
    data => Some((data._1.name, data._2))
  }.verifying(new Constraint(None, Seq())({
    data: (Option[Account], String) => data match {
      case (Some(account: Account), _) if !account.active => Invalid(ValidationError("inactive"))
      case (Some(account: Account), password) if account.password==hashedPass(account.name, password) => Valid
      case _ => Invalid(ValidationError("login/pass mismatch"))
    }
  }))
)

And then the controller becomes much simpler:

def authenticate() = Action.async { implicit request => 
  loginForm.bindFromRequest.fold(
    errors => Future.successful(BadRequest(views.html.logon(errors))),
    usersData =>{
      collection.update(BSONDocument("_id" -> usersData._1.id), 
                        BSONDocument("$set" -> 
                        BSONDocument("lastLogin" -> BSONDateTime(new org.joda.time.DateTime().getMillis()))))
                .flatMap(x => gotoLoginSucceeded(user.id.stringify))

    }
  )
}

Toponym

Compilation error. this.collection.find(BSONDocument("name" -> name)).one[Account] : Future[Option[Account]] – Feisty

I don't think that DB interaction is something that fits very well as a part of form validation, and also, as sh1ng says it will only work if you are using a synchronous/blocking db client. – Wadding