martinzink commented on code in PR #1383:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1383#discussion_r954898078


##########
extensions/http-curl/client/HTTPClient.cpp:
##########
@@ -163,212 +160,228 @@ bool HTTPClient::setSpecificSSLVersion(SSLVersion 
specific_version) {
 }
 
 // If not set, the default will be TLS 1.0, see 
https://curl.haxx.se/libcurl/c/CURLOPT_SSLVERSION.html
-bool HTTPClient::setMinimumSSLVersion(SSLVersion minimum_version) {
+bool HTTPClient::setMinimumSSLVersion(utils::SSLVersion minimum_version) {
   CURLcode ret = CURLE_UNKNOWN_OPTION;
   switch (minimum_version) {
-    case SSLVersion::TLSv1_0:
-      ret = curl_easy_setopt(http_session_, CURLOPT_SSLVERSION, 
CURL_SSLVERSION_TLSv1_0);
+    case utils::SSLVersion::TLSv1_0:
+      ret = curl_easy_setopt(http_session_.get(), CURLOPT_SSLVERSION, 
CURL_SSLVERSION_TLSv1_0);
       break;
-    case SSLVersion::TLSv1_1:
-      ret = curl_easy_setopt(http_session_, CURLOPT_SSLVERSION, 
CURL_SSLVERSION_TLSv1_1);
+    case utils::SSLVersion::TLSv1_1:
+      ret = curl_easy_setopt(http_session_.get(), CURLOPT_SSLVERSION, 
CURL_SSLVERSION_TLSv1_1);
       break;
-    case SSLVersion::TLSv1_2:
-      ret = curl_easy_setopt(http_session_, CURLOPT_SSLVERSION, 
CURL_SSLVERSION_TLSv1_2);
+    case utils::SSLVersion::TLSv1_2:
+      ret = curl_easy_setopt(http_session_.get(), CURLOPT_SSLVERSION, 
CURL_SSLVERSION_TLSv1_2);
       break;
   }
 
   return ret == CURLE_OK;
 }
 
-DEPRECATED(/*deprecated in*/ 0.8.0, /*will remove in */ 2.0) void 
HTTPClient::setConnectionTimeout(int64_t timeout) {
-  setConnectionTimeout(std::chrono::milliseconds(timeout * 1000));
-}
-
-DEPRECATED(/*deprecated in*/ 0.8.0, /*will remove in */ 2.0) void 
HTTPClient::setReadTimeout(int64_t timeout) {
-  setReadTimeout(std::chrono::milliseconds(timeout * 1000));
+void HTTPClient::setKeepAliveProbe(std::optional<KeepAliveProbeData> 
probe_data) {
+  if (probe_data) {
+    curl_easy_setopt(http_session_.get(), CURLOPT_TCP_KEEPALIVE, true);
+    curl_easy_setopt(http_session_.get(), CURLOPT_TCP_KEEPINTVL, 
probe_data->keep_alive_interval.count());
+    curl_easy_setopt(http_session_.get(), CURLOPT_TCP_KEEPIDLE, 
probe_data->keep_alive_delay.count());
+  } else {
+    curl_easy_setopt(http_session_.get(), CURLOPT_TCP_KEEPALIVE, false);
+  }
 }
 
 void HTTPClient::setConnectionTimeout(std::chrono::milliseconds timeout) {
-  connect_timeout_ms_ = timeout;
+  if (timeout < 0ms) {
+    logger_->log_error("Invalid timeout");
+    return;
+  }
+  connect_timeout_ = timeout;
 }
 
 void HTTPClient::setReadTimeout(std::chrono::milliseconds timeout) {
-  read_timeout_ms_ = timeout;
+  if (timeout < 0ms) {
+    logger_->log_error("Invalid timeout");
+    return;
+  }
+  read_timeout_ = timeout;
 }
 
-void HTTPClient::setReadCallback(HTTPReadCallback *callbackObj) {
-  callback = callbackObj;
-  curl_easy_setopt(http_session_, CURLOPT_WRITEFUNCTION, 
&utils::HTTPRequestResponse::recieve_write);
-  curl_easy_setopt(http_session_, CURLOPT_WRITEDATA, 
static_cast<void*>(callbackObj));
+void HTTPClient::setReadCallback(std::unique_ptr<utils::HTTPReadCallback> 
callback) {
+  read_callback_ = std::move(callback);
+  curl_easy_setopt(http_session_.get(), CURLOPT_WRITEFUNCTION, 
&utils::HTTPRequestResponse::recieve_write);
+  curl_easy_setopt(http_session_.get(), CURLOPT_WRITEDATA, 
static_cast<void*>(read_callback_.get()));
 }
 
-void HTTPClient::setUploadCallback(HTTPUploadCallback *callbackObj) {
+void HTTPClient::setUploadCallback(std::unique_ptr<utils::HTTPUploadCallback> 
callback) {
   logger_->log_debug("Setting callback for %s", url_);
-  write_callback_ = callbackObj;
-  if (method_ == "put" || method_ == "PUT") {
-    curl_easy_setopt(http_session_, CURLOPT_INFILESIZE_LARGE, (curl_off_t) 
callbackObj->ptr->getBufferSize());
+  write_callback_ = std::move(callback);
+  if (method_ == "PUT") {
+    curl_easy_setopt(http_session_.get(), CURLOPT_INFILESIZE_LARGE, 
(curl_off_t) write_callback_->getBufferSize());
   }
-  curl_easy_setopt(http_session_, CURLOPT_READFUNCTION, 
&utils::HTTPRequestResponse::send_write);
-  curl_easy_setopt(http_session_, CURLOPT_READDATA, 
static_cast<void*>(callbackObj));
-}
-
-void HTTPClient::setSeekFunction(HTTPUploadCallback *callbackObj) {
-  curl_easy_setopt(http_session_, CURLOPT_SEEKDATA, 
static_cast<void*>(callbackObj));
-  curl_easy_setopt(http_session_, CURLOPT_SEEKFUNCTION, 
&utils::HTTPRequestResponse::seek_callback);
+  curl_easy_setopt(http_session_.get(), CURLOPT_READFUNCTION, 
&utils::HTTPRequestResponse::send_write);
+  curl_easy_setopt(http_session_.get(), CURLOPT_READDATA, 
static_cast<void*>(write_callback_.get()));
+  curl_easy_setopt(http_session_.get(), CURLOPT_SEEKDATA, 
static_cast<void*>(write_callback_.get()));
+  curl_easy_setopt(http_session_.get(), CURLOPT_SEEKFUNCTION, 
&utils::HTTPRequestResponse::seek_callback);
 }
 
 void HTTPClient::setContentType(std::string content_type) {
-  content_type_ = "Content-Type: " + content_type;
-  headers_ = curl_slist_append(headers_, content_type_.c_str());
+  request_headers_["Content-Type"] = std::move(content_type);
 }
 
 std::string HTTPClient::escape(std::string string_to_escape) {
   struct curl_deleter { void operator()(void* p) noexcept { curl_free(p); } };
-  std::unique_ptr<char, curl_deleter> 
escaped_chars{curl_easy_escape(http_session_, string_to_escape.c_str(), 
gsl::narrow<int>(string_to_escape.length()))};
+  std::unique_ptr<char, curl_deleter> 
escaped_chars{curl_easy_escape(http_session_.get(), string_to_escape.c_str(), 
gsl::narrow<int>(string_to_escape.length()))};
   std::string escaped_string(escaped_chars.get());
   return escaped_string;
 }
 
 void HTTPClient::setPostFields(const std::string& input) {
-  curl_easy_setopt(http_session_, CURLOPT_POSTFIELDSIZE, input.length());
-  curl_easy_setopt(http_session_, CURLOPT_COPYPOSTFIELDS, input.c_str());
+  curl_easy_setopt(http_session_.get(), CURLOPT_POSTFIELDSIZE, input.length());
+  curl_easy_setopt(http_session_.get(), CURLOPT_COPYPOSTFIELDS, input.c_str());
 }
 
 void HTTPClient::setPostSize(size_t size) {
-  curl_easy_setopt(http_session_, CURLOPT_POSTFIELDSIZE, size);
+  curl_easy_setopt(http_session_.get(), CURLOPT_POSTFIELDSIZE, size);
 }
 
-void HTTPClient::setHeaders(struct curl_slist *list) {
-  headers_ = list;
+void HTTPClient::setHTTPProxy(const utils::HTTPProxy &proxy) {
+  if (!proxy.host.empty()) {
+    curl_easy_setopt(http_session_.get(), CURLOPT_PROXY, proxy.host.c_str());
+    curl_easy_setopt(http_session_.get(), CURLOPT_PROXYPORT, proxy.port);
+    if (!proxy.username.empty()) {
+      curl_easy_setopt(http_session_.get(), CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+      std::string value = proxy.username + ":" + proxy.password;
+      curl_easy_setopt(http_session_.get(), CURLOPT_PROXYUSERPWD, 
value.c_str());
+    }
+  }
 }
 
-void HTTPClient::appendHeader(const std::string &new_header) {
-  headers_ = curl_slist_append(headers_, new_header.c_str());
+void HTTPClient::setRequestHeader(std::string key, std::optional<std::string> 
value) {
+  if (value)
+    request_headers_[std::move(key)] = std::move(*value);
+  else
+    request_headers_.erase(key);
 }
 
-void HTTPClient::appendHeader(const std::string &key, const std::string 
&value) {
-  headers_ = curl_slist_append(headers_, utils::StringUtils::join_pack(key, ": 
", value).c_str());
-}
+namespace {
+struct CurlSListFreeAll {
+  void operator()(struct curl_slist* slist) const {
+    curl_slist_free_all(slist);
+  }
+};
+
+std::unique_ptr<struct curl_slist, CurlSListFreeAll> getCurlSList(const 
std::unordered_map<std::string, std::string>& request_headers) {
+  curl_slist* new_list = nullptr;
+  for (const auto& [header_key, header_value] : request_headers)
+    new_list = curl_slist_append(new_list, 
utils::StringUtils::join_pack(header_key, ": ", header_value).c_str());
 
-void HTTPClient::setUseChunkedEncoding() {
-  headers_ = curl_slist_append(headers_, "Transfer-Encoding: chunked");
+  return {new_list, {}};
 }
+}  // namespace
+
 
 bool HTTPClient::submit() {
-  if (IsNullOrEmpty(url_))
+  if (url_.empty()) {
+    logger_->log_error("Tried to submit to an empty url");
     return false;
+  }
 
-  int absoluteTimeout = std::max(0, 3 * 
static_cast<int>(read_timeout_ms_.count()));
+  response_data_.clear();
 
-  curl_easy_setopt(http_session_, CURLOPT_NOSIGNAL, 1);
-  // setting it to 0 will result in the default 300 second timeout
-  curl_easy_setopt(http_session_, CURLOPT_CONNECTTIMEOUT_MS, std::max(0, 
static_cast<int>(connect_timeout_ms_.count())));
-  curl_easy_setopt(http_session_, CURLOPT_TIMEOUT_MS, absoluteTimeout);
+  curl_easy_setopt(http_session_.get(), CURLOPT_NOSIGNAL, 1);
+  curl_easy_setopt(http_session_.get(), CURLOPT_CONNECTTIMEOUT_MS, 
connect_timeout_.count());
+  curl_easy_setopt(http_session_.get(), CURLOPT_TIMEOUT_MS, 
getAbsoluteTimeout().count());
 
-  if (read_timeout_ms_.count() > 0) {
+  if (read_timeout_ > 0ms) {
     progress_.reset();
-    curl_easy_setopt(http_session_, CURLOPT_NOPROGRESS, 0);
-    curl_easy_setopt(http_session_, CURLOPT_XFERINFOFUNCTION, onProgress);
-    curl_easy_setopt(http_session_, CURLOPT_XFERINFODATA, this);
+    curl_easy_setopt(http_session_.get(), CURLOPT_NOPROGRESS, 0);
+    curl_easy_setopt(http_session_.get(), CURLOPT_XFERINFOFUNCTION, 
onProgress);
+    curl_easy_setopt(http_session_.get(), CURLOPT_XFERINFODATA, this);
   } else {
     // the user explicitly set it to 0
-    curl_easy_setopt(http_session_, CURLOPT_NOPROGRESS, 1);
-  }
-  if (headers_ != nullptr) {
-    headers_ = curl_slist_append(headers_, "Expect:");
-    curl_easy_setopt(http_session_, CURLOPT_HTTPHEADER, headers_);
+    curl_easy_setopt(http_session_.get(), CURLOPT_NOPROGRESS, 1);
   }
 
-  if (username_password_) {
-    curl_easy_setopt(http_session_, CURLOPT_USERNAME, 
username_password_->username.c_str());
-    curl_easy_setopt(http_session_, CURLOPT_PASSWORD, 
username_password_->password.c_str());
+  auto headers = getCurlSList(request_headers_);
+  if (headers) {
+    curl_slist_append(headers.get(), "Expect:");
+    curl_easy_setopt(http_session_.get(), CURLOPT_HTTPHEADER, headers.get());
   }
 
-  curl_easy_setopt(http_session_, CURLOPT_URL, url_.c_str());
+  curl_easy_setopt(http_session_.get(), CURLOPT_URL, url_.c_str());
   logger_->log_debug("Submitting to %s", url_);
-  if (callback == nullptr) {
-    content_.ptr = &read_callback_;
-    curl_easy_setopt(http_session_, CURLOPT_WRITEFUNCTION, 
&utils::HTTPRequestResponse::recieve_write);
-    curl_easy_setopt(http_session_, CURLOPT_WRITEDATA, 
static_cast<void*>(&content_));
-  }
-  curl_easy_setopt(http_session_, CURLOPT_HEADERFUNCTION, 
&utils::HTTPHeaderResponse::receive_headers);
-  curl_easy_setopt(http_session_, CURLOPT_HEADERDATA, 
static_cast<void*>(&header_response_));
-  if (keep_alive_probe_.count() > 0) {
-    const auto keepAlive = 
std::chrono::duration_cast<std::chrono::seconds>(keep_alive_probe_);
-    const auto keepIdle = 
std::chrono::duration_cast<std::chrono::seconds>(keep_alive_idle_);
-    logger_->log_debug("Setting keep alive to %" PRId64 " seconds", 
keepAlive.count());
-    curl_easy_setopt(http_session_, CURLOPT_TCP_KEEPALIVE, 1L);
-    curl_easy_setopt(http_session_, CURLOPT_TCP_KEEPINTVL, keepAlive.count());
-    curl_easy_setopt(http_session_, CURLOPT_TCP_KEEPIDLE, keepIdle.count());
-  } else {
-    logger_->log_debug("Not using keep alive");
-    curl_easy_setopt(http_session_, CURLOPT_TCP_KEEPALIVE, 0L);
+  if (read_callback_ == nullptr) {
+    curl_easy_setopt(http_session_.get(), CURLOPT_WRITEFUNCTION, 
&utils::HTTPRequestResponse::recieve_write);
+    curl_easy_setopt(http_session_.get(), CURLOPT_WRITEDATA, 
static_cast<void*>(&content_));
   }
+
+  curl_easy_setopt(http_session_.get(), CURLOPT_HEADERFUNCTION, 
&utils::HTTPHeaderResponse::receive_headers);
+  curl_easy_setopt(http_session_.get(), CURLOPT_HEADERDATA, 
static_cast<void*>(&response_data_.header_response));
+
   if (form_ != nullptr) {
-    curl_easy_setopt(http_session_, CURLOPT_MIMEPOST, form_);
-  }
-  res = curl_easy_perform(http_session_);
-  if (callback == nullptr) {
-    read_callback_.close();
-  }
-  long http_code;  // NOLINT long due to libcurl API
-  curl_easy_getinfo(http_session_, CURLINFO_RESPONSE_CODE, &http_code);
-  http_code_ = http_code;
-  curl_easy_getinfo(http_session_, CURLINFO_CONTENT_TYPE, &content_type_str_);
-  if (res == CURLE_OPERATION_TIMEDOUT) {
-    logger_->log_error("HTTP operation timed out, with absolute timeout 
%dms\n", absoluteTimeout);
-  }
-  if (res != CURLE_OK) {
-    logger_->log_error("curl_easy_perform() failed %s on %s, error code %d\n", 
curl_easy_strerror(res), url_, res);
+    curl_easy_setopt(http_session_.get(), CURLOPT_MIMEPOST, form_.get());
+  }
+  res_ = curl_easy_perform(http_session_.get());
+  if (read_callback_ == nullptr) {
+    content_.close();
+  }
+  long http_code;  // NOLINT(runtime/int) long due to libcurl API
+  curl_easy_getinfo(http_session_.get(), CURLINFO_RESPONSE_CODE, &http_code);
+  response_data_.response_code = http_code;
+  curl_easy_getinfo(http_session_.get(), CURLINFO_CONTENT_TYPE, 
&response_data_.response_content_type);
+  if (res_ == CURLE_OPERATION_TIMEDOUT) {
+    logger_->log_error("HTTP operation timed out, with absolute timeout %" 
PRId64 "ms\n", getAbsoluteTimeout().count());
+  }
+  if (res_ != CURLE_OK) {
+    logger_->log_info("%d", request_headers_.size());
+    logger_->log_error("curl_easy_perform() failed %s on %s, error code %d\n", 
curl_easy_strerror(res_), url_, res_);
     return false;
   }
 
   logger_->log_debug("Finished with %s", url_);
   return true;
 }
 
-CURLcode HTTPClient::getResponseResult() {
-  return res;
-}
-
 int64_t HTTPClient::getResponseCode() const {
-  return http_code_;
+  return response_data_.response_code;
 }
 
 const char *HTTPClient::getContentType() {
-  return content_type_str_;
+  return response_data_.response_content_type;
 }
 
 const std::vector<char> &HTTPClient::getResponseBody() {
-  if (response_body_.empty()) {
-    if (callback && callback->ptr) {
-      response_body_ = callback->ptr->to_string();
+  if (response_data_.response_body.empty()) {
+    if (read_callback_) {
+      response_data_.response_body = read_callback_->to_string();
     } else {
-      response_body_ = read_callback_.to_string();
+      response_data_.response_body = content_.to_string();
     }
   }
-  return response_body_;
-}
-
-void HTTPClient::set_request_method(const std::string method) {
-  std::string my_method = method;
-  std::transform(my_method.begin(), my_method.end(), my_method.begin(), 
::toupper);
-  if (my_method == "POST") {
-    curl_easy_setopt(http_session_, CURLOPT_POST, 1L);
-  } else if (my_method == "PUT") {
-    curl_easy_setopt(http_session_, CURLOPT_UPLOAD, 1L);
-  } else if (my_method == "HEAD") {
-    curl_easy_setopt(http_session_, CURLOPT_NOBODY, 1L);
-  } else if (my_method == "GET") {
+  return response_data_.response_body;
+}
+
+void HTTPClient::set_request_method(std::string method) {
+  if (method_ == method)
+    return;
+  method_ = std::move(method);
+  std::transform(method_.begin(), method_.end(), method_.begin(), ::toupper);

Review Comment:
   not nitpicking at all, thanks I've included this in 
https://github.com/apache/nifi-minifi-cpp/pull/1383/commits/044ce269808a28e5a6eaeca34fb30747426a6ab7#diff-e83ea756e38995b3194ecc99d0a64fc19c8ef992fb7a575a34def27a67f7819aR362



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to