From 8f7166afda6785df37ddd5ffe10e343bc9f0047a Mon Sep 17 00:00:00 2001 From: Andre Gleichner Date: Sun, 7 Apr 2024 15:59:49 +0200 Subject: [PATCH 1/3] Made interceptors runnable for any number of requests on a single Session object and allow repeating of requests within an interceptor. Repeated requests intentionally only run subsequent interceptors of the interceptor chain as otherwise re-entrance of intercept() would be an issue. --- cpr/multiperform.cpp | 44 +++++++++++++++++++++++++++----------- cpr/session.cpp | 44 +++++++++++++++++++++++++++----------- include/cpr/multiperform.h | 9 ++++++-- include/cpr/session.h | 11 +++++++--- test/interceptor_tests.cpp | 41 ++++++++++++++++++++++++++++++++++- 5 files changed, 118 insertions(+), 31 deletions(-) diff --git a/cpr/multiperform.cpp b/cpr/multiperform.cpp index 27b246316..bbef6d756 100644 --- a/cpr/multiperform.cpp +++ b/cpr/multiperform.cpp @@ -19,7 +19,10 @@ namespace cpr { -MultiPerform::MultiPerform() : multicurl_(new CurlMultiHolder()) {} +MultiPerform::MultiPerform() : multicurl_(new CurlMultiHolder()) { + current_interceptor_ = interceptors_.end(); + first_interceptor_ = interceptors_.end(); +} MultiPerform::~MultiPerform() { // Unlock all sessions @@ -154,18 +157,18 @@ std::vector MultiPerform::ReadMultiInfo(const std::function MultiPerform::MakeRequest() { - if (!interceptors_.empty()) { - return intercept(); - } + auto r = intercept(); + if (r.has_value()) + return r.value(); DoMultiPerform(); return ReadMultiInfo([](Session& session, CURLcode curl_error) -> Response { return session.Complete(curl_error); }); } std::vector MultiPerform::MakeDownloadRequest() { - if (!interceptors_.empty()) { - return intercept(); - } + auto r = intercept(); + if (r.has_value()) + return r.value(); DoMultiPerform(); return ReadMultiInfo([](Session& session, CURLcode curl_error) -> Response { return session.CompleteDownload(curl_error); }); @@ -325,15 +328,30 @@ std::vector MultiPerform::proceed() { return MakeRequest(); } -std::vector MultiPerform::intercept() { - // At least one interceptor exists -> Execute its intercept function - const std::shared_ptr interceptor = interceptors_.front(); - interceptors_.pop(); - return interceptor->intercept(*this); +std::optional> MultiPerform::intercept() { + if (current_interceptor_ == interceptors_.end()) + current_interceptor_ = first_interceptor_; + else + current_interceptor_++; + + if (current_interceptor_ != interceptors_.end()) { + auto icpt = current_interceptor_; + // Nested makeRequest() start at first_interceptor_, thus excluding previous interceptors. + first_interceptor_ = current_interceptor_; + ++first_interceptor_; + + auto r = (*current_interceptor_)->intercept(*this); + + first_interceptor_ = icpt; + + return r; + } + return std::nullopt; } void MultiPerform::AddInterceptor(const std::shared_ptr& pinterceptor) { - interceptors_.push(pinterceptor); + interceptors_.push_back(pinterceptor); + first_interceptor_ = interceptors_.begin(); } } // namespace cpr diff --git a/cpr/session.cpp b/cpr/session.cpp index 97eb93a71..fd50ce9cd 100644 --- a/cpr/session.cpp +++ b/cpr/session.cpp @@ -139,12 +139,14 @@ Session::Session() : curl_(new CurlHolder()) { #if LIBCURL_VERSION_NUM >= 0x071900 // 7.25.0 curl_easy_setopt(curl_->handle, CURLOPT_TCP_KEEPALIVE, 1L); #endif + current_interceptor_ = interceptors_.end(); + first_interceptor_ = interceptors_.end(); } Response Session::makeDownloadRequest() { - if (!interceptors_.empty()) { - return intercept(); - } + auto r = intercept(); + if (r.has_value()) + return r.value(); const CURLcode curl_error = DoEasyPerform(); @@ -262,11 +264,12 @@ void Session::prepareCommonDownload() { } Response Session::makeRequest() { - if (!interceptors_.empty()) { - return intercept(); - } + auto r = intercept(); + if (r.has_value()) + return r.value(); const CURLcode curl_error = DoEasyPerform(); + return Complete(curl_error); } @@ -880,7 +883,10 @@ Response Session::CompleteDownload(CURLcode curl_error) { } void Session::AddInterceptor(const std::shared_ptr& pinterceptor) { - interceptors_.push(pinterceptor); + // shall only add before first interceptor run + assert(current_interceptor_ == interceptors_.end()); + interceptors_.push_back(pinterceptor); + first_interceptor_ = interceptors_.begin(); } Response Session::proceed() { @@ -888,11 +894,25 @@ Response Session::proceed() { return makeRequest(); } -Response Session::intercept() { - // At least one interceptor exists -> Execute its intercept function - const std::shared_ptr interceptor = interceptors_.front(); - interceptors_.pop(); - return interceptor->intercept(*this); +std::optional Session::intercept() { + if (current_interceptor_ == interceptors_.end()) + current_interceptor_ = first_interceptor_; + else + current_interceptor_++; + + if (current_interceptor_ != interceptors_.end()) { + auto icpt = current_interceptor_; + // Nested makeRequest() start at first_interceptor_, thus excluding previous interceptors. + first_interceptor_ = current_interceptor_; + ++first_interceptor_; + + auto r = (*current_interceptor_)->intercept(*this); + + first_interceptor_ = icpt; + + return r; + } + return std::nullopt; } void Session::prepareBodyPayloadOrMultipart() const { diff --git a/include/cpr/multiperform.h b/include/cpr/multiperform.h index 14b9b1c01..26b455ca4 100644 --- a/include/cpr/multiperform.h +++ b/include/cpr/multiperform.h @@ -81,7 +81,7 @@ class MultiPerform { template void PrepareDownload(DownloadArgTypes... args); - std::vector intercept(); + std::optional> intercept(); std::vector proceed(); std::vector MakeRequest(); std::vector MakeDownloadRequest(); @@ -93,7 +93,12 @@ class MultiPerform { std::unique_ptr multicurl_; bool is_download_multi_perform{false}; - std::queue> interceptors_; + using InterceptorsContainer = std::list>; + InterceptorsContainer interceptors_; + // Currently running interceptor + InterceptorsContainer::iterator current_interceptor_; + // Interceptor within the chain where to start with each repeated request + InterceptorsContainer::iterator first_interceptor_; }; template diff --git a/include/cpr/session.h b/include/cpr/session.h index 0ff13f847..767b317dc 100644 --- a/include/cpr/session.h +++ b/include/cpr/session.h @@ -5,9 +5,9 @@ #include #include #include +#include #include #include -#include #include #include "cpr/accept_encoding.h" @@ -261,14 +261,19 @@ class Session : public std::enable_shared_from_this { size_t response_string_reserve_size_{0}; std::string response_string_; std::string header_string_; - std::queue> interceptors_; + using InterceptorsContainer = std::list>; + InterceptorsContainer interceptors_; + // Currently running interceptor + InterceptorsContainer::iterator current_interceptor_; + // Interceptor within the chain where to start with each repeated request + InterceptorsContainer::iterator first_interceptor_; bool isUsedInMultiPerform{false}; bool isCancellable{false}; Response makeDownloadRequest(); Response makeRequest(); Response proceed(); - Response intercept(); + std::optional intercept(); /** * Prepares the curl object for a request with everything used by all requests. **/ diff --git a/test/interceptor_tests.cpp b/test/interceptor_tests.cpp index 8bdc06b25..1732a3944 100644 --- a/test/interceptor_tests.cpp +++ b/test/interceptor_tests.cpp @@ -127,6 +127,19 @@ class ChangeRequestMethodToPatchInterceptor : public Interceptor { } }; +class RetryInterceptor : public Interceptor { + public: + Response intercept(Session& session) override { + // Proceed the chain + Response response = proceed(session); + + // retried request + response = proceed(session); + + return response; + } +}; + TEST(InterceptorTest, HiddenUrlRewriteInterceptorTest) { Url url{server->GetBaseUrl() + "/basic.json"}; Session session; @@ -151,6 +164,32 @@ TEST(InterceptorTest, ChangeStatusCodeInterceptorTest) { EXPECT_EQ(url, response.url); EXPECT_EQ(expected_status_code, response.status_code); EXPECT_EQ(ErrorCode::OK, response.error.code); + + // second request + response = session.Get(); + EXPECT_EQ(url, response.url); + EXPECT_EQ(expected_status_code, response.status_code); + EXPECT_EQ(ErrorCode::OK, response.error.code); +} + +TEST(InterceptorTest, RetryInterceptorTest) { + Url url{server->GetBaseUrl() + "/hello.html"}; + Session session; + session.SetUrl(url); + session.AddInterceptor(std::make_shared()); + session.AddInterceptor(std::make_shared()); + Response response = session.Get(); + + long expected_status_code{12345}; + EXPECT_EQ(url, response.url); + EXPECT_EQ(expected_status_code, response.status_code); + EXPECT_EQ(ErrorCode::OK, response.error.code); + + // second request + response = session.Get(); + EXPECT_EQ(url, response.url); + EXPECT_EQ(expected_status_code, response.status_code); + EXPECT_EQ(ErrorCode::OK, response.error.code); } TEST(InterceptorTest, DownloadChangeStatusCodeInterceptorTest) { @@ -366,4 +405,4 @@ int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); ::testing::AddGlobalTestEnvironment(server); return RUN_ALL_TESTS(); -} \ No newline at end of file +} From 3f2e2d47e9d820dd78b8a3984e3e8dced48df3c3 Mon Sep 17 00:00:00 2001 From: Andre Gleichner Date: Thu, 18 Apr 2024 21:49:52 +0200 Subject: [PATCH 2/3] Using const_iterator as there's no need to modify the pointed to elem. Fixing clang-tidy issues. --- cpr/multiperform.cpp | 12 ++++++++---- cpr/session.cpp | 12 ++++++++---- include/cpr/session.h | 5 +++-- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/cpr/multiperform.cpp b/cpr/multiperform.cpp index bbef6d756..e66d6b971 100644 --- a/cpr/multiperform.cpp +++ b/cpr/multiperform.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -158,8 +159,9 @@ std::vector MultiPerform::ReadMultiInfo(const std::function MultiPerform::MakeRequest() { auto r = intercept(); - if (r.has_value()) + if (r.has_value()) { return r.value(); + } DoMultiPerform(); return ReadMultiInfo([](Session& session, CURLcode curl_error) -> Response { return session.Complete(curl_error); }); @@ -167,8 +169,9 @@ std::vector MultiPerform::MakeRequest() { std::vector MultiPerform::MakeDownloadRequest() { auto r = intercept(); - if (r.has_value()) + if (r.has_value()) { return r.value(); + } DoMultiPerform(); return ReadMultiInfo([](Session& session, CURLcode curl_error) -> Response { return session.CompleteDownload(curl_error); }); @@ -329,10 +332,11 @@ std::vector MultiPerform::proceed() { } std::optional> MultiPerform::intercept() { - if (current_interceptor_ == interceptors_.end()) + if (current_interceptor_ == interceptors_.end()) { current_interceptor_ = first_interceptor_; - else + } else { current_interceptor_++; + } if (current_interceptor_ != interceptors_.end()) { auto icpt = current_interceptor_; diff --git a/cpr/session.cpp b/cpr/session.cpp index fd50ce9cd..8a2666be5 100644 --- a/cpr/session.cpp +++ b/cpr/session.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -145,8 +146,9 @@ Session::Session() : curl_(new CurlHolder()) { Response Session::makeDownloadRequest() { auto r = intercept(); - if (r.has_value()) + if (r.has_value()) { return r.value(); + } const CURLcode curl_error = DoEasyPerform(); @@ -265,8 +267,9 @@ void Session::prepareCommonDownload() { Response Session::makeRequest() { auto r = intercept(); - if (r.has_value()) + if (r.has_value()) { return r.value(); + } const CURLcode curl_error = DoEasyPerform(); @@ -895,10 +898,11 @@ Response Session::proceed() { } std::optional Session::intercept() { - if (current_interceptor_ == interceptors_.end()) + if (current_interceptor_ == interceptors_.end()) { current_interceptor_ = first_interceptor_; - else + } else { current_interceptor_++; + } if (current_interceptor_ != interceptors_.end()) { auto icpt = current_interceptor_; diff --git a/include/cpr/session.h b/include/cpr/session.h index 767b317dc..97652f4b6 100644 --- a/include/cpr/session.h +++ b/include/cpr/session.h @@ -261,12 +261,13 @@ class Session : public std::enable_shared_from_this { size_t response_string_reserve_size_{0}; std::string response_string_; std::string header_string_; + // Container type is required to keep iterator valid on elem insertion. E.g. list but not vector. using InterceptorsContainer = std::list>; InterceptorsContainer interceptors_; // Currently running interceptor - InterceptorsContainer::iterator current_interceptor_; + InterceptorsContainer::const_iterator current_interceptor_; // Interceptor within the chain where to start with each repeated request - InterceptorsContainer::iterator first_interceptor_; + InterceptorsContainer::const_iterator first_interceptor_; bool isUsedInMultiPerform{false}; bool isCancellable{false}; From d0a63d460b057767e65cb6c0ce92e93ce9078bb5 Mon Sep 17 00:00:00 2001 From: Andre Gleichner Date: Wed, 24 Apr 2024 21:04:42 +0200 Subject: [PATCH 3/3] Made intercept() return a const value. Used explicit type instead of auto for intercept() retval. Tried to extend a MultiPerform interceptor test to showcase it can run multiple times, just to find that MultiPerform is not yet able to perform multiple times. --- cpr/multiperform.cpp | 11 +++++++---- cpr/session.cpp | 10 +++++----- include/cpr/multiperform.h | 2 +- include/cpr/session.h | 2 +- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/cpr/multiperform.cpp b/cpr/multiperform.cpp index e66d6b971..27c40ef30 100644 --- a/cpr/multiperform.cpp +++ b/cpr/multiperform.cpp @@ -6,6 +6,7 @@ #include "cpr/response.h" #include "cpr/session.h" #include +#include #include #include #include @@ -158,7 +159,7 @@ std::vector MultiPerform::ReadMultiInfo(const std::function MultiPerform::MakeRequest() { - auto r = intercept(); + const std::optional> r = intercept(); if (r.has_value()) { return r.value(); } @@ -168,7 +169,7 @@ std::vector MultiPerform::MakeRequest() { } std::vector MultiPerform::MakeDownloadRequest() { - auto r = intercept(); + const std::optional> r = intercept(); if (r.has_value()) { return r.value(); } @@ -331,7 +332,7 @@ std::vector MultiPerform::proceed() { return MakeRequest(); } -std::optional> MultiPerform::intercept() { +const std::optional> MultiPerform::intercept() { if (current_interceptor_ == interceptors_.end()) { current_interceptor_ = first_interceptor_; } else { @@ -344,7 +345,7 @@ std::optional> MultiPerform::intercept() { first_interceptor_ = current_interceptor_; ++first_interceptor_; - auto r = (*current_interceptor_)->intercept(*this); + const std::optional> r = (*current_interceptor_)->intercept(*this); first_interceptor_ = icpt; @@ -354,6 +355,8 @@ std::optional> MultiPerform::intercept() { } void MultiPerform::AddInterceptor(const std::shared_ptr& pinterceptor) { + // Shall only add before first interceptor run + assert(current_interceptor_ == interceptors_.end()); interceptors_.push_back(pinterceptor); first_interceptor_ = interceptors_.begin(); } diff --git a/cpr/session.cpp b/cpr/session.cpp index 8a2666be5..914d0bff1 100644 --- a/cpr/session.cpp +++ b/cpr/session.cpp @@ -145,7 +145,7 @@ Session::Session() : curl_(new CurlHolder()) { } Response Session::makeDownloadRequest() { - auto r = intercept(); + const std::optional r = intercept(); if (r.has_value()) { return r.value(); } @@ -266,7 +266,7 @@ void Session::prepareCommonDownload() { } Response Session::makeRequest() { - auto r = intercept(); + const std::optional r = intercept(); if (r.has_value()) { return r.value(); } @@ -886,7 +886,7 @@ Response Session::CompleteDownload(CURLcode curl_error) { } void Session::AddInterceptor(const std::shared_ptr& pinterceptor) { - // shall only add before first interceptor run + // Shall only add before first interceptor run assert(current_interceptor_ == interceptors_.end()); interceptors_.push_back(pinterceptor); first_interceptor_ = interceptors_.begin(); @@ -897,7 +897,7 @@ Response Session::proceed() { return makeRequest(); } -std::optional Session::intercept() { +const std::optional Session::intercept() { if (current_interceptor_ == interceptors_.end()) { current_interceptor_ = first_interceptor_; } else { @@ -910,7 +910,7 @@ std::optional Session::intercept() { first_interceptor_ = current_interceptor_; ++first_interceptor_; - auto r = (*current_interceptor_)->intercept(*this); + const std::optional r = (*current_interceptor_)->intercept(*this); first_interceptor_ = icpt; diff --git a/include/cpr/multiperform.h b/include/cpr/multiperform.h index 26b455ca4..d4ac445ee 100644 --- a/include/cpr/multiperform.h +++ b/include/cpr/multiperform.h @@ -81,7 +81,7 @@ class MultiPerform { template void PrepareDownload(DownloadArgTypes... args); - std::optional> intercept(); + const std::optional> intercept(); std::vector proceed(); std::vector MakeRequest(); std::vector MakeDownloadRequest(); diff --git a/include/cpr/session.h b/include/cpr/session.h index 97652f4b6..df1324707 100644 --- a/include/cpr/session.h +++ b/include/cpr/session.h @@ -274,7 +274,7 @@ class Session : public std::enable_shared_from_this { Response makeDownloadRequest(); Response makeRequest(); Response proceed(); - std::optional intercept(); + const std::optional intercept(); /** * Prepares the curl object for a request with everything used by all requests. **/