Merge #503

503: Avoid hiding streamed content with internal errors r=mattBrzezinski a=omus I encountered this when working on AWSS3 where the error returned was: ``` XMLError: XML declaration allowed only at the start of the document from XML parser (code: 64, line: 2) ``` but the streaming content was not shown so I couldn't investigate further. With a POC version of this PR I managed to see the streaming content: ``` <?xml version="1.0" encoding="UTF-8"?> <Error><Code>NotImplemented</Code><Message>A header you provided implies functionality that is not implemented</Message><BucketName>ocaws.jl.test.20211029t193302z</BucketName><Resource>/ocaws.jl.test.20211029t193302z</Resource><RequestId>16B29752DB5BE300</RequestId><HostId>b3a8880e-5b1b-44ff-9155-7653d625b88e</HostId></Error><?xml version="1.0" encoding="UTF-8"?> <Error><Code>NotImplemented</Code><Message>A header you provided implies functionality that is not implemented</Message><BucketName>ocaws.jl.test.20211029t193302z</BucketName><Resource>/ocaws.jl.test.20211029t193302z</Resource><RequestId>16B29752DF0ACDB8</RequestId><HostId>b3a8880e-5b1b-44ff-9155-7653d625b88e</HostId></Error><?xml version="1.0" encoding="UTF-8"?> <Error><Code>NotImplemented</Code><Message>A header you provided implies functionality that is not implemented</Message><BucketName>ocaws.jl.test.20211029t193302z</BucketName><Resource>/ocaws.jl.test.20211029t193302z</Resource><RequestId>16B29752FF5DD3F8</RequestId><HostId>b3a8880e-5b1b-44ff-9155-7653d625b88e</HostId></Error><?xml version="1.0" encoding="UTF-8"?> <Error><Code>NotImplemented</Code><Message>A header you provided implies functionality that is not implemented</Message><BucketName>ocaws.jl.test.20211029t193302z</BucketName><Resource>/ocaws.jl.test.20211029t193302z</Resource><RequestId>16B2975417EDF488</RequestId><HostId>b3a8880e-5b1b-44ff-9155-7653d625b88e</HostId></Error> ``` This change should ensure that internal parsing failures don't mask the actual failure being returned from the API. Co-authored-by: Curtis Vogt <[email protected]>
JuliaCloud · Nov 2, 2021 · 99fcd5b · 99fcd5b
2 parents 66b5203 + bcb1d4d
commit 99fcd5b
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 56 deletions.
diff --git a/src/AWSExceptions.jl b/src/AWSExceptions.jl
@@ -54,10 +54,6 @@ function Base.show(io::IO, e::AWSException)
     return nothing
 end
 
-http_status(e::HTTP.StatusError) = e.status
-content_type(e::HTTP.StatusError) = HTTP.header(e.response, "Content-Type")
-is_valid_xml_string(str) = startswith(str, '<')
-
 AWSException(e::HTTP.StatusError) = AWSException(e, String(copy(e.response.body)))
 
 function AWSException(e::HTTP.StatusError, stream::IO)
@@ -67,33 +63,44 @@ function AWSException(e::HTTP.StatusError, stream::IO)
 end
 
 function AWSException(e::HTTP.StatusError, body::AbstractString)
-    code = string(http_status(e))
+    content_type = HTTP.header(e.response, "Content-Type")
+    code = string(e.status)
     message = "AWSException"
     info = Dict{String,Dict}()
 
-    if !isempty(body)
-        # Extract API error code from Lambda-style JSON error message...
-        if endswith(content_type(e), "json")
-            info = JSON.parse(body)
-        end
+    try
+        if !isempty(body)
+            # Extract API error code from Lambda-style JSON error message...
+            if endswith(content_type, "json")
+                info = JSON.parse(body)
+            end
 
-        # Extract API error code from JSON error message...
-        if occursin(r"^application/x-amz-json-1\.[01]$", content_type(e))
-            info = JSON.parse(body)
-            if haskey(info, "__type")
-                code = rsplit(info["__type"], '#'; limit=2)[end]
+            # Extract API error code from JSON error message...
+            if occursin(r"^application/x-amz-json-1\.[01]$", content_type)
+                info = JSON.parse(body)
+                if haskey(info, "__type")
+                    code = rsplit(info["__type"], '#'; limit=2)[end]
+                end
             end
-        end
 
-        # Extract API error code from XML error message...
-        error_content_types = ["", "application/xml", "text/xml"]
-        if content_type(e) in error_content_types && is_valid_xml_string(body)
-            info = parse_xml(body)
+            # Extract API error code from XML error message...
+            if endswith(content_type, "/xml") || startswith(body, "<?xml")
+                info = parse_xml(body)
+            end
+        elseif parse(Int, HTTP.header(e.response, "Content-Length", "0")) > 0
+            # Should only occur streaming a response and error handling is improperly configured
+            @error "Internal Error: provided body is empty while the reported content-length " *
+                "is non-zero"
+        end
+    catch err
+        # Avoid throwing internal exceptions when parsing the error as this will result
+        # in the streamed content being hidden from the user.
+        @error sprint() do io
+            println(io, "Internal error: Failed to extract API error code:")
+            Base.showerror(io, err)
+            Base.show_backtrace(io, catch_backtrace())
+            println(io)
         end
