feat core: avoid url copies in clients::http::Request

Отчёт о тестировании: протестировано юнит-тестами commit_hash:64783d3230c657d26c43470449e5e74bbc090d3f
userver-framework · Feb 13, 2025 · 36e3ea8 · 36e3ea8
1 parent 572dd39
commit 36e3ea8
Show file tree

Hide file tree

Showing 5 changed files with 92 additions and 50 deletions.
diff --git a/core/include/userver/clients/http/request.hpp b/core/include/userver/clients/http/request.hpp
@@ -97,54 +97,54 @@ class Request final {
     Request& get() &;
     Request get() &&;
     /// GET request with url
-    Request& get(const std::string& url) &;
-    Request get(const std::string& url) &&;
+    Request& get(std::string url) &;
+    Request get(std::string url) &&;
     /// HEAD request
     Request& head() &;
     Request head() &&;
     /// HEAD request with url
-    Request& head(const std::string& url) &;
-    Request head(const std::string& url) &&;
+    Request& head(std::string url) &;
+    Request head(std::string url) &&;
     /// POST request
     Request& post() &;
     Request post() &&;
     /// POST request with url and data
-    Request& post(const std::string& url, std::string data = {}) &;
-    Request post(const std::string& url, std::string data = {}) &&;
+    Request& post(std::string url, std::string data = {}) &;
+    Request post(std::string url, std::string data = {}) &&;
     /// POST request with url and multipart/form-data
-    Request& post(const std::string& url, Form&& form) &;
-    Request post(const std::string& url, Form&& form) &&;
+    Request& post(std::string url, Form&& form) &;
+    Request post(std::string url, Form&& form) &&;
     /// PUT request
     Request& put() &;
     Request put() &&;
     /// PUT request with url and data
-    Request& put(const std::string& url, std::string data = {}) &;
-    Request put(const std::string& url, std::string data = {}) &&;
+    Request& put(std::string url, std::string data = {}) &;
+    Request put(std::string url, std::string data = {}) &&;
 
     /// PATCH request
     Request& patch() &;
     Request patch() &&;
     /// PATCH request with url and data
-    Request& patch(const std::string& url, std::string data = {}) &;
-    Request patch(const std::string& url, std::string data = {}) &&;
+    Request& patch(std::string url, std::string data = {}) &;
+    Request patch(std::string url, std::string data = {}) &&;
 
     /// DELETE request
     Request& delete_method() &;
     Request delete_method() &&;
     /// DELETE request with url
-    Request& delete_method(const std::string& url) &;
-    Request delete_method(const std::string& url) &&;
+    Request& delete_method(std::string url) &;
+    Request delete_method(std::string url) &&;
     /// DELETE request with url and data
-    Request& delete_method(const std::string& url, std::string data) &;
-    Request delete_method(const std::string& url, std::string data) &&;
+    Request& delete_method(std::string url, std::string data) &;
+    Request delete_method(std::string url, std::string data) &&;
 
     /// Set custom request method. Only replaces name of the HTTP method
     Request& set_custom_http_request_method(std::string method) &;
     Request set_custom_http_request_method(std::string method) &&;
 
     /// url if you don't specify request type with url
-    Request& url(const std::string& url) &;
-    Request url(const std::string& url) &&;
+    Request& url(std::string url) &;
+    Request url(std::string url) &&;
     /// data for POST request
     Request& data(std::string data) &;
     Request data(std::string data) &&;

diff --git a/core/src/clients/http/client_test.cpp b/core/src/clients/http/client_test.cpp
@@ -1513,6 +1513,33 @@ UTEST(HttpClient, CheckSchema) {
     UEXPECT_THROW(http_client_ptr->CreateRequest().url("telnet://localhost"), clients::http::BadArgumentException);
 }
 
+UTEST(HttpClient, ShortUrl) {
+    const std::shared_ptr<clients::http::Client> client = utest::CreateHttpClient();
+    clients::http::Request request = client->CreateRequest();
+    request.url("http://ex");
+
+    EXPECT_EQ(request.GetUrl(), "http://ex");
+}
+
+UTEST(HttpClient, LongUrl) {
+    const std::shared_ptr<clients::http::Client> client = utest::CreateHttpClient();
+    clients::http::Request request = client->CreateRequest();
+    request.url("http://large_enough_to_kick_the_sso_out.com");
+
+    EXPECT_EQ(request.GetUrl(), "http://large_enough_to_kick_the_sso_out.com");
+}
+
+UTEST(HttpRequest, GracefulExceptionOnInvalidUrl) {
+    const std::shared_ptr<clients::http::Client> client = utest::CreateHttpClient();
+    clients::http::Request request = client->CreateRequest();
+
+    UASSERT_THROW_MSG(
+        request.url("https://port_number_is_too_large.com:99999/yandsearch"),
+        clients::http::BadArgumentException,
+        "https://port_number_is_too_large.com:99999/yandsearch"
+    );
+}
+
 UTEST(HttpClient, DigestAuth) {
     AuthCallback auth_callback;
     const utest::SimpleServer http_server{auth_callback};

diff --git a/core/src/clients/http/request.cpp b/core/src/clients/http/request.cpp
@@ -241,18 +241,22 @@ std::shared_ptr<Response> Request::perform(utils::impl::SourceLocation location)
     return async_perform(location).Get();
 }
 
-Request& Request::url(const std::string& url) & {
+Request& Request::url(std::string url) & {
     if (!IsAllowedSchemaInUrl(url)) {
         throw BadArgumentException(curl::errc::EasyErrorCode::kUnsupportedProtocol, "Bad URL", url, {});
     }
+
+    RequestState& impl = *pimpl_;
     std::error_code ec;
-    pimpl_->easy().set_url(url, ec);
+    impl.easy().set_url(std::move(url), ec);
+
+    /// `curl::easy::set_url(std::string&&, std::error_code&)` doesn't consume the string if fails.
     if (ec) throw BadArgumentException(ec, "Bad URL", url, {});
 
-    pimpl_->SetDestinationMetricNameAuto(USERVER_NAMESPACE::http::ExtractMetaTypeFromUrl(url));
+    impl.SetDestinationMetricNameAuto(USERVER_NAMESPACE::http::ExtractMetaTypeFromUrl(impl.easy().get_original_url()));
     return *this;
 }
-Request Request::url(const std::string& url) && { return std::move(this->url(url)); }
+Request Request::url(std::string url) && { return std::move(this->url(std::move(url))); }
 
 Request& Request::timeout(long timeout_ms) & {
     pimpl_->set_timeout(timeout_ms);
@@ -477,40 +481,46 @@ Request Request::set_custom_http_request_method(std::string method) && {
     return std::move(this->set_custom_http_request_method(std::move(method)));
 }
 
-Request& Request::get(const std::string& url) & { return get().url(url); }
-Request Request::get(const std::string& url) && { return std::move(this->get(url)); }
+Request& Request::get(std::string url) & { return get().url(std::move(url)); }
+Request Request::get(std::string url) && { return std::move(this->get(std::move(url))); }
 
-Request& Request::head(const std::string& url) & { return head().url(url); }
-Request Request::head(const std::string& url) && { return std::move(this->head(url)); }
+Request& Request::head(std::string url) & { return head().url(std::move(url)); }
+Request Request::head(std::string url) && { return std::move(this->head(std::move(url))); }
 
-Request& Request::post(const std::string& url, Form&& form) & { return this->url(url).form(std::move(form)); }
-Request Request::post(const std::string& url, Form&& form) && { return std::move(this->post(url, std::move(form))); }
+Request& Request::post(std::string url, Form&& form) & { return this->url(std::move(url)).form(std::move(form)); }
+Request Request::post(std::string url, Form&& form) && {
+    return std::move(this->post(std::move(url), std::move(form)));
+}
 
-Request& Request::post(const std::string& url, std::string data) & {
-    return this->url(url).data(std::move(data)).post();
+Request& Request::post(std::string url, std::string data) & {
+    return this->url(std::move(url)).data(std::move(data)).post();
 }
-Request Request::post(const std::string& url, std::string data) && {
-    return std::move(this->post(url, std::move(data)));
+Request Request::post(std::string url, std::string data) && {
+    return std::move(this->post(std::move(url), std::move(data)));
 }
 
-Request& Request::put(const std::string& url, std::string data) & { return this->url(url).data(std::move(data)).put(); }
-Request Request::put(const std::string& url, std::string data) && { return std::move(this->put(url, std::move(data))); }
+Request& Request::put(std::string url, std::string data) & {
+    return this->url(std::move(url)).data(std::move(data)).put();
+}
+Request Request::put(std::string url, std::string data) && {
+    return std::move(this->put(std::move(url), std::move(data)));
+}
 
-Request& Request::patch(const std::string& url, std::string data) & {
-    return this->url(url).data(std::move(data)).patch();
+Request& Request::patch(std::string url, std::string data) & {
+    return this->url(std::move(url)).data(std::move(data)).patch();
 }
-Request Request::patch(const std::string& url, std::string data) && {
-    return std::move(this->patch(url, std::move(data)));
+Request Request::patch(std::string url, std::string data) && {
+    return std::move(this->patch(std::move(url), std::move(data)));
 }
 
-Request& Request::delete_method(const std::string& url) & { return this->url(url).delete_method(); }
-Request Request::delete_method(const std::string& url) && { return std::move(this->delete_method(url)); }
+Request& Request::delete_method(std::string url) & { return this->url(std::move(url)).delete_method(); }
+Request Request::delete_method(std::string url) && { return std::move(this->delete_method(std::move(url))); }
 
-Request& Request::delete_method(const std::string& url, std::string data) & {
-    return this->url(url).data(std::move(data)).delete_method();
+Request& Request::delete_method(std::string url, std::string data) & {
+    return this->url(std::move(url)).data(std::move(data)).delete_method();
 }
-Request Request::delete_method(const std::string& url, std::string data) && {
-    return std::move(this->delete_method(url, std::move(data)));
+Request Request::delete_method(std::string url, std::string data) && {
+    return std::move(this->delete_method(std::move(url), std::move(data)));
 }
 
 Request& Request::SetLoggedUrl(std::string url) & {

diff --git a/core/src/curl-ev/easy.cpp b/core/src/curl-ev/easy.cpp
@@ -309,17 +309,18 @@ void easy::set_url(std::string url_str) {
     throw_error(ec, "set_url");
 }
 
-void easy::set_url(std::string url_str, std::error_code& ec) {
-    orig_url_str_ = std::move(url_str);
-    url_.SetAbsoluteUrl(orig_url_str_.c_str(), ec);
+void easy::set_url(std::string&& url_str, std::error_code& ec) {
+    url_.SetAbsoluteUrl(url_str.c_str(), ec);
     if (!ec) {
         ec = static_cast<errc::EasyErrorCode>(
             native::curl_easy_setopt(handle_, native::CURLOPT_CURLU, url_.native_handle())
         );
         UASSERT(!ec);
     }
-    // not else, catch all errors
-    if (ec) {
+
+    if (!ec) {
+        orig_url_str_ = std::move(url_str);
+    } else {
         orig_url_str_.clear();
     }
 }

diff --git a/core/src/curl-ev/easy.hpp b/core/src/curl-ev/easy.hpp
@@ -304,7 +304,11 @@ class easy final : public std::enable_shared_from_this<easy> {
     // network options
 
     void set_url(std::string url_str);
-    void set_url(std::string url_str, std::error_code& ec);
+
+    /// @overload `set_url`
+    /// @note Doesn't perform a move from `url_str`, if error occurs.
+    void set_url(std::string&& url_str, std::error_code& ec);
+
     const std::string& get_original_url() const;
     const url& get_easy_url() const;