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
2021-04-06 20:23:34 -07:00 · 2021-04-06 20:23:34 -07:00 · 4db9a563e7
commit 4db9a563e7
parent bb87bd1f19
7 changed files with 172 additions and 117 deletions
--- 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<T>` 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<T>::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.

--- 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 <class T> const T& GetValue(Key const& key) const
+    template <class T> 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<const T*>(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<const T*>(ptr->Value.get());
          return true;
        }
      }
--- 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<int32_t*>(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<int32_t*>(RetryKey, ptr);
+
+  return *ptr;
 }

 std::unique_ptr<RawResponse> RetryPolicy::Send(
--- 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<bool>(key);
+  bool value;
+  EXPECT_TRUE(c2.TryGetValue<bool>(key, value));
  EXPECT_TRUE(value == true);
+
+  Context::Key const anotherKey;
+  auto c3 = c2.WithValue(anotherKey, std::make_shared<bool>(true));
+
+  std::shared_ptr<bool> sharedPtrBool;
+  EXPECT_FALSE(c2.TryGetValue<std::shared_ptr<bool>>(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<int>(key);
+  int value;
+  EXPECT_TRUE(c2.TryGetValue<int>(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<std::string>(key);
+  std::string value;
+  EXPECT_TRUE(c2.TryGetValue<std::string>(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<const char*>(key);
+  const char* value;
+  EXPECT_TRUE(c2.TryGetValue<const char*>(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<std::string>(key, value));
+  EXPECT_EQ(value, "Value");
+  value = "temp";
+  EXPECT_FALSE(context.TryGetValue<std::string>(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<std::string>(key, value));
+  EXPECT_EQ(value, "Value");
+  EXPECT_FALSE(context.TryGetValue<std::string>(key, value));
+  EXPECT_FALSE(c3.TryGetValue<std::string>(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<std::string>(key, value));
+  EXPECT_EQ(value, "Value");
+  EXPECT_FALSE(context.TryGetValue<std::string>(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<std::string>(key, value));
+  EXPECT_EQ(value, "Value");
+  EXPECT_FALSE(context.TryGetValue<std::string>(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<int>(key2);
-  auto& valueT3 = finalContext.GetValue<int>(key3);
-  auto& valueT4 = finalContext.GetValue<int>(key4);
-  auto& valueT5 = finalContext.GetValue<const char*>(key5);
-  auto& valueT6 = finalContext.GetValue<const char*>(key6);
-  auto& valueT7 = finalContext.GetValue<const char*>(key7);
-  auto& valueT8 = finalContext.GetValue<const char*>(keyFinal);
+  int valueT2;
+  EXPECT_TRUE(finalContext.TryGetValue<int>(key2, valueT2));
+  int valueT3;
+  EXPECT_TRUE(finalContext.TryGetValue<int>(key3, valueT3));
+  int valueT4;
+  EXPECT_TRUE(finalContext.TryGetValue<int>(key4, valueT4));
+  const char* valueT5;
+  EXPECT_TRUE(finalContext.TryGetValue<const char*>(key5, valueT5));
+  const char* valueT6;
+  EXPECT_TRUE(finalContext.TryGetValue<const char*>(key6, valueT6));
+  const char* valueT7;
+  EXPECT_TRUE(finalContext.TryGetValue<const char*>(key7, valueT7));
+  const char* valueT8;
+  EXPECT_TRUE(finalContext.TryGetValue<const char*>(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<int>(key);
-  auto& valueT3 = c3.GetValue<int>(key);
+  int valueT2;
+  EXPECT_TRUE(c2.TryGetValue<int>(key, valueT2));
+  int valueT3;
+  EXPECT_TRUE(c3.TryGetValue<int>(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<SomeStructForContext>(key);
+  SomeStructForContext contextValueRef;
+  EXPECT_TRUE(contextP.TryGetValue<SomeStructForContext>(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<SomeStructForContext>());
-  auto& contextValueRef = contextP.GetValue<std::unique_ptr<SomeStructForContext>>(key);
+  SomeStructForContext value;
+  auto contextP = Context::GetApplicationContext().WithValue(key, &value);
+
+  SomeStructForContext* contextValueRef;
+  EXPECT_TRUE(contextP.TryGetValue<SomeStructForContext*>(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<TestClass>(&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<TestClass> 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<std::string>(a, value));
+    EXPECT_EQ(value, "a");

    auto secondGeneration = firstGeneration.WithValue(b, std::string("b"));
-    EXPECT_TRUE(secondGeneration.HasKey(a));
-    EXPECT_EQ("a", secondGeneration.GetValue<std::string>(a));
-    EXPECT_TRUE(secondGeneration.HasKey(b));
-    EXPECT_EQ("b", secondGeneration.GetValue<std::string>(b));
+    EXPECT_TRUE(secondGeneration.TryGetValue<std::string>(a, value));
+    EXPECT_EQ(value, "a");
+    EXPECT_TRUE(secondGeneration.TryGetValue<std::string>(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<std::string>(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<std::string>(b));
-    EXPECT_TRUE(secondGeneration.HasKey(c)); // Check new value
-    EXPECT_EQ("c", secondGeneration.GetValue<std::string>(c));
+    EXPECT_TRUE(secondGeneration.TryGetValue<std::string>(a, value));
+    EXPECT_EQ(value, "a"); // Still know about first gen - The link is still in heap
+    EXPECT_TRUE(secondGeneration.TryGetValue<std::string>(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<std::string>(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<std::string>(a));
-    EXPECT_TRUE(secondGeneration.HasKey(b));
-    EXPECT_EQ("b", secondGeneration.GetValue<std::string>(b));
-    EXPECT_TRUE(secondGeneration.HasKey(c));
-    EXPECT_EQ("c", secondGeneration.GetValue<std::string>(c));
-    EXPECT_TRUE(secondGeneration.HasKey(d));
-    EXPECT_EQ("d", secondGeneration.GetValue<std::string>(d));
+    EXPECT_TRUE(secondGeneration.TryGetValue<std::string>(a, value));
+    EXPECT_EQ(value, "a");
+    EXPECT_TRUE(secondGeneration.TryGetValue<std::string>(b, value));
+    EXPECT_EQ(value, "b");
+    EXPECT_TRUE(secondGeneration.TryGetValue<std::string>(c, value));
+    EXPECT_EQ(value, "c");
+    EXPECT_TRUE(secondGeneration.TryGetValue<std::string>(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<std::string>(a));
-  EXPECT_TRUE(thirdGeneration.HasKey(b));
-  EXPECT_EQ("b", thirdGeneration.GetValue<std::string>(b));
-  EXPECT_TRUE(thirdGeneration.HasKey(c));
-  EXPECT_EQ("c", thirdGeneration.GetValue<std::string>(c));
-  EXPECT_TRUE(thirdGeneration.HasKey(d));
-  EXPECT_EQ("d", thirdGeneration.GetValue<std::string>(d));
-  EXPECT_TRUE(thirdGeneration.HasKey(e));
-  EXPECT_EQ("e", thirdGeneration.GetValue<std::string>(e));
+  EXPECT_TRUE(thirdGeneration.TryGetValue<std::string>(a, value));
+  EXPECT_EQ(value, "a");
+  EXPECT_TRUE(thirdGeneration.TryGetValue<std::string>(b, value));
+  EXPECT_EQ(value, "b");
+  EXPECT_TRUE(thirdGeneration.TryGetValue<std::string>(c, value));
+  EXPECT_EQ(value, "c");
+  EXPECT_TRUE(thirdGeneration.TryGetValue<std::string>(d, value));
+  EXPECT_EQ(value, "d");
+  EXPECT_TRUE(thirdGeneration.TryGetValue<std::string>(e, value));
+  EXPECT_EQ(value, "e");
 }

 Context::Key const GlobalKey1;
--- 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());
--- 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<std::string>(TheKey);
-      EXPECT_EQ("TheValue", value);
-    }
+    std::string valueHolder;
+    EXPECT_TRUE(ctx.TryGetValue<std::string>(TheKey, valueHolder));
+    EXPECT_EQ("TheValue", valueHolder);
    return nextHttpPolicy.Send(request, ctx);
  }
 };
--- 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<bool> replicaStatus;
-    if (ctx.HasKey(SecondaryHostReplicaStatusKey))
-    {
-      replicaStatus = ctx.GetValue<std::shared_ptr<bool>>(SecondaryHostReplicaStatusKey);
-    }
+    ctx.TryGetValue(SecondaryHostReplicaStatusKey, replicaStatus);

    bool considerSecondary = (request.GetMethod() == Azure::Core::Http::HttpMethod::Get
                              || request.GetMethod() == Azure::Core::Http::HttpMethod::Head)