From 5fdb9f4b5d25b201bc24840cb40accea396009d4 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Thu, 25 Feb 2021 22:37:34 +0000 Subject: [PATCH] Remove null body stream (#1741) * Remove LimitBodyStream Move NullBodyStream to internal --- sdk/core/azure-core/CHANGELOG.md | 2 + sdk/core/azure-core/CMakeLists.txt | 1 + .../inc/azure/core/http/body_stream.hpp | 64 ------------------- .../azure-core/inc/azure/core/http/http.hpp | 14 +--- .../core/internal/http/null_body_stream.hpp | 47 ++++++++++++++ sdk/core/azure-core/src/http/body_stream.cpp | 10 --- sdk/core/azure-core/src/http/http.cpp | 14 ++++ sdk/core/azure-core/test/ut/http.cpp | 7 +- .../src/share_file_client.cpp | 3 +- 9 files changed, 72 insertions(+), 90 deletions(-) create mode 100644 sdk/core/azure-core/inc/azure/core/internal/http/null_body_stream.hpp diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index 7d4518523..9f79fcafb 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -10,6 +10,8 @@ - Renamed `NoRevoke` to `EnableCertificateRevocationListCheck` for `Azure::Core::Http::CurlTransportSSLOptions`. - Renamed `GetString()` to `ToString()` in `Azure::Core::DateTime`. - Renamed `GetUuidString()` tp `ToString()` in `Azure::Core::Uuid`. +- Moved `NullBodyStream` to internal usage only. It is not meant for public use. +- Removed `LimitBodyStream`. ### Bug Fixes diff --git a/sdk/core/azure-core/CMakeLists.txt b/sdk/core/azure-core/CMakeLists.txt index fe070834e..225296417 100644 --- a/sdk/core/azure-core/CMakeLists.txt +++ b/sdk/core/azure-core/CMakeLists.txt @@ -52,6 +52,7 @@ set( inc/azure/core/http/transport.hpp inc/azure/core/internal/contract.hpp inc/azure/core/internal/hkeyholder.hpp + inc/azure/core/internal/http/null_body_stream.hpp inc/azure/core/internal/http/pipeline.hpp inc/azure/core/internal/json_serializable.hpp inc/azure/core/internal/json.hpp diff --git a/sdk/core/azure-core/inc/azure/core/http/body_stream.hpp b/sdk/core/azure-core/inc/azure/core/http/body_stream.hpp index 9ab8bc38a..f7d36396d 100644 --- a/sdk/core/azure-core/inc/azure/core/http/body_stream.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/body_stream.hpp @@ -160,38 +160,6 @@ namespace Azure { namespace Core { namespace Http { void Rewind() override { m_offset = 0; } }; - /** - * @brief Empty #Azure::Core::Http::BodyStream. - * @remark Used for requests with no body. - */ - class NullBodyStream : public Azure::Core::Http::BodyStream { - private: - int64_t OnRead(Azure::Core::Context const& context, uint8_t* buffer, int64_t count) override - { - (void)context; - (void)buffer; - (void)count; - return 0; - }; - - public: - /// Constructor. - explicit NullBodyStream() {} - - int64_t Length() const override { return 0; } - - void Rewind() override {} - - /** - * @brief Gets a singleton instance of a #Azure::Core::Http::NullBodyStream. - */ - static NullBodyStream* GetNullBodyStream() - { - static NullBodyStream nullBodyStream; - return &nullBodyStream; - } - }; - /** * @brief #Azure::Core::Http::BodyStream providing its data from a file. */ @@ -243,36 +211,4 @@ namespace Azure { namespace Core { namespace Http { int64_t Length() const override { return this->m_length; }; }; - /** - * @brief #Azure::Core::Http::BodyStream that provides its data from another - * #Azure::Core::Http::BodyStream. - */ - class LimitBodyStream : public BodyStream { - private: - BodyStream* m_inner; - int64_t m_length; - int64_t m_bytesRead = 0; - - int64_t OnRead(Context const& context, uint8_t* buffer, int64_t count) override; - - public: - /** - * @brief Construct from another #Azure::Core::Http::BodyStream. - * - * @param inner #Azure::Core::Http::BodyStream to provide the data from to the readers. - * @param max_length Maximum number of bytes to provide to the readers. - */ - LimitBodyStream(BodyStream* inner, int64_t max_length) - : m_inner(inner), m_length((std::min)(inner->Length(), max_length)) - { - } - - int64_t Length() const override { return this->m_length; } - void Rewind() override - { - this->m_inner->Rewind(); - this->m_bytesRead = 0; - } - }; - }}} // namespace Azure::Core::Http diff --git a/sdk/core/azure-core/inc/azure/core/http/http.hpp b/sdk/core/azure-core/inc/azure/core/http/http.hpp index cc859f7ae..4ab0edc04 100644 --- a/sdk/core/azure-core/inc/azure/core/http/http.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/http.hpp @@ -499,14 +499,7 @@ namespace Azure { namespace Core { namespace Http { * @param downloadViaStream A boolean value indicating whether download should happen via * stream. */ - explicit Request(HttpMethod httpMethod, Url url, bool downloadViaStream) - : Request( - httpMethod, - std::move(url), - NullBodyStream::GetNullBodyStream(), - downloadViaStream) - { - } + explicit Request(HttpMethod httpMethod, Url url, bool downloadViaStream); /** * @brief Construct an #Azure::Core::Http::Request. @@ -514,10 +507,7 @@ namespace Azure { namespace Core { namespace Http { * @param httpMethod HTTP method. * @param url URL. */ - explicit Request(HttpMethod httpMethod, Url url) - : Request(httpMethod, std::move(url), NullBodyStream::GetNullBodyStream(), false) - { - } + explicit Request(HttpMethod httpMethod, Url url); /** * @brief Add HTTP header to the #Azure::Core::Http::Request. diff --git a/sdk/core/azure-core/inc/azure/core/internal/http/null_body_stream.hpp b/sdk/core/azure-core/inc/azure/core/internal/http/null_body_stream.hpp new file mode 100644 index 000000000..2ee1697f5 --- /dev/null +++ b/sdk/core/azure-core/inc/azure/core/internal/http/null_body_stream.hpp @@ -0,0 +1,47 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT + +/** + * @file + * @brief A null body stream for Http requests without a payload. + */ + +#pragma once + +#include "azure/core/http/body_stream.hpp" + +namespace Azure { namespace Core { namespace Internal { namespace Http { + + /** + * @brief Empty #Azure::Core::Http::BodyStream. + * @remark Used for requests with no body. + */ + class NullBodyStream : public Azure::Core::Http::BodyStream { + private: + int64_t OnRead(Azure::Core::Context const& context, uint8_t* buffer, int64_t count) override + { + (void)context; + (void)buffer; + (void)count; + return 0; + }; + + public: + /// Constructor. + explicit NullBodyStream() {} + + int64_t Length() const override { return 0; } + + void Rewind() override {} + + /** + * @brief Gets a singleton instance of a #Azure::Core::Http::NullBodyStream. + */ + static NullBodyStream* GetNullBodyStream() + { + static NullBodyStream nullBodyStream; + return &nullBodyStream; + } + }; + +}}}} // namespace Azure::Core::Internal::Http diff --git a/sdk/core/azure-core/src/http/body_stream.cpp b/sdk/core/azure-core/src/http/body_stream.cpp index 9ed11f535..4b53b5e0c 100644 --- a/sdk/core/azure-core/src/http/body_stream.cpp +++ b/sdk/core/azure-core/src/http/body_stream.cpp @@ -133,13 +133,3 @@ int64_t FileBodyStream::OnRead(Azure::Core::Context const& context, uint8_t* buf return numberOfBytesRead; } #endif - -int64_t LimitBodyStream::OnRead(Context const& context, uint8_t* buffer, int64_t count) -{ - (void)context; - // Read up to count or whatever length is remaining; whichever is less - uint64_t bytesRead - = m_inner->Read(context, buffer, std::min(count, this->m_length - this->m_bytesRead)); - this->m_bytesRead += bytesRead; - return bytesRead; -} diff --git a/sdk/core/azure-core/src/http/http.cpp b/sdk/core/azure-core/src/http/http.cpp index 21fe13c77..f08c418f8 100644 --- a/sdk/core/azure-core/src/http/http.cpp +++ b/sdk/core/azure-core/src/http/http.cpp @@ -2,9 +2,13 @@ // SPDX-License-Identifier: MIT #include "azure/core/http/http.hpp" +#include "azure/core/internal/http/null_body_stream.hpp" #include +using namespace Azure::Core::Http; +using namespace Azure::Core::Internal::Http; + void Azure::Core::Http::Details::InsertHeaderWithValidation( std::map& headers, std::string const& headerName, @@ -155,3 +159,13 @@ void Azure::Core::Http::Details::InsertHeaderWithValidation( // insert (override if duplicated) headers[headerName] = headerValue; } + +Request::Request(HttpMethod httpMethod, Url url, bool downloadViaStream) + : Request(httpMethod, std::move(url), NullBodyStream::GetNullBodyStream(), downloadViaStream) +{ +} + +Request::Request(HttpMethod httpMethod, Url url) + : Request(httpMethod, std::move(url), NullBodyStream::GetNullBodyStream(), false) +{ +} diff --git a/sdk/core/azure-core/test/ut/http.cpp b/sdk/core/azure-core/test/ut/http.cpp index b32245004..a4d959cb4 100644 --- a/sdk/core/azure-core/test/ut/http.cpp +++ b/sdk/core/azure-core/test/ut/http.cpp @@ -5,6 +5,7 @@ #include "http.hpp" #include +#include #include #include @@ -167,8 +168,8 @@ namespace Azure { namespace Core { namespace Test { Http::Url url("http://test.com"); Http::Request req(httpMethod, url); - Azure::Core::Http::NullBodyStream* d - = dynamic_cast(req.GetBodyStream()); + Azure::Core::Internal::Http::NullBodyStream* d + = dynamic_cast(req.GetBodyStream()); EXPECT_TRUE(d); req.StartTry(); @@ -184,7 +185,7 @@ namespace Azure { namespace Core { namespace Test { EXPECT_FALSE(headers.count("name")); - d = dynamic_cast(req.GetBodyStream()); + d = dynamic_cast(req.GetBodyStream()); EXPECT_TRUE(d); } diff --git a/sdk/storage/azure-storage-files-shares/src/share_file_client.cpp b/sdk/storage/azure-storage-files-shares/src/share_file_client.cpp index 07544bbaf..c64842ba4 100644 --- a/sdk/storage/azure-storage-files-shares/src/share_file_client.cpp +++ b/sdk/storage/azure-storage-files-shares/src/share_file_client.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -512,7 +513,7 @@ namespace Azure { namespace Storage { namespace Files { namespace Shares { protocolLayerOptions.LeaseIdOptional = options.AccessConditions.LeaseId; auto response = Details::ShareRestClient::File::UploadRange( m_shareFileUrl, - *Azure::Core::Http::NullBodyStream::GetNullBodyStream(), + *Azure::Core::Internal::Http::NullBodyStream::GetNullBodyStream(), *m_pipeline, context, protocolLayerOptions);