diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index 42e65ebc0..7c915756d 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -54,6 +54,7 @@ - Renamed `azure/core/http/policy.hpp` to `azure/core/http/policies/policy.hpp` - Renamed `azure/core/http/curl/curl.hpp` to `azure/core/http/curl_transport.hpp` - Renamed `azure/core/http/winhttp/win_http_client.hpp` to `azure/core/http/win_http_transport.hpp` +- Changed return type from `std::unique_ptrOperation::Poll()` to `RawResponse const& Operation::Poll()`. ### Bug Fixes diff --git a/sdk/core/azure-core/inc/azure/core/operation.hpp b/sdk/core/azure-core/inc/azure/core/operation.hpp index cfd3c32f4..89e1f58c9 100644 --- a/sdk/core/azure-core/inc/azure/core/operation.hpp +++ b/sdk/core/azure-core/inc/azure/core/operation.hpp @@ -14,6 +14,7 @@ #include #include +#include #include namespace Azure { namespace Core { @@ -29,15 +30,52 @@ namespace Azure { namespace Core { virtual std::unique_ptr PollInternal(Context& context) = 0; virtual Response PollUntilDoneInternal(std::chrono::milliseconds period, Context& context) = 0; + virtual Azure::Core::Http::RawResponse const& GetRawResponseInternal() const = 0; protected: + std::unique_ptr m_rawResponse = nullptr; OperationStatus m_status = OperationStatus::NotStarted; + Operation() = default; + + // Define how an Operation can be move-constructed from rvalue other. Parameter `other` + // gave up ownership for the rawResponse. + Operation(Operation&& other) + : m_rawResponse(std::move(other.m_rawResponse)), m_status(other.m_status) + { + } + + // Define how an Operation can be copy-constructed from some other Operation reference. + // Operation will create a clone of the rawResponse from `other`. + Operation(Operation const& other) + : m_rawResponse(std::make_unique(other.GetRawResponse())), + m_status(other.m_status) + { + } + + // Define how an Operation can be move-assigned from rvalue other. Parameter `other` + // gave up ownership for the rawResponse. + Operation& operator=(Operation&& other) + { + this->m_rawResponse = std::move(other.m_rawResponse); + this->m_status = other.m_status; + return *this; + } + + // Define how an Operation can be copy-assigned from some other Operation reference. + // Operation will create a clone of the rawResponse from `other`. + Operation& operator=(Operation const& other) + { + this->m_rawResponse = std::make_unique(other.GetRawResponse()); + this->m_status = other.m_status; + return *this; + } + public: virtual ~Operation() {} /** - * @brief Final reuslt of the long-running operation. + * @brief Final result of the long-running operation. * * @return Response the final result of the long-running operation. */ @@ -56,7 +94,14 @@ namespace Azure { namespace Core { * @return A reference to an #Azure::Core::Http::RawResponse. * @note Does not give up ownership of the RawResponse. */ - virtual Azure::Core::Http::RawResponse const& GetRawResponse() const = 0; + Azure::Core::Http::RawResponse const& GetRawResponse() const + { + if (!m_rawResponse) + { + throw std::runtime_error("The raw response was not yet set for the Operation."); + } + return *m_rawResponse; + }; /** * @brief Returns the current #Azure::Core::OperationStatus of the long-running operation. @@ -88,11 +133,11 @@ namespace Azure { namespace Core { * * @return An HTTP #Azure::Core::Http::RawResponse returned from the service. */ - std::unique_ptr Poll() + Http::RawResponse const& Poll() { // In the cases where the customer doesn't want to use a context we new one up and pass it // through - return PollInternal(Context::GetApplicationContext()); + return Poll(Context::GetApplicationContext()); } /** @@ -102,7 +147,12 @@ namespace Azure { namespace Core { * * @return An HTTP #Azure::Core::Http::RawResponse returned from the service. */ - std::unique_ptr Poll(Context& context) { return PollInternal(context); } + Http::RawResponse const& Poll(Context& context) + { + context.ThrowIfCancelled(); + m_rawResponse = PollInternal(context); + return *m_rawResponse; + } /** * @brief Periodically calls the server till the long-running operation completes. @@ -115,7 +165,7 @@ namespace Azure { namespace Core { { // In the cases where the customer doesn't want to use a context we new one up and pass it // through - return PollUntilDoneInternal(period, Context::GetApplicationContext()); + return PollUntilDone(period, Context::GetApplicationContext()); } /** @@ -128,6 +178,7 @@ namespace Azure { namespace Core { */ Response PollUntilDone(std::chrono::milliseconds period, Context& context) { + context.ThrowIfCancelled(); return PollUntilDoneInternal(period, context); } }; diff --git a/sdk/core/azure-core/test/ut/operation_test.hpp b/sdk/core/azure-core/test/ut/operation_test.hpp index 8e035d9b5..7175cd4d8 100644 --- a/sdk/core/azure-core/test/ut/operation_test.hpp +++ b/sdk/core/azure-core/test/ut/operation_test.hpp @@ -21,7 +21,6 @@ namespace Azure { namespace Core { namespace Test { private: std::string m_operationToken; std::string m_value; - std::unique_ptr m_rawResponse; private: int m_count = 0; @@ -44,21 +43,29 @@ namespace Azure { namespace Core { namespace Test { Response PollUntilDoneInternal(std::chrono::milliseconds period, Context& context) override { - std::unique_ptr response; while (!IsDone()) { // Sleep for the period // Actual clients should respect the retry after header if present std::this_thread::sleep_for(period); - response = Poll(context); + // Poll gets a new rawResponse and replace it inside the Operation. Then return a ref to + // that raw response. + auto const& response = Poll(context); + // We can use the rawResponse from the Operation here + // Major and minor version are mocked on `PollInternal` + EXPECT_EQ(response.GetMajorVersion(), 1); + EXPECT_EQ(response.GetMinorVersion(), 0); } - return Response(m_value, std::move(response)); + return Response(m_value, std::make_unique(*m_rawResponse)); } public: - Azure::Core::Http::RawResponse const& GetRawResponse() const override { return *m_rawResponse; } + Azure::Core::Http::RawResponse const& GetRawResponseInternal() const override + { + return *m_rawResponse; + } std::string GetResumeToken() const override { return m_operationToken; } diff --git a/sdk/keyvault/azure-security-keyvault-keys/inc/azure/keyvault/keys/delete_key_operation.hpp b/sdk/keyvault/azure-security-keyvault-keys/inc/azure/keyvault/keys/delete_key_operation.hpp index 237514682..e52d57c64 100644 --- a/sdk/keyvault/azure-security-keyvault-keys/inc/azure/keyvault/keys/delete_key_operation.hpp +++ b/sdk/keyvault/azure-security-keyvault-keys/inc/azure/keyvault/keys/delete_key_operation.hpp @@ -38,7 +38,6 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { std::shared_ptr m_pipeline; Azure::Security::KeyVault::Keys::DeletedKey m_value; - std::unique_ptr m_rawResponse; std::string m_continuationToken; /* This is the implementation for checking the status of a deleted key. The key is considered @@ -53,7 +52,8 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { { while (true) { - m_rawResponse = Poll(context); + // Poll will update the raw response. + Poll(context); if (IsDone()) { break; @@ -82,7 +82,10 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { * @return A reference to an #Azure::Core::Http::RawResponse. * @note Does not give up ownership of the RawResponse. */ - Azure::Core::Http::RawResponse const& GetRawResponse() const override { return *m_rawResponse; } + Azure::Core::Http::RawResponse const& GetRawResponseInternal() const override + { + return *m_rawResponse; + } /** * @brief Get the #Azure::Security::KeyVault::Keys::DeletedKey object. diff --git a/sdk/keyvault/azure-security-keyvault-keys/src/delete_key_operation.cpp b/sdk/keyvault/azure-security-keyvault-keys/src/delete_key_operation.cpp index c76bdc211..b38fb6ee7 100644 --- a/sdk/keyvault/azure-security-keyvault-keys/src/delete_key_operation.cpp +++ b/sdk/keyvault/azure-security-keyvault-keys/src/delete_key_operation.cpp @@ -30,9 +30,10 @@ inline Azure::Core::OperationStatus CheckCompleted(Azure::Core::Http::RawRespons std::unique_ptr Azure::Security::KeyVault::Keys::DeleteKeyOperation::PollInternal(Azure::Core::Context& context) { + std::unique_ptr rawResponse; if (!IsDone()) { - m_rawResponse = m_pipeline->GetResponse( + rawResponse = m_pipeline->GetResponse( context, Azure::Core::Http::HttpMethod::Get, {_detail::DeletedKeysPath, m_value.Name()}); m_status = CheckCompleted(*m_rawResponse); } @@ -40,7 +41,7 @@ Azure::Security::KeyVault::Keys::DeleteKeyOperation::PollInternal(Azure::Core::C // To ensure the success of calling Poll multiple times, even after operation is completed, a // copy of the raw http response is returned instead of transfering the ownership of the raw // response inside the Operation. - return std::make_unique(*m_rawResponse); + return rawResponse; } Azure::Security::KeyVault::Keys::DeleteKeyOperation::DeleteKeyOperation( diff --git a/sdk/keyvault/azure-security-keyvault-keys/test/ut/key_client_delete_test_live.cpp b/sdk/keyvault/azure-security-keyvault-keys/test/ut/key_client_delete_test_live.cpp index f4c90175c..58029d2e2 100644 --- a/sdk/keyvault/azure-security-keyvault-keys/test/ut/key_client_delete_test_live.cpp +++ b/sdk/keyvault/azure-security-keyvault-keys/test/ut/key_client_delete_test_live.cpp @@ -89,7 +89,7 @@ TEST_F(KeyVaultClientTest, DeleteKeyOperationPoll) // Expected not completed operation EXPECT_EQ( static_cast::type>( - pollResponse->GetStatusCode()), + pollResponse.GetStatusCode()), 404); } } diff --git a/sdk/storage/azure-storage-blobs/inc/azure/storage/blobs/blob_responses.hpp b/sdk/storage/azure-storage-blobs/inc/azure/storage/blobs/blob_responses.hpp index 3be8e5f78..157edef1a 100644 --- a/sdk/storage/azure-storage-blobs/inc/azure/storage/blobs/blob_responses.hpp +++ b/sdk/storage/azure-storage-blobs/inc/azure/storage/blobs/blob_responses.hpp @@ -87,7 +87,10 @@ namespace Azure { namespace Storage { namespace Blobs { * @return A reference to an #Azure::Core::Http::RawResponse. * @note Does not give up ownership of the RawResponse. */ - Azure::Core::Http::RawResponse const& GetRawResponse() const override { return *m_rawResponse; } + Azure::Core::Http::RawResponse const& GetRawResponseInternal() const override + { + return *m_rawResponse; + } StartCopyBlobOperation() = default; diff --git a/sdk/storage/azure-storage-blobs/src/blob_responses.cpp b/sdk/storage/azure-storage-blobs/src/blob_responses.cpp index 7a8a44dad..c61399d50 100644 --- a/sdk/storage/azure-storage-blobs/src/blob_responses.cpp +++ b/sdk/storage/azure-storage-blobs/src/blob_responses.cpp @@ -8,9 +8,8 @@ namespace Azure { namespace Storage { namespace Blobs { std::unique_ptr StartCopyBlobOperation::PollInternal( - Azure::Core::Context& context) + Azure::Core::Context&) { - (void)context; auto response = m_blobClient->GetProperties(); if (!response->CopyStatus.HasValue()) @@ -39,12 +38,12 @@ namespace Azure { namespace Storage { namespace Blobs { { while (true) { - auto rawResponse = PollInternal(context); + auto rawResponse = Poll(context); if (m_status == Azure::Core::OperationStatus::Succeeded) { return Azure::Response( - m_pollResult, std::move(rawResponse)); + m_pollResult, std::make_unique(rawResponse)); } else if (m_status == Azure::Core::OperationStatus::Failed) { diff --git a/sdk/storage/azure-storage-files-shares/inc/azure/storage/files/shares/share_responses.hpp b/sdk/storage/azure-storage-files-shares/inc/azure/storage/files/shares/share_responses.hpp index 30c94d46b..811b29fa0 100644 --- a/sdk/storage/azure-storage-files-shares/inc/azure/storage/files/shares/share_responses.hpp +++ b/sdk/storage/azure-storage-files-shares/inc/azure/storage/files/shares/share_responses.hpp @@ -203,7 +203,10 @@ namespace Azure { namespace Storage { namespace Files { namespace Shares { * @return A reference to an #Azure::Core::Http::RawResponse. * @note Does not give up ownership of the RawResponse. */ - Azure::Core::Http::RawResponse const& GetRawResponse() const override { return *m_rawResponse; } + Azure::Core::Http::RawResponse const& GetRawResponseInternal() const override + { + return *m_rawResponse; + } StartCopyShareFileOperation() = default; @@ -227,7 +230,6 @@ namespace Azure { namespace Storage { namespace Files { namespace Shares { std::chrono::milliseconds period, Azure::Core::Context& context) override; - std::unique_ptr m_rawResponse; std::shared_ptr m_fileClient; Models::GetShareFilePropertiesResult m_pollResult; diff --git a/sdk/storage/azure-storage-files-shares/src/share_responses.cpp b/sdk/storage/azure-storage-files-shares/src/share_responses.cpp index d766eacce..00df4687b 100644 --- a/sdk/storage/azure-storage-files-shares/src/share_responses.cpp +++ b/sdk/storage/azure-storage-files-shares/src/share_responses.cpp @@ -10,9 +10,8 @@ namespace Azure { namespace Storage { namespace Files { namespace Shares { std::unique_ptr StartCopyShareFileOperation::PollInternal( - Azure::Core::Context& context) + Azure::Core::Context&) { - (void)context; auto response = m_fileClient->GetProperties(); if (!response->CopyStatus.HasValue()) @@ -40,12 +39,12 @@ namespace Azure { namespace Storage { namespace Files { namespace Shares { { while (true) { - auto rawResponse = PollInternal(context); + auto rawResponse = Poll(context); if (m_status == Azure::Core::OperationStatus::Succeeded) { return Azure::Response( - m_pollResult, std::move(rawResponse)); + m_pollResult, std::make_unique(rawResponse)); } else if (m_status == Azure::Core::OperationStatus::Failed) {