hunyadi-dev commented on a change in pull request #797: URL: https://github.com/apache/nifi-minifi-cpp/pull/797#discussion_r431026306
########## 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: Basically, this class has the implementation of a basic `optional<bool>`. Shouldn't it be cleaner to have it extracted out to a helper class with `get` `set` and `clear` methods, and just call those here? ########## 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<PropertyValidator>& other) : validator_(other) {} + 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; + return *this; + } + + const std::shared_ptr<PropertyValidator>& operator->() const { + return validator_; + } + + operator bool() const { + return (bool)validator_; + } + + const std::shared_ptr<PropertyValidator>& 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: The right side of `||` is redundant here ```c++ if(!validator) { return Result::SUCCESS; } return validation_result_; ``` ########## 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: What is the point of defining a constant for " "? ########## 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<PropertyValidator>& other) : validator_(other) {} + 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; + return *this; + } + + const std::shared_ptr<PropertyValidator>& operator->() const { + return validator_; + } + + operator bool() const { + return (bool)validator_; Review comment: C style cast. ########## File path: libminifi/include/core/PropertyValue.h ########## @@ -199,15 +214,40 @@ class PropertyValue : public state::response::ValueNode { auto operator=(const std::string &ref) -> typename std::enable_if< std::is_same<T, DataSizeValue >::value || std::is_same<T, TimePeriodValue >::value,PropertyValue&>::type { - value_ = std::make_shared<T>(ref); - type_id = value_->getTypeIndex(); - return *this; + validator_.clearValidationResult(); + return WithAssignmentGuard(ref, [&] () -> PropertyValue& { + value_ = std::make_shared<T>(ref); + type_id = value_->getTypeIndex(); + return *this; + }); + } + + private: + + bool isValueUsable() const { + if (!value_) return false; + if (validator_.isValid() == CachedValueValidator::Result::FAILURE) return false; + if (validator_.isValid() == CachedValueValidator::Result::SUCCESS) return true; + return validate("__unknown__").valid(); + } + + template<typename Fn> + auto WithAssignmentGuard(const std::string& ref, Fn&& functor) -> decltype(std::forward<Fn>(functor)()) { + // TODO: as soon as c++17 comes jump to a RAII implementation Review comment: Minor, but once this is linter checked, the expected format for TODO-s would become `// TODO(username): ...` ########## 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: Might be simpler using [copy-and-swap](https://stackoverflow.com/a/3279550/13369902). ########## 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: Is there any point in setting the validator if it is not set? I find it a bit confusing. We now have a `CachedValidator` in `PropertyValue` which would return VALID if no validator is set. ########## 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<int>(0) + ->build(); + REQUIRE_THROWS_AS(prop.setValue("not int"), utils::ParseException); + REQUIRE_THROWS_AS(static_cast<int>(prop.getValue()), utils::InvalidValueException); +} + +TEST_CASE("Parsing int") { + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<int>(0) + ->build(); + REQUIRE_THROWS_AS(prop.setValue("not int"), utils::ParseException); +} + +TEST_CASE("Parsing int has baggage after") { + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<int>(0) + ->build(); + REQUIRE_THROWS_AS(prop.setValue("55almost int"), utils::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 "), utils::ParseException); +} + +TEST_CASE("Parsing bool has baggage after") { + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<bool>(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<typename Fn> + void setPropertyModifiedCallback(Fn&& functor) { + onPropertyModifiedCallback = std::forward<Fn>(functor); + } + + template<typename Fn> + void setDynamicPropertyModifiedCallback(Fn&& functor) { + onDynamicPropertyModifiedCallback = std::forward<Fn>(functor); + } + + private: + std::function<void(const Property&, const Property&)> onPropertyModifiedCallback; + std::function<void(const Property&, const Property&)> onDynamicPropertyModifiedCallback; +}; + +TEST_CASE("Missing Required With Default") { + auto prop = PropertyBuilder::createProperty("prop") + ->isRequired(true) + ->withDefaultValue<std::string>("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(false) + ->build(); + TestConfigurableComponent component; + component.setSupportedProperties({prop}); + std::string value; + REQUIRE_FALSE(component.getProperty(prop.getName(), value)); +} + +TEST_CASE("Valid Optional Without Default") { + // without a default the value will be stored as a string + auto prop = PropertyBuilder::createProperty("prop") + ->isRequired(false) + ->build(); + TestConfigurableComponent component; + component.setSupportedProperties({prop}); + component.setProperty(prop.getName(), "some data"); + std::string value; + REQUIRE(component.getProperty(prop.getName(), value)); + REQUIRE(value == "some data"); +} + +TEST_CASE("Invalid With Default") { + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<bool>(true) + ->build(); + TestConfigurableComponent component; + component.setSupportedProperties({prop}); + REQUIRE_THROWS_AS(component.setProperty("prop", "banana"), utils::ParseException); + std::string value; + REQUIRE_THROWS_AS(component.getProperty(prop.getName(), value), utils::InvalidValueException); +} + +TEST_CASE("Valid With Default") { + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<int>(55) + ->build(); + TestConfigurableComponent component; + component.setSupportedProperties({prop}); + REQUIRE(component.setProperty("prop", "23")); + int value; + REQUIRE(component.getProperty(prop.getName(), value)); + REQUIRE(value == 23); +} + +TEST_CASE("Invalid conversion") { + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<std::string>("banana") + ->build(); + TestConfigurableComponent component; + component.setSupportedProperties({prop}); + bool value; + REQUIRE_THROWS_AS(component.getProperty(prop.getName(), value), utils::ConversionException); +} + +TEST_CASE("Write Invalid Then Override With Valid") { + // we always base the assignment on the default value + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<int>(55) + ->build(); + TestConfigurableComponent component; + component.setSupportedProperties({prop}); + REQUIRE_THROWS_AS(component.setProperty(prop.getName(), "banana"), utils::ConversionException); + component.setProperty(prop.getName(), "98"); + int value; + REQUIRE(component.getProperty(prop.getName(), value)); + REQUIRE(value == 98); +} + +TEST_CASE("Property Change notification gets called even on erroneous assignment") { + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<bool>(true) + ->build(); + TestConfigurableComponent component; + component.setSupportedProperties({prop}); + int callbackCount = 0; + component.setPropertyModifiedCallback([&] (const Property&, const Property&) { + ++callbackCount; + }); + REQUIRE_THROWS_AS(component.setProperty(prop.getName(), "banana"), utils::ConversionException); + REQUIRE(callbackCount == 1); +} + +TEST_CASE("Correctly Typed Property With Invalid Validation") { + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<int64_t>(5, std::make_shared<LongValidator>("myValidator", 0, 10)) + ->build(); + TestConfigurableComponent component; + component.setSupportedProperties({prop}); + int callbackCount = 0; + component.setPropertyModifiedCallback([&] (const Property&, const Property&) { + ++callbackCount; + }); + REQUIRE_THROWS_AS(component.setProperty(prop.getName(), "20"), utils::InvalidValueException); + REQUIRE(callbackCount == 1); +} + Review comment: Minor, but maybe it would make it a bit more organized/decrease the ammount of cluttering, if the tests were separated in sections. ########## File path: libminifi/include/utils/PropertyErrors.h ########## @@ -0,0 +1,118 @@ +/** + * + * 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_PROPERTYERRORS_H +#define NIFI_MINIFI_CPP_PROPERTYERRORS_H + +#include "Exception.h" + +namespace org{ +namespace apache{ +namespace nifi{ +namespace minifi{ Review comment: The namespaces until this point could be combined with the ones below. ########## File path: libminifi/include/utils/PropertyErrors.h ########## @@ -0,0 +1,118 @@ +/** + * + * 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_PROPERTYERRORS_H +#define NIFI_MINIFI_CPP_PROPERTYERRORS_H + +#include "Exception.h" + +namespace org{ +namespace apache{ +namespace nifi{ +namespace minifi{ +namespace core{ +class PropertyValue; +class ConfigurableComponent; +class Property; +} Review comment: No comments on closing tags for namespaces. ########## 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: Is this better than the original? What problem does it solve? If it is throwing in `addValue`, then maybe we should fix that possibility by catching potential exceptions there. ########## 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<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{ + 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: Minor, but do we really want to force the caller to handle required and non required properties differently? If we really want that functionality implemented here, maybe a better design would be to have two functions, this with the original behaviour, and one that explicitly has "throws if missing required" in its name. ########## 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 <exception> +#include <string> +#include <cstring> +#include <vector> +#include <cstdlib> +#include "PropertyErrors.h" +#include <type_traits> +#include <limits> + +namespace org { +namespace apache { +namespace nifi { +namespace minifi { +namespace utils { + +class ValueParser { + private: + template< class... > + using void_t = void; Review comment: This is already defined in `GeneralUtils.h` ########## 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: Why do we have an rvalue reference here? ########## File path: libminifi/include/core/PropertyValue.h ########## @@ -71,64 +72,75 @@ class PropertyValue : public state::response::ValueNode { } std::shared_ptr<PropertyValidator> 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: How about refactoring these functions to something like: ```c++ template <typename T> T convert() { if(!isValueUsable())throw utils::InvalidValueException("Cannot convert invalid value"); T res; if (value_->convertValue(res)) { return res; } // __FUNCSIG__ on WIN32 throw utils::ConversionException(std::string("Invalid ") + __PRETTY_FUNCTION__ + value_->getStringValue()); } operator uint64_t() const { return convert<uint64_t>(); } operator int64_t() const { return convert<int64_t>(); } // ... ########## 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 <exception> +#include <string> +#include <cstring> +#include <vector> +#include <cstdlib> +#include "PropertyErrors.h" +#include <type_traits> +#include <limits> + +namespace org { +namespace apache { +namespace nifi { +namespace minifi { +namespace utils { + +class ValueParser { + private: + template< class... > + using void_t = void; + + 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: + 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"); + try { + char *end; + long result{std::strtol(str.c_str() + offset, &end, 10)}; + offset = end - str.c_str(); + if (result < (std::numeric_limits<int>::min)() || result > (std::numeric_limits<int>::max)()) { + throw ParseException("Cannot convert long to int"); + } + out = {static_cast<int>(result)}; + return *this; + }catch(...){ + throw ParseException("Could not parse int"); + } + } + + template<typename Out> + ValueParser& parseLong(Out& out) { + static_assert(is_non_narrowing_convertible<long, Out>::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<typename Out> + ValueParser& parseLongLong(Out& out) { + static_assert(is_non_narrowing_convertible<long long, Out>::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<typename Out> + ValueParser& parseUInt32(Out& out) { + static_assert(is_non_narrowing_convertible<uint32_t, Out>::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<uint32_t>::max)()) { + throw ParseException("Cannot convert unsigned long to uint32_t"); + } + out = {static_cast<uint32_t>(result)}; + return *this; + }catch(...){ + throw ParseException("Could not parse unsigned long"); + } + } + + 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"); + 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<typename Out> + 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<std::string> &options) { + parseSpace(); + for (std::size_t optionIdx = 0; optionIdx < options.size(); ++optionIdx) { + const auto &option = options[optionIdx]; + if (offset + option.length() <= str.length()) { + if (std::equal(option.begin(), option.end(), str.begin() + offset)) { + offset += option.length(); + return optionIdx; + } + } + } + return -1; + } + + template<std::size_t N> + int parseAny(const char* (&options)[N]) { Review comment: Is this used? Do we expect to know `N` in compile time? ########## File path: libminifi/include/core/PropertyValue.h ########## @@ -138,26 +150,28 @@ class PropertyValue : public state::response::ValueNode { */ template<typename T> auto operator=(const T ref) -> typename std::enable_if<std::is_same<T, std::string>::value,PropertyValue&>::type { Review comment: Maybe I am missing the obvious, but what is the advantage of templating this over just having ```c++ PropertyValue& operator=(const std::string& ref) { ... } ``` Would this for example not fail in case we pass `std::string&&` or `const std::string` as `T`? ########## 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<int>(0) + ->build(); + REQUIRE_THROWS_AS(prop.setValue("not int"), utils::ParseException); + REQUIRE_THROWS_AS(static_cast<int>(prop.getValue()), utils::InvalidValueException); +} + +TEST_CASE("Parsing int") { + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<int>(0) + ->build(); + REQUIRE_THROWS_AS(prop.setValue("not int"), utils::ParseException); +} + +TEST_CASE("Parsing int has baggage after") { + auto prop = PropertyBuilder::createProperty("prop") + ->withDefaultValue<int>(0) + ->build(); + REQUIRE_THROWS_AS(prop.setValue("55almost int"), utils::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") { Review comment: 👍 It is a good idea to test this. ########## 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 <exception> +#include <string> +#include <cstring> +#include <vector> +#include <cstdlib> +#include "PropertyErrors.h" +#include <type_traits> +#include <limits> + +namespace org { +namespace apache { +namespace nifi { +namespace minifi { +namespace utils { + +class ValueParser { + private: + template< class... > + using void_t = void; + + template<typename From, typename To, typename = void> + struct is_non_narrowing_convertible: std::false_type { Review comment: Maybe you could simplify the asserts by pulling in [this](https://stackoverflow.com/a/29603857/13369902). [[Demo]](https://godbolt.org/#g:!((g:!((k:0.7001909611712285,l:'2',m:100,n:'0',o:'',t:'0'),(g:!((g:!((h:codeEditor,i:(fontScale:14,j:2,lang:c%2B%2B,selection:(endColumn:1,endLineNumber:22,positionColumn:1,positionLineNumber:22,selectionStartColumn:1,selectionStartLineNumber:22,startColumn:1,startLineNumber:22),source:'%23include+%3Ciostream%3E%0A%23include+%3Cstring%3E%0A%23include+%3Cutility%3E%0A%23include+%3Cmemory%3E%0A%23include+%3Ctype_traits%3E%0A%0Atemplate%3Ctypename...+Conds%3E%0A++struct+and_%0A++:+std::true_type%0A++%7B+%7D%3B%0A%0Atemplate%3Ctypename+Cond,+typename...+Conds%3E%0A++struct+and_%3CCond,+Conds...%3E%0A++:+std::conditional%3CCond::value,+and_%3CConds...%3E,+std::false_type%3E::type%0A++%7B+%7D%3B%0A%0Atemplate%3Ctypename...+T%3E%0Ausing+all_integer+%3D+and_%3Cstd::is_integral%3CT%3E...%3E%3B%0A%0Astatic_assert(all_integer%3Cint,+uint64_t,+bool,+int%3E::value,+%22All+integers%22)%3B%0A//+static_assert(all_integer%3Cint,+uint64_t,+float,+bool%3E::value,+%22Not+all+integers%22)%3B%0A%0Aint+main()%0A%7B%0A++++return+0%3B%0A%7D'),l:'5',n:'0',o:'C%2B%2B+source+%232',t:'0')),k:50,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:clang1000,filters:(b:'0',binary:'1',commentOnly:'0',demangle:'0',directives:'0',execute:'0',intel:'0',libraryCode:'1',trim:'1'),fontScale:14,j:1,lang:c%2B%2B,libs:!(),options:'',selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:2),l:'5',n:'0',o:'x86-64+clang+10.0.0+(Editor+%232,+Compiler+%231)+C%2B%2B',t:'0')),header:(),k:50,l:'4',m:50,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compiler:1,editor:2,fontScale:8,wrap:'1'),l:'5',n:'0',o:'%231+with+x86-64+clang+10.0.0',t:'0')),header:(),l:'4',m:50,n:'0',o:'',s:0,t:'0')),k:50,l:'3',n:'0',o:'',t:'0')),k:99.29980903882876,l:'2',m:100,n:'0',o:'',t:'0')),l:'2',n:'0',o:'',t:'0')),version:4) ########## 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 <exception> +#include <string> +#include <cstring> +#include <vector> +#include <cstdlib> +#include "PropertyErrors.h" +#include <type_traits> +#include <limits> + +namespace org { +namespace apache { +namespace nifi { +namespace minifi { +namespace utils { + +class ValueParser { + private: + template< class... > + using void_t = void; + + 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: + 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"); + try { + char *end; + long result{std::strtol(str.c_str() + offset, &end, 10)}; + offset = end - str.c_str(); + if (result < (std::numeric_limits<int>::min)() || result > (std::numeric_limits<int>::max)()) { Review comment: Maybe this wouldn't work (due to `-Wnarrow`), but wouldn't this simplify things? ```c++ if(out != static_cast<int>(out)) { throw ...; } return { result }; ``` ########## 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<int>(0) + ->build(); + REQUIRE_THROWS_AS(prop.setValue("not int"), utils::ParseException); + REQUIRE_THROWS_AS(static_cast<int>(prop.getValue()), utils::InvalidValueException); +} + +TEST_CASE("Parsing int") { Review comment: Isn't this the same test case as the one above? ---------------------------------------------------------------- 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]
