S3 200 error handling #3024

sbera87 · 2024-07-02T14:21:59Z

Issue #, if available:

Description of changes:

Check all that applies:

Did a review by yourself.
Added proper tests to cover this PR. (If tests are not applicable, explain.)
Checked if this PR is a breaking (APIs have been changed) change.
Checked if this PR will not introduce cross-platform inconsistent behavior.
Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

src/aws-cpp-sdk-core/source/client/AWSClient.cpp

sbiscigl · 2024-07-02T18:51:14Z

src/aws-cpp-sdk-core/source/client/AWSClient.cpp

    {
        AWS_LOGSTREAM_DEBUG(AWS_CLIENT_LOG_TAG, "Request returned error. Attempting to generate appropriate error codes from response");
        auto error = BuildAWSError(httpResponse);
        return HttpResponseOutcome(std::move(error));
    }
+    else if(request.HasEmbeddedError(httpResponse->GetResponseBody(), httpResponse->GetHeaders()))
+    {
+        return HttpResponseOutcome(std::move(BuildAWSErrorFromResponseBody(httpResponse)) );


gives a clang warning error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]

should update this to use a initializer list

return {BuildAWSErrorFromResponseBody(httpResponse)};

sbera87 · 2024-07-05T13:07:23Z

tests/testing-resources/include/aws/testing/mocks/aws/client/MockAWSClient.h

@@ -41,8 +43,14 @@ class AmazonWebServiceRequestMock : public Aws::AmazonWebServiceRequest
    virtual const char* GetServiceRequestName() const override { return "AmazonWebServiceRequestMock"; }
    virtual bool HasEmbeddedError(Aws::IOStream& body, const Aws::Http::HeaderValueCollection& header) const override {
        (void) header;
+
+        auto readPointer = body.tellg();


This fixes the body of response parsing issue existing in test cases. The pointer in the stream was being moved to the end which caused subsequent parsing to fail.

sbera87 · 2024-07-05T13:08:41Z

tests/testing-resources/include/aws/testing/mocks/aws/client/MockAWSClient.h

@@ -61,7 +69,7 @@ class CountedRetryStrategy : public Aws::Client::DefaultRetryStrategy
          m_attemptedRetries(0)
      {}
    CountedRetryStrategy(long maxRetires)
-        : Aws::Client::DefaultRetryStrategy(maxRetires <= 0 ? (std::numeric_limits<long>::max)() : maxRetires),
+        : Aws::Client::DefaultRetryStrategy(maxRetires < 0 ? (std::numeric_limits<long>::max)() : maxRetires),


This allows to try explicit tests without retry . There are times when we want to test the workflow in the first iteration only and not retry (which changes the error codes)

sbera87 · 2024-07-05T13:09:40Z

tests/testing-resources/include/aws/testing/mocks/aws/client/MockAWSClient.h

                    GetMockSecretAccessKey()), "service", config.region.empty() ? Aws::Region::US_EAST_1 : config.region),
-            Aws::MakeShared<MockAWSErrorMarshaller>("MockAWSClient")),
-        m_countedRetryStrategy(std::static_pointer_cast<CountedRetryStrategy>(config.retryStrategy)) { }
+                    errorMarshaller


Just adding delegate constructor and leveraging it in passing different error marshallers for different test use cases

sbiscigl · 2024-07-05T13:45:11Z

generated/src/aws-cpp-sdk-lambda/source/model/InvokeRequest.cpp

@@ -65,3 +65,5 @@ Aws::Http::HeaderValueCollection InvokeRequest::GetRequestSpecificHeaders() cons
  return headers;

 }
+


lets figure out what is causing this white space to change, this potentiallly could a delta of thousands of files. only codegen changes that should be visible should be in S3.

sbiscigl · 2024-07-05T13:46:44Z

src/aws-cpp-sdk-core/include/aws/core/client/AWSClient.h

@@ -74,7 +74,7 @@ namespace Aws
         * Abstract AWS Client. Contains most of the functionality necessary to build an http request, get it signed, and send it across the wire.
         */
        class AWS_CORE_API AWSClient
