mcgilman commented on code in PR #8965: URL: https://github.com/apache/nifi/pull/8965#discussion_r1960763375
########## nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/flow-designer/ui/controller-service/move-controller-service/move-controller-service.component.html: ########## @@ -0,0 +1,87 @@ +<!-- + ~ Licensed to the Apache Software Foundation (ASF) under one or more + ~ contributor license agreements. See the NOTICE file distributed with + ~ this work for additional information regarding copyright ownership. + ~ The ASF licenses this file to You under the Apache License, Version 2.0 + ~ (the "License"); you may not use this file except in compliance with + ~ the License. You may obtain a copy of the License at + ~ + ~ http://www.apache.org/licenses/LICENSE-2.0 + ~ + ~ Unless required by applicable law or agreed to in writing, software + ~ distributed under the License is distributed on an "AS IS" BASIS, + ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + ~ See the License for the specific language governing permissions and + ~ limitations under the License. + --> + +<h2 mat-dialog-title>Move Controller Service</h2> +<form class="controller-service-move-form" [formGroup]="moveControllerServiceForm"> + <mat-dialog-content> + <div class="py-4 flex gap-x-3"> + <div class="flex basis-1/2 flex-col gap-y-4 pr-4 overflow-hidden"> + <div> + <div>Service</div> + <div class="tertiary-color font-medium truncate" [title]="controllerService.component.name"> + {{ controllerService.component.name }} + </div> + </div> + <div> + <mat-form-field> + <mat-label>Process Group</mat-label> + <mat-select formControlName="processGroups"> + @for (option of processGroupOptions; track option) { + <div + nifiTooltip + [tooltipDisabled]="!option.disabled" + [tooltipComponentType]="TextTip" + [tooltipInputData]="option.description" + [delayClose]="false"> + <mat-option [value]="option.value" [disabled]="option.disabled"> + <div + *ngIf="option.description != undefined; else valid" + class="pointer fa fa-warning has-errors caution-color" + style="padding: 3px"></div> Review Comment: 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. ########## nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ControllerServiceResource.java: ########## @@ -664,6 +668,206 @@ public Response updateControllerService( ); } + @GET + @Consumes(MediaType.WILDCARD) + @Produces(MediaType.APPLICATION_JSON) + @Path("{id}/move-options") + @Operation( + summary = "Gets the move options for a controller service", + responses = { + @ApiResponse(content = @Content(schema = @Schema(implementation = ProcessGroupOptionEntity.class))), + @ApiResponse(responseCode = "400", description = "NiFi was unable to complete the request because it was invalid. The request should not be retried without modification."), + @ApiResponse(responseCode = "401", description = "Client could not be authenticated."), + @ApiResponse(responseCode = "403", description = "Client is not authorized to make this request."), + @ApiResponse(responseCode = "404", description = "The specified resource could not be found."), + @ApiResponse(responseCode = "409", description = "The request was valid but NiFi was not in the appropriate state to process it.") + }, + security = { + @SecurityRequirement(name = "Read - /flow") + } + ) + public Response getProcessGroupOptions( + @Parameter(description = "The controller service id.") + @PathParam("id") final String controllerServiceId) { + + if (controllerServiceId == null) { + throw new IllegalArgumentException("Controller service id must be specified."); + } + + List<ProcessGroupOptionEntity> options = new ArrayList<>(); + serviceFacade.authorizeAccess(lookup -> { + final NiFiUser user = NiFiUserUtils.getNiFiUser(); + + final ComponentAuthorizable authorizableControllerService = lookup.getControllerService(controllerServiceId); + boolean authorized = authorizableControllerService.getAuthorizable().isAuthorized(authorizer, RequestAction.WRITE, user); + + final ControllerServiceDTO controllerServiceDTO = serviceFacade.getControllerService(controllerServiceId, true).getComponent(); + + final ProcessGroupAuthorizable authorizableProcessGroupCurrent = lookup.getProcessGroup(controllerServiceDTO.getParentGroupId()); + authorized = authorized && authorizableProcessGroupCurrent.getAuthorizable().isAuthorized(authorizer, RequestAction.WRITE, user); + + if (authorized) { + if (authorizableProcessGroupCurrent.getProcessGroup().getParent() != null) { + final ProcessGroupAuthorizable authorizableProcessGroupParent = lookup.getProcessGroup(authorizableProcessGroupCurrent.getProcessGroup().getParent().getIdentifier()); + if (authorizableProcessGroupParent.getAuthorizable().isAuthorized(authorizer, RequestAction.READ, user) + && authorizableProcessGroupParent.getAuthorizable().isAuthorized(authorizer, RequestAction.WRITE, user)) { + options.add(generateProcessGroupOption(controllerServiceDTO, authorizableProcessGroupParent, lookup, user)); + } + } + + authorizableProcessGroupCurrent.getProcessGroup().getProcessGroups().forEach(processGroup -> { + final ProcessGroupAuthorizable authorizableProcessGroup = lookup.getProcessGroup(processGroup.getIdentifier()); + if (authorizableProcessGroup.getAuthorizable().isAuthorized(authorizer, RequestAction.READ, user) + && authorizableProcessGroup.getAuthorizable().isAuthorized(authorizer, RequestAction.WRITE, user)) { + options.add(generateProcessGroupOption(controllerServiceDTO, authorizableProcessGroup, lookup, user)); + } + }); + } + }); + + return generateOkResponse(options).build(); + } + + private ProcessGroupOptionEntity generateProcessGroupOption(ControllerServiceDTO controllerServiceDTO, ProcessGroupAuthorizable processGroup, AuthorizableLookup lookup, NiFiUser user) { Review Comment: 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. ########## nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ControllerServiceResource.java: ########## @@ -664,6 +668,206 @@ public Response updateControllerService( ); } + @GET + @Consumes(MediaType.WILDCARD) + @Produces(MediaType.APPLICATION_JSON) + @Path("{id}/move-options") + @Operation( + summary = "Gets the move options for a controller service", + responses = { + @ApiResponse(content = @Content(schema = @Schema(implementation = ProcessGroupOptionEntity.class))), + @ApiResponse(responseCode = "400", description = "NiFi was unable to complete the request because it was invalid. The request should not be retried without modification."), + @ApiResponse(responseCode = "401", description = "Client could not be authenticated."), + @ApiResponse(responseCode = "403", description = "Client is not authorized to make this request."), + @ApiResponse(responseCode = "404", description = "The specified resource could not be found."), + @ApiResponse(responseCode = "409", description = "The request was valid but NiFi was not in the appropriate state to process it.") + }, + security = { + @SecurityRequirement(name = "Read - /flow") + } + ) + public Response getProcessGroupOptions( + @Parameter(description = "The controller service id.") + @PathParam("id") final String controllerServiceId) { + + if (controllerServiceId == null) { + throw new IllegalArgumentException("Controller service id must be specified."); + } + + List<ProcessGroupOptionEntity> options = new ArrayList<>(); + serviceFacade.authorizeAccess(lookup -> { + final NiFiUser user = NiFiUserUtils.getNiFiUser(); + + final ComponentAuthorizable authorizableControllerService = lookup.getControllerService(controllerServiceId); + boolean authorized = authorizableControllerService.getAuthorizable().isAuthorized(authorizer, RequestAction.WRITE, user); + + final ControllerServiceDTO controllerServiceDTO = serviceFacade.getControllerService(controllerServiceId, true).getComponent(); + + final ProcessGroupAuthorizable authorizableProcessGroupCurrent = lookup.getProcessGroup(controllerServiceDTO.getParentGroupId()); + authorized = authorized && authorizableProcessGroupCurrent.getAuthorizable().isAuthorized(authorizer, RequestAction.WRITE, user); + + if (authorized) { + if (authorizableProcessGroupCurrent.getProcessGroup().getParent() != null) { + final ProcessGroupAuthorizable authorizableProcessGroupParent = lookup.getProcessGroup(authorizableProcessGroupCurrent.getProcessGroup().getParent().getIdentifier()); + if (authorizableProcessGroupParent.getAuthorizable().isAuthorized(authorizer, RequestAction.READ, user) + && authorizableProcessGroupParent.getAuthorizable().isAuthorized(authorizer, RequestAction.WRITE, user)) { + options.add(generateProcessGroupOption(controllerServiceDTO, authorizableProcessGroupParent, lookup, user)); + } + } + + authorizableProcessGroupCurrent.getProcessGroup().getProcessGroups().forEach(processGroup -> { + final ProcessGroupAuthorizable authorizableProcessGroup = lookup.getProcessGroup(processGroup.getIdentifier()); + if (authorizableProcessGroup.getAuthorizable().isAuthorized(authorizer, RequestAction.READ, user) + && authorizableProcessGroup.getAuthorizable().isAuthorized(authorizer, RequestAction.WRITE, user)) { + options.add(generateProcessGroupOption(controllerServiceDTO, authorizableProcessGroup, lookup, user)); + } + }); Review Comment: 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. ########## nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts: ########## @@ -658,6 +659,86 @@ export class ControllerServicesEffects { { 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((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! + } + }) + ); + } + }); + }), + catchError((errorResponse: HttpErrorResponse) => { + this.dialog.closeAll(); + return of( + ErrorActions.snackBarError({ error: this.errorHelper.getErrorString(errorResponse) }) + ); Review Comment: The effects for this action does not dispatch so this `return` has no effect. You'll want to replace this with `this.store.dispatch(...)` ########## nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.reducer.ts: ########## @@ -99,6 +100,17 @@ export const controllerServicesReducer = createReducer( draftState.saving = false; }); }), + on(moveControllerServiceSuccess, (state, { response }) => { Review Comment: `moveControllerService` should be added to the reducer above to ensure that the `saving` flag is set to `true`. ########## nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ControllerServiceResource.java: ########## @@ -664,6 +668,206 @@ public Response updateControllerService( ); } + @GET + @Consumes(MediaType.WILDCARD) + @Produces(MediaType.APPLICATION_JSON) + @Path("{id}/move-options") + @Operation( + summary = "Gets the move options for a controller service", + responses = { + @ApiResponse(content = @Content(schema = @Schema(implementation = ProcessGroupOptionEntity.class))), + @ApiResponse(responseCode = "400", description = "NiFi was unable to complete the request because it was invalid. The request should not be retried without modification."), + @ApiResponse(responseCode = "401", description = "Client could not be authenticated."), + @ApiResponse(responseCode = "403", description = "Client is not authorized to make this request."), + @ApiResponse(responseCode = "404", description = "The specified resource could not be found."), + @ApiResponse(responseCode = "409", description = "The request was valid but NiFi was not in the appropriate state to process it.") + }, + security = { + @SecurityRequirement(name = "Read - /flow") + } + ) + public Response getProcessGroupOptions( Review Comment: 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. ########## nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts: ########## @@ -658,6 +659,86 @@ export class ControllerServicesEffects { { 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((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! + } + }) + ); + } + }); + }), + catchError((errorResponse: HttpErrorResponse) => { + this.dialog.closeAll(); + return of( + ErrorActions.snackBarError({ error: this.errorHelper.getErrorString(errorResponse) }) + ); + }) + ) + ) + ), + { dispatch: false } + ); + + moveControllerService$ = createEffect(() => + this.actions$.pipe( + ofType(ControllerServicesActions.moveControllerService), + map((action) => action.request), + switchMap((request) => + from(this.controllerServiceService.moveControllerService(request)).pipe( + map((response) => + ControllerServicesActions.moveControllerServiceSuccess({ + response: { + controllerService: response + } + }) + ), + catchError((errorResponse: HttpErrorResponse) => + of(ErrorActions.snackBarError({ error: this.errorHelper.getErrorString(errorResponse) })) Review Comment: 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. -- 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]
