You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/11/29 16:25:25 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request #1220: MINIFICPP-1694 Remove false positive error log messages
lordgamez opened a new pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220
Several users have encountered a few error messages while debugging unrelated issues. These error messages were shown during startup and did not actually signal any problems, but they were misleading enough to suspect some relation to the issues being debugged. These error messages were removed or changed to warnings with some additional information to be more elaborate about their nature.
- For controllers both class and type fields are accepted without any error messages
- When a configuration file does not exist like the default bootstrap file, instead of the previous `load configure file failed` we get a warning with the message `Configuration file './conf/bootstrap.conf' does not exist, so it could not be loaded.`
- When a rocksdb key cannot be retrieved when it is probably not initialized yet we only get a warning
https://issues.apache.org/jira/browse/MINIFICPP-1694
----------------------------------------------------------------------------------------------
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by 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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by 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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220#discussion_r764774883
##########
File path: libminifi/src/core/yaml/YamlConfiguration.cpp
##########
@@ -529,14 +529,20 @@ void YamlConfiguration::parseControllerServices(const YAML::Node& controllerServ
CONFIG_YAML_CONTROLLER_SERVICES_KEY);
std::string type = "";
- try {
- yaml::checkRequiredField(&controllerServiceNode, "class", logger_, CONFIG_YAML_CONTROLLER_SERVICES_KEY);
+ if (yaml::isFieldPresent(&controllerServiceNode, "class")) {
type = controllerServiceNode["class"].as<std::string>();
- } catch (const std::invalid_argument &) {
- yaml::checkRequiredField(&controllerServiceNode, "type", logger_, CONFIG_YAML_CONTROLLER_SERVICES_KEY);
+ } else {
+ const YAML::Node name_node = controllerServiceNode.as<YAML::Node>()["name"];
+ std::string err_msg =
+ name_node ?
+ "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as required field 'class' or 'type' is missing" :
+ "Unable to parse configuration file as required field 'class' or 'type' is missing";
+ err_msg += " [in '" + std::string(CONFIG_YAML_CONTROLLER_SERVICES_KEY) + "' section of configuration file]";
+ yaml::checkRequiredField(&controllerServiceNode, "type", logger_, CONFIG_YAML_CONTROLLER_SERVICES_KEY, err_msg);
Review comment:
Good idea, added in 31e4679c2f41a3f2a788132ca34a77fedf5d9ec7
--
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
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220#discussion_r765712286
##########
File path: libminifi/src/core/yaml/CheckRequiredField.cpp
##########
@@ -27,28 +28,48 @@ namespace minifi {
namespace core {
namespace yaml {
-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) {
- std::string errMsg = errorMessage;
- if (!yamlNode->as<YAML::Node>()[fieldName]) {
- if (errMsg.empty()) {
- const YAML::Node name_node = yamlNode->as<YAML::Node>()["name"];
- // Build a helpful error message for the user so they can fix the
- // invalid YAML config file, using the component name if present
- errMsg =
- name_node ?
- "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as required field '" + fieldName + "' is missing" :
- "Unable to parse configuration file as required field '" + fieldName + "' is missing";
- if (!yamlSection.empty()) {
- errMsg += " [in '" + yamlSection + "' section of configuration file]";
- }
- const YAML::Mark mark = yamlNode->Mark();
- if (!mark.is_null()) {
- errMsg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
- }
+bool isFieldPresent(const YAML::Node &yaml_node, std::string_view field_name) {
+ return bool{yaml_node.as<YAML::Node>()[field_name.data()]};
+}
+
+std::string buildErrorMessage(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_field_names, std::string_view yaml_section) {
+ const YAML::Node name_node = yaml_node.as<YAML::Node>()["name"];
+ // Build a helpful error message for the user so they can fix the
+ // invalid YAML config file, using the component name if present
+ auto field_list_string = utils::StringUtils::join(", ", alternate_field_names);
+ std::string err_msg =
+ name_node ?
+ "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as none of the possible required fields [" + field_list_string + "] is available" :
+ "Unable to parse configuration file as none of the possible required fields [" + field_list_string + "] is available";
+ if (!yaml_section.empty()) {
+ err_msg += " [in '" + std::string(yaml_section) + "' section of configuration file]";
+ }
+ const YAML::Mark mark = yaml_node.Mark();
+ if (!mark.is_null()) {
+ err_msg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
+ }
+ return err_msg;
+}
+
+void checkRequiredField(const YAML::Node &yaml_node, std::string_view field_name, std::string_view yaml_section, std::string error_message) {
+ if (!isFieldPresent(yaml_node, field_name)) {
+ if (error_message.empty()) {
+ error_message = buildErrorMessage(yaml_node, std::vector<std::string>{std::string(field_name)}, yaml_section);
}
- logger->log_error(errMsg.c_str());
- throw std::invalid_argument(errMsg);
+ throw std::invalid_argument(error_message);
+ }
+}
+
+std::string getRequiredField(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_names, std::string_view yaml_section, std::string error_message) {
Review comment:
oh yeah, sorry `Node::operator[]` returns a `Node` not a ref to some preconstructed object, obv the ref return type doesn't work in this case
--
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
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220#discussion_r765689199
##########
File path: libminifi/src/core/yaml/CheckRequiredField.cpp
##########
@@ -27,28 +28,48 @@ namespace minifi {
namespace core {
namespace yaml {
-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) {
- std::string errMsg = errorMessage;
- if (!yamlNode->as<YAML::Node>()[fieldName]) {
- if (errMsg.empty()) {
- const YAML::Node name_node = yamlNode->as<YAML::Node>()["name"];
- // Build a helpful error message for the user so they can fix the
- // invalid YAML config file, using the component name if present
- errMsg =
- name_node ?
- "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as required field '" + fieldName + "' is missing" :
- "Unable to parse configuration file as required field '" + fieldName + "' is missing";
- if (!yamlSection.empty()) {
- errMsg += " [in '" + yamlSection + "' section of configuration file]";
- }
- const YAML::Mark mark = yamlNode->Mark();
- if (!mark.is_null()) {
- errMsg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
- }
+bool isFieldPresent(const YAML::Node &yaml_node, std::string_view field_name) {
+ return bool{yaml_node.as<YAML::Node>()[field_name.data()]};
+}
+
+std::string buildErrorMessage(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_field_names, std::string_view yaml_section) {
+ const YAML::Node name_node = yaml_node.as<YAML::Node>()["name"];
+ // Build a helpful error message for the user so they can fix the
+ // invalid YAML config file, using the component name if present
+ auto field_list_string = utils::StringUtils::join(", ", alternate_field_names);
+ std::string err_msg =
+ name_node ?
+ "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as none of the possible required fields [" + field_list_string + "] is available" :
+ "Unable to parse configuration file as none of the possible required fields [" + field_list_string + "] is available";
+ if (!yaml_section.empty()) {
+ err_msg += " [in '" + std::string(yaml_section) + "' section of configuration file]";
+ }
+ const YAML::Mark mark = yaml_node.Mark();
+ if (!mark.is_null()) {
+ err_msg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
+ }
+ return err_msg;
+}
+
+void checkRequiredField(const YAML::Node &yaml_node, std::string_view field_name, std::string_view yaml_section, std::string error_message) {
+ if (!isFieldPresent(yaml_node, field_name)) {
+ if (error_message.empty()) {
+ error_message = buildErrorMessage(yaml_node, std::vector<std::string>{std::string(field_name)}, yaml_section);
}
- logger->log_error(errMsg.c_str());
- throw std::invalid_argument(errMsg);
+ throw std::invalid_argument(error_message);
+ }
+}
+
+std::string getRequiredField(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_names, std::string_view yaml_section, std::string error_message) {
Review comment:
I think we should return the node ref instead, converting it into a string seems something the caller should decide to do
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220#discussion_r765692663
##########
File path: libminifi/src/core/yaml/CheckRequiredField.cpp
##########
@@ -27,28 +28,48 @@ namespace minifi {
namespace core {
namespace yaml {
-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) {
- std::string errMsg = errorMessage;
- if (!yamlNode->as<YAML::Node>()[fieldName]) {
- if (errMsg.empty()) {
- const YAML::Node name_node = yamlNode->as<YAML::Node>()["name"];
- // Build a helpful error message for the user so they can fix the
- // invalid YAML config file, using the component name if present
- errMsg =
- name_node ?
- "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as required field '" + fieldName + "' is missing" :
- "Unable to parse configuration file as required field '" + fieldName + "' is missing";
- if (!yamlSection.empty()) {
- errMsg += " [in '" + yamlSection + "' section of configuration file]";
- }
- const YAML::Mark mark = yamlNode->Mark();
- if (!mark.is_null()) {
- errMsg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
- }
+bool isFieldPresent(const YAML::Node &yaml_node, std::string_view field_name) {
+ return bool{yaml_node.as<YAML::Node>()[field_name.data()]};
+}
+
+std::string buildErrorMessage(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_field_names, std::string_view yaml_section) {
+ const YAML::Node name_node = yaml_node.as<YAML::Node>()["name"];
+ // Build a helpful error message for the user so they can fix the
+ // invalid YAML config file, using the component name if present
+ auto field_list_string = utils::StringUtils::join(", ", alternate_field_names);
+ std::string err_msg =
+ name_node ?
+ "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as none of the possible required fields [" + field_list_string + "] is available" :
+ "Unable to parse configuration file as none of the possible required fields [" + field_list_string + "] is available";
+ if (!yaml_section.empty()) {
+ err_msg += " [in '" + std::string(yaml_section) + "' section of configuration file]";
+ }
+ const YAML::Mark mark = yaml_node.Mark();
+ if (!mark.is_null()) {
+ err_msg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
+ }
+ return err_msg;
+}
+
+void checkRequiredField(const YAML::Node &yaml_node, std::string_view field_name, std::string_view yaml_section, std::string error_message) {
+ if (!isFieldPresent(yaml_node, field_name)) {
+ if (error_message.empty()) {
+ error_message = buildErrorMessage(yaml_node, std::vector<std::string>{std::string(field_name)}, yaml_section);
}
- logger->log_error(errMsg.c_str());
- throw std::invalid_argument(errMsg);
+ throw std::invalid_argument(error_message);
+ }
+}
+
+std::string getRequiredField(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_names, std::string_view yaml_section, std::string error_message) {
Review comment:
Returning the node resulted in a compile warning because it was a reference to a temporary object, that's why I went with returning a string.
--
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
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220#discussion_r764680221
##########
File path: libminifi/src/core/yaml/YamlConfiguration.cpp
##########
@@ -529,14 +529,20 @@ void YamlConfiguration::parseControllerServices(const YAML::Node& controllerServ
CONFIG_YAML_CONTROLLER_SERVICES_KEY);
std::string type = "";
- try {
- yaml::checkRequiredField(&controllerServiceNode, "class", logger_, CONFIG_YAML_CONTROLLER_SERVICES_KEY);
+ if (yaml::isFieldPresent(&controllerServiceNode, "class")) {
type = controllerServiceNode["class"].as<std::string>();
- } catch (const std::invalid_argument &) {
- yaml::checkRequiredField(&controllerServiceNode, "type", logger_, CONFIG_YAML_CONTROLLER_SERVICES_KEY);
+ } else {
+ const YAML::Node name_node = controllerServiceNode.as<YAML::Node>()["name"];
+ std::string err_msg =
+ name_node ?
+ "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as required field 'class' or 'type' is missing" :
+ "Unable to parse configuration file as required field 'class' or 'type' is missing";
+ err_msg += " [in '" + std::string(CONFIG_YAML_CONTROLLER_SERVICES_KEY) + "' section of configuration file]";
+ yaml::checkRequiredField(&controllerServiceNode, "type", logger_, CONFIG_YAML_CONTROLLER_SERVICES_KEY, err_msg);
Review comment:
it seems we have duplicated the error message from `checkRequiredField`, should we maybe create a `checkRequiredField` overload that takes a list of alternative keys (here {"class", "type"})? we could even add a "parser" parameter, and use it as
```
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, const std::string& errorMessage = "") {...}
std::string type = parseRequiredField(&controllerServiceNode, {"class", "type"}, logger_, CONFIG_YAML_CONTROLLER_SERVICES_KEY,
[] (const YAML::Node& node) {return node.as<std::string>();});
```
--
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
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220#discussion_r765577036
##########
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:
I think we might as well drop the `parseRequiredField` and keep the `getRequiredField`, we wouldn't need to template anything and could move all implementations to the cpp file
`getRequiredField(...).as<std::string>()` even looks better than `parseRequiredField(..., [] (const auto& node) {return node.as<std::string>()}` IMO
--
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220#discussion_r765690424
##########
File path: libminifi/src/core/yaml/CheckRequiredField.cpp
##########
@@ -27,28 +28,48 @@ namespace minifi {
namespace core {
namespace yaml {
-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) {
- std::string errMsg = errorMessage;
- if (!yamlNode->as<YAML::Node>()[fieldName]) {
- if (errMsg.empty()) {
- const YAML::Node name_node = yamlNode->as<YAML::Node>()["name"];
- // Build a helpful error message for the user so they can fix the
- // invalid YAML config file, using the component name if present
- errMsg =
- name_node ?
- "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as required field '" + fieldName + "' is missing" :
- "Unable to parse configuration file as required field '" + fieldName + "' is missing";
- if (!yamlSection.empty()) {
- errMsg += " [in '" + yamlSection + "' section of configuration file]";
- }
- const YAML::Mark mark = yamlNode->Mark();
- if (!mark.is_null()) {
- errMsg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
- }
+bool isFieldPresent(const YAML::Node &yaml_node, std::string_view field_name) {
+ return bool{yaml_node.as<YAML::Node>()[field_name.data()]};
+}
+
+std::string buildErrorMessage(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_field_names, std::string_view yaml_section) {
+ const YAML::Node name_node = yaml_node.as<YAML::Node>()["name"];
+ // Build a helpful error message for the user so they can fix the
+ // invalid YAML config file, using the component name if present
+ auto field_list_string = utils::StringUtils::join(", ", alternate_field_names);
+ std::string err_msg =
+ name_node ?
+ "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as none of the possible required fields [" + field_list_string + "] is available" :
+ "Unable to parse configuration file as none of the possible required fields [" + field_list_string + "] is available";
+ if (!yaml_section.empty()) {
+ err_msg += " [in '" + std::string(yaml_section) + "' section of configuration file]";
+ }
+ const YAML::Mark mark = yaml_node.Mark();
+ if (!mark.is_null()) {
+ err_msg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
+ }
+ return err_msg;
+}
+
+void checkRequiredField(const YAML::Node &yaml_node, std::string_view field_name, std::string_view yaml_section, std::string error_message) {
+ if (!isFieldPresent(yaml_node, field_name)) {
+ if (error_message.empty()) {
+ error_message = buildErrorMessage(yaml_node, std::vector<std::string>{std::string(field_name)}, yaml_section);
}
- logger->log_error(errMsg.c_str());
- throw std::invalid_argument(errMsg);
+ throw std::invalid_argument(error_message);
+ }
+}
+
+std::string getRequiredField(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_names, std::string_view yaml_section, std::string error_message) {
Review comment:
I think it's fine as a string if it's only ever used as a string, but I'm fine with returning a node ref as well.
--
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220#discussion_r765681409
##########
File path: libminifi/src/core/yaml/CheckRequiredField.cpp
##########
@@ -27,28 +28,48 @@ namespace minifi {
namespace core {
namespace yaml {
-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) {
- std::string errMsg = errorMessage;
- if (!yamlNode->as<YAML::Node>()[fieldName]) {
- if (errMsg.empty()) {
- const YAML::Node name_node = yamlNode->as<YAML::Node>()["name"];
- // Build a helpful error message for the user so they can fix the
- // invalid YAML config file, using the component name if present
- errMsg =
- name_node ?
- "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as required field '" + fieldName + "' is missing" :
- "Unable to parse configuration file as required field '" + fieldName + "' is missing";
- if (!yamlSection.empty()) {
- errMsg += " [in '" + yamlSection + "' section of configuration file]";
- }
- const YAML::Mark mark = yamlNode->Mark();
- if (!mark.is_null()) {
- errMsg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
- }
+bool isFieldPresent(const YAML::Node &yaml_node, std::string_view field_name) {
+ return bool{yaml_node.as<YAML::Node>()[field_name.data()]};
+}
+
+std::string buildErrorMessage(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_field_names, std::string_view yaml_section) {
+ const YAML::Node name_node = yaml_node.as<YAML::Node>()["name"];
+ // Build a helpful error message for the user so they can fix the
+ // invalid YAML config file, using the component name if present
+ auto field_list_string = utils::StringUtils::join(", ", alternate_field_names);
+ std::string err_msg =
+ name_node ?
+ "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as none of the possible required fields [" + field_list_string + "] is available" :
+ "Unable to parse configuration file as none of the possible required fields [" + field_list_string + "] is available";
+ if (!yaml_section.empty()) {
+ err_msg += " [in '" + std::string(yaml_section) + "' section of configuration file]";
+ }
+ const YAML::Mark mark = yaml_node.Mark();
+ if (!mark.is_null()) {
+ err_msg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
+ }
+ return err_msg;
+}
+
+void checkRequiredField(const YAML::Node &yaml_node, std::string_view field_name, std::string_view yaml_section, std::string_view error_message) {
+ if (!isFieldPresent(yaml_node, field_name)) {
+ if (error_message.empty()) {
+ throw std::invalid_argument(buildErrorMessage(yaml_node, std::vector<std::string>{std::string(field_name)}, yaml_section));
}
- logger->log_error(errMsg.c_str());
- throw std::invalid_argument(errMsg);
+ throw std::invalid_argument(std::string(error_message));
+ }
+}
+
+std::string getRequiredField(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_names, std::string_view yaml_section, std::string_view error_message) {
+ for (const auto& name : alternate_names) {
+ if (yaml::isFieldPresent(yaml_node, name)) {
+ return yaml_node[name].as<std::string>();
+ }
+ }
+ if (error_message.empty()) {
+ std::invalid_argument(buildErrorMessage(yaml_node, alternate_names, yaml_section));
Review comment:
I think you meant to `throw` this.
--
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
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220#discussion_r765693386
##########
File path: libminifi/src/core/yaml/CheckRequiredField.cpp
##########
@@ -27,28 +28,48 @@ namespace minifi {
namespace core {
namespace yaml {
-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) {
- std::string errMsg = errorMessage;
- if (!yamlNode->as<YAML::Node>()[fieldName]) {
- if (errMsg.empty()) {
- const YAML::Node name_node = yamlNode->as<YAML::Node>()["name"];
- // Build a helpful error message for the user so they can fix the
- // invalid YAML config file, using the component name if present
- errMsg =
- name_node ?
- "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as required field '" + fieldName + "' is missing" :
- "Unable to parse configuration file as required field '" + fieldName + "' is missing";
- if (!yamlSection.empty()) {
- errMsg += " [in '" + yamlSection + "' section of configuration file]";
- }
- const YAML::Mark mark = yamlNode->Mark();
- if (!mark.is_null()) {
- errMsg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
- }
+bool isFieldPresent(const YAML::Node &yaml_node, std::string_view field_name) {
+ return bool{yaml_node.as<YAML::Node>()[field_name.data()]};
+}
+
+std::string buildErrorMessage(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_field_names, std::string_view yaml_section) {
+ const YAML::Node name_node = yaml_node.as<YAML::Node>()["name"];
+ // Build a helpful error message for the user so they can fix the
+ // invalid YAML config file, using the component name if present
+ auto field_list_string = utils::StringUtils::join(", ", alternate_field_names);
+ std::string err_msg =
+ name_node ?
+ "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as none of the possible required fields [" + field_list_string + "] is available" :
+ "Unable to parse configuration file as none of the possible required fields [" + field_list_string + "] is available";
+ if (!yaml_section.empty()) {
+ err_msg += " [in '" + std::string(yaml_section) + "' section of configuration file]";
+ }
+ const YAML::Mark mark = yaml_node.Mark();
+ if (!mark.is_null()) {
+ err_msg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
+ }
+ return err_msg;
+}
+
+void checkRequiredField(const YAML::Node &yaml_node, std::string_view field_name, std::string_view yaml_section, std::string error_message) {
+ if (!isFieldPresent(yaml_node, field_name)) {
+ if (error_message.empty()) {
+ error_message = buildErrorMessage(yaml_node, std::vector<std::string>{std::string(field_name)}, yaml_section);
}
- logger->log_error(errMsg.c_str());
- throw std::invalid_argument(errMsg);
+ throw std::invalid_argument(error_message);
+ }
+}
+
+std::string getRequiredField(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_names, std::string_view yaml_section, std::string error_message) {
Review comment:
the only place I could find that doesn't use a required field as a string is the `Input Ports` in RPGs, so I'm fine with leaving it to return a string until more use-cases arise
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220#discussion_r765684215
##########
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:
Thanks for your comments, I appreciate the effort you put into them. I updated most of the points listed, unfortunately I could not avoid allocating the vector used for the alternate name so I didn't use `gsl::span` and also out implementation of `StringUtils::join` does not work with `std::string_view`s so I had to fallback to strings.
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220#discussion_r765684513
##########
File path: libminifi/src/core/yaml/CheckRequiredField.cpp
##########
@@ -27,28 +28,48 @@ namespace minifi {
namespace core {
namespace yaml {
-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) {
- std::string errMsg = errorMessage;
- if (!yamlNode->as<YAML::Node>()[fieldName]) {
- if (errMsg.empty()) {
- const YAML::Node name_node = yamlNode->as<YAML::Node>()["name"];
- // Build a helpful error message for the user so they can fix the
- // invalid YAML config file, using the component name if present
- errMsg =
- name_node ?
- "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as required field '" + fieldName + "' is missing" :
- "Unable to parse configuration file as required field '" + fieldName + "' is missing";
- if (!yamlSection.empty()) {
- errMsg += " [in '" + yamlSection + "' section of configuration file]";
- }
- const YAML::Mark mark = yamlNode->Mark();
- if (!mark.is_null()) {
- errMsg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
- }
+bool isFieldPresent(const YAML::Node &yaml_node, std::string_view field_name) {
+ return bool{yaml_node.as<YAML::Node>()[field_name.data()]};
+}
+
+std::string buildErrorMessage(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_field_names, std::string_view yaml_section) {
+ const YAML::Node name_node = yaml_node.as<YAML::Node>()["name"];
+ // Build a helpful error message for the user so they can fix the
+ // invalid YAML config file, using the component name if present
+ auto field_list_string = utils::StringUtils::join(", ", alternate_field_names);
+ std::string err_msg =
+ name_node ?
+ "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as none of the possible required fields [" + field_list_string + "] is available" :
+ "Unable to parse configuration file as none of the possible required fields [" + field_list_string + "] is available";
+ if (!yaml_section.empty()) {
+ err_msg += " [in '" + std::string(yaml_section) + "' section of configuration file]";
+ }
+ const YAML::Mark mark = yaml_node.Mark();
+ if (!mark.is_null()) {
+ err_msg += " [line:column, pos at " + std::to_string(mark.line) + ":" + std::to_string(mark.column) + ", " + std::to_string(mark.pos) + "]";
+ }
+ return err_msg;
+}
+
+void checkRequiredField(const YAML::Node &yaml_node, std::string_view field_name, std::string_view yaml_section, std::string_view error_message) {
+ if (!isFieldPresent(yaml_node, field_name)) {
+ if (error_message.empty()) {
+ throw std::invalid_argument(buildErrorMessage(yaml_node, std::vector<std::string>{std::string(field_name)}, yaml_section));
}
- logger->log_error(errMsg.c_str());
- throw std::invalid_argument(errMsg);
+ throw std::invalid_argument(std::string(error_message));
+ }
+}
+
+std::string getRequiredField(const YAML::Node &yaml_node, const std::vector<std::string> &alternate_names, std::string_view yaml_section, std::string_view error_message) {
+ for (const auto& name : alternate_names) {
+ if (yaml::isFieldPresent(yaml_node, name)) {
+ return yaml_node[name].as<std::string>();
+ }
+ }
+ if (error_message.empty()) {
+ std::invalid_argument(buildErrorMessage(yaml_node, alternate_names, yaml_section));
Review comment:
Fixed in e6d61d1d6b803061bc6dd60ec9800109e789efda
--
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
[GitHub] [nifi-minifi-cpp] adamdebreceni closed pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by GitBox <gi...@apache.org>.
adamdebreceni closed pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220
--
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
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1220: MINIFICPP-1694 Remove false positive error log messages
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1220:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1220#discussion_r764680221
##########
File path: libminifi/src/core/yaml/YamlConfiguration.cpp
##########
@@ -529,14 +529,20 @@ void YamlConfiguration::parseControllerServices(const YAML::Node& controllerServ
CONFIG_YAML_CONTROLLER_SERVICES_KEY);
std::string type = "";
- try {
- yaml::checkRequiredField(&controllerServiceNode, "class", logger_, CONFIG_YAML_CONTROLLER_SERVICES_KEY);
+ if (yaml::isFieldPresent(&controllerServiceNode, "class")) {
type = controllerServiceNode["class"].as<std::string>();
- } catch (const std::invalid_argument &) {
- yaml::checkRequiredField(&controllerServiceNode, "type", logger_, CONFIG_YAML_CONTROLLER_SERVICES_KEY);
+ } else {
+ const YAML::Node name_node = controllerServiceNode.as<YAML::Node>()["name"];
+ std::string err_msg =
+ name_node ?
+ "Unable to parse configuration file for component named '" + name_node.as<std::string>() + "' as required field 'class' or 'type' is missing" :
+ "Unable to parse configuration file as required field 'class' or 'type' is missing";
+ err_msg += " [in '" + std::string(CONFIG_YAML_CONTROLLER_SERVICES_KEY) + "' section of configuration file]";
+ yaml::checkRequiredField(&controllerServiceNode, "type", logger_, CONFIG_YAML_CONTROLLER_SERVICES_KEY, err_msg);
Review comment:
it seems we have duplicated the error message from `checkRequiredField`, should we maybe create a `checkRequiredField` overload that takes a list of alternative keys (here {"class", "type"})? we could even add a "parser" parameter, and use it as
```
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, const std::string& errorMessage = "") {...}
std::string type = parseRequiredField(&controllerServiceNode, {"class", "type"}, logger_, CONFIG_YAML_CONTROLLER_SERVICES_KEY, [] (const YAML::Node& node) {return node.as<std::string>();});
```
--
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