NIFI-3785: Added feature to move controller services between process …

[Freedom9339]

…groups with the new UI

Summary

NIFI-3785: Added an option on the controller services grid that moves a controller service to a specified process group. The controller service can be moved to the parent process group or a child process group. However, it can only be moved one level at a time. If there are any scope conflicts with referencing components, the move will fail displaying the reason.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

• Apache NiFi Jira issue created

Pull Request Tracking

• Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
• Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

• Pull Request based on current revision of the main branch
• Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

• Build completed using mvn clean install -P contrib-check
  • JDK 21

Licensing

• New dependencies are compatible with the Apache License 2.0 according to the License Policy
• New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

• Documentation formatting appears as expected in rendered files

[Freedom9339]

@markap14 @exceptionfactory I've resubmitted this PR with the changes done to the new UI. The backend is the same as it was in the closed PR(#7734), but the UI changes were done against the new UI.

[Freedom9339]

@markobean

[exceptionfactory]

Thanks for putting in the work to refactor this for the new UI @Freedom9339.

Tagging @mcgilman and @rfellows for potential review.

[mcgilman]

Thanks for the mention @exceptionfactory! I'll have a look...

[mcgilman]

Thanks for the PR @Freedom9339! I've left a number of comments below that mostly appear to be from using the Enable/Disable Service dialog has a starting point. Since that dialog needs to be used for both Services in the Flow and in the Controller Settings there area a number of considerations there that are not needed here.

In addition to the items below, please ensure you run prettier and that this PR isn't introducing any new lint issues (I think there are currently 31 left that we're slowly trying to work through). From nifi/nifi-frontend/src/main/frontend please run:

$ npx nx prettier:format
$ npx nx lint

I will defer to @exceptionfactory or @markap14 for a more thorough review of the back end changes.

[Freedom9339]

@mcgilman Just checking in. Any updates on this?

[mcgilman]

@mcgilman Just checking in. Any updates on this?

Thanks for the ping. Sorry for the delay, I'll try to get some more eyes on this PR this week.

[Freedom9339]

@mcgilman Sorry the pinging again. Just want to make sure this doesn't fall through the cracks.

[mcgilman]

@mcgilman Sorry the pinging again. Just want to make sure this doesn't fall through the cracks.

I just pulled down the latest. After rebasing against current main there were build failures due to changes that have landed after your PR was created. So I tried to run the PR as-is without rebasing and I'm still seeing issues. When running through the dev server it fails to serve with

Application bundle generation failed. [13.461 seconds]

✘ [ERROR] Undefined function.
╷
109 │ $surface-light-palette: mat.define-palette($surface-palette, 600, 100, 900);
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
╵
libs/shared/src/assets/themes/supplemental.scss 109:25 @import
apps/nifi/src/styles.scss 62:9 root stylesheet [plugin angular-sass]

angular:styles/global:styles:1:8:
  1 │ @import 'apps/nifi/src/styles.scss';

I then tried running with the built application but I'm seeing this error in dev tools when attempting to open the MoveControllerService Component.

TypeError: g is not iterable
    at chunk-VIZEOFFB.js:70:66450
    at Array.forEach (<anonymous>)
    at o.loadChildOptions (chunk-VIZEOFFB.js:70:66333)
    at new o (chunk-VIZEOFFB.js:70:65423)
    at o.ɵfac [as factory] (chunk-VIZEOFFB.js:70:67428)
    at hn (chunk-ZKVQE3GD.js:7:23746)
    at TC (chunk-ZKVQE3GD.js:7:63202)
    at mn.create (chunk-ZKVQE3GD.js:7:61795)
    at Vg.createComponent (chunk-ZKVQE3GD.js:7:64954)
    at t.attachComponentPortal (chunk-6XYXU2WR.js:1:110675)

Once the PR is updated I should be able to get some more eyes on it quickly.

[Freedom9339]

@mcgilman I fixed the issue caused by rebasing and pushed. Thank You!

[mcgilman]

Thanks for the rebase! I've left some additional feedback below.

[Freedom9339]

@mcgilman I've completed the change/fixes requested. Thank You!

[Freedom9339]

@mcgilman Sorry, I just realized my latest commit didn't go through. It's there now.

[Freedom9339]

@mcgilman Any update on this? I just did a fresh rebase.

[mcgilman]

Thanks for the updates @Freedom9339! I've left a few more comments below.

[Freedom9339]

@mcgilman Thank you for the review. I've made the requested changes. Thank You.

[mcgilman]