-    elseif parse(Int, HTTP.header(e.response, "Content-Length", "0")) > 0
-        # Should only occur streaming a response and error handling is improperly configured
-        @error "Internal Error: provided body is empty while the reported content-length " *
-            "is non-zero"
     end
 
     # There are times when Errors or Error are returned back

diff --git a/src/utilities/response.jl b/src/utilities/response.jl
@@ -37,7 +37,7 @@ function mime_type(r::Response)
 
     T = if occursin(r"/xml", mime) || magic_bytes == b"<?xml"
         MIME"application/xml"
-    elseif occursin(r"/x-amz-json-1.[01]$", mime) || endswith(mime, "json")
+    elseif occursin(r"/x-amz-json-1\.[01]$", mime) || endswith(mime, "json")
         MIME"application/json"
     elseif startswith(mime, "text/")
         MIME"text/plain"

diff --git a/test/AWSExceptions.jl b/test/AWSExceptions.jl
@@ -2,9 +2,10 @@
     function _test_exception(ex::AWSException, expected::AbstractDict, msg::String)
         @test ex.code == expected["code"]
         @test ex.message == ex.info[msg] == expected["message"]
-        @test String(ex.cause.response.body) == expected["body"]
+        @test ex.cause.response.body == expected["body"]
         @test ex.cause.status == expected["status_code"]
         @test ex.cause.response.headers == expected["headers"]
+        @test ex.streamed_body == expected["streamed_body"]
     end
 
     cases = [
@@ -23,26 +24,24 @@
             "status_code" => 400,
         )
 
-        body = """
-        <?xml version="1.0" encoding="UTF-8"?>
-        <$err>
-        <Code>$(expected["code"])</Code>
-        <$msg>$(expected["message"])</$msg>
-        <Resource>$(expected["resource"])</Resource>
-        <RequestId>$(expected["requestId"])</RequestId>
-        </$err>
-        """
-
-        expected["body"] = body
+        expected["body"] = HTTP.MessageRequest.body_was_streamed
+        expected["streamed_body"] = """
+            <?xml version="1.0" encoding="UTF-8"?>
+            <$err>
+            <Code>$(expected["code"])</Code>
+            <$msg>$(expected["message"])</$msg>
+            <Resource>$(expected["resource"])</Resource>
+            <RequestId>$(expected["requestId"])</RequestId>
+            </$err>
+            """
 
         # This does not actually send a request, just creates the object to test with
-        req = HTTP.Request(
-            "GET", "https://amazon.ca", expected["headers"], expected["body"]
-        )
+        req = HTTP.Request("GET", "https://aws.com", expected["headers"], expected["body"])
         resp = HTTP.Response(
             expected["status_code"], expected["headers"]; body=expected["body"], request=req
         )
-        ex = AWSException(HTTP.StatusError(expected["status_code"], resp))
+        status_error = HTTP.StatusError(expected["status_code"], resp)
+        ex = AWSException(status_error, expected["streamed_body"])
 
         _test_exception(ex, expected, msg)
         @test ex.info["Resource"] == expected["resource"]
@@ -51,19 +50,20 @@
 
     @testset "XMLRequest - Invalid XML" begin
         expected = Dict(
-            "body" => "InvalidXML",
+            "body" => HTTP.MessageRequest.body_was_streamed,
+            "streamed_body" => """<?xml version="1.0" encoding="UTF-8"?>InvalidXML""",
             "headers" => ["Content-Type" => "application/xml"],
             "status_code" => 404,
         )
-        req = HTTP.Request(
-            "GET", "https://amazon.ca", expected["headers"], expected["body"]
-        )
+        req = HTTP.Request("GET", "https://aws.com", expected["headers"], expected["body"])
         resp = HTTP.Response(
             expected["status_code"], expected["headers"]; body=expected["body"], request=req
         )
-        ex = AWSException(HTTP.StatusError(expected["status_code"], resp))
+        status_error = HTTP.StatusError(expected["status_code"], resp)
+        ex = @test_logs (:error,) AWSException(status_error, expected["streamed_body"])
 
         @test ex.code == "404"
+        @test ex.streamed_body == expected["streamed_body"]
     end
 
     @testset "JSON Request -- $msg" for msg in ["message", "Message"]
@@ -74,20 +74,21 @@
             "status_code" => 400,
         )
 
-        body = """
-        {
-        "__type": "$(expected["code"])",
-        "$msg": "$(expected["message"])"
-        }
-        """
-        expected["body"] = body
+        expected["body"] = HTTP.MessageRequest.body_was_streamed
+        expected["streamed_body"] = """
+            {
+            "__type": "$(expected["code"])",
+            "$msg": "$(expected["message"])"
+            }
+            """
 
         # This does not actually send a request, just creates the object to test with
-        req = HTTP.Request("GET", "https://amazon.ca", expected["headers"], body)
+        req = HTTP.Request("GET", "https://aws.com", expected["headers"], expected["body"])
         resp = HTTP.Response(
-            expected["status_code"], expected["headers"]; body=body, request=req
+            expected["status_code"], expected["headers"]; body=expected["body"], request=req
         )
-        ex = AWSException(HTTP.StatusError(expected["status_code"], resp))
+        status_error = HTTP.StatusError(expected["status_code"], resp)
+        ex = AWSException(status_error, expected["streamed_body"])
 
         _test_exception(ex, expected, "$msg")
         @test ex.info["__type"] == expected["code"]