From 10d244e511dc62a527885c3d4eec09e5e267136e Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Wed, 5 May 2021 16:14:31 -0700 Subject: [PATCH] LRO operation resume token - following factory method design (#2195) * Add tests for resume token pattern * update deleteKeyOperation --- sdk/core/azure-core/test/ut/operation.cpp | 19 +++++++++ .../azure-core/test/ut/operation_test.hpp | 18 ++++++++- .../azure-security-keyvault-keys/CHANGELOG.md | 2 + .../keyvault/keys/delete_key_operation.hpp | 39 +++++++++++++++---- .../inc/azure/keyvault/keys/key_client.hpp | 29 -------------- .../keys/recover_deleted_key_operation.hpp | 38 ++++++++++++++---- .../src/delete_key_operation.cpp | 15 +++++-- .../src/key_client.cpp | 22 +---------- .../src/recover_deleted_key_operation.cpp | 17 +++++--- .../test/ut/key_client_delete_test_live.cpp | 12 ++++-- 10 files changed, 133 insertions(+), 78 deletions(-) diff --git a/sdk/core/azure-core/test/ut/operation.cpp b/sdk/core/azure-core/test/ut/operation.cpp index fc6898c03..40667587e 100644 --- a/sdk/core/azure-core/test/ut/operation.cpp +++ b/sdk/core/azure-core/test/ut/operation.cpp @@ -88,3 +88,22 @@ TEST(Operation, Status) EXPECT_THROW(operation.Value(), std::runtime_error); EXPECT_EQ(operation.Status(), OperationStatus::Cancelled); } + +TEST(Operation, ResumeToken) +{ + StringClient client; + std::string token; + { + auto operation = client.StartStringUpdate(); + token = operation.GetResumeToken(); + } + { + for (auto resumedOperation = StringOperation::CreateFromResumeToken(token, client); + !resumedOperation.IsDone(); + resumedOperation.Poll()) + { + EXPECT_FALSE(resumedOperation.HasValue()); + EXPECT_THROW(resumedOperation.Value(), std::runtime_error); + } + } +} diff --git a/sdk/core/azure-core/test/ut/operation_test.hpp b/sdk/core/azure-core/test/ut/operation_test.hpp index 87fa39fd7..9af91e45c 100644 --- a/sdk/core/azure-core/test/ut/operation_test.hpp +++ b/sdk/core/azure-core/test/ut/operation_test.hpp @@ -21,8 +21,6 @@ namespace Azure { namespace Core { namespace Test { private: std::string m_operationToken; std::string m_value; - - private: int m_count = 0; private: @@ -60,7 +58,14 @@ namespace Azure { namespace Core { namespace Test { return Response(m_value, std::make_unique(*m_rawResponse)); } + StringOperation(std::string const& resumeToken, StringClient const&) + : m_operationToken(resumeToken) + { + } + public: + StringOperation() = default; + Azure::Core::Http::RawResponse const& GetRawResponseInternal() const override { return *m_rawResponse; @@ -81,6 +86,15 @@ namespace Azure { namespace Core { namespace Test { // This is a helper method to allow testing of the underlying operation behaviors // ClientOperations would not expose a way to control status void SetOperationStatus(OperationStatus status) { m_status = status; } + + static StringOperation CreateFromResumeToken( + std::string const& resumeToken, + StringClient const& client) + { + StringOperation operation(resumeToken, client); + operation.Poll(); + return operation; + } }; class StringClient { diff --git a/sdk/keyvault/azure-security-keyvault-keys/CHANGELOG.md b/sdk/keyvault/azure-security-keyvault-keys/CHANGELOG.md index b321dc760..b5909ba5c 100644 --- a/sdk/keyvault/azure-security-keyvault-keys/CHANGELOG.md +++ b/sdk/keyvault/azure-security-keyvault-keys/CHANGELOG.md @@ -5,6 +5,7 @@ ### New Features - Added support for importing and deserializing EC and OCT keys. +- Added `CreateFromResumeToken()` to `DeletedKeyOperation` and `RecoverKeyOperation`. ### Breaking Changes @@ -23,6 +24,7 @@ - Changed the returned type for list keys, key versions, and deleted keys from `Response` to `PagedResponse` affecting: - `GetPropertiesOfKeysSinglePage()` and `GetPropertiesOfKeyVersionsSinglePage()` now returns `KeyProperties`. - `GetDeletedKeysSinglePage()` now returns `DeletedKey`. +- Removed `ResumeDeleteKeyOperation()` and `ResumeRecoverKeyOperation()`. ### Bug Fixes 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 2fe96b45a..45f25740f 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 @@ -9,13 +9,12 @@ #pragma once +#include #include #include #include #include -#include - #include "azure/keyvault/keys/deleted_key.hpp" #include @@ -24,6 +23,8 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { + class KeyClient; + /** * @brief A long running operation to delete a key. * @@ -35,7 +36,7 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { * constructor is private and requires internal components.*/ friend class KeyClient; - std::shared_ptr m_pipeline; + std::shared_ptr m_keyClient; Azure::Security::KeyVault::Keys::DeletedKey m_value; std::string m_continuationToken; @@ -71,13 +72,13 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { * Since C++ doesn't offer `internal` access, we use friends-only instead. */ DeleteKeyOperation( - std::shared_ptr keyvaultPipeline, + std::shared_ptr keyClient, Azure::Response response); DeleteKeyOperation( - std::shared_ptr keyvaultPipeline, - std::string resumeToken) - : m_pipeline(keyvaultPipeline), m_value(DeletedKey(resumeToken)), + std::string resumeToken, + std::shared_ptr keyClient) + : m_keyClient(keyClient), m_value(DeletedKey(resumeToken)), m_continuationToken(std::move(resumeToken)) { } @@ -108,6 +109,30 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { * @return std::string */ std::string GetResumeToken() const override { return m_continuationToken; } + + /** + * @brief Create a #DeleteKeyOperation from the \p resumeToken fetched from another + * `Operation`, updated to the the latest operation status. + * + * @remark After the operation is initialized, it is used to poll the last update from the + * server using the \p context. + * + * @param resumeToken A previously generated token used to resume the polling of the operation. + * @param client A #KeyClient that is used for getting status updates. + * @param context A #Azure::Core::Context controlling the request lifetime. + * @return DeleteKeyOperation + */ + static DeleteKeyOperation CreateFromResumeToken( + std::string const& resumeToken, + Azure::Security::KeyVault::Keys::KeyClient const& client, + Azure::Core::Context const& context = Azure::Core::Context()) + { + + DeleteKeyOperation operation( + resumeToken, std::make_shared(client)); + operation.Poll(context); + return operation; + } }; }}}} // namespace Azure::Security::KeyVault::Keys diff --git a/sdk/keyvault/azure-security-keyvault-keys/inc/azure/keyvault/keys/key_client.hpp b/sdk/keyvault/azure-security-keyvault-keys/inc/azure/keyvault/keys/key_client.hpp index 290db5eac..e74823176 100644 --- a/sdk/keyvault/azure-security-keyvault-keys/inc/azure/keyvault/keys/key_client.hpp +++ b/sdk/keyvault/azure-security-keyvault-keys/inc/azure/keyvault/keys/key_client.hpp @@ -40,10 +40,6 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { */ struct GetKeyOptions { - /** - * @brief Context for cancelling long running operations. - */ - Azure::Core::Context Context; /** * @brief Specify the key version to get. */ @@ -228,18 +224,6 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { std::string const& name, Azure::Core::Context const& context = Azure::Core::Context()) const; - /** - * @brief Construct a #DeletedKeyOperation from a resume token previously generated by calling - * #DeleteKeyOperation::GetResumeToken after a call to #StartDeleteKey(). - * - * @param resumeToken A generated token from a DeletedKeyOperation. - * @param context A cancellation token controlling the request lifetime. - * @return Azure::Security::KeyVault::Keys::DeleteKeyOperation - */ - Azure::Security::KeyVault::Keys::DeleteKeyOperation ResumeDeleteKey( - std::string const& resumeToken, - Azure::Core::Context const& context = Azure::Core::Context()) const; - /** * @brief Gets the public part of a deleted key. * @@ -308,19 +292,6 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { std::string const& name, Azure::Core::Context const& context = Azure::Core::Context()) const; - /** - * @brief Construct a #RecoverDeletedKeyOperation from a resume token previously generated by - * calling #RecoverDeletedKeyOperation::GetResumeToken after a call to - * #StartRecoverDeletedKey(). - * - * @param resumeToken A generated token from a StartRecoverDeletedKey. - * @param context A cancellation token controlling the request lifetime. - * @return Azure::Security::KeyVault::Keys::RecoverDeletedKeyOperation - */ - Azure::Security::KeyVault::Keys::RecoverDeletedKeyOperation ResumeRecoverDeletedKey( - std::string const& resumeToken, - Azure::Core::Context const& context = Azure::Core::Context()) const; - /** * @brief The update key operation changes specified attributes of a stored key and can be * applied to any key type and key version stored in Azure Key Vault. diff --git a/sdk/keyvault/azure-security-keyvault-keys/inc/azure/keyvault/keys/recover_deleted_key_operation.hpp b/sdk/keyvault/azure-security-keyvault-keys/inc/azure/keyvault/keys/recover_deleted_key_operation.hpp index a3a862a87..7e8ced9ce 100644 --- a/sdk/keyvault/azure-security-keyvault-keys/inc/azure/keyvault/keys/recover_deleted_key_operation.hpp +++ b/sdk/keyvault/azure-security-keyvault-keys/inc/azure/keyvault/keys/recover_deleted_key_operation.hpp @@ -15,8 +15,6 @@ #include #include -#include - #include "azure/keyvault/keys/key_vault_key.hpp" #include @@ -25,6 +23,8 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { + class KeyClient; + /** * @brief A long running operation to recover a key. * @@ -35,7 +35,7 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { * The constructor is private and requires internal components.*/ friend class KeyClient; - std::shared_ptr m_pipeline; + std::shared_ptr m_keyClient; Azure::Security::KeyVault::Keys::KeyVaultKey m_value; std::string m_continuationToken; @@ -68,13 +68,13 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { * Since C++ doesn't offer `internal` access, we use friends-only instead. */ RecoverDeletedKeyOperation( - std::shared_ptr keyvaultPipeline, + std::shared_ptr keyClient, Azure::Response response); RecoverDeletedKeyOperation( - std::shared_ptr keyvaultPipeline, - std::string resumeToken) - : m_pipeline(keyvaultPipeline), m_value(DeletedKey(resumeToken)), + std::string resumeToken, + std::shared_ptr keyClient) + : m_keyClient(keyClient), m_value(DeletedKey(resumeToken)), m_continuationToken(std::move(resumeToken)) { } @@ -105,6 +105,30 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys { * @return std::string */ std::string GetResumeToken() const override { return m_continuationToken; } + + /** + * @brief Create a #RecoverDeletedKeyOperation from the \p resumeToken fetched from another + * `Operation`, updated to the the latest operation status. + * + * @remark After the operation is initialized, it is used to poll the last update from the + * server using the \p context. + * + * @param resumeToken A previously generated token used to resume the polling of the operation. + * @param client A #KeyClient that is used for getting status updates. + * @param context A #Azure::Core::Context controlling the request lifetime. + * @return DeleteKeyOperation + */ + static RecoverDeletedKeyOperation CreateFromResumeToken( + std::string const& resumeToken, + Azure::Security::KeyVault::Keys::KeyClient const& client, + Azure::Core::Context const& context = Azure::Core::Context()) + { + + RecoverDeletedKeyOperation operation( + resumeToken, std::make_shared(client)); + operation.Poll(context); + return operation; + } }; }}}} // namespace Azure::Security::KeyVault::Keys 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 6799d7ee7..321923e35 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 @@ -8,6 +8,7 @@ #include "azure/keyvault/keys/delete_key_operation.hpp" #include "azure/keyvault/keys/details/key_constants.hpp" #include "azure/keyvault/keys/details/key_serializers.hpp" +#include "azure/keyvault/keys/key_client.hpp" using namespace Azure::Security::KeyVault::Keys; using namespace Azure::Security::KeyVault; @@ -19,8 +20,14 @@ Azure::Security::KeyVault::Keys::DeleteKeyOperation::PollInternal( std::unique_ptr rawResponse; if (!IsDone()) { - rawResponse = m_pipeline->Send( - context, Azure::Core::Http::HttpMethod::Get, {_detail::DeletedKeysPath, m_value.Name()}); + try + { + rawResponse = m_keyClient->GetDeletedKey(m_value.Name(), context).RawResponse; + } + catch (Azure::Core::RequestFailedException& error) + { + rawResponse = std::move(error.RawResponse); + } switch (rawResponse->GetStatusCode()) { @@ -53,9 +60,9 @@ Azure::Security::KeyVault::Keys::DeleteKeyOperation::PollInternal( } Azure::Security::KeyVault::Keys::DeleteKeyOperation::DeleteKeyOperation( - std::shared_ptr keyvaultPipeline, + std::shared_ptr keyClient, Azure::Response response) - : m_pipeline(keyvaultPipeline) + : m_keyClient(keyClient) { // The response becomes useless and the value and rawResponse are now owned by the // DeleteKeyOperation. This is fine because the DeleteKeyOperation is what the delete key api diff --git a/sdk/keyvault/azure-security-keyvault-keys/src/key_client.cpp b/sdk/keyvault/azure-security-keyvault-keys/src/key_client.cpp index 9af5d4773..c5f4c7055 100644 --- a/sdk/keyvault/azure-security-keyvault-keys/src/key_client.cpp +++ b/sdk/keyvault/azure-security-keyvault-keys/src/key_client.cpp @@ -205,7 +205,7 @@ Azure::Security::KeyVault::Keys::DeleteKeyOperation KeyClient::StartDeleteKey( Azure::Core::Context const& context) const { return Azure::Security::KeyVault::Keys::DeleteKeyOperation( - m_pipeline, + std::make_shared(*this), m_pipeline->SendRequest( context, Azure::Core::Http::HttpMethod::Delete, @@ -215,30 +215,12 @@ Azure::Security::KeyVault::Keys::DeleteKeyOperation KeyClient::StartDeleteKey( {_detail::KeysPath, name})); } -Azure::Security::KeyVault::Keys::DeleteKeyOperation KeyClient::ResumeDeleteKey( - std::string const& resumeToken, - Azure::Core::Context const& context) const -{ - Azure::Security::KeyVault::Keys::DeleteKeyOperation operation(m_pipeline, resumeToken); - operation.Poll(context); - return operation; -} - -Azure::Security::KeyVault::Keys::RecoverDeletedKeyOperation KeyClient::ResumeRecoverDeletedKey( - std::string const& resumeToken, - Azure::Core::Context const& context) const -{ - Azure::Security::KeyVault::Keys::RecoverDeletedKeyOperation operation(m_pipeline, resumeToken); - operation.Poll(context); - return operation; -} - Azure::Security::KeyVault::Keys::RecoverDeletedKeyOperation KeyClient::StartRecoverDeletedKey( std::string const& name, Azure::Core::Context const& context) const { return Azure::Security::KeyVault::Keys::RecoverDeletedKeyOperation( - m_pipeline, + std::make_shared(*this), m_pipeline->SendRequest( context, Azure::Core::Http::HttpMethod::Post, diff --git a/sdk/keyvault/azure-security-keyvault-keys/src/recover_deleted_key_operation.cpp b/sdk/keyvault/azure-security-keyvault-keys/src/recover_deleted_key_operation.cpp index e8f7a4928..ba8a7e535 100644 --- a/sdk/keyvault/azure-security-keyvault-keys/src/recover_deleted_key_operation.cpp +++ b/sdk/keyvault/azure-security-keyvault-keys/src/recover_deleted_key_operation.cpp @@ -5,6 +5,7 @@ #include "azure/keyvault/keys/details/key_constants.hpp" #include "azure/keyvault/keys/details/key_serializers.hpp" +#include "azure/keyvault/keys/key_client.hpp" #include "azure/keyvault/keys/recover_deleted_key_operation.hpp" using namespace Azure::Security::KeyVault::Keys; @@ -17,10 +18,14 @@ Azure::Security::KeyVault::Keys::RecoverDeletedKeyOperation::PollInternal( std::unique_ptr rawResponse; if (!IsDone()) { - rawResponse = m_pipeline->Send( - context, - Azure::Core::Http::HttpMethod::Get, - {_detail::KeysPath, m_value.Name(), m_value.Properties.Version}); + try + { + rawResponse = m_keyClient->GetKey(m_value.Name(), {}, context).RawResponse; + } + catch (Azure::Core::RequestFailedException& error) + { + rawResponse = std::move(error.RawResponse); + } switch (rawResponse->GetStatusCode()) { @@ -52,9 +57,9 @@ Azure::Security::KeyVault::Keys::RecoverDeletedKeyOperation::PollInternal( } Azure::Security::KeyVault::Keys::RecoverDeletedKeyOperation::RecoverDeletedKeyOperation( - std::shared_ptr keyvaultPipeline, + std::shared_ptr keyClient, Azure::Response response) - : m_pipeline(keyvaultPipeline) + : m_keyClient(keyClient) { // The response becomes useless and the value and rawResponse are now owned by the // DeleteKeyOperation. This is fine because the DeleteKeyOperation is what the delete key api 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 0c6760611..b0e648d6e 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 @@ -337,7 +337,9 @@ TEST_F(KeyVaultClientTest, DeleteOperationResumeToken) } // Resume operation from token { - auto resumeOperation = keyClient.ResumeDeleteKey(resumeToken); + auto resumeOperation + = Azure::Security::KeyVault::Keys::DeleteKeyOperation::CreateFromResumeToken( + resumeToken, keyClient); resumeOperation.PollUntilDone(std::chrono::milliseconds(500)); } { @@ -375,7 +377,9 @@ TEST_F(KeyVaultClientTest, RecoverOperationResumeToken) } // Resume operation from token { - auto resumeOperation = keyClient.ResumeDeleteKey(resumeToken); + auto resumeOperation + = Azure::Security::KeyVault::Keys::DeleteKeyOperation::CreateFromResumeToken( + resumeToken, keyClient); resumeOperation.PollUntilDone(std::chrono::milliseconds(500)); } { @@ -385,7 +389,9 @@ TEST_F(KeyVaultClientTest, RecoverOperationResumeToken) } { // resume from token - auto resumeRecoveryOp = keyClient.ResumeRecoverDeletedKey(resumeToken); + auto resumeRecoveryOp + = Azure::Security::KeyVault::Keys::RecoverDeletedKeyOperation::CreateFromResumeToken( + resumeToken, keyClient); auto keyResponse = resumeRecoveryOp.PollUntilDone(std::chrono::milliseconds(500)); auto key = keyResponse.Value; // Delete again for purging