[
https://issues.apache.org/jira/browse/NIFI-14400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17940937#comment-17940937
]
Pierre Villard commented on NIFI-14400:
---------------------------------------
Great write up [~EndzeitBegins] - I think option 1 is definitely what we should
be doing. Having something at framework level to make sure we have proper
behavior everywhere is what we should aim for.
> Properties with unmet dependency constraint should not be regarded as set /
> provide a value
> -------------------------------------------------------------------------------------------
>
> Key: NIFI-14400
> URL: https://issues.apache.org/jira/browse/NIFI-14400
> Project: Apache NiFi
> Issue Type: Bug
> Reporter: endzeit
> Priority: Major
>
> The {{dependsOn}} mechanism in {{PropertyDescriptor.Builder{}}} establishes
> conditional property relevance based on other property values. As documented:
> {noformat}
> Establishes a relationship between this Property and the given property by
> declaring that this Property is only relevant if the given Property has a
> non-null value.
> Furthermore, if one or more explicit Allowable Values are provided, this
> Property will not be relevant unless the given Property's value is equal to
> one of the given Allowable Values.
> If this method is called multiple times, each with a different dependency,
> then a relationship is established such that this Property is relevant only
> if all dependencies are satisfied.
> {noformat}
> As documented, _In the case that this property is NOT considered to be
> relevant ..., the property will not be shown in the component's configuration
> in the User Interface._
> However, the property retains its {{PropertyValue.isSet()}} status and a
> non-null {{PropertyValue.getValue()}} even when its dependencies are not met,
> leading to potential implementation errors and difficult to trace errors.
> h2. Example
> The {{MergeContent}} processor uses {{dependsOn}} to manage properties based
> on the selected "Merge Strategy". The default "Bin-Packing Algorithm"
> strategy includes dependent properties like "Maximum Group Size".
> * Problem:
> # User sets "Maximum Group Size" in "Bin-Packing Algorithm" mode.
> # User switches to "Defragment" mode, where "Maximum Group Size" is no longer
> visible in the UI.
> # The processor still uses the previously set "Maximum Group Size" value.
> *Consequences:*
> * FlowFiles can be incorrectly routed to the FAILURE relationship due to
> unexpected size constraints.
> * This can create endless loops if the FAILURE relationship routes back to
> the processor.
> * Other FlowFiles in the bundle may be queued indefinitely.
> * The issue is difficult to diagnose from the UI, as the configuration
> appears correct. However, the problematic property value is visible in the
> API request.
> h2. Test Case
> A test case demonstrating the issue shows that:
> # Without the dependent property set, the processor behaves as expected.
> # With the dependent property set, it is incorrectly applied even when the
> dependency is not met.
> {code}
> @Test
> public void testDefragmentWithDependantProperty() throws IOException {
> final TestRunner runner = TestRunners.newTestRunner(new
> MergeContent());
> runner.setProperty(MergeContent.MERGE_STRATEGY,
> MergeContent.MERGE_STRATEGY_DEFRAGMENT);
> final Map<String, String> attributes = new HashMap<>();
> attributes.put(MergeContent.FRAGMENT_ID_ATTRIBUTE, "1");
> attributes.put(MergeContent.FRAGMENT_COUNT_ATTRIBUTE, "2");
> // this works as expected
> attributes.put(MergeContent.FRAGMENT_INDEX_ATTRIBUTE, "1");
> runner.enqueue("Some long text to demonstrate
> ".getBytes(StandardCharsets.UTF_8), attributes);
> runner.run();
> attributes.put(MergeContent.FRAGMENT_INDEX_ATTRIBUTE, "2");
> runner.enqueue("Another text".getBytes(StandardCharsets.UTF_8),
> attributes);
> runner.run();
> runner.assertTransferCount(MergeContent.REL_MERGED, 1);
> final MockFlowFile assembled =
> runner.getFlowFilesForRelationship(MergeContent.REL_MERGED).getFirst();
> assembled.assertContentEquals("Some long text to demonstrate Another
> text".getBytes(StandardCharsets.UTF_8));
> runner.clearTransferState();
> // this does no longer, even though the property should not be taken
> into account and is not visible in UI
> runner.setProperty(MergeContent.MAX_SIZE, "15 b");
> attributes.put(MergeContent.FRAGMENT_INDEX_ATTRIBUTE, "1");
> runner.enqueue("Some long text to demonstrate
> ".getBytes(StandardCharsets.UTF_8), attributes);
> runner.run();
> attributes.put(MergeContent.FRAGMENT_INDEX_ATTRIBUTE, "2");
> runner.enqueue("Another text".getBytes(StandardCharsets.UTF_8),
> attributes);
> runner.run();
> // whoops - one goes to FAILURE
> runner.assertTransferCount(MergeContent.REL_FAILURE, 1);
> final MockFlowFile failed =
> runner.getFlowFilesForRelationship(MergeContent.REL_FAILURE).getFirst();
> failed.assertContentEquals("Some long text to demonstrate
> ".getBytes(StandardCharsets.UTF_8));
> runner.assertQueueNotEmpty(); // and the other ones still queued up
> }
> {code}
> ----
> h2. Proposed Solutions
> Several potential solutions exist to address the discrepancy between UI
> visibility and actual property value retention when using dependsOn.
> h3. Option 1: Framework-Level Dependency Enforcement
> Modify the framework to treat properties whose dependency constraints are not
> met as if they are not set. Consequently, {{PropertyValue.isSet()}} would
> return false, and {{PropertyValue.getValue()}} would return {{null}}. This
> approach would prevent users from encountering hard-to-detect configuration
> errors and eliminate the need for processor implementors to explicitly check
> dependencies before accessing dependent properties.
> h3. Option 2: UI-Driven Value Suppression
> Instead of modifying the framework, the UI could be adjusted to prevent the
> submission of values for properties that are not visible to the user. This
> would ensure that the API receives only relevant property values, effectively
> mitigating the issue without altering the underlying framework behaviour.
> h3. Option 3: Processor-Specific Fix (MergeContent)
> Implement a targeted fix specifically for the MergeContent processor, where
> this issue was initially observed. This solution would address the problem in
> the immediate context but would not provide a general solution for other
> processors that may exhibit similar behavior.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
