Scala for-comprehension

Sessions.scala

object Sessions extends SilhouetteEnvironment {
  def details(eventId: String, sessionId: String) = SecuredAction.async { implicit req =>
    implicit val user = req.identity
    val res: Future[Result] = for {
      eventOpt: Option[Event] <- EventRepository.getByUuid(eventId)
      sessionOpt: Option[Session] <- SessionRepository.getByUuid(sessionId)
    } yield {
      var res2: Option[Result] = for {
        event: Event <- eventOpt
        session: Session <- sessionOpt
      } yield {
        Ok(backend.views.html.Events.Sessions.details(session, List(), event))
      }
      res2.getOrElse { NotFound(views.html.error("404", "Event not found...")) }
    }
    res
  }
  
  def doUpdate(eventId: String, sessionId: String) = SecuredAction.async { implicit req =>
    implicit val user = req.identity
    val res: Future[Future[Result]] = for {
      eventOpt: Option[Event] <- EventRepository.getByUuid(eventId)
      sessionOpt: Option[Session] <- SessionRepository.getByUuid(sessionId)
    } yield {
      val res2: Option[Future[Result]] = for {
        event: Event <- eventOpt
        session: Session <- sessionOpt
      } yield {
        createForm.bindFromRequest.fold(
          formWithErrors => Future(BadRequest(views.html.Sessions.update(formWithErrors, session, event))),
          formData: Session => SessionRepository.update(sessionId, formData).map { err: LastError =>
            Redirect(controllers.routes.Sessions.details(eventId, sessionId))
          })
      }
      res2.getOrElse(Future(NotFound(views.html.error("404", "Event not found..."))))
    }
    res.flatMap(identity)
  }
}

SessionsByMuhuk.scala

object Sessions extends SilhouetteEnvironment {
  def details(eventId: String, sessionId: String) = SecuredAction.async { implicit req =>
    implicit val user = req.identity
    val res: Future[Result] = for {
      Some(event: Event) <- EventRepository.getByUuid(eventId)
      Some(session: Session) <- SessionRepository.getByUuid(sessionId)
    } yield {
      Ok(backend.views.html.Events.Sessions.details(session, List(), event))
    }
    // throws 'NoSuchElementException: Future.filter predicate is not satisfied' on None :(
    res.recover {
      case NoSuchElementException => Future(NotFound("Not found"))
    }
  }
  
  def doUpdate(eventId: String, sessionId: String) = SecuredAction.async { implicit req =>
    implicit val user = req.identity
    val res: Future[Future[Result]] = for {
      Some(event: Event) <- EventRepository.getByUuid(eventId)
      Some(session: Session) <- SessionRepository.getByUuid(sessionId)
    } yield {
      createForm.bindFromRequest.fold(
        formWithErrors => Future(BadRequest(views.html.Sessions.update(formWithErrors, session, event))),
        formData: Session => SessionRepository.update(sessionId, formData).map { err: LastError =>
          Redirect(controllers.routes.Sessions.details(eventId, sessionId))
        })
    }
    res.flatMap(identity).recover {
      case NoSuchElementException => Future(NotFound("Not found"))
    }
  }
}

SessionsByStudiodev.scala

object Sessions extends SilhouetteEnvironment {
  def details(eventId: String, sessionId: String) = SecuredAction.async { implicit req =>
    implicit val user = req.identity
    withEvent(eventId) { event =>
      withSession(sessionId) { session =>
        Ok(backend.views.html.Events.Sessions.details(session, List(), event))
      }
    }
  }

  def doUpdate(eventId: String, sessionId: String) = SecuredAction.async { implicit req =>
    implicit val user = req.identity
    withEvent(eventId) { event =>
      withSession(sessionId) { session =>
        createForm.bindFromRequest.fold(
          formWithErrors => Future(BadRequest(views.html.Sessions.update(formWithErrors, session, event))),
          formData: Session => SessionRepository.update(sessionId, formData).map { err: LastError =>
            Redirect(controllers.routes.Sessions.details(eventId, sessionId))
          })
      }
    }
  }
  
  def withEvent(eventId: String)(block: Event => Future[Result]): Future[Result] = {
    EventRepository.getByUuid(eventId).flatMap {
      case Some(event: Event) => block(event)
      case None => Future(NotFound("Event not found"))
    }
  }

  def withSession(sessionId: String)(block: Session => Future[Result]): Future[Result] = {
    SessionRepository.getByUuid(sessionId).flatMap {
      case Some(session: Session) => block(session)
      case None => Future(NotFound("Session not found"))
    }
  }
}

@loicknuchel the last part (yield block) is a map(), it seems you need to turn it into a flatMap().

