Async Controller Play 2.3

Question

Is this example the correct way of writing an Async controller end point in Scala using Play 2.3?

def submitResponse(qkey:String) = CorsAction.async(parse.json){
 req =>
 val json = req.body
 val channel = json.\("section").as[String]
 val currentQuestionKey = Cache.getAs[String](channel).getOrElse("N_A")
 if(!currentQuestionKey.equals(qkey)){
 scala.concurrent.Future{BadRequest("Inactive or Invalid Key")}
 }
 else {
 val futureResponse = scala.concurrent.Future {
 responseService.create(qkey,channel)
 }
 futureResponse.map{response =>
 response match {
 case Failure(x) => { BadRequest("Duplicate " + qkey)}
 case Success(storedResponse) => { Ok(storedResponse)}
 }
 }
 }
 }

• Jean: please provide the signature of responseService.create, based on the pattern matching it looks like responseService returns a Try[T].

Answer 1

Let's assume the following enclosing context for your method, this gives us types to work with (types are very important in scala). Notice I changed back from CorsAction to Action since that shouldn't change anything to the types and I don't have the implementation for CorsAction at hand. Also your code doesn't give enough context to know which execution context you have in scope.

import play.api.cache.Cache
import play.api.mvc.{Action, Controller}
import scala.concurrent.Future
import scala.util.{Try, Success, Failure}
trait ResponseService{
 def create(key:String, channel:String):Try[String]
}
class Temp(responseService:ResponseService) extends Controller {
 def submitResponse(qkey:String) = Action.async(parse.json){ req =>
 ???
 }
}

I assume you have tests for your code so I can start refactoring right away. The first thing is not going through a thread to produce a future for which we know the result and since we have imports we can use short names:

if(!currentQuestionKey.equals(qkey)){
 scala.concurrent.Future{BadRequest("Inactive or Invalid Key")}
}

should really be

if(!currentQuestionKey.equals(qkey)){
 Future.successful(BadRequest("Inactive or Invalid Key"))
}

the Future.apply will execute asynchronously in a different thread capturing exceptions as needed, here that would mean a useless context switch to assign a static value which can not throw exceptions.

The same more or less holds true for

val futureResponse = scala.concurrent.Future {
 responseService.create(qkey,channel)
}
futureResponse.map{response =>
 response match {
 case Failure(x) => { BadRequest("Duplicate " + qkey)}
 case Success(storedResponse) => { Ok(storedResponse)}
 }
}

Because of the pattern matching which is below the Future instantiation, it seems that the return type of responseService.create is a Try[T].

futureResponse currently has a Future[Try[T]] type which encodes redondant information (see below). The futureResponse.map call can be simplified as :

futureResponse.map{
 case Failure(x) => BadRequest("Duplicate " + qkey)
 case Success(storedResponse) => Ok(storedResponse)
}

which eliminates a bit of boilerplate. By the way, curly braces are unneeded around case expression's bodies since there is no "fall through" to the next case once a case has been matched.

Try[T] will catch the same exceptions which would be caught by Future (NonFatal exceptions) therefore I will assume that the responseService.create is a very long operation. If it is not the case, then using an asynchronous controller makes no sense. If it is indeed the case and you control the signature of responseService.create, I suggest having it return a Future[T] instead of a Try[T].

Assuming you can't change the signature of responseService.create, there is still a change (debatable) that can be made.

val futureResponse = Future(responseService.create(qkey,channel).get) 
futureResponse.map( storedResponse => Ok(storedResponse) )
 .recover{ case t:Throwable => BadRequest("Duplicate " + qkey) } 

I assume the type of the exception in the failure doesn't matter since your pattern matching was ignoring it too.

I have avoided Future.fromTry since that would not execute responseService.create asynchronously.

The complete code in context would look like:

import play.api.cache.Cache
import play.api.mvc.{Action, Controller}
import scala.concurrent.Future
import scala.util.{Try, Success, Failure}
trait ResponseService{
 def create(key:String, channel:String):Try[String]
}
class Temp(responseService:ResponseService) extends Controller {
 // in scala 2.11, use
 def fromTry[T](t:Try[T]):Future[T] = {
 t.map(Future.successful).recover{case t:Throwable=>Future.failed(t)}.get
 }
 def submitResponse(qkey:String) = Action.async(parse.json){
 req =>
 val json = req.body
 val channel = json.\("section").as[String]
 val currentQuestionKey = Cache.getAs[String](channel).getOrElse("N_A")
 if(!currentQuestionKey.equals(qkey)){
 Future.successful(BadRequest("Inactive or Invalid Key"))
 }
 else {
 val futureResponse = Future(responseService.create(qkey,channel).get)
 futureResponse.map{ storedResponse => Ok(storedResponse) }
 .recover{ case t:Throwable => BadRequest("Duplicate " + qkey)}
 }
 }
}

Actually there is a bit more which can be improved.

 val json = req.body
 val channel = json.\("section").as[String]

json is never used anywhere else, it can be inlined.

 val channel = req.body.\("section").as[String]

This will throw an exception and return a status 500 if section is not a String.