@Freedom9339 Thanks again for the updates! It's looking good. I've left a couple comments and I've noted a number of nit-picky coding style things to better follow some of the conventions currently practiced.

[mcgilman]

Process Group is not a valid value for this field. We could initialize the value to null but we're setting the value below. We could instead determine the value before creating the form and then just use the correct value when creating the FormControl.

[Freedom9339]

Not sure what you mean by this. "Process Group" is what is shown whenever there is no process group selected. The only time this happens is if there are no valid process groups available.

[mcgilman]

The first argument to the FormControl is the value. The string Process Group is not a valid value. What you see in the control when no value is select comes from the mat-label in the mat-form-field. If you want something else than the mat-label value you can specify a placeholder on the mat-select.

[Freedom9339]

I see. I've removed the initial value.

[Freedom9339]

@mcgilman Sorry for the delay. I addressed the issues you mentioned and added more user validation to not expose any unauthorized components. I've tested every scenario I could think of. Please let me know if there is one I missed.

[Freedom9339]

@mcgilman Any update on this? I've just rebased to fix merge conflicts.

[mcgilman]

Thanks for the updates and the mention @Freedom9339! The front end changes are starting to look good. I've noted a couple minor things below. I've also noted a few things on the back end code but we'll want to get a closer look from @markap14 or @exceptionfactory before we sign off.

[mcgilman]

I'm not sure we should be using the warning icon here. When we use this, the user can hover to reveal the issue. However, here the icon cannot be interacted with and instead the warning is revealed as a tooltip on the menu item.

[Freedom9339]

I've removed the warning icon.

[mcgilman]

Both parent and child options are added to the same collection. Do you think it would be helpful to differentiate between the parent Process Group from the child Process Groups in the listing shown to the user. I'm not suggesting we have to do this, just looking for feedback if you think it may be helpful to the user especially when dealing with a large flow.

The options collection is already ordered and should position the parent Process Group first but it'll only be present when the user has permissions.

[Freedom9339]

I've added "(Parent)" to the end of the parent process group and added a divider between the parent and child options.

[mcgilman]

This sort of logic is typically within the service facade. The Resource tier handles request validation, authorization, and request replication. It would defer to the service facade to handle the actual logic of the request.

[Freedom9339]

I've moved all the logic to the service facade.

[mcgilman]

This endpoint needs to be replicated across the cluster. NiFi supports an extensible authorization model. That considered, for endpoints like we the call would be replicated to each node in the cluster. When in the response merger, we need to essentially return the intersection of options that are approved and in common for all nodes in the cluster.

[Freedom9339]

Added endpoint replication for clusters.

[exceptionfactory]

The method implementation still appears to be limited to the current node, as opposed to replicating the request. In addition, I don't see the response merger implemented.

[mcgilman]

Since this action happens with the dialog open and the dialog remains open, we should be using a banner error here. This will show the error in a banner within the dialog.

[Freedom9339]

I've changed this to use the banner instead of a toast.

[mcgilman]

The effects for this action does not dispatch so this return has no effect. You'll want to replace this with this.store.dispatch(...)

[Freedom9339]

Changed this to tap and used store.dispatch. Error show up correctly now.

[mcgilman]

moveControllerService should be added to the reducer above to ensure that the saving flag is set to true.

[Freedom9339]

I've added it to the reducers.

[Freedom9339]

@mcgilman Thank you for the review. I've made the changed requested.

[Freedom9339]

@mcgilman Any new updates on this?

[mcgilman]

@Freedom9339 In my last review I mentioned the UI portions were looking good but I will take a peek at your latest updates. I also provided some initial feedback on some of the back end changes but we'll want some additional eyes on those from someone more familiar with those portions of the code base.

[mcgilman]

