Using if-guard in for-comprehension to check for existence

Question

My am making 3 database queries, each return a Future. I am trying to use for comprehension to resolve the Futures but it seems I am not using if correctly in for

Each query depends on result of previous one. I look for a token, if found, I look for user and it found, I update the user. Each database query returns a Future[Option]] and I thought I could considitionally perform the next query depending on whether the previous one returns Some or None. I am using isDefined for this. But when I ran the code for an invalid token, I got error [NoSuchElementException: None.get] for code userOption:Option[User]<-userRepo.findUser(tokenOption.get.loginInfo); if tokenOption.isDefined

def verifyUser(token:String) = Action.async {
  implicit request => {
    val result:Future[Result] = for{
      //generator 1 - get token from database
      tokenOption:Option[UserToken] <- userTokenRepo.find(UserTokenKey(UUID.fromString(token)))
      //generator2. found token, look for corresponding user to which the token belongs
      userOption:Option[User] <- userRepo.findUser(tokenOption.get.loginInfo); if tokenOption.isDefined
      //generator 3. found user and token. Update profile 
      modifiedUser:Option[User] <-  confirmSignupforUser(userOption.get); if userOption.isDefined
       } yield 
         { //check if we have user and token and modified user here. If any is missing, return error else success
           if(tokenOption.isDefined && userOption.isDefined && modifiedUser.isDefined)
              Redirect("http://localhost:9000/home"+";signup=success")//TODOM - pick from config
           else
             if(tokenOption.isEmpty)
             Redirect("http://localhost:9000/home"+";signup=error")//TODOM - pick from config
           else if(userOption.isEmpty)
             Redirect("http://localhost:9000/home"+";signup=error")//TODOM - pick from config
           else if(modifiedUser.isEmpty)
             Redirect("http://localhost:9000/home"+";signup=error")//TODOM - pick from config
           else //this shouldn't happen. Unexpected
             Redirect("http://localhost:9000/home"+";signup=error")//TODOM - pick from config
         }
       result
     }
   }

This is going to require a lot of rewriting. I think you should spend a little time getting used to using Option types. You shouldn't need to do any of these .isEmpty or .isDefined calls. You will want to just pass the option around in such a way that at then end of your computation, you'll either have a Some(result) or a None. Then, at that point, handle the None case. A large if/else block in your yield is also unusual. Instead, return the value there, and then as a subsequent step use Pattern Matching to return the correct Redirect — Metropolis, Sep 18 '18 at 11:38
But changing function signatures is not in my control. If know I could write the above code using ‘map’ and ‘flatMap’ but that code becomes too nested and unreadable. I should be able to write the logic using ‘for’ as well — Manu Chadha, Sep 18 '18 at 11:43
I'm not suggesting changing the signature. I'm suggesting you get rid of all the "if x.isDefined" and "if x.isEmpty" entirely. If one of your Option's is None, it will simply pass that None along. Then at the end, handle the None case. Don't check for None at every step. — Metropolis, Sep 18 '18 at 11:47
sorry but I am struggling to understand how I can remove a check if I need it. `userRepo.findUser(tokenOption.get.loginInfo)` doesn't accept an `Option` so I can't pass `None` to it and I have to get the `loginInfo` from `tokenOption` and pass it. But I don't know whether the token is valid or not so I need to check it before passing it to `findUser`. — Manu Chadha, Sep 18 '18 at 11:57
I understand what you are saying but am unable to translate it to code. Eg, I see now that the `if` isn't working because because it translates to `userRepo.findUser(tokenOption.get.loginInfo).withFilter` and not `if (tokenOption.isDefined) {userRepo.findUser(tokenOption.get.loginInfo)}` — Manu Chadha, Sep 18 '18 at 11:59
Please see the way I have done it now (my answer below). I know you weren't happy with he `if/else` in `yield` but this is the best I could some up with without cluttering the code. Does it look OK? — Manu Chadha, Sep 18 '18 at 14:25

Yaneeve · Answer 1 · 2018-09-18T12:07:07.393

TL;DR

Consider using OptionT https://typelevel.org/cats/datatypes/optiont.html

Have a look at my toned down implementation:

from https://scastie.scala-lang.org/hsXXtRAFRrGpMO1Jl1Li7A

import scala.concurrent.Future
import scala.concurrent.ExecutionContext.Implicits.global
import scala.concurrent.Await.result
import scala.concurrent.duration._
import scala.language.postfixOps

type UserToken = String
type User = String

def fromToken(token: String): Future[Option[UserToken]] = Future.successful(None)
def findUser(userToken: UserToken): Future[Option[User]] = Future.successful(None)
def modify(user: User): Future[Option[User]] = Future.successful(None)

