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

Support HTTP.jl 1.0 #560

Merged
merged 9 commits into from
Jun 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,7 @@ jobs:
include:
# Add a job using the earliest version of Julia supported by this package
- os: ubuntu-latest
version: 1.3
arch: x64
# Add a 1.5 job because that's what Invenia actually uses
- os: ubuntu-latest
version: "1.5"
version: 1.6
arch: x64
env:
MINIO_ACCESS_KEY: minio
Expand Down
4 changes: 2 additions & 2 deletions Project.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ XMLDict = "228000da-037f-5747-90a9-8195ccbf91a5"
[compat]
Compat = "3.29"
GitHub = "5"
HTTP = "0.9.17"
HTTP = "1"
IniFile = "0.5"
JSON = "0.18, 0.19, 0.20, 0.21"
MbedTLS = "0.6, 0.7, 1"
Expand All @@ -33,7 +33,7 @@ OrderedCollections = "1.3"
StableRNGs = "1"
URIs = "1"
XMLDict = "0.3, 0.4"
julia = "1.3"
julia = "1.6"

[extras]
Pkg = "44cfe95a-1eb2-52ea-b672-e2afdf69b78f"
Expand Down
3 changes: 1 addition & 2 deletions bors.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
status = [
"Julia 1.3 - ubuntu-latest - x64",
"Julia 1.5 - ubuntu-latest - x64",
"Julia 1.6 - ubuntu-latest - x64",
"Julia 1 - macOS-latest - x64",
"Julia 1 - ubuntu-latest - x64",
]
Expand Down
3 changes: 1 addition & 2 deletions src/AWSExceptions.jl
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
module AWSExceptions

using HTTP
using HTTP.MessageRequest: body_was_streamed
using JSON
using XMLDict
using XMLDict: XMLDictElement
Expand Down Expand Up @@ -114,7 +113,7 @@ function AWSException(e::HTTP.StatusError, body::AbstractString)
message = get(info, "Message", message)
message = get(info, "message", message)

streamed_body = e.response.body == body_was_streamed ? body : nothing
streamed_body = !HTTP.isbytes(e.response.body) ? body : nothing

