lordgamez commented on a change in pull request #1040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1040#discussion_r604739781
##########
File path: controller/MiNiFiController.cpp
##########
@@ -130,11 +128,11 @@ int main(int argc, char **argv) {
if ((IsNullOrEmpty(host) && port == -1)) {
std::cout << "MiNiFi Controller is disabled" << std::endl;
exit(0);
- } else
-
+ } else {
if (result.count("noheaders")) {
show_headers = false;
}
+ }
Review comment:
This could be merged into a single `else if`
##########
File path: controller/MiNiFiController.cpp
##########
@@ -167,9 +165,10 @@ int main(int argc, char **argv) {
socket = secure_context != nullptr ?
stream_factory_->createSecureSocket(host, port, secure_context) :
stream_factory_->createSocket(host, port);
if (getConnectionSize(std::move(socket), std::cout, connection) < 0)
std::cout << "Could not connect to remote host " << host << ":" <<
port << std::endl;
- } else
+ } else {
std::cout << "Could not connect to remote host " << host << ":" <<
port << std::endl;
}
+ }
Review comment:
The indentations do not match
##########
File path: extensions/http-curl/processors/InvokeHTTP.cpp
##########
@@ -243,12 +243,10 @@ void InvokeHTTP::onSchedule(const
std::shared_ptr<core::ProcessContext> &context
logger_->log_debug("%s attribute is missing, so default value of %s will
be used", UseChunkedEncoding.getName(), UseChunkedEncoding.getValue());
}
- utils::StringUtils::StringToBool(useChunkedEncoding, use_chunked_encoding_);
+ use_chunked_encoding_ =
utils::StringUtils::toBool(useChunkedEncoding).value_or(false);
- std::string disablePeerVerification = "false";
- if (context->getProperty(DisablePeerVerification.getName(),
disablePeerVerification)) {
- utils::StringUtils::StringToBool(disablePeerVerification,
disable_peer_verification_);
- }
+ std::string disablePeerVerification;
+ disable_peer_verification_ =
(context->getProperty(DisablePeerVerification.getName(),
disablePeerVerification) &&
utils::StringUtils::toBool(disablePeerVerification).value_or(false));
Review comment:
I don't think the outer braces are necessary here.
##########
File path: extensions/opc/src/putopc.cpp
##########
@@ -301,8 +300,9 @@ namespace processors {
break;
}
case opc::OPCNodeDataType::Boolean: {
- bool value;
- if (utils::StringUtils::StringToBool(contentstr, value)) {
+ utils::optional<bool> contentstr_parsed =
utils::StringUtils::toBool(contentstr);
+ if (contentstr_parsed) {
+ bool value = contentstr_parsed.value();
Review comment:
I think the local variable is not needed here, this could merged with
the next line.
##########
File path: extensions/http-curl/processors/InvokeHTTP.cpp
##########
@@ -206,7 +206,7 @@ void InvokeHTTP::onSchedule(const
std::shared_ptr<core::ProcessContext> &context
logger_->log_debug("%s attribute is missing, so default value of %s will
be used", DateHeader.getName(), DateHeader.getValue());
}
- date_header_include_ = utils::StringUtils::StringToBool(dateHeaderStr,
date_header_include_);
+ date_header_include_ =
utils::StringUtils::toBool(dateHeaderStr).value_or(DateHeader.getValue());
Review comment:
Some unnecessary whitespace was added
##########
File path: extensions/standard-processors/tests/unit/GetFileTests.cpp
##########
@@ -29,6 +29,28 @@
#include <fileapi.h>
#endif
+void checkLoggedFiles(TestController testController, std::shared_ptr<TestPlan>
plan, std::shared_ptr<core::Processor> get_file ) {
+ char in_dir[] = "/tmp/gt.XXXXXX";
+ auto temp_path = testController.createTempDirectory(in_dir);
Review comment:
You could use `minifi::utils::createTempDir` instead then the format
would not be needed in this case.
##########
File path: extensions/standard-processors/processors/HashContent.cpp
##########
@@ -48,6 +48,7 @@ void HashContent::initialize() {
std::set<core::Property> properties;
properties.insert(HashAttribute);
properties.insert(HashAlgorithm);
+ properties.insert(FailOnEmpty);
Review comment:
Good catch :+1:
##########
File path: extensions/pcap/CapturePacket.cpp
##########
@@ -70,19 +70,17 @@ std::string CapturePacket::generate_new_pcap(const
std::string &base_path) {
void CapturePacket::packet_callback(pcpp::RawPacket* packet,
pcpp::PcapLiveDevice* /*dev*/, void* data) {
// parse the packet
- PacketMovers* capture_mechanism = (PacketMovers*) data;
+ PacketMovers* capture_mechanism = reinterpret_cast <PacketMovers*> (data);
Review comment:
Same as before, I would avoid whitespaces
`reinterpret_cast<PacketMovers*>(data);`
##########
File path: extensions/standard-processors/tests/unit/HashContentTest.cpp
##########
@@ -131,4 +131,36 @@ TEST_CASE("Test usage of ExtractText",
"[extracttextTest]") {
REQUIRE(LogTestController::getInstance().contains(log_check));
}
+TEST_CASE("TestingFailOnEmptyProperty", "[HashContentPropertiesCheck]") {
+ TestController testController;
+
LogTestController::getInstance().setTrace<org::apache::nifi::minifi::processors::LogAttribute>();
+
LogTestController::getInstance().setTrace<org::apache::nifi::minifi::processors::GetFile>();
+ LogTestController::getInstance().setTrace<core::ProcessSession>();
+
LogTestController::getInstance().setTrace<org::apache::nifi::minifi::processors::HashContent>();
+ std::shared_ptr<TestPlan> plan = testController.createPlan();
+ std::shared_ptr<TestRepository> repo = std::make_shared<TestRepository>();
+
+ char dir[] = "/tmp/gt.XXXXXX";
+ auto tempdir = testController.createTempDirectory(dir);
Review comment:
`minifi::utils::createTempDir` could be used here as well.
##########
File path: extensions/http-curl/tests/unit/InvokeHTTPTests.cpp
##########
@@ -37,12 +37,12 @@
#include "core/ProcessSession.h"
#include "core/ProcessorNode.h"
#include "processors/InvokeHTTP.h"
-#include "processors/ListenHTTP.h"
#include "processors/LogAttribute.h"
#include "utils/gsl.h"
TEST_CASE("HTTPTestsWithNoResourceClaimPOST", "[httptest1]") {
TestController testController;
+ // TODO(aminadinari19): fix this test
Review comment:
Does this test still fail? What's the problem with it?
##########
File path: extensions/mqtt/processors/AbstractMQTTProcessor.cpp
##########
@@ -146,7 +148,7 @@ void AbstractMQTTProcessor::onSchedule(const
std::shared_ptr<core::ProcessContex
MQTTClient_create(&client_, uri_.c_str(), clientID_.c_str(),
MQTTCLIENT_PERSISTENCE_NONE, NULL);
}
if (client_) {
- MQTTClient_setCallbacks(client_, (void *) this, connectionLost,
msgReceived, msgDelivered);
+ MQTTClient_setCallbacks(client_, reinterpret_cast<void *> (this),
connectionLost, msgReceived, msgDelivered);
Review comment:
I would avoid whitespaces in the cast like `reinterpret_cast<void
*>(this)` or `reinterpret_cast<void*>(this)`
##########
File path: extensions/opc/src/putopc.cpp
##########
@@ -378,8 +378,9 @@ namespace processors {
break;
}
case opc::OPCNodeDataType::Boolean: {
- bool value;
- if (utils::StringUtils::StringToBool(contentstr, value)) {
+ utils::optional<bool> contentstr_parsed =
utils::StringUtils::toBool(contentstr);
+ if (contentstr_parsed) {
+ bool value = contentstr_parsed.value();
Review comment:
Same as before, this could be merged with the next line.
##########
File path: extensions/standard-processors/tests/unit/PutFileTests.cpp
##########
@@ -467,4 +467,39 @@ TEST_CASE("TestPutFilePermissions",
"[PutFilePermissions]") {
REQUIRE(utils::file::FileUtils::get_permissions(putfiledir, perms));
REQUIRE(perms == 0777);
}
+
+TEST_CASE("PutFileCreateDirectoryTest", "[PutFilePermissions]") {
+ TestController testController;
+ LogTestController::getInstance().setDebug<minifi::processors::GetFile>();
+ LogTestController::getInstance().setDebug<TestPlan>();
+ LogTestController::getInstance().setDebug<minifi::processors::PutFile>();
+
LogTestController::getInstance().setDebug<minifi::processors::PutFile::ReadCallback>();
+
LogTestController::getInstance().setDebug<minifi::processors::LogAttribute>();
+
+ std::shared_ptr<TestPlan> plan = testController.createPlan();
+ std::shared_ptr<core::Processor> getfile = plan->addProcessor("GetFile",
"getfileCreate2");
+ std::shared_ptr<core::Processor> putfile = plan->addProcessor("PutFile",
"putfile", core::Relationship("success", "description"), true);
+ plan->addProcessor("LogAttribute", "logattribute",
core::Relationship("success", "description"), true);
+
+ // Define Directory
+ char format[] = "/tmp/gt.XXXXXX";
+ auto dir = testController.createTempDirectory(format);
+ char format2[] = "/tmp/ft.XXXXXX";
+ // Defining a sub directory
+ auto putfiledir = testController.createTempDirectory(format2) +
utils::file::FileUtils::get_separator() + "test_dir";
Review comment:
`minifi::utils::createTempDir` could be used here as well. I think the
same format can be used for the temp directory as it is randomized.
##########
File path: extensions/standard-processors/tests/unit/HashContentTest.cpp
##########
@@ -131,4 +131,36 @@ TEST_CASE("Test usage of ExtractText",
"[extracttextTest]") {
REQUIRE(LogTestController::getInstance().contains(log_check));
}
+TEST_CASE("TestingFailOnEmptyProperty", "[HashContentPropertiesCheck]") {
+ TestController testController;
+
LogTestController::getInstance().setTrace<org::apache::nifi::minifi::processors::LogAttribute>();
+
LogTestController::getInstance().setTrace<org::apache::nifi::minifi::processors::GetFile>();
+ LogTestController::getInstance().setTrace<core::ProcessSession>();
+
LogTestController::getInstance().setTrace<org::apache::nifi::minifi::processors::HashContent>();
+ std::shared_ptr<TestPlan> plan = testController.createPlan();
+ std::shared_ptr<TestRepository> repo = std::make_shared<TestRepository>();
+
+ char dir[] = "/tmp/gt.XXXXXX";
+ auto tempdir = testController.createTempDirectory(dir);
+ std::shared_ptr<core::Processor> getfile = plan->addProcessor("GetFile",
"getfileCreate2");
+ plan->setProperty(getfile,
org::apache::nifi::minifi::processors::GetFile::Directory.getName(), tempdir);
+ plan->setProperty(getfile,
org::apache::nifi::minifi::processors::GetFile::KeepSourceFile.getName(),
"true");
+
+ std::shared_ptr<core::Processor> md5processor =
plan->addProcessor("HashContent", "HashContentMD5",
+
core::Relationship("success", "description"), true);
+ plan->setProperty(md5processor,
org::apache::nifi::minifi::processors::HashContent::HashAttribute.getName(),
MD5_ATTR);
+ plan->setProperty(md5processor,
org::apache::nifi::minifi::processors::HashContent::HashAlgorithm.getName(),
"MD5");
+ plan->setProperty(md5processor,
org::apache::nifi::minifi::processors::HashContent::FailOnEmpty.getName(),
"true");
+
+ std::stringstream ss1;
Review comment:
I would rename this to something more specific.
--
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:
[email protected]