def doUpdate(eventId: String, sessionId: String) = SecuredAction.async { implicit req =>
  implicit val user = req.identity
  val res: Future[Result] = for {
    Some(event: Event) <- EventRepository.getByUuid(eventId)
    Some(session: Session) <- SessionRepository.getByUuid(sessionId)
    x <- createForm.bindFromRequest.fold(
           formWithErrors => Future(BadRequest(views.html.Sessions.update(formWithErrors, session, event))),
           formData: Session => SessionRepository.update(sessionId, formData).map { err: LastError =>
             Redirect(controllers.routes.Sessions.details(eventId, sessionId))
           }
         )
    y <- identity(x)
  } yield {
    y
  }
}

To get rid of the Future[Future[_]]. I don't have a Scala IDE open right now, but I guarantee that this will not work. Because you can't mix Options and Futures in a single for comprehension.

At this point I think @StudioDev's solution is the best way to go.

BTW you were not fetching event & session in parallel anyway. A for-comprehension is a series of flatMaps (for each bind, <-) and then a map (for the yield block). So it's still sequential. If you want parallellism you need to move async calls outside of the for:

val sumtinFuture: Future[Sumtin] = gimmeSumtin()
val otherFuture: Future[Other] = gimmeOther()

for {
  sumtin <- sumtinFuture
  other <- otherFuture
} yield { 
  doIt(sumtin, other)
}

Also a type of Option[Future[_]] is weird.

Like that:

(for {
      Some(event: Event) <- EventRepository.getByUuid(eventId)
      Some(session: Session) <- SessionRepository.getByUuid(sessionId)
    } yield {
      Ok(backend.views.html.Events.Sessions.details(session, List(), event))
    }
).recover {
 case NoSuchElementException => NotFound("blabla")
}

But I don't really like this solution, because you lost the control on the error (I hate APIs that respond: "something got wrong, figure it out yourself and fuck off").

One quick improvement is to not return an Option in your 'getByUUID' but a failed future with custom exceptions, something like:

trait BusinessException extends RuntimeException {
  val errorResult: Result
}

case object EventNotFound extends BusinessException {
  val errorResult = NotFound("event not found")
}

Write your action with just :

def toto = BusinessAction {
 for {
      event: Future[Event] <- EventRepository.getByUuid(eventId)
      session: Future[Session] <- SessionRepository.getByUuid(sessionId)
    } yield {
      Ok(backend.views.html.Events.Sessions.details(session, List(), event))
    }
}

where BusinessAction is a custom action which check if the action throw a BusinessException.

  val BusinessAction = new ActionBuilder[Request] {
    override def invokeBlock[A](request: Request[A], block: (Request[A]) => Future[Result]) = {
      block(request).recover {
        case be: BusinessException => be.errorResult
      }
    }
  }

How about returning a Future[Option[_]] instead of Option[Future[_]]?

for {
  eventOpt <- EventRepository.getByUuid(eventId)
  sessionOpt <- SessionRepository.getByUuid(sessionId)
} yield {
  (eventOpt, sessionOpt) match {
    case (Some(event), Some(session)) => ... // success case
    case _ => ... // error case
  }
}

I'm not a huge fan to write custom actions, custom exceptions... Just for that.
For now, I think the @StudioDev solution is quite good (custom & centralized errors, simple actions)

@muhuk: Nice solution (pattern matching in yield) but it's still verbose and custom errors will be a lot more verbose...

Thanks a lot for your time, if you have other ideas, I'm open :)

Monad transformer

import scalaz.OptionT
import scalaz.Scalaz.ToIdOps
import scalaz.std.scalaFuture._

    def fOptionT[A](x: Future[Option[A]]): OptionT[Future,A] = OptionT[Future,A](x)

    def details(eventId: String, sessionId: String) = SecuredAction.async { implicit req =>
        implicit val user = req.identity
        val res = for {
            event     <- EventRepository.getByUuid(eventId)             |> fOptionT
            session   <- SessionRepository.getByUuid(sessionId)         |> fOptionT
        } yield Ok(backend.views.html.Events.Sessions.details(session, List(), event))

        res.getOrElse { NotFound(views.html.error("404", "Event not found...")) }
    }

La solution avec les Monad transformer est pas mal (assez proche de celle de @muhuk finalement) mais la gestion d'erreur n'est pas évidente.
La librairie https://github.com/Kanaka-io/play-monadic-actions répond bien à cette problématique mais fait malgré tout monter le projet en complexité pour un résultat proche de ce que @StudioDev propose...

Encore merci à tous :)

    import play.api.libs.concurrent.Execution.Implicits.defaultContext
    import scalaz.OptionT
    import scalaz.Scalaz.{ToIdOps, ToEitherOps}
    import scalaz.{\/, EitherT}
    import scalaz.std.scalaFuture._

    def fOptionT[A](x: Future[Option[A]]): OptionT[Future,A] = OptionT[Future,A](x)
    def fEitherT[E,A](x: Future[E \/ A]) = EitherT[Future, E, A](x)
    def toEither[E, A](e: E)(x: OptionT[Future, A]): EitherT[Future, E, A] = x.fold(some => some.right[E], e.left[A]) |> fEitherT[E,A]


    def details(eventId: String, sessionId: String) = SecuredAction.async { implicit req =>
        implicit val user = req.identity
        val res = for {
            event <- EventRepository.getByUuid(eventId)         |> fOptionT |> toEither("event nor found")
            session <- SessionRepository.getByUuid(sessionId)   |> fOptionT |> toEither("session not found")
        } yield Ok(backend.views.html.Events.Sessions.details(session, List(), event))

        res.fold(
            e => NotFound(views.html.error("404", e),
            r => r
        )
    }

Merci @ubourdon, effectivement ça commence à être sympa :)

