bbende commented on code in PR #11645:
URL: https://github.com/apache/nifi/pull/11645#discussion_r3972573556
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/parameter/StandardParameterContext.java:
##########
@@ -805,7 +805,10 @@ public void verifyCanSetParameters(final Map<String,
Parameter> updatedParameter
* @throws IllegalStateException if setting the given set of Parameters is
not legal
*/
private void verifyCanSetParameters(final Map<String, Parameter>
updatedParameters, final boolean duringUpdate) {
- verifyCanSetParameters(parameters, updatedParameters, duringUpdate);
+ // Use the effective parameter map so inherited parameters and
resolved aliases are compared against
+ // the values components actually see. Using the local map treats an
inherited name as "new" and
+ // incorrectly requires referencing components to be stopped.
+ verifyCanSetParameters(getEffectiveParameters(), updatedParameters,
duringUpdate);
Review Comment:
This is a correct and worthwhile fix, but it technically isn't required now
that the synchronizer skips inherited parameters.
It's a separate behavioral change and is used by every parameter-context
update in the code base. I would consider separating this to it's own JIRA with
the new contract stated explicitly and tests for the provider-backed and
sensitivity-shadowing cases.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/test/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizerTest.java:
##########
@@ -1918,6 +1918,111 @@ public void
testParameterContextDescriptionUpdatedDuringProcessGroupSync() throw
assertEquals(UPDATED_CONTEXT_DESCRIPTION,
paramContext.getDescription());
}
+ @Test
+ public void
testExistingLocalParameterValuePreservedWhenPreserveExistingEntries() throws
FlowSynchronizationException, InterruptedException, TimeoutException {
+ final VersionedParameterContext versionedContext =
createVersionedParameterContextWithDescriptions(CONTEXT_NAME_PARAMS,
+ SINGLE_PARAMETER, ORIGINAL_DESCRIPTION_MAP,
Collections.emptySet());
+ synchronizer.synchronize(null, versionedContext,
synchronizationOptions);
+
+ final ParameterContext paramContext =
parameterContextManager.getParameterContextNameMapping().get(CONTEXT_NAME_PARAMS);
+ assertEquals(VALUE_XYZ,
paramContext.getParameter(PARAM_ABC).get().getValue());
+ assertEquals(ORIGINAL_PARAMETER_DESCRIPTION,
paramContext.getParameter(PARAM_ABC).get().getDescriptor().getDescription());
+
+ final ProcessGroup processGroup = createMockProcessGroup();
+ when(processGroup.getParameterContext()).thenReturn(paramContext);
+
+ final VersionedParameterContext proposedParams =
createVersionedParameterContextWithDescriptions(CONTEXT_NAME_PARAMS,
+ Map.of(PARAM_ABC, VALUE_123), UPDATED_DESCRIPTION_MAP,
Collections.emptySet());
+ proposedParams.setDescription(UPDATED_CONTEXT_DESCRIPTION);
+
+ final VersionedProcessGroup rootGroup = new VersionedProcessGroup();
+ rootGroup.setIdentifier(processGroup.getIdentifier());
+ rootGroup.setParameterContextName(CONTEXT_NAME_PARAMS);
+
+ final VersionedExternalFlow externalFlow = new VersionedExternalFlow();
+ externalFlow.setFlowContents(rootGroup);
+ externalFlow.setParameterContexts(Map.of(CONTEXT_NAME_PARAMS,
proposedParams));
+
+ synchronizer.synchronize(processGroup, externalFlow,
preserveExistingParameterContextOptions());
+
+ assertEquals(VALUE_XYZ,
paramContext.getParameter(PARAM_ABC).get().getValue(),
+ "KEEP_EXISTING import must not overwrite an existing parameter
value");
+ assertEquals(ORIGINAL_PARAMETER_DESCRIPTION,
paramContext.getParameter(PARAM_ABC).get().getDescriptor().getDescription(),
Review Comment:
Interestingly, if you run these 3 new tests without the rest of the changes
in this PR (i.e. using the behavior on main), they all pass except for this
line about the description, the value check above passes indicating that the
value is not altered.
Should we try to alter these tests to reproduce the problem you experienced
to ensure it is fixed?
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ProcessGroupResource.java:
##########
@@ -1160,8 +1160,10 @@ public Response createProcessGroup(
// placed the Process Group. However, we do want to
use the name of the Process Group that is in the Flow Contents.
// To accomplish this, we call
updateProcessGroupContents() passing 'true' for the updateSettings flag but
null out the position.
flowSnapshot.getFlowContents().setPosition(null);
+ final boolean preserveExistingParameterContextEntries =
+
ParameterContextHandlingStrategy.KEEP_EXISTING.equals(parameterContextHandlingStrategy);
Review Comment:
The existing code reads `parameterContextHandlingStrategy` exactly once, and
it does so inside a block that is guarded so it only runs on the originating
node:
`ProcessGroupResource.java`
`Ln 1070–1091`
```
if (versionControlInfo != null &&
requestProcessGroupEntity.getVersionedFlowSnapshot() == null) {
// ... fetch snapshot from registry ...
// Step 4: Replace parameter contexts if necessary
if
(ParameterContextHandlingStrategy.REPLACE.equals(parameterContextHandlingStrategy))
{
parameterContextReplacer.replaceParameterContexts(flowSnapshot,
serviceFacade.getParameterContexts());
}
```
The `getVersionedFlowSnapshot() == null` condition is the key. Step 6 of
that same block sets the snapshot on the entity, and the entity is what gets
replicated. So on the receiving nodes the snapshot is already populated, the
whole block is skipped, and the query param is never consulted there. The
renaming decision has already been baked into the request body. That's why
`REPLACE` works correctly in a cluster today despite the query param being
dropped by getAbsolutePath().
The PR adds a second read of the strategy, and it puts it inside the
`withWriteLock` callback — which is the one place that runs on every node,
always after replication. On the receiving nodes that read has no query param
to work with, so it always resolves to the `@DefaultValue("KEEP_EXISTING")`.
So the underlying mechanism (query params don't survive replication) is
pre-existing, and the existing code is correctly written around it. The
divergence between standalone and clustered behavior is new in this PR.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java:
##########
@@ -2423,28 +2423,42 @@ private void addMissingConfiguration(final
VersionedParameterContext versionedPa
return;
}
+ // NIFI-16318: KEEP_EXISTING imports must not overwrite existing
values or materialize local overrides
+ // for inherited parameters. Version upgrades still apply
description-only updates to locally defined parameters.
+ final boolean preserveExistingEntries = syncOptions != null &&
syncOptions.isPreserveExistingParameterContextEntries();
final Map<String, Parameter> parameters = new HashMap<>();
for (final VersionedParameter versionedParameter :
versionedParameterContext.getParameters()) {
- final Optional<Parameter> parameterOption =
currentParameterContext.getParameter(versionedParameter.getName());
- if (parameterOption.isPresent()) {
- final Parameter existingParameter = parameterOption.get();
- if
(!Objects.equals(existingParameter.getDescriptor().getDescription(),
versionedParameter.getDescription())) {
+ final Parameter localParameter =
currentParameterContext.getParameters().get(parameterDescriptor(versionedParameter.getName()));
+ if (localParameter != null) {
+ // Never overwrite an existing local value. Description-only
updates apply on version upgrades,
+ // not KEEP_EXISTING imports, and must be built from the local
raw parameter so aliases are preserved.
+ if (!preserveExistingEntries
+ &&
!Objects.equals(localParameter.getDescriptor().getDescription(),
versionedParameter.getDescription())) {
final Parameter updatedParameter = new Parameter.Builder()
- .fromParameter(existingParameter)
+ .fromParameter(localParameter)
Review Comment:
Just wanted to clarify that the original intent here of using
`fromParameter` was to preserve the value.
I think the actual bug was in the previous code at line 2428 when it used
`currentParameterContext.getParameter` which returns the resolved effective
parameter. So for a parameter whose value is exactly `#{other}` in a context
that has inheritance, a description-only update materialized a local parameter
holding the resolved literal and destroyed the reference.
Is that what you ran into?
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java:
##########
@@ -2423,28 +2423,42 @@ private void addMissingConfiguration(final
VersionedParameterContext versionedPa
return;
}
+ // NIFI-16318: KEEP_EXISTING imports must not overwrite existing
values or materialize local overrides
+ // for inherited parameters. Version upgrades still apply
description-only updates to locally defined parameters.
+ final boolean preserveExistingEntries = syncOptions != null &&
syncOptions.isPreserveExistingParameterContextEntries();
final Map<String, Parameter> parameters = new HashMap<>();
for (final VersionedParameter versionedParameter :
versionedParameterContext.getParameters()) {
- final Optional<Parameter> parameterOption =
currentParameterContext.getParameter(versionedParameter.getName());
- if (parameterOption.isPresent()) {
- final Parameter existingParameter = parameterOption.get();
- if
(!Objects.equals(existingParameter.getDescriptor().getDescription(),
versionedParameter.getDescription())) {
+ final Parameter localParameter =
currentParameterContext.getParameters().get(parameterDescriptor(versionedParameter.getName()));
+ if (localParameter != null) {
+ // Never overwrite an existing local value. Description-only
updates apply on version upgrades,
+ // not KEEP_EXISTING imports, and must be built from the local
raw parameter so aliases are preserved.
+ if (!preserveExistingEntries
+ &&
!Objects.equals(localParameter.getDescriptor().getDescription(),
versionedParameter.getDescription())) {
final Parameter updatedParameter = new Parameter.Builder()
- .fromParameter(existingParameter)
+ .fromParameter(localParameter)
Review Comment:
See the later comment about the tests, I think we should try to update them
to test this scenario with losing the reference
--
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]
