Add a content type type member to response encoders

[travisbrown]

This is a quick proof of concept to follow up on the conversation in #328 this afternoon. The issue is that it's currently easy to accidentally use an EncodeResponse[String] instance that's intended for encoding strings as JSON when you just want to encode a string as plain text.

This change adds a type member to EncodeResponse that tracks the content type as a type-level string (this allows us to avoid enumerating content types as singleton objects or whatever).

The behavior is still the same when you don't specify the content type:

scala> import io.finch.response._
import io.finch.response.EncodeResponse

scala> implicitly[EncodeResponse[String]].apply("foo")
res0: com.twitter.io.Buf = ByteBuffer(3)

scala> import io.finch.argonaut._
import io.finch.argonaut._

scala> implicitly[EncodeResponse[String]].apply("foo")
res1: com.twitter.io.Buf = ByteBuffer(5)

But when you do specify the content type you'll get the instance you want:

scala> implicitly[EncodeTextResponse[String]].apply("foo")
res2: com.twitter.io.Buf = ByteBuffer(3)

scala> implicitly[EncodeJsonResponse[String]].apply("foo")
res3: com.twitter.io.Buf = ByteBuffer(5)

Here EncodeTextResponse and EncodeJsonResponse are just type aliases:

type EncodeTextResponse[-A] = EncodeResponse.Aux[A, Witness.`"text/plain"`.T]
type EncodeJsonResponse[-A] = EncodeResponse.Aux[A, Witness.`"application/json"`.T]

(The Witness syntax is handy but kind of unpleasant looking.)

Note that the usage of EncodeResponse.apply is changed up a bit in order to help Scala's type inference be a little more useful. Otherwise there are no changes to tests or examples.

[codecov-io]

Current coverage is 94.56%

Merging #329 into master will change coverage by +1.84% by d855b3f

Powered by Codecov

[vkostyukov]

This looks cool!

I have just one question. Should we change ResponseBuilder API to support new EncodeResponse types?

[travisbrown]

@vkostyukov Yeah, that makes sense, and should be pretty straightforward. I'm out for a bit but I'll update the PR when I get home.

[vkostyukov]

I'm not sure I understand Witness correctly. Does it mean that we've enabled compile-time checking for the errors like EncodeResponse("aplication/json")... (single p)?

[travisbrown]

Not really—the advantage is that it allows the compiler to select an implicit instance on the basis of the content string. I can ask for an instance for encoding strings as text/plain and I'll get that one instead of the application/json instance that may be in scope.

You could accomplish almost exactly the same thing by enumerating content types like this:

class ContentType(val contentType: String)
case object ApplicationJson extends ContentType("application/json")
case object PlainText extends ContentType("text/plain")

And then putting a ContentType type member in EncodeResponse. Using a type-level string is just more convenient, less boilerplate-y, allows users to add their own content types, etc.

[vkostyukov]

Oh, I see. That makes sense. Thanks for the explanation @travisbrown!

[travisbrown]

I started thinking a little more last night about how we want to make this new functionality available to users, and I'm not sure RequestReader is the best place to do it (or at least I'm not sure that it should be the only place).

Instead as a short-term solution we could provide a toServiceAs("text/plain") method that would be similar to toService but would only work if all of the values in the coproduct router have EncodeResponse instances for "text/plain". Then users could create endpoints for specific content types and "or" them together with :+: (as in #324).

In the longer term I think there might be a nice way to handle content negotiation along these lines, but that would be a bigger API change. So that's probably something to consider before 1.0, but for now it would be nice to have something for 0.8 that handles e.g. the TechEmpower benchmark use case cleanly.

Any thoughts, @vkostyukov, @penland365, @rpless?

[penland365]

I like this - the automatic preference to encode a plain string as JSON bit me last week ( easy fix though ). I do especially like the idea of a toServiceAs("text/plain") but that's also something that seems easy to those of that have been using Finch for awhile. Agreed that long term there needs to be more work around content negotiation, but that's a really hard topic to get right.

[vkostyukov]

I had something in mind for a while (as part of Finch 1.0) that should give us an answer on how to do that. The fact that this issue's showed up gives me a hope that the version of Finch 1.0 I was imagining fits the real world pretty good so far.

It was a pretty busy week, so I didn't manage to put a roadmap doc together. I will try to finish it this weekend.

[rpless]

I'm fine with toServiceAs as a short term solution. I'd prefer something closer to the current API, but I can't see a way to do that right now.

[vkostyukov]

The ideas in this PR were merged in as part of #541. Thanks @travisbrown for the inspiration!