Suggested change

	openMoveControllerServiceDialog$ = createEffect(
	() =>
	this.actions$.pipe(
	ofType(ControllerServicesActions.openMoveControllerServiceDialog),
	map((action) => action.request),
	switchMap((request) =>
	from(this.controllerServiceService.getMoveOptions(request.controllerService.id)).pipe(
	map((response: SelectOption[]) => {
	const moveDialogReference = this.dialog.open(MoveControllerService, {
	...LARGE_DIALOG,
	data: {
	controllerService: request.controllerService,
	options: response
	}
	});
	moveDialogReference.componentInstance.goToReferencingComponent = (
	component: ControllerServiceReferencingComponent
	) => {
	const route: string[] = this.getRouteForReference(component);
	this.router.navigate(route);
	};
	moveDialogReference.afterClosed().subscribe((response) => {
	if (response != 'ROUTED') {
	this.store.dispatch(
	ControllerServicesActions.loadControllerServices({
	request: {
	processGroupId: request.controllerService.parentGroupId!
	}
	})
	);
	}
	});
	}),
	tap({
	error: (errorResponse: HttpErrorResponse) => {
	this.dialog.closeAll();
	this.store.dispatch(
	ErrorActions.snackBarError({
	error: this.errorHelper.getErrorString(errorResponse)
	})
	);
	}
	})
	)
	)
	),
	{ dispatch: false }
	);
	openMoveControllerServiceDialog$ = createEffect(
	() =>
	this.actions$.pipe(
	ofType(ControllerServicesActions.openMoveControllerServiceDialog),
	map((action) => action.request),
	switchMap((request) =>
	from(this.controllerServiceService.getMoveOptions(request.controllerService.id)).pipe(
	map((options: SelectOption[]) => {
	return { request, options };
	}),
	tap({
	error: (errorResponse: HttpErrorResponse) => {
	this.store.dispatch(
	ErrorActions.snackBarError({
	error: this.errorHelper.getErrorString(errorResponse)
	})
	);
	}
	})
	)
	),
	tap(({ request, options }) => {
	const moveDialogReference = this.dialog.open(MoveControllerService, {
	...LARGE_DIALOG,
	data: {
	controllerService: request.controllerService,
	options
	}
	});
	moveDialogReference.componentInstance.goToReferencingComponent = (
	component: ControllerServiceReferencingComponent
	) => {
	const route: string[] = this.getRouteForReference(component);
	this.router.navigate(route);
	};
	moveDialogReference.afterClosed().subscribe((response) => {
	if (response != 'ROUTED') {
	this.store.dispatch(
	ControllerServicesActions.loadControllerServices({
	request: {
	processGroupId: request.controllerService.parentGroupId!
	}
	})
	);
	}
	});
	})
	),
	{ dispatch: false }
	);

Here's a suggestion for the effect for querying for the available options prior to opening the dialog.

[Freedom9339]

Applied this suggestion.

[exceptionfactory]

Thanks for the substantive effort on this feature @Freedom9339, and for the significant review work @mcgilman.

Unfortunately, I think the length of time this pull request has remained open, and the remaining issues around the service implementation, raise a concern about this going forward. As evidenced by the amount of changes required thus far, this feature presents a number of complexities related to request replication and permission checking that must be implemented correctly. Part of the challenge relates to the current complexity surrounding fine-grained permissions.

Although this provides helpful background, it seems like it may require a project maintainer to bring it to a state that is ready for merging. Again, this is primarily due to the significant complexity around permissions and clustering, requiring direct development as opposed a more standard pull request feedback cycle. That may mean this is put on hold for a time until there are sufficient cycles to bring it to completion.

[exceptionfactory]

The method implementation still appears to be limited to the current node, as opposed to replicating the request. In addition, I don't see the response merger implemented.

[Freedom9339]

@exceptionfactory I've added the replicating of the move-options end point as well as merging the responses. Please let me know if this is being placed on hold or if there is anything else I need to do.

[exceptionfactory]

@Freedom9339 I raised a few questions and noted several concerns on the REST API and service implementation. As mentioned, this is a complex feature, so I appreciate the effort. However, it is complex enough that it appears to require direct maintainer involvement to move forward. The work here is very helpful background, I'm not in a position to work on it right now, so it seems best to leave and restart down the road.

[exceptionfactory]

Why is the exception ignored?

[exceptionfactory]

This message declaration could be collapsed into a single line.

[exceptionfactory]

This method returns the name or ID of referencing components, but "Conflicting Components" doesn't make that clear, so it seems like the naming is questionable, and it would probably be clearer to return a list of objects.

[exceptionfactory]

This ProcessGroupOptionEntity is very generic, whereas it seems like it should be more targeted to the use case in question.

[exceptionfactory]

This class is missing a license header.

[exceptionfactory]

move-options seems too generic for the purpose of the method. It seems more accurate to name this in relation to Process Groups, such as Destination Process Groups, or Available Process Groups.

[exceptionfactory]

Although the REST API is not completely consistent, the general pattern is to have the path as a resource, which would change the approach to have the Process Group in the path.

[exceptionfactory]

Is there a reason for not checking moving status inside the verifyCanDelete method?

[exceptionfactory]

Adding this temporary state to the Node object raises a number of questions about the implementation, as it does not seem to fit with the other stateful properties.