Merge pull request #223 from DinosL/improve_curl

CURL: Refactor the client to better use the curl API for constructing URLs

Author: carlopi

src/httpfs_curl_client.cpp

@@ -115,7 +115,8 @@ static idx_t httpfs_client_count = 0;
 class HTTPFSCurlClient : public HTTPClient {
 public:
 	HTTPFSCurlClient(HTTPFSParams &http_params, const string &proto_host_port) {
-		// FIXME: proto_host_port is not used
+		base_url = curl_url();
+		curl_url_set(base_url, CURLUPART_URL, proto_host_port.c_str(), 0);
 		Initialize(http_params);
 	}
 	void Initialize(HTTPParams &http_p) override {
@@ -178,18 +179,10 @@ class HTTPFSCurlClient : public HTTPClient {
 	}
 
 	~HTTPFSCurlClient() {
+		curl_url_cleanup(base_url);
 		DestroyCurlGlobal();
 	}
 
-	static string EncodeSpaces(const string &url) {
-		string out;
-		out.reserve(url.size());
-		for (char c : url) {
-			out += (c == ' ') ? "%20" : string(1, c);
-		}
-		return out;
-	}
-
 	unique_ptr<HTTPResponse> Get(GetRequestInfo &info) override {
 		ResetRequestInfo();
 		if (state) {
@@ -201,12 +194,16 @@ class HTTPFSCurlClient : public HTTPClient {
 
 		CURLcode res;
 		{
-			// If the same handle served a HEAD request, we must set NOBODY back to 0L to request content again
 			curl_easy_setopt(*curl, CURLOPT_NOBODY, 0L);
-			auto encoded_url = EncodeSpaces(request_info->url);
-			curl_easy_setopt(*curl, CURLOPT_URL, encoded_url.c_str());
+			CURLU *url = curl_url_dup(base_url);
+			curl_url_set(url, CURLUPART_URL, info.path.c_str(), 0);
+
+			curl_easy_setopt(*curl, CURLOPT_URL, nullptr);
+			curl_easy_setopt(*curl, CURLOPT_CURLU, url);
 			curl_easy_setopt(*curl, CURLOPT_HTTPHEADER, curl_headers ? curl_headers.headers : nullptr);
+
 			res = curl->Execute();
+			curl_url_cleanup(url);
 		}
 
 		curl_easy_getinfo(*curl, CURLINFO_RESPONSE_CODE, &request_info->response_code);
@@ -253,8 +250,11 @@ class HTTPFSCurlClient : public HTTPClient {
 
 		CURLcode res;
 		{
-			auto encoded_url = EncodeSpaces(request_info->url);
-			curl_easy_setopt(*curl, CURLOPT_URL, encoded_url.c_str());
+			CURLU *url = curl_url_dup(base_url);
+			curl_url_set(url, CURLUPART_URL, info.path.c_str(), 0);
+
+			curl_easy_setopt(*curl, CURLOPT_URL, nullptr);
+			curl_easy_setopt(*curl, CURLOPT_CURLU, url);
 			// Perform PUT
 			curl_easy_setopt(*curl, CURLOPT_CUSTOMREQUEST, "PUT");
 			// Include PUT body
@@ -265,6 +265,7 @@ class HTTPFSCurlClient : public HTTPClient {
 			curl_easy_setopt(*curl, CURLOPT_HTTPHEADER, curl_headers ? curl_headers.headers : nullptr);
 
 			res = curl->Execute();
+			curl_url_cleanup(url);
 		}
 
 		curl_easy_getinfo(*curl, CURLINFO_RESPONSE_CODE, &request_info->response_code);
@@ -284,19 +285,22 @@ class HTTPFSCurlClient : public HTTPClient {
 
 		CURLcode res;
 		{
-			// Set URL
-			auto encoded_url = EncodeSpaces(request_info->url);
-			curl_easy_setopt(*curl, CURLOPT_URL, encoded_url.c_str());
-
 			// Perform HEAD request instead of GET
 			curl_easy_setopt(*curl, CURLOPT_NOBODY, 1L);
 			curl_easy_setopt(*curl, CURLOPT_HTTPGET, 0L);
 
+			CURLU *url = curl_url_dup(base_url);
+			curl_url_set(url, CURLUPART_URL, info.path.c_str(), 0);
+
+			curl_easy_setopt(*curl, CURLOPT_URL, nullptr);
+			curl_easy_setopt(*curl, CURLOPT_CURLU, url);
+
 			// Add headers if any
 			curl_easy_setopt(*curl, CURLOPT_HTTPHEADER, curl_headers ? curl_headers.headers : nullptr);
 
 			// Execute HEAD request
 			res = curl->Execute();
+			curl_url_cleanup(url);
 		}
 
 		curl_easy_getinfo(*curl, CURLINFO_RESPONSE_CODE, &request_info->response_code);
@@ -315,9 +319,11 @@ class HTTPFSCurlClient : public HTTPClient {
 
 		CURLcode res;
 		{
-			// Set URL
-			auto encoded_url = EncodeSpaces(request_info->url);
-			curl_easy_setopt(*curl, CURLOPT_URL, encoded_url.c_str());
+			CURLU *url = curl_url_dup(base_url);
+			curl_url_set(url, CURLUPART_URL, info.path.c_str(), 0);
+
+			curl_easy_setopt(*curl, CURLOPT_URL, nullptr);
+			curl_easy_setopt(*curl, CURLOPT_CURLU, url);
 
 			// Set DELETE request method
 			curl_easy_setopt(*curl, CURLOPT_CUSTOMREQUEST, "DELETE");
@@ -330,6 +336,7 @@ class HTTPFSCurlClient : public HTTPClient {
 
 			// Execute DELETE request
 			res = curl->Execute();
+			curl_url_cleanup(url);
 		}
 
 		// Get HTTP response status code
@@ -352,8 +359,11 @@ class HTTPFSCurlClient : public HTTPClient {
 
 		CURLcode res;
 		{
-			auto encoded_url = EncodeSpaces(request_info->url);
-			curl_easy_setopt(*curl, CURLOPT_URL, encoded_url.c_str());
+			CURLU *url = curl_url_dup(base_url);
+			curl_url_set(url, CURLUPART_URL, info.path.c_str(), 0);
+
+			curl_easy_setopt(*curl, CURLOPT_URL, nullptr);
+			curl_easy_setopt(*curl, CURLOPT_CURLU, url);
 			if (info.send_post_as_get_request) {
 				curl_easy_setopt(*curl, CURLOPT_CUSTOMREQUEST, "GET");
 			} else {
@@ -368,6 +378,7 @@ class HTTPFSCurlClient : public HTTPClient {
 
 			// Execute POST request
 			res = curl->Execute();
+			curl_url_cleanup(url);
 		}
 
 		curl_easy_getinfo(*curl, CURLINFO_RESPONSE_CODE, &request_info->response_code);
@@ -440,6 +451,7 @@ class HTTPFSCurlClient : public HTTPClient {
 	unique_ptr<CURLHandle> curl;
 	optional_ptr<HTTPState> state;
 	unique_ptr<RequestInfo> request_info;
+	CURLU *base_url = nullptr;
 
 	static std::mutex &GetRefLock() {
 		static std::mutex mtx;

src/s3fs.cpp

@@ -1433,7 +1433,7 @@ string AWSListObjectV2::Request(const string &path, HTTPParams &http_params, S3A
 
 		// Get requests use fresh connection
 		string full_host = parsed_url.http_proto + parsed_url.host;
-		string listobjectv2_url = full_host + req_path + "?" + req_params;
+		string listobjectv2_url = req_path + "?" + req_params;
 		std::stringstream response;
 		ErrorData error;
 		GetRequestInfo get_request(

test/sql/httpfs/check_glob_error_message.test

@@ -0,0 +1,12 @@
+# name: test/sql/httpfs/check_glob_error_message.test
+# description: check error message consistency between curl and httplib
+# group: [httpfs]
+
+require httpfs
+
+require parquet
+
+statement error
+FROM 's3://coiled-datasets/timeseries/20-years/parquet/1*.parquet'
+----
+<REGEX>:.*IO Error.*