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.

Sign up for GitHub

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

Jump to bottom

Add the location to all responses returned #96

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add the location to all responses returned #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions lib/httpoison.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
defmodule HTTPoison.Response do
defstruct status_code: nil, body: nil, headers: []
@type t :: %__MODULE__{status_code: integer, body: binary, headers: list}
defstruct status_code: nil, body: nil, headers: [], location: nil
@type t :: %__MODULE__{status_code: integer, body: binary, headers: list, location: binary | nil}

@doc """
Returns the location that the response ended up at. This method should be favoured over
returning the location from the struct directly, to protect against future API changes.
"""
@spec location(Response.t) :: binary | nil
def location(response), do: response.location
end

defmodule HTTPoison.AsyncResponse do
Expand Down Expand Up @@ -65,4 +72,3 @@ defmodule HTTPoison do

use HTTPoison.Base
end

14 changes: 8 additions & 6 deletions lib/httpoison/base.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,24 +394,26 @@ defmodule HTTPoison.Base do

case :hackney.request(method, request_url, request_headers,
request_body, hn_options) do
{:ok, status_code, headers, _client} when status_code in [204, 304] ->
response(process_status_code, process_headers, process_response_body, status_code, headers, "")
{:ok, status_code, headers} -> response(process_status_code, process_headers, process_response_body, status_code, headers, "")
{:ok, status_code, headers, client} when status_code in [204, 304] ->
response(process_status_code, process_headers, process_response_body, status_code, headers, "", :hackney.location(client))
{:ok, status_code, headers} -> response(process_status_code, process_headers, process_response_body, status_code, headers, "", nil)
{:ok, status_code, headers, client} ->
location = :hackney.location(client) # FIXME The current version of Hackney (0.8) requires that we read the location before the body.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benoitc, do you mean this?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah probably :) i need to think on a better way to keep that state..

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does that mean I should not rework this PR to make it work against the current httpoison codebase and wait?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edgurgel @benoitc so what do I do here? Should I rebase or wait?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we just keep moving as this has been open for a while. We can always rework it if hackney changes in the future. Thanks for pushing this :D

case :hackney.body(client) do
{:ok, body} -> response(process_status_code, process_headers, process_response_body, status_code, headers, body)
{:ok, body} -> response(process_status_code, process_headers, process_response_body, status_code, headers, body, location)
{:error, reason} -> {:error, %Error{reason: reason} }
end
{:ok, id} -> { :ok, %HTTPoison.AsyncResponse{ id: id } }
{:error, reason} -> {:error, %Error{reason: reason}}
end
end

defp response(process_status_code, process_headers, process_response_body, status_code, headers, body) do
defp response(process_status_code, process_headers, process_response_body, status_code, headers, body, location) do
{:ok, %Response {
status_code: process_status_code.(status_code),
headers: process_headers.(headers),
body: process_response_body.(body)
body: process_response_body.(body),
location: location
} }
end
end
36 changes: 27 additions & 9 deletions test/httpoison_base_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,13 @@ defmodule HTTPoisonBaseTest do
expect(:hackney, :request, [{[:post, "http://localhost", {:req_headers, []}, {:req_body, "body"}, []],
{:ok, 200, "headers", :client}}])
expect(:hackney, :body, 1, {:ok, "response"})
expect(:hackney, :location, 1, "location")

assert Example.post!("localhost", "body") ==
%HTTPoison.Response{ status_code: {:code, 200},
headers: {:headers, "headers"},
body: {:resp_body, "response"} }
body: {:resp_body, "response"},
location: "location" }

assert validate :hackney
end
Expand All @@ -45,11 +47,13 @@ defmodule HTTPoisonBaseTest do
expect(:hackney, :request, [{[:post, "http://localhost", {:req_headers, []}, {:req_body, "body"}, []],
{:ok, 200, "headers", :client}}])
expect(:hackney, :body, 1, {:ok, "response"})
expect(:hackney, :location, 1, "location")

assert ExampleDefp.post!("localhost", "body") ==
%HTTPoison.Response{ status_code: {:code, 200},
headers: {:headers, "headers"},
body: {:resp_body, "response"} }
body: {:resp_body, "response"},
location: "location" }

assert validate :hackney
end
Expand All @@ -71,11 +75,13 @@ defmodule HTTPoisonBaseTest do
expect(:hackney, :request, [{[:post, "http://localhost", [], "body", [connect_timeout: 12345]],
{:ok, 200, "headers", :client}}])
expect(:hackney, :body, 1, {:ok, "response"})
expect(:hackney, :location, 1, "location")

assert HTTPoison.post!("localhost", "body", [], timeout: 12345) ==
%HTTPoison.Response{ status_code: 200,
headers: "headers",
body: "response" }
body: "response",
location: "location" }

assert validate :hackney
end
Expand All @@ -84,11 +90,13 @@ defmodule HTTPoisonBaseTest do
expect(:hackney, :request, [{[:post, "http://localhost", [], "body", [recv_timeout: 12345]],
{:ok, 200, "headers", :client}}])
expect(:hackney, :body, 1, {:ok, "response"})
expect(:hackney, :location, 1, "location")

assert HTTPoison.post!("localhost", "body", [], recv_timeout: 12345) ==
%HTTPoison.Response{ status_code: 200,
headers: "headers",
body: "response" }
body: "response",
location: "location" }

