lordgamez commented on a change in pull request #1046:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1046#discussion_r610629341
##########
File path: libminifi/include/utils/HTTPClient.h
##########
@@ -362,10 +363,27 @@ class BaseHTTPClient {
virtual inline bool matches(const std::string &value, const std::string
&sregex) = 0;
};
-extern std::string get_token(utils::BaseHTTPClient *client, std::string
username, std::string password);
+std::string get_token(utils::BaseHTTPClient *client, std::string username,
std::string password);
+
+class URL {
+ public:
+ explicit URL(const std::string& url_input);
+ bool isValid() const { return is_valid_; }
+ std::string protocol() const { return protocol_; }
+ std::string host() const { return host_; }
+ int port() const;
+ std::string hostPort() const;
+ std::string toString() const;
+
+ private:
+ std::string protocol_;
+ std::string host_;
+ utils::optional<int> port_;
Review comment:
Maybe now that we don't use `-1` for invalid port this could be
unsigned, or would that break something?
##########
File path: libminifi/src/utils/HTTPClient.cpp
##########
@@ -48,72 +73,87 @@ std::string get_token(utils::BaseHTTPClient *client,
std::string username, std::
return token;
}
-void parse_url(const std::string *url, std::string *host, int *port,
std::string *protocol) {
- static std::string http("http://";);
- static std::string https("https://";);
-
- if (url->compare(0, http.size(), http) == 0)
- *protocol = http;
-
- if (url->compare(0, https.size(), https) == 0)
- *protocol = https;
-
- if (!protocol->empty()) {
- size_t pos = url->find_first_of(":", protocol->size());
+URL::URL(const std::string& url_input) {
+ if (utils::StringUtils::startsWith(url_input, HTTP)) {
+ protocol_ = HTTP;
+ } else if (utils::StringUtils::startsWith(url_input, HTTPS)) {
+ protocol_ = HTTPS;
+ } else {
+ logger_->log_error("Unknown protocol in URL '%s'", url_input);
+ return;
+ }
Review comment:
This could be extracted as a separate function `parseProtocol`
##########
File path: libminifi/test/unit/HTTPUtilTests.cpp
##########
@@ -17,51 +17,74 @@
*/
#include <string>
-#include <iostream>
#include "../TestBase.h"
#include "utils/HTTPClient.h"
-TEST_CASE("TestHTTPUtils::simple", "[test parse no port]") {
- std::string protocol, host;
- int port = -1;
- std::string url = "http://nifi.io/nifi";;
- minifi::utils::parse_url(&url, &host, &port, &protocol);
- REQUIRE(port == -1);
- REQUIRE(host == "nifi.io");
- REQUIRE(protocol == "http://";);
+TEST_CASE("The URL class can parse various URL strings", "[URL][parsing]") {
Review comment:
Great tests, easy to understand the functionality :+1:
##########
File path: libminifi/src/utils/HTTPClient.cpp
##########
@@ -15,8 +15,33 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-#include "utils/HTTPClient.h"
+#include <algorithm>
#include <string>
+
+#include "utils/HTTPClient.h"
+#include "utils/StringUtils.h"
+
+namespace {
+
+constexpr const char* HTTP = "http://";;
+constexpr const char* HTTPS = "https://";;
+
+utils::optional<int> parsePortNumber(const std::string& port_string) {
+ try {
+ size_t pos;
+ int port = std::stoi(port_string, &pos);
+ if (pos == port_string.size()) {
+ return port;
+ }
+ } catch (const std::invalid_argument&) {
+ } catch (const std::out_of_range&) {
+ }
+
+ return {};
+}
Review comment:
Just an opinion, but these might fit better as static members of the URL
class.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
