lordgamez commented on code in PR #1926:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1926#discussion_r1991887637
##########
extensions/civetweb/processors/ListenHTTP.cpp:
##########
@@ -39,71 +40,26 @@ void ListenHTTP::initialize() {
}
void ListenHTTP::onSchedule(core::ProcessContext& context,
core::ProcessSessionFactory&) {
- std::string basePath;
+ std::string base_path = context.getProperty(BasePath) |
utils::expect("ListenHTTP::BasePath has default value");
Review Comment:
We could make the BasePath required with non blank validator
##########
extensions/standard-processors/tests/unit/GenerateFlowFileTests.cpp:
##########
@@ -102,7 +102,7 @@ TEST_CASE("GenerateFlowFileCustomTextEmptyTest") {
constexpr int32_t file_size = 10;
- test_controller.plan->setProperty(generate_flow_file,
GenerateFlowFile::FileSize, std::to_string(file_size));
+ CHECK(test_controller.plan->setProperty(generate_flow_file,
GenerateFlowFile::FileSize, std::to_string(file_size)));
Review Comment:
Why only this setProperty has a CHECK?
##########
extensions/standard-processors/processors/UpdateAttribute.cpp:
##########
@@ -33,15 +34,7 @@ void UpdateAttribute::initialize() {
setSupportedRelationships(Relationships);
}
-void UpdateAttribute::onSchedule(core::ProcessContext& context,
core::ProcessSessionFactory&) {
- attributes_.clear();
- const auto &dynamic_prop_keys = context.getDynamicPropertyKeys();
- logger_->log_info("UpdateAttribute registering {} keys",
dynamic_prop_keys.size());
-
- for (const auto &key : dynamic_prop_keys) {
-
attributes_.emplace_back(core::PropertyDefinitionBuilder<>::createProperty(key).withDescription("auto
generated").supportsExpressionLanguage(true).build());
- logger_->log_info("UpdateAttribute registered attribute '{}'", key);
- }
+void UpdateAttribute::onSchedule(core::ProcessContext&,
core::ProcessSessionFactory&) {
Review Comment:
This could be removed altogether
##########
extensions/standard-processors/tests/unit/LogAttributeTests.cpp:
##########
@@ -66,7 +66,7 @@ TEST_CASE("LogAttribute LogLevel and LogPrefix",
"[LogAttribute]") {
std::make_tuple("error", "",
"--------------------------------------------------"));
REQUIRE(controller.plan->setProperty(log_attribute, LogAttribute::LogLevel,
log_level));
- REQUIRE(controller.plan->setProperty(log_attribute, LogAttribute::LogPrefix,
log_prefix));
Review Comment:
We could still check this for non-empty log_prefix
##########
minifi-api/include/minifi-cpp/core/Property.h:
##########
@@ -53,158 +47,67 @@ class Property {
Property();
- Property(const PropertyReference&);
+ Property(const PropertyReference &);
virtual ~Property() = default;
- void setTransient() {
- is_transient_ = true;
- }
+ void setTransient() { is_transient_ = true; }
- bool isTransient() const {
- return is_transient_;
- }
+ bool isTransient() const { return is_transient_; }
+ std::vector<std::string> getAllowedValues() const { return allowed_values_; }
+ void setAllowedValues(std::vector<std::string> allowed_values) {
allowed_values_ = std::move(allowed_values); }
+ std::optional<std::string> getDefaultValue() const { return default_value_; }
std::string getName() const;
std::string getDisplayName() const;
std::vector<std::string> getAllowedTypes() const;
std::string getDescription() const;
const PropertyValidator& getValidator() const;
- const PropertyValue &getValue() const;
+ [[nodiscard]] nonstd::expected<std::span<const std::string>,
std::error_code> getAllValues() const;
+ [[nodiscard]] nonstd::expected<std::string_view, std::error_code> getValue()
const;
+ nonstd::expected<void, std::error_code> setValue(std::string value);
+ nonstd::expected<void, std::error_code> appendValue(std::string value);
+ void clearValues();
bool getRequired() const;
bool isSensitive() const;
bool supportsExpressionLanguage() const;
std::vector<std::string> getDependentProperties() const;
std::vector<std::pair<std::string, std::string>> getExclusiveOfProperties()
const;
std::vector<std::string> getValues();
-
- const PropertyValue &getDefaultValue() const {
- return default_value_;
- }
-
- template<typename T = std::string>
- void setValue(const T &value) {
- if (!is_collection_) {
- values_.clear();
- values_.push_back(default_value_);
- } else {
- values_.push_back(default_value_);
- }
- PropertyValue& vn = values_.back();
- vn.setValidator(*validator_);
- vn = value;
- ValidationResult result = vn.validate(name_);
- if (!result.valid) {
- throw utils::internal::InvalidValueException(name_ + " value validation
failed");
- }
+ PropertyReference getReference() const {
+ return PropertyReference(name_, display_name_, description_, is_required_,
is_sensitive_, {}, {}, {}, {}, default_value_, validator_, supports_el_);
}
- void setValue(PropertyValue &newValue) {
- if (!is_collection_) {
- values_.clear();
- values_.push_back(newValue);
- } else {
- values_.push_back(newValue);
- }
- PropertyValue& vn = values_.back();
- vn.setValidator(*validator_);
- ValidationResult result = vn.validate(name_);
- if (!result.valid) {
- throw utils::internal::InvalidValueException(name_ + " value validation
failed");
- }
- }
void setSupportsExpressionLanguage(bool supportEl);
- std::vector<PropertyValue> getAllowedValues() const {
- return allowed_values_;
- }
-
- void setAllowedValues(gsl::span<const std::string_view> allowed_values,
const core::PropertyParser& property_parser);
-
+ /**
+ * Add value to the collection of values.
+ */
void addValue(const std::string &value);
Property &operator=(const Property &other) = default;
Property &operator=(Property &&other) = default;
-
- bool operator<(const Property & right) const;
-
- static bool StringToPermissions(const std::string& input, uint32_t& output) {
- uint32_t temp = 0U;
- if (input.size() == 9U) {
- /* Probably rwxrwxrwx formatted */
- for (size_t i = 0; i < 3; i++) {
- if (input[i * 3] == 'r') {
- temp |= 04 << ((2 - i) * 3);
- } else if (input[i * 3] != '-') {
- return false;
- }
- if (input[i * 3 + 1] == 'w') {
- temp |= 02 << ((2 - i) * 3);
- } else if (input[i * 3 + 1] != '-') {
- return false;
- }
- if (input[i * 3 + 2] == 'x') {
- temp |= 01 << ((2 - i) * 3);
- } else if (input[i * 3 + 2] != '-') {
- return false;
- }
- }
- } else {
- /* Probably octal */
- try {
- size_t pos = 0U;
- temp = std::stoul(input, &pos, 8);
- if (pos != input.size()) {
- return false;
- }
- if ((temp & ~static_cast<uint32_t>(0777)) != 0U) {
- return false;
- }
- } catch (...) {
- return false;
- }
- }
- output = temp;
- return true;
- }
-
- template<typename T>
- static bool StringToInt(std::string input, T &output);
-
- static bool StringToInt(std::string input, int64_t &output) {
- return StringToInt<int64_t>(input, output);
- }
-
- static bool StringToInt(std::string input, uint64_t &output) {
- return StringToInt<uint64_t>(input, output);
- }
-
- static bool StringToInt(std::string input, int32_t &output) {
- return StringToInt<int32_t>(input, output);
- }
-
- static bool StringToInt(std::string input, uint32_t &output) {
- return StringToInt<uint32_t>(input, output);
- }
+ // Compare
Review Comment:
I think these comments can be removed
##########
libminifi/src/agent/JsonSchema.cpp:
##########
@@ -44,6 +44,7 @@ static std::string escape(std::string str) {
static std::string prettifyJson(const std::string& str) {
rapidjson::Document doc;
+ std::cout << str << std::endl;
Review Comment:
I suppose this could be removed
##########
extensions/rocksdb-repos/controllers/RocksDbStateStorage.cpp:
##########
@@ -45,19 +45,19 @@ void RocksDbStateStorage::onEnable() {
return;
}
- const auto always_persist = getProperty<bool>(AlwaysPersist).value_or(false);
+ const auto always_persist = getProperty(AlwaysPersist.name)
+ | utils::andThen(parsing::parseBool)
+ | utils::expect("RocksDbStateStorage::AlwaysPersist has default value");
logger_->log_info("Always Persist property: {}", always_persist);
- const auto auto_persistence_interval =
getProperty<core::TimePeriodValue>(AutoPersistenceInterval).value_or(core::TimePeriodValue{}).getMilliseconds();
+ const auto auto_persistence_interval =
getProperty(AutoPersistenceInterval.name)
+ | utils::andThen(parsing::parseDuration<std::chrono::milliseconds>)
+ | utils::expect("RocksDbStateStorage::AutoPersistenceInterval has
default value");
Review Comment:
We could also make these properties required if they are always needed and
already have a default value
##########
extensions/standard-processors/tests/unit/FlowJsonTests.cpp:
##########
@@ -1469,13 +1467,9 @@ TEST_CASE("Parameter context inheritance order is
respected") {
REQUIRE(flow);
auto* proc = flow->findProcessorByName("MyProcessor");
- std::string value;
- REQUIRE(proc->getDynamicProperty("My A Property", value));
- CHECK(value == "1");
- REQUIRE(proc->getDynamicProperty("My B Property", value));
- CHECK(value == "3");
- REQUIRE(proc->getDynamicProperty("My C Property", value));
- CHECK(value == "5");
+ REQUIRE("1" == proc->getDynamicProperty("My A Property"));
+ REQUIRE("3" == proc->getDynamicProperty("My B Property"));
+ REQUIRE("5" == proc->getDynamicProperty("My C Property"));
Review Comment:
minor, but I think it's better to leave these as CHECKs
##########
libminifi/src/core/controller/StandardControllerServiceNode.cpp:
##########
@@ -23,14 +23,11 @@
namespace org::apache::nifi::minifi::core::controller {
bool StandardControllerServiceNode::enable() {
+ controller_service_->setState(ENABLED);
logger_->log_trace("Enabling CSN {}", getName());
- if (active) {
- logger_->log_debug("CSN {} is already enabled", getName());
- return true;
- }
- Property property("Linked Services", "Referenced Controller Services");
- if (getProperty(property.getName(), property)) {
- for (const auto& linked_service : property.getValues()) {
+ if (auto linked_services = getAllPropertyValues("Linked Services")) {
+ active = true;
Review Comment:
What was the reason for this change? In PR
https://github.com/apache/nifi-minifi-cpp/pull/1933 I have also have some
changes for this state handling so we should align them.
##########
extensions/standard-processors/tests/unit/ManifestTests.cpp:
##########
@@ -78,7 +78,6 @@ TEST_CASE("Test Valid Regex", "[validRegex]") {
CHECK("required" == prop_0.children[3].name);
CHECK("sensitive" == prop_0.children[4].name);
CHECK("expressionLanguageScope" == prop_0.children[5].name);
- CHECK("defaultValue" == prop_0.children[6].name);
Review Comment:
How was the defaultValue removed from the manifest?
--
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]
