fgerlits commented on code in PR #1926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1926#discussion_r1995853440
##########
libminifi/src/core/flow/StructuredConfiguration.cpp:
##########
@@ -580,13 +579,12 @@ void
StructuredConfiguration::parseProvenanceReporting(const Node& node, core::P
throw std::invalid_argument("Invalid scheduling strategy " +
schedulingStrategyStr);
}
- int64_t lvalue = 0;
if (node["host"] && node["port"]) {
auto hostStr = node["host"].getString().value();
std::string portStr = node["port"].getIntegerAsString().value();
- if (core::Property::StringToInt(portStr, lvalue) && !hostStr.empty()) {
- logger_->log_debug("ProvenanceReportingTask port {}", lvalue);
+ if (auto port = parsing::parseIntegral<int64_t>(portStr)) {
+ logger_->log_debug("ProvenanceReportingTask port {}", *port);
std::string url = hostStr + ":" + portStr;
reportTask->setURL(url);
}
Review Comment:
previously, we were skipping this if `hostStr` is empty; we should continue
to do that, unless there is a reason not to
##########
libminifi/src/core/flow/StructuredConfiguration.cpp:
##########
@@ -789,114 +785,88 @@ void
StructuredConfiguration::parsePropertyValueSequence(const std::string& prop
logger_->log_debug("Found property {}", property_name);
- if (!component.updateProperty(property_name, rawValueString)) {
- auto proc = dynamic_cast<core::Connectable*>(&component);
- if (proc) {
- logger_->log_warn("Received property {} with value {} but is not one
of the properties for {}. Attempting to add as dynamic property.",
property_name, rawValueString, proc->getName());
- if (!component.updateDynamicProperty(property_name, rawValueString))
{
- logger_->log_warn("Unable to set the dynamic property {}",
property_name);
- } else {
- logger_->log_warn("Dynamic property {} has been set",
property_name);
- }
+ const auto append_prop_result = component.appendProperty(property_name,
rawValueString);
+ if (!append_prop_result && append_prop_result.error() ==
make_error_code(PropertyErrorCode::NotSupportedProperty)) {
+ logger_->log_warn("Received property {} with value {} but is not one
of the properties for {}. Attempting to add as dynamic property.",
property_name, rawValueString, component.getName());
Review Comment:
We need to handle the case when validation fails, although I'm not sure what
should happen: log and move on, or throw an exception?
##########
libminifi/src/properties/Properties.cpp:
##########
@@ -187,8 +158,8 @@ void fixValidatedProperty(const std::string& property_name,
} // namespace
// Load Configure File
-// If the loaded property is time-period or data-size validated and it has no
explicit units ms or B will be appended.
-// If the loaded property is integer validated and it has some explicit
unit(time-period or data-size) it will be converted to ms/B and its unit cut off
+// If the loaded property is time-period or data-size validated, and it has no
explicit units ms or B will be appended.
+// If the loaded property is integer validated, and it has some explicit
unit(time-period or data-size) it will be converted to ms/B and its unit cut off
Review Comment:
If we are adding punctuation, we should add (and remove) some more:
```
// If the loaded property is time period or data size validated, and it has
no explicit units, then ms or B will be appended.
// If the loaded property is integer validated, and it has some explicit
unit (time period or data size), it will be converted to ms or B, and its unit
is cut off.
```
Also, these two lines of comment should be moved before the
`fixValidatedProperty()` function. (And the "Load Configure File" comment could
be removed.)
##########
libminifi/src/core/flow/StructuredConfiguration.cpp:
##########
@@ -753,27 +751,25 @@ void StructuredConfiguration::parseRPGPort(const Node&
port_node, core::ProcessG
processor.setScheduledState(core::RUNNING);
if (auto tasksNode = port_node[schema_.max_concurrent_tasks]) {
- std::string rawMaxConcurrentTasks = tasksNode.getIntegerAsString().value();
- int32_t maxConcurrentTasks = 0;
- if (core::Property::StringToInt(rawMaxConcurrentTasks,
maxConcurrentTasks)) {
- processor.setMaxConcurrentTasks(maxConcurrentTasks);
+ const std::string raw_max_concurrent_tasks =
tasksNode.getIntegerAsString().value();
+ if (auto max_concurrent_tasks =
parsing::parseIntegral<uint8_t>(raw_max_concurrent_tasks).value_or(0)) {
+ logger_->log_debug("parseProcessorNode: maxConcurrentTasks => [{}]",
max_concurrent_tasks);
+ processor.setMaxConcurrentTasks(max_concurrent_tasks);
}
Review Comment:
we shouldn't use integer to boolean implicit conversions; I suggest removing
the `.value_or(0)`
##########
libminifi/src/agent/JsonSchema.cpp:
##########
@@ -61,35 +62,37 @@ void writePropertySchema(const core::Property& prop,
std::ostream& out) {
if (const auto& values = prop.getAllowedValues(); !values.empty()) {
out << R"(, "enum": [)"
<< (values
- | ranges::views::transform([] (auto& val) {return '"' +
escape(val.to_string()) + '"';})
+ | ranges::views::transform([] (auto& val) {return '"' +
escape(val) + '"';})
| ranges::views::join(',')
| ranges::to<std::string>())
<< "]";
}
- if (const auto& def_value = prop.getDefaultValue(); !def_value.empty()) {
- const auto& type = def_value.getTypeInfo();
- // order is important as both DataSizeValue and TimePeriodValue's type_id
is uint64_t
- if (std::dynamic_pointer_cast<core::DataSizeValue>(def_value.getValue())
- ||
std::dynamic_pointer_cast<core::TimePeriodValue>(def_value.getValue())) { //
NOLINT(bugprone-branch-clone)
- // special value types
- out << R"(, "type": "string", "default": ")" <<
escape(def_value.to_string()) << "\"";
- } else if (type == state::response::Value::INT_TYPE
- || type == state::response::Value::INT64_TYPE
- || type == state::response::Value::UINT32_TYPE
- || type == state::response::Value::UINT64_TYPE) {
- out << R"(, "type": "integer", "default": )" <<
static_cast<int64_t>(def_value);
- } else if (type == state::response::Value::DOUBLE_TYPE) {
- out << R"(, "type": "number", "default": )" <<
static_cast<double>(def_value);
- } else if (type == state::response::Value::BOOL_TYPE) {
- out << R"(, "type": "boolean", "default": )" <<
(static_cast<bool>(def_value) ? "true" : "false");
+ if (const auto validator_name =
prop.getValidator().getEquivalentNifiStandardValidatorName()) {
+ if (validator_name == "INTEGER_VALIDATOR" || validator_name ==
"NON_NEGATIVE_INTEGER_VALIDATOR") {
Review Comment:
I think
```suggestion
if (validator_name ==
core::StandardPropertyTypes::INTEGER_VALIDATOR.getEquivalentNifiStandardValidatorName()
|| validator_name ==
core::StandardPropertyTypes::UNSIGNED_INTEGER_VALIDATOR.getEquivalentNifiStandardValidatorName())
{
```
would be better; also for `BOOLEAN_VALIDATOR` below
##########
libminifi/src/DiskSpaceWatchdog.cpp:
##########
@@ -28,22 +28,17 @@ namespace org::apache::nifi::minifi {
namespace {
namespace chr = std::chrono;
-template<typename T, typename = std::enable_if_t<std::is_integral<T>::value>>
-std::optional<T> data_size_string_to_int(const std::string& str) {
- T result{};
- // actually aware of data units like B, kB, MB, etc.
- const bool success = core::Property::StringToInt(str, result);
- if (!success) return std::nullopt;
- return std::make_optional(result);
+std::optional<uint64_t> data_size_string_to_int(const std::string& str) {
+ return (parsing::parseDataSize(str) | utils::transform([](const auto& value)
{ return std::optional{value}; })).value_or(std::nullopt);
Review Comment:
this could use `toOptional()`
```suggestion
return parsing::parseDataSize(str) | utils::toOptional();
```
##########
libminifi/src/core/flow/StructuredConfiguration.cpp:
##########
@@ -789,114 +785,88 @@ void
StructuredConfiguration::parsePropertyValueSequence(const std::string& prop
logger_->log_debug("Found property {}", property_name);
- if (!component.updateProperty(property_name, rawValueString)) {
- auto proc = dynamic_cast<core::Connectable*>(&component);
- if (proc) {
- logger_->log_warn("Received property {} with value {} but is not one
of the properties for {}. Attempting to add as dynamic property.",
property_name, rawValueString, proc->getName());
- if (!component.updateDynamicProperty(property_name, rawValueString))
{
- logger_->log_warn("Unable to set the dynamic property {}",
property_name);
- } else {
- logger_->log_warn("Dynamic property {} has been set",
property_name);
- }
+ const auto append_prop_result = component.appendProperty(property_name,
rawValueString);
+ if (!append_prop_result && append_prop_result.error() ==
make_error_code(PropertyErrorCode::NotSupportedProperty)) {
+ logger_->log_warn("Received property {} with value {} but is not one
of the properties for {}. Attempting to add as dynamic property.",
property_name, rawValueString, component.getName());
+ if (!component.appendDynamicProperty(property_name, rawValueString)) {
+ logger_->log_warn("Unable to set the dynamic property {}",
property_name);
+ } else {
+ logger_->log_warn("Dynamic property {} has been set", property_name);
}
}
}
}
}
-PropertyValue
StructuredConfiguration::getValidatedProcessorPropertyForDefaultTypeInfo(const
core::Property& property_from_processor, const Node& property_value_node,
+std::optional<std::string>
StructuredConfiguration::getReplacedParametersValueOrDefault(const
std::string_view property_name,
+ const bool is_sensitive,
+ const std::optional<std::string_view> default_value,
+ const Node& property_value_node,
ParameterContext* parameter_context) {
- using state::response::Value;
- PropertyValue defaultValue;
- defaultValue = property_from_processor.getDefaultValue();
- const std::type_index defaultType = defaultValue.getTypeInfo();
try {
- PropertyValue coercedValue = defaultValue;
- auto int64_val = property_value_node.getInt64();
- if (defaultType == Value::INT64_TYPE && int64_val) {
- coercedValue = gsl::narrow<int64_t>(int64_val.value());
- } else if (defaultType == Value::UINT64_TYPE && int64_val) {
- coercedValue = gsl::narrow<uint64_t>(int64_val.value());
- } else if (defaultType == Value::UINT32_TYPE && int64_val) {
- coercedValue = gsl::narrow<uint32_t>(int64_val.value());
- } else if (defaultType == Value::INT_TYPE && int64_val) {
- coercedValue = gsl::narrow<int>(int64_val.value());
- } else if (defaultType == Value::BOOL_TYPE &&
property_value_node.getBool()) {
- coercedValue = property_value_node.getBool().value();
+ std::unique_ptr<core::ParameterTokenParser> token_parser;
+ std::string property_value_string;
+ if (is_sensitive) {
+ property_value_string =
utils::crypto::property_encryption::decrypt(property_value_node.getScalarAsString().value(),
sensitive_values_encryptor_);
+ token_parser =
std::make_unique<core::SensitiveParameterTokenParser>(std::move(property_value_string),
sensitive_values_encryptor_);
} else {
- std::string property_value_string;
- std::unique_ptr<core::ParameterTokenParser> token_parser;
- if (property_from_processor.isSensitive()) {
- property_value_string =
utils::crypto::property_encryption::decrypt(property_value_node.getScalarAsString().value(),
sensitive_values_encryptor_);
- token_parser =
std::make_unique<core::SensitiveParameterTokenParser>(property_value_string,
sensitive_values_encryptor_);
- } else {
- property_value_string =
property_value_node.getScalarAsString().value();
- token_parser =
std::make_unique<core::NonSensitiveParameterTokenParser>(property_value_string);
- }
- property_value_string =
token_parser->replaceParameters(parameter_context);
- coercedValue = property_value_string;
+ property_value_string = property_value_node.getScalarAsString().value();
+ token_parser =
std::make_unique<core::NonSensitiveParameterTokenParser>(std::move(property_value_string));
}
- return coercedValue;
+ auto replaced_property_value_string =
token_parser->replaceParameters(parameter_context);
+ return replaced_property_value_string;
} catch (const utils::crypto::EncryptionError& e) {
logger_->log_error("Fetching property failed with a decryption error: {}",
e.what());
throw;
} catch (const ParameterException& e) {
- logger_->log_error("Error while substituting parameters in property '{}':
{}", property_from_processor.getName(), e.what());
+ logger_->log_error("Error while substituting parameters in property '{}':
{}", property_name, e.what());
throw;
} catch (const std::exception& e) {
logger_->log_error("Fetching property failed with an exception of {}",
e.what());
- logger_->log_error("Invalid conversion for field {}. Value {}",
property_from_processor.getName(), property_value_node.getDebugString());
+ logger_->log_error("Invalid conversion for field {}. Value {}",
property_name, property_value_node.getDebugString());
} catch (...) {
- logger_->log_error("Invalid conversion for field {}. Value {}",
property_from_processor.getName(), property_value_node.getDebugString());
+ logger_->log_error("Invalid conversion for field {}. Value {}",
property_name, property_value_node.getDebugString());
}
- return defaultValue;
+ return default_value | utils::transform([](const std::string_view def_value)
{ return std::string{def_value}; });
}
-void StructuredConfiguration::parseSingleProperty(const std::string&
property_name, const Node& property_value_node, core::ConfigurableComponent&
processor,
+void StructuredConfiguration::parseSingleProperty(const std::string&
property_name, const Node& property_value_node, core::ConfigurableComponent&
component,
ParameterContext* parameter_context) {
- core::Property myProp(property_name, "", "");
- processor.getProperty(property_name, myProp);
-
- PropertyValue coercedValue =
getValidatedProcessorPropertyForDefaultTypeInfo(myProp, property_value_node,
parameter_context);
+ auto my_prop = component.getPropertyReference(property_name);
+ const bool is_sensitive = my_prop ? my_prop->is_sensitive : false;
+ const std::optional<std::string_view> default_value = my_prop ?
my_prop->default_value : std::nullopt;
- bool property_set = false;
- try {
- property_set = processor.setProperty(myProp, coercedValue);
- } catch(const utils::internal::InvalidValueException&) {
- auto component = dynamic_cast<core::CoreComponent*>(&processor);
- if (component == nullptr) {
- logger_->log_error("processor was not a CoreComponent for property
'{}'", property_name);
- } else {
- logger_->log_error("Invalid value was set for property '{}' creating
component '{}'", property_name, component->getName());
- }
- throw;
+ const auto value_to_set = getReplacedParametersValueOrDefault(property_name,
is_sensitive, default_value, property_value_node, parameter_context);
+ if (!value_to_set) {
+ return;
}
- if (!property_set) {
- const auto rawValueString = coercedValue.getValue()->getStringValue();
- auto proc = dynamic_cast<core::Connectable*>(&processor);
- if (proc) {
- logger_->log_warn("Received property {} but is not one of the properties
for {}. Attempting to add as dynamic property.", property_name,
proc->getName());
- if (!processor.setDynamicProperty(property_name, rawValueString)) {
- logger_->log_warn("Unable to set the dynamic property {}",
property_name);
- } else {
- logger_->log_warn("Dynamic property {} has been set", property_name);
- }
+ if (my_prop) {
+ const auto prop_set = component.setProperty(property_name, *value_to_set);
+ if (!prop_set) {
+ logger_->log_error("Invalid value was set for property '{}' creating
component '{}'", property_name, component.getName());
+ raiseComponentError(component.getName(), "", prop_set.error().message());
Review Comment:
We should distinguish the cases of `NotSupportedProperty` and
`ValidationFailed` here, too.
--
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]
