fgerlits commented on code in PR #1926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1926#discussion_r1993490744
##########
libminifi/test/unit/PropertyTests.cpp:
##########
Review Comment:
I would remove this file completely; it's only testing `trimLeft` and
`trimRight` now, which have better tests in `StringUtilsTests.cpp`.
##########
libminifi/test/unit/OptionalTest.cpp:
##########
@@ -77,3 +77,19 @@ TEST_CASE("optional filter", "[optional][filter]") {
REQUIRE(7 == (std::make_optional(7) | utils::filter([](int i) { return i % 2
== 1; })).value());
REQUIRE(std::nullopt == (std::make_optional(8) | utils::filter([](int i) {
return i % 2 == 1; })));
}
+
+TEST_CASE("optional toExpected") {
+ std::optional<int> opt_with_value = 5;
+ std::optional<int> opt_without_value = std::nullopt;
+
+ nonstd::expected<int, std::error_code> expected_from_value_ec =
opt_with_value | utils::toExpected(std::make_error_code(std::io_errc::stream));
+ nonstd::expected<int, std::error_code> expected_from_null_opt_ec =
opt_without_value |
utils::toExpected(std::make_error_code(std::io_errc::stream));
+ nonstd::expected<int, int> expected_from_value_int = opt_with_value |
utils::toExpected(9);
+ nonstd::expected<int, int> expected_from_null_opt_int = opt_without_value |
utils::toExpected(9);
+
+ CHECK(expected_from_value_ec == 5);
+ CHECK(expected_from_value_int == 5);
+
+ CHECK(expected_from_null_opt_ec.error() ==
std::make_error_code(std::io_errc::stream));
+ CHECK(expected_from_null_opt_int.error() == 9);
Review Comment:
somewhat paranoid, but in theory calling `error()` on an `unexpected` with
an expected value is UB, so
```suggestion
CHECK(!expected_from_null_opt_ec && expected_from_null_opt_ec.error() ==
std::make_error_code(std::io_errc::stream));
CHECK(!expected_from_null_opt_ec && expected_from_null_opt_int.error() ==
9);
```
##########
libminifi/test/unit/PropertyValidationTests.cpp:
##########
@@ -17,77 +17,57 @@
#include "unit/TestBase.h"
#include "unit/Catch.h"
-#include "core/ConfigurableComponent.h"
#include "core/PropertyDefinitionBuilder.h"
-#include "utils/PropertyErrors.h"
#include "core/PropertyType.h"
-#include "core/PropertyValue.h"
+#include "utils/PropertyErrors.h"
namespace org::apache::nifi::minifi::core {
-/**
- * This Tests checks a deprecated behavior that should be removed
- * in the next major release.
- */
-TEST_CASE("Some default values get coerced to typed variants") {
- auto prop = Property("prop", "d", "true");
- REQUIRE_THROWS_AS(prop.setValue("banana"),
utils::internal::ConversionException);
-
- const std::string SPACE = " ";
- auto prop2 = Property("prop", "d", SPACE + "true");
- prop2.setValue("banana");
-}
TEST_CASE("Converting invalid PropertyValue") {
static constexpr auto property_definition =
PropertyDefinitionBuilder<>::createProperty("prop")
- .withPropertyType(core::StandardPropertyTypes::INTEGER_TYPE)
+ .withValidator(core::StandardPropertyTypes::INTEGER_VALIDATOR)
.withDefaultValue("0")
.build();
Property property{property_definition};
- REQUIRE_THROWS_AS(property.setValue("not int"),
utils::internal::ParseException);
- auto cast_check = [&]{ return static_cast<int>(property.getValue()) == 0; };
// To avoid unused-value warning
- REQUIRE_THROWS_AS(cast_check(), utils::internal::InvalidValueException);
+ CHECK_FALSE(property.setValue("not int").has_value());
+ CHECK(property.getValue() == "0");
}
TEST_CASE("Parsing int has baggage after") {
static constexpr auto property_definition =
PropertyDefinitionBuilder<>::createProperty("prop")
- .withPropertyType(core::StandardPropertyTypes::INTEGER_TYPE)
+ .withValidator(core::StandardPropertyTypes::INTEGER_VALIDATOR)
.withDefaultValue("0")
.build();
Property property{property_definition};
- REQUIRE_THROWS_AS(property.setValue("55almost int"),
utils::internal::ParseException);
+ CHECK_FALSE(property.setValue("not int").has_value());
+ CHECK(property.getValue() == "0");
}
TEST_CASE("Parsing int has spaces") {
static constexpr auto property_definition =
PropertyDefinitionBuilder<>::createProperty("prop")
- .withPropertyType(core::StandardPropertyTypes::INTEGER_TYPE)
- .withDefaultValue("0")
- .build();
- Property property{property_definition};
- property.setValue(" 55 ");
- REQUIRE(static_cast<int>(property.getValue()) == 55);
-}
-
-TEST_CASE("Parsing int out of range") {
- static constexpr auto property_definition =
PropertyDefinitionBuilder<>::createProperty("prop")
- .withPropertyType(core::StandardPropertyTypes::INTEGER_TYPE)
+ .withValidator(core::StandardPropertyTypes::INTEGER_VALIDATOR)
.withDefaultValue("0")
.build();
Property property{property_definition};
- REQUIRE_THROWS_AS(property.setValue(" 5000000000 "),
utils::internal::ParseException);
Review Comment:
This test should still work, it just needs ten more zeros and a different
error condition -- no need to delete it.
##########
extensions/standard-processors/processors/GetTCP.cpp:
##########
@@ -86,33 +88,26 @@ std::optional<asio::ssl::context>
GetTCP::parseSSLContext(core::ProcessContext&
}
uint64_t GetTCP::parseMaxBatchSize(core::ProcessContext& context) {
- if (auto max_batch_size = context.getProperty<uint64_t>(MaxBatchSize)) {
- if (*max_batch_size == 0) {
- throw Exception(PROCESS_SCHEDULE_EXCEPTION, fmt::format("{} should be
non-zero.", MaxBatchSize.name));
- }
- return *max_batch_size;
- }
- static_assert(MaxBatchSize.default_value);
- return MaxBatchSize.type->parse(*MaxBatchSize.default_value);
+ return utils::parseU64Property(context, MaxBatchSize);
Review Comment:
we used to throw if the value is zero, and now we don't -- why?
##########
minifi-api/include/minifi-cpp/core/ProcessSessionReadCallback.h:
##########
Review Comment:
This has confused the github diff tool, but it has confused me, too: why was
`libminifi/include/core/ProcessSessionReadCallback.h` copied to
`minifi-api/include/minifi-cpp/core/ProcessSessionReadCallback.h`?
##########
libminifi/test/unit/PropertyValidationTests.cpp:
##########
Review Comment:
It would be good to have at least one test for each validator type:
`NON_BLANK_VALIDATOR`, `UNSIGNED_INTEGER_VALIDATOR`, `DATA_SIZE_VALIDATOR` and
`PORT_VALIDATOR` are missing. Or are they tested somewhere else?
##########
extensions/expression-language/ProcessContextExpr.h:
##########
@@ -50,24 +50,18 @@ class ProcessContextExpr final : public
core::ProcessContextImpl {
~ProcessContextExpr() override = default;
- bool getProperty(const Property& property, std::string &value, const
FlowFile* const flow_file) override;
+ nonstd::expected<std::string, std::error_code> getProperty(std::string_view
name, const FlowFile*) const override;
+ nonstd::expected<std::string, std::error_code>
getDynamicProperty(std::string_view name, const FlowFile*) const override;
- bool getProperty(const PropertyReference& property, std::string &value,
const FlowFile* const flow_file) override;
+ nonstd::expected<void, std::error_code> setProperty(std::string_view name,
std::string value) override;
+ nonstd::expected<void, std::error_code> setDynamicProperty(std::string name,
std::string value) override;
- bool getDynamicProperty(const Property &property, std::string &value, const
FlowFile* const flow_file) override;
-
- bool setProperty(const std::string& property, std::string value) override;
-
- bool setDynamicProperty(const std::string& property, std::string value)
override;
-
- using ProcessContextImpl::getProperty;
+ std::map<std::string, std::string> getDynamicProperties(const FlowFile*)
const override;
private:
- bool getProperty(bool supports_expression_language, std::string_view
property_name, std::string& value, const FlowFile* const flow_file);
+ mutable std::unordered_map<std::string, expression::Expression,
utils::string::transparent_string_hash, std::equal_to<>> cached_expressions_;
+ mutable std::unordered_map<std::string, expression::Expression,
utils::string::transparent_string_hash, std::equal_to<>>
cached_dynamic_expressions_;
Review Comment:
Why is `transparent_string_hash` needed? Would the default
`std::hash<std::string>` not work?
##########
utils/include/core/PropertyType.h:
##########
@@ -365,8 +49,6 @@ enum class PropertyTypeCode : int64_t {
PORT = 6
};
Review Comment:
I think the `PropertyTypeCode` class can be deleted, as well.
##########
utils/include/core/PropertyType.h:
##########
@@ -20,340 +20,24 @@
#include <memory>
#include <string>
#include <utility>
+#include <minifi-cpp/core/PropertyValidator.h>
-#include "minifi-cpp/core/PropertyType.h"
-#include "PropertyValue.h"
-#include "minifi-cpp/core/state/Value.h"
-#include "TypedValues.h"
#include "utils/StringUtils.h"
namespace org::apache::nifi::minifi::core {
-class PropertyTypeImpl : public PropertyType {
- public:
- virtual constexpr ~PropertyTypeImpl() {} // NOLINT can't use = default
because of gcc bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93413
-
- [[nodiscard]] PropertyValue parse(std::string_view input) const override;
-
- protected:
- template<typename T>
- [[nodiscard]] ValidationResult _validate_internal(const std::string
&subject, const std::shared_ptr<minifi::state::response::Value> &input) const {
- if (std::dynamic_pointer_cast<T>(input) != nullptr) {
- return ValidationResult{.valid = true, .subject = subject, .input =
input->getStringValue()};
- } else {
- state::response::ValueNode vn;
- vn = input->getStringValue();
- return validate(subject, input->getStringValue());
- }
- }
-};
-
-class ConstantPropertyType : public PropertyTypeImpl {
- public:
- explicit constexpr ConstantPropertyType(bool value) : value_(value) {}
-
- constexpr ~ConstantPropertyType() override {} // NOLINT see comment at
parent
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::shared_ptr<minifi::state::response::Value> &input) const override {
- return ValidationResult{.valid = value_, .subject = subject, .input =
input->getStringValue()};
- }
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::string &input) const override {
- return ValidationResult{.valid = value_, .subject = subject, .input =
input};
- }
-
- private:
- bool value_;
-};
-
-class AlwaysValidPropertyType : public ConstantPropertyType {
- public:
- constexpr AlwaysValidPropertyType() : ConstantPropertyType{true} {}
-
- constexpr ~AlwaysValidPropertyType() override {} // NOLINT see comment at
parent
-
- [[nodiscard]] std::string_view getValidatorName() const override { return
"VALID"; }
-};
-
-class NeverValidPropertyType : public ConstantPropertyType {
- public:
- constexpr NeverValidPropertyType() : ConstantPropertyType{false} {}
-
- constexpr ~NeverValidPropertyType() override {} // NOLINT see comment at
parent
-
- [[nodiscard]] std::string_view getValidatorName() const override { return
"INVALID"; }
-};
-
-class BooleanPropertyType : public PropertyTypeImpl {
- public:
- constexpr ~BooleanPropertyType() override {} // NOLINT see comment at parent
-
- [[nodiscard]] std::string_view getValidatorName() const override { return
"BOOLEAN_VALIDATOR"; }
-
- [[nodiscard]] PropertyValue parse(std::string_view input) const override;
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::shared_ptr<minifi::state::response::Value> &input) const override {
- return
PropertyTypeImpl::_validate_internal<minifi::state::response::BoolValue>(subject,
input);
- }
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::string &input) const override {
- if (utils::string::equalsIgnoreCase(input, "true") ||
utils::string::equalsIgnoreCase(input, "false"))
- return ValidationResult{.valid = true, .subject = subject, .input =
input};
- else
- return ValidationResult{.valid = false, .subject = subject, .input =
input};
- }
-};
-
-class IntegerPropertyType : public PropertyTypeImpl {
- public:
- constexpr ~IntegerPropertyType() override {} // NOLINT see comment at parent
-
- [[nodiscard]] std::string_view getValidatorName() const override { return
"INTEGER_VALIDATOR"; }
-
- [[nodiscard]] PropertyValue parse(std::string_view input) const override;
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::shared_ptr<minifi::state::response::Value> &input) const override {
- return
PropertyTypeImpl::_validate_internal<minifi::state::response::IntValue>(subject,
input);
- }
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::string &input) const override {
- try {
- (void) std::stoi(input);
- return ValidationResult{.valid = true, .subject = subject, .input =
input};
- } catch (...) {
- }
- return ValidationResult{.valid = false, .subject = subject, .input =
input};
- }
-};
-
-class UnsignedIntPropertyType : public PropertyTypeImpl {
- public:
- constexpr ~UnsignedIntPropertyType() override {} // NOLINT see comment at
parent
-
- [[nodiscard]] std::string_view getValidatorName() const override { return
"NON_NEGATIVE_INTEGER_VALIDATOR"; }
-
- [[nodiscard]] PropertyValue parse(std::string_view input) const override;
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::shared_ptr<minifi::state::response::Value> &input) const override {
- return
PropertyTypeImpl::_validate_internal<minifi::state::response::UInt32Value>(subject,
input);
- }
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::string &input) const override {
- try {
- auto negative = input.find_first_of('-') != std::string::npos;
- if (negative) {
- throw std::out_of_range("non negative expected");
- }
- (void) std::stoul(input);
- return ValidationResult{.valid = true, .subject = subject, .input =
input};
- } catch (...) {
- }
- return ValidationResult{.valid = false, .subject = subject, .input =
input};
- }
-};
-
-class LongPropertyType : public PropertyType {
- public:
- explicit constexpr LongPropertyType(int64_t min =
std::numeric_limits<int64_t>::min(), int64_t max =
std::numeric_limits<int64_t>::max())
- : min_(min),
- max_(max) {
- }
-
- constexpr ~LongPropertyType() override {} // NOLINT see comment at parent
-
- [[nodiscard]] std::string_view getValidatorName() const override { return
"LONG_VALIDATOR"; }
-
- [[nodiscard]] PropertyValue parse(std::string_view input) const override;
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::shared_ptr<minifi::state::response::Value> &input) const override {
- if (auto in64 =
std::dynamic_pointer_cast<minifi::state::response::Int64Value>(input)) {
- return ValidationResult{.valid = in64->getValue() >= min_ &&
in64->getValue() <= max_, .subject = subject, .input = in64->getStringValue()};
- } else if (auto intb =
std::dynamic_pointer_cast<minifi::state::response::IntValue>(input)) {
- return ValidationResult{.valid = intb->getValue() >= min_ &&
intb->getValue() <= max_, .subject = subject, .input = intb->getStringValue()};
- } else {
- return validate(subject, input->getStringValue());
- }
- }
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::string &input) const override {
- try {
- auto res = std::stoll(input);
-
- return ValidationResult{.valid = res >= min_ && res <= max_, .subject =
subject, .input = input};
- } catch (...) {
- }
- return ValidationResult{.valid = false, .subject = subject, .input =
input};
- }
-
- private:
- int64_t min_;
- int64_t max_;
-};
-
-class UnsignedLongPropertyType : public PropertyTypeImpl {
- public:
- explicit constexpr UnsignedLongPropertyType(uint64_t min =
std::numeric_limits<uint64_t>::min(), uint64_t max =
std::numeric_limits<uint64_t>::max())
- : min_(min),
- max_(max) {
- }
-
- constexpr ~UnsignedLongPropertyType() override {} // NOLINT see comment at
parent
-
- [[nodiscard]] std::string_view getValidatorName() const override { return
"LONG_VALIDATOR"; } // name is used by java nifi validators, so we should keep
this as LONG instead of UNSIGNED_LONG
-
- [[nodiscard]] PropertyValue parse(std::string_view input) const override;
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::shared_ptr<minifi::state::response::Value> &input) const override {
- return
PropertyTypeImpl::_validate_internal<minifi::state::response::UInt64Value>(subject,
input);
- }
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::string &input) const override {
- try {
- auto negative = input.find_first_of('-') != std::string::npos;
- if (negative) {
- throw std::out_of_range("non negative expected");
- }
- auto res = std::stoull(input);
- return ValidationResult{.valid = res >= min_ && res <= max_, .subject =
subject, .input = input};
- } catch (...) {
- }
- return ValidationResult{.valid = false, .subject = subject, .input =
input};
- }
-
- private:
- uint64_t min_;
- uint64_t max_;
-};
-
-class NonBlankPropertyType : public PropertyTypeImpl {
- public:
- constexpr ~NonBlankPropertyType() override {} // NOLINT see comment at
parent
-
- [[nodiscard]] std::string_view getValidatorName() const override { return
"NON_BLANK_VALIDATOR"; }
-
- [[nodiscard]] ValidationResult validate(const std::string& subject, const
std::shared_ptr<minifi::state::response::Value>& input) const final {
- return validate(subject, input->getStringValue());
- }
-
- [[nodiscard]] ValidationResult validate(const std::string& subject, const
std::string& input) const final {
- return ValidationResult{.valid = !utils::string::trimLeft(input).empty(),
.subject = subject, .input = input};
- }
-};
-
-class DataSizePropertyType : public PropertyTypeImpl {
- public:
- constexpr ~DataSizePropertyType() override {} // NOLINT see comment at
parent
-
- [[nodiscard]] std::string_view getValidatorName() const override { return
"DATA_SIZE_VALIDATOR"; }
-
- [[nodiscard]] PropertyValue parse(std::string_view input) const override;
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::shared_ptr<minifi::state::response::Value> &input) const override {
- return PropertyTypeImpl::_validate_internal<core::DataSizeValue>(subject,
input);
- }
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::string &input) const override {
- uint64_t out;
- return ValidationResult{.valid = core::DataSizeValue::StringToInt(input,
out), .subject = subject, .input = input};
- }
-};
-
-class PortPropertyType : public LongPropertyType {
- public:
- constexpr PortPropertyType()
- : LongPropertyType(1, 65535) {
- }
-
- constexpr ~PortPropertyType() override {} // NOLINT see comment at parent
-
- [[nodiscard]] std::string_view getValidatorName() const override { return
"PORT_VALIDATOR"; }
-};
-
-// Use only for specifying listen ports, where 0 means a randomly chosen one!
-class ListenPortValidator : public LongPropertyType {
- public:
- constexpr ListenPortValidator()
- : LongPropertyType(0, 65535) {
- }
-
- constexpr ~ListenPortValidator() override {} // NOLINT see comment at parent
-
- [[nodiscard]] std::string_view getValidatorName() const override { return
"PORT_VALIDATOR"; }
-};
-
-class TimePeriodPropertyType : public PropertyTypeImpl {
- public:
- constexpr ~TimePeriodPropertyType() override {} // NOLINT see comment at
parent
-
- [[nodiscard]] std::string_view getValidatorName() const override { return
"TIME_PERIOD_VALIDATOR"; }
-
- [[nodiscard]] PropertyValue parse(std::string_view input) const override;
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::shared_ptr<minifi::state::response::Value> &input) const override {
- return
PropertyTypeImpl::_validate_internal<core::TimePeriodValue>(subject, input);
- }
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::string &input) const override {
- auto parsed_time =
utils::timeutils::StringToDuration<std::chrono::milliseconds>(input);
- return ValidationResult{.valid = parsed_time.has_value(), .subject =
subject, .input = input};
- }
-};
-
-class DataTransferSpeedPropertyType : public PropertyTypeImpl {
- public:
- constexpr ~DataTransferSpeedPropertyType() override {} // NOLINT see
comment at parent
-
- [[nodiscard]] std::string_view getValidatorName() const override { return
"VALID"; }
-
- [[nodiscard]] PropertyValue parse(std::string_view input) const override;
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::shared_ptr<minifi::state::response::Value> &input) const override {
- return
PropertyTypeImpl::_validate_internal<core::DataTransferSpeedValue>(subject,
input);
- }
-
- [[nodiscard]] ValidationResult validate(const std::string &subject, const
std::string &input) const override {
- uint64_t out;
- return ValidationResult{.valid =
core::DataTransferSpeedValue::StringToInt(input, out), .subject = subject,
.input = input};
- }
-};
namespace StandardPropertyTypes {
-inline constexpr auto INVALID_TYPE = NeverValidPropertyType{};
-inline constexpr auto INTEGER_TYPE = IntegerPropertyType{};
-inline constexpr auto UNSIGNED_INT_TYPE = UnsignedIntPropertyType{};
-inline constexpr auto LONG_TYPE = LongPropertyType{};
-inline constexpr auto UNSIGNED_LONG_TYPE = UnsignedLongPropertyType{};
-inline constexpr auto BOOLEAN_TYPE = BooleanPropertyType{};
-inline constexpr auto DATA_SIZE_TYPE = DataSizePropertyType{};
-inline constexpr auto TIME_PERIOD_TYPE = TimePeriodPropertyType{};
-inline constexpr auto NON_BLANK_TYPE = NonBlankPropertyType{};
-inline constexpr auto VALID_TYPE = AlwaysValidPropertyType{};
-inline constexpr auto PORT_TYPE = PortPropertyType{};
-inline constexpr auto LISTEN_PORT_TYPE = ListenPortValidator{};
-inline constexpr auto DATA_TRANSFER_SPEED_TYPE =
DataTransferSpeedPropertyType{};
+inline constexpr auto ALWAYS_VALID_VALIDATOR = AlwaysValidValidator{};
+inline constexpr auto NON_BLANK_VALIDATOR = NonBlankValidator{};
+inline constexpr auto TIME_PERIOD_VALIDATOR = TimePeriodValidator{};
+inline constexpr auto BOOLEAN_VALIDATOR = BooleanValidator{};
+inline constexpr auto INTEGER_VALIDATOR = IntegerValidator{};
+inline constexpr auto UNSIGNED_INTEGER_VALIDATOR = UnsignedIntegerValidator{};
+inline constexpr auto DATA_SIZE_VALIDATOR = DataSizeValidator{};
+inline constexpr auto PORT_VALIDATOR = PortValidator{};
Review Comment:
Ideally, these standard validators should move to a
`StandardPropertyValidators` namespace in `PropertyValidator.h`, but since that
would add a lot more diffs to this already huge PR, we can do that later.
--
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]
