fgerlits commented on code in PR #1926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1926#discussion_r1977778331
##########
extension-utils/src/core/ProcessContext.cpp:
##########
@@ -16,21 +16,7 @@
*/
#include "minifi-cpp/core/ProcessContext.h"
-#include "core/TypedValues.h"
namespace org::apache::nifi::minifi::core {
-bool ProcessContext::getProperty(std::string_view name, detail::NotAFlowFile
auto &value) const {
Review Comment:
`NotAFlowFile` is not used anywhere now, so it can be removed from the
header file.
##########
extension-utils/src/utils/ListingStateManager.cpp:
##########
@@ -51,10 +51,11 @@ uint64_t
ListingStateManager::getLatestListedKeyTimestampInMilliseconds(const st
stored_listed_key_timestamp_str = it->second;
}
- int64_t stored_listed_key_timestamp = 0;
- core::Property::StringToInt(stored_listed_key_timestamp_str,
stored_listed_key_timestamp);
+ if (auto stored_listed_key_timestamp =
parsing::parseIntegral<int64_t>(stored_listed_key_timestamp_str)) {
+ return *stored_listed_key_timestamp;
+ }
- return stored_listed_key_timestamp;
+ throw std::runtime_error("Invalid listed key timestamp");
Review Comment:
Until now, we simply ignored the error (which is very unlikely). I would
prefer to log an error and continue to ignore it as before. We could also put
in an assertion. Throwing and rolling back the flow file (which most likely
did not cause the error) doesn't seem like the correct way to handle this.
##########
extension-utils/src/utils/ProcessorConfigUtils.cpp:
##########
@@ -16,37 +16,93 @@
*/
#include "utils/ProcessorConfigUtils.h"
-#include <vector>
#include <string>
-#include <string_view>
-#include "range/v3/algorithm/contains.hpp"
-#include "utils/StringUtils.h"
+#include "utils/expected.h"
namespace org::apache::nifi::minifi::utils {
-std::vector<std::string> listFromCommaSeparatedProperty(const
core::ProcessContext& context, std::string_view property_name) {
- std::string property_string;
- context.getProperty(property_name, property_string);
- return utils::string::splitAndTrim(property_string, ",");
+std::string parseProperty(const core::ProcessContext& ctx, const
core::PropertyReference& property, const core::FlowFile* flow_file) {
+ return ctx.getProperty(property, flow_file) |
utils::expect(fmt::format("Expected valid value from {}::{}",
ctx.getProcessor().getName(), property.name));
}
-std::vector<std::string> listFromRequiredCommaSeparatedProperty(const
core::ProcessContext& context, std::string_view property_name) {
- return utils::string::splitAndTrim(getRequiredPropertyOrThrow(context,
property_name), ",");
+std::optional<std::string> parseOptionalProperty(const core::ProcessContext&
ctx, const core::PropertyReference& property, const core::FlowFile* flow_file) {
+ if (const auto property_str = ctx.getProperty(property, flow_file)) {
+ return *property_str;
+ }
+ return std::nullopt;
+}
+
+std::optional<bool> parseOptionalBoolProperty(const core::ProcessContext& ctx,
const core::PropertyReference& property, const core::FlowFile* flow_file) {
+ if (const auto property_str = ctx.getProperty(property, flow_file)) {
+ return parsing::parseBool(*property_str) |
utils::expect(fmt::format("Expected parsable bool from {}::{}",
ctx.getProcessor().getName(), property.name));
+ }
+ return std::nullopt;
+}
+
+bool parseBoolProperty(const core::ProcessContext& ctx, const
core::PropertyReference& property, const core::FlowFile* flow_file) {
+ return ctx.getProperty(property, flow_file) |
utils::andThen(parsing::parseBool) | utils::expect(fmt::format("Expected
parsable bool from {}::{}", ctx.getProcessor().getName(), property.name));
+}
+
+std::optional<uint64_t> parseOptionalU64Property(const core::ProcessContext&
ctx, const core::PropertyReference& property, const core::FlowFile* flow_file) {
+ if (const auto property_str = ctx.getProperty(property, flow_file)) {
+ if (property_str->empty()) {
+ return std::nullopt;
+ }
+ return parsing::parseIntegral<uint64_t>(*property_str) |
utils::expect(fmt::format("Expected parsable uint64_t from {}::{}",
ctx.getProcessor().getName(), property.name));
+ }
+
+ return std::nullopt;
+}
+
+uint64_t parseU64Property(const core::ProcessContext& ctx, const
core::PropertyReference& property, const core::FlowFile* flow_file) {
+ return ctx.getProperty(property, flow_file) |
utils::andThen(parsing::parseIntegral<uint64_t>) |
utils::expect(fmt::format("Expected parsable uint64_t from {}::{}",
ctx.getProcessor().getName(), property.name));
+}
+
+std::optional<int64_t> parseOptionalI64Property(const core::ProcessContext&
ctx, const core::PropertyReference& property, const core::FlowFile* flow_file) {
+ if (const auto property_str = ctx.getProperty(property, flow_file)) {
+ if (property_str->empty()) {
+ return std::nullopt;
+ }
+ return parsing::parseIntegral<int64_t>(*property_str) |
utils::expect(fmt::format("Expected parsable int64_t from {}::{}",
ctx.getProcessor().getName(), property.name));
+ }
+
+ return std::nullopt;
}
-bool parseBooleanPropertyOrThrow(const core::ProcessContext& context,
std::string_view property_name) {
- const std::string value_str = getRequiredPropertyOrThrow(context,
property_name);
- const auto maybe_value = utils::string::toBool(value_str);
- if (!maybe_value) {
- throw std::runtime_error(std::string(property_name) + " property is
invalid: value is " + value_str);
+int64_t parseI64Property(const core::ProcessContext& ctx, const
core::PropertyReference& property, const core::FlowFile* flow_file) {
+ return ctx.getProperty(property, flow_file) |
utils::andThen(parsing::parseIntegral<int64_t>) |
utils::expect(fmt::format("Expected parsable int64_t from {}::{}",
ctx.getProcessor().getName(), property.name));
+}
+
+std::optional<std::chrono::milliseconds> parseOptionalMsProperty(const
core::ProcessContext& ctx, const core::PropertyReference& property, const
core::FlowFile* flow_file) {
+ if (const auto property_str = ctx.getProperty(property, flow_file)) {
+ if (property_str->empty()) {
+ return std::nullopt;
+ }
+ return parsing::parseDuration(*property_str) |
utils::expect(fmt::format("Expected parsable duration from {}::{}",
ctx.getProcessor().getName(), property.name));
}
- return maybe_value.value();
+
+ return std::nullopt;
}
-std::chrono::milliseconds parseTimePropertyMSOrThrow(const
core::ProcessContext& context, std::string_view property_name) {
- const auto time_property =
getRequiredPropertyOrThrow<core::TimePeriodValue>(context, property_name);
- return time_property.getMilliseconds();
+std::chrono::milliseconds parseMsProperty(const core::ProcessContext& ctx,
const core::PropertyReference& property, const core::FlowFile* flow_file) {
Review Comment:
I would prefer `parse[Optional]DurationProperty`, since the property value
is a duration which can use many other units, not just milliseconds. The return
type already shows that it gets converted to milliseconds.
##########
extensions/couchbase/controllerservices/CouchbaseClusterService.cpp:
##########
@@ -216,12 +216,10 @@ void CouchbaseClusterService::initialize() {
}
void CouchbaseClusterService::onEnable() {
- std::string connection_string;
- getProperty(ConnectionString, connection_string);
- std::string username;
- getProperty(UserName, username);
- std::string password;
- getProperty(UserPassword, password);
+ std::string connection_string = getProperty(ConnectionString.name) |
utils::expect("required property");
Review Comment:
This message is misleading, as this will only throw if `ConnectionString` is
not a supported property (ie., never).
Maybe we could have two types of `utils::expect` (with two different names,
of course), one that throws and one that terminates (for situations which
should be impossible)?
##########
extension-utils/src/utils/ProcessorConfigUtils.cpp:
##########
@@ -16,37 +16,93 @@
*/
#include "utils/ProcessorConfigUtils.h"
-#include <vector>
#include <string>
-#include <string_view>
-#include "range/v3/algorithm/contains.hpp"
-#include "utils/StringUtils.h"
+#include "utils/expected.h"
namespace org::apache::nifi::minifi::utils {
-std::vector<std::string> listFromCommaSeparatedProperty(const
core::ProcessContext& context, std::string_view property_name) {
- std::string property_string;
- context.getProperty(property_name, property_string);
- return utils::string::splitAndTrim(property_string, ",");
+std::string parseProperty(const core::ProcessContext& ctx, const
core::PropertyReference& property, const core::FlowFile* flow_file) {
+ return ctx.getProperty(property, flow_file) |
utils::expect(fmt::format("Expected valid value from {}::{}",
ctx.getProcessor().getName(), property.name));
}
-std::vector<std::string> listFromRequiredCommaSeparatedProperty(const
core::ProcessContext& context, std::string_view property_name) {
- return utils::string::splitAndTrim(getRequiredPropertyOrThrow(context,
property_name), ",");
+std::optional<std::string> parseOptionalProperty(const core::ProcessContext&
ctx, const core::PropertyReference& property, const core::FlowFile* flow_file) {
+ if (const auto property_str = ctx.getProperty(property, flow_file)) {
+ return *property_str;
+ }
+ return std::nullopt;
Review Comment:
this could be
```suggestion
return ctx.getProperty(property, flow_file)) | utils::toOptional();
```
##########
extension-utils/src/utils/ProcessorConfigUtils.cpp:
##########
Review Comment:
We could help the compiler eliminate a `PropertyDefinition` ->
`PropertyReference` conversion if we moved these functions to the header and
used the `string_view` version of `getProperty()` in their bodies, i.e.,
`ctx.getProperty(property.name, flow_file)`.
This may be premature optimization, but we use these functions in so many
places, it may be worth it.
##########
extensions/aws/s3/MultipartUploadStateStorage.cpp:
##########
@@ -57,11 +57,11 @@ std::optional<MultipartUploadState>
MultipartUploadStateStorage::getState(const
MultipartUploadState state;
state.upload_id = state_map[state_key + ".upload_id"];
- core::Property::StringToInt(state_map[state_key + ".upload_time"],
state.upload_time_ms_since_epoch);
- core::Property::StringToInt(state_map[state_key + ".uploaded_parts"],
state.uploaded_parts);
- core::Property::StringToInt(state_map[state_key + ".uploaded_size"],
state.uploaded_size);
- core::Property::StringToInt(state_map[state_key + ".part_size"],
state.part_size);
- core::Property::StringToInt(state_map[state_key + ".full_size"],
state.full_size);
+ state.upload_time_ms_since_epoch =
parsing::parseIntegral<int64_t>(state_map[state_key + ".upload_time"]) |
utils::expect("Expected parsable upload_time_ms_since_epoch");
+ state.uploaded_parts = parsing::parseIntegral<size_t>(state_map[state_key +
".uploaded_parts"]) | utils::expect("Expected parsable state.uploaded_parts");
+ state.uploaded_size = parsing::parseIntegral<uint64_t>(state_map[state_key +
".uploaded_size"]) | utils::expect("Expected parsable state.uploaded_size");
+ state.part_size = parsing::parseIntegral<uint64_t>(state_map[state_key +
".part_size"]) | utils::expect("Expected parsable state.part_size");
+ state.full_size = parsing::parseIntegral<uint64_t>(state_map[state_key +
".full_size"]) | utils::expect("Expected parsable state.full_size");
Review Comment:
not sure how useful it would be, but we could add the value of `state_key`
to the text of the exception
##########
extensions/aws/processors/S3Processor.cpp:
##########
@@ -155,8 +139,9 @@ std::optional<CommonProperties>
S3Processor::getCommonELSupportedProperties(
}
properties.proxy = proxy.value();
- context.getProperty(EndpointOverrideURL, properties.endpoint_override_url,
flow_file);
- if (!properties.endpoint_override_url.empty()) {
+ const auto endpoint_override_url = context.getProperty(EndpointOverrideURL,
flow_file);
+ if (endpoint_override_url) {
+ properties.endpoint_override_url = *endpoint_override_url;
Review Comment:
`EndpointOverrideURL` should have a `NON_BLANK_VALIDATOR`
--
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]