def verifyUser(token: String) = {
  val result = for {
    tokenOption: Option[UserToken] <- fromToken(token) //generator 1 - get token from database
    userOption: Option[User] <- findUser(tokenOption.get);
    if tokenOption.isDefined //generator2. found token, look for corresponding user to which the token belongs
    modifiedUser: Option[User] <- modify(userOption.get);
    if userOption.isDefined //generator 3. found user and token. Update profile
  } yield { //check if we have user and token and modified user here. If any is missing, return error else success
    if (tokenOption.isDefined && userOption.isDefined && modifiedUser.isDefined)
      println("happy")
    else
      println("sad")
  }
  result
}

result(verifyUser("hello"), 1 second)

I used the following compile time flags, the last one is important:

scalacOptions ++= Seq(
  "-deprecation",
  "-encoding", "UTF-8",
  "-feature",
  "-unchecked",
  "-Xprint:typer"
)

Let's focus on this line of the compile output:

(((tokenOption: Option[Playground.this.UserToken]) => Playground.this.findUser(tokenOption.get).
withFilter(((check$ifrefutable$2: Option[Playground.this.User]) => (check$ifrefutable$2: Option[Playground.this.User] @unchecked) match {
      case (userOption @ (_: Option[Playground.this.User])) => true
      case _ => false
...

You can see that the tokenOption.get is invoked before the withFilter. These gets are the source of the exception you get

The, almost complete output of the compile is:

[[syntax trees at end of                     typer]] // main.scala
....
    import scala.concurrent.Future;
    import scala.concurrent.ExecutionContext.Implicits.global;
    import scala.concurrent.Await.result;
    import scala.concurrent.duration._;
    import scala.language.postfixOps;
    type UserToken = String;
    type User = String;
    def fromToken(token: String): scala.concurrent.Future[Option[Playground.this.UserToken]] = scala.concurrent.Future.successful[None.type](scala.None);
    def findUser(userToken: Playground.this.UserToken): scala.concurrent.Future[Option[Playground.this.User]] = scala.concurrent.Future.successful[None.type](scala.None);
    def modify(user: Playground.this.User): scala.concurrent.Future[Option[Playground.this.User]] = scala.concurrent.Future.successful[None.type](scala.None);
    def verifyUser(token: String): scala.concurrent.Future[Unit] = {
      val result: scala.concurrent.Future[Unit] = Playground.this.fromToken(token).withFilter(((check$ifrefutable$1: Option[Playground.this.UserToken]) => (check$ifrefutable$1: Option[Playground.this.UserToken] @unchecked) match {
  case (tokenOption @ (_: Option[Playground.this.UserToken])) => true
  case _ => false
}))(scala.concurrent.ExecutionContext.Implicits.global).flatMap[Unit](((tokenOption: Option[Playground.this.UserToken]) => Playground.this.findUser(tokenOption.get).withFilter(((check$ifrefutable$2: Option[Playground.this.User]) => (check$ifrefutable$2: Option[Playground.this.User] @unchecked) match {
  case (userOption @ (_: Option[Playground.this.User])) => true
  case _ => false
}))(scala.concurrent.ExecutionContext.Implicits.global).withFilter(((userOption: Option[Playground.this.User]) => tokenOption.isDefined))(scala.concurrent.ExecutionContext.Implicits.global).flatMap[Unit](((userOption: Option[Playground.this.User]) => Playground.this.modify(userOption.get).withFilter(((check$ifrefutable$3: Option[Playground.this.User]) => (check$ifrefutable$3: Option[Playground.this.User] @unchecked) match {
  case (modifiedUser @ (_: Option[Playground.this.User])) => true
  case _ => false
}))(scala.concurrent.ExecutionContext.Implicits.global).withFilter(((modifiedUser: Option[Playground.this.User]) => userOption.isDefined))(scala.concurrent.ExecutionContext.Implicits.global).map[Unit](((modifiedUser: Option[Playground.this.User]) => if (tokenOption.isDefined.&&(userOption.isDefined).&&(modifiedUser.isDefined))
        scala.Predef.println("happy")
      else
        scala.Predef.println("sad")))(scala.concurrent.ExecutionContext.Implicits.global)))(scala.concurrent.ExecutionContext.Implicits.global)))(scala.concurrent.ExecutionContext.Implicits.global);
      result
    };
    scala.Predef.locally[Unit]({
      val $t: Unit = scala.concurrent.Await.result[Unit](Playground.this.verifyUser("hello"), scala.concurrent.duration.`package`.DurationInt(1).second);
      Playground.this.instrumentationMap$.update(com.olegych.scastie.api.Position.apply(1199, 1236), com.olegych.scastie.api.runtime.Runtime.render[Unit]($t)((ClassTag.Unit: scala.reflect.ClassTag[Unit])));
      $t
    })
  };
  object Main extends scala.AnyRef {
    def <init>(): Main.type = {
      Main.super.<init>();
      ()
    };
    private[this] val playground: Playground = new Playground();
    <stable> <accessor> def playground: Playground = Main.this.playground;
    def main(args: Array[String]): Unit = scala.Predef.println(com.olegych.scastie.api.runtime.Runtime.write(Main.this.playground.instrumentations$))
  }
}

Thanks but considering that I am new to Scala, I'll prefer to first learn it property. `cats` might be too much for me at the moment. — Manu Chadha, Sep 18 '18 at 14:26
I have been new to scala for over 3 years. All I can recommend is keep on challenging yourself - there is always something new to learn - the scala ocean is vast, don't let that deter you. — Yaneeve, Sep 18 '18 at 14:38

Dima · Answer 2 · 2018-09-18T13:15:08.383

I am not sure why you are surprised you are getting and error for None.get with invalid token: if token is invalid, tokenOption is None, so, the next statement tokenOption.get will fail with exactly this error.

You want the "guard" executed before the statement you want to short circuit, not after it:

for {
   foo <- bar if foo.isDefined
   baz <- foo.get
 } yield baz

But this would fail in the end anyway, because there would be nothing to yield (this trick works with Options or Lists etc., but Future.withFilter will end up failing if predicate is not satisfied, there is no other alternative).

The general rule to avoid this kind of errors is never use .get on an Option (or on a Try). Also, never use .head on a List, .apply on a Map, etc.

Here is one (almost) idiomatic way to write what you want:

case object Error extends RuntimeException("")
userTokenRepo
  .find(UserTokenKey(UUID.fromString(token)))
  .map { _.getOrElse(throw Error)
  .flatMap { userRepo.find(_.loginInfo) }
  .map { _.getOrElse(throw Error) }
  .flatMap(confirmSignupForUser)
  .map { _.getOrElse(throw Error) }
  .map { _ => "success") }
  .recover { case Error => "error" }
  .map { result => Redirect(s"http://localhost:9000/home;signup=$result" }

Note, I said this was "almost" idiomatic, because throwing exceptions in scala is frowned upon. A purist would object to it, and suggest using something like a Try . or a biased Either instead, or to make use of a third party library, like cats or scalaz, that provide additional tools for working with Futures of Option (namely, OptionT).

But I would not recommend getting into that right now. You should get comfortable enough with basic "vanilla" scala before starting with that advanced stuff to avoid ending up with something completely incomprehensible.

You could also write this differently, in a completely idiomatic way (without using exceptions), with something like this:

  userTokenRepo.find(UserTokenKey(UUID.fromString(token)))
    .flatMap { 
        case Some(token) => userRepo.find(token.loginInfo)
        case None => Future.successful(None) 
     }.flatMap { 
        case Some(user) => confirmSignupForUser(user)
        case None => Future.successful(None)
     }.map {
        case Some(_) => "success"
        case None => "error"
     }.map { result => 
        Redirect(s"http://localhost:9000/home;signup=$result"     
     }

This is more "pure", but a little more repetitive, so my personal preference is the first variant.

Finally, you could do away with my Error thingy, and just handle the NoSuchElement exception directly. This is going to be the shortest, but kinda icky even to my taste (what if some downstream code throws this exception because of a bug?):

 userTokenRepo
   .find(UserTokenKey(UUID.fromString(token)))
   .flatMap { userRepo.find(_.get.loginInfo) }
   .flatMap(confirmSignupForUser(_.get))
   .map { _.get }
   .map { _ => "success") }
   .recover { case _: NoSuchElementException => "error" }
   .map { result => 
     Redirect(s"http://localhost:9000/home;signup=$result" 
   }

I really don't recommend the last version though, despite it being the shortest, and arguably, the most readable one (you can even rewrite it with a for-comprehension to look even nicer). Using Option.get is commonly considered "code smell", and is almost never a good thing to do.

Thanks. I am able to write the code using `map` and `flatMap` but I wanted to give `for` a try as I thought it will make the code more readable. Please see the way I have done it now (my answer below). Does it look OK? — Manu Chadha, Sep 18 '18 at 14:28
@ManuChadha well, as I mentioned earlier, calling `.isDefined` and especially `.get` on `Option` is "code smell", so ... not really. This just isn't a good use case of a for-comprehension (except if you combine it with monad transformers). — Dima, Sep 18 '18 at 16:42
Do you think that I should use ‘for’ if I need to resolve multiple futures which don’t depend on one another? If they do then map/flatmap are better option? — Manu Chadha, Sep 18 '18 at 16:45
If they don't depend on one another, you can just `traverse` or `sequence` them. That way they'd be run in parallel. `for-comprehension` is the really same thing as a chain of flatMaps, it just sometimes one form reads than the other. In particular, for comprehension is helpful, when some of the futures in the chain depend on _several_ of preceding results. It may get cumbersome to express that with `.flatMap`. If you can write it so that each next future only needs the previous result, my personal preference is `.flatMap`. — Dima, Sep 18 '18 at 16:51
Don't throw Error! Error is reserved for unrecoverable errors, like OOM. Throw Exception. — Brian McCutchon, Sep 18 '18 at 19:12
@BrianMcCutchon Nah, you are thinking of `java.lang.Error`. This is my own `Error`, different thing. It is _actually_ a `RuntimeException`. — Dima, Sep 18 '18 at 19:21
I am not redefining them (I am not sure it's even possible to redefine it). — Dima, Sep 18 '18 at 19:40
Call it shadowing, then, if you prefer. And don't do it. When people see Error in your code, they will think it's java.lang.Error, because that is imported by default. If someone else modifies your code and tries to reference java.lang.Error, it will refer to your type instead. — Brian McCutchon, Sep 18 '18 at 20:57
@BrianMcCutchon I can't control what people think. If people can't read the code, they better not modify it. What you call "shadowing" is exactly the intent of packages in the first place. That's how they are _supposed_ to be used. — Dima, Sep 18 '18 at 21:06
You should write your code so as not to break people's expectations. https://en.m.wikipedia.org/wiki/Principle_of_least_astonishment — Brian McCutchon, Sep 18 '18 at 21:29
Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/180301/discussion-between-brian-mccutchon-and-dima). — Brian McCutchon, Sep 18 '18 at 21:31

score 0 · Answer 3 · answered Sep 18 '18 at 14:24

Motivated by How to best handle Future.filter predicate is not satisfied type errors

I rewrote like the following. While the code works, I am curious to know if I am doing it the right way (functional!). Does it look fine?

   def verifyUser(token:String) = Action.async {
     implicit request => {
       println("verifyUser action called with token: " + token) //TODOM - add proper handling and response

       val result:Future[Result] = for{tokenOption:Option[UserToken] <- userTokenRepo.find(UserTokenKey(UUID.fromString(token)))  //generator 1 - get token from database
                                    userOption:Option[User] <- if (tokenOption.isDefined) userRepo.findUser(tokenOption.get.loginInfo) else Future.successful(None) //generator2. found token, look for corresponding user to which the token belongs
                                    modifiedUser:Option[User] <- if (userOption.isDefined) confirmSignupforUser(userOption.get) else Future.successful(None) //generator 3. found user and token. Update profile
                                    deletedToken:Option[UserTokenKey] <- if(modifiedUser.isDefined) userTokenRepo.remove(UserTokenKey(UUID.fromString(token))) else Future.successful(None)
       }
         yield { //check if we have user and token and modified user here. If any is missing, return error else success
           println("db query results tokenOption: "+tokenOption+", userOption: "+userOption+" : modifiedUserOption: "+modifiedUser+", deletedToken: "+deletedToken)
           if(tokenOption.isDefined && userOption.isDefined && modifiedUser.isDefined && deletedToken.isDefined)
              Redirect("http://localhost:9000/home"+";signup=success")//TODOM - pick from config
           else
             if(tokenOption.isEmpty)
             Redirect("http://localhost:9000/home"+";signup=error")//TODOM - pick from config
           else if(userOption.isEmpty)
             Redirect("http://localhost:9000/home"+";signup=error")//TODOM - pick from config
           else if(modifiedUser.isEmpty)
             Redirect("http://localhost:9000/home"+";signup=error")//TODOM - pick from config
           else //this shouldn't happen. Unexpected
             Redirect("http://localhost:9000/home"+";signup=error")//TODOM - pick from config
         }
       result
     }
   }

_right (functionl!) and idiomatic_ are contentious words. My personal preference for this kind of situation, as I had mentioned in my answer, are monad transformers. But, others would disagree — Yaneeve, Sep 18 '18 at 14:31
You can replace `if(foo.isDefined) stuff(foo.get) else Future.successful(None)` with `foo.fold(Future.successful(None))(stuff(_))`. Doing `.get` on an option smells really badly. Also in the `yield` clause, four error branches are exactly the same. If you can just do `Redirect(url + deletedToken.fold("success")(_ => "error"))` instead of the whole `if` thingy — Dima, Sep 18 '18 at 16:48

Using if-guard in for-comprehension to check for existence

3 Answers3