Doesn't request with Accept

[pbrisbin]

I notice the client doesn't add any request headers (besides User-Agent based on Configuration), but shouldn't it include an Accept header based on what it knows the Operation's content-type is?

[joel-bach]

Hey @pbrisbin I think this would be an option if the response only contains one response media type key. If there are multiples this would need to be a separate input I guess (would be interesting but I do not think I will have the time to implement this but feel free to open a PR). Out of curiosity: Is this creating a problem on your end or is it more of a theoretical question?

[pbrisbin]

I think this would be an option if the response only contains one response media type key

Yeah that's fair. FWIW, in my case the API is all JSON, so a hook in Configuration to add arbitrary headers to all requests would meet my needs too; and could be generally useful IMO, such as to include an X-Client-Version or Trace-Id or whatever other stuff might be needed. Currently, I'm doing it in the definition of httpBS itself, which is maybe good enough now that I think about it.

Is this creating a problem on your end or is it more of a theoretical question?

A little of both.

It's not creating a problem in production usage, although it could if the API actually did use Accept to choose a response, which we do in other services here (just not the one being used by this client, yet) such as an admin index page that gives back HTML to browsers, JSON to clients, and CSV when admins ask for a download.

In my case, I ran into it because I implemented an HTTP stubbing system for tests that matches requests with stubbed responses based on attributes, and when I use the json helper there it sets things up to only match requests with a JSON Accept header -- so until I was able to make these requests do so, none of my stubs matched.

[joel-bach]

Thank you for the additional context!

I like the idea of having the ability to add more headers if necessary, things like Trace-Id do make a lot of sense. When you say you did it in the definition of httpBS itself, do you mean you have a custom implementation of the MonadHTTP class? Or some way else?

I am thinking of allowing to add request headers in the Configuration which would make it quite reusable as part of the monad. What do you think?

[pbrisbin]

When you say you did it in the definition of httpBS itself, do you mean you have a custom implementation of the MonadHTTP class?

Correct.

We have our own MonadHttp class so that we can implement the stubbing I mentioned, and add tracing to all HTTP throughout our app:

instance MonadHttp (HandlerFor App) where
  httpLbs req = inSpan spanName spanArguments $ do
    app <- getYesod
    runReaderT (appHttpLbs req) app

appHttpLbs
  :: (MonadIO m, MonadReader App m) => Request -> m (Response BSL.ByteString)
appHttpLbs req = do
  mStubs <- asks appHttpStubs
  case mStubs of
    Nothing -> liftIO $ httpLbs req
    Just stubs -> pure $ httpStubbed stubs req

(I took inspiration when I first saw your MonadHTTP class, so thanks for that!)

We define any of the client-generated MonadHTTP instances in terms of it.

instance SomeService.MonadHTTP (HandlerFor App) where
  httpBS = fmap (fmap BSL.toStrict) . httpLbs

It's a convenient place to do other stuff like setting a global response timeout to:

instance GUR.MonadHTTP (HandlerFor App) where
  httpBS =
    fmap (fmap BSL.toStrict)
      . httpLbs
      . setResponseTimeout (responseTimeoutMicro $ 2 * 1000000)
   where
    setResponseTimeout x req = req {responseTimeout = x}

So in this JSON API's client, I made it always add accept header:

instance CurriculaApi.MonadHTTP (HandlerFor App) where
  -- https://github.com/Haskell-OpenAPI-Code-Generator/Haskell-OpenAPI-Client-Code-Generator/issues/98
  httpBS =
    fmap (fmap BSL.toStrict)
      . httpCached (memcachedHttpCacheSettings fiveMinuteTTL) httpLbs
      . addRequestHeader hAccept "application/json"

I am thinking of allowing to add request headers in the Configuration which would make it quite reusable as part of the monad. What do you think?

I'm not against the Configuration idea, I think it makes sense to generalize the current user-agent handling in that way. But I'm now starting to think it's not as valuable, since doing it via MonadHTTP doesn't seem to have any downsides. In fact, something like Trace-Id would probably need to be done in MonadHTTP anyway, since you want to pull and use a parent-trace-id from your own request context if you have one, which can't be done when setting up a static Configuration value at start-up.

[joel-bach]

I'm not against the Configuration idea, I think it makes sense to generalize the current user-agent handling in that way. But I'm now starting to think it's not as valuable, since doing it via MonadHTTP doesn't seem to have any downsides. In fact, something like Trace-Id would probably need to be done in MonadHTTP anyway, since you want to pull and use a parent-trace-id from your own request context if you have one, which can't be done when setting up a static Configuration value at start-up.

Yeah, that is a fair point. Also, with the configuration approach it would still be limited to headers (or rather we have to add all configurable parts we want to support manually).

Thank you for the detailed write-up, I think this can help others as well and is something we can point to when questions about how to do this arise 🙏

Do we want to close this issue then for the moment? @NorfairKing what is your opinion? Do you see a better way to allow full customization of the request?

[pbrisbin]

I'll go ahead and close the issue

To recap:

My original reason for opening it was that I thought the library could know what content-types a given operation was for and automatically add the correct Accept header. Now that I see that would take pretty substantial redesign (separate functions, more arguments, etc), I think it's reasonable if you want to call that a WONTFIX for now.

The idea of configuration that we discussed might be useful, but wasn't the main motivation for this Issue anyway, and can be pretty well solved by a custom MonadHTTP at present anyway. If we want to explore that further, I think it could be its own Issue.

Thanks for the discussion!