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

HTTP.request sidesteps a bit too much ? #463

Closed
fredrikekre opened this issue Nov 5, 2019 · 8 comments
Closed

HTTP.request sidesteps a bit too much ? #463

fredrikekre opened this issue Nov 5, 2019 · 8 comments

Comments

@fredrikekre
Copy link
Member

This new method:

request(a...; kw...)::Response = return request(HTTP.stack(;kw...), a...; kw...)
sidesteps a bit too much. I assume it is meant to call the method just above:

HTTP.jl/src/HTTP.jl

Lines 306 to 314 in 7bcb6f5

function request(stack::Type{<:Layer}, method, url, h=Header[], b=nobody;
headers=h, body=b, query=nothing, kw...)::Response
uri = URI(url)
if query !== nothing
uri = merge(uri, query=query)
end
return request(stack, string(method), uri, mkheaders(headers), body; kw...)
end
in order to correctly deal with headers kwargs etc. But that does not happen, because we dispatch to
function request(::Type{RedirectLayer{Next}},
which completely ignores headers (it is included in kwargs...).

Here is an example, where headers is ignored:

julia> using HTTP

julia> uri = HTTP.URI("https://api.github.com");

julia> headers=Dict("User-Agent" => "HTTP.jl");

julia> HTTP.request("GET", uri; headers=headers)
ERROR: HTTP.ExceptionRequest.StatusError(403, "GET", "/", HTTP.Messages.Response:
"""
HTTP/1.0 403 Forbidden
Cache-Control: no-cache
Connection: close
Content-Type: text/html

Request forbidden by administrative rules. Please make sure your request has a User-Agent header (http://developer.github.com/v3/#user-agent-required). Check https://developer.github.com for other possible causes.
""")
fredrikekre added a commit to fredrikekre/HTTP.jl that referenced this issue Nov 5, 2019
fredrikekre added a commit to fredrikekre/HTTP.jl that referenced this issue Nov 5, 2019
fredrikekre added a commit to fredrikekre/HTTP.jl that referenced this issue Nov 5, 2019
fredrikekre added a commit to fredrikekre/HTTP.jl that referenced this issue Nov 5, 2019
@oxinabox oxinabox assigned oxinabox and unassigned oxinabox Nov 5, 2019
@oxinabox
Copy link
Member

oxinabox commented Nov 5, 2019

I cannot assign @mattBrzezinski to this issue because he isn't a collaborator.
But he says he is going to work on this now.

@mattBrzezinski
Copy link
Member

First attempt:

#464

@mattBrzezinski
Copy link
Member

I spent a bit of time looking into a solution for this, and using your test case from above this does not seem to be an issue with v0.8.6 and the introducing for a secondary entry point.

(v1.1) pkg> add HTTP@0.8.1
julia> import Pkg; Pkg.installed()
Dict{String,Union{Nothing, VersionNumber}} with 22 entries:
  ...
  "HTTP"           => v"0.8.1"
  ...
julia> using HTTP
julia> uri = HTTP.URI("https://api.github.com");
julia>  headers=Dict("User-Agent" => "HTTP.jl");
julia> HTTP.request("GET", uri; headers=headers)
ERROR: HTTP.ExceptionRequest.StatusError(403, "GET", "/", HTTP.Messages.Response:
"""
HTTP/1.0 403 Forbidden
Cache-Control: no-cache
Connection: close
Content-Type: text/html

Request forbidden by administrative rules. Please make sure your request has a User-Agent header (http://developer.github.com/v3/#user-agent-required). Check https://developer.github.com for other possible causes.
""")

It looks like this has been around for a while now, I'll spend some more time looking into this later this week.

@iamed2
Copy link
Contributor

iamed2 commented Nov 5, 2019

The change was made here to add a stack argument to HTTP.request. However, this basically ensures that method will never be called, since a stack is just a Layer, so it will dispatch to the appropriate layer.

fredrikekre added a commit to fredrikekre/HTTP.jl that referenced this issue Nov 6, 2019
@quinnj
Copy link
Member

quinnj commented Dec 7, 2019

What's the status here? @fredrikekre, is your PR ready to review? (it's currently marked as a draft). @mattBrzezinski, did you ever discover anything when you said you'd look into this?

@iamed2
Copy link
Contributor

iamed2 commented Dec 7, 2019

#470 resolved this particular issue, and it looks like #469 was noted as a breaking change

@fredrikekre
Copy link
Member Author

Yes, I think (something like) #469 would be good for the next breaking release, it is just too difficult to separata public API from internal API when everything is methods of the same function.

@quinnj
Copy link
Member

quinnj commented May 25, 2022

Resolved in #789

@quinnj quinnj closed this as completed May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants