Handling HTTP requests and saving user to database

Question

I am using Play Framework and Slick. In userController.scala I am handling HTTP post requests and sending to personRepository.scala. Is this controller and method written in the correct way?

userController.scala

 def register = Action.async { request =>
 var user = new UserInsert("", "", "", "", "", "")
 request.body match {
 case AnyContentAsText(string) => user = userModel(Json.parse(string))
 case AnyContentAsJson(json) => user = userModel(json)
 case _ => BadRequest {
 "Bad Request: " + play.api.http.Status.BAD_REQUEST
 }
 }
 personRepository.create(user.email, user.provider, user.uid, user.facebook_token, user.display_name, user.photo_url).map(i => {
 Ok(Json.toJson(i))
 })
 }
 def userModel(json: JsValue) = {
 new UserInsert((json \ "email").as[String], (json \ "provider").as[String], (json \ "uid").as[String], (json \ "facebook_token").as[String], (json \ "display_name").as[String], (json \ "photo_url").as[String])
 }

personRepository.scala

 def create(
 email: String,
 provider: String,
 uid: String,
 facebook_token: String,
 display_name: String,
 photo_url: String) = db.run {
 val salt = current.configuration.getString("salt")
 val token = BCrypt.hashpw(email, salt.get)
 (user.map(p => (p.token, p.email, p.provider, p.uid, p.facebook_token, p.display_name, p.photo_url))
 returning user.map(p => p)
 ) +=(token, email, provider, uid, facebook_token, display_name, photo_url)
 }

• Antot: Would you please: 1) better formulate the question; 2) detail which is the return type of personRepository.create(args) ?
• infaustus: Thanky you @Antot for the response. I edited my question, and paste personRepository.scala snippet
• Jean: I am curious : in pesronRepository.scala what are the full types of: db in db.run{} and user in (user.map(p=>... because as I would say that the snippet doesn't compile as normal scala. If it does compile, it is using a DSL which I don't recognize.

Answer 1

I know that PersonRepository#create returns a Future[A] as that is the only signature that can make the map call at the end of register actually compile. I'll assume it returns a Future[UserInser], and therefore has a signature equivalent to :

class PersonRepository {
 def create(email: String, provider: String, uid: String, facebook_token: String, display_name: String, photo_url: String):Future[UserInsert] = ??? 
}

That I have to make this assumption at all shows a first issue in your code : the name i used in

personRepository.create(user.email, user.provider, user.uid, user.facebook_token, user.display_name, user.photo_url).map(i => {
 Ok(Json.toJson(i))
 })

expresses no intent and forces the reviewer to go and check the signature of create to know what's going on.

Assuming I am right that it returns the inserted user, it would be nice to improve the naming as such:

personRepository.create(user.email, user.provider, user.uid, user.facebook_token, user.display_name, user.photo_url).map(savedUser => {
 Ok(Json.toJson(savedUser))
 })

Apart from that, your code definitely doesn't do what you want as it will try to insert an empty user in the database if the request is incorrect.

This could be avoided along with the whole pattern matching by delegating to play using a form such as :

val userRegisterForm = Form(
 mapping(
 "email"->text,
 "provider"->text,
 "uid"->text,
 "facebook_token"->text,
 "display_name"->text,
 "photo_url"->text
 )(UserInsert.apply)(UserInsert.unapply)
 )

Your controller then becomes :

 /**
 * Login/register action.
 */
 def register = Action.async { implicit request =>
 val maybeUser: Form[UserInsert] = userRegisterForm.bindFromRequest()
 maybeUser.fold(
 errors => Future.successful(BadRequest("Bad Request: " + play.api.http.Status.BAD_REQUEST)),
 user => personRepository.create(user.email, user.provider, user.uid, user.facebook_token, user.display_name, user.photo_url).map(savedUser => {
 Ok(Json.toJson(savedUser))
 })
 )
 }

I would also argue that

errors => Future.successful(BadRequest("Bad Request: " + play.api.http.Status.BAD_REQUEST))

doesn't make sense. It is just repeating bad request all over the place without providing any useful information. Just replace it with

errors => Future.successful(BadRequest)

or provide a meaningful error message

errors => Future.successful(BadRequest("Unable to parse request" + Json.prettyPrint(errors.errorsAsJson)))

which gives you :

 /**
 * Login/register action.
 */
 def register = Action.async { implicit request =>
 val maybeUser: Form[UserInsert] = userRegisterForm.bindFromRequest()
 maybeUser.fold(
 errors => Future.successful(BadRequest),
 user => personRepository.create(user.email, user.provider, user.uid, user.facebook_token, user.display_name, user.photo_url).map(savedUser => {
 Ok(Json.toJson(savedUser))
 })
 )
 }

Regarding the remainder of your code :

def userModel(json: JsValue) = {
 new UserInsert((json \ "email").as[String], (json \ "provider").as[String], (json \ "uid").as[String], (json \ "facebook_token").as[String], (json \ "display_name").as[String], (json \ "photo_url").as[String])
 }

should probably be made private.

Last but not least, the scala style guide says All public methods should have explicit type annotations. This should definitely be applied to PersonRepository#create

It can probably be improved further but that's a start.