From 0676f788c3741f3d9413fb9126f60e782b7efa75 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Wed, 4 Nov 2020 09:03:35 -0800 Subject: [PATCH] remove throw from NextPolicy (#877) Update NextPolicy to that a reference instead of a pointer to the policies vector. This way we don't need to check if the pointer is null. Then, handle the case were no transport adapter was found fixes: https://github.com/Azure/azure-sdk-for-cpp/issues/874 --- .../inc/azure/core/http/pipeline.hpp | 2 +- .../azure-core/inc/azure/core/http/policy.hpp | 4 +- sdk/core/azure-core/src/http/policy.cpp | 11 ++--- sdk/core/azure-core/test/ut/CMakeLists.txt | 1 + sdk/core/azure-core/test/ut/policy.cpp | 46 +++++++++++++++++++ 5 files changed, 55 insertions(+), 9 deletions(-) create mode 100644 sdk/core/azure-core/test/ut/policy.cpp diff --git a/sdk/core/azure-core/inc/azure/core/http/pipeline.hpp b/sdk/core/azure-core/inc/azure/core/http/pipeline.hpp index 837e422e9..4651ab698 100644 --- a/sdk/core/azure-core/inc/azure/core/http/pipeline.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/pipeline.hpp @@ -99,7 +99,7 @@ namespace Azure { namespace Core { namespace Http { { // Accessing position zero is fine because pipeline must be constructed with at least one // policy. - return m_policies[0]->Send(ctx, request, NextHttpPolicy(0, &m_policies)); + return m_policies[0]->Send(ctx, request, NextHttpPolicy(0, m_policies)); } }; }}} // namespace Azure::Core::Http diff --git a/sdk/core/azure-core/inc/azure/core/http/policy.hpp b/sdk/core/azure-core/inc/azure/core/http/policy.hpp index 85fb3868d..e6d826ad3 100644 --- a/sdk/core/azure-core/inc/azure/core/http/policy.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/policy.hpp @@ -65,7 +65,7 @@ namespace Azure { namespace Core { namespace Http { // Represents the next HTTP policy in the stack sequence of policies. class NextHttpPolicy { const std::size_t m_index; - const std::vector>* m_policies; + const std::vector>& m_policies; public: /** @@ -78,7 +78,7 @@ namespace Azure { namespace Core { namespace Http { */ explicit NextHttpPolicy( std::size_t index, - const std::vector>* policies) + const std::vector>& policies) : m_index(index), m_policies(policies) { } diff --git a/sdk/core/azure-core/src/http/policy.cpp b/sdk/core/azure-core/src/http/policy.cpp index b3a6a9ac4..75a049fc6 100644 --- a/sdk/core/azure-core/src/http/policy.cpp +++ b/sdk/core/azure-core/src/http/policy.cpp @@ -16,16 +16,15 @@ Azure::Core::Logging::LogClassification const Azure::Core::Http::LogClassification::HttpTransportAdapter; #endif +// The NextHttpPolicy can't be created from a nullptr because it is a reference. So we don't need to +// check if m_policies is nullptr. std::unique_ptr NextHttpPolicy::Send(Context const& ctx, Request& req) { - if (m_policies == nullptr) - throw; - - if (m_index == m_policies->size() - 1) + if (m_index == m_policies.size() - 1) { // All the policies have run without running a transport policy - throw; + throw std::invalid_argument("Invalid pipeline. No transport policy found. Endless policy."); } - return (*m_policies)[m_index + 1]->Send(ctx, req, NextHttpPolicy{m_index + 1, m_policies}); + return m_policies[m_index + 1]->Send(ctx, req, NextHttpPolicy{m_index + 1, m_policies}); } diff --git a/sdk/core/azure-core/test/ut/CMakeLists.txt b/sdk/core/azure-core/test/ut/CMakeLists.txt index 7a50f171b..fa8c773d5 100644 --- a/sdk/core/azure-core/test/ut/CMakeLists.txt +++ b/sdk/core/azure-core/test/ut/CMakeLists.txt @@ -30,6 +30,7 @@ add_executable ( main.cpp nullable.cpp pipeline.cpp + policy.cpp simplified_header.cpp string.cpp telemetry_policy.cpp diff --git a/sdk/core/azure-core/test/ut/policy.cpp b/sdk/core/azure-core/test/ut/policy.cpp new file mode 100644 index 000000000..d993eb039 --- /dev/null +++ b/sdk/core/azure-core/test/ut/policy.cpp @@ -0,0 +1,46 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT + +#include "gtest/gtest.h" +#include +#include + +#include + +TEST(Policy, throwWhenNoTransportPolicy) +{ + // Construct pipeline without exception + std::vector> policies; + policies.push_back(std::make_unique("test", "test")); + policies.push_back(std::make_unique("test", "test")); + policies.push_back(std::make_unique("test", "test")); + policies.push_back(std::make_unique("test", "test")); + + Azure::Core::Http::HttpPipeline pipeline(policies); + Azure::Core::Http::Url url(""); + Azure::Core::Http::Request request(Azure::Core::Http::HttpMethod::Get, url); + EXPECT_THROW(pipeline.Send(Azure::Core::GetApplicationContext(), request), std::invalid_argument); +} + +TEST(Policy, throwWhenNoTransportPolicyMessage) +{ + // Construct pipeline without exception + std::vector> policies; + policies.push_back(std::make_unique("test", "test")); + policies.push_back(std::make_unique("test", "test")); + policies.push_back(std::make_unique("test", "test")); + policies.push_back(std::make_unique("test", "test")); + + Azure::Core::Http::HttpPipeline pipeline(policies); + Azure::Core::Http::Url url(""); + Azure::Core::Http::Request request(Azure::Core::Http::HttpMethod::Get, url); + + try + { + pipeline.Send(Azure::Core::GetApplicationContext(), request); + } + catch (std::invalid_argument& err) + { + EXPECT_STREQ("Invalid pipeline. No transport policy found. Endless policy.", err.what()); + } +}