Update the RetryPolicy for the GA release, keeping ShouldRetry extension point hidden. (#5771)

* Update the RetryPolicy for the GA release, keeping ShouldRetry extension point hidden. * Mark test helper virtual functions private, so they aren't accessible/callable by callers. * Update the changelog. * Update CL.
2024-07-11 20:39:37 -04:00 · 2024-07-11 20:39:37 -04:00 · 9ccd206ff8
commit 9ccd206ff8
parent 313fb0e58f
3 changed files with 229 additions and 204 deletions
--- a/sdk/core/azure-core/CHANGELOG.md
+++ b/sdk/core/azure-core/CHANGELOG.md
@ -11,6 +11,7 @@
 ### Other Changes

 - Updated JSON library to 3.11.3.
+- Hide methods on the `RetryPolicy` that are not intended for public use.

 ## 1.13.0-beta.1 (2024-06-06)

--- a/sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp
+++ b/sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp
@ -30,6 +30,14 @@
 #include <utility>
 #include <vector>

+#if defined(_azure_TESTING_BUILD)
+// Define the classes used from tests
+namespace Azure { namespace Core { namespace Test {
+  class RetryPolicyTest;
+  class RetryLogic;
+}}} // namespace Azure::Core::Test
+#endif
+
 /**
 * A function that should be implemented and linked to the end-user application in order to override
 * an HTTP transport implementation provided by Azure SDK with custom implementation.
@ -363,7 +371,16 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
    /**
     * @brief HTTP retry policy.
     */
-    class RetryPolicy : public HttpPolicy {
+    class RetryPolicy
+#if !defined(_azure_TESTING_BUILD)
+        final
+#endif
+        : public HttpPolicy {
+#if defined(_azure_TESTING_BUILD)
+      // make tests classes friends
+      friend class Azure::Core::Test::RetryPolicyTest;
+      friend class Azure::Core::Test::RetryLogic;
+#endif
    private:
      RetryOptions m_retryOptions;

@ -398,7 +415,7 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
       */
      static int32_t GetRetryCount(Context const& context);

-    protected:
+    private:
      virtual bool ShouldRetryOnTransportFailure(
          RetryOptions const& retryOptions,
          int32_t attempt,
--- a/sdk/core/azure-core/test/ut/retry_policy_test.cpp
+++ b/sdk/core/azure-core/test/ut/retry_policy_test.cpp
@ -13,207 +13,214 @@ using namespace Azure::Core::Http;
 using namespace Azure::Core::Http::Policies;
 using namespace Azure::Core::Http::Policies::_internal;

-namespace {
-class TestTransportPolicy final : public HttpPolicy {
-private:
-  std::function<std::unique_ptr<RawResponse>()> m_send;
+namespace Azure { namespace Core { namespace Test {
+  class TestTransportPolicy final : public HttpPolicy {
+  private:
+    std::function<std::unique_ptr<RawResponse>()> m_send;

-public:
-  TestTransportPolicy(std::function<std::unique_ptr<RawResponse>()> send) : m_send(send) {}
+  public:
+    TestTransportPolicy(std::function<std::unique_ptr<RawResponse>()> send) : m_send(send) {}

-  std::unique_ptr<Azure::Core::Http::RawResponse> Send(
-      Request&,
-      NextHttpPolicy,
-      Azure::Core::Context const&) const override
-  {
-    return m_send();
-  }
+    std::unique_ptr<Azure::Core::Http::RawResponse> Send(
+        Request&,
+        NextHttpPolicy,
+        Azure::Core::Context const&) const override
+    {
+      return m_send();
+    }

-  std::unique_ptr<HttpPolicy> Clone() const override
-  {
-    return std::make_unique<TestTransportPolicy>(*this);
-  }
-};
+    std::unique_ptr<HttpPolicy> Clone() const override
+    {
+      return std::make_unique<TestTransportPolicy>(*this);
+    }
+  };

-class RetryPolicyTest final : public RetryPolicy {
-private:
-  std::function<bool(RetryOptions const&, int32_t, std::chrono::milliseconds&, double)>
-      m_shouldRetryOnTransportFailure;
+  class RetryPolicyTest final : public RetryPolicy {
+  private:
+    std::function<bool(RetryOptions const&, int32_t, std::chrono::milliseconds&, double)>
+        m_shouldRetryOnTransportFailure;

-  std::function<
-      bool(RawResponse const&, RetryOptions const&, int32_t, std::chrono::milliseconds&, double)>
-      m_shouldRetryOnResponse;
+    std::function<
+        bool(RawResponse const&, RetryOptions const&, int32_t, std::chrono::milliseconds&, double)>
+        m_shouldRetryOnResponse;

-public:
-  bool BaseShouldRetryOnTransportFailure(
-      RetryOptions const& retryOptions,
-      int32_t attempt,
-      std::chrono::milliseconds& retryAfter,
-      double jitterFactor) const
-  {
-    return RetryPolicy::ShouldRetryOnTransportFailure(
-        retryOptions, attempt, retryAfter, jitterFactor);
-  }
+  public:
+    bool BaseShouldRetryOnTransportFailure(
+        RetryOptions const& retryOptions,
+        int32_t attempt,
+        std::chrono::milliseconds& retryAfter,
+        double jitterFactor) const
+    {
+      return RetryPolicy::ShouldRetryOnTransportFailure(
+          retryOptions, attempt, retryAfter, jitterFactor);
+    }

-  bool BaseShouldRetryOnResponse(
-      RawResponse const& response,
-      RetryOptions const& retryOptions,
-      int32_t attempt,
-      std::chrono::milliseconds& retryAfter,
-      double jitterFactor) const
-  {
-    return RetryPolicy::ShouldRetryOnResponse(
-        response, retryOptions, attempt, retryAfter, jitterFactor);
-  }
+    bool BaseShouldRetryOnResponse(
+        RawResponse const& response,
+        RetryOptions const& retryOptions,
+        int32_t attempt,
+        std::chrono::milliseconds& retryAfter,
+        double jitterFactor) const
+    {
+      return RetryPolicy::ShouldRetryOnResponse(
+          response, retryOptions, attempt, retryAfter, jitterFactor);
+    }

-  RetryPolicyTest(
-      RetryOptions const& retryOptions,
-      decltype(m_shouldRetryOnTransportFailure) shouldRetryOnTransportFailure,
-      decltype(m_shouldRetryOnResponse) shouldRetryOnResponse)
-      : RetryPolicy(retryOptions),
-        m_shouldRetryOnTransportFailure(
-            shouldRetryOnTransportFailure != nullptr //
-                ? shouldRetryOnTransportFailure
-                : static_cast<decltype(m_shouldRetryOnTransportFailure)>( //
-                    [this](auto options, auto attempt, auto retryAfter, auto jitter) {
-                      retryAfter = std::chrono::milliseconds(0);
-                      auto ignore = decltype(retryAfter)();
-                      return this->BaseShouldRetryOnTransportFailure(
-                          options, attempt, ignore, jitter);
-                    })),
-        m_shouldRetryOnResponse(
-            shouldRetryOnResponse != nullptr //
-                ? shouldRetryOnResponse
-                : static_cast<decltype(m_shouldRetryOnResponse)>( //
-                    [this](
-                        RawResponse const& response,
-                        auto options,
-                        auto attempt,
-                        auto retryAfter,
-                        auto jitter) {
-                      retryAfter = std::chrono::milliseconds(0);
-                      auto ignore = decltype(retryAfter)();
-                      return this->BaseShouldRetryOnResponse(
-                          response, options, attempt, ignore, jitter);
-                    }))
-  {
-  }
+    RetryPolicyTest(
+        RetryOptions const& retryOptions,
+        decltype(m_shouldRetryOnTransportFailure) shouldRetryOnTransportFailure,
+        decltype(m_shouldRetryOnResponse) shouldRetryOnResponse)
+        : RetryPolicy(retryOptions),
+          m_shouldRetryOnTransportFailure(
+              shouldRetryOnTransportFailure != nullptr //
+                  ? shouldRetryOnTransportFailure
+                  : static_cast<decltype(m_shouldRetryOnTransportFailure)>( //
+                      [this](auto options, auto attempt, auto retryAfter, auto jitter) {
+                        retryAfter = std::chrono::milliseconds(0);
+                        auto ignore = decltype(retryAfter)();
+                        return this->BaseShouldRetryOnTransportFailure(
+                            options, attempt, ignore, jitter);
+                      })),
+          m_shouldRetryOnResponse(
+              shouldRetryOnResponse != nullptr //
+                  ? shouldRetryOnResponse
+                  : static_cast<decltype(m_shouldRetryOnResponse)>( //
+                      [this](
+                          RawResponse const& response,
+                          auto options,
+                          auto attempt,
+                          auto retryAfter,
+                          auto jitter) {
+                        retryAfter = std::chrono::milliseconds(0);
+                        auto ignore = decltype(retryAfter)();
+                        return this->BaseShouldRetryOnResponse(
+                            response, options, attempt, ignore, jitter);
+                      }))
+    {
+    }

-  std::unique_ptr<HttpPolicy> Clone() const override
-  {
-    return std::make_unique<RetryPolicyTest>(*this);
-  }
+    std::unique_ptr<HttpPolicy> Clone() const override
+    {
+      return std::make_unique<RetryPolicyTest>(*this);
+    }

-protected:
-  bool ShouldRetryOnTransportFailure(
-      RetryOptions const& retryOptions,
-      int32_t attempt,
-      std::chrono::milliseconds& retryAfter,
-      double jitterFactor) const override
-  {
-    return m_shouldRetryOnTransportFailure(retryOptions, attempt, retryAfter, jitterFactor);
-  }
+  protected:
+    bool ShouldRetryOnTransportFailure(
+        RetryOptions const& retryOptions,
+        int32_t attempt,
+        std::chrono::milliseconds& retryAfter,
+        double jitterFactor) const override
+    {
+      return m_shouldRetryOnTransportFailure(retryOptions, attempt, retryAfter, jitterFactor);
+    }

-  bool ShouldRetryOnResponse(
-      RawResponse const& response,
-      RetryOptions const& retryOptions,
-      int32_t attempt,
-      std::chrono::milliseconds& retryAfter,
-      double jitterFactor) const override
-  {
-    return m_shouldRetryOnResponse(response, retryOptions, attempt, retryAfter, jitterFactor);
-  }
-};
+    bool ShouldRetryOnResponse(
+        RawResponse const& response,
+        RetryOptions const& retryOptions,
+        int32_t attempt,
+        std::chrono::milliseconds& retryAfter,
+        double jitterFactor) const override
+    {
+      return m_shouldRetryOnResponse(response, retryOptions, attempt, retryAfter, jitterFactor);
+    }
+  };

-class RetryPolicyTest_CustomRetry_True final : public RetryPolicy {
-public:
-  RetryPolicyTest_CustomRetry_True(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) {}
+  class RetryPolicyTest_CustomRetry_True final : public RetryPolicy {
+  public:
+    RetryPolicyTest_CustomRetry_True(RetryOptions const& retryOptions) : RetryPolicy(retryOptions)
+    {
+    }

-  std::unique_ptr<HttpPolicy> Clone() const override
-  {
-    return std::make_unique<RetryPolicyTest_CustomRetry_True>(*this);
-  }
+    std::unique_ptr<HttpPolicy> Clone() const override
+    {
+      return std::make_unique<RetryPolicyTest_CustomRetry_True>(*this);
+    }

-protected:
-  bool ShouldRetry(std::unique_ptr<RawResponse> const&, RetryOptions const&) const override
-  {
-    return true;
-  }
-};
-
-class RetryPolicyTest_CustomRetry_Throw final : public RetryPolicy {
-public:
-  RetryPolicyTest_CustomRetry_Throw(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) {}
-
-  std::unique_ptr<HttpPolicy> Clone() const override
-  {
-    return std::make_unique<RetryPolicyTest_CustomRetry_Throw>(*this);
-  }
-
-protected:
-  bool ShouldRetry(std::unique_ptr<RawResponse> const&, RetryOptions const&) const override
-  {
-    throw TransportException("Short-circuit evaluation means this should never be called.");
-  }
-};
-
-class RetryPolicyTest_CustomRetry_CopySource final : public RetryPolicy {
-public:
-  RetryPolicyTest_CustomRetry_CopySource(RetryOptions const& retryOptions)
-      : RetryPolicy(retryOptions)
-  {
-  }
-
-  std::unique_ptr<HttpPolicy> Clone() const override
-  {
-    return std::make_unique<RetryPolicyTest_CustomRetry_CopySource>(*this);
-  }
-
-protected:
-  bool ShouldRetry(std::unique_ptr<RawResponse> const& response, RetryOptions const&) const override
-  {
-    if (response == nullptr)
+  protected:
+    bool ShouldRetry(std::unique_ptr<RawResponse> const&, RetryOptions const&) const override
    {
      return true;
    }
+  };

-    if (static_cast<std::underlying_type_t<Azure::Core::Http::HttpStatusCode>>(
-            response->GetStatusCode())
-        >= 400)
+  class RetryPolicyTest_CustomRetry_Throw final : public RetryPolicy {
+  public:
+    RetryPolicyTest_CustomRetry_Throw(RetryOptions const& retryOptions) : RetryPolicy(retryOptions)
    {
-      const auto& headers = response->GetHeaders();
-      auto ite = headers.find("x-ms-error-code");
-      if (ite != headers.end())
-      {
-        const std::string error = ite->second;
-
-        if (error == "InternalError" || error == "OperationTimedOut" || error == "ServerBusy")
-        {
-          return true;
-        }
-      }
    }

-    if (static_cast<std::underlying_type_t<Azure::Core::Http::HttpStatusCode>>(
-            response->GetStatusCode())
-        >= 400)
+    std::unique_ptr<HttpPolicy> Clone() const override
    {
-      const auto& headers = response->GetHeaders();
-      auto ite = headers.find("x-ms-copy-source-error-code");
-      if (ite != headers.end())
-      {
-        const std::string error = ite->second;
+      return std::make_unique<RetryPolicyTest_CustomRetry_Throw>(*this);
+    }

-        if (error == "InternalError" || error == "OperationTimedOut" || error == "ServerBusy")
+  protected:
+    bool ShouldRetry(std::unique_ptr<RawResponse> const&, RetryOptions const&) const override
+    {
+      throw TransportException("Short-circuit evaluation means this should never be called.");
+    }
+  };
+
+  class RetryPolicyTest_CustomRetry_CopySource final : public RetryPolicy {
+  public:
+    RetryPolicyTest_CustomRetry_CopySource(RetryOptions const& retryOptions)
+        : RetryPolicy(retryOptions)
+    {
+    }
+
+    std::unique_ptr<HttpPolicy> Clone() const override
+    {
+      return std::make_unique<RetryPolicyTest_CustomRetry_CopySource>(*this);
+    }
+
+  protected:
+    bool ShouldRetry(std::unique_ptr<RawResponse> const& response, RetryOptions const&)
+        const override
+    {
+      if (response == nullptr)
+      {
+        return true;
+      }
+
+      if (static_cast<std::underlying_type_t<Azure::Core::Http::HttpStatusCode>>(
+              response->GetStatusCode())
+          >= 400)
+      {
+        const auto& headers = response->GetHeaders();
+        auto ite = headers.find("x-ms-error-code");
+        if (ite != headers.end())
        {
-          return true;
+          const std::string error = ite->second;
+
+          if (error == "InternalError" || error == "OperationTimedOut" || error == "ServerBusy")
+          {
+            return true;
+          }
        }
      }
+
+      if (static_cast<std::underlying_type_t<Azure::Core::Http::HttpStatusCode>>(
+              response->GetStatusCode())
+          >= 400)
+      {
+        const auto& headers = response->GetHeaders();
+        auto ite = headers.find("x-ms-copy-source-error-code");
+        if (ite != headers.end())
+        {
+          const std::string error = ite->second;
+
+          if (error == "InternalError" || error == "OperationTimedOut" || error == "ServerBusy")
+          {
+            return true;
+          }
+        }
+      }
+      return false;
    }
-    return false;
-  }
-};
-} // namespace
+  };
+}}} // namespace Azure::Core::Test
+
+using namespace Azure::Core::Test;

 TEST(RetryPolicy, ShouldRetry)
 {
@ -589,38 +596,38 @@ TEST(RetryPolicy, ShouldRetryOnTransportFailure)
  EXPECT_EQ(jitterReceived, -1);
 }

-namespace {
-class RetryLogic final : private RetryPolicy {
-  RetryLogic() : RetryPolicy(RetryOptions()) {}
-  ~RetryLogic() {}
+namespace Azure { namespace Core { namespace Test {
+  class RetryLogic final : private RetryPolicy {
+    RetryLogic() : RetryPolicy(RetryOptions()) {}
+    ~RetryLogic() {}

-  static RetryLogic const g_retryPolicy;
+    static RetryLogic const g_retryPolicy;

-public:
-  static bool TestShouldRetryOnTransportFailure(
-      RetryOptions const& retryOptions,
-      int32_t attempt,
-      std::chrono::milliseconds& retryAfter,
-      double jitterFactor)
-  {
-    return g_retryPolicy.ShouldRetryOnTransportFailure(
-        retryOptions, attempt, retryAfter, jitterFactor);
-  }
+  public:
+    static bool TestShouldRetryOnTransportFailure(
+        RetryOptions const& retryOptions,
+        int32_t attempt,
+        std::chrono::milliseconds& retryAfter,
+        double jitterFactor)
+    {
+      return g_retryPolicy.ShouldRetryOnTransportFailure(
+          retryOptions, attempt, retryAfter, jitterFactor);
+    }

-  static bool TestShouldRetryOnResponse(
-      RawResponse const& response,
-      RetryOptions const& retryOptions,
-      int32_t attempt,
-      std::chrono::milliseconds& retryAfter,
-      double jitterFactor)
-  {
-    return g_retryPolicy.ShouldRetryOnResponse(
-        response, retryOptions, attempt, retryAfter, jitterFactor);
-  }
-};
+    static bool TestShouldRetryOnResponse(
+        RawResponse const& response,
+        RetryOptions const& retryOptions,
+        int32_t attempt,
+        std::chrono::milliseconds& retryAfter,
+        double jitterFactor)
+    {
+      return g_retryPolicy.ShouldRetryOnResponse(
+          response, retryOptions, attempt, retryAfter, jitterFactor);
+    }
+  };

-RetryLogic const RetryLogic::g_retryPolicy;
-} // namespace
+  RetryLogic const RetryLogic::g_retryPolicy;
+}}} // namespace Azure::Core::Test

 TEST(RetryPolicy, Exponential)
 {