[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
szaszm commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r446156346
##
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:
With the new approach, these are no longer used and can be removed.
edit: That is, if you leave the new approach. It's currently failing on the
CI because there is no `parse(uint64_t&)` overload
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] szaszm commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
szaszm commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r446156346
##
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:
With the new approach, these are no longer used and can be removed.
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] szaszm commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
szaszm commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r445538363
##
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:
This particular case is not specified by the style guide but it
specifies most cases of continuation indentation to be 4 spaces wide, so I
suggest making this one (and the rest of the tests in this file) so as well.
##
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:
Tip: `using ValueException::ValueException;`
##
File path: libminifi/include/core/ConfigurableComponent.h
##
@@ -215,18 +215,23 @@ bool ConfigurableComponent::getProperty(const std::string
name, T ) const
auto & = 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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
szaszm commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r443776103
##
File path: libminifi/include/core/PropertyValue.h
##
@@ -202,14 +196,50 @@ class PropertyValue : public state::response::ValueNode {
auto operator=(const std::string ) -> 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:
Good point, I take my suggestions back.
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] szaszm commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
szaszm commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r441597164
##
File path: libminifi/include/core/PropertyValue.h
##
@@ -202,14 +196,50 @@ class PropertyValue : public state::response::ValueNode {
auto operator=(const std::string ) -> 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:
This could use `core::getClassName`. I would also make this public
and encourage its use over the implicit conversion operators, with a long term
goal of moving to explicit conversions.
##
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 ) 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:
This logic should move to `CachedValueValidator` and the caching
behavior should be transparent IMO.
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] szaszm commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
szaszm commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r441593095
##
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:
Not that I'm aware of. I think we can assume that if something is inside
an `impl` or `internal` or similar namespace, then people won't depend on it,
but sadly no clear conventions in this regard.
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] szaszm commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
szaszm commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r440829307
##
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:
In my understanding, when I move construct something, I want the other
object's content in the new object and preferably leave the other object in a
somewhat valid state. (There is no clear consensus in the community about the
last part as far as I'm aware.)
What happens here is we take the validator, but not the cached result. This
way after a move, we are forced to redo the validation, and I assume this is
somewhat expensive, otherwise we wouldn't need a cache. In the other object, we
leave a moved-from validator with a validation result that no longer
corresponds to the validator, and is therefore invalid.
Given the above, this seems less than ideal, but it's possible that I didn't
fully understand your point and the code.
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] szaszm commented on a change in pull request #797: MINIFICPP-1231 - General property validation + use them in MergeContent.
szaszm commented on a change in pull request #797:
URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r440784872
##
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:
This approach seems to violate the Open-closed principle. I don't mind
it, as I'm not a big fan of its "open" part, but I'm interested why did you
choose this approach over `protected` constructors.
##
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:
Instead of taking over the `validator_` but leaving the
`validation_result_` in place, why don't we take over both and reset
`other.validation_result_` to the default `RECOMPUTE`?
##
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,
+