Mais je trouve que

def details(eventId: String, sessionId: String) = SecuredAction.async { implicit req =>
  implicit val user = req.identity
  withEvent(eventId) { event =>
    withSession(sessionId) { session =>
      Ok(backend.views.html.Events.Sessions.details(session, List(), event))
    }
  }
}

est quand même plus simple et compréhensible... Et n'importe qui peut le comprendre :D

Tu fais comme tu veux. :)
Je pense que tu te trompes par contre ;)

Les exception plutot que les types ...
l'imbrication de flatMap plutot qu'une notation impérative ...
Un code non réutilisable dans d'autres cas ...

Avec les monadT le code est générique et ne marche pas qu'avec des sessions et des events

Je comprends pas pourquoi tu parles d'exceptions... Il n'y en a pas... Et tout est typé... non ?
Pour le code réutilisable, effectivement il faut écrire un with??? pour chaque type mais c'est pas très lourd et c'est le seul inconvénient que je vois...

Après c'est tout un débat... Qu'on pourra avoir à l'occasion ;)

Pour les exception c'est parce que c'est ce que j'avais lu de la proposition de @StudioDev.
Pour le code réutilisable, si tu me dis que ce n'est pas un critère important, je ne peux rien faire pour toi :) Surtout quand ça coute même pas 3 ligne de code.
Ensuite le notation imbriquée, dès que tu dépasses 2 appels de service ça devient complètement illisible alors qu'avec une notation for-yield tu écris du code impératif très lisible.

Hi, sorry to be so late to the party.

So, using (sorry for the self promotion) https://github.com/Kanaka-io/play-monadic-actions, your example would look something like (typing this directly here, with the help of no compiler, please excuse the probable compile errors) :

object Sessions extends SilhouetteEnvironment with MonadicActions  {
  def details(eventId: String, sessionId: String) = SecuredActionM { implicit req =>
    implicit val user = req.identity
    for {
      event <- EventRepository.getByUuid(eventId) ?| NotFound(s"no event found with Id $eventId")
      session <- SessionRepository.getByUuid(sessionId) ?| NotFound(s"no session found with Id $sessionId")
    } yield Ok(backend.views.html.Events.Sessions.details(session, List(), event))
  }

  def doUpdate(eventId: String, sessionId: String) = SecuredActionM { implicit req =>
    implicit val user = req.identity
    for {
      event <- EventRepository.getByUuid(eventId) ?| NotFound(s"no event found with Id $eventId")
      session <- SessionRepository.getByUuid(sessionId) ?| NotFound(s"no session found with Id $sessionId")
      formData <- createForm.bindFromRequest ?| (err => BadRequest(views.html.Sessions.update(err, session, event))
      _ <- SessionRepository.update(sessionId, formData) ?| (throwable => InternalServerError("unable to update session : ${throwable.getMessage}"))
    } yield Redirect(controllers.routes.Sessions.details(eventId, sessionId))
}

I may have (lazily) forgotten to render errors using a template, but an interesting outcome of this mechanic translation is the discovery that the incidental complexity of the vanilla-scala solution made you forget some error cases that were not properly handled in your example. Using for-comprehensions along with the ?| operator forces you to think about every little edge case (otherwise it doesn't compile) which, after about a year of continued usage, I feel very helpful, since us careless programmers have quite a hard time thinking outside the happy path.

This goodness (IMHO) comes with a little price : you've got to write that strange SecuredActionM (the M stands for "monadic"). This is not a big deal, and should definitely be part of the lib at some point, but for the time being, you would need something like :

package object controllers {
  def constructPlayResult(result: HttpResult[Result])(implicit ec: ExecutionContext) = result.run.map {
    _.toEither.merge
  }


  import scala.language.higherKinds

  case class MonadicAction[R[_]](inner: ActionFunction[Request, R]) extends ControllerUtils {

    def apply[A](bodyParser: BodyParser[A])(block: R[A] => HttpResult[Result]) = new Action[A] {
      override def parser = bodyParser

      override def apply(request: Request[A]) = inner.invokeBlock(request, (req: R[A]) => constructPlayResult(block(req)))
    }

    def apply(block: R[AnyContent] => HttpResult[Result]): Action[AnyContent] = apply(BodyParsers.parse.anyContent)(block)

  }
}

Equipped with that, you would simply define SecuredActionM as

val SecuredActionM = MonadicAction(SecuredAction)

@vil1 no shame for promoting good libs :)

Your solution is really elegant and not that nerdy (as some scala code could be...).