ResponseBuilder & Futures [Improvement]

[milesburton]

ResponseBuilder should resolve Future[Object]

Expected behavior

Should resolve the content of the Future

Actual behavior

Returns serialized Future (the future object itself)

Steps to reproduce the behavior

    val someData = TwitterFuture[String] { "test"}
    response.ok(someData)

    // returns 
    {
      "result" : {
        "r" : "test",
        "throw" : false,
        "return" : true
      },
      "defined" : true
    }

[milesburton]

The work around appears to be to map the Future to a Response. Something akin to:

myFuture.map(t =>
          response.ok(t)
}

[fayimora]

@milesburton What I usually do is "return" the future. So:

get("/future") { req: Request =>
  val dataFut = Future.value(Map("hey" -> "world"))
  dataFut
}

but admittedly, it starts to get tricky when you want something other than HTTP OK

Here, what I do is map the future to Future[Response].

get("/future") { req: Request =>
  val dataFut = Future.value(Map("hey" -> "world"))
  dataFut.map(m => response.ok(m))
}

[milesburton]

Yeah that's fine for basic responses. As you mentioned, as you start building more advanced responses (content-dispositions, headers etc) it's quite convoluted.

I suspect it could be fairly easily resolved by some sort of inversion. Potentially have the builder return a Future itself, and resolve first to last. ie:

val response: Future[Response] = response.ok(myFuture)

and under the hood myFuture.map(builderActions)

[fayimora]

Do you have a specific use case where the above-mentioned technique doesn't work for you?

Btw, looking at the code, there's no special treatment for body of type Future[T]

[milesburton]

@fayimora

Yeah I had a dig through the code - the signature would need to change to support Future[T].

Mapping is fine. I think it may be wise to update the documentation to mention that the builder doesn't implicitly support Future's.

[milesburton]

Minor suggestion for additional documentation mentioned here: https://github.com/milesburton/finatra/pull/1

[mosesn]

@milesburton I think we intentionally want to decouple these two concerns–one of them is "How do I work with futures" and the other is, "How do I construct a response?" Constructing a response is synchronous, so when we want to attach it to asynchronous work, we pollute the asynchronous work by using Future#map.

I don't think we want to change the documentation in the controller section, since I think it might confuse people into thinking that futures and building a response have a special relationship, when this is normal operation of futures. If the futures documentation doesn't make this clear, then we should improve that part, not the controller docs.

What do you think?

[milesburton]

@mosesn I entirely understand your reasoning.

My worry relates to the implicit handling of Future[T] within a controller, but no analogous approach for customised responses.

Whilst the builder is clearly an explicit action - concerned with constructing a response - I feel it would be wise to provide some sort of hint when dealing with Futures.

Whether this is an addendum to the documentation, or the addition of support code wouldn't matter too much.

Regardless. I thought I'd raise my observation. No doubt Google has indexed this issue so it may very well be implicitly documented, and consequently moot.

[mosesn]

👍 good point. I'll close this issue for now, and folks can reopen if they run into again. Thanks for the feedback!

[cacoco]

@milesburton sorry I'm just catching up on this thread. I was in/out near the end of last year and this was closed before I had a chance to respond. I hope to shed a little more light here.

FWIW, you can see that this is the expected pattern as we even do this internally in the CallbackConverter.

The point is either you do it we do it, that is a conversion of a Future[T] to a Future[Response]. The benefit of you doing it in your Controller is that you have more fine-grained control over the created Response. But for ease of use we allow you to simply return a Future[T] and will do the conversion for you.

Now the question is around the ResponseBuilder. As @mosesn mentioned, constructing a response is synchronous (independent of any Future constructs), thus the ResponseBuilder has no concept of Futures, though I can see how it could be confusing if you merely attempt to do response.ok(someFuture) as that seems plausible but in actuality is non-sensical.

We expect some level of understanding around Futures when using the Finatra framework thus it is perhaps not documented clearly enough that you cannot put a Future into the body of a response and expect a correct response (if you trace the code it's effectively just a toString on what is passed) and we can work to make that clearer. Thanks!