Accept multiple content-types in servant-client.

[cdepillabout]

When defining a route like the following,

type API = Get '[JSON, PlainText] Int

servant-client is only able to receive responses with the content type
that comes first in the list. In this example, it will only be able to
receive application/json responses.

This PR changes it so that servant-client will accept any content-type
in the list.

In the example above, it will accept responses with a content type of
application/json or text/plain.

[phadej]

If only we could say pick some (i.e. at least one) of MimeUnrender for the cts.

This makes APIs unusable where is e.g. HTML content type. We have report end-points, which give you JSON, CSV or pretty-rendered HTML. AFAICS, with this change I couldn't consume them with servant-client anymore.

[cdepillabout]

@phadej: I'm not quite sure I understand what your routes look like.

Is it something like this?

data Foo = Foo

type API = Get '[JSON, CSV, HTML] Foo

In this example, API represents a single route, which will respond to a GET request, returning a Foo (that was encoded with a content-type of JSON, CSV, or HTML).

As long as you have MimeUnrender JSON Foo, MimeUnrender CSV Foo, and MimeUnrender HTML Foo instances, with this PR, servant-client should be able to generate a method that returns a Foo, regardless as to whether the content-type is "application/json", "application/csv", or "text/html".

...hmm, maybe you're saying that you don't have a MimeUnrender HTML Foo instance, so you wouldn't be able to use your route with servant-client anymore?

Let me give my reason for wanting this change. I'm running into the problem described in #292. I need to deal with an API that's randomly sending back responses in one of two content types. The way servant-client currently works, it is failing on one of the two content types, and throwing a DecodeFailure error. This PR fixes this.

[phadej]

...hmm, maybe you're saying that you don't have a MimeUnrender HTML Foo instance, so you wouldn't be able to use your route with servant-client anymore?

Yes

I need to deal with an API that's randomly sending back responses in one of two content types.

It doesn't respect Accept: -header? On random? Duh. That's nasty.

---

I could use a type family to filter out HTML content-type from my API before giving it for servant-client. Maybe I'll have to do that then.

[cdepillabout]

In commit 2b737e4, I updated this PR to get rid of the conflict with master.

[alpmestan]

@phadej Note that such a filtering thing could be useful in general and maybe even added to servant. I have done this in the past, you need it now as well, so it may be useful to other people out there.

@cdepillabout Ideally I'd like to have the client pick the content type and only make the user "pay" (i.e have MimeUnrender instances) for the content types used that way. However, I know about all the nasty apps out there that don't necessarily respect the Accept header, which you're a victim of here, and we can't just pretend those don't exist. I'm in favor of merging this because of this precise reason even though it doesn't feel like a clean solution.

[phadej]

@alpmestan the problem with type families, is that one have to do tricks to make higher order ones. E.g. servant could have a function to transform leaves of api (i.e. Verbs), but type family cannot be parametrised by type-family directly. E.g.

• one want to filter HTML out of content
• other may want also sort JSON to be in front.
• deal differently with nodes which will end with empty content list
• filter out POST
• ...

Now we have to make tailor made type family for each case. Luckily modifier :> sublayout pattern is quite extensible, that's the great part!

[jkarni]

So I don't think we should change the default behavior (because of the problem @phadej mentioned of this requiring more instances than necessary). But we could have a variant of client, clientWithCTs, that takes a proxy of the content-types list. client would just be = clientWithCTs <first-ct>.

What does everyone think?

[alpmestan]

we can definitely give this a shot and see how useful it is to people.

[soenkehahn]

I also wouldn't want to change the default behavior of client in this regard. I think it makes a lot of sense that client uses the Accept header to demand a content-type, and using the first one in the list seems like a good default.

@cdepillabout: Having a function clientWithCTs like @jkarni suggested, surely would be a great feature. Would you be interested in working on that? (And having test-cases for additional features would be good.)

[cdepillabout]

@soenkehahn: Okay, I'll give it a shot!

[cdepillabout]

Okay, I whipped something together. This is building on my original PR.

The client function currently looks like this:

client :: HasClient api => Proxy api -> Client api
client p = clientWithRoute p defReq

class HasClient api where
   type Client api :: *
   clientWithRoute :: Proxy api -> Req -> Client api

I changed client to look like this:

