From 4db9a563e75bd7f67ffd3f2a895577ecda122ef1 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Tue, 6 Apr 2021 20:23:34 -0700 Subject: [PATCH] Removed `Azure::Core::Context::HasKey()` and add `TryGetValue()` instead (#2037) ~@vhvb1989 This change is blocked until after the retry policy implementation has been updated. Let me know once you have that PR available.~ Fixes https://github.com/Azure/azure-sdk-for-cpp/issues/2036 --- sdk/core/azure-core/CHANGELOG.md | 15 +- .../azure-core/inc/azure/core/context.hpp | 38 +--- sdk/core/azure-core/src/http/retry_policy.cpp | 21 +- sdk/core/azure-core/test/ut/context.cpp | 197 ++++++++++++------ .../azure-core/test/ut/global_context.cpp | 4 +- sdk/core/azure-core/test/ut/policy.cpp | 9 +- .../storage_switch_to_secondary_policy.cpp | 5 +- 7 files changed, 172 insertions(+), 117 deletions(-) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index 0b7a77bf7..725b3a4f0 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -5,28 +5,29 @@ ### New Features - Added `Azure::Core::Url::GetScheme()`. +- Added `Azure::Core::Context::TryGetValue()`. ### Breaking Changes - Simplified the `Response` API surface to expose two public fields with direct access: `T Value` and a `unique_ptr` to an `Azure::Core::Http::RawResponse`. - Removed from `Azure::Core::Http::Request`: - - `SetUploadChunkSize`. - - `GetHTTPMessagePreBody`. - - `GetUploadChunkSize`. + - `SetUploadChunkSize()`. + - `GetHTTPMessagePreBody()`. + - `GetUploadChunkSize()`. - Removed from `Azure::Core::Http::RawResponse`: - `SetHeader(std::string const& header)` - `SetHeader(uint8_t const* const first, uint8_t const* const last)`. - - `GetMajorVersion`. - - `GetMinorVersion`. + - `GetMajorVersion()`. + - `GetMinorVersion()`. - Renamed `Azure::Nullable::GetValue()` to `Value()`. - Changes to `Azure::Core::Context`: - - Renamed `Get()` to `GetValue()`. + - Removed `Get()` and `HasKey()` in favor of a new method `TryGetValue()`. - Changed input parameter type of `WithDeadline()` to `Azure::DateTime`. - Removed `Azure::Core::PackageVersion`. - Removed from `Azure::Core::Http::Policies` namespace: `HttpPolicyOrder`, `TransportPolicy`, `RetryPolicy`, `RequestIdPolicy`, `TelemetryPolicy`, `BearerTokenAuthenticationPolicy`, `LogPolicy`. - Renamed `Azure::Core::Http::RawResponse::GetBodyStream()` to `ExtractBodyStream()`. - Removed `AppendQueryParameters()`, `GetUrlWithoutQuery()` and `GetUrlAuthorityWithScheme()` from `Azure::Core::Url`. -- Changed the `Azure::Core::Http::HttpMethod` regular enum into an extensible enum class and removed the `HttpMethodToString` helper method. +- Changed the `Azure::Core::Http::HttpMethod` regular enum into an extensible enum class and removed the `HttpMethodToString()` helper method. - Removed `Azure::Core::Http::Request::GetHeadersAsString()`. - Introduced `Azure::Core::Context::Key` class which takes place of `std::string` used for `Azure::Core::Context` keys previously. diff --git a/sdk/core/azure-core/inc/azure/core/context.hpp b/sdk/core/azure-core/inc/azure/core/context.hpp index 8065f4ab6..557b7e315 100644 --- a/sdk/core/azure-core/inc/azure/core/context.hpp +++ b/sdk/core/azure-core/inc/azure/core/context.hpp @@ -157,15 +157,19 @@ namespace Azure { namespace Core { DateTime GetDeadline() const; /** - * @brief Get a value associated with a \p key parameter within this context or the branch of - * contexts this context belongs to. + * @brief Try to get a value associated with a \p key parameter within this context or the + * branch of contexts this context belongs to. * * @param key A key associated with a context to find. + * @param outputValue A reference to the value corresponding to the \p key to be set, if found + * within the context tree. * - * @return A value associated with the context found; an empty value if a specific value can't - * be found. + * @return If found, returns `true`, with \p outputValue set to the value associated with the + * key found; otherwise returns `false`. + * + * @remark The \p outputValue is left unmodified it the \p key is not found. */ - template const T& GetValue(Key const& key) const + template bool TryGetValue(Key const& key, T& outputValue) const { for (auto ptr = m_contextSharedState; ptr; ptr = ptr->Parent) { @@ -176,29 +180,7 @@ namespace Azure { namespace Core { // type mismatch std::abort(); } - return *reinterpret_cast(ptr->Value.get()); - } - } - std::abort(); - // It should be expected that keys may not exist - // That implies we return T* and NOT a T& - } - - /** - * @brief Check whether the context has a key matching \p key parameter in it itself, or in the - * branch the context belongs to. - * - * @param key A key associated with a context to find. - * - * @return `true` if this context, or the tree branch this context belongs to has a \p key - * associated with it. `false` otherwise. - */ - bool HasKey(Key const& key) const - { - for (auto ptr = m_contextSharedState; ptr; ptr = ptr->Parent) - { - if (ptr->Key == key) - { + outputValue = *reinterpret_cast(ptr->Value.get()); return true; } } diff --git a/sdk/core/azure-core/src/http/retry_policy.cpp b/sdk/core/azure-core/src/http/retry_policy.cpp index 16bbaf1b1..9838b1e7f 100644 --- a/sdk/core/azure-core/src/http/retry_policy.cpp +++ b/sdk/core/azure-core/src/http/retry_policy.cpp @@ -93,16 +93,17 @@ Context::Key const RetryKey; int32_t RetryPolicy::GetRetryCount(Context const& context) { - if (!context.HasKey(RetryKey)) - { - // Context with no data abut sending request with retry policy = -1 - // First try = 0 - // Second try = 1 - // third try = 2 - // ... - return -1; - } - return *context.GetValue(RetryKey); + int32_t number = -1; + + // Context with no data abut sending request with retry policy = -1 + // First try = 0 + // Second try = 1 + // third try = 2 + // ... + int32_t* ptr = &number; + context.TryGetValue(RetryKey, ptr); + + return *ptr; } std::unique_ptr RetryPolicy::Send( diff --git a/sdk/core/azure-core/test/ut/context.cpp b/sdk/core/azure-core/test/ut/context.cpp index 73e6e7b30..ff6cc4c22 100644 --- a/sdk/core/azure-core/test/ut/context.cpp +++ b/sdk/core/azure-core/test/ut/context.cpp @@ -18,7 +18,9 @@ TEST(Context, Basic) Context context; Context::Key const key; - EXPECT_FALSE(context.HasKey(key)); + int placeholder = -15; + EXPECT_FALSE(context.TryGetValue(key, placeholder)); + EXPECT_EQ(placeholder, -15); } TEST(Context, BasicBool) @@ -28,8 +30,20 @@ TEST(Context, BasicBool) // New context from previous auto c2 = context.WithValue(key, true); - auto& value = c2.GetValue(key); + bool value; + EXPECT_TRUE(c2.TryGetValue(key, value)); EXPECT_TRUE(value == true); + + Context::Key const anotherKey; + auto c3 = c2.WithValue(anotherKey, std::make_shared(true)); + + std::shared_ptr sharedPtrBool; + EXPECT_FALSE(c2.TryGetValue>(anotherKey, sharedPtrBool)); + EXPECT_FALSE(sharedPtrBool); + + EXPECT_TRUE(c3.TryGetValue(anotherKey, sharedPtrBool)); + EXPECT_TRUE(sharedPtrBool); + EXPECT_TRUE(*sharedPtrBool); } TEST(Context, BasicInt) @@ -39,7 +53,8 @@ TEST(Context, BasicInt) // New context from previous auto c2 = context.WithValue(key, 123); - auto& value = c2.GetValue(key); + int value; + EXPECT_TRUE(c2.TryGetValue(key, value)); EXPECT_TRUE(value == 123); } @@ -52,7 +67,8 @@ TEST(Context, BasicStdString) // New context from previous auto c2 = context.WithValue(key, s); - auto& value = c2.GetValue(key); + std::string value; + EXPECT_TRUE(c2.TryGetValue(key, value)); EXPECT_TRUE(value == s); } @@ -66,7 +82,8 @@ TEST(Context, BasicChar) // New context from previous auto c2 = context.WithValue(key, str); - auto& value = c2.GetValue(key); + const char* value; + EXPECT_TRUE(c2.TryGetValue(key, value)); EXPECT_TRUE(value == s); EXPECT_TRUE(value == str); } @@ -91,10 +108,15 @@ TEST(Context, NestedIsCancelled) Context context; Context::Key const key; - auto c2 = context.WithValue(key, "Value"); + std::string actualValue = "Value"; + auto c2 = context.WithValue(key, actualValue); EXPECT_FALSE(c2.IsCancelled()); - EXPECT_TRUE(c2.HasKey(key)); - EXPECT_FALSE(context.HasKey(key)); + std::string value = "a"; + EXPECT_TRUE(c2.TryGetValue(key, value)); + EXPECT_EQ(value, "Value"); + value = "temp"; + EXPECT_FALSE(context.TryGetValue(key, value)); + EXPECT_EQ(value, "temp"); auto c3 = context.WithDeadline(deadline); EXPECT_FALSE(context.IsCancelled()); @@ -106,9 +128,11 @@ TEST(Context, NestedIsCancelled) EXPECT_FALSE(c2.IsCancelled()); EXPECT_TRUE(c3.IsCancelled()); - EXPECT_TRUE(c2.HasKey(key)); - EXPECT_FALSE(context.HasKey(key)); - EXPECT_FALSE(c3.HasKey(key)); + value = "b"; + EXPECT_TRUE(c2.TryGetValue(key, value)); + EXPECT_EQ(value, "Value"); + EXPECT_FALSE(context.TryGetValue(key, value)); + EXPECT_FALSE(c3.TryGetValue(key, value)); } TEST(Context, CancelWithValue) @@ -116,18 +140,22 @@ TEST(Context, CancelWithValue) Context context; Context::Key const key; - auto c2 = context.WithValue(key, "Value"); + std::string actualValue = "Value"; + auto c2 = context.WithValue(key, actualValue); EXPECT_FALSE(context.IsCancelled()); EXPECT_FALSE(c2.IsCancelled()); - EXPECT_TRUE(c2.HasKey(key)); - EXPECT_FALSE(context.HasKey(key)); + std::string value = "a"; + EXPECT_TRUE(c2.TryGetValue(key, value)); + EXPECT_EQ(value, "Value"); + EXPECT_FALSE(context.TryGetValue(key, value)); c2.Cancel(); EXPECT_TRUE(c2.IsCancelled()); EXPECT_FALSE(context.IsCancelled()); - EXPECT_TRUE(c2.HasKey(key)); - EXPECT_FALSE(context.HasKey(key)); + EXPECT_TRUE(c2.TryGetValue(key, value)); + EXPECT_EQ(value, "Value"); + EXPECT_FALSE(context.TryGetValue(key, value)); } TEST(Context, ThrowIfCancelled) @@ -162,13 +190,20 @@ TEST(Context, Chain) auto c7 = c6.WithValue(key7, "7"); auto finalContext = c7.WithValue(keyFinal, "Final"); - auto& valueT2 = finalContext.GetValue(key2); - auto& valueT3 = finalContext.GetValue(key3); - auto& valueT4 = finalContext.GetValue(key4); - auto& valueT5 = finalContext.GetValue(key5); - auto& valueT6 = finalContext.GetValue(key6); - auto& valueT7 = finalContext.GetValue(key7); - auto& valueT8 = finalContext.GetValue(keyFinal); + int valueT2; + EXPECT_TRUE(finalContext.TryGetValue(key2, valueT2)); + int valueT3; + EXPECT_TRUE(finalContext.TryGetValue(key3, valueT3)); + int valueT4; + EXPECT_TRUE(finalContext.TryGetValue(key4, valueT4)); + const char* valueT5; + EXPECT_TRUE(finalContext.TryGetValue(key5, valueT5)); + const char* valueT6; + EXPECT_TRUE(finalContext.TryGetValue(key6, valueT6)); + const char* valueT7; + EXPECT_TRUE(finalContext.TryGetValue(key7, valueT7)); + const char* valueT8; + EXPECT_TRUE(finalContext.TryGetValue(keyFinal, valueT8)); EXPECT_TRUE(valueT2 == 123); EXPECT_TRUE(valueT3 == 456); @@ -188,8 +223,10 @@ TEST(Context, MatchingKeys) auto c2 = context.WithValue(key, 123); auto c3 = c2.WithValue(key, 456); - auto& valueT2 = c2.GetValue(key); - auto& valueT3 = c3.GetValue(key); + int valueT2; + EXPECT_TRUE(c2.TryGetValue(key, valueT2)); + int valueT3; + EXPECT_TRUE(c3.TryGetValue(key, valueT3)); EXPECT_TRUE(valueT2 == 123); EXPECT_TRUE(valueT3 == 456); @@ -204,21 +241,58 @@ TEST(Context, InstanceValue) { Context::Key const key; auto contextP = Context::GetApplicationContext().WithValue(key, SomeStructForContext()); - auto& contextValueRef = contextP.GetValue(key); + SomeStructForContext contextValueRef; + EXPECT_TRUE(contextP.TryGetValue(key, contextValueRef)); EXPECT_EQ(contextValueRef.someField, 12345); } -TEST(Context, UniquePtr) +TEST(Context, Ptr) { Context::Key const key; - auto contextP - = Context::GetApplicationContext().WithValue(key, std::make_unique()); - auto& contextValueRef = contextP.GetValue>(key); + SomeStructForContext value; + auto contextP = Context::GetApplicationContext().WithValue(key, &value); + + SomeStructForContext* contextValueRef; + EXPECT_TRUE(contextP.TryGetValue(key, contextValueRef)); EXPECT_EQ(contextValueRef->someField, 12345); + EXPECT_EQ(&value, contextValueRef); +} + +TEST(Context, NestedClassPtr) +{ + class TestClass { + private: + int* m_instanceCount; + + public: + TestClass(int* instanceCount) : m_instanceCount(instanceCount) { ++(*m_instanceCount); } + ~TestClass() { --(*m_instanceCount); } + }; + + int instanceCount = 0; + { + auto sharedPtr = std::make_shared(&instanceCount); + EXPECT_EQ(sharedPtr.use_count(), 1); + + Context::Key const key; + + auto context = Context::GetApplicationContext().WithValue(key, sharedPtr); + EXPECT_EQ(sharedPtr.use_count(), 2); + + std::shared_ptr foundPtr; + EXPECT_TRUE(context.TryGetValue(key, foundPtr)); + EXPECT_EQ(foundPtr.get(), sharedPtr.get()); + EXPECT_EQ(instanceCount, 1); + EXPECT_EQ(sharedPtr.use_count(), 3); + } + + // Verify that context calls the destructor of shared_ptr it is holding + EXPECT_EQ(instanceCount, 0); } TEST(Context, HeapLinkIntegrity) { + std::string value = "z"; Context::Key const a; Context::Key const b; Context::Key const c; @@ -228,51 +302,52 @@ TEST(Context, HeapLinkIntegrity) { Context root; auto firstGeneration = root.WithValue(a, std::string("a")); - EXPECT_TRUE(firstGeneration.HasKey(a)); + EXPECT_TRUE(firstGeneration.TryGetValue(a, value)); + EXPECT_EQ(value, "a"); auto secondGeneration = firstGeneration.WithValue(b, std::string("b")); - EXPECT_TRUE(secondGeneration.HasKey(a)); - EXPECT_EQ("a", secondGeneration.GetValue(a)); - EXPECT_TRUE(secondGeneration.HasKey(b)); - EXPECT_EQ("b", secondGeneration.GetValue(b)); + EXPECT_TRUE(secondGeneration.TryGetValue(a, value)); + EXPECT_EQ(value, "a"); + EXPECT_TRUE(secondGeneration.TryGetValue(b, value)); + EXPECT_EQ(value, "b"); // Now overide the generation secondGeneration = secondGeneration.WithValue(c, std::string("c")); - EXPECT_TRUE( - secondGeneration.HasKey(a)); // Still know about first gen - The link is still in heap - EXPECT_EQ("a", secondGeneration.GetValue(a)); - EXPECT_TRUE(secondGeneration.HasKey( - b)); // Still knows about the initial second gen, as a shared_ptr, it is still on heap - EXPECT_EQ("b", secondGeneration.GetValue(b)); - EXPECT_TRUE(secondGeneration.HasKey(c)); // Check new value - EXPECT_EQ("c", secondGeneration.GetValue(c)); + EXPECT_TRUE(secondGeneration.TryGetValue(a, value)); + EXPECT_EQ(value, "a"); // Still know about first gen - The link is still in heap + EXPECT_TRUE(secondGeneration.TryGetValue(b, value)); + EXPECT_EQ( + value, + "b"); // Still knows about the initial second gen, as a shared_ptr, it is still on heap + EXPECT_TRUE(secondGeneration.TryGetValue(c, value)); + EXPECT_EQ(value, "c"); // Check new value // One more override secondGeneration = secondGeneration.WithValue(d, std::string("d")); - EXPECT_TRUE(secondGeneration.HasKey(a)); - EXPECT_EQ("a", secondGeneration.GetValue(a)); - EXPECT_TRUE(secondGeneration.HasKey(b)); - EXPECT_EQ("b", secondGeneration.GetValue(b)); - EXPECT_TRUE(secondGeneration.HasKey(c)); - EXPECT_EQ("c", secondGeneration.GetValue(c)); - EXPECT_TRUE(secondGeneration.HasKey(d)); - EXPECT_EQ("d", secondGeneration.GetValue(d)); + EXPECT_TRUE(secondGeneration.TryGetValue(a, value)); + EXPECT_EQ(value, "a"); + EXPECT_TRUE(secondGeneration.TryGetValue(b, value)); + EXPECT_EQ(value, "b"); + EXPECT_TRUE(secondGeneration.TryGetValue(c, value)); + EXPECT_EQ(value, "c"); + EXPECT_TRUE(secondGeneration.TryGetValue(d, value)); + EXPECT_EQ(value, "d"); // New Gen thirdGeneration = secondGeneration.WithValue(e, std::string("e")); } // Went out of scope, root and secondGeneration are destroyed. but should remain in heap for the // third-generation since the previous geneations are still alive inside his heart <3. - EXPECT_TRUE(thirdGeneration.HasKey(a)); - EXPECT_EQ("a", thirdGeneration.GetValue(a)); - EXPECT_TRUE(thirdGeneration.HasKey(b)); - EXPECT_EQ("b", thirdGeneration.GetValue(b)); - EXPECT_TRUE(thirdGeneration.HasKey(c)); - EXPECT_EQ("c", thirdGeneration.GetValue(c)); - EXPECT_TRUE(thirdGeneration.HasKey(d)); - EXPECT_EQ("d", thirdGeneration.GetValue(d)); - EXPECT_TRUE(thirdGeneration.HasKey(e)); - EXPECT_EQ("e", thirdGeneration.GetValue(e)); + EXPECT_TRUE(thirdGeneration.TryGetValue(a, value)); + EXPECT_EQ(value, "a"); + EXPECT_TRUE(thirdGeneration.TryGetValue(b, value)); + EXPECT_EQ(value, "b"); + EXPECT_TRUE(thirdGeneration.TryGetValue(c, value)); + EXPECT_EQ(value, "c"); + EXPECT_TRUE(thirdGeneration.TryGetValue(d, value)); + EXPECT_EQ(value, "d"); + EXPECT_TRUE(thirdGeneration.TryGetValue(e, value)); + EXPECT_EQ(value, "e"); } Context::Key const GlobalKey1; diff --git a/sdk/core/azure-core/test/ut/global_context.cpp b/sdk/core/azure-core/test/ut/global_context.cpp index 59b27e709..f86a92f71 100644 --- a/sdk/core/azure-core/test/ut/global_context.cpp +++ b/sdk/core/azure-core/test/ut/global_context.cpp @@ -24,7 +24,9 @@ TEST(Context, ApplicationContext) { Context appContext = Context::GetApplicationContext(); - EXPECT_FALSE(appContext.HasKey(Context::Key())); + int value = 42; + EXPECT_FALSE(appContext.TryGetValue(Context::Key(), value)); + EXPECT_EQ(value, 42); auto duration = std::chrono::milliseconds(250); EXPECT_FALSE(appContext.IsCancelled()); diff --git a/sdk/core/azure-core/test/ut/policy.cpp b/sdk/core/azure-core/test/ut/policy.cpp index 7dc37a04a..80259653d 100644 --- a/sdk/core/azure-core/test/ut/policy.cpp +++ b/sdk/core/azure-core/test/ut/policy.cpp @@ -59,12 +59,9 @@ struct TestContextTreeIntegrity : public Azure::Core::Http::Policies::HttpPolicy Azure::Core::Http::Policies::NextHttpPolicy nextHttpPolicy, Azure::Core::Context const& ctx) const override { - EXPECT_TRUE(ctx.HasKey(TheKey)); - if (ctx.HasKey(TheKey)) - { - auto value = ctx.GetValue(TheKey); - EXPECT_EQ("TheValue", value); - } + std::string valueHolder; + EXPECT_TRUE(ctx.TryGetValue(TheKey, valueHolder)); + EXPECT_EQ("TheValue", valueHolder); return nextHttpPolicy.Send(request, ctx); } }; diff --git a/sdk/storage/azure-storage-common/src/storage_switch_to_secondary_policy.cpp b/sdk/storage/azure-storage-common/src/storage_switch_to_secondary_policy.cpp index f1882b98c..ca0494989 100644 --- a/sdk/storage/azure-storage-common/src/storage_switch_to_secondary_policy.cpp +++ b/sdk/storage/azure-storage-common/src/storage_switch_to_secondary_policy.cpp @@ -13,10 +13,7 @@ namespace Azure { namespace Storage { namespace _internal { const Azure::Core::Context& ctx) const { std::shared_ptr replicaStatus; - if (ctx.HasKey(SecondaryHostReplicaStatusKey)) - { - replicaStatus = ctx.GetValue>(SecondaryHostReplicaStatusKey); - } + ctx.TryGetValue(SecondaryHostReplicaStatusKey, replicaStatus); bool considerSecondary = (request.GetMethod() == Azure::Core::Http::HttpMethod::Get || request.GetMethod() == Azure::Core::Http::HttpMethod::Head)