Bug in decodedAs?

[edsko]

In Servant.Client.Core.Internal.RunClient, we find

decodedAs :: forall ct a m. (MimeUnrender ct a, RunClient m)
  => Response -> Proxy ct -> m a
decodedAs response contentType = do
  responseContentType <- checkContentTypeHeader response
  unless (any (matches responseContentType) accept) $
    throwServantError $ UnsupportedContentType responseContentType response
  case mimeUnrender contentType $ responseBody response of
    Left err -> throwServantError $ DecodeFailure (T.pack err) response
    Right val -> return val
  where
    accept = toList $ contentTypes contentType

I think the line

  unless (any (matches responseContentType) accept) $

is wrong. The documentation for matches reads

This relation must be a total order, where more specific terms on the left can produce a match, but a less specific term on the left can never produce a match. For instance, when matching against media types it is important that if the client asks for a general type then we can choose a more specific offering from the server, but if a client asks for a specific type and the server only offers a more general form, then we cannot generalise. In this case, the server types will be the left argument, and the client types the right.

Here however the client type is given on the left instead.

The result is that for a client that accepts a HTML response (accept will be [text/html;charset=utf-8]) but a server that returns a content-type of text/html, the response is rejected as UnsupportedContentType.

[phadej]

the server types are the left argument, aren'т тhey?

[edsko]

Hello again @phadej :D Uh, well, the roles are reversed here I think. responseContentType is the specific content type returned by the server; matches is the list of content types that we can accept. As the docs say: "a less specific term on the left can never produce a match". Here the server returns something unspecific text/html, and we specify something quite specific (text/html;charset=utf-8), and hence it doesn't match.

[phadej]

hmm. For JSON we do have also non specific application/json, don't we have the same for HTML?

Maybe we don't, but OTOH, i'd rather require you to write own HTML, which has charset-less media-type. Non setting the charset for html endpoints is asking for troubles.

header('Content-Type: text/html; charset=utf-8');

is like the first thing people do in their PHP scripts, so browsers don't need to guess...

[edsko]

I don't control the endpoint, I am writing a client, not a server :)

And no, we don't:

*QuerySFZD.API.Ours Network.HTTP.Media Data.Foldable> toList $ contentTypes (Proxy @HTML)
[text/html;charset=utf-8]

[phadej]

I don't think that anything bad would happen if we add plain `text/html` to the lists (as the second one). Check how JSON case is done. I'd like a PR to both -blaze and -lucid packages, Thanks!

…

Sent from my iPhone

On 15 Dec 2018, at 20.17, Edsko de Vries ***@***.***> wrote: I don't control the endpoint, I am writing a client, not a server :) And no, we don't: *QuerySFZD.API.Ours Network.HTTP.Media Data.Foldable> toList $ contentTypes (Proxy @html) [text/html;charset=utf-8] — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[edsko]

Done. haskell-servant/servant-blaze#11 and haskell-servant/servant-lucid#10 .

[phadej]

Thanks, these will probably fail with old base, but i'll just drop the support for pre GHC-8.0. ie no need to fix those

[phadej]

new versions of servant-lucid and servant-blaze released