return AWSException(code, message, info, e, streamed_body)
end
Expand Down
6 changes: 5 additions & 1 deletion src/deprecated.jl
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ function legacy_response(

# When a user defined I/O stream is passed in use the actual `HTTP.Response` body
# instead of the `AWS.Response` body which requires the I/O stream to be seekable.
body = request.response_stream !== nothing ? response.response.body : response.body
body = if request.response_stream !== nothing
b"[Message Body was streamed]"
mattBrzezinski marked this conversation as resolved.
Show resolved Hide resolved
else
response.body
end

# The stored service name is always lowercase and may not match the module name
# specified by the user. We'll assume that the typical casing used is titlecase.
Expand Down
2 changes: 1 addition & 1 deletion src/utilities/credentials.jl
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ function _ec2_metadata(metadata_endpoint::String)

return request === nothing ? nothing : String(request.body)
catch e
e isa HTTP.IOError || e isa HTTP.StatusError && e.status == 404 || rethrow(e)
e isa HTTP.RequestError || e isa HTTP.StatusError && e.status == 404 || rethrow(e)
end

return nothing
Expand Down
6 changes: 1 addition & 5 deletions src/utilities/downloads_backend.jl
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
using HTTP.MessageRequest: body_was_streamed

"""
DownloadsBackend <: AWS.AbstractBackend

Expand Down Expand Up @@ -125,9 +123,7 @@ function _http_request(backend::DownloadsBackend, request::Request, response_str
end

function _http_response(req::Request, res::Downloads.Response; throw::Bool=true)
response = HTTP.Response(
res.status, res.headers; body=body_was_streamed, request=nothing
)
response = HTTP.Response(res.status, res.headers; body=IOBuffer(), request=nothing)

if throw && HTTP.iserror(response)
target = HTTP.resource(HTTP.URI(req.url))
Expand Down
24 changes: 9 additions & 15 deletions src/utilities/request.jl
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ struct HTTPBackend <: AbstractBackend
http_options::AbstractDict{Symbol,<:Any}
end

function statuserror(status, resp)
return HTTP.StatusError(status, resp.request.method, resp.request.target, resp)
end

function HTTPBackend(; kwargs...)
return if isempty(kwargs)
HTTPBackend(LittleDict{Symbol,Any}())
Expand Down Expand Up @@ -129,7 +133,7 @@ function submit_request(aws::AbstractAWSConfig, request::Request; return_headers
if HTTP.header(response, "Location") != ""
request.url = HTTP.header(response, "Location")
else
e = HTTP.StatusError(response.status, response)
e = statuserror(response.status, response)
Copy link
Member

@ericphanson ericphanson Jul 19, 2022

Choose a reason for hiding this comment

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

I think this broke some things for the Downloads backend, because it assumes the response is coming from HTTP.jl and has method and target fields, whereas before it just wrapped the response, which could be coming from Downloads.jl and may not have those fields. I guess the AWS tests aren't enough to check this case, but I saw when adding Downloads.jl tests to AWSS3: JuliaCloud/AWSS3.jl#262. See https://github.com/JuliaCloud/AWSS3.jl/runs/7409333758?check_suite_focus=true#step:6:388.

Copy link
Member

Choose a reason for hiding this comment

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

Can the DownloadsBackend-created HTTP.Response that's returned just include the request here?

throw(AWSException(e, stream))
end
end
Expand Down Expand Up @@ -200,11 +204,6 @@ end
function _http_request(http_backend::HTTPBackend, request::Request, response_stream::IO)
http_options = merge(http_backend.http_options, request.http_options)

# HTTP options such as `status_exception` need to be used when creating the stack
http_stack = HTTP.stack(;
redirect=false, retry=false, aws_authorization=false, http_options...
quinnj marked this conversation as resolved.
Show resolved Hide resolved
)

local buffer
local response

Expand All @@ -219,12 +218,12 @@ function _http_request(http_backend::HTTPBackend, request::Request, response_str
buffer = Base.BufferStream()

response = @mock HTTP.request(
http_stack,
request.request_method,
HTTP.URI(request.url),
HTTP.mkheaders(request.headers),
request.content;
require_ssl_verification=false,
quinnj marked this conversation as resolved.
Show resolved Hide resolved
redirect=false,
retry=false,
response_stream=buffer,
http_options...,
)
Expand All @@ -235,13 +234,8 @@ function _http_request(http_backend::HTTPBackend, request::Request, response_str
end

check = function (s, e)
# `Base.IOError` is needed because HTTP.jl can often have errors that aren't
# caught and wrapped in an `HTTP.IOError`
# https://github.com/JuliaWeb/HTTP.jl/issues/382
return isa(e, Sockets.DNSError) ||
isa(e, HTTP.ParseError) ||
isa(e, HTTP.IOError) ||
quinnj marked this conversation as resolved.
Show resolved Hide resolved
isa(e, Base.IOError) ||
return isa(e, HTTP.ConnectError) ||
isa(e, HTTP.RequestError) ||
(isa(e, HTTP.StatusError) && _http_status(e) >= 500)
end

Expand Down
12 changes: 6 additions & 6 deletions test/AWSExceptions.jl
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"status_code" => 400,
)

expected["body"] = HTTP.MessageRequest.body_was_streamed
expected["body"] = IOBuffer()
expected["streamed_body"] = """
<?xml version="1.0" encoding="UTF-8"?>
<$err>
Expand All @@ -40,7 +40,7 @@
resp = HTTP.Response(
expected["status_code"], expected["headers"]; body=expected["body"], request=req
)
status_error = HTTP.StatusError(expected["status_code"], resp)
status_error = AWS.statuserror(expected["status_code"], resp)
ex = AWSException(status_error, expected["streamed_body"])

_test_exception(ex, expected, msg)
Expand All @@ -50,7 +50,7 @@

@testset "XMLRequest - Invalid XML" begin
expected = Dict(
"body" => HTTP.MessageRequest.body_was_streamed,
"body" => IOBuffer(),
"streamed_body" => """<?xml version="1.0" encoding="UTF-8"?>InvalidXML""",
"headers" => ["Content-Type" => "application/xml"],
"status_code" => 404,
Expand All @@ -59,7 +59,7 @@
resp = HTTP.Response(
expected["status_code"], expected["headers"]; body=expected["body"], request=req
)
status_error = HTTP.StatusError(expected["status_code"], resp)
status_error = AWS.statuserror(expected["status_code"], resp)
ex = @test_logs (:error,) AWSException(status_error, expected["streamed_body"])

@test ex.code == "404"
Expand All @@ -74,7 +74,7 @@
"status_code" => 400,
)

expected["body"] = HTTP.MessageRequest.body_was_streamed
expected["body"] = IOBuffer()
expected["streamed_body"] = """
{
"__type": "$(expected["code"])",
Expand All @@ -87,7 +87,7 @@
resp = HTTP.Response(
expected["status_code"], expected["headers"]; body=expected["body"], request=req
)
status_error = HTTP.StatusError(expected["status_code"], resp)
status_error = AWS.statuserror(expected["status_code"], resp)
ex = AWSException(status_error, expected["streamed_body"])

_test_exception(ex, expected, "$msg")
Expand Down
4 changes: 2 additions & 2 deletions test/issues.jl
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ try
attempt_num += 1
if attempt_num <= num_attempts_to_fail
write(response_stream, data[1:(n - 1)]) # an incomplete stream that shouldn't be retained
throw(HTTP.IOError(EOFError(), "msg"))
throw(HTTP.RequestError(HTTP.Request(), EOFError()))
else
write(response_stream, data)
return HTTP.Response(200, "{\"Location\": \"us-east-1\"}")
Expand Down Expand Up @@ -212,7 +212,7 @@ try

@testset "Fail all 4 attempts then throw" begin
err_t = if AWS.DEFAULT_BACKEND[] isa AWS.HTTPBackend
HTTP.IOError
HTTP.RequestError
else
Downloads.RequestError
end
Expand Down
7 changes: 4 additions & 3 deletions test/patch.jl
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,8 @@ end
_http_options_patches = [
@patch function HTTP.request(args...; kwargs...)
options = Dict(kwargs)
delete!(options, :require_ssl_verification)
delete!(options, :redirect)
delete!(options, :retry)
delete!(options, :response_stream)
return options
end
Expand Down Expand Up @@ -171,11 +172,11 @@ function gen_http_options_400_patches(message)
if response_stream !== nothing
write(response_stream, body)
close(response_stream) # Simulating current HTTP.jl 0.9.14 behaviour
body = HTTP.MessageRequest.body_was_streamed
body = IOBuffer()
end

response = HTTP.Response(400, headers; body=body, request=request)
exception = HTTP.StatusError(400, response)
exception = AWS.statuserror(400, response)
return !status_exception ? response : throw(exception)
end
@patch function Downloads.request(args...; output=nothing, kwargs...)
Expand Down