HTTPoison.AsyncResponse id's can be {:maybe_redirect_response, status, headers, client}

[vereis]

I ran into the following situation working with a script.google.com/... API:

POST-ing to a URL which returned a 302 meant that redirection was not followed because :hackney will only follow redirects if a POST request status was 304, but if follow_redirects: true is given as a request option, HTTPoison returns: {:ok, %HTTPoison.AsyncResponse{id: {:maybe_redirect, ..., ..., ...}}

Passing hackney: [force_redirects: true] to the request to try to circumvent this does not work as expected, because :hackney doesn't follow the spec and change the method to GET, causing the request to return a 405.

For my use case, handling the error is perfectly fine, but unfortunately dialyzer complains about the following match because the HTTPoison.AsyncResponse's id is not a reference but instead just a tuple probably bubbling up from :hackney:

    case HTTPoison.post(@url, params, [], follow_redirect: true) do
      {:ok, %HTTPoison.AsyncResponse{id: {:maybe_redirect, 302, headers, _}}} ->
        Logger.warning(...)
        :ok

      ... ->
    end

Not sure if this was known behaviour, but if so I've amended the type spec in this MR although perhaps one should return a different struct altogether?

[edgurgel]

@vereis Thanks for the very detailed explanation 🏆

I agree that the result is a bit weird given that the id is a tuple 🤔 I would consider this a bug tbh...

Maybe the best solution is to return a different struct as you suggested? What do you think?

[vereis]

Yeah that sounds better, I just didn't want to do too much out of nowhere, but yeah this seems more like a bug! Maybe I should have created an issue first 😅

I think we can just handle where this gets returned and check if it looks like a :maybe_redirect, perhaps return something like {:ok, %HTTPoison.MaybeRedirect{...}} or an error? I'm not sure what you'd prefer in this case to be honest:

If we go with something like {:ok, %HTTPoison.MaybeRedirect{}} I guess we can return the location in this struct, but it'd be up to the user to manually try GET-ing this.

Perhaps since manual intervention is required this is moreso an {:error, %HTTPoison.MaybeRedirect{}} instead? What do you think?

If we wanted to handle this explicitly we could perhaps also add a function HTTPoison.follow_redirect(%HTTPoison.MaybeRedirect{}) which would perform the GET for you, keeping the headers of the original request?

[edgurgel]

If we go with something like {:ok, %HTTPoison.MaybeRedirect{}} I guess we can return the location in this struct, but it'd be up to the user to manually try GET-ing this.

I like this. Most people won't need to worry as it's not super easy to get to this case?

If we wanted to handle this explicitly we could perhaps also add a function HTTPoison.follow_redirect(%HTTPoison.MaybeRedirect{}) which would perform the GET for you, keeping the headers of the original request?

Maybe we could wait for more feedback once we release the new struct?

What do you think?

[vereis]

I like this. Most people won't need to worry as it's not super easy to get to this case?

Yeah exactly, this makes sense to me. I'll give it a shot and update this MR. Thanks for clarifying 💪

[edgurgel]

Nice one! I intend to release a new version soon. Cheers! 🎉

[Nitrino]

@edgurgel Can you please release a new version with these changes?

[edgurgel]

Yes! Absolutely! I completely forgot about this! 🤦 I will do it later today!