assert validate :hackney
end
Expand All @@ -97,11 +105,13 @@ defmodule HTTPoisonBaseTest do
expect(:hackney, :request, [{[:post, "http://localhost", [], "body", [proxy: "proxy"]],
{:ok, 200, "headers", :client}}])
expect(:hackney, :body, 1, {:ok, "response"})
expect(:hackney, :location, 1, "location")

assert HTTPoison.post!("localhost", "body", [], proxy: "proxy") ==
%HTTPoison.Response{ status_code: 200,
headers: "headers",
body: "response" }
body: "response",
location: "location" }

assert validate :hackney
end
Expand All @@ -110,11 +120,13 @@ defmodule HTTPoisonBaseTest do
expect(:hackney, :request, [{[:post, "http://localhost", [], "body", [proxy_auth: {"username", "password"}, proxy: "proxy"]],
{:ok, 200, "headers", :client}}])
expect(:hackney, :body, 1, {:ok, "response"})
expect(:hackney, :location, 1, "location")

assert HTTPoison.post!("localhost", "body", [], [proxy: "proxy", proxy_auth: {"username", "password"}]) ==
%HTTPoison.Response{ status_code: 200,
headers: "headers",
body: "response" }
body: "response",
location: "location" }

assert validate :hackney
end
Expand All @@ -123,11 +135,13 @@ defmodule HTTPoisonBaseTest do
expect(:hackney, :request, [{[:post, "http://localhost", [], "body", [ssl_options: [certfile: "certs/client.crt"]]],
{:ok, 200, "headers", :client}}])
expect(:hackney, :body, 1, {:ok, "response"})
expect(:hackney, :location, 1, "location")

assert HTTPoison.post!("localhost", "body", [], ssl: [certfile: "certs/client.crt"]) ==
%HTTPoison.Response{ status_code: 200,
headers: "headers",
body: "response" }
body: "response",
location: "location" }

assert validate :hackney
end
Expand All @@ -136,11 +150,13 @@ defmodule HTTPoisonBaseTest do
expect(:hackney, :request, [{[:post, "http://localhost", [], "body", [follow_redirect: true]],
{:ok, 200, "headers", :client}}])
expect(:hackney, :body, 1, {:ok, "response"})
expect(:hackney, :location, 1, "location")

assert HTTPoison.post!("localhost", "body", [], follow_redirect: true) ==
%HTTPoison.Response{ status_code: 200,
headers: "headers",
body: "response" }
body: "response",
location: "location" }

assert validate :hackney
end
Expand All @@ -149,11 +165,13 @@ defmodule HTTPoisonBaseTest do
expect(:hackney, :request, [{[:post, "http://localhost", [], "body", [max_redirect: 2]],
{:ok, 200, "headers", :client}}])
expect(:hackney, :body, 1, {:ok, "response"})
expect(:hackney, :location, 1, "location")

assert HTTPoison.post!("localhost", "body", [], max_redirect: 2) ==
%HTTPoison.Response{ status_code: 200,
headers: "headers",
body: "response" }
body: "response",
location: "location" }

assert validate :hackney
end
Expand Down
14 changes: 10 additions & 4 deletions test/httpoison_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ defmodule HTTPoisonTest do
test "get" do
assert_response HTTPoison.get("localhost:8080/deny"), fn(response) ->
assert :erlang.size(response.body) == 197
assert HTTPoison.Response.location(response) == "http://localhost:8080/deny"
end
end

Expand Down Expand Up @@ -65,11 +66,15 @@ defmodule HTTPoisonTest do
end

test "option follow redirect absolute url" do
assert_response HTTPoison.get("http://localhost:8080/redirect-to?url=http%3A%2F%2Flocalhost:8080%2Fget", [], [follow_redirect: true])
assert_response HTTPoison.get("http://localhost:8080/redirect-to?url=http%3A%2F%2Flocalhost:8080%2Fget", [], [follow_redirect: true]), fn(response) ->
assert HTTPoison.Response.location(response) == "http://localhost:8080/get"
end
end

test "option follow redirect relative url" do
assert_response HTTPoison.get("http://localhost:8080/relative-redirect/1", [], [follow_redirect: true])
assert_response HTTPoison.get("http://localhost:8080/relative-redirect/1", [], [follow_redirect: true]), fn(response) ->
assert HTTPoison.Response.location(response) == "/get"
end
end

test "basic_auth hackney option" do
Expand Down Expand Up @@ -102,7 +107,7 @@ defmodule HTTPoisonTest do
test "cached request" do
if_modified = %{"If-Modified-Since" => "Tue, 11 Dec 2012 10:10:24 GMT"}
response = HTTPoison.get!("localhost:8080/cache", if_modified)
assert %HTTPoison.Response{status_code: 304, body: ""} = response
assert %HTTPoison.Response{status_code: 304, body: "", location: "http://localhost:8080/cache"} = response
end

test "send cookies" do
Expand Down Expand Up @@ -140,7 +145,8 @@ defmodule HTTPoisonTest do
assert response.status_code == 200
assert is_binary(response.body)

unless function == nil, do: function.(response)
unless HTTPoison.Response.location(response) == nil, do: assert is_binary(HTTPoison.Response.location(response))
unless function == nil, do: function.(response)
end

defp get_header(headers, key) do
Expand Down