Copilot commented on code in PR #2241:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2241#discussion_r3756503169
##########
libminifi/src/core/flow/StructuredConfiguration.cpp:
##########
@@ -936,6 +935,18 @@ void
StructuredConfiguration::parsePropertyNodeElement(const std::string& proper
ParameterContext* parameter_context) {
logger_->log_trace("Encountered {}", property_name);
if (!property_value_node || property_value_node.isNull()) {
+ auto my_prop = component.getSupportedProperty(property_name);
+ if (!my_prop.has_value()) {
+ // Dynamic property fallback for previous workflow
+ return;
+ }
+ if (my_prop->getRequired()) {
+ raiseComponentError(component.getName(), "", "Can't explicitly unset
required property");
Review Comment:
The error identifies neither the property nor its configuration section, so
components with several required properties leave users unable to tell which
null entry failed. Include `property_name` in the reason, consistent with the
required-property validation message below.
##########
libminifi/src/core/flow/StructuredConfiguration.cpp:
##########
@@ -384,6 +381,8 @@ void StructuredConfiguration::parseProcessorNode(const
Node& processors_node, co
// handle processor properties
if (Node propertiesNode = procNode[schema_.processor_properties]) {
parsePropertiesNode(propertiesNode, *processor, procCfg.name,
parentGroup->getParameterContext());
+ } else {
+ validateComponentProperties(*processor, procCfg.name, "");
Review Comment:
This only closes the omitted-`Properties` validation gap for processors.
Parameter providers (`parseParameterProvidersNode`, lines 242–244) and
controller-service implementations (`parseControllerServices`, lines 662–668)
still invoke `validateComponentProperties` only when a properties node exists,
so required properties on those component types remain bypassable when the
entire section is omitted. Apply the same absent-node validation at those call
sites as well.
##########
extensions/stable-api-testing/tests/ConfigTests.cpp:
##########
@@ -0,0 +1,215 @@
+/**
+ *
+ * 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 "core/flow/AdaptiveConfiguration.h"
+#include "unit/Catch.h"
+#include "unit/ConfigurationTestController.h"
+#include "unit/TestBase.h"
+#include "unit/TestUtils.h"
+
+using namespace std::literals::chrono_literals;
+
+TEST_CASE("Required property without Properties Entry (adaptive yaml)") {
+ const ConfigurationTestController controller;
+ core::flow::AdaptiveConfiguration config{controller.getContext()};
+ const auto config_yaml = R"(
+MiNiFi Config Version: 3
+Flow Controller:
+ name: MiNiFi Flow
+Processors:
+ - name: My processor
+ id: 00000000-0000-0000-0000-000000000001
+ class: PropertyTester
+Connections: [ ]
+Remote Process Groups: [ ]
+)";
+ REQUIRE_THROWS(config.getRootFromPayload(config_yaml));
+ REQUIRE(minifi::test::utils::verifyLogLinePresenceInPollTime(100ms,
+ "[error] Error while processing configuration file: Unable to parse
configuration file for component named 'My processor' because required "
+ "property 'RequiredPropertyWithoutDefaultValue' is not set"));
+}
+
+TEST_CASE("Required property without Properties Entry (adaptive json)") {
+ const ConfigurationTestController controller;
+ core::flow::AdaptiveConfiguration config{controller.getContext()};
+ const auto config_json = R"(
+{
+ "Flow Controller": {"name": "root"},
+ "Processors": [
+ {
+ "id": "00000000-0000-0000-0000-000000000001",
+ "class": "PropertyTester",
+ "name": "My processor"
+ }
+ ],
+ "Connections": []
+}
+ )";
+ REQUIRE_THROWS(config.getRootFromPayload(config_json));
+ REQUIRE(minifi::test::utils::verifyLogLinePresenceInPollTime(100ms,
+ "[error] Error while processing configuration file: Unable to parse
configuration file for component named 'My processor' because required "
+ "property 'RequiredPropertyWithoutDefaultValue' is not set"));
+}
+
+TEST_CASE("Explicitly unsetting required property (adaptive yaml)") {
+ const ConfigurationTestController controller;
+ core::flow::AdaptiveConfiguration config{controller.getContext()};
+ const auto config_yaml = R"(
+MiNiFi Config Version: 3
+Flow Controller:
+ name: MiNiFi Flow
+Processors:
+ - name: My processor
+ id: 00000000-0000-0000-0000-000000000001
+ class: PropertyTester
+ Properties:
+ RequiredPropertyWithDefaultValue: ~
+Connections: [ ]
+Remote Process Groups: [ ]
+)";
+ REQUIRE_THROWS(config.getRootFromPayload(config_yaml));
+ REQUIRE(minifi::test::utils::verifyLogLinePresenceInPollTime(100ms,
+ "[error] Error while processing configuration file: Unable to parse
configuration file for component named 'My processor' because Can't "
+ "explicitly unset required property"));
+}
+
+TEST_CASE("Explicitly unsetting required property (adaptive json)") {
+ const ConfigurationTestController controller;
+ core::flow::AdaptiveConfiguration config{controller.getContext()};
+ const auto config_json = R"(
+{
+ "Flow Controller": {"name": "root"},
+ "Processors": [
+ {
+ "id": "00000000-0000-0000-0000-000000000001",
+ "class": "PropertyTester",
+ "name": "My processor",
+ "Properties": {
+ "RequiredPropertyWithDefaultValue": null,
+ },
+ }
Review Comment:
This payload has trailing commas after both the property and processor
objects, so RapidJSON rejects it and `AdaptiveConfiguration` falls back to
parsing it as YAML. Consequently this test does not exercise explicit null
handling through the JSON node implementation as its name claims. Remove both
trailing commas so the JSON path is actually covered.
This issue also appears in the following locations of the same file:
- line 152
- line 204
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
