You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/08/06 19:13:37 UTC
[GitHub] [nifi] markap14 commented on a change in pull request #5072: NIFI-8490: Adding inherited parameter contexts
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org