You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "mcgilman (via GitHub)" <gi...@apache.org> on 2023/06/08 18:09:33 UTC
[GitHub] [nifi] mcgilman commented on a diff in pull request #7353: NIFI-11658 Streamline using single parameter context for nested PGs
mcgilman commented on code in PR #7353:
URL: https://github.com/apache/nifi/pull/7353#discussion_r1223267396
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/new-process-group-dialog.jsp:
##########
@@ -28,6 +28,12 @@
</div>
<input id="new-process-group-name" type="text" placeholder="Enter a name or select a file to upload"/>
</div>
+ </div>
+ <div class="setting">
+ <div class="setting-name">Parameter Context</div>
Review Comment:
The changes to the new Process Group Dialog should warrant increasing the vertical height.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/process-group-configuration.jsp:
##########
@@ -109,6 +100,34 @@
<div id="process-group-controller-services-tab-content" class="configuration-tab">
<div id="process-group-controller-services-table" class="settings-table"></div>
</div>
+ <div id="process-group-parameter-contexts-tab-content" class="configuration-tab">
Review Comment:
What was the motivation for moving this configuration into a new tab?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/header/components/nf-ng-group-component.js:
##########
@@ -127,6 +130,67 @@
return filepath.replace(/^.*[\\\/]/, '').replace(/\..*/, '');
};
+ /**
+ * parameter context
+ */
+ var parameterContextOptions = [];
+ var noParameterContext = {
+ text: 'No parameter context',
+ value: null
+ };
+ var selectedOption = noParameterContext;
+ var parentParameterContextUrl = '../nifi-api/flow/process-groups/' + encodeURIComponent(nfCanvasUtils.getGroupId());
Review Comment:
I think there is a bug here. I think this code is evaluated once when this loads. And current Process Group Id will be different based on when and where a new Process Group is being added. We should determine this URL when the Process Group is being added. I think we should limit variables here to things that are constants and do not change based on the current context.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -123,6 +175,14 @@
dataType: 'json',
contentType: 'application/json'
}).done(function (response) {
+ // check parameter context recursive update requirement
+ if ($('#parameter-contexts-recursive').hasClass('checkbox-checked')) {
+ var parameterContextId = $('#process-group-parameter-context-combo').combo('getSelectedOption').value;
+ allDescendantProcessGroups.forEach(function (processGroup) {
+ updateParameterContext(processGroup.revision.version, processGroup.id, parameterContextId);
Review Comment:
What happens if one of these calls fails? Is there a reason why the `recursive` flag isn't passed into the call update the current Process Group?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/header/components/nf-ng-group-component.js:
##########
@@ -349,11 +416,15 @@
}
});
+ getParameterContextOptions();
Review Comment:
This should return a `deferred` with the options so we don't execute the following code until it completes.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/header/components/nf-ng-group-component.js:
##########
@@ -127,6 +130,67 @@
return filepath.replace(/^.*[\\\/]/, '').replace(/\..*/, '');
};
+ /**
+ * parameter context
+ */
+ var parameterContextOptions = [];
Review Comment:
I would also suggest we move this variable out of this scope and instead return it from `getParameterContextOptions`. The function could return a `deferred` which provides the populated array.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -123,6 +175,14 @@
dataType: 'json',
contentType: 'application/json'
}).done(function (response) {
+ // check parameter context recursive update requirement
+ if ($('#parameter-contexts-recursive').hasClass('checkbox-checked')) {
+ var parameterContextId = $('#process-group-parameter-context-combo').combo('getSelectedOption').value;
+ allDescendantProcessGroups.forEach(function (processGroup) {
+ updateParameterContext(processGroup.revision.version, processGroup.id, parameterContextId);
Review Comment:
Applying the Parameter Context change recursively does not work when the user lacks permission to any Parameter Contexts referenced by any descendent Process Groups. Currently, the UI reports that everything was saved successfully. But you can see there are requests failing with 403.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -513,6 +552,20 @@
// initialize the parameter context combo
$('#process-group-parameter-context-combo').combo('destroy').combo(comboOptions);
+
+ // initialize parameter context recursive checkbox
+ getAllDescendantProcessGroup(groupId).then(function (response) {
Review Comment:
Making this call when the user does not have permissions to the current Process Group, results in an error in the UI. Currently, we should be handling this without a user-facing error because the user may lack permissions for the current PG but have explicit permissions for a Controller Service in the listing.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -85,6 +85,58 @@
return $('#process-group-controller-services-table');
};
+ /**
+ *
+ * Gets all process groups of a process group
+ * @param groupId
+ * @returns process groups array
+ **/
+ var allDescendantProcessGroups;
Review Comment:
It would be preferable if this was retrieved when needed or passed in as an argument rather then storing it here.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -123,6 +175,14 @@
dataType: 'json',
contentType: 'application/json'
}).done(function (response) {
+ // check parameter context recursive update requirement
+ if ($('#parameter-contexts-recursive').hasClass('checkbox-checked')) {
+ var parameterContextId = $('#process-group-parameter-context-combo').combo('getSelectedOption').value;
+ allDescendantProcessGroups.forEach(function (processGroup) {
+ updateParameterContext(processGroup.revision.version, processGroup.id, parameterContextId);
Review Comment:
Applying the Parameter Context change recursively does not work when the user lacks permissions to any descendent Process Group. The UI here also reported success but there was a failed request with 500. The corresponding stack trace was:
```
15 java.lang.NullPointerException: null
16 at org.apache.nifi.web.api.ProcessGroupResource.lambda$updateProcessGroup$4(ProcessGroupResource.java:561)
17 at org.apache.nifi.web.StandardNiFiServiceFacade.authorizeAccess(StandardNiFiServiceFacade.java:468)
18 at org.apache.nifi.web.StandardNiFiServiceFacade$$FastClassBySpringCGLIB$$358780e0.invoke(<generated>)
19 at org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:218)
```
--
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