type family FirstCT cts :: [*] where
    FirstCT '[] = '[]
    FirstCT (ct ': cts) = '[ct]

type family FirstCTVerb x :: * where
    FirstCTVerb (Verb method statusCode cts r) = (Verb method statusCode (FirstCT cts) r)
    FirstCTVerb (a :> b) = (a :> FirstCTVerb b)
    FirstCTVerb (a :<|> b) = (FirstCTVerb a :<|> FirstCTVerb b)
    FirstCTVerb a = a

client
    :: forall api . HasClient (FirstCTVerb api)
    => Proxy api -> Client (FirstCTVerb api)
client _ = clientWithRoute apiWithOnlyFirstCT defReq
  where
    apiWithOnlyFirstCT :: Proxy (FirstCTVerb api)
    apiWithOnlyFirstCT = Proxy

The FirstCTVerb type family rewrites all Verbs to only contain the very first content-type in their content-type list.

I also added a clientWithCTs function:

type family ReplaceCTs api (newCTs :: [*]) :: * where
    ReplaceCTs (Verb method statusCode oldCTs r) newCTs = (Verb method statusCode newCTs r)
    ReplaceCTs (a :> b) newCTs = (a :> ReplaceCTs b newCTs)
    ReplaceCTs (a :<|> b) newCTs = (ReplaceCTs a newCTs :<|> ReplaceCTs b newCTs)
    ReplaceCTs a newCTs = a

clientWithCTs
    :: forall api cts . (HasClient (ReplaceCTs api cts))
    => Proxy cts -> Proxy api -> Client (ReplaceCTs api cts)
clientWithCTs _ _ = clientWithRoute replacedCTs defReq
  where
    replacedCTs :: Proxy (ReplaceCTs api cts)
    replacedCTs = Proxy

The downside of this new client function is that the type is a little more complicated. It may be confusing for new users.

What does everyone think of this?

[soenkehahn]

I think it'd be nice if we didn't have to change client at all. And you should really fix the test-suite. It'd also give some insight into how much impact your change has.

[cdepillabout]

@soenkehahn: Thanks for the feedback! I was trying to flesh out the API and get everyone's opinions, so I didn't mess with the tests yet. I didn't want to rewrite all the tests if my API suggestions were going to get shot down (which I expected to happen ;-))

I agree that it would be better if client didn't have to change. In order for client to not have to change, what would have to be done?

The previous suggestion from you (@soenkehahn) and @jkarni was to have a new function clientWithCTs which takes a proxy of the content-types. I was imagining something like this:

clientWithCTs
    :: forall api cts . (HasClient api)
    => Proxy cts -> Proxy api -> Client _FOO_
clientWithCTs _ _ = clientWithRoute (Proxy :: Proxy _FOO_) defReq

The problem with this is that I couldn't figure out what type to make _FOO_. I solved it by using a type family ReplaceCTs that walked through the api type and replaced all the Verb's content-types with cts.

I ended up doing something similar with client, which gives it a more complicated type.

Can you think of a better way to write client that doesn't change its type?

I am basing the above changes on my original pull request, which forces all the content-types to actually be receivable from the server.

Maybe you're saying that this change in the receivable content-types shouldn't happen either? The problem is that without this change, I couldn't figure out a way to write clientWithCTs. Even if I pass additional content-types, they'll just get ignored by the performRequestCT function.

[soenkehahn]

@cdepillabout: Sorry for the delay.

To me it looks like your ReplaceCT actually replaces the existing content-types regardless of whether the new ones are a subset of the old ones. That actually makes this unsafe, because you can just replace all the content-types of all api endpoints with arbitrary content-types. Is my understanding correct?

What about something like this: https://gist.github.com/b94c48821d6463e11c170cc6cfbd2a8e? This would allow you to specify your wanted content-type, while still being completely safe. The current client as implemented in master would then just work (it'll always pick the first content-type). And this is even completely independent of the current server-client code. Could even be published in a separate package.

[cdepillabout]

@soenkehahn Thanks for the reply! I'm currently at ICFP this week, so you'll have to give a week or so to look over your code!

[cdepillabout]

@soenkehahn Okay, I finally got a chance to sit down a look at your code.

That actually makes this unsafe, because you can just replace all the content-types of all api endpoints with arbitrary content-types. Is my understanding correct?

This is correct.

What about something like this: https://gist.github.com/b94c48821d6463e11c170cc6cfbd2a8e? This would allow you to specify your wanted content-type, while still being completely safe. The current client as implemented in master would then just work (it'll always pick the first content-type).

So, I'm facing the same problem as #292. I'm dealing with a server that forces requests to have a content type of application/json, but randomly sends back responses with a content type of either text/plain or application/json. I need servant-client to be able to accept responses with one of two content types.

My PR allows the user to specify which content types they want to be able to receive. However, it complicates the types in the servant-client package.

I can understand if this is a rather specific problem, and you don't want to complicate servant to be able to deal with it.

[soenkehahn]

Yeah, I think we definitely don't want to complicate the use-case of talking to standard-compliant servers.

On the other hand we should try to follow the Robustness Principle. So ideally servant-client would allow to accept every content-type that is declared in the api. I don't want client to behave that way, because we don't want to force users to implement MimeUnrender instances for every declared content-type. (As @phadej mentioned above, this is not even easily possible with things like HTML.) But we could have a function clientLenient:

• that requires MimeUnrender instances for every declared content-type,
• sends the first content-type as the Accept header,
• accepts not only that first content-type but all specified ones, picking the MimeUnrender instance by what the server sets in the Content-Type response header and
• that does not add too much complexity to servant-client. As @cdepillabout points out, this is a rather specific use-case for talking to misbehaving servers, so we should weigh the added complexity against the benefits.

@cdepillabout: Would this solve your problem? Are you still interested in working on this? What's the problem in implementing this without changing the type of client?

[cdepillabout]

@soenkehahn Thanks for the reply.

I think I'll be able to use #614 and #615 to solve my problem. It seems like a better way than to this PR (#552). I'll close this PR.