markap14 commented on a change in pull request #5072:
URL: https://github.com/apache/nifi/pull/5072#discussion_r684438347
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/integration/processor/ProcessorParameterTokenIT.java
##########
@@ -73,7 +73,8 @@ public void testProperReferences() throws ExecutionException,
InterruptedExcepti
final ProcessorNode procNode = createProcessorNode(WriteText.class);
procNode.setAutoTerminatedRelationships(Collections.singleton(REL_SUCCESS));
- final ParameterContext parameterContext = new
StandardParameterContext(UUID.randomUUID().toString(),
"testEscapedParameterReference", ParameterReferenceManager.EMPTY, null);
+ final ParameterContext parameterContext = new
StandardParameterContext(UUID.randomUUID().toString(),
"testEscapedParameterReference", ParameterReferenceManager.EMPTY, null
+ );
Review comment:
Were these line wraps intentional? Probably best not to wrap the line
for `);` after all arguments on previous line
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/parameter/StandardParameterReferenceManager.java
##########
@@ -53,7 +53,8 @@ public StandardParameterReferenceManager(final FlowManager
flowManager) {
final ProcessGroup rootGroup = flowManager.getRootGroup();
final String contextId = parameterContext.getIdentifier();
final List<ProcessGroup> referencingGroups =
rootGroup.findAllProcessGroups(
- group -> group.getParameterContext() != null &&
group.getParameterContext().getIdentifier().equals(contextId));
+ group -> group.getParameterContext() != null &&
(group.getParameterContext().getIdentifier().equals(contextId)
Review comment:
The logic here - "is context is not null and (context id is what we want
or context inherits from what we want" - is a bit non-trivial, and is repeated
a bit down below. And I think it will be easy to later forget to check the
"inherits from what we want" part later if we add a similar check elsewhere.
How do you feel about adding a `boolean referencesParameterContext(String
contextId)` or `boolean referencesParameterContext(ParameterContext context)`
?? Feels a little cleaner to me.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/dao/impl/StandardParameterContextDAO.java
##########
@@ -54,18 +58,55 @@ public boolean hasParameterContext(final String
parameterContextId) {
@Override
public void verifyCreate(final ParameterContextDTO parameterContextDto) {
verifyNoNamingConflict(parameterContextDto.getName());
+
verifyInheritedParameterContextRefs(parameterContextDto.getInheritedParameterContexts());
+ }
+
+ private void verifyInheritedParameterContextRefs(final
List<ParameterContextReferenceEntity> inheritedParameterContexts) {
+ if (inheritedParameterContexts != null) {
+ // This will throw an exception if one is not found
+ inheritedParameterContexts.stream().forEach(entity ->
flowManager.getParameterContextManager()
+ .getParameterContext(entity.getComponent().getId()));
+ }
}
@Override
public ParameterContext createParameterContext(final ParameterContextDTO
parameterContextDto) {
final Map<String, Parameter> parameters =
getParameters(parameterContextDto, null);
- final ParameterContext parameterContext =
flowManager.createParameterContext(parameterContextDto.getId(),
parameterContextDto.getName(), parameters);
+
+ resolveInheritedParameterContexts(parameterContextDto);
+
+ final ParameterContext parameterContext =
flowManager.createParameterContext(parameterContextDto.getId(),
parameterContextDto.getName(),
+ parameters,
parameterContextDto.getInheritedParameterContexts());
if (parameterContextDto.getDescription() != null) {
parameterContext.setDescription(parameterContextDto.getDescription());
}
return parameterContext;
}
+ private void resolveInheritedParameterContexts(final ParameterContextDTO
parameterContextDto) {
+ if (parameterContextDto.getInheritedParameterContexts() != null &&
!parameterContextDto.getInheritedParameterContexts().isEmpty()) {
Review comment:
Logic here gets a little simpler here IMO if this if is reversed -
```
final List<ParameterContextReferenceDTO> references =
parameterContextDto.getInheritedParameterContexts();
if (references == null || references.isEmpty()) {
return;
}
```
It avoids having to nest so much logic.
It also tends to convey intent better, IMO. If it's null or empty - do
nothing. Establishes that right upfront.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/dao/impl/StandardParameterContextDAO.java
##########
@@ -54,18 +58,55 @@ public boolean hasParameterContext(final String
parameterContextId) {
@Override
public void verifyCreate(final ParameterContextDTO parameterContextDto) {
verifyNoNamingConflict(parameterContextDto.getName());
+
verifyInheritedParameterContextRefs(parameterContextDto.getInheritedParameterContexts());
+ }
+
+ private void verifyInheritedParameterContextRefs(final
List<ParameterContextReferenceEntity> inheritedParameterContexts) {
+ if (inheritedParameterContexts != null) {
+ // This will throw an exception if one is not found
+ inheritedParameterContexts.stream().forEach(entity ->
flowManager.getParameterContextManager()
+ .getParameterContext(entity.getComponent().getId()));
+ }
}
@Override
public ParameterContext createParameterContext(final ParameterContextDTO
parameterContextDto) {
final Map<String, Parameter> parameters =
getParameters(parameterContextDto, null);
- final ParameterContext parameterContext =
flowManager.createParameterContext(parameterContextDto.getId(),
parameterContextDto.getName(), parameters);
+
+ resolveInheritedParameterContexts(parameterContextDto);
+
+ final ParameterContext parameterContext =
flowManager.createParameterContext(parameterContextDto.getId(),
parameterContextDto.getName(),
+ parameters,
parameterContextDto.getInheritedParameterContexts());
if (parameterContextDto.getDescription() != null) {
parameterContext.setDescription(parameterContextDto.getDescription());
}
return parameterContext;
}
+ private void resolveInheritedParameterContexts(final ParameterContextDTO
parameterContextDto) {
+ if (parameterContextDto.getInheritedParameterContexts() != null &&
!parameterContextDto.getInheritedParameterContexts().isEmpty()) {
+ final Map<String, ParameterContext> paramContextNameMap =
flowManager.getParameterContextManager().getParameterContextNameMapping();
+ for (final ParameterContextReferenceEntity ref :
parameterContextDto.getInheritedParameterContexts()) {
+ if (ref.getComponent() == null || (ref.getComponent().getId()
== null && ref.getComponent().getName() == null)) {
+ throw new IllegalStateException(String.format("Could not
resolve inherited parameter context references in Parameter Context [%s]",
+ parameterContextDto.getName()));
+ }
+ final ParameterContextReferenceDTO refDto = ref.getComponent();
+ // If resolving by name only, look up the ids
+ if (refDto.getId() == null) {
Review comment:
Same argument can be made here -
```
if (refDto.getId() != null) {
continue;
}
```
##########
File path:
nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/parameters/ParameterContextIT.java
##########
@@ -609,8 +636,25 @@ public ParameterEntity createParameterEntity(final String
name, final String des
return getClientUtil().createParameterEntity(name, description,
sensitive, value);
}
+ public ParameterContextEntity createParameterContextEntity(final String
name, final String description, final Set<ParameterEntity> parameters,
+ final
List<ParameterContextEntity> parameterContextRefs) {
+ final List<ParameterContextReferenceEntity> refs = new ArrayList<>();
+ if (parameterContextRefs != null) {
+ refs.addAll(parameterContextRefs.stream().map(pce -> {
+ ParameterContextReferenceEntity ref = new
ParameterContextReferenceEntity();
+ ref.setId(pce.getId());
+ ParameterContextReferenceDTO refDto = new
ParameterContextReferenceDTO();
+ refDto.setId(pce.getComponent().getId());
+ refDto.setName(pce.getComponent().getName());
+ ref.setComponent(refDto);
+ return ref;
+ }).collect(Collectors.toList()));
+ }
+ return getClientUtil().createParameterContextEntity(name, description,
parameters, refs);
+ }
+
public ParameterContextEntity createParameterContextEntity(final String
name, final String description, final Set<ParameterEntity> parameters) {
- return getClientUtil().createParameterContextEntity(name, description,
parameters);
+ return createParameterContextEntity(name, description, parameters,
Collections.EMPTY_LIST);
Review comment:
Should prefer Collections.emptyList() over EMPTY_LIST
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/dao/impl/StandardParameterContextDAO.java
##########
@@ -268,6 +325,14 @@ public void verifyDelete(final String parameterContextId) {
}
}
}
+
+ for (final ParameterContext parameterContext :
flowManager.getParameterContextManager().getParameterContexts()) {
+ if
(parameterContext.getInheritedParameterContexts().stream().filter(pc ->
pc.getIdentifier().equals(parameterContextId))
Review comment:
Might be cleaner to use the following?
```
if (parameterContext.getInheritedParameterContexts().stream().anyMatches(pc
-> pc.getIdentifier().equals(parameterContextId))) {
```
##########
File path:
nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/NiFiClientUtil.java
##########
@@ -173,10 +173,16 @@ public ParameterEntity createParameterEntity(final String
name, final String des
}
public ParameterContextEntity createParameterContextEntity(final String
name, final String description, final Set<ParameterEntity> parameters) {
+ return createParameterContextEntity(name, description, parameters,
Collections.EMPTY_LIST);
Review comment:
emptyList() should be preferred over EMPTY_LIST
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/test/java/org/apache/nifi/parameter/TestStandardParameterContext.java
##########
@@ -214,13 +215,203 @@ public void testChangingParameterForRunningProcessor() {
assertEquals("321", context.getParameter("abc").get().getValue());
}
+ @Test
+ public void testChangingNestedParameterForRunningProcessor() {
+ final String inheritedParamName = "def";
+ final String originalValue = "123";
+ final String changedValue = "321";
+
+ final HashMapParameterReferenceManager referenceManager = new
HashMapParameterReferenceManager();
+ final StandardParameterContextManager parameterContextLookup = new
StandardParameterContextManager();
+ final ParameterContext a = createParameterContext("a",
parameterContextLookup, referenceManager);
+ addParameter(a, "abc", "123");
+
+ final ParameterContext b = createParameterContext("b",
parameterContextLookup, referenceManager);
+ addParameter(b, inheritedParamName, originalValue);
+
+ a.setInheritedParameterContexts(Arrays.asList(b));
+
+ // Structure is now:
+ // Param context A
+ // Param abc
+ // (Inherited) Param def (from B)
+
+ // Processor references param 'def'
+ final ProcessorNode procNode = getProcessorNode(inheritedParamName,
referenceManager);
+
+ // Show that inherited param 'def' starts with the original value from
B
+ Assert.assertEquals(originalValue,
a.getParameter(inheritedParamName).get().getValue());
+
+ // Now demonstrate that we can't effectively add the parameter by
referencing Context B while processor runs
+ a.setInheritedParameterContexts(Collections.EMPTY_LIST); // A now no
longer includes 'def'
Review comment:
`Collections.emptyList()` is preferred over `Collections.EMPTY_LIST`
--
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]
