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<int>(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 <string> + +#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 &value) const auto &&it = properties_.find(name); if (it != properties_.end()) { - Property item = it->second; - value = static_cast<T>(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()); Review comment: 1. A required property is missing. This should be a warning or error rather than a debug level log message. 2. This is core API, the behavior change here may cause existing setups to break. It may also be fine, I'm interested in the input of @arpadboda here. ########## 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<int>(0) + ->build(); + REQUIRE_THROWS_AS(prop.setValue("not int"), ParseException); + REQUIRE_THROWS_AS(static_cast<int>(prop.getValue()), InvalidValueException); +} + +TEST_CASE("Parsing int has baggage after") { + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<int>(0) + ->build(); + REQUIRE_THROWS_AS(prop.setValue("55almost int"), ParseException); +} + +TEST_CASE("Parsing int has spaces") { + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<int>(0) + ->build(); + prop.setValue(" 55 "); + REQUIRE(static_cast<int>(prop.getValue()) == 55); +} + +TEST_CASE("Parsing int out of range") { + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<int>(0) + ->build(); + REQUIRE_THROWS_AS(prop.setValue(" 5000000000 "), ParseException); Review comment: The standard permits `int` to be larger than 32 bits. We may want to 1. ignore those platforms if they exist and assume that all our supported platforms have 32 bit wide `int`s, 2. use an even larger number to cover some future architectures, or 3. generate a string with a large number based on `sizeof(int)`. I'm fine with the current version (option 1), just wanted to raise this question. ########## File path: libminifi/include/core/Property.h ########## @@ -129,41 +133,32 @@ class Property { template<typename T = std::string> 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: Would wrapping the validator in `gsl::not_null` be a viable option? It could keep compatibility with APIs that expect `shared_ptr` and still be guaranteed to be present. ########## 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 <exception> +#include <string> +#include <cstring> +#include <vector> +#include <cstdlib> +#include <type_traits> +#include <limits> + +#include "PropertyErrors.h" +#include "GeneralUtils.h" + +namespace org { +namespace apache { +namespace nifi { +namespace minifi { +namespace utils { +namespace internal { + +class ValueParser { + private: + template<typename From, typename To, typename = void> + struct is_non_narrowing_convertible : std::false_type { + static_assert(std::is_integral<From>::value && std::is_integral<To>::value, "Checks only integral values"); + }; + + template<typename From, typename To> + struct is_non_narrowing_convertible<From, To, void_t<decltype(To{std::declval<From>()})>> : std::true_type { + static_assert(std::is_integral<From>::value && std::is_integral<To>::value, "Checks only integral values"); + }; + + public: + explicit ValueParser(const std::string& str, std::size_t offset = 0) : str(str), offset(offset) {} + + template<typename Out> + ValueParser& parseInt(Out& out) { + static_assert(is_non_narrowing_convertible<int, Out>::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<int>::min)() || result > (std::numeric_limits<int>::max)()) { + throw ParseException("Cannot convert long to int"); + } + offset += len; + out = {static_cast<int>(result)}; + return *this; + } + + template<typename Out> + ValueParser& parseLong(Out& out) { + static_assert(is_non_narrowing_convertible<long, Out>::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<typename Out> + ValueParser& parseLongLong(Out& out) { + static_assert(is_non_narrowing_convertible<long long, Out>::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<typename Out> + ValueParser& parseUInt32(Out& out) { + static_assert(is_non_narrowing_convertible<uint32_t, Out>::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<uint32_t>::max)()) { + throw ParseException("Cannot convert unsigned long to uint32_t"); + } + offset += len; + out = {static_cast<uint32_t>(result)}; + return *this; + } + + template<typename Out> + ValueParser& parseUnsignedLongLong(Out& out) { + static_assert(is_non_narrowing_convertible<unsigned long long, Out>::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<typename Out> + ValueParser& parseBool(Out& out) { Review comment: Thinking about these signatures, I got the idea that I would really love to see: ``` bool result; ValueParser{string}.parse(result).parseEnd(); ``` `parse` could be templated on the parsed type and let it be deduced if it is an exact match with the out type. If it's not an exact match, an interface like this could work: `.parse<bool>(result)`, where `result` can be a different type where lossless conversion is possible, like now. This interface would probably make future uses of the class in generic code much easier. Untested implementation idea: ``` template<typename T, typename Out> ValueParser& parse(Out&); // unimplemented template<typename T> ValueParser& parse<T, T>(T& out) { return parse<T>(out); } template<typename Out> ValueParser& parse<bool, Out>(Out& out) { return parseBool(out); } template<typename Out> ValueParser& parse<int, Out>(Out& out) { return parseInt(out); } // ... ``` Let me know if you would like to work on a similar interface, if there are problems with the idea, or you just don't want to spend time on this. ########## 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<T>(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: related issue: https://issues.apache.org/jira/browse/MINIFICPP-1213 I had a brief attempt at fixing this in the past but the required/not required difference got me a bit confused, so I moved on. Now with the above change, the fix looks trivial: change from warn to debug. Do you agree? Would you mind doing it in context of this PR or rather suggest doing it separately? ########## 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 <utility> +#include <memory> +#include <string> +#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<PropertyValidator>& other) : validator_(other) {} + + explicit CachedValueValidator(std::shared_ptr<PropertyValidator>&& other) : validator_(std::move(other)) {} + + CachedValueValidator& operator=(const std::shared_ptr<PropertyValidator>& new_validator) { + validator_ = new_validator; + validation_result_ = Result::RECOMPUTE; + return *this; + } + + CachedValueValidator& operator=(std::shared_ptr<PropertyValidator>&& new_validator) { + validator_ = std::move(new_validator); + validation_result_ = Result::RECOMPUTE; Review comment: Calling `invalidateCachedResult()` even in members would be more human-readable 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: [email protected]
