[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages

[GitBox <gi...@apache.org>]

szaszm commented on a change in pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220#discussion_r765564052



##########
File path: libminifi/include/core/yaml/CheckRequiredField.h
##########
@@ -49,7 +53,23 @@ namespace yaml {
  *                               not present in 'yamlNode'
  */
 void checkRequiredField(
-    const YAML::Node *yamlNode, const std::string &fieldName, const std::shared_ptr<logging::Logger>& logger, const std::string &yamlSection = "", const std::string &errorMessage = "");
+    const YAML::Node *yamlNode, const std::string &fieldName, const std::shared_ptr<logging::Logger>& logger, const std::string &yamlSection = "", std::string errorMessage = "");
+
+template<typename T>
+T parseRequiredField(const YAML::Node* yamlNode,
+    const std::vector<std::string>& alternateNames, const std::shared_ptr<logging::Logger>& logger,
+    const std::string &yamlSection, std::function<T(const YAML::Node&)> parser, std::string errorMessage = "") {
+  for (const auto& name : alternateNames) {
+    if (yaml::isFieldPresent(yamlNode, name)) {
+      return parser((*yamlNode)[name]);
+    }
+  }
+  if (errorMessage.empty()) {
+    errorMessage = buildErrorMessage(yamlNode, alternateNames, yamlSection);
+  }
+  logger->log_error(errorMessage.c_str());
+  throw std::invalid_argument(errorMessage);
+}

Review comment:
       1. Most of this logic could go to the implementation file, since the only thing requiring it to be a template is the `parser` call. Extract the logic to something like `const YAML::Node& getRequiredField(const YAML::Node& yaml_node, gsl::span<std::string_view> alternate_names, logging::Logger& logger, std::string_view yaml_section, std::string_view error_message = {});`
   2. The passed strings could be string_view instead to not force allocations for all strings that are passed in. The collection could be a `gsl::span` to avoid the allocation from `std::vector`.
   3. The pointer parameter doesn't seem to be nullable, so it should be a reference instead.
   4. If it's a template anyway, there's no need to type-erase the `parser` function
   5. If logging a string, use `"%s"`, so that the logging doesn't misbehave when the error message happens to contain format specifiers.
   6. One of log or throw should normally be enough, so the error handling (catch) code can decide whether to log the error or not. Maybe it could recover.
   I changed the parameter names to snake_case in the suggestion, because I like them better, but it's mixed in the codebase, so I don't insist on changing that.
   ```suggestion
   // in the cpp file
   const YAML::Node& getRequiredField(const YAML::Node& yaml_node, gsl::span<std::string_view> alternate_names, logging::Logger& logger, std::string_view yaml_section, std::string error_message) {
     for (const auto& name : alternate_names) {
       if (yaml::isFieldPresent(yaml_node, name)) {
         return parser((*yaml_node)[name]);
       }
     }
     if (error_message.empty()) {
       error_message = buildErrorMessage(yaml_node, alternate_names, yaml_section);
     }
     // logger->log_error("%s", error_message);
     throw std::invalid_argument(error_message);
   }
   
   // in the header
   const YAML::Node& getRequiredField(const YAML::Node& yaml_node, gsl::span<std::string_view> alternate_names, logging::Logger& logger, std::string_view yaml_section, std::string error_message = {});
   
   
   template<typename T, typename F>
   auto parseRequiredField(const YAML::Node& yaml_node, gsl::span<std::string_view> alternate_names, logging::Logger& logger, std::string_view yaml_section, F parser, std::string error_message = {})
       -> std::enable_if_t<std::is_convertible_v<decltype(parser(std::declval<const YAML::Node&>())), T>, T> {
     return parser(getRequiredField(yaml_node, alternate_names, logger, yaml_section, std::move(error_message)));
   }
   
   // or alternatively automatically deduce the return type based on the parser function
   template<typename F>
   auto parseRequiredField(const YAML::Node& yaml_node, gsl::span<std::string_view> alternate_names, logging::Logger& logger, std::string_view yaml_section, F parser, std::string error_message = {})
       /* optional explicit return type: -> decltype(parser(std::declval<const YAML::Node&>())) */ {
     return parser(getRequiredField(yaml_node, alternate_names, logger, yaml_section, std::move(error_message)));
   }
   ```




-- 
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