Add redirect loop that keeps track of intermediary requests and responses

[DanielG]

Hey,

I haven't tested this properly yet, just submitting it to get some feedback.

I added a function (httpRedirect') that is basically the same as the old httpRedirect but returns a list of (Request, Response) pairs which have been performed instead of discarding the path taken through the redirect jungle.

I need this to get the URL of the last redirect, since the old function only returned the final response there was no easy way to get at the URL we ended up at.

To give the user maximum flexibility I had to change the type of the http' function passed to httpRedirect'. The new type is now: Request -> IO (Either (Response b, Request) (Response c)) this is so the user can strictly retrieve part or whole of the intermediary requests while still allowing them to stream the final response or do whatever else the types allow.

Also httpRedirect is now implemented in terms of httpRedirect' and (hopefully) maintains full backwards compatibility.

I didn't actually export httpRedirect' I leave that to the maintainer if this patch is accepted :)

[DanielG]

Instead of adding httpRedirect' I replaced the original httpRedirect and generalized responseOpen into responseOpen' in this patch.

[snoyberg]

Overall this looks good to me. AFAICT, it does not change behavior for the normal use case, which is a good thing. However, I preferred the initial implementation that didn't introduce an API breaking change. Is there a reason you moved away from that approach?

[DanielG]

Well I initially overlooked that responseOpen has a significant amount of code as well and if I only add httpRedirect' my main use case would be unnecessarily hard to implement, i.e. I have to reimplement what responseOpen' does now in my project.

This only breaks the Internal part of the API anyways (which people shouldn't use ;), not the stuff that's exported through Network.HTTP.Client.

[DanielG]

How about this? Turned out to be easier than I thought, just had to pull defaultMapRes to the top level. (I can't come up with a better name for that function, any suggestions?)

[snoyberg]

I was looking at the existing documentation, and while it's a bit out-of-date, it seems like the function you're trying to get is implementable with the current provided API:

responseOpenHistory :: Request -> Manager -> IO ([(Request, Response L.ByteString)], (Request, Response BodyReader))
responseOpenHistory req0 man = do
    reqRef <- newIORef req0
    historyRef <- newIORef id
    let go req = do
            res <- httpRaw req man
            case getRedirectedRequest req (responseHeaders res) (responseCookieJar res) (statusCode $ responseStatus res) of
                Nothing -> return (res, Nothing)
                Just req' -> do
                    writeIORef reqRef req'
                    body <- brReadSome (responseBody res) 1024
                    modifyIORef historyRef (. ((req, res { responseBody = body }):))
                    return (res, Just req')
    res <- httpRedirect (redirectCount req0) go req0
    reqFinal <- readIORef reqRef
    history <- readIORef historyRef
    return (history [], (reqFinal, res))

withResponseHistory :: Request -> Manager -> (([(Request, Response L.ByteString)], (Request, Response BodyReader)) -> IO a) -> IO a
withResponseHistory req man = bracket (responseOpenHistory req man) (responseClose . snd . snd)

Or available as an interactive project: https://www.fpcomplete.com/user/chad/snippets/random-code-snippets/http-redirects. I'm not opposed to adding this function to http-client, but I would rather stick with the previous internals if possible. Does this implementation work for you?

[DanielG]

I suppose that would work, I'm just not a big fan of using IORefs when they're not absolutely needed. Other than that my version does allow users of the internal API to do arbitrary things with the body of the intermediary responses whereas with your version there is no way to say get the whole response body. That's probably not a big deal though.

In the latest version of the patch I did revert back to preserving the internal API, not sure if you looked at it?

[snoyberg]

I did review your code, but given that all the functionality can be achieved with the current API, I'm pretty hesitant to change the internal functions. I'm also fairly certain that any arbitrary action could be done with the body of the intermediate responses, my responseOpenHistory simply chooses one such behavior (reading in 1024 bytes).

[DanielG]

Ok, adding responseOpenHistory to http-client's API will serve my use case well enough then :)

[snoyberg]

I've added the API in a new commit, can you test it out in your use case before I release to Hackage?

[DanielG]

Works perfectly fine, thanks :)