ncover21 commented on code in PR #11567:
URL: https://github.com/apache/nifi/pull/11567#discussion_r3825535779
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/StandardNiFiServiceFacade.java:
##########
@@ -1810,14 +1810,20 @@ private void setEffectiveParameterUpdates(final
ParameterContextDTO parameterCon
parameterEntity =
dtoFactory.createParameterEntity(parameterContext, parameter, revisionManager,
parameterContextDAO);
}
- // Parameter is inherited if either this is the removal of a
parameter not directly in this context, or it's parameter not specified
directly in the DTO
- final boolean isInherited = (parameter == null &&
!parameterContext.getParameters().containsKey(new
ParameterDescriptor.Builder().name(parameterName).build()))
- || (parameter != null &&
!parameterEntities.containsKey(parameterName));
- parameterEntity.getParameter().setInherited(isInherited);
+
parameterEntity.getParameter().setInherited(isInheritedParameterUpdate(parameter,
parameterContext, parameterName));
parameterContextDto.getParameters().add(parameterEntity);
}
}
+ static boolean isInheritedParameterUpdate(final Parameter parameter, final
ParameterContext parameterContext,
+ final String parameterName) {
+ if (parameter == null) {
+ return !parameterContext.getParameters().containsKey(new
ParameterDescriptor.Builder().name(parameterName).build());
+ }
+
+ return
!parameter.getParameterContextId().equals(parameterContext.getIdentifier());
Review Comment:
I think the diagnosis is right and moving the classification onto the source
context lines up with what
`StandardParameterContext.getEffectiveParameterUpdates` already does.
One concern though. I think this regresses deleting a local parameter that
overrides a same named inherited one.
Walking it with a child context that locally overrides the parent's `shared`:
1. Client submits the deletion, a `ParameterDTO` named `shared` with
description/sensitive/value/referencedAssets all null, so
`StandardParameterContextDAO.getParameters` treats it as a deletion.
2. `getProposedParameters` removes the local copy, then the merge
re-supplies `shared` from the parent, so the effective value doesnt actually
disappear.
3. `Parameter.equals` compares `parameterContextId`, so `shared` always
lands in `proposedParameterUpdates`, carrying the parent's `Parameter` and the
parent's id.
4. `parameterEntities` contains `shared` because the client submitted it, so
the loop reuses the client's original deletion entity and stamps
`inherited=true` onto it.
5. On apply, `getParameters` skips anything with `inherited == true`, so the
map never gets the null value that means "delete this one", and
`updateParameters` only walks the delta map, so `shared` is left untouched.
Net effect is the update request reports success but the local override is
still there. The old predicate returned false here because the name was present
in the incoming DTO, so this case worked before.
I dont think its cluster only either,
`ParameterUpdateManager.performParameterContextUpdate` applies the same mutated
DTO on the standalone path. And it looks reachable from the UI, once an
override is saved `DtoFactory` reports it as `inherited=false`, so `canDelete`
takes the normal path and sends just the name.
There is a second case I'm less sure about but wanted to flag while I'm
here: a same context value reference like `Y = #{X}`. If you update only `X`,
then `Y` lands in the effective updates with its resolved value and a
`parameterContextId` equal to this context, so the new predicate calls it local
and it gets persisted with the resolved literal, or with `********` if `Y` is
sensitive. Before, `Y` was marked inherited because it wasnt in the DTO, so it
was skipped on apply.
What I think might work is keying off what the client actually submitted
rather than the source context
--
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]
