Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add redirect loop that keeps track of intermediary requests and responses #79

Closed
wants to merge 3 commits into from

Conversation

@DanielG
Copy link

DanielG commented Oct 8, 2014

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

This comment has been minimized.

Copy link
Author

DanielG commented Oct 9, 2014

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

@snoyberg

This comment has been minimized.

Copy link
Owner

snoyberg commented Oct 13, 2014

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

This comment has been minimized.

Copy link
Author

DanielG commented Oct 13, 2014

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

This comment has been minimized.

Copy link
Author

DanielG commented Oct 13, 2014

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

This comment has been minimized.

Copy link
Owner

snoyberg commented Oct 14, 2014

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

This comment has been minimized.

Copy link
Author

DanielG commented Oct 14, 2014

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

This comment has been minimized.

Copy link
Owner

snoyberg commented Oct 14, 2014

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

This comment has been minimized.

Copy link
Author

DanielG commented Oct 15, 2014

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

snoyberg added a commit that referenced this pull request Oct 16, 2014
@snoyberg

This comment has been minimized.

Copy link
Owner

snoyberg commented Oct 16, 2014

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

@DanielG

This comment has been minimized.

Copy link
Author

DanielG commented Oct 16, 2014

Works perfectly fine, thanks :)

@DanielG DanielG closed this Oct 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.