[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r446873735
##
File path: libminifi/include/utils/ValueParser.h
##
@@ -0,0 +1,183 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_VALUEPARSER_H_
+#define LIBMINIFI_INCLUDE_UTILS_VALUEPARSER_H_
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "PropertyErrors.h"
+#include "GeneralUtils.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+namespace internal {
+
+class ValueParser {
+ private:
+ template
+ struct is_non_narrowing_convertible : std::false_type {
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ template
+ struct is_non_narrowing_convertible()})>> : std::true_type {
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
Review comment:
done
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r446102381
##
File path: libminifi/include/core/Property.h
##
@@ -129,41 +133,32 @@ class Property {
template
void setValue(const T &value) {
-PropertyValue vn = default_value_;
-vn = value;
-if (validator_) {
- vn.setValidator(validator_);
- ValidationResult result = validator_->validate(name_, vn.getValue());
- if (!result.valid()) {
-// throw some exception?
- }
-} else {
- vn.setValidator(core::StandardValidators::VALID);
-}
if (!is_collection_) {
values_.clear();
- values_.push_back(vn);
+ values_.push_back(default_value_);
} else {
- values_.push_back(vn);
+ values_.push_back(default_value_);
}
+PropertyValue& vn = values_.back();
+vn.setValidator(validator_ ? validator_ : core::StandardValidators::VALID);
+vn = value;
+ValidationResult result = vn.validate(name_);
+if(!result.valid())
+ throw utils::InvalidValueException(name_ + " value validation failed");
}
- void setValue(PropertyValue &vn) {
-if (validator_) {
- vn.setValidator(validator_);
- ValidationResult result = validator_->validate(name_, vn.getValue());
- if (!result.valid()) {
-// throw some exception?
- }
-} else {
- vn.setValidator(core::StandardValidators::VALID);
-}
+ void setValue(PropertyValue &newValue) {
if (!is_collection_) {
values_.clear();
- values_.push_back(vn);
+ values_.push_back(newValue);
} else {
- values_.push_back(vn);
+ values_.push_back(newValue);
}
+PropertyValue& vn = values_.back();
+vn.setValidator(validator_ ? validator_ : core::StandardValidators::VALID);
Review comment:
propagated `gsl::not_null` all around, had to remove
`StandardValidator::VALID` as multiple statically initialized `Property`s use
it
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r446101051
##
File path: libminifi/include/core/CachedValueValidator.h
##
@@ -0,0 +1,129 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_CORE_CACHEDVALUEVALIDATOR_H_
+#define LIBMINIFI_INCLUDE_CORE_CACHEDVALUEVALIDATOR_H_
+
+#include
+#include
+#include
+#include "PropertyValidation.h"
+#include "state/Value.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+
+class PropertyValue;
+
+namespace internal {
+
+class CachedValueValidator {
+ friend class core::PropertyValue;
+
+ public:
+ enum class Result {
+FAILURE,
+SUCCESS,
+RECOMPUTE
+ };
+
+ CachedValueValidator() = default;
+
+ CachedValueValidator(const CachedValueValidator& other) :
validator_(other.validator_) {}
+
+ CachedValueValidator(CachedValueValidator&& other) noexcept :
validator_(std::move(other.validator_)) {}
+
+ CachedValueValidator& operator=(const CachedValueValidator& other) {
+if (this == &other) {
+ return *this;
+}
+validator_ = other.validator_;
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+
+ CachedValueValidator& operator=(CachedValueValidator&& other) {
+if (this == &other) {
+ return *this;
+}
+validator_ = std::move(other.validator_);
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+
+ explicit CachedValueValidator(const std::shared_ptr&
other) : validator_(other) {}
+
+ explicit CachedValueValidator(std::shared_ptr&& other) :
validator_(std::move(other)) {}
+
+ CachedValueValidator& operator=(const std::shared_ptr&
new_validator) {
+validator_ = new_validator;
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+
+ CachedValueValidator& operator=(std::shared_ptr&&
new_validator) {
+validator_ = std::move(new_validator);
+validation_result_ = Result::RECOMPUTE;
Review comment:
done
##
File path: libminifi/include/core/ConfigurableComponent.h
##
@@ -215,18 +215,23 @@ bool ConfigurableComponent::getProperty(const std::string
name, T &value) const
auto &&it = properties_.find(name);
if (it != properties_.end()) {
- Property item = it->second;
- value = static_cast(item.getValue());
- if (item.getValue().getValue() != nullptr) {
- logger_->log_debug("Component %s property name %s value %s", name,
item.getName(), item.getValue().to_string());
- return true;
- } else {
- logger_->log_warn("Component %s property name %s, empty value", name,
item.getName());
- return false;
- }
+const Property& item = it->second;
+if (item.getValue().getValue() == nullptr) {
+ // empty value
+ if (item.getRequired()) {
+logger_->log_debug("Component %s required property %s is empty", name,
item.getName());
+throw utils::internal::RequiredPropertyMissingException("Required
property is empty: " + item.getName());
+ }
+ logger_->log_warn("Component %s property name %s, empty value", name,
item.getName());
Review comment:
done
##
File path: libminifi/include/core/ConfigurableComponent.h
##
@@ -215,18 +215,23 @@ bool ConfigurableComponent::getProperty(const std::string
name, T &value) const
auto &&it = properties_.find(name);
if (it != properties_.end()) {
- Property item = it->second;
- value = static_cast(item.getValue());
- if (item.getValue().getValue() != nullptr) {
- logger_->log_debug("Component %s property name %s value %s", name,
item.getName(), item.getValue().to_string());
- return true;
- } else {
- logger_->log_warn("Component %s property name %s, empty value", name,
item.getName());
- return false;
- }
+const Property& item = it->second;
+if (item.getValue().getValue() == nullptr) {
+ // empty value
+ if (item.getRequired()) {
+logger_->log_debug("Component %s required property %s is empty", name,
item.getName());
+throw utils::internal::RequiredPropertyMissingException("Required
prop
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r446101347
##
File path: libminifi/test/unit/PropertyValidationTests.cpp
##
@@ -0,0 +1,238 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "../TestBase.h"
+#include "core/ConfigurableComponent.h"
+#include "utils/PropertyErrors.h"
+#include "core/PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+
+using namespace utils::internal;
+/**
+ * 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"), ConversionException);
+
+ const std::string SPACE = " ";
+ auto prop2 = Property("prop", "d", SPACE + "true");
+ prop2.setValue("banana");
+}
+
+TEST_CASE("Converting invalid PropertyValue") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->withDefaultValue(0)
+->build();
Review comment:
done
##
File path: libminifi/include/utils/ValueParser.h
##
@@ -0,0 +1,194 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_VALUEPARSER_H_
+#define LIBMINIFI_INCLUDE_UTILS_VALUEPARSER_H_
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "PropertyErrors.h"
+#include "GeneralUtils.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+namespace internal {
+
+class ValueParser {
+ private:
+ template
+ struct is_non_narrowing_convertible : std::false_type {
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ template
+ struct is_non_narrowing_convertible()})>> : std::true_type {
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ public:
+ explicit ValueParser(const std::string& str, std::size_t offset = 0) :
str(str), offset(offset) {}
+
+ template
+ ValueParser& parseInt(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from int");
+long result; // NOLINT
+auto len = safeCallConverter(std::strtol, result);
+if (len == 0) {
+ throw ParseException("Couldn't parse int");
+}
+if (result < (std::numeric_limits::min)() || result >
(std::numeric_limits::max)()) {
+ throw ParseException("Cannot convert long to int");
+}
+offset += len;
+out = {static_cast(result)};
+return *this;
+ }
+
+ template
+ ValueParser& parseLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from long"); // NOLINT
+long result; // NOLINT
+auto len = safeCallConverter(std::strtol, result);
+if (len == 0) {
+ throw ParseException("Couldn't parse long");
+}
+offset += len;
+out = {result};
+return *this;
+ }
+
+ template
+ ValueParser& parseLongLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value,
"Expected lossless conversion from long long"); // NOLINT
+long long result; // NOLINT
+auto len = safeCallConverter(std::strtoll, result);
+if (len == 0) {
+ throw ParseException("Couldn't parse long long");
+
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r446027617
##
File path: libminifi/include/utils/ValueParser.h
##
@@ -0,0 +1,194 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_VALUEPARSER_H_
+#define LIBMINIFI_INCLUDE_UTILS_VALUEPARSER_H_
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "PropertyErrors.h"
+#include "GeneralUtils.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+namespace internal {
+
+class ValueParser {
+ private:
+ template
+ struct is_non_narrowing_convertible : std::false_type {
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ template
+ struct is_non_narrowing_convertible()})>> : std::true_type {
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ public:
+ explicit ValueParser(const std::string& str, std::size_t offset = 0) :
str(str), offset(offset) {}
+
+ template
+ ValueParser& parseInt(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from int");
+long result; // NOLINT
+auto len = safeCallConverter(std::strtol, result);
+if (len == 0) {
+ throw ParseException("Couldn't parse int");
+}
+if (result < (std::numeric_limits::min)() || result >
(std::numeric_limits::max)()) {
+ throw ParseException("Cannot convert long to int");
+}
+offset += len;
+out = {static_cast(result)};
+return *this;
+ }
+
+ template
+ ValueParser& parseLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from long"); // NOLINT
+long result; // NOLINT
+auto len = safeCallConverter(std::strtol, result);
+if (len == 0) {
+ throw ParseException("Couldn't parse long");
+}
+offset += len;
+out = {result};
+return *this;
+ }
+
+ template
+ ValueParser& parseLongLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value,
"Expected lossless conversion from long long"); // NOLINT
+long long result; // NOLINT
+auto len = safeCallConverter(std::strtoll, result);
+if (len == 0) {
+ throw ParseException("Couldn't parse long long");
+}
+offset += len;
+out = {result};
+return *this;
+ }
+
+ template
+ ValueParser& parseUInt32(Out& out) {
+static_assert(is_non_narrowing_convertible::value,
"Expected lossless conversion from uint32_t");
+skipWhitespace();
+if (offset < str.length() && str[offset] == '-') {
+ throw ParseException("Not an unsigned long");
+}
+unsigned long result; // NOLINT
+auto len = safeCallConverter(std::strtoul, result);
+if (len == 0) {
+ throw ParseException("Couldn't parse uint32_t");
+}
+if (result > (std::numeric_limits::max)()) {
+ throw ParseException("Cannot convert unsigned long to uint32_t");
+}
+offset += len;
+out = {static_cast(result)};
+return *this;
+ }
+
+ template
+ ValueParser& parseUnsignedLongLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected lossless conversion from unsigned long long"); // NOLINT
+skipWhitespace();
+if (offset < str.length() && str[offset] == '-') {
+ throw ParseException("Not an unsigned long");
+}
+unsigned long long result; // NOLINT
+auto len = safeCallConverter(std::strtoull, result);
+if (len == 0) {
+ throw ParseException("Couldn't parse unsigned long long");
+}
+offset += len;
+out = {result};
+return *this;
+ }
+
+ template
+ ValueParser& parseBool(Out& out) {
Review comment:
I dig it, the only problem with this specific approach is that, template
function partial specialization is a no-go, but will think about something like
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 th
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r446008304
##
File path: libminifi/include/utils/PropertyErrors.h
##
@@ -0,0 +1,101 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_PROPERTYERRORS_H_
+#define LIBMINIFI_INCLUDE_UTILS_PROPERTYERRORS_H_
+
+#include
+
+#include "Exception.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+
+namespace core {
+
+class PropertyValue;
+class ConfigurableComponent;
+class Property;
+
+} /* namespace core */
+
+namespace utils {
+namespace internal {
+
+class ValueException : public Exception {
+ protected:
+ explicit ValueException(const std::string& err) :
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+ explicit ValueException(const char* err) :
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+
+ // base class already has a virtual destructor
+};
+
+class PropertyException : public Exception {
+ protected:
+ explicit PropertyException(const std::string& err) :
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+ explicit PropertyException(const char* err) :
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+
+ // base class already has a virtual destructor
+};
+
+/**
+ * Thrown during converting from and to Value
+ */
+class ConversionException : public ValueException {
+ public:
+ explicit ConversionException(const std::string& err) : ValueException(err) {}
+ explicit ConversionException(const char* err) : ValueException(err) {}
Review comment:
could you expand on what problem this would solve?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r446008007
##
File path: libminifi/include/utils/PropertyErrors.h
##
@@ -0,0 +1,101 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_PROPERTYERRORS_H_
+#define LIBMINIFI_INCLUDE_UTILS_PROPERTYERRORS_H_
+
+#include
+
+#include "Exception.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+
+namespace core {
+
+class PropertyValue;
+class ConfigurableComponent;
+class Property;
+
+} /* namespace core */
+
+namespace utils {
+namespace internal {
+
+class ValueException : public Exception {
+ protected:
+ explicit ValueException(const std::string& err) :
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+ explicit ValueException(const char* err) :
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+
+ // base class already has a virtual destructor
+};
+
+class PropertyException : public Exception {
+ protected:
+ explicit PropertyException(const std::string& err) :
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+ explicit PropertyException(const char* err) :
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+
+ // base class already has a virtual destructor
+};
+
+/**
+ * Thrown during converting from and to Value
+ */
+class ConversionException : public ValueException {
+ public:
+ explicit ConversionException(const std::string& err) : ValueException(err) {}
+ explicit ConversionException(const char* err) : ValueException(err) {}
Review comment:
could you expand on what problem this would solve?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r445999612
##
File path: libminifi/test/unit/PropertyValidationTests.cpp
##
@@ -0,0 +1,238 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "../TestBase.h"
+#include "core/ConfigurableComponent.h"
+#include "utils/PropertyErrors.h"
+#include "core/PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+
+using namespace utils::internal;
+/**
+ * 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"), ConversionException);
+
+ const std::string SPACE = " ";
+ auto prop2 = Property("prop", "d", SPACE + "true");
+ prop2.setValue("banana");
+}
+
+TEST_CASE("Converting invalid PropertyValue") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->withDefaultValue(0)
+->build();
+ REQUIRE_THROWS_AS(prop.setValue("not int"), ParseException);
+ REQUIRE_THROWS_AS(static_cast(prop.getValue()), InvalidValueException);
+}
+
+TEST_CASE("Parsing int has baggage after") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->withDefaultValue(0)
+->build();
+ REQUIRE_THROWS_AS(prop.setValue("55almost int"), ParseException);
+}
+
+TEST_CASE("Parsing int has spaces") {
+ auto prop = PropertyBuilder::createProperty("prop")
+ ->withDefaultValue(0)
+ ->build();
+ prop.setValue(" 55 ");
+ REQUIRE(static_cast(prop.getValue()) == 55);
+}
+
+TEST_CASE("Parsing int out of range") {
+ auto prop = PropertyBuilder::createProperty("prop")
+ ->withDefaultValue(0)
+ ->build();
+ REQUIRE_THROWS_AS(prop.setValue(" 50 "), ParseException);
Review comment:
(not only can it bigger, but it can also be as small as 16 bits)
I'll leave this for now as is, if this test fails due to the platform, it
should be ease to fix, and also warn us, that we should be more mindful about
`int` at other parts of the code
I would actually go as far as to remove the `int` (and other non-fixed with
integers) as a possible value type, as these are user facing and we can't have
a property validation fail on one agent but succeed on an other just because
they are running on different platforms
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r445999612
##
File path: libminifi/test/unit/PropertyValidationTests.cpp
##
@@ -0,0 +1,238 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "../TestBase.h"
+#include "core/ConfigurableComponent.h"
+#include "utils/PropertyErrors.h"
+#include "core/PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+
+using namespace utils::internal;
+/**
+ * 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"), ConversionException);
+
+ const std::string SPACE = " ";
+ auto prop2 = Property("prop", "d", SPACE + "true");
+ prop2.setValue("banana");
+}
+
+TEST_CASE("Converting invalid PropertyValue") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->withDefaultValue(0)
+->build();
+ REQUIRE_THROWS_AS(prop.setValue("not int"), ParseException);
+ REQUIRE_THROWS_AS(static_cast(prop.getValue()), InvalidValueException);
+}
+
+TEST_CASE("Parsing int has baggage after") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->withDefaultValue(0)
+->build();
+ REQUIRE_THROWS_AS(prop.setValue("55almost int"), ParseException);
+}
+
+TEST_CASE("Parsing int has spaces") {
+ auto prop = PropertyBuilder::createProperty("prop")
+ ->withDefaultValue(0)
+ ->build();
+ prop.setValue(" 55 ");
+ REQUIRE(static_cast(prop.getValue()) == 55);
+}
+
+TEST_CASE("Parsing int out of range") {
+ auto prop = PropertyBuilder::createProperty("prop")
+ ->withDefaultValue(0)
+ ->build();
+ REQUIRE_THROWS_AS(prop.setValue(" 50 "), ParseException);
Review comment:
(not only can it be bigger, but it can also be as small as 16 bits)
I'll leave this for now as is, if this test fails due to the platform, it
should be ease to fix, and also warn us, that we should be more mindful about
`int` at other parts of the code
I would actually go as far as to remove the `int` (and other non-fixed with
integers) as a possible value type, as these are user facing and we can't have
a property validation fail on one agent but succeed on an other just because
they are running on different platforms
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r443587382
##
File path: libminifi/include/core/PropertyValue.h
##
@@ -75,64 +77,52 @@ class PropertyValue : public state::response::ValueNode {
}
std::shared_ptr getValidator() const {
-return validator_;
+return *validator_;
}
ValidationResult validate(const std::string &subject) const {
-if (validator_) {
- return validator_->validate(subject, getValue());
-} else {
+auto cachedResult = validator_.isValid();
+if (cachedResult == CachedValueValidator::Result::SUCCESS) {
return ValidationResult::Builder::createBuilder().isValid(true).build();
}
+if (cachedResult == CachedValueValidator::Result::FAILURE) {
+ return
ValidationResult::Builder::createBuilder().withSubject(subject).withInput(getValue()->getStringValue()).isValid(false).build();
+}
+auto result = validator_->validate(subject, getValue());
+validator_.setValidationResult(result.valid());
+return result;
Review comment:
done
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r443546920
##
File path: libminifi/include/core/PropertyValue.h
##
@@ -202,14 +196,50 @@ class PropertyValue : public state::response::ValueNode {
auto operator=(const std::string &ref) -> typename std::enable_if<
std::is_same::value ||
std::is_same::value, PropertyValue&>::type {
-value_ = std::make_shared(ref);
-type_id = value_->getTypeIndex();
-return *this;
+validator_.clearValidationResult();
+return WithAssignmentGuard(ref, [&] () -> PropertyValue& {
+ value_ = std::make_shared(ref);
+ type_id = value_->getTypeIndex();
+ return *this;
+});
+ }
+
+ private:
+ template
+ T convertImpl(const char* const type_name) const {
+if (!isValueUsable()) {
+ throw utils::InvalidValueException("Cannot convert invalid value");
+}
+T res;
+if (value_->convertValue(res)) {
+ return res;
+}
+throw utils::ConversionException(std::string("Invalid conversion to ") +
type_name + " for " + value_->getStringValue());
+ }
Review comment:
we could always slap a nice macro on it
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r443531079
##
File path: libminifi/include/core/PropertyValue.h
##
@@ -202,14 +196,50 @@ class PropertyValue : public state::response::ValueNode {
auto operator=(const std::string &ref) -> typename std::enable_if<
std::is_same::value ||
std::is_same::value, PropertyValue&>::type {
-value_ = std::make_shared(ref);
-type_id = value_->getTypeIndex();
-return *this;
+validator_.clearValidationResult();
+return WithAssignmentGuard(ref, [&] () -> PropertyValue& {
+ value_ = std::make_shared(ref);
+ type_id = value_->getTypeIndex();
+ return *this;
+});
+ }
+
+ private:
+ template
+ T convertImpl(const char* const type_name) const {
+if (!isValueUsable()) {
+ throw utils::InvalidValueException("Cannot convert invalid value");
+}
+T res;
+if (value_->convertValue(res)) {
+ return res;
+}
+throw utils::ConversionException(std::string("Invalid conversion to ") +
type_name + " for " + value_->getStringValue());
+ }
Review comment:
if we can live with `uint64_t` being printed as `unsigned long` on some
platforms and `unsigned __int64` on others (windows) I can make the change, but
then the error message would be invalid, as we do not want to convert to
`unsigned long` we want to convert to `uint64_t` that the two coincide is a
platform dependent implementation detail
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r442078583
##
File path: libminifi/include/core/CachedValueValidator.h
##
@@ -0,0 +1,103 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+#define NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+
+#include "PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+
+class CachedValueValidator{
+ public:
+ enum class Result {
+FAILURE,
+SUCCESS,
+RECOMPUTE
+ };
+
+ CachedValueValidator() = default;
+ CachedValueValidator(const CachedValueValidator& other) :
validator_(other.validator_) {}
+ CachedValueValidator(CachedValueValidator&& other) noexcept :
validator_(std::move(other.validator_)) {}
+ CachedValueValidator& operator=(const CachedValueValidator& other) {
+validator_ = other.validator_;
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+ CachedValueValidator& operator=(CachedValueValidator&& other) {
+validator_ = std::move(other.validator_);
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+
+ CachedValueValidator(const std::shared_ptr& other) :
validator_(other) {}
+ CachedValueValidator(std::shared_ptr&& other) :
validator_(std::move(other)) {}
+ CachedValueValidator& operator=(const std::shared_ptr&
new_validator) {
+validator_ = new_validator;
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+ CachedValueValidator& operator=(std::shared_ptr&&
new_validator) {
+validator_ = std::move(new_validator);
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+
+ const std::shared_ptr& operator->() const {
+return validator_;
+ }
+
+ operator bool() const {
Review comment:
done, and the self-assignment check is also done
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797: URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r442078239 ## File path: libminifi/include/utils/ValueUtils.h ## @@ -0,0 +1,193 @@ +/** Review comment: done 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r442076380
##
File path: libminifi/include/utils/PropertyErrors.h
##
@@ -0,0 +1,115 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_PROPERTYERRORS_H_
+#define LIBMINIFI_INCLUDE_UTILS_PROPERTYERRORS_H_
+
+#include
+
+#include "Exception.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+
+namespace core {
+
+class PropertyValue;
+class ConfigurableComponent;
+class Property;
+
+} /* namespace core */
+
+namespace utils {
+
+class ValueException: public Exception{
+ private:
+ explicit ValueException(const std::string& err):
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+ explicit ValueException(const char* err):
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+
+ // base class already has a virtual destructor
+
+ friend class ParseException;
+ friend class ConversionException;
+ friend class InvalidValueException;
+};
Review comment:
moved most stuff into the namespace `internal`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r442076547
##
File path: libminifi/include/utils/ValueParser.h
##
@@ -0,0 +1,193 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_VALUEUTILS_H_
+#define LIBMINIFI_INCLUDE_UTILS_VALUEUTILS_H_
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "PropertyErrors.h"
+#include "GeneralUtils.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+class ValueParser {
+ private:
+ template
+ struct is_non_narrowing_convertible: std::false_type {
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ template
+ struct is_non_narrowing_convertible()})>>: std::true_type {
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ public:
+ explicit ValueParser(const std::string& str, std::size_t offset = 0):
str(str), offset(offset) {}
+
+ template
+ ValueParser& parseInt(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from int");
+long result; // NOLINT
+auto len = safeCallConverter(std::strtol, result);
+if ( len == 0 ) {
+ throw ParseException("Couldn't parse int");
+}
+if (result < (std::numeric_limits::min)() || result >
(std::numeric_limits::max)()) {
+ throw ParseException("Cannot convert long to int");
+}
+offset += len;
+out = {static_cast(result)};
+return *this;
+ }
+
+ template
+ ValueParser& parseLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from long"); // NOLINT
+long result; // NOLINT
+auto len = safeCallConverter(std::strtol, result);
+if ( len == 0 ) {
+ throw ParseException("Couldn't parse long");
+}
+offset += len;
+out = {result};
+return *this;
+ }
+
+ template
+ ValueParser& parseLongLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value,
"Expected lossless conversion from long long"); // NOLINT
+long long result; // NOLINT
+auto len = safeCallConverter(std::strtoll, result);
+if ( len == 0 ) {
+ throw ParseException("Couldn't parse long long");
+}
+offset += len;
+out = {result};
+return *this;
+ }
+
+ template
+ ValueParser& parseUInt32(Out& out) {
+static_assert(is_non_narrowing_convertible::value,
"Expected lossless conversion from uint32_t");
+parseSpace();
+if (offset < str.length() && str[offset] == '-') {
+ throw ParseException("Not an unsigned long");
+}
+unsigned long result; // NOLINT
+auto len = safeCallConverter(std::strtoul, result);
+if ( len == 0 ) {
+ throw ParseException("Couldn't parse uint32_t");
+}
+if (result > (std::numeric_limits::max)()) {
+ throw ParseException("Cannot convert unsigned long to uint32_t");
+}
+offset += len;
+out = {static_cast(result)};
+return *this;
+ }
+
+ template
+ ValueParser& parseUnsignedLongLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected lossless conversion from unsigned long long"); // NOLINT
+parseSpace();
+if (offset < str.length() && str[offset] == '-') {
+ throw ParseException("Not an unsigned long");
+}
+unsigned long long result; // NOLINT
+auto len = safeCallConverter(std::strtoull, result);
+if ( len == 0 ) {
+ throw ParseException("Couldn't parse unsigned long long");
+}
+offset += len;
+out = {result};
+return *this;
+ }
+
+ template
+ ValueParser& parseBool(Out& out) {
+parseSpace();
+if (std::strncmp(str.c_str() + offset, "false", std::strlen("false")) ==
0) {
+ offset += std::strlen("false");
+ out = false;
+} else if (std::strncmp(str.c_str() + offset, "true", std::strlen("true"))
== 0) {
+ offset += std::strlen("true");
+ out = true;
+} else {
+ throw ParseException("Couldn't parse bool");
+}
+return *this;
+ }
+
+ void parse
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r442075945
##
File path: libminifi/include/core/CachedValueValidator.h
##
@@ -0,0 +1,103 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+#define NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+
+#include "PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+
+class CachedValueValidator{
+ public:
+ enum class Result {
+FAILURE,
+SUCCESS,
+RECOMPUTE
+ };
+
+ CachedValueValidator() = default;
+ CachedValueValidator(const CachedValueValidator& other) :
validator_(other.validator_) {}
+ CachedValueValidator(CachedValueValidator&& other) noexcept :
validator_(std::move(other.validator_)) {}
+ CachedValueValidator& operator=(const CachedValueValidator& other) {
+validator_ = other.validator_;
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+ CachedValueValidator& operator=(CachedValueValidator&& other) {
+validator_ = std::move(other.validator_);
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+
+ CachedValueValidator(const std::shared_ptr& other) :
validator_(other) {}
+ CachedValueValidator(std::shared_ptr&& other) :
validator_(std::move(other)) {}
+ CachedValueValidator& operator=(const std::shared_ptr&
new_validator) {
+validator_ = new_validator;
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+ CachedValueValidator& operator=(std::shared_ptr&&
new_validator) {
+validator_ = std::move(new_validator);
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+
+ const std::shared_ptr& operator->() const {
+return validator_;
+ }
+
+ operator bool() const {
+return (bool)validator_;
+ }
+
+ const std::shared_ptr& operator*() const {
+return validator_;
+ }
+
+ void setValidationResult(bool success) const {
+validation_result_ = success ? Result::SUCCESS : Result::FAILURE;
+ }
+
+ void clearValidationResult() const {
+validation_result_ = Result::RECOMPUTE;
+ }
Review comment:
clearValidationResult can be not const, but setValidationResult must be
const as `PropertyValue::validate` is const, so the cached result must be
mutable
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r442075945
##
File path: libminifi/include/core/CachedValueValidator.h
##
@@ -0,0 +1,103 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+#define NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+
+#include "PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+
+class CachedValueValidator{
+ public:
+ enum class Result {
+FAILURE,
+SUCCESS,
+RECOMPUTE
+ };
+
+ CachedValueValidator() = default;
+ CachedValueValidator(const CachedValueValidator& other) :
validator_(other.validator_) {}
+ CachedValueValidator(CachedValueValidator&& other) noexcept :
validator_(std::move(other.validator_)) {}
+ CachedValueValidator& operator=(const CachedValueValidator& other) {
+validator_ = other.validator_;
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+ CachedValueValidator& operator=(CachedValueValidator&& other) {
+validator_ = std::move(other.validator_);
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+
+ CachedValueValidator(const std::shared_ptr& other) :
validator_(other) {}
+ CachedValueValidator(std::shared_ptr&& other) :
validator_(std::move(other)) {}
+ CachedValueValidator& operator=(const std::shared_ptr&
new_validator) {
+validator_ = new_validator;
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+ CachedValueValidator& operator=(std::shared_ptr&&
new_validator) {
+validator_ = std::move(new_validator);
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+
+ const std::shared_ptr& operator->() const {
+return validator_;
+ }
+
+ operator bool() const {
+return (bool)validator_;
+ }
+
+ const std::shared_ptr& operator*() const {
+return validator_;
+ }
+
+ void setValidationResult(bool success) const {
+validation_result_ = success ? Result::SUCCESS : Result::FAILURE;
+ }
+
+ void clearValidationResult() const {
+validation_result_ = Result::RECOMPUTE;
+ }
Review comment:
clearValidationResult can be not const, but setValidationResult must be
const are `PropertyValue::validate` is const, so the cached result must be
mutable
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r442075344
##
File path: extensions/libarchive/MergeContent.cpp
##
@@ -39,14 +39,29 @@ namespace nifi {
namespace minifi {
namespace processors {
-core::Property MergeContent::MergeStrategy("Merge Strategy", "Defragment or
Bin-Packing Algorithm", MERGE_STRATEGY_DEFRAGMENT);
-core::Property MergeContent::MergeFormat("Merge Format", "Merge Format",
MERGE_FORMAT_CONCAT_VALUE);
+core::Property MergeContent::MergeStrategy(
+ core::PropertyBuilder::createProperty("Merge Strategy")
+ ->withDescription("Defragment or Bin-Packing Algorithm")
+ ->withAllowableValues({MERGE_STRATEGY_DEFRAGMENT,
MERGE_STRATEGY_BIN_PACK})
+ ->withDefaultValue(MERGE_STRATEGY_DEFRAGMENT)->build());
+core::Property MergeContent::MergeFormat(
+ core::PropertyBuilder::createProperty("Merge Format")
+ ->withDescription("Merge Format")
+ ->withAllowableValues({MERGE_FORMAT_CONCAT_VALUE,
MERGE_FORMAT_TAR_VALUE, MERGE_FORMAT_ZIP_VALUE})
+ ->withDefaultValue(MERGE_FORMAT_CONCAT_VALUE)->build());
core::Property MergeContent::CorrelationAttributeName("Correlation Attribute
Name", "Correlation Attribute Name", "");
-core::Property MergeContent::DelimiterStratgey("Delimiter Strategy",
"Determines if Header, Footer, and Demarcator should point to files",
DELIMITER_STRATEGY_FILENAME);
+core::Property MergeContent::DelimiterStratgey(
Review comment:
done
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r440848973
##
File path: libminifi/include/utils/PropertyErrors.h
##
@@ -0,0 +1,115 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_PROPERTYERRORS_H_
+#define LIBMINIFI_INCLUDE_UTILS_PROPERTYERRORS_H_
+
+#include
+
+#include "Exception.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+
+namespace core {
+
+class PropertyValue;
+class ConfigurableComponent;
+class Property;
+
+} /* namespace core */
+
+namespace utils {
+
+class ValueException: public Exception{
+ private:
+ explicit ValueException(const std::string& err):
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+ explicit ValueException(const char* err):
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+
+ // base class already has a virtual destructor
+
+ friend class ParseException;
+ friend class ConversionException;
+ friend class InvalidValueException;
+};
Review comment:
do we have some `implementation_detail` directory where we could stash
it, so it doesn't become part of the API?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r440840987
##
File path: libminifi/include/core/CachedValueValidator.h
##
@@ -0,0 +1,103 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+#define NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+
+#include "PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+
+class CachedValueValidator{
+ public:
+ enum class Result {
+FAILURE,
+SUCCESS,
+RECOMPUTE
+ };
+
+ CachedValueValidator() = default;
+ CachedValueValidator(const CachedValueValidator& other) :
validator_(other.validator_) {}
+ CachedValueValidator(CachedValueValidator&& other) noexcept :
validator_(std::move(other.validator_)) {}
Review comment:
the only owner of a `CachedValueValidator` instance should be a
`PropertyValue`, ideally the move-constructor (and move-assignment) should be
deleted, but since the `PropertyValue` has those defaulted that's not an
option, when you move a `CachedValueValidator` into a
`PropertyValue::validator_` you change the validator object in a value thus the
cached validation result cannot be trusted anymore, the most important part is
that such a `CachedValueValidator` should only be executed on its owner value
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r440836150
##
File path: libminifi/include/utils/PropertyErrors.h
##
@@ -0,0 +1,115 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_PROPERTYERRORS_H_
+#define LIBMINIFI_INCLUDE_UTILS_PROPERTYERRORS_H_
+
+#include
+
+#include "Exception.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+
+namespace core {
+
+class PropertyValue;
+class ConfigurableComponent;
+class Property;
+
+} /* namespace core */
+
+namespace utils {
+
+class ValueException: public Exception{
+ private:
+ explicit ValueException(const std::string& err):
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+ explicit ValueException(const char* err):
Exception(ExceptionType::GENERAL_EXCEPTION, err) {}
+
+ // base class already has a virtual destructor
+
+ friend class ParseException;
+ friend class ConversionException;
+ friend class InvalidValueException;
+};
Review comment:
I wanted the API as tight as possible, introducing new classes that are
up for grabs is rarely beneficial, especially with the new major release coming
up we should be extra careful what unremovable stuff we introduce, this clearly
indicates that only we should be extending these classes
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r440831812
##
File path: libminifi/include/core/state/Value.h
##
@@ -86,35 +87,57 @@ class Value {
}
virtual bool getValue(uint32_t &ref) {
-const auto negative = string_value.find_first_of('-') != std::string::npos;
- if (negative) {
- return false;
- }
-ref = std::stoul(string_value);
+try {
+ uint32_t value;
+ utils::ValueParser(string_value).parseUInt32(value).parseEnd();
+ ref = value;
+} catch(const utils::ParseException&) {
+ return false;
+}
Review comment:
they are not ment to be more general, they are ment to be correct, all
these conversion methods are incorrect as they all happily convert `"1banana"`
to `1`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r440827688
##
File path: extensions/coap/controllerservice/CoapConnector.cpp
##
@@ -39,7 +39,7 @@ static core::Property RemoteServer;
static core::Property Port;
static core::Property MaxQueueSize;
-core::Property
CoapConnectorService::RemoteServer(core::PropertyBuilder::createProperty("Remote
Server")->withDescription("Remote CoAP server")->isRequired(true)->build());
+core::Property
CoapConnectorService::RemoteServer(core::PropertyBuilder::createProperty("Remote
Server")->withDescription("Remote CoAP server")->isRequired(false)->build());
Review comment:
one of the tests did not provide the property and failed, from the code
it seems like this service can handle it not being set
```
void CoapConnectorService::onEnable() {
std::string port_str;
if (getProperty(RemoteServer.getName(), host_) && !host_.empty() &&
getProperty(Port.getName(), port_str) && !port_str.empty()) {
core::Property::StringToInt(port_str, port_);
} else {
// this is the case where we aren't being used in the context of a
single controller service.
if (configuration_->get("nifi.c2.agent.coap.host", host_) &&
configuration_->get("nifi.c2.agent.coap.port", port_str)) {
core::Property::StringToInt(port_str, port_);
}
}
}
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r440823993
##
File path: libminifi/include/core/CachedValueValidator.h
##
@@ -0,0 +1,103 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+#define NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+
+#include "PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+
+class CachedValueValidator{
+ public:
+ enum class Result {
+FAILURE,
+SUCCESS,
+RECOMPUTE
+ };
+
+ CachedValueValidator() = default;
+ CachedValueValidator(const CachedValueValidator& other) :
validator_(other.validator_) {}
+ CachedValueValidator(CachedValueValidator&& other) noexcept :
validator_(std::move(other.validator_)) {}
Review comment:
I generally like to take only what I use and making it as explicit as
possible, instead of adjusting the effects after the fact, but if you think
this would improve the readability I could make the change
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r435033689
##
File path: libminifi/include/core/CachedValueValidator.h
##
@@ -0,0 +1,103 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+#define NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+
+#include "PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+
+class CachedValueValidator{
+ public:
+ enum class Result {
+FAILURE,
+SUCCESS,
+RECOMPUTE
+ };
+
+ CachedValueValidator() = default;
+ CachedValueValidator(const CachedValueValidator& other) :
validator_(other.validator_) {}
+ CachedValueValidator(CachedValueValidator&& other) :
validator_(std::move(other.validator_)) {}
+ CachedValueValidator& operator=(const CachedValueValidator& other) {
Review comment:
I think for these simple cases it would be an overkill, also when we
change `std::shared_ptr` to something else I do not yet see
if we want to allow it to be default constructible, also after a `swap` I would
expect the two objects to exchange all states, this is not the case for the
`validation_result_`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r433780575
##
File path: libminifi/include/utils/ValueUtils.h
##
@@ -0,0 +1,203 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef NIFI_MINIFI_CPP_VALUEUTILS_H
+#define NIFI_MINIFI_CPP_VALUEUTILS_H
+
+#include
+#include
+#include
+#include
+#include
+#include "PropertyErrors.h"
+#include
+#include
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+class ValueParser {
+ private:
+ template< class... >
+ using void_t = void;
+
+ template
+ struct is_non_narrowing_convertible: std::false_type {
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ template
+ struct is_non_narrowing_convertible()})>>: std::true_type{
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ public:
+ ValueParser(const std::string& str, std::size_t offset = 0): str(str),
offset(offset) {}
+
+ template
+ ValueParser& parseInt(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from int");
+try {
+ char *end;
+ long result{std::strtol(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ if (result < (std::numeric_limits::min)() || result >
(std::numeric_limits::max)()) {
Review comment:
this wouldn't work for two reasons:
- as you said the return expression prevents narrowing conversions
- casting to signed integrals, if the source value cannot be represented in
the target type, is implementation defined (might even raise a signal)
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r433773488
##
File path: libminifi/include/core/PropertyValue.h
##
@@ -71,64 +72,75 @@ class PropertyValue : public state::response::ValueNode {
}
std::shared_ptr getValidator() const {
-return validator_;
+return *validator_;
}
ValidationResult validate(const std::string &subject) const {
-if (validator_) {
- return validator_->validate(subject, getValue());
-} else {
+auto cachedResult = validator_.isValid();
+if(cachedResult == CachedValueValidator::Result::SUCCESS){
return ValidationResult::Builder::createBuilder().isValid(true).build();
}
+if(cachedResult == CachedValueValidator::Result::FAILURE){
+ return
ValidationResult::Builder::createBuilder().withSubject(subject).withInput(getValue()->getStringValue()).isValid(false).build();
+}
+auto result = validator_->validate(subject, getValue());
+validator_.setValidationResult(result.valid());
+return result;
}
operator uint64_t() const {
+if(!isValueUsable())throw utils::InvalidValueException("Cannot convert
invalid value");
Review comment:
done
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r433744548
##
File path: libminifi/include/core/Property.h
##
@@ -129,41 +133,32 @@ class Property {
template
void setValue(const T &value) {
-PropertyValue vn = default_value_;
-vn = value;
-if (validator_) {
- vn.setValidator(validator_);
- ValidationResult result = validator_->validate(name_, vn.getValue());
- if (!result.valid()) {
-// throw some exception?
- }
-} else {
- vn.setValidator(core::StandardValidators::VALID);
-}
if (!is_collection_) {
values_.clear();
- values_.push_back(vn);
+ values_.push_back(default_value_);
} else {
- values_.push_back(vn);
+ values_.push_back(default_value_);
}
+PropertyValue& vn = values_.back();
+vn.setValidator(validator_ ? validator_ : core::StandardValidators::VALID);
+vn = value;
+ValidationResult result = vn.validate(name_);
+if(!result.valid())
+ throw utils::InvalidValueException(name_ + " value validation failed");
}
- void setValue(PropertyValue &vn) {
-if (validator_) {
- vn.setValidator(validator_);
- ValidationResult result = validator_->validate(name_, vn.getValue());
- if (!result.valid()) {
-// throw some exception?
- }
-} else {
- vn.setValidator(core::StandardValidators::VALID);
-}
+ void setValue(PropertyValue &newValue) {
if (!is_collection_) {
values_.clear();
- values_.push_back(vn);
+ values_.push_back(newValue);
} else {
- values_.push_back(vn);
+ values_.push_back(newValue);
}
+PropertyValue& vn = values_.back();
+vn.setValidator(validator_ ? validator_ : core::StandardValidators::VALID);
Review comment:
yes, replacing `std::shared_ptr` with a suitable
wrapper that guarantees a `PropertyValidator` instance is viable, but since
most API functions expect/return a `shared_ptr` I would wait for after `0.8.0`
to make the change
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r433736539
##
File path: libminifi/include/core/CachedValueValidator.h
##
@@ -0,0 +1,103 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+#define NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+
+#include "PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+
+class CachedValueValidator{
+ public:
+ enum class Result {
+FAILURE,
+SUCCESS,
+RECOMPUTE
+ };
+
+ CachedValueValidator() = default;
+ CachedValueValidator(const CachedValueValidator& other) :
validator_(other.validator_) {}
+ CachedValueValidator(CachedValueValidator&& other) :
validator_(std::move(other.validator_)) {}
+ CachedValueValidator& operator=(const CachedValueValidator& other) {
+validator_ = other.validator_;
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+ CachedValueValidator& operator=(CachedValueValidator&& other) {
+validator_ = std::move(other.validator_);
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+
+ CachedValueValidator(const std::shared_ptr& other) :
validator_(other) {}
+ CachedValueValidator(std::shared_ptr&& other) :
validator_(std::move(other)) {}
+ CachedValueValidator& operator=(const std::shared_ptr&
new_validator) {
+validator_ = new_validator;
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+ CachedValueValidator& operator=(std::shared_ptr&&
new_validator) {
+validator_ = std::move(new_validator);
+validation_result_ = Result::RECOMPUTE;
+return *this;
+ }
+
+ const std::shared_ptr& operator->() const {
+return validator_;
+ }
+
+ operator bool() const {
+return (bool)validator_;
+ }
+
+ const std::shared_ptr& operator*() const {
+return validator_;
+ }
+
+ void setValidationResult(bool success) const {
+validation_result_ = success ? Result::SUCCESS : Result::FAILURE;
+ }
+
+ void clearValidationResult() const {
+validation_result_ = Result::RECOMPUTE;
+ }
+
+ Result isValid() const {
+if(!validator_ || validation_result_ == Result::SUCCESS){
Review comment:
done
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r433730693
##
File path: libminifi/include/utils/ValueUtils.h
##
@@ -0,0 +1,203 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef NIFI_MINIFI_CPP_VALUEUTILS_H
+#define NIFI_MINIFI_CPP_VALUEUTILS_H
+
+#include
+#include
+#include
+#include
+#include
+#include "PropertyErrors.h"
+#include
+#include
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+class ValueParser {
+ private:
+ template< class... >
+ using void_t = void;
+
+ template
+ struct is_non_narrowing_convertible: std::false_type {
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ template
+ struct is_non_narrowing_convertible()})>>: std::true_type{
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ public:
+ ValueParser(const std::string& str, std::size_t offset = 0): str(str),
offset(offset) {}
+
+ template
+ ValueParser& parseInt(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from int");
+try {
+ char *end;
+ long result{std::strtol(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ if (result < (std::numeric_limits::min)() || result >
(std::numeric_limits::max)()) {
+throw ParseException("Cannot convert long to int");
+ }
+ out = {static_cast(result)};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse int");
+}
+ }
+
+ template
+ ValueParser& parseLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from long");
+try {
+ char *end;
+ long result{std::strtol(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ out = {result};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse long");
+}
+ }
+
+ template
+ ValueParser& parseLongLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value,
"Expected lossless conversion from long long");
+try {
+ char *end;
+ long long result{std::strtoll(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ out = {result};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse long long");
+}
+ }
+
+ template
+ ValueParser& parseUInt32(Out& out) {
+static_assert(is_non_narrowing_convertible::value,
"Expected lossless conversion from uint32_t");
+try {
+ parseSpace();
+ if (offset < str.length() && str[offset] == '-') {
+throw ParseException("Not an unsigned long");
+ }
+ char *end;
+ unsigned long result{std::strtoul(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ if (result > (std::numeric_limits::max)()) {
+throw ParseException("Cannot convert unsigned long to uint32_t");
+ }
+ out = {static_cast(result)};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse unsigned long");
+}
+ }
+
+ template
+ ValueParser& parseUnsignedLongLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected lossless conversion from unsigned long long");
+try {
+ parseSpace();
+ if (offset < str.length() && str[offset] == '-') {
+throw ParseException("Not an unsigned long");
+ }
+ char *end;
+ unsigned long long result{std::strtoull(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ out = {result};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse unsigned long long");
+}
+ }
+
+ template
+ ValueParser& parseBool(Out& out){
+const char* options[] = {"false", "true"};
+const bool values[] = {false, true};
+auto index = parseAny(options);
+if(index == -1)throw ParseException("Couldn't parse bool");
+out = values[index];
+return *this;
+ }
+
+ int parseAny(const std::vector &options) {
+parseSpace()
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r433729206
##
File path: libminifi/test/unit/PropertyValidationTests.cpp
##
@@ -0,0 +1,244 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "../TestBase.h"
+#include "core/ConfigurableComponent.h"
+#include "utils/PropertyErrors.h"
+#include "core/PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace 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::ConversionException);
+
+ const std::string SPACE = " ";
+ auto prop2 = Property("prop", "d", SPACE + "true");
+ prop2.setValue("banana");
+}
+
+TEST_CASE("Converting invalid PropertyValue") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->withDefaultValue(0)
+->build();
+ REQUIRE_THROWS_AS(prop.setValue("not int"), utils::ParseException);
+ REQUIRE_THROWS_AS(static_cast(prop.getValue()),
utils::InvalidValueException);
+}
+
+TEST_CASE("Parsing int") {
Review comment:
done
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r433701464
##
File path: libminifi/test/unit/PropertyValidationTests.cpp
##
@@ -0,0 +1,244 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "../TestBase.h"
+#include "core/ConfigurableComponent.h"
+#include "utils/PropertyErrors.h"
+#include "core/PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace 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::ConversionException);
+
+ const std::string SPACE = " ";
+ auto prop2 = Property("prop", "d", SPACE + "true");
+ prop2.setValue("banana");
+}
+
+TEST_CASE("Converting invalid PropertyValue") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->withDefaultValue(0)
+->build();
+ REQUIRE_THROWS_AS(prop.setValue("not int"), utils::ParseException);
+ REQUIRE_THROWS_AS(static_cast(prop.getValue()),
utils::InvalidValueException);
+}
+
+TEST_CASE("Parsing int") {
Review comment:
indeed it is, removed duplicate
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r433724046
##
File path: libminifi/test/unit/PropertyValidationTests.cpp
##
@@ -0,0 +1,244 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "../TestBase.h"
+#include "core/ConfigurableComponent.h"
+#include "utils/PropertyErrors.h"
+#include "core/PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace 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::ConversionException);
+
+ const std::string SPACE = " ";
+ auto prop2 = Property("prop", "d", SPACE + "true");
+ prop2.setValue("banana");
+}
+
+TEST_CASE("Converting invalid PropertyValue") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->withDefaultValue(0)
+->build();
+ REQUIRE_THROWS_AS(prop.setValue("not int"), utils::ParseException);
+ REQUIRE_THROWS_AS(static_cast(prop.getValue()),
utils::InvalidValueException);
+}
+
+TEST_CASE("Parsing int") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->withDefaultValue(0)
+->build();
+ REQUIRE_THROWS_AS(prop.setValue("not int"), utils::ParseException);
+}
+
+TEST_CASE("Parsing int has baggage after") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->withDefaultValue(0)
+->build();
+ REQUIRE_THROWS_AS(prop.setValue("55almost int"), utils::ParseException);
+}
+
+TEST_CASE("Parsing int has spaces") {
+ auto prop = PropertyBuilder::createProperty("prop")
+ ->withDefaultValue(0)
+ ->build();
+ prop.setValue(" 55 ");
+ REQUIRE(static_cast(prop.getValue()) == 55);
+}
+
+TEST_CASE("Parsing int out of range") {
+ auto prop = PropertyBuilder::createProperty("prop")
+ ->withDefaultValue(0)
+ ->build();
+ REQUIRE_THROWS_AS(prop.setValue(" 50 "), utils::ParseException);
+}
+
+TEST_CASE("Parsing bool has baggage after") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->withDefaultValue(true)
+->build();
+ REQUIRE_THROWS_AS(prop.setValue("false almost bool"), utils::ParseException);
+}
+
+class TestConfigurableComponent : public ConfigurableComponent {
+ public:
+ bool supportsDynamicProperties() override {
+return true;
+ }
+
+ bool canEdit() override {
+return true;
+ }
+
+ void onPropertyModified(const Property &old_property, const Property
&new_property) override {
+if (onPropertyModifiedCallback) onPropertyModifiedCallback(old_property,
new_property);
+ }
+
+ void onDynamicPropertyModified(const Property &old_property, const Property
&new_property) override {
+if (onDynamicPropertyModifiedCallback)
onDynamicPropertyModifiedCallback(old_property, new_property);
+ }
+
+ template
+ void setPropertyModifiedCallback(Fn&& functor) {
+onPropertyModifiedCallback = std::forward(functor);
+ }
+
+ template
+ void setDynamicPropertyModifiedCallback(Fn&& functor) {
+onDynamicPropertyModifiedCallback = std::forward(functor);
+ }
+
+ private:
+ std::function
onPropertyModifiedCallback;
+ std::function
onDynamicPropertyModifiedCallback;
+};
+
+TEST_CASE("Missing Required With Default") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->isRequired(true)
+->withDefaultValue("default")
+->build();
+ TestConfigurableComponent component;
+ component.setSupportedProperties({prop});
+ std::string value;
+ REQUIRE(component.getProperty(prop.getName(), value));
+ REQUIRE(value == "default");
+}
+
+TEST_CASE("Missing Required Without Default") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->isRequired(true)
+->build();
+ TestConfigurableComponent component;
+ component.setSupportedProperties({prop});
+ std::string value;
+ REQUIRE_THROWS_AS(component.getProperty(prop.getName(), value),
utils::RequiredPropertyMissingException);
+}
+
+TEST_CASE("Missing Optional Without Default") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->isRequired(fal
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r433701464
##
File path: libminifi/test/unit/PropertyValidationTests.cpp
##
@@ -0,0 +1,244 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "../TestBase.h"
+#include "core/ConfigurableComponent.h"
+#include "utils/PropertyErrors.h"
+#include "core/PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace 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::ConversionException);
+
+ const std::string SPACE = " ";
+ auto prop2 = Property("prop", "d", SPACE + "true");
+ prop2.setValue("banana");
+}
+
+TEST_CASE("Converting invalid PropertyValue") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->withDefaultValue(0)
+->build();
+ REQUIRE_THROWS_AS(prop.setValue("not int"), utils::ParseException);
+ REQUIRE_THROWS_AS(static_cast(prop.getValue()),
utils::InvalidValueException);
+}
+
+TEST_CASE("Parsing int") {
Review comment:
indeed it is, removing duplicate
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r433701464
##
File path: libminifi/test/unit/PropertyValidationTests.cpp
##
@@ -0,0 +1,244 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "../TestBase.h"
+#include "core/ConfigurableComponent.h"
+#include "utils/PropertyErrors.h"
+#include "core/PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace 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::ConversionException);
+
+ const std::string SPACE = " ";
+ auto prop2 = Property("prop", "d", SPACE + "true");
+ prop2.setValue("banana");
+}
+
+TEST_CASE("Converting invalid PropertyValue") {
+ auto prop = PropertyBuilder::createProperty("prop")
+->withDefaultValue(0)
+->build();
+ REQUIRE_THROWS_AS(prop.setValue("not int"), utils::ParseException);
+ REQUIRE_THROWS_AS(static_cast(prop.getValue()),
utils::InvalidValueException);
+}
+
+TEST_CASE("Parsing int") {
Review comment:
indeed it is removing duplicate
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r433678132
##
File path: libminifi/include/core/ConfigurableComponent.h
##
@@ -216,16 +216,20 @@ bool ConfigurableComponent::getProperty(const std::string
name, T &value) const{
auto &&it = properties_.find(name);
Review comment:
no idea, removed it, but it is an universal reference and will bind
everything, useful when the expression returns a proxy reference, e.g. when
accessing elements of `std::vector`, I mostly use them in range-based
loops, but generally makes people think that you are doing something funny, so
I would use them with great care
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r432935664
##
File path: libminifi/include/utils/ValueUtils.h
##
@@ -0,0 +1,203 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef NIFI_MINIFI_CPP_VALUEUTILS_H
+#define NIFI_MINIFI_CPP_VALUEUTILS_H
+
+#include
+#include
+#include
+#include
+#include
+#include "PropertyErrors.h"
+#include
+#include
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+class ValueParser {
+ private:
+ template< class... >
+ using void_t = void;
+
+ template
+ struct is_non_narrowing_convertible: std::false_type {
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ template
+ struct is_non_narrowing_convertible()})>>: std::true_type{
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ public:
+ ValueParser(const std::string& str, std::size_t offset = 0): str(str),
offset(offset) {}
+
+ template
+ ValueParser& parseInt(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from int");
+try {
+ char *end;
+ long result{std::strtol(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ if (result < (std::numeric_limits::min)() || result >
(std::numeric_limits::max)()) {
+throw ParseException("Cannot convert long to int");
+ }
+ out = {static_cast(result)};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse int");
+}
+ }
+
+ template
+ ValueParser& parseLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from long");
+try {
+ char *end;
+ long result{std::strtol(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ out = {result};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse long");
+}
+ }
+
+ template
+ ValueParser& parseLongLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value,
"Expected lossless conversion from long long");
+try {
+ char *end;
+ long long result{std::strtoll(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ out = {result};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse long long");
+}
+ }
+
+ template
+ ValueParser& parseUInt32(Out& out) {
+static_assert(is_non_narrowing_convertible::value,
"Expected lossless conversion from uint32_t");
+try {
+ parseSpace();
+ if (offset < str.length() && str[offset] == '-') {
+throw ParseException("Not an unsigned long");
+ }
+ char *end;
+ unsigned long result{std::strtoul(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ if (result > (std::numeric_limits::max)()) {
+throw ParseException("Cannot convert unsigned long to uint32_t");
+ }
+ out = {static_cast(result)};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse unsigned long");
+}
+ }
+
+ template
+ ValueParser& parseUnsignedLongLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected lossless conversion from unsigned long long");
+try {
+ parseSpace();
+ if (offset < str.length() && str[offset] == '-') {
+throw ParseException("Not an unsigned long");
+ }
+ char *end;
+ unsigned long long result{std::strtoull(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ out = {result};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse unsigned long long");
+}
+ }
+
+ template
+ ValueParser& parseBool(Out& out){
+const char* options[] = {"false", "true"};
+const bool values[] = {false, true};
+auto index = parseAny(options);
+if(index == -1)throw ParseException("Couldn't parse bool");
+out = values[index];
+return *this;
+ }
+
+ int parseAny(const std::vector &options) {
+parseSpace()
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r432935664
##
File path: libminifi/include/utils/ValueUtils.h
##
@@ -0,0 +1,203 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef NIFI_MINIFI_CPP_VALUEUTILS_H
+#define NIFI_MINIFI_CPP_VALUEUTILS_H
+
+#include
+#include
+#include
+#include
+#include
+#include "PropertyErrors.h"
+#include
+#include
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+class ValueParser {
+ private:
+ template< class... >
+ using void_t = void;
+
+ template
+ struct is_non_narrowing_convertible: std::false_type {
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ template
+ struct is_non_narrowing_convertible()})>>: std::true_type{
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ public:
+ ValueParser(const std::string& str, std::size_t offset = 0): str(str),
offset(offset) {}
+
+ template
+ ValueParser& parseInt(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from int");
+try {
+ char *end;
+ long result{std::strtol(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ if (result < (std::numeric_limits::min)() || result >
(std::numeric_limits::max)()) {
+throw ParseException("Cannot convert long to int");
+ }
+ out = {static_cast(result)};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse int");
+}
+ }
+
+ template
+ ValueParser& parseLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from long");
+try {
+ char *end;
+ long result{std::strtol(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ out = {result};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse long");
+}
+ }
+
+ template
+ ValueParser& parseLongLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value,
"Expected lossless conversion from long long");
+try {
+ char *end;
+ long long result{std::strtoll(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ out = {result};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse long long");
+}
+ }
+
+ template
+ ValueParser& parseUInt32(Out& out) {
+static_assert(is_non_narrowing_convertible::value,
"Expected lossless conversion from uint32_t");
+try {
+ parseSpace();
+ if (offset < str.length() && str[offset] == '-') {
+throw ParseException("Not an unsigned long");
+ }
+ char *end;
+ unsigned long result{std::strtoul(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ if (result > (std::numeric_limits::max)()) {
+throw ParseException("Cannot convert unsigned long to uint32_t");
+ }
+ out = {static_cast(result)};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse unsigned long");
+}
+ }
+
+ template
+ ValueParser& parseUnsignedLongLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected lossless conversion from unsigned long long");
+try {
+ parseSpace();
+ if (offset < str.length() && str[offset] == '-') {
+throw ParseException("Not an unsigned long");
+ }
+ char *end;
+ unsigned long long result{std::strtoull(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ out = {result};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse unsigned long long");
+}
+ }
+
+ template
+ ValueParser& parseBool(Out& out){
+const char* options[] = {"false", "true"};
+const bool values[] = {false, true};
+auto index = parseAny(options);
+if(index == -1)throw ParseException("Couldn't parse bool");
+out = values[index];
+return *this;
+ }
+
+ int parseAny(const std::vector &options) {
+parseSpace()
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r432817914
##
File path: libminifi/include/core/ConfigurableComponent.h
##
@@ -216,16 +216,20 @@ bool ConfigurableComponent::getProperty(const std::string
name, T &value) const{
auto &&it = properties_.find(name);
if (it != properties_.end()) {
- Property item = it->second;
- value = static_cast(item.getValue());
- if (item.getValue().getValue() != nullptr){
- logger_->log_debug("Component %s property name %s value %s", name,
item.getName(), item.getValue().to_string());
- return true;
- }
- else{
+ const Property& item = it->second;
+ if (item.getValue().getValue() == nullptr) {
+ // empty value
+ if (item.getRequired()) {
+ logger_->log_debug("Component %s required property %s is empty",
name, item.getName());
+ throw utils::RequiredPropertyMissingException("Required property is
empty: " + item.getName());
Review comment:
the thing is, that you as a processor implementer should not care if
`getProperty` throws or not, returning `false` means: "hey some error occurred
that you should handle", throwing an exception means that somebody upstream
messed up, e.g. called `onSchedule` without validating if all required
properties are present or not checking invalid properties, if I mark a property
required or add a validator I do not want to be have to handle if it is missing
or invalid
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r431218532
##
File path: libminifi/include/core/CachedValueValidator.h
##
@@ -0,0 +1,103 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+#define NIFI_MINIFI_CPP_CACHEDVALUEVALIDATOR_H
+
+#include "PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+
+class CachedValueValidator{
+ public:
+ enum class Result {
Review comment:
I would argue that a value of type `CachedValueValidator::Result`
carries more intention than that of a `std::optional`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r431216668
##
File path: libminifi/include/core/PropertyValue.h
##
@@ -138,26 +150,28 @@ class PropertyValue : public state::response::ValueNode {
*/
template
auto operator=(const T ref) -> typename std::enable_if::value,PropertyValue&>::type {
Review comment:
I believe `T` if not explicitly provided will always be deduced to a
non-reference type, good question what the author wanted to prevent (or
facilitate) with this solution
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r431210652
##
File path: libminifi/src/core/ConfigurableComponent.cpp
##
@@ -103,11 +105,12 @@ bool ConfigurableComponent::updateProperty(const
std::string &name, const std::s
if (it != properties_.end()) {
Property orig_property = it->second;
-Property new_property = orig_property;
+Property& new_property = it->second;
+utils::ScopeGuard onExit([&] {
Review comment:
if `addValue` throws this still notifies the component of the changed
property, as we do not have `finally` in c++ it is cleaner than calling it both
in a catch and outside
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r431200537
##
File path: libminifi/include/core/Property.h
##
@@ -129,41 +133,32 @@ class Property {
template
void setValue(const T &value) {
-PropertyValue vn = default_value_;
-vn = value;
-if (validator_) {
- vn.setValidator(validator_);
- ValidationResult result = validator_->validate(name_, vn.getValue());
- if (!result.valid()) {
-// throw some exception?
- }
-} else {
- vn.setValidator(core::StandardValidators::VALID);
-}
if (!is_collection_) {
values_.clear();
- values_.push_back(vn);
+ values_.push_back(default_value_);
} else {
- values_.push_back(vn);
+ values_.push_back(default_value_);
}
+PropertyValue& vn = values_.back();
+vn.setValidator(validator_ ? validator_ : core::StandardValidators::VALID);
+vn = value;
+ValidationResult result = vn.validate(name_);
+if(!result.valid())
+ throw utils::InvalidValueException(name_ + " value validation failed");
}
- void setValue(PropertyValue &vn) {
-if (validator_) {
- vn.setValidator(validator_);
- ValidationResult result = validator_->validate(name_, vn.getValue());
- if (!result.valid()) {
-// throw some exception?
- }
-} else {
- vn.setValidator(core::StandardValidators::VALID);
-}
+ void setValue(PropertyValue &newValue) {
if (!is_collection_) {
values_.clear();
- values_.push_back(vn);
+ values_.push_back(newValue);
} else {
- values_.push_back(vn);
+ values_.push_back(newValue);
}
+PropertyValue& vn = values_.back();
+vn.setValidator(validator_ ? validator_ : core::StandardValidators::VALID);
Review comment:
it sets the validator in the PropertyValue, if we didn't the incoming
`newValue`'s validator would remain in the copy, yes we could also just clear
the validator as most places interpret the absence of a validator as being
valid, but I find it unappealing, imo we should always have a non-empty
validator instance
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r431194612
##
File path: libminifi/include/core/ConfigurableComponent.h
##
@@ -216,16 +216,20 @@ bool ConfigurableComponent::getProperty(const std::string
name, T &value) const{
auto &&it = properties_.find(name);
if (it != properties_.end()) {
- Property item = it->second;
- value = static_cast(item.getValue());
- if (item.getValue().getValue() != nullptr){
- logger_->log_debug("Component %s property name %s value %s", name,
item.getName(), item.getValue().to_string());
- return true;
- }
- else{
+ const Property& item = it->second;
+ if (item.getValue().getValue() == nullptr) {
+ // empty value
+ if (item.getRequired()) {
+ logger_->log_debug("Component %s required property %s is empty",
name, item.getName());
+ throw utils::RequiredPropertyMissingException("Required property is
empty: " + item.getName());
Review comment:
definitely, if I create a processor and mark a property required and it
is not there, it is the fault of the processor scheduler and I should not
handle its absence (should not catch the exception but let it up to the
scheduler), on the other hand if I have an optional property and it is not
there, tough luck but I'll have to manage its absence
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r431188642
##
File path: libminifi/include/utils/ValueUtils.h
##
@@ -0,0 +1,203 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenseas/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef NIFI_MINIFI_CPP_VALUEUTILS_H
+#define NIFI_MINIFI_CPP_VALUEUTILS_H
+
+#include
+#include
+#include
+#include
+#include
+#include "PropertyErrors.h"
+#include
+#include
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+class ValueParser {
+ private:
+ template< class... >
+ using void_t = void;
+
+ template
+ struct is_non_narrowing_convertible: std::false_type {
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ template
+ struct is_non_narrowing_convertible()})>>: std::true_type{
+static_assert(std::is_integral::value &&
std::is_integral::value, "Checks only integral values");
+ };
+
+ public:
+ ValueParser(const std::string& str, std::size_t offset = 0): str(str),
offset(offset) {}
+
+ template
+ ValueParser& parseInt(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from int");
+try {
+ char *end;
+ long result{std::strtol(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ if (result < (std::numeric_limits::min)() || result >
(std::numeric_limits::max)()) {
+throw ParseException("Cannot convert long to int");
+ }
+ out = {static_cast(result)};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse int");
+}
+ }
+
+ template
+ ValueParser& parseLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected
lossless conversion from long");
+try {
+ char *end;
+ long result{std::strtol(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ out = {result};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse long");
+}
+ }
+
+ template
+ ValueParser& parseLongLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value,
"Expected lossless conversion from long long");
+try {
+ char *end;
+ long long result{std::strtoll(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ out = {result};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse long long");
+}
+ }
+
+ template
+ ValueParser& parseUInt32(Out& out) {
+static_assert(is_non_narrowing_convertible::value,
"Expected lossless conversion from uint32_t");
+try {
+ parseSpace();
+ if (offset < str.length() && str[offset] == '-') {
+throw ParseException("Not an unsigned long");
+ }
+ char *end;
+ unsigned long result{std::strtoul(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ if (result > (std::numeric_limits::max)()) {
+throw ParseException("Cannot convert unsigned long to uint32_t");
+ }
+ out = {static_cast(result)};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse unsigned long");
+}
+ }
+
+ template
+ ValueParser& parseUnsignedLongLong(Out& out) {
+static_assert(is_non_narrowing_convertible::value, "Expected lossless conversion from unsigned long long");
+try {
+ parseSpace();
+ if (offset < str.length() && str[offset] == '-') {
+throw ParseException("Not an unsigned long");
+ }
+ char *end;
+ unsigned long long result{std::strtoull(str.c_str() + offset, &end, 10)};
+ offset = end - str.c_str();
+ out = {result};
+ return *this;
+}catch(...){
+ throw ParseException("Could not parse unsigned long long");
+}
+ }
+
+ template
+ ValueParser& parseBool(Out& out){
+const char* options[] = {"false", "true"};
+const bool values[] = {false, true};
+auto index = parseAny(options);
+if(index == -1)throw ParseException("Couldn't parse bool");
+out = values[index];
+return *this;
+ }
+
+ int parseAny(const std::vector &options) {
+parseSpace()
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
adamdebreceni commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r431181731
##
File path: libminifi/test/unit/PropertyValidationTests.cpp
##
@@ -0,0 +1,244 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "../TestBase.h"
+#include "core/ConfigurableComponent.h"
+#include "utils/PropertyErrors.h"
+#include "core/PropertyValidation.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace 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::ConversionException);
+
+ const std::string SPACE = " ";
Review comment:
I was torn between adding a comment like "mind the leading space", or
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
