fix reviews on awschunking and default behavior

Author: kai-ion

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClientBase.h

@@ -100,11 +100,11 @@ namespace client
           m_serviceUserAgentName(std::move(serviceUserAgentName)),
           m_httpClient(std::move(httpClient)),
           m_errorMarshaller(std::move(errorMarshaller)),
-          m_interceptors{Aws::MakeShared<ChecksumInterceptor>("AwsSmithyClientBase", *m_clientConfig)}
+          m_interceptors{
+              Aws::MakeShared<ChecksumInterceptor>("AwsSmithyClientBase", *m_clientConfig),
+              Aws::MakeShared<features::ChunkingInterceptor>("AwsSmithyClientBase", *m_clientConfig)
+          }
         {
-            if (m_clientConfig->httpClientChunkedMode == Aws::Client::HttpClientChunkedMode::DEFAULT) {
-                m_interceptors.emplace_back(Aws::MakeShared<features::ChunkingInterceptor>("AwsSmithyClientBase", *m_clientConfig));
-            }
             baseInit();
         }
 

src/aws-cpp-sdk-core/include/smithy/client/features/ChunkingInterceptor.h

@@ -7,7 +7,9 @@
 
 #include <aws/core/Core_EXPORTS.h>
 #include <aws/core/client/ClientConfiguration.h>
-#include <aws/core/utils/stream/AwsChunkedStream.h>
+#include <aws/core/utils/HashingUtils.h>
+#include <aws/core/utils/StringUtils.h>
+#include <aws/core/utils/Array.h>
 #include <smithy/interceptor/Interceptor.h>
 #include <smithy/client/common/AwsSmithyClientUtils.h>
 