-        {
+        { 


nit pick: whitepace change

sbiscigl · 2024-07-05T13:50:15Z

generated/src/aws-cpp-sdk-s3/source/model/AbortMultipartUploadRequest.cpp

+  const Aws::Http::HeaderValueCollection &header) const
+{
+  // Header is unused
+  (void) header;


we have a macro AWS_UNREFERENCED_PARAM that we try to use in this instance instead of a void cast, which is itself a void cast, but its a little more readable.

sbiscigl · 2024-07-05T13:55:08Z

tests/aws-cpp-sdk-s3-unit-tests/S3UnitTests.cpp

  _mockHttpClient->AddResponseToReturn(mockResponse);

  const auto response = _s3Client->CopyObject(request);
+  std::cout<<"Custom Error2:"<<response.GetError()<<std::endl;


think you forgot to remove this

ack, will remove

sbiscigl · 2024-07-05T13:57:07Z

...ava/com/amazonaws/util/awsclientgenerator/generators/cpp/s3/S3RestXmlCppClientGenerator.java

@@ -1,578 +1,669 @@
-/**


what happend here, should try to preserve git history so that we can trace the different S3 customizations that we have going on here.

sbiscigl · 2024-07-05T14:11:45Z

src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp

+            Aws::String xmlString((Aws::IStreamBufIterator(httpResponse->GetResponseBody())), Aws::IStreamBufIterator());
+
+            ss <<xmlString;
+            ss << " current pointer location in stream = "<<httpResponse->GetResponseBody().tellp();


instead of returning the pointer location lets just stilll return No response body. and log out a debug statement. The message consumed as a error message in a response could be somewhat confusing.

sbiscigl · 2024-07-05T14:14:19Z

...src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/StreamRequestSource.vm

@@ -76,6 +76,32 @@ Aws::Http::HeaderValueCollection ${typeInfo.className}::GetRequestSpecificHeader

 }
 #end
+


ah this is is what is causing the whitespace change, put it inside the #if. a empty line in a vm file will result in a new line in the code that is generated.

sbiscigl · 2024-07-05T14:14:46Z

...azonaws/util/awsclientgenerator/velocity/cpp/s3/PutBucketNotificationConfigurationRequest.vm

@@ -83,4 +83,30 @@ Aws::Http::HeaderValueCollection ${typeInfo.className}::GetRequestSpecificHeader
  return headers;
 }
 #end
+


same thing here no new white space change outside of the if statement

generated/src/aws-cpp-sdk-s3/source/S3ErrorMarshaller.cpp

sbera87 · 2024-07-05T13:14:05Z

src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp

        Aws::StringStream ss;
-        ss << "No response body.";
+


added appropriate error message based upon the case

sbera87 · 2024-07-05T13:14:53Z

tests/aws-cpp-sdk-core-tests/aws/client/AWSClientTest.cpp

@@ -143,6 +148,28 @@ class AWSClientTestSuite : public Aws::Testing::AwsCppSdkGTestSuite
    }
 };

+
+class XMLClientTestSuite : public AWSClientTestSuite


Adding a child test suite class for xml parsing which uses a different error marshaller

this is nice!

sbera87 · 2024-07-05T14:27:41Z

generated/src/aws-cpp-sdk-s3/source/model/AbortMultipartUploadRequest.cpp

+  const Aws::Http::HeaderValueCollection &header) const
+{
+  // Header is unused
+  (void) header;


sbera87 · 2024-07-05T14:27:54Z

tests/aws-cpp-sdk-s3-unit-tests/S3UnitTests.cpp

  _mockHttpClient->AddResponseToReturn(mockResponse);

  const auto response = _s3Client->CopyObject(request);
+  std::cout<<"Custom Error2:"<<response.GetError()<<std::endl;


ack, will remove

sbera87 · 2024-07-05T14:28:08Z

...ava/com/amazonaws/util/awsclientgenerator/generators/cpp/s3/S3RestXmlCppClientGenerator.java

@@ -1,578 +1,669 @@
-/**


generated/src/aws-cpp-sdk-s3/source/S3ErrorMarshaller.cpp

sbera87 · 2024-07-08T13:11:20Z

tests/aws-cpp-sdk-s3-unit-tests/S3UnitTests.cpp

@@ -16,8 +17,29 @@ using namespace Aws::S3::Model;

 const char* ALLOCATION_TAG = "S3UnitTest";

+class S3UnitTest_S3EmbeddedErrorTestNonOKResponse_Test;
+
+class S3TestClient : public S3Client


This is a quick way of adding a test wrapper on top of S3 client class to be able to make unit test related changes

sbera87 · 2024-07-08T13:12:41Z

tests/aws-cpp-sdk-s3-unit-tests/S3UnitTests.cpp

+
+
+//Set http error and error in body in a way to hit generic xml error marshaller, which sets the exception name
+TEST_F(S3UnitTest, S3EmbeddedErrorTestNonOKResponse) {


This test case improves code coverage and tests corner case when there is a error body in response but http response code is not ok as well as the client is not S3. This defaults to generic error marshaller then.

sbera87 · 2024-07-08T13:13:25Z

tests/aws-cpp-sdk-s3-unit-tests/S3UnitTests.cpp

  auto mockRequest = Aws::MakeShared<Standard::StandardHttpRequest>(ALLOCATION_TAG, "mockuri", HttpMethod::HTTP_GET);
  mockRequest->SetResponseStreamFactory([]() -> IOStream* {
-    return Aws::New<StringStream>(ALLOCATION_TAG, R"(<?xml version="1.0" encoding="UTF-8"?><Error><Code>SlowDown</Code><Message>Please reduce your request rate.</Message></Error>)", std::ios_base::in | std::ios_base::binary);
+    const std::string mockResponseString = "\n         <?xml version=\"1.0\" encoding=\"UTF-8\"?>\n\n         <Error>\n          <Code>InternalError</Code>\n          <Message>We encountered an internal error. Please try again.</Message>\n          <RequestId>656c76696e6727732072657175657374</RequestId>\n          <HostId>Uuag1LuByRx9e6j5Onimru9pO4ZVKnJ2Qz7/C1NPcfTWAtRPfTaOFg==</HostId>\n         </Error>\n    ";


add a retryable error

sbera87 · 2024-07-08T13:14:26Z

tests/aws-cpp-sdk-s3-unit-tests/S3UnitTests.cpp

@@ -16,8 +17,29 @@ using namespace Aws::S3::Model;

 const char* ALLOCATION_TAG = "S3UnitTest";

+class S3UnitTest_S3EmbeddedErrorTestNonOKResponse_Test;


Forward declaration of test class for friend class in google test suite

sbera87 · 2024-07-08T13:16:47Z

...n/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/S3ErrorMarshallerSource.vm

+  //if response code not ok, use error marshaller
+  if(httpResponse.GetResponseCode() != HttpResponseCode::OK)
+  {
+    return XmlErrorMarshaller::Marshall(httpResponse);


for any error code other than ok, fall back to generic xml error marshaller

SergeyRyabinin · 2024-07-09T05:51:30Z

generated/src/aws-cpp-sdk-s3-crt/source/S3CrtErrorMarshaller.cpp

+    return XmlErrorMarshaller::Marshall(httpResponse);
+  }
+
+  Aws::String message{"Error in body of the response"};


why do we need this initial string value?

This is the base case error in case the if conditions are not met. This is basically default error in the top condition block

SergeyRyabinin · 2024-07-09T05:57:51Z

generated/src/aws-cpp-sdk-s3-crt/source/S3CrtErrorMarshaller.cpp

+      auto messageNode = doc.GetRootElement().FirstChild("Message") ;
+      if(!messageNode.IsNull())
+      {
+          message = messageNode.GetText();


GetText
Get the inner text of the node (potentially includes other nodes)

Please check that the xml node does not have any children (to avoid accidental tracing of wrong message). The best case would be to check if the node contains only text value, but our XML wrapper seems to not provide such api.

Ack. added an extra check if have children. But in my opinion this would still be ok because as long as the first root element is error , we will indicate error in response body. The specific code is obtained from parsing the exact message child node which is where the additional check will help

SergeyRyabinin · 2024-07-09T05:59:14Z

generated/src/aws-cpp-sdk-s3-crt/source/S3CrtErrorMarshaller.cpp

+          bodyError = codeNode.GetText();
+      }
+  }
+  body.seekg(readPointer);


nit: I think it would be better to rewind the stream right after parsing (anyway parsing is done by copying the stream into another string).

ack, in this case it won't matter since the steam was already parsed and string copied over already by this point. but I can move it to the end of the function.

SergeyRyabinin · 2024-07-09T06:04:07Z

generated/src/aws-cpp-sdk-s3-crt/source/model/AbortMultipartUploadRequest.cpp

+  const Aws::Http::HeaderValueCollection &header) const
+{
+  // Header is unused
+  (void) header;


nit but please change: AWS_UNREFERENCED_PARAM(header); or just remove the header variable name from the signature.

SergeyRyabinin · 2024-07-09T06:05:41Z

generated/src/aws-cpp-sdk-s3-crt/source/S3CrtErrorMarshaller.cpp

+  Aws::String message{"Error in body of the response"};
+  //extract error message and code in the body
+  auto& body = httpResponse.GetResponseBody();
+  auto readPointer = body.tellg();


check if the stream is valid, i.e. good before doing operations on it.

SergeyRyabinin · 2024-07-09T06:08:56Z

src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp

+        }
+        else
+        {
+            Aws::String xmlString((Aws::IStreamBufIterator(httpResponse->GetResponseBody())), Aws::IStreamBufIterator());


some debug tracing here, please remove or update

SergeyRyabinin · 2024-07-09T06:13:28Z

generated/src/aws-cpp-sdk-s3-crt/source/model/CreateBucketRequest.cpp

+  // Header is unused
+  (void) header;
+
+  auto readPointer = body.tellg();


not very clear why a similar logic is here and in ErrorMarshaller (well I guess I understand why, because one answers if there is such error and another parses and error to the object).

nit: would be nice to extract this logic into some common function in S3 Client to avoid copy-pasted generated code.

While I agree, this cleanup can be done as part of a separate PR after further review of XML error parsing. The XML parser does a few more steps than this . As of now, we can keep S3 handling separate before redirecting to a common API

…translate pin functions. With this change, customers can use one-time TR-31 keys directly in dataplane operations without the need to first import them into the service. Add v2 smoke tests and smithy smokeTests trait for SDK testing. Added further restrictions on logging of potentially sensitive inputs and outputs. JSON body inspection: Update documentation to clarify that JSON parsing doesn't include full validation. Add v2 smoke tests and smithy smokeTests trait for SDK testing. Add v2 smoke tests and smithy smokeTests trait for SDK testing. Authentication profiles are Amazon Connect resources (in gated preview) that allow you to configure authentication settings for users in your contact center. This release adds support for new ListAuthenticationProfiles, DescribeAuthenticationProfile and UpdateAuthenticationProfile APIs. Updates EKS managed node groups to support EC2 Capacity Blocks for ML Add v2 smoke tests and smithy smokeTests trait for SDK testing. Add v2 smoke tests and smithy smokeTests trait for SDK testing.

add potential fix for windows test fail n delay loading minimalize fix for delay loading issue in windows builds

sbera87 requested a review from sbiscigl July 2, 2024 14:21

SergeyRyabinin reviewed Jul 2, 2024

View reviewed changes

src/aws-cpp-sdk-core/source/client/AWSClient.cpp Outdated Show resolved Hide resolved

sbiscigl reviewed Jul 2, 2024

View reviewed changes

sbera87 requested review from sbiscigl and SergeyRyabinin July 4, 2024 23:39

sbera87 commented Jul 5, 2024

View reviewed changes

sbiscigl reviewed Jul 5, 2024

View reviewed changes

sbera87 requested a review from sbiscigl July 8, 2024 13:08

sbera87 commented Jul 8, 2024

View reviewed changes

SergeyRyabinin reviewed Jul 9, 2024

View reviewed changes

sbera87 requested a review from SergeyRyabinin July 9, 2024 13:47

sbera87 changed the title ~~S3 200 error handling [Work in Progress]~~ S3 200 error handling Jul 11, 2024

sbera87 force-pushed the s3_200 branch from 50c323f to 6fbfb58 Compare July 11, 2024 13:24

sbera87 and others added 8 commits July 11, 2024 13:29

add embedded errors support to more apis

89f820f

working interim changes

c5ffe2b

added new function for processing error in body

c9e5497

changes to autogen files

fd69d6b

test for single file

03fc37d

regenerated files

9a1cf95

extend embedded errors support to missing apis

b11ee1e

sbera87 and others added 16 commits July 11, 2024 13:51

get error by name

0e3cf94

changing tests for correctness

273ea06

fixes for unit tests

ad26378

updated error logging

9c2b9c7

remove stray error message

2cfbeb1

changes from feedback + infra for new tests

f129d02

add test to verify generic error marshaller with error response body

f2ecd84

remove empty line in autogen files

a3ba57d

remove newline from autogen files

dcfde10

replace std string with AWS string

3bfa18e

fix namespace in autogen

ab5aa87

changes per feedback

3ffd739

fix namespace

e93cab9

make code cleaner

45dee12

make code cleaner

6a7f01d

up crt (#3028)

f4c6bfa

add potential fix for windows test fail n delay loading minimalize fix for delay loading issue in windows builds

sbera87 force-pushed the s3_200 branch from 6fbfb58 to f4c6bfa Compare July 11, 2024 13:53

fix line enings

5dad9f3

SergeyRyabinin approved these changes Jul 11, 2024

View reviewed changes

sbera87 merged commit 6651887 into main Jul 11, 2024
4 checks passed

sbera87 deleted the s3_200 branch November 25, 2024 20:51

		@@ -65,3 +65,5 @@ Aws::Http::HeaderValueCollection InvokeRequest::GetRequestSpecificHeaders() cons
		return headers;

		}

		@@ -76,6 +76,32 @@ Aws::Http::HeaderValueCollection ${typeInfo.className}::GetRequestSpecificHeader

		}
		#end



		//Set http error and error in body in a way to hit generic xml error marshaller, which sets the exception name
		TEST_F(S3UnitTest, S3EmbeddedErrorTestNonOKResponse) {

		@@ -16,8 +17,29 @@ using namespace Aws::S3::Model;

		const char* ALLOCATION_TAG = "S3UnitTest";

		class S3UnitTest_S3EmbeddedErrorTestNonOKResponse_Test;

S3 200 error handling #3024

S3 200 error handling #3024

Conversation

sbera87 commented Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbera87 commented Jul 2, 2024 •

edited

Loading