[
https://issues.apache.org/jira/browse/NIFI-14400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17941359#comment-17941359
]
endzeit edited comment on NIFI-14400 at 4/6/25 12:03 PM:
---------------------------------------------------------
Thank you for the feedback [~pvillard].
While investigating a framework-wide solution for NIFI-14400 I've noticed that
a lot of existing components access properties regardless of their dependencies.
It can be assumed, that components maintained outside of the main NiFi project
exhibit similar behaviour.
Simply adjusting the behaviour of {{ProcessContext.getProperty}} or
{{PropertyValue.getValue}} / {{PropertyValue.isSet}} to take the dependencies
into consideration would then result in undesired runtime errors.
Thus, I'd like to propose a solution that consists of three steps:
- NIFI-14436 Adjust the {{TestRunner}} so that it errors when relying on
properties with unsatisfied dependencies - this should be opt-out. This both
hints developers that they should adjust their implementation but also eases
migration efforts by allowing to ignore the warning temporarily.
- NIFI-14437 Adjust the components in the NiFi project to only rely on
properties with satisfied dependencies
- NIFI-14438 At last, we can adjust the standard implementation of
ProcessContext to take unsatisfied dependencies into consideration. This should
only be implemented after the TestRunner adjustment has been in place for a
long time, maybe even as part of a major release as it may break existing flows.
was (Author: endzeitbegins):
Thank you for the feedback [~pvillard].
While investigating a framework-wide solution for NIFI-14400 I've noticed that
a lot of existing components access properties regardless of their dependencies.
It can be assumed, that components maintained outside of the main NiFi project
exhibit similar behaviour.
Simply adjusting the behaviour of {{ProcessContext.getProperty}} or
{{PropertyValue.getValue}} / {{PropertyValue.isSet}} to take the dependencies
into consideration would then result in undesired runtime errors.
Thus, I'd like to propose a solution that consists of three steps:
- NIFI-14436 Adjust the {{TestRunner}} so that it errors - this should be
opt-out. This both hints developers that they should adjust their
implementation but also eases migration efforts by allowing to ignore the
warning temporarily.
- NIFI-14437 Adjust the components in the NiFi project to only rely on
properties with satisfied dependencies
- NIFI-14438 At last, we can adjust the standard implementation of
ProcessContext to take unsatisfied dependencies into consideration. This should
only be implemented after the TestRunner adjustment has been in place for a
long time, maybe even as part of a major release as it may break existing flows.
> 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)
