Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2423,26 +2423,37 @@ private void addMissingConfiguration(final VersionedParameterContext versionedPa
return;
}

// NIFI-16318: Never overwrite existing local parameter values or materialize local overrides for inherited parameters.
// Description-only updates apply to locally defined parameters and must be built from the local raw parameter
// so that parameter references (e.g. #{other}) are preserved and not flattened to literal resolved values.
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) {
if (!Objects.equals(localParameter.getDescriptor().getDescription(), versionedParameter.getDescription())) {
final Parameter updatedParameter = new Parameter.Builder()
.fromParameter(existingParameter)
.fromParameter(localParameter)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the later comment about the tests, I think we should try to update them to test this scenario with losing the reference

.description(versionedParameter.getDescription())
.build();
parameters.put(versionedParameter.getName(), updatedParameter);
}
continue;
}

final Parameter inheritedParameter = currentParameterContext.getRawEffectiveParameters().get(parameterDescriptor(versionedParameter.getName()));
if (inheritedParameter != null) {
// The parameter is provided by inheritance. Do not materialize a local override from the snapshot,
// which would shadow the inherited value with the registry value.
continue;
}

final Parameter parameter = createParameter(currentParameterContext.getIdentifier(), versionedParameter, versionedParameterContext.getParameterProvider() != null);
parameters.put(versionedParameter.getName(), parameter);
}

currentParameterContext.setParameters(parameters);
if (!parameters.isEmpty()) {
currentParameterContext.setParameters(parameters);
}

if (!Objects.equals(currentParameterContext.getDescription(), versionedParameterContext.getDescription())) {
currentParameterContext.setDescription(versionedParameterContext.getDescription());
Expand Down Expand Up @@ -2488,6 +2499,10 @@ private void addMissingConfiguration(final VersionedParameterContext versionedPa
}
}

private static ParameterDescriptor parameterDescriptor(final String name) {
return new ParameterDescriptor.Builder().name(name).build();
}

private Parameter createParameter(final String contextId, final VersionedParameter versionedParameter, final boolean providerBacked) {
final List<VersionedAsset> referencedAssets = versionedParameter.getReferencedAssets();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1918,6 +1918,167 @@ public void testParameterContextDescriptionUpdatedDuringProcessGroupSync() throw
assertEquals(UPDATED_CONTEXT_DESCRIPTION, paramContext.getDescription());
}

@Test
public void testExistingLocalParameterValuePreservedWhenDescriptionUpdated() 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, synchronizationOptions);

assertEquals(VALUE_XYZ, paramContext.getParameter(PARAM_ABC).get().getValue(),
"Existing parameter value must not be overwritten by versioned flow value");
assertEquals(UPDATED_DESCRIPTION_MAP.get(PARAM_ABC), paramContext.getParameter(PARAM_ABC).get().getDescriptor().getDescription(),
"Parameter description should be updated from versioned flow");
assertEquals(UPDATED_CONTEXT_DESCRIPTION, paramContext.getDescription(),
"Parameter context description should be updated from versioned flow");
}

@Test
public void testInheritedParameterNotMaterializedAsLocalOverrideWhenDescriptionDiffers()
throws FlowSynchronizationException, InterruptedException, TimeoutException {
final VersionedParameterContext versionedParent = createVersionedParameterContextWithDescriptions("P2",
Map.of("paramA", "prod-value"), Map.of("paramA", "parent-description"), Collections.emptySet());
synchronizer.synchronize(null, versionedParent, synchronizationOptions);
final ParameterContext parent = parameterContextManager.getParameterContextNameMapping().get("P2");

final VersionedParameterContext versionedChild = createVersionedParameterContext("P1", Map.of("paramOwn", "ownValue"), Collections.emptySet());
synchronizer.synchronize(null, versionedChild, synchronizationOptions);
final ParameterContext child = parameterContextManager.getParameterContextNameMapping().get("P1");
child.setInheritedParameterContexts(List.of(parent));

assertFalse(child.getParameters().containsKey(new ParameterDescriptor.Builder().name("paramA").build()),
"paramA should be inherited, not local, before sync");
assertEquals("prod-value", child.getParameter("paramA").get().getValue());
assertEquals("parent-description", child.getParameter("paramA").get().getDescriptor().getDescription());

final ProcessGroup processGroup = createMockProcessGroup();
when(processGroup.getParameterContext()).thenReturn(child);

// Versioned flow has child and parent contexts, with paramA having a dev value and a different description
final VersionedParameterContext proposedParent = createVersionedParameterContextWithDescriptions("P2",
Map.of("paramA", "dev-value"), Map.of("paramA", "dev-description"), Collections.emptySet());
final VersionedParameterContext proposedChild = createVersionedParameterContextWithDescriptions("P1",
Map.of("paramOwn", "ownValue", "paramA", "dev-value"), Map.of("paramA", "dev-description"), Collections.emptySet());
proposedChild.setInheritedParameterContexts(List.of("P2"));

final VersionedProcessGroup rootGroup = new VersionedProcessGroup();
rootGroup.setIdentifier(processGroup.getIdentifier());
rootGroup.setParameterContextName("P1");

final VersionedExternalFlow externalFlow = new VersionedExternalFlow();
externalFlow.setFlowContents(rootGroup);
externalFlow.setParameterContexts(Map.of("P1", proposedChild, "P2", proposedParent));

synchronizer.synchronize(processGroup, externalFlow, synchronizationOptions);

assertFalse(child.getParameters().containsKey(new ParameterDescriptor.Builder().name("paramA").build()),
"Synchronization must not create a local override on child for an inherited parameter even when descriptions differ");
assertEquals("prod-value", child.getParameter("paramA").get().getValue());
assertEquals("prod-value", parent.getParameter("paramA").get().getValue(),
"Synchronization must not overwrite the inherited parameter's value on the parent context");
assertEquals("dev-description", parent.getParameter("paramA").get().getDescriptor().getDescription(),
"Parent context parameter description should be updated");
}

