-
-
Save loicknuchel/faa974c3661351b73227 to your computer and use it in GitHub Desktop.
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) | |
} | |
} |
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")) | |
} | |
} | |
} |
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")) | |
} | |
} | |
} |
One proposition: i like this way because it's very simple : just basic functions, no Monad/Builder, etc
https://gist.github.com/julienlafont/154700fcab410ee06c20
BTW I'm sure there is a way to keep the same idea but with composability, something like
(WithEvent(eventId) and WithSession(sessionId)).map { case (event, session) => Ok(...) }
(I've one other in head, i post it asap)
Suggestion from @muhuk :
def details(eventId: String, sessionId: String) = SecuredAction.async { implicit req =>
implicit val user = req.identity
for {
Some(event) <- EventRepository.getByUuid(eventId)
Some(session) <- SessionRepository.getByUuid(sessionId)
} yield {
Ok(views.html.Sessions.details(session, event))
}
}
The for-comprehension return a Future[Result]
and not an Option[Future[Result]]
so if I get a None I get : NoSuchElementException: Future.filter predicate is not satisfied
:(
you can recover the failed future with .recover { case NoSuchElementException => NotFound("blabla") }
.
But it's a bit brutish, and doesn't allow to handle each atomic errors.
@StudioDev Where do you add the .recover
?
@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 Option
s and Future
s 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...).
Many of my Play actions looks like this and I'm wondering if there is a way for an improved & shorter syntax...
My "problem" is on line 8, 20 and 24, I need to assign the result of the for-comprehension to an intermediate value to call
getOrElse
on it (I can also add parenthesis but it's less lisible I think). I've got a similar problem withFuture
because I can'tflatMap
the result of theyield
, I would have hoped that thisflatMap
is not necessary...Do you know a way to "improve" this code ?
Is there a way to
flatMap
theyield
result and to chain function after theyield
?Thanks for all :)