curl -X POST -H"Content-Type: application/json" -d '{"section":1234}' localhost:9000/temp/ABC

will result in

[error] [play-akka.actor.default-dispatcher-2] play - Cannot invoke the action, eventually got an error: play.api.libs.json.JsResultException: JsResultException(errors:List((,List(ValidationError(error.expected.jsstring,WrappedArray())))))
[error] [play-akka.actor.default-dispatcher-2] application -
! @6l2ocbc00 - Internal server error, for (POST) [/temp/ABC] ->
play.api.Application$$anon1ドル: Execution exception[[JsResultException: JsResultException(errors:List((,List(ValidationError(error.expected.jsstring,WrappedArray())))))]]
 	at play.api.Application$class.handleError(Application.scala:296) ~[play_2.10-2.3.6.jar:2.3.6]
 	at play.api.DefaultApplication.handleError(Application.scala:402) [play_2.10-2.3.6.jar:2.3.6]
 	at play.core.server.netty.PlayDefaultUpstreamHandler$$anonfun3ドル$$anonfun$applyOrElse4ドル.apply(PlayDefaultUpstreamHandler.scala:320) [play_2.10-2.3.6.jar:2.3.6]
 	at play.core.server.netty.PlayDefaultUpstreamHandler$$anonfun3ドル$$anonfun$applyOrElse4ドル.apply(PlayDefaultUpstreamHandler.scala:320) [play_2.10-2.3.6.jar:2.3.6]
 	at scala.Option.map(Option.scala:145) [scala-library.jar:na]
 Caused by: play.api.libs.json.JsResultException: JsResultException(errors:List((,List(ValidationError(error.expected.jsstring,WrappedArray())))))
 	at play.api.libs.json.JsValue$$anonfun2ドル.apply(JsValue.scala:67) ~[play-json_2.10-2.3.6.jar:2.3.6]
 at play.api.libs.json.JsValue$$anonfun2ドル.apply(JsValue.scala:67) ~[play-json_2.10-2.3.6.jar:2.3.6]
 at play.api.libs.json.JsResult$class.fold(JsResult.scala:77) ~[play-json_2.10-2.3.6.jar:2.3.6]
 at play.api.libs.json.JsError.fold(JsResult.scala:13) ~[play-json_2.10-2.3.6.jar:2.3.6]
 at play.api.libs.json.JsValue$class.as(JsValue.scala:65) ~[play-json_2.10-2.3.6.jar:2.3.6]

To avoid that you can use the validation API.

(req.body \ "section").validate[String]

will return a JsResult[String] which is either a JsSuccess(value, path) or a JsError(errors).

You can fold over that or map+getOrElse depending on your tastes. I suggest extracting the successul case to a specific method for improved readability.

The final code looks like :

import play.api.cache.Cache
import play.api.mvc.{Result, Action, Controller}
import scala.concurrent.{ExecutionContext, Future}
import scala.util.{Try, Success, Failure}
class ResponseService{
 def create(key:String, channel:String):Try[String] = Try("x")
}
class Temp(responseService:ResponseService)(implicit app:play.api.Application, executor:ExecutionContext) extends Controller {
 // in scala 2.11, use
 def fromTry[T](t:Try[T]):Future[T] = {
 t.map(Future.successful).recover{case t:Throwable=>Future.failed(t)}.get
 }
 def submitResponse(qkey:String) = Action.async(parse.json){ req =>
 def respondWithChannel(channel:String): Future[Result] = {
 val currentQuestionKey = Cache.getAs[String](channel).getOrElse("N_A")
 if(!currentQuestionKey.equals(qkey)){
 Future.successful(BadRequest("Inactive or Invalid Key"))
 } else {
 val futureResponse = Future(responseService.create(qkey,channel).get)
 futureResponse.map{ storedResponse => Ok(storedResponse) }
 .recover{ case t:Throwable => BadRequest("Duplicate " + qkey)}
 }
 }
 (req.body \ "section").validate[String].fold(
 error => Future.successful(BadRequest("section was not a valid JSON string ")),
 respondWithChannel
 )
 }
}

Notice that I chose to use a class, which means you will have to inject the necessary depencies through the constructor. For local testing, I used a static compile time injection through an object instantiation:

object Temp extends Temp(new ResponseService)(play.api.Play.current, 
play.api.libs.concurrent.Execution.defaultContext)

The corresponding route being

POST /temp/:qkey controllers.Temp.submitResponse(qkey:String)

Also note that by using the parse.json body parser any call which is made to your action without providing a json body will be rejected with a 400 error generated in the framework.

• smk: Thanks for such a detailed response Jean. I have so many follow up questions. 1. If it is indeed the case and you control the signature of responseService.create, I suggest having it return a Future[T] instead of a Try[T]." <-- why? Firstly, responseService.create is long operation hence Future. Secondly, it can fail. So I should return Future[Try[T]].. No?
• Jean: Failure is already encoded in a Future. Future and Try are considered Dual one being the Asynchronous version of the other see cstheory.stackexchange.com/questions/20117/… (make sure to read comments on the answer)