fgerlits commented on code in PR #1424:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1424#discussion_r1039600994
##########
extensions/standard-processors/tests/unit/TailFileTests.cpp:
##########
@@ -99,18 +89,16 @@ TEST_CASE("TailFile reads the file until the first
delimiter", "[simple]") {
plan->addProcessor("LogAttribute", "logattribute",
core::Relationship("success", "description"), true);
auto dir = testController.createTempDirectory();
- std::stringstream temp_file;
- temp_file << dir << utils::file::get_separator() << TMP_FILE;
+ auto temp_file_path = dir / TMP_FILE;
std::ofstream tmpfile;
- tmpfile.open(temp_file.str(), std::ios::out | std::ios::binary);
+ tmpfile.open(temp_file_path, std::ios::out | std::ios::binary);
tmpfile << NEWLINE_FILE;
tmpfile.close();
- std::stringstream state_file;
- state_file << dir << utils::file::get_separator() << STATE_FILE;
+ auto state_file_path = dir / STATE_FILE;
Review Comment:
this is unused, could be removed
##########
libminifi/test/rocksdb-tests/RocksDBTests.cpp:
##########
@@ -232,9 +231,9 @@ void
withDefaultEnv(minifi::internal::Writable<rocksdb::DBOptions>& db_opts) {
}
TEST_CASE_METHOD(RocksDBTest, "Error is logged if different encryption keys
are used", "[rocksDBTest10]") {
- utils::Path home_dir{createTempDirectory()};
- utils::file::FileUtils::create_dir((home_dir / "conf").str());
- std::ofstream{(home_dir / "conf" / "bootstrap.conf").str()}
+ auto home_dir = createTempDirectory();
+ utils::file::FileUtils::create_dir((home_dir / "conf"));
+ std::ofstream{(home_dir / "conf" / "bootstrap.conf")}
Review Comment:
parens could be removed here, too
##########
minifi_main/MainHelper.cpp:
##########
@@ -133,43 +128,41 @@ std::string determineMinifiHome(const
std::shared_ptr<logging::Logger>& logger)
return "";
}();
- if (minifiHome.empty()) {
+ if (minifi_home.empty()) {
logger->log_error("No " MINIFI_HOME_ENV_KEY " could be inferred. "
"Please set " MINIFI_HOME_ENV_KEY " or run minifi from a
valid location.");
return "";
}
/* Verify that MINIFI_HOME is valid */
- bool minifiHomeValid = false;
- if (validHome(minifiHome)) {
- minifiHomeValid = true;
+ bool minifi_home_is_valid = false;
+ if (validHome(minifi_home)) {
+ minifi_home_is_valid = true;
} else {
- logger->log_info("%s is not a valid " MINIFI_HOME_ENV_KEY ", because there
is no " DEFAULT_NIFI_PROPERTIES_FILE " file in it.", minifiHome);
-
- std::string minifiHomeWithoutBin;
- std::string binDir;
- std::tie(minifiHomeWithoutBin, binDir) =
minifi::utils::file::split_path(minifiHome);
- if (!minifiHomeWithoutBin.empty() && (binDir == "bin" || binDir ==
std::string("bin") + minifi::utils::file::get_separator())) {
- if (validHome(minifiHomeWithoutBin)) {
- logger->log_info("%s is a valid " MINIFI_HOME_ENV_KEY ", falling back
to it.", minifiHomeWithoutBin);
- minifiHomeValid = true;
- minifiHome = std::move(minifiHomeWithoutBin);
+ logger->log_info("%s is not a valid %s, because there is no %s file in
it.", minifi_home.string(), MINIFI_HOME_ENV_KEY,
DEFAULT_NIFI_PROPERTIES_FILE.string());
+
+ auto minifi_home_without_bin = minifi_home.parent_path();
+ auto bin_dir = minifi_home.filename();
+ if (!minifi_home_without_bin.empty() && (bin_dir ==
std::filesystem::path("bin") || bin_dir == (std::filesystem::path("bin")/""))) {
Review Comment:
I don't think it's possible for `minifi_home.filename()` to end in a
separator, so
```suggestion
if (!minifi_home_without_bin.empty() && bin_dir ==
std::filesystem::path("bin")) {
```
##########
libminifi/include/core/ConfigurableComponent.h:
##########
@@ -59,6 +59,18 @@ class ConfigurableComponent {
template<typename T>
bool getProperty(const std::string name, T &value) const;
+ template<typename T = std::string>
+ std::enable_if_t<std::is_default_constructible<T>::value, std::optional<T>>
+ getProperty(const std::string& property_name) const {
Review Comment:
can we use concepts now?
```suggestion
template<std::default_initializable T = std::string>
std::optional<T> getProperty(const std::string& property_name) const {
```
would be nicer
##########
libminifi/test/rocksdb-tests/EncryptionTests.cpp:
##########
@@ -77,8 +75,8 @@ TEST_CASE_METHOD(FFRepoFixture, "FlowFileRepository creates
checkpoint and loads
// pass
}
SECTION("With encryption") {
- utils::file::FileUtils::create_dir((home_ / "conf").str());
- std::ofstream{(home_ / "conf" / "bootstrap.conf").str()}
+ utils::file::FileUtils::create_dir((home_ / "conf"));
+ std::ofstream{(home_ / "conf" / "bootstrap.conf")}
Review Comment:
nitpicking, but these parens are not needed any longer:
```suggestion
utils::file::FileUtils::create_dir(home_ / "conf");
std::ofstream{home_ / "conf" / "bootstrap.conf"}
```
##########
libminifi/src/core/ProcessSessionReadCallback.cpp:
##########
@@ -22,18 +22,19 @@
#include <cstdio>
#include <memory>
#include <string>
+#include <utility>
#include "core/logging/LoggerConfiguration.h"
#include "utils/gsl.h"
namespace org::apache::nifi::minifi::core {
-ProcessSessionReadCallback::ProcessSessionReadCallback(const std::string
&tmpFile,
- std::string destFile,
+ProcessSessionReadCallback::ProcessSessionReadCallback(std::filesystem::path
tmpFile,
+ std::filesystem::path
destFile,
std::shared_ptr<logging::Logger> logger)
: logger_(std::move(logger)),
_tmpFileOs(tmpFile, std::ios::binary),
- _tmpFile(tmpFile),
+ _tmpFile(std::move(tmpFile)),
_destFile(std::move(destFile)) {
Review Comment:
this is old code, but can we rename these to `tmpFileOs_` etc, please?
##########
minifi_main/MainHelper.cpp:
##########
@@ -133,43 +128,41 @@ std::string determineMinifiHome(const
std::shared_ptr<logging::Logger>& logger)
return "";
}();
- if (minifiHome.empty()) {
+ if (minifi_home.empty()) {
logger->log_error("No " MINIFI_HOME_ENV_KEY " could be inferred. "
"Please set " MINIFI_HOME_ENV_KEY " or run minifi from a
valid location.");
return "";
}
/* Verify that MINIFI_HOME is valid */
- bool minifiHomeValid = false;
- if (validHome(minifiHome)) {
- minifiHomeValid = true;
+ bool minifi_home_is_valid = false;
+ if (validHome(minifi_home)) {
+ minifi_home_is_valid = true;
} else {
- logger->log_info("%s is not a valid " MINIFI_HOME_ENV_KEY ", because there
is no " DEFAULT_NIFI_PROPERTIES_FILE " file in it.", minifiHome);
-
- std::string minifiHomeWithoutBin;
- std::string binDir;
- std::tie(minifiHomeWithoutBin, binDir) =
minifi::utils::file::split_path(minifiHome);
- if (!minifiHomeWithoutBin.empty() && (binDir == "bin" || binDir ==
std::string("bin") + minifi::utils::file::get_separator())) {
- if (validHome(minifiHomeWithoutBin)) {
- logger->log_info("%s is a valid " MINIFI_HOME_ENV_KEY ", falling back
to it.", minifiHomeWithoutBin);
- minifiHomeValid = true;
- minifiHome = std::move(minifiHomeWithoutBin);
+ logger->log_info("%s is not a valid %s, because there is no %s file in
it.", minifi_home.string(), MINIFI_HOME_ENV_KEY,
DEFAULT_NIFI_PROPERTIES_FILE.string());
+
+ auto minifi_home_without_bin = minifi_home.parent_path();
+ auto bin_dir = minifi_home.filename();
+ if (!minifi_home_without_bin.empty() && (bin_dir ==
std::filesystem::path("bin") || bin_dir == (std::filesystem::path("bin")/""))) {
+ if (validHome(minifi_home_without_bin)) {
+ logger->log_info("%s is a valid %s, falling back to it.",
minifi_home_without_bin.string());
Review Comment:
the second `%s` has no target:
```suggestion
logger->log_info("%s is a valid %s, falling back to it.",
minifi_home_without_bin.string(), MINIFI_HOME_ENV_KEY);
```
##########
libminifi/test/unit/FileStreamTests.cpp:
##########
@@ -301,32 +267,25 @@ TEST_CASE("Existing file read/write test") {
stream.seek(0);
}
-#if !defined(WIN32) || defined(USE_BOOST)
-// This could be simplified with C++17 std::filesystem
TEST_CASE("Opening file without permission creates error logs") {
TestController test_controller;
auto dir = test_controller.createTempDirectory();
- std::string path_to_permissionless_file(utils::file::concat_path(dir,
"permissionless_file.txt"));
+ auto path_to_permissionless_file = dir / "permissionless_file.txt";
{
std::ofstream outfile(path_to_permissionless_file);
outfile << "this file has been just created" << std::endl;
outfile.close();
-#ifndef WIN32
utils::file::FileUtils::set_permissions(path_to_permissionless_file, 0);
-#else
- boost::filesystem::permissions(path_to_permissionless_file,
boost::filesystem::no_perms);
-#endif
}
- minifi::io::FileStream stream(path_to_permissionless_file, 0, false);
+ minifi::io::FileStream stream(path_to_permissionless_file, 0, true);
Review Comment:
why did this change from `false` to `true`?
--
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]