@Test
public void testParameterReferencePreservedWhenDescriptionUpdated()
throws FlowSynchronizationException, InterruptedException, TimeoutException {
// Parent context defines targetParam
final VersionedParameterContext versionedParent = createVersionedParameterContext("ParentContext",
Map.of("targetParam", "targetValue"), Collections.emptySet());
synchronizer.synchronize(null, versionedParent, synchronizationOptions);
final ParameterContext parentContext = parameterContextManager.getParameterContextNameMapping().get("ParentContext");

// Child context inherits ParentContext and defines aliasParam referencing #{targetParam}
final VersionedParameterContext versionedChild = createVersionedParameterContextWithDescriptions("ChildContext",
Map.of("aliasParam", "#{targetParam}"),
Map.of("aliasParam", "old alias description"),
Collections.emptySet());
synchronizer.synchronize(null, versionedChild, synchronizationOptions);
final ParameterContext childContext = parameterContextManager.getParameterContextNameMapping().get("ChildContext");
childContext.setInheritedParameterContexts(List.of(parentContext));

// Effective value resolves to targetValue, but raw local value is #{targetParam}
assertEquals("targetValue", childContext.getParameter("aliasParam").get().getValue());
assertEquals("#{targetParam}", childContext.getParameters().get(new ParameterDescriptor.Builder().name("aliasParam").build()).getValue());

final ProcessGroup processGroup = createMockProcessGroup();
when(processGroup.getParameterContext()).thenReturn(childContext);

// Versioned flow has aliasParam with a new description and literal value
final VersionedParameterContext proposedChild = createVersionedParameterContextWithDescriptions("ChildContext",
Map.of("aliasParam", "devLiteral"),
Map.of("aliasParam", "new alias description"),
Collections.emptySet());
proposedChild.setInheritedParameterContexts(List.of("ParentContext"));

final VersionedProcessGroup rootGroup = new VersionedProcessGroup();
rootGroup.setIdentifier(processGroup.getIdentifier());
rootGroup.setParameterContextName("ChildContext");

final VersionedExternalFlow externalFlow = new VersionedExternalFlow();
externalFlow.setFlowContents(rootGroup);
externalFlow.setParameterContexts(Map.of("ChildContext", proposedChild, "ParentContext", versionedParent));

synchronizer.synchronize(processGroup, externalFlow, synchronizationOptions);

assertEquals("new alias description", childContext.getParameter("aliasParam").get().getDescriptor().getDescription(),
"Parameter description should be updated from versioned flow");
assertEquals("#{targetParam}", childContext.getParameters().get(new ParameterDescriptor.Builder().name("aliasParam").build()).getValue(),
"Raw parameter value must preserve the '#{targetParam}' reference syntax and not flatten to a resolved literal");
assertEquals("targetValue", childContext.getParameter("aliasParam").get().getValue(),
"Effective parameter value must still resolve through the reference");
}

@Test
public void testMissingParameterStillAddedWhenPreserveExistingEntries() throws FlowSynchronizationException, InterruptedException, TimeoutException {
final VersionedParameterContext versionedContext = createVersionedParameterContext(CONTEXT_NAME_PARAMS, SINGLE_PARAMETER, Collections.emptySet());
synchronizer.synchronize(null, versionedContext, synchronizationOptions);

final ParameterContext paramContext = parameterContextManager.getParameterContextNameMapping().get(CONTEXT_NAME_PARAMS);

final ProcessGroup processGroup = createMockProcessGroup();
when(processGroup.getParameterContext()).thenReturn(paramContext);

final VersionedParameterContext proposedParams = createVersionedParameterContext(CONTEXT_NAME_PARAMS,
Map.of(PARAM_ABC, VALUE_123, "paramNew", "new-value"), Collections.emptySet());

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, synchronizationOptions);

assertEquals(VALUE_XYZ, paramContext.getParameter(PARAM_ABC).get().getValue(),
"Existing parameter value must be preserved");
assertTrue(paramContext.getParameter("paramNew").isPresent(), "Missing parameters should still be added");
assertEquals("new-value", paramContext.getParameter("paramNew").get().getValue());
}

@Test
public void testParameterDescriptionUnchangedWhenValueSame() throws FlowSynchronizationException, InterruptedException, TimeoutException {
final VersionedParameterContext versionedContext = createVersionedParameterContextWithDescriptions(CONTEXT_NAME_1,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ <h2 mat-dialog-title>Import From Registry</h2>
class="fa fa-info-circle primary-color"
nifiTooltip
[tooltipComponentType]="TextTip"
tooltipInputData="When not selected, only directly associated Parameter Contexts will be copied, inherited Contexts with no direct assignment to a Process Group are ignored."></i>
tooltipInputData="When selected, Process Groups bind to existing Parameter Contexts with the same name. Existing parameter values and inheritance are left unchanged; only parameters that do not already exist (locally or via inheritance) are added. When not selected, only directly associated Parameter Contexts will be copied; inherited Contexts with no direct assignment to a Process Group are ignored."></i>
</mat-checkbox>
</div>
<div class="flex flex-col mb-5">
Expand Down
Loading
Loading