Handling a case of when my future returns a None in my play controller action method,

Question

I want to refactor by update action below to look a little more readable and also handle the failure case better

The userService has the following functions:

class UserService {
    def getUserByUsername: Future[Option[Int]] // which is the UserId
    def getUserById: Future[User]
}

My action looks like:

def update(userId: Int) = Action.async { implicit request =>
    request.body.validate[User] match {
        case JsSuccess(user, _) => {
            userService.getUserByUsername(user.username).map { userId => 
                userService.getUserById(userId.get).map { existingUser =>
                    userService.update(user.username)

                    Ok
                }
            }
        }
        case JsError(err) => Future.sucessful(BadRequest(err))
    }
}

1. How do I handle the situation where getUserByUsername returns a None?
2. Would this look cleaner if it was in a for comprehension, is it better style?

Answer 1

You have some missing data in your questions such as case classes for the User model, userService class. also better to attach the original function.

Anyways, I will do something as follows:

 def update(userId: Int) = Action { implicit request =>
    request.body.validate[User] match {
      case JsSuccess(user: User, _) => {
        val userId = getUserByUsername(user.username)
        userId match {
          case Some(userId) => {
            for {
              _ <- userService.getUserById(userId)
              _ <- userService.update(user.username)
            } yield Ok
          }.recover {
            case t: Throwable => 
              Metrics.errOnUpdate.increment() // Some metric to monitor 
              logger.error(s"update userId: $userId failed with ex: ${t.getMessage}") // log the error 
              InternalServerError(Json.toJson(Json.obj("error" -> "Failure occured on update"))) // return custom made exception to the client
          }
          case None => Future.successful(NotFound(s"No such user with ${user.username}"))
        }
      }
      case JsError(err) => Future.sucessful(BadRequest(err))
    }
  }

Note: If .update returns Future, you actually not waiting to update before returning Ok to the user, thus, if its fails, its still returns Ok. To fix that, use flatMap and then map the value of update response.

You can also separate the recovering for the getUserById and update if you prefer.

---

Edit:

  def update(userId: Int) = Action { implicit request =>
    request.body.validate[User] match {
      case JsSuccess(user: User, _) => {
          getUserByUsername(user.username).flatMap {
             case Some(userId) => for {
                _ <- userService.getUserById(userId)
                _ <- userService.update(user.username)
              } yield Ok
             case None => Future.successful(NotFound(s"No such user with ${user.username}"))
          }
        }.recover {
          case t: Throwable =>
            Metrics.errOnUpdate.increment() // Some metric to monitor 
            logger.error(s"update userId: $userId failed with ex: ${t.getMessage}") // log the error 
            InternalServerError(Json.toJson(Json.obj("error" -> "Failure occured on update"))) // return custom made exception to the client
        }
      }
      case JsError(err) => Future.sucessful(BadRequest(err))
    }
  }