Prelude
I'll assume the following code :
trait BaseModel{
def pk:String
def id:String=pk
}
case class Account(name:String) extends BaseModel{
override val pk = name
}
As the internal structure of the Account
class. I made it extends BaseModel
so it can be used with the following fake DAO and my sample compiles. I had to add an insert which takes both an Account and a company Id.
class AccountDAO extends CRUDDAO[Account]{
override def insert(model: Account): Future[Any] = Future.successful(model)
def insert(model: Account,companyId:String): Future[Any] = model.id match{
case "account1"|"account2" => Future.successful(model)
case _ => Future.failed(new RuntimeException("Unable to save account"))
}
override def update(model: Account): Future[Int] = ???
override def get(pk: String): Future[Option[Account]] = pk match {
case "account1" => Future.successful(Some(Account(pk)))
case "account2" => Future.successful(None)
case _ => Future.failed(new RuntimeException("no such account"))
}
override def all: Future[Seq[Account]] = ???
override def all(page: Int, perPage: Int): Future[Seq[Account]] = ???
}
I made the original code sample compile by adapting the bottom of the code as Created(account)
wouldn't compile here is what it looks like.
Initial code
class Companies @Inject()(primaryDAO:AccountDAO)(implicit ec:ExecutionContext) extends Controller {
implicit val AccountReads = Json.format[Account]
def create = Action.async { implicit request =>
if (request.body.asJson.isEmpty) {
Future.successful(BadRequest("Missing body"))
}
else {
val body = request.body.asJson.get.as[JsObject]
val companyID = (body \ "company" \ "id").validate[String]
val parsedAccount = (body \ "account").validate[Account]
// Check that we have all of the fields we need
if (parsedAccount.isError) {
Future.successful(BadRequest("Missing account data"))
} else if (companyID.isError) {
Future.successful(BadRequest("Missing company data"))
}
else {
// Insert the new account
val account = parsedAccount.get
(for {
_ <- primaryDAO.insert(account, companyID.get)
account <- primaryDAO.get(account.id)
} yield account).map {
case Some(a) => Created(a.id)
case None => InternalServerError("Unable to create Account")
}.recover {
case e => BadRequest(e.getMessage)
}
}
}
}
}
object Companies extends Companies(new AccountDAO)(play.api.libs.concurrent.Execution.defaultContext)
Following are some sample request/reponses using httpie:
$> echo '{"company":{"id":"1"}, "account":{"name":"account1"}}'| http :9000/foobar
HTTP/1.1 201 Created
Content-Length: 8
Content-Type: text/plain; charset=utf-8
Date: Thu, 28 Apr 2016 20:17:19 GMT
account1
$> echo '{"company":{"id":"1"}, "account":{"name":"account2"}}'| http :9000/foobar
HTTP/1.1 500 Internal Server Error
Content-Length: 24
Content-Type: text/plain; charset=utf-8
Date: Thu, 28 Apr 2016 20:18:45 GMT
Unable to create Account
$> echo '{"company":{"id":"1"}, "account":{"name":"account3"}}'| http :9000/foobar
HTTP/1.1 400 Bad Request
Content-Length: 22
Content-Type: text/plain; charset=utf-8
Date: Thu, 28 Apr 2016 20:21:19 GMT
Unable to save account
$> echo '{ "account":{"name":"account3"}}'| http :9000/foobar
HTTP/1.1 400 Bad Request
Content-Length: 20
Content-Type: text/plain; charset=utf-8
Date: Thu, 28 Apr 2016 20:21:44 GMT
Missing company data
$> echo '{"company":{"id":"1"}, "account":{}}'| http :9000/foobar
HTTP/1.1 400 Bad Request
Content-Length: 20
Content-Type: text/plain; charset=utf-8
Date: Thu, 28 Apr 2016 20:22:07 GMT
Missing account data
Embracing the HTTP framework
Now there is a way to write this using early returns, I'll show it for completeness sake but you really really don't want to do that as I'll explain below. Return doesn't have a type so you are forced to explicitely provide a return type which is impossible in an anonymous function (the block after Action.async
is just an anonymous function). You can easily extract your code to a named method with explicit types and use that as the action body:
class Companies @Inject()(primaryDAO:AccountDAO)(implicit ec:ExecutionContext) extends Controller {
implicit val AccountReads = Json.format[Account]
def doCreate(implicit request:Request[AnyContent]):Future[Result]={
if (request.body.asJson.isEmpty) {
return Future.successful(BadRequest("Missing body"))
}
val body = request.body.asJson.get.as[JsObject]
val companyID = (body \ "company" \ "id").validate[String]
val parsedAccount = (body \ "account").validate[Account]
// Check that we have all of the fields we need
if (parsedAccount.isError) {
return Future.successful(BadRequest("Missing account data"))
}
if (companyID.isError) {
return Future.successful(BadRequest("Missing company data"))
}
// Insert the new account
val account = parsedAccount.get
(for {
_ <- primaryDAO.insert(account, companyID.get)
account <- primaryDAO.get(account.id)
} yield account).map {
case Some(a) => Created(a.id)
case None => InternalServerError("Unable to create Account")
}.recover {
case e => BadRequest(e.getMessage)
}
}
def create = Action.async(doCreate)
}
object Companies extends Companies(new AccountDAO)(play.api.libs.concurrent.Execution.defaultContext)
I said that doing this is wrong and you really don't want to do this. Quoting Rob Norris (tpolecat) :
If you find yourself in a situation where you think you want to return early, you need to re-think the way you have defined your computation
So let's do some rethinking :) I'll posit that What you really want isn't so much using early returns as it is avoiding deep nesting of if/else clauses.
Let's have a look at the types we are manipulating :
request.body.asJson
returns an Option[JsValue]
. The current implementation tests to check if it is empty and returns a
BadRequest
. Play offers a similar and much cleaner way to check that you actually receive an application/json
body for your endpoint (I'll leave the wrapping code to concentrate on the action itself for now) using a specific body parser:
def create = Action.async(parse.json) { implicit request =>
val body = request.body
val companyID = (body \ "company" \ "id").validate[String]
val parsedAccount = (body \ "account").validate[Account]
// Check that we have all of the fields we need
if (parsedAccount.isError) {
Future.successful(BadRequest("Missing account data"))
}
if (companyID.isError) {
Future.successful(BadRequest("Missing company data"))
} else {
// Insert the new account
val account = parsedAccount.get
(for {
_ <- primaryDAO.insert(account, companyID.get)
account <- primaryDAO.get(account.id)
} yield account).map {
case Some(a) => Created(a.id)
case None => InternalServerError("Unable to create Account")
}.recover {
case e => BadRequest(e.getMessage)
}
}
}
Using a body parser in the action will enforce the media type for the endpoint (in this case it will have to be a form of json). Trying to call it with a content type such as application/x-www-form-urlencoded
will fail with a 415 Unsupported Media Type
error, passing an invalid json body will yield a 400 BadRequest
for you :
$> echo 'coucou'| http --form :9000/foobar
HTTP/1.1 415 Unsupported Media Type
Content-Length: 2163
Content-Type: text/html; charset=utf-8
Date: Thu, 28 Apr 2016 20:41:15 GMT
$> echo "coucou"| http :9000/foobar
HTTP/1.1 400 Bad Request
Content-Length: 2289
Content-Type: text/html; charset=utf-8
Date: Thu, 28 Apr 2016 20:43:30 GMT
Embracing the Json library
The next step consists of leveraging play-json's validation facilities. First let's define a reader which enforces all the protocol constraints for your endpoint :
import play.api.libs.json._
import play.api.libs.functional.syntax._
implicit val CreateDTOReads =
(
(__ \ "company" \ "id").read[String] and
(__ \ "account").read[Account]
).tupled
Now we can use that to fully validate the incoming payload and reject it if it is incorrect:
def create = Action.async(parse.json) { implicit request =>
val createDto:JsResult[(String,Account)] = request.body.validate(CreateDTOReads)
// Check that we have all of the fields we need
if (createDto.isError) {
Future.successful(BadRequest("Missing account or company data"))
} else {
// Insert the new account
val (companyId,account) = createDto.get
(for {
_ <- primaryDAO.insert(account, companyId)
account <- primaryDAO.get(account.id)
} yield account).map {
case Some(a) => Created(a.id)
case None => InternalServerError("Unable to create Account")
}.recover {
case e => BadRequest(e.getMessage)
}
}
}
Notice that at this point we have lost a bit of precision since I don't distinguish between the two errors anymore. The information is still there, captured in the errors of the JsResult
. You could use pattern matching or even a cast to get a JsError
out of the JsResult
and once you have a JsError
you get the list of all validation errors for each path which you can manipulate and translate as you like. For instance :
def create = Action.async(parse.json) { implicit request =>
val createCommand:JsResult[(String,Account)] = request.body.validate(CreateDTOReads)
// Check that we have all of the fields we need
if (createCommand.isError) {
val errors = createCommand.asInstanceOf[JsError]
Json.prettyPrint(JsError.toJson(errors))
Future.successful(BadRequest(Json.prettyPrint(JsError.toJson(errors))))
} else {
// Insert the new account
val (companyId,account) = createCommand.get
(for {
_ <- primaryDAO.insert(account, companyId)
account <- primaryDAO.get(account.id)
} yield account).map {
case Some(a) => Created(a.id)
case None => InternalServerError("Unable to create Account")
}.recover {
case e => BadRequest(e.getMessage)
}
}
}
returns something like :
$> echo '{"coucou":""}'| http :9000/foobar
HTTP/1.1 400 Bad Request
Content-Length: 173
Content-Type: text/plain; charset=utf-8
Date: Fri, 29 Apr 2016 08:41:35 GMT
{
"obj.account" : [ {
"msg" : [ "error.path.missing" ],
"args" : [ ]
} ],
"obj.company.id" : [ {
"msg" : [ "error.path.missing" ],
"args" : [ ]
} ]
}
This is still not looking very nice since this is not the idiomatic way to extract information from a JsResult. The proper way is to fold over the JsResult
. The fold method signature on a JsResult is fold[X](errors: (Seq[(JsPath, Seq[ValidationError])]) => X, valid: (A) => X): X)
. In our case we want X
to be a Future[JsResult]
, and can write it like this :
def create = Action.async(parse.json) { implicit request =>
val createCommandResult:JsResult[(String,Account)] = request.body.validate(CreateDTOReads)
// Check that we have all of the fields we need
createCommandResult.fold(
errors => Future.successful(BadRequest(Json.prettyPrint(JsError.toJson(errors)))),
createCommand => {
val (companyId,account) = createCommand
(for {
_ <- primaryDAO.insert(account, companyId)
account <- primaryDAO.get(account.id)
} yield account).map {
case Some(a) => Created(a.id)
case None => InternalServerError("Unable to create Account")
}.recover {
case e => BadRequest(e.getMessage)
}
}
)
}
Single Responsibility Principle
Now we are getting there but the valid case is not looking so good. This is because the create action handles too many responsibilities. At the REST endpoint level you should only handle HTTP protocol concerns :
- content negotiation
- deserialization of the payload (can include some validation)
- serialization of the responses
Let's extract the business logic, however simple, to an AccountService
class :
@Singleton
class AccountService @Inject() (primaryDAO: AccountDAO){
def createAccount(companyId:String,account:Account)(implicit ec: ExecutionContext) : Future[Option[Account]] =
for {
_ <- primaryDAO.insert(account, companyId)
account <- primaryDAO.get(account.id)
} yield account
}
Now our endpoint only handles the HTTP translation logic :
def create = Action.async(parse.json) { implicit request =>
val createCommandResult:JsResult[(String,Account)] = request.body.validate(CreateDTOReads)
// Check that we have all of the fields we need
createCommandResult.fold(
errors => Future.successful(BadRequest(Json.prettyPrint(JsError.toJson(errors)))),
createCommand => {
val (companyId,account) = createCommand
val createdAccountF: Future[Option[Account]] = accountService.createAccount(companyId, account)
createdAccountF.map {
case Some(a) => Created(a.id)
case None => InternalServerError("Unable to create Account")
}.recover {
case e => BadRequest(e.getMessage)
}
}
)
}
The error handling code:
createdAccountF.map {
case Some(a) => Created(a.id)
case None => InternalServerError("Unable to create Account")
}.recover {
case e => BadRequest(e.getMessage)
}
Is a good candidate for abstraction. If you wanted to always return Json for instance you could have the following :
object JsonResultMapper extends Results {
import play.api.libs.json.Writes
def jsonOk[A](subject: A)(implicit writer: Writes[A]) = Ok(Json.toJson(subject))
def jsonNotfound(msg: String) = NotFound(Json.obj("reason" -> msg))
def exception2Location(exception: Exception): String =
Option(exception.getStackTrace)
.flatMap(_.headOption)
.map(_.toString)
.getOrElse("unknown")
def jsonInternalServerError(msg: String, cause: Exception) = {
val jsonMsg = Json.obj(
"reason" -> msg,
"location" -> exception2Location(cause)
)
InternalServerError(jsonMsg)
}
def toJsonResult[A](subjectOptionFuture: Future[Option[A]],noneMsg: => String = "NotFound")
(implicit writer: Writes[A]): Future[SimpleResult] = {
subjectOptionFuture.map {
case Some(subject) => jsonOk(subject)
case None => jsonNotfound(noneMsg)
}.recover {
case e: Exception => jsonInternalServerError(e.getMessage, e)
}
}
}
and then write your action as :
def create = Action.async(parse.json) { implicit request =>
val createCommandResult:JsResult[(String,Account)] = request.body.validate(CreateDTOReads)
// Check that we have all of the fields we need
createCommandResult.fold(
errors => Future.successful(BadRequest(Json.prettyPrint(JsError.toJson(errors)))),
createCommand => {
val (companyId,account) = createCommand
val createdAccountF: Future[Option[Account]] = accountService.createAccount(companyId, account)
JsonResultMapper.toJsonResult(createdAccountF, s"Unable to create account")
}
)
}
You could stop there and I will for the purpose of this review, but there are still things which can probably be improved. I'll give you a couple leads to further improve:
PrimaryDao protocol
The Future[Option[Account]]
may not be a good signature for primaryDAO.get(account.id)
or for AccountService#Create
.
As you can see, it has 2 errors paths and 1 happy path. However when serializing it, the happy path and first error path are processed together in the same block, then the second error path (exception raised) in a different block.
Some would argue that one error path is a business error while the other is a technical error which makes it ok.
Whether we hide this in the ResultMapper or not I personally don't like it. To get rid of it an depending on your team standards, you can go for:
In the same logic Future[Any]
is not a very good signature for :
trait DAOInsert[A <: BaseModel] extends DAOGet[A] {
def insert(model: A): Future[Any]
}
I strongly suggest changing that to
trait DAOInsert[A <: BaseModel] extends DAOGet[A] {
def insert(model: A): Future[A]
}
and having the insert return the saved instance if it is possible. This would allow you to distinguish between : there was an error while inserting vs I couldn't read the instance which are not necessarily the same.
Using specific types
customerId is a string which carries very little information, creating and using a CustomerId
type would probably prove very useful if it is used throughout your application.
Disclaimer
I don't know enough of the business to properly name things in my refactoring. Naming is probably the single most important thing when writing code and it is known to be one of the hardest (with invalidating caches).
Final code
package controllers.company
import scala.concurrent.{ExecutionContext, Future}
import com.google.inject.{Inject, Singleton}
import play.api.libs.json.Json
import play.api.mvc.{Action, Controller}
trait BaseModel {
def pk: String
def id: String = pk
}
case class Account(name: String) extends BaseModel {
override val pk = name
}
trait DAOGet[A <: BaseModel] {
def get(pk: String): Future[Option[A]]
def all: Future[Seq[A]]
def all(page: Int, perPage: Int): Future[Seq[A]]
}
trait DAOInsert[A <: BaseModel] extends DAOGet[A] {
def insert(model: A): Future[Any]
}
trait DAOUpdate[A <: BaseModel] extends DAOGet[A] {
def update(model: A): Future[Int]
}
trait DAODelete[A <: BaseModel] {
def delete(pk: String): Future[Int]
}
trait CRUDDAO[A <: BaseModel] extends DAOGet[A] with DAOInsert[A] with DAOUpdate[A]
class AccountDAO extends CRUDDAO[Account] {
override def insert(model: Account): Future[Any] = Future.successful(model)
def insert(model: Account, companyId: String): Future[Any] = model.id match {
case "account1" | "account2" => Future.successful(model)
case _ => Future.failed(new RuntimeException("Unable to save account"))
}
override def update(model: Account): Future[Int] = ???
override def get(pk: String): Future[Option[Account]] = pk match {
case "account1" => Future.successful(Some(Account(pk)))
case "account2" => Future.successful(None)
case _ => Future.failed(new RuntimeException("no such account"))
}
override def all: Future[Seq[Account]] = ???
override def all(page: Int, perPage: Int): Future[Seq[Account]] = ???
}
@Singleton
class AccountService @Inject() (primaryDAO: AccountDAO){
def createAccount(companyId:String,account:Account)(implicit ec: ExecutionContext) : Future[Option[Account]] =
for {
_ <- primaryDAO.insert(account, companyId)
account <- primaryDAO.get(account.id)
} yield account
}
@Singleton
class Companies @Inject()(accountService: AccountService)(implicit ec: ExecutionContext) extends Controller {
implicit val AccountReads = Json.format[Account]
import play.api.libs.functional.syntax._
import play.api.libs.json._
implicit val CreateDTOReads =
(
(__ \ "company" \ "id").read[String] and
(__ \ "account").read[Account]
).tupled
def create = Action.async(parse.json) { implicit request =>
val createCommandResult:JsResult[(String,Account)] = request.body.validate(CreateDTOReads)
// Check that we have all of the fields we need
createCommandResult.fold(
errors => Future.successful(BadRequest(Json.prettyPrint(JsError.toJson(errors)))),
createCommand => {
val (companyId,account) = createCommand
val createdAccountF: Future[Option[Account]] = accountService.createAccount(companyId, account)
createdAccountF.map {
case Some(a) => Created(a.id)
case None => InternalServerError("Unable to create Account")
}.recover {
case e => BadRequest(e.getMessage)
}
}
)
}
}
object Companies extends Companies(new AccountService(new AccountDAO()))(play.api.libs.concurrent.Execution.defaultContext)