@@ -17,14 +19,17 @@ namespace features {
 
 namespace {
 
-// String constants
 static const char* const CHECKSUM_HEADER_PREFIX = "x-amz-checksum-";
 static const char* const ALLOCATION_TAG = "ChunkingInterceptor";
+static const size_t DATA_BUFFER_SIZE = 65536;
+
+} // anonymous namespace
 
 class AwsChunkedStreamBuf : public std::streambuf {
 public:
-    AwsChunkedStreamBuf(Aws::Http::HttpRequest* request, const std::shared_ptr<Aws::IOStream>& originalBody)
-        : m_chunkStream(request, originalBody) {
+    AwsChunkedStreamBuf(Aws::Http::HttpRequest* request, const std::shared_ptr<Aws::IOStream>& originalBody, size_t bufferSize = DATA_BUFFER_SIZE)
+        : m_request(request), m_stream(originalBody), m_data(bufferSize), m_bufferSize(bufferSize),
+          m_chunkingStream(Aws::MakeShared<Aws::StringStream>(ALLOCATION_TAG)) {
         setg(nullptr, nullptr, nullptr);
     }
 
@@ -33,8 +38,23 @@ class AwsChunkedStreamBuf : public std::streambuf {
         if (gptr() < egptr()) {
             return traits_type::to_int_type(*gptr());
         }
+      
+        if (m_stream->good()) {
+            m_stream->read(m_data.GetUnderlyingData(), m_bufferSize);
+            size_t bytesRead = static_cast<size_t>(m_stream->gcount());
+            writeChunk(bytesRead);
+            
+            if ((m_stream->peek() == EOF || m_stream->eof()) && !m_stream->bad()) {
+                writeTrailerToUnderlyingStream();
+            }
+        }
 
-        size_t bytesRead = m_chunkStream.BufferedRead(m_buffer, sizeof(m_buffer));
+        if ((m_chunkingStream->peek() == EOF || m_chunkingStream->eof()) && !m_chunkingStream->bad()) {
+            return traits_type::eof();
+        }
+        
+        m_chunkingStream->read(m_buffer, sizeof(m_buffer));
+        size_t bytesRead = static_cast<size_t>(m_chunkingStream->gcount());
         if (bytesRead == 0) {
             return traits_type::eof();
         }
@@ -43,24 +63,51 @@ class AwsChunkedStreamBuf : public std::streambuf {
         return traits_type::to_int_type(*gptr());
     }
 
-    std::streamsize showmanyc() override {
-        return egptr() - gptr();
+private:
+    void writeTrailerToUnderlyingStream() {
+        Aws::StringStream chunkedTrailerStream;
+        chunkedTrailerStream << "0\r\n";
+        if (m_request->GetRequestHash().second != nullptr) {
+            chunkedTrailerStream << "x-amz-checksum-" << m_request->GetRequestHash().first << ":"
+                               << Aws::Utils::HashingUtils::Base64Encode(m_request->GetRequestHash().second->GetHash().GetResult()) << "\r\n";
+        }
+        chunkedTrailerStream << "\r\n";
+        const auto chunkedTrailer = chunkedTrailerStream.str();
+        if (m_chunkingStream->eof()) {
+            m_chunkingStream->clear();
+        }
+        *m_chunkingStream << chunkedTrailer;
+    }
+    
+    void writeChunk(size_t bytesRead) {
+        if (m_request->GetRequestHash().second != nullptr) {
+            m_request->GetRequestHash().second->Update(reinterpret_cast<unsigned char*>(m_data.GetUnderlyingData()), bytesRead);
+        }
+        
+        if (bytesRead > 0 && m_chunkingStream != nullptr && !m_chunkingStream->bad()) {
+            if (m_chunkingStream->eof()) {
+                m_chunkingStream->clear();
+            }
+            *m_chunkingStream << Aws::Utils::StringUtils::ToHexString(bytesRead) << "\r\n";
+            m_chunkingStream->write(m_data.GetUnderlyingData(), bytesRead);
+            *m_chunkingStream << "\r\n";
+        }
     }
 
-private:
-    Aws::Utils::Stream::AwsChunkedStream<> m_chunkStream;
+    Aws::Http::HttpRequest* m_request;
+    std::shared_ptr<Aws::IOStream> m_stream;
+    Aws::Utils::Array<char> m_data;
+    size_t m_bufferSize;
+    std::shared_ptr<Aws::IOStream> m_chunkingStream;
     char m_buffer[8192];
 };
 
-} // anonymous namespace
-
 /**
  * Interceptor that handles chunked encoding for streaming requests with checksums.
  * Wraps request body with chunked stream and sets appropriate headers.
  */
 class ChunkingInterceptor : public smithy::interceptor::Interceptor {
 public:
-    ChunkingInterceptor() : m_httpClientChunkedMode(Aws::Client::HttpClientChunkedMode::DEFAULT) {}
     explicit ChunkingInterceptor(const Aws::Client::ClientConfiguration& config)
         : m_httpClientChunkedMode(config.httpClientChunkedMode) {}
     ~ChunkingInterceptor() override = default;

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

@@ -140,7 +140,7 @@ AWSClient::AWSClient(const Aws::Client::ClientConfiguration& configuration,
     m_enableClockSkewAdjustment(configuration.enableClockSkewAdjustment),
     m_requestCompressionConfig(configuration.requestCompressionConfig),
     m_userAgentInterceptor{Aws::MakeShared<smithy::client::UserAgentInterceptor>(AWS_CLIENT_LOG_TAG, configuration, m_retryStrategy->GetStrategyName(), m_serviceName)},
-    m_interceptors{Aws::MakeShared<smithy::client::ChecksumInterceptor>(AWS_CLIENT_LOG_TAG), Aws::MakeShared<smithy::client::features::ChunkingInterceptor>(AWS_CLIENT_LOG_TAG), m_userAgentInterceptor}
+    m_interceptors{Aws::MakeShared<smithy::client::ChecksumInterceptor>(AWS_CLIENT_LOG_TAG), Aws::MakeShared<smithy::client::features::ChunkingInterceptor>(AWS_CLIENT_LOG_TAG, configuration), m_userAgentInterceptor}
 {
 }
 

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

@@ -220,15 +220,9 @@ void setLegacyClientConfigurationParameters(ClientConfiguration& clientConfig)
     clientConfig.writeRateLimiter = nullptr;
     clientConfig.readRateLimiter = nullptr;
     clientConfig.httpLibOverride = Aws::Http::TransferLibType::DEFAULT_CLIENT;
-    
-    // Set chunking mode based on HTTP client type
-    // AWS built-in clients should use SDK's ChunkingInterceptor (DEFAULT mode)
-    // Custom clients should handle chunking themselves (CLIENT_IMPLEMENTATION mode)
-    if (clientConfig.httpLibOverride == Aws::Http::TransferLibType::DEFAULT_CLIENT) {
-        clientConfig.httpClientChunkedMode = HttpClientChunkedMode::CLIENT_IMPLEMENTATION;
-    } else {
-        clientConfig.httpClientChunkedMode = HttpClientChunkedMode::DEFAULT;
-    }
+
+    // Users can explicitly set CLIENT_IMPLEMENTATION if their custom client handles chunking
+    clientConfig.httpClientChunkedMode = HttpClientChunkedMode::DEFAULT;
 
     clientConfig.followRedirects = FollowRedirectsPolicy::DEFAULT;
     clientConfig.disableExpectHeader = false;

src/aws-cpp-sdk-core/source/http/curl/CurlHttpClient.cpp

@@ -370,6 +370,14 @@ static size_t SeekBody(void* userdata, curl_off_t offset, int origin)
         return CURL_SEEKFUNC_FAIL;
     }
 
+    // Fail seek for aws-chunk encoded body as the length and offset is unknown
+    if (context->m_request &&
+        context->m_request->HasHeader(Aws::Http::CONTENT_ENCODING_HEADER) &&
+        context->m_request->GetHeaderValue(Aws::Http::CONTENT_ENCODING_HEADER).find(Aws::Http::AWS_CHUNKED_VALUE) != Aws::String::npos)
+    {
+        return CURL_SEEKFUNC_FAIL;
+    }
+
 
     HttpRequest* request = context->m_request;
     const std::shared_ptr<Aws::IOStream>& ioStream = request->GetContentBody();

tests/aws-cpp-sdk-core-tests/utils/stream/AwsChunkedStreamTest.cpp

@@ -5,95 +5,154 @@
 #include <aws/core/http/standard/StandardHttpRequest.h>
 #include <aws/core/utils/crypto/CRC32.h>
 #include <aws/core/utils/stream/AwsChunkedStream.h>
+#include <smithy/client/features/ChunkingInterceptor.h>
 #include <aws/testing/AwsCppSdkGTestSuite.h>
 
 using namespace Aws;
 using namespace Aws::Http::Standard;
 using namespace Aws::Utils::Stream;
 using namespace Aws::Utils::Crypto;
 
-class AwsChunkedStreamTest : public Aws::Testing::AwsCppSdkGTestSuite {};
-
 const char* TEST_LOG_TAG = "AWS_CHUNKED_STREAM_TEST";
 
+class AwsChunkedStreamTest : public Aws::Testing::AwsCppSdkGTestSuite {
+protected:
+    StandardHttpRequest CreateRequestWithChecksum(const std::string& url, const std::string& /* data */) {
+        StandardHttpRequest request{url, Http::HttpMethod::HTTP_GET};
+        auto requestHash = Aws::MakeShared<CRC32>(TEST_LOG_TAG);
+        request.SetRequestHash("crc32", requestHash);
+        return request;
+    }
+    
+    std::shared_ptr<Aws::IOStream> CreateChunkedStreamBuf(StandardHttpRequest& request, const std::string& data, size_t bufferSize = 65536) {
+        auto inputStream = Aws::MakeShared<StringStream>(TEST_LOG_TAG, data);
+        auto chunkedBuf = Aws::MakeUnique<smithy::client::features::AwsChunkedStreamBuf>(
+            TEST_LOG_TAG, &request, inputStream, bufferSize);
+        return std::shared_ptr<Aws::IOStream>(new Aws::IOStream(chunkedBuf.release()));
+    }
+    
+    std::string ReadEntireStream(std::shared_ptr<Aws::IOStream> stream) {
+        Aws::StringStream output;
+        char buffer[100];
+        while (stream->read(buffer, sizeof(buffer))) {
+            output.write(buffer, stream->gcount());
+        }
+        if (stream->gcount() > 0) {
+            output.write(buffer, stream->gcount());
+        }
+        return output.str();
+    }
+    
+    template<size_t BufferSize>
+    std::string ReadEntireStream(std::shared_ptr<AwsChunkedStream<BufferSize>> stream) {
+        Aws::StringStream output;
+        Aws::Utils::Array<char> buffer{100};
+        size_t bytesRead;
+        while ((bytesRead = stream->BufferedRead(buffer.GetUnderlyingData(), 100)) > 0) {
+            std::copy(buffer.GetUnderlyingData(), buffer.GetUnderlyingData() + bytesRead, std::ostream_iterator<char>(output));
+        }
+        return output.str();
+    }
+    
+    template<size_t BufferSize>
+    std::shared_ptr<AwsChunkedStream<BufferSize>> CreateAwsChunkedStream(StandardHttpRequest& request, const std::string& data) {
+        auto inputStream = Aws::MakeShared<StringStream>(TEST_LOG_TAG, data);
+        return Aws::MakeShared<AwsChunkedStream<BufferSize>>(TEST_LOG_TAG, &request, inputStream);
+    }
+};
+
 TEST_F(AwsChunkedStreamTest, ChunkedStreamShouldWork) {
-  StandardHttpRequest request{"www.elda.com/will", Http::HttpMethod::HTTP_GET};
-  auto requestHash = Aws::MakeShared<CRC32>(TEST_LOG_TAG);
-  request.SetRequestHash("crc32", requestHash);
-  std::shared_ptr<IOStream> inputStream = Aws::MakeShared<StringStream>(TEST_LOG_TAG, "1234567890123456789012345");
-  AwsChunkedStream<10> chunkedStream{&request, inputStream};
-  Aws::Utils::Array<char> outputBuffer{100};
-  Aws::StringStream output;
-  size_t readIterations{4};
-  size_t bufferOffset{0};
-  while (readIterations > 0) {
-    bufferOffset = chunkedStream.BufferedRead(outputBuffer.GetUnderlyingData(), 10);
-    std::copy(outputBuffer.GetUnderlyingData(), outputBuffer.GetUnderlyingData() + bufferOffset, std::ostream_iterator<char>(output));
-    readIterations--;
-  }
-  // Read trailing checksum that is greater than 10 chars
-  bufferOffset = chunkedStream.BufferedRead(outputBuffer.GetUnderlyingData(), 40);
-  EXPECT_EQ(36ul, bufferOffset);
-  std::copy(outputBuffer.GetUnderlyingData(), outputBuffer.GetUnderlyingData() + bufferOffset, std::ostream_iterator<char>(output));
-  const auto encodedStr = output.str();
+  auto request = CreateRequestWithChecksum("www.elda.com/will", "1234567890123456789012345");
+  auto chunkedStream = CreateAwsChunkedStream<10>(request, "1234567890123456789012345");
+  
+  const auto encodedStr = ReadEntireStream(chunkedStream);
   auto expectedStreamWithChecksum = "A\r\n1234567890\r\nA\r\n1234567890\r\n5\r\n12345\r\n0\r\nx-amz-checksum-crc32:78DeVw==\r\n\r\n";
   EXPECT_EQ(expectedStreamWithChecksum, encodedStr);
 }
 
 TEST_F(AwsChunkedStreamTest, ShouldNotRequireTwoReadsOnSmallChunk) {
-  StandardHttpRequest request{"www.clemar.com/strohl", Http::HttpMethod::HTTP_GET};
-  auto requestHash = Aws::MakeShared<CRC32>(TEST_LOG_TAG);
-  request.SetRequestHash("crc32", requestHash);
-  std::shared_ptr<IOStream> inputStream = Aws::MakeShared<StringStream>(TEST_LOG_TAG, "12345");
-  AwsChunkedStream<100> chunkedStream{&request, inputStream};
-  Aws::Utils::Array<char> outputBuffer{100};
-  Aws::StringStream output;
-  const auto bufferOffset = chunkedStream.BufferedRead(outputBuffer.GetUnderlyingData(), 100);
-  std::copy(outputBuffer.GetUnderlyingData(), outputBuffer.GetUnderlyingData() + bufferOffset, std::ostream_iterator<char>(output));
-  EXPECT_EQ(46ul, bufferOffset);
-  const auto encodedStr = output.str();
+  auto request = CreateRequestWithChecksum("www.clemar.com/strohl", "12345");
+  auto chunkedStream = CreateAwsChunkedStream<100>(request, "12345");
+  
+  const auto encodedStr = ReadEntireStream(chunkedStream);
   auto expectedStreamWithChecksum = "5\r\n12345\r\n0\r\nx-amz-checksum-crc32:y/U6HA==\r\n\r\n";
   EXPECT_EQ(expectedStreamWithChecksum, encodedStr);
 }
 
 TEST_F(AwsChunkedStreamTest, ShouldWorkOnSmallBuffer) {
-  StandardHttpRequest request{"www.eugief.com/hesimay", Http::HttpMethod::HTTP_GET};
-  auto requestHash = Aws::MakeShared<CRC32>(TEST_LOG_TAG);
-  request.SetRequestHash("crc32", requestHash);
-  std::shared_ptr<IOStream> inputStream = Aws::MakeShared<StringStream>(TEST_LOG_TAG, "1234567890");
-  AwsChunkedStream<5> chunkedStream{&request, inputStream};
-  Aws::Utils::Array<char> outputBuffer{100};
-  // Read first 5 bytes, we get back ten bytes chunk encoded since it is "5\r\n12345\r\n"
-  Aws::StringStream firstRead;
-  auto amountRead = chunkedStream.BufferedRead(outputBuffer.GetUnderlyingData(), 100);
-  std::copy(outputBuffer.GetUnderlyingData(), outputBuffer.GetUnderlyingData() + amountRead, std::ostream_iterator<char>(firstRead));
+  auto request = CreateRequestWithChecksum("www.eugief.com/hesimay", "1234567890");
+  auto chunkedStream = CreateAwsChunkedStream<5>(request, "1234567890");
+  
+  // Read first chunk
+  char buffer[10];
+  auto amountRead = chunkedStream->BufferedRead(buffer, sizeof(buffer));
+  std::string firstRead(buffer, amountRead);
   EXPECT_EQ(10ul, amountRead);
-  auto encodedStr = firstRead.str();
-  EXPECT_EQ("5\r\n12345\r\n", encodedStr);
-  // Read second 5 bytes, we get back 46 bytes because we exhaust the underlying buffer
-  // abd write the trailer "5\r\n67890\r\n0\r\nx-amz-checksum-crc32:Jh2u5Q==\r\n\r\n"
-  Aws::StringStream secondRead;
-  amountRead = chunkedStream.BufferedRead(outputBuffer.GetUnderlyingData(), 100);
-  std::copy(outputBuffer.GetUnderlyingData(), outputBuffer.GetUnderlyingData() + amountRead, std::ostream_iterator<char>(secondRead));
+  EXPECT_EQ("5\r\n12345\r\n", firstRead);
+  
+  // Read remaining data
+  char buffer2[50];
+  amountRead = chunkedStream->BufferedRead(buffer2, sizeof(buffer2));
+  std::string secondRead(buffer2, amountRead);
   EXPECT_EQ(46ul, amountRead);
-  encodedStr = secondRead.str();
-  EXPECT_EQ("5\r\n67890\r\n0\r\nx-amz-checksum-crc32:Jh2u5Q==\r\n\r\n", encodedStr);
-  // Any subsequent reads will return 0 because all streams are exhausted
-  amountRead = chunkedStream.BufferedRead(outputBuffer.GetUnderlyingData(), 100);
+  EXPECT_EQ("5\r\n67890\r\n0\r\nx-amz-checksum-crc32:Jh2u5Q==\r\n\r\n", secondRead);
+  
+  // Any subsequent reads should return 0
+  amountRead = chunkedStream->BufferedRead(buffer, sizeof(buffer));
   EXPECT_EQ(0ul, amountRead);
 }
 
 TEST_F(AwsChunkedStreamTest, ShouldWorkOnEmptyStream) {
-  StandardHttpRequest request{"www.nidia.com/juna", Http::HttpMethod::HTTP_GET};
-  auto requestHash = Aws::MakeShared<CRC32>(TEST_LOG_TAG);
-  request.SetRequestHash("crc32", requestHash);
-  std::shared_ptr<IOStream> inputStream = Aws::MakeShared<StringStream>(TEST_LOG_TAG, "");
-  AwsChunkedStream<5> chunkedStream{&request, inputStream};
-  Aws::Utils::Array<char> outputBuffer{100};
-  Aws::StringStream firstRead;
-  auto amountRead = chunkedStream.BufferedRead(outputBuffer.GetUnderlyingData(), 100);
-  std::copy(outputBuffer.GetUnderlyingData(), outputBuffer.GetUnderlyingData() + amountRead, std::ostream_iterator<char>(firstRead));
-  EXPECT_EQ(36ul, amountRead);
-  auto encodedStr = firstRead.str();
+  auto request = CreateRequestWithChecksum("www.nidia.com/juna", "");
+  auto chunkedStream = CreateAwsChunkedStream<5>(request, "");
+  
+  const auto encodedStr = ReadEntireStream(chunkedStream);
+  EXPECT_EQ("0\r\nx-amz-checksum-crc32:AAAAAA==\r\n\r\n", encodedStr);
+}
+
+TEST_F(AwsChunkedStreamTest, ChunkingInterceptorStreamBufShouldWork) {
+  auto request = CreateRequestWithChecksum("www.elda.com/will", "1234567890123456789012345");
+  auto chunkedBody = CreateChunkedStreamBuf(request, "1234567890123456789012345", 10);
+  
+  const auto encodedStr = ReadEntireStream(chunkedBody);
+  auto expectedStreamWithChecksum = "A\r\n1234567890\r\nA\r\n1234567890\r\n5\r\n12345\r\n0\r\nx-amz-checksum-crc32:78DeVw==\r\n\r\n";
+  EXPECT_EQ(expectedStreamWithChecksum, encodedStr);
+}
+
+TEST_F(AwsChunkedStreamTest, ChunkingInterceptorShouldNotRequireTwoReadsOnSmallChunk) {
+  auto request = CreateRequestWithChecksum("www.clemar.com/strohl", "12345");
+  auto chunkedBody = CreateChunkedStreamBuf(request, "12345", 100);
+  
+  const auto encodedStr = ReadEntireStream(chunkedBody);
+  auto expectedStreamWithChecksum = "5\r\n12345\r\n0\r\nx-amz-checksum-crc32:y/U6HA==\r\n\r\n";
+  EXPECT_EQ(expectedStreamWithChecksum, encodedStr);
+}
+
+TEST_F(AwsChunkedStreamTest, ChunkingInterceptorShouldWorkOnSmallBuffer) {
+  auto request = CreateRequestWithChecksum("www.eugief.com/hesimay", "1234567890");
+  auto chunkedBody = CreateChunkedStreamBuf(request, "1234567890", 5);
+  
+  // Read first chunk
+  char buffer[10];
+  chunkedBody->read(buffer, sizeof(buffer));
+  std::string firstRead(buffer, chunkedBody->gcount());
+  EXPECT_EQ("5\r\n12345\r\n", firstRead);
+  
+  // Read remaining data
+  char buffer2[50];
+  chunkedBody->read(buffer2, sizeof(buffer2));
+  std::string secondRead(buffer2, chunkedBody->gcount());
+  EXPECT_EQ("5\r\n67890\r\n0\r\nx-amz-checksum-crc32:Jh2u5Q==\r\n\r\n", secondRead);
+  
+  // Any subsequent reads should return 0
+  chunkedBody->read(buffer, sizeof(buffer));
+  EXPECT_EQ(0, chunkedBody->gcount());
+}
+
+TEST_F(AwsChunkedStreamTest, ChunkingInterceptorShouldWorkOnEmptyStream) {
+  auto request = CreateRequestWithChecksum("www.nidia.com/juna", "");
+  auto chunkedBody = CreateChunkedStreamBuf(request, "", 5);
+  
+  const auto encodedStr = ReadEntireStream(chunkedBody);
   EXPECT_EQ("0\r\nx-amz-checksum-crc32:AAAAAA==\r\n\r\n", encodedStr);
 }