[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1543: MINIFICPP-2074 Fix time-period/integer validated properties during lo…

["szaszm (via GitHub)" <gi...@apache.org>]

szaszm commented on code in PR #1543:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1543#discussion_r1150080892


##########
minifi_main/MiNiFiMain.cpp:
##########
@@ -263,6 +263,7 @@ int main(int argc, char **argv) {
     configure->setHome(minifiHome);
     configure->loadConfigureFile(DEFAULT_NIFI_PROPERTIES_FILE);
 
+    configure->commitChanges();

Review Comment:
   Can we skip this commit, and still have the changed/mapped properties appear as part of the manifest?
   
   It feels wrong to write the config right after reading it, and we will do the mapping of the values anyway while reading. I think persisting them in a batch with the first normal persist is enough, and we can rely on in-memory mapping integers to milliseconds.



##########
libminifi/src/properties/Properties.cpp:
##########
@@ -87,13 +128,17 @@ void Properties::loadConfigureFile(const std::filesystem::path& configuration_fi
     return;
   }
   properties_.clear();
+  dirty_ = false;
   for (const auto& line : PropertiesFile{file}) {
+    auto key = line.getKey();
     auto persisted_value = line.getValue();
     auto value = utils::StringUtils::replaceEnvironmentVariables(persisted_value);
-    properties_[line.getKey()] = {persisted_value, value, false};
+    bool need_to_persist_new_value = false;
+    formatConfigurationProperty(key, persisted_value, value, need_to_persist_new_value);
+    dirty_ |= need_to_persist_new_value;

Review Comment:
   This operator does bitwise OR, not logical OR. It happens to work out fine, due to the memory representation of booleans, but I'd prefer using logical operators with booleans.



##########
libminifi/src/properties/Properties.cpp:
##########
@@ -62,6 +63,46 @@ int Properties::getInt(const std::string &key, int default_value) const {
   return it != properties_.end() ? std::stoi(it->second.active_value) : default_value;
 }
 
+namespace {
+void ensureTimePeriodValidatedPropertyHasExplicitUnit(const core::PropertyValidator* const validator, std::string& persisted_value, std::string& value, bool& need_to_persist_new_value) {
+  if (validator != core::StandardValidators::get().TIME_PERIOD_VALIDATOR.get())
+    return;
+  if (value.empty() || !std::all_of(value.begin(), value.end(), ::isdigit))
+    return;
+
+  value += " ms";
+  persisted_value = value;
+  need_to_persist_new_value = true;
+}
+
+bool integerValidatedProperty(const core::PropertyValidator* const validator) {
+  return validator == core::StandardValidators::get().INTEGER_VALIDATOR.get()
+      || validator == core::StandardValidators::get().UNSIGNED_INT_VALIDATOR.get()
+      || validator == core::StandardValidators::get().LONG_VALIDATOR.get()
+      || validator == core::StandardValidators::get().UNSIGNED_LONG_VALIDATOR.get();
+}
+
+void ensureIntegerValidatedPropertyHasNoUnit(const core::PropertyValidator* const validator, std::string& persisted_value, std::string& value, bool& need_to_persist_new_value) {
+  if (!integerValidatedProperty(validator))
+    return;
+
+  if (auto parsed_time = utils::timeutils::StringToDuration<std::chrono::milliseconds>(value)) {
+    value = fmt::format("{}", parsed_time->count());
+    persisted_value = value;
+    need_to_persist_new_value = true;
+  }
+}
+
+void formatConfigurationProperty(std::string_view key, std::string& persisted_value, std::string& value, bool& need_to_persist_new_value) {
+  auto configuration_property = Configuration::CONFIGURATION_PROPERTIES.find(key);
+  if (configuration_property == Configuration::CONFIGURATION_PROPERTIES.end())
+    return;
+
+  ensureTimePeriodValidatedPropertyHasExplicitUnit(configuration_property->second, persisted_value, value, need_to_persist_new_value);
+  ensureIntegerValidatedPropertyHasNoUnit(configuration_property->second, persisted_value, value, need_to_persist_new_value);
+}
+}  // namespace
+

Review Comment:
   Could you extend the comment above `loadConfigureFile` to specify the kind of mapping that's happening here during load, and the motivation? Just to avoid confusing future readers of the code.



-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org