Implementation of API to create a company account in a database

Question

I'm working on an API that has a lot of controller functions like this:

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 account => Created(account)
            }.recover {
                case e => BadRequest(e)
            }
        }
    }
}

I was hoping that I would be able to do something more like this (using early returns):

def create = Action.async { implicit request =>

    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 account => Created(account)
    }.recover {
        case e => BadRequest(e)
    }
}

However this is not possible because the return statement only returns from my nested function (and back into the Action.async)

I am wondering what I can do in place of early returns (which I would use in imperative programming languages) to make my code cleaner.

The primary DAO implements a generic trait that I use for most of my DAOs and looks like this:

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] with DAODelete[A]

I'll finish and post my answer after diner in a couple hours — Jean, Apr 28 '16 at 18:51

Jean · Accepted Answer · 2016-04-29 09:26:10Z

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:

A BusinessException such as AccountNotFound which is thrown instead of returning an Option
A custom composition of Future and Option (see http://www.edofic.com/posts/2014-03-07-practical-future-option.html and http://loicdescotte.github.io/posts/scala-compose-option-future/ )
A ScalaZ monad transformer which does the same as the previous option in a generic way

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)

\$\begingroup\$ This is one of the best answers I have seen on SE \$\endgroup\$ – TylerAndFriends Apr 29 '16 at 15:55 — TylerAndFriends, Apr 29 '16 at 15:55

Stack Exchange Network

current community

your communities

more stack exchange communities

Implementation of API to create a company account in a database

1 Answer 1

Prelude

Initial code

Embracing the HTTP framework

Embracing the Json library

Single Responsibility Principle

PrimaryDao protocol

Using specific types

Disclaimer

Final code

Your Answer

Not the answer you're looking for? Browse other questions tagged validation error-handling scala asynchronous or ask your own question.

Hot Network Questions

Implementation of API to create a company account in a database

1 Answer 1

Prelude

Initial code

Embracing the HTTP framework

Embracing the Json library

Single Responsibility Principle

PrimaryDao protocol

Using specific types

Disclaimer

Final code

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged validation error-handling scala asynchronous or ask your own question.

Related

Hot Network Questions