From d2b8e6e461414a49ddda09d1f9f100fa168455e0 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Mon, 28 Apr 2025 11:27:59 -0400 Subject: [PATCH] address review comments Signed-off-by: Kashish Mittal --- .../scaffolder-backend/report-alpha.api.md | 3 +- plugins/scaffolder-backend/report.api.md | 4 --- .../tasks/DatabaseTaskStore.test.ts | 2 +- .../src/scaffolder/tasks/DatabaseTaskStore.ts | 32 ++++--------------- .../src/scaffolder/tasks/StorageTaskBroker.ts | 7 ---- .../src/scaffolder/tasks/types.ts | 2 -- .../src/service/permissions.ts | 5 ++- .../scaffolder-backend/src/service/router.ts | 2 +- .../src/service/rules.test.ts | 4 +-- .../scaffolder-backend/src/service/rules.ts | 6 ++-- plugins/scaffolder-node/report.api.md | 6 ++-- plugins/scaffolder-node/src/tasks/types.ts | 6 ++-- .../OngoingTask/OngoingTask.test.tsx | 10 ------ .../components/OngoingTask/OngoingTask.tsx | 2 +- 14 files changed, 21 insertions(+), 70 deletions(-) diff --git a/plugins/scaffolder-backend/report-alpha.api.md b/plugins/scaffolder-backend/report-alpha.api.md index bf2dd186a9..6c64bfa848 100644 --- a/plugins/scaffolder-backend/report-alpha.api.md +++ b/plugins/scaffolder-backend/report-alpha.api.md @@ -11,7 +11,6 @@ import { PermissionCriteria } from '@backstage/plugin-permission-common'; import { PermissionRule } from '@backstage/plugin-permission-node'; import { ResourcePermission } from '@backstage/plugin-permission-common'; import { SerializedTask } from '@backstage/plugin-scaffolder-node'; -import { TaskFilter } from '@backstage/plugin-scaffolder-node'; import { TemplateEntityStepV1beta3 } from '@backstage/plugin-scaffolder-common'; import { TemplateParametersV1beta3 } from '@backstage/plugin-scaffolder-common'; @@ -89,7 +88,7 @@ export const scaffolderTaskConditions: Conditions<{ hasCreatedBy: PermissionRule< SerializedTask, { - property: TaskFilter['property']; + key: string; values: any; }, 'scaffolder-task', diff --git a/plugins/scaffolder-backend/report.api.md b/plugins/scaffolder-backend/report.api.md index cc7d83c7e1..e22a7d1370 100644 --- a/plugins/scaffolder-backend/report.api.md +++ b/plugins/scaffolder-backend/report.api.md @@ -329,8 +329,6 @@ export class DatabaseTaskStore implements TaskStore { // (undocumented) getTask(taskId: string): Promise; // (undocumented) - getTasks(taskIds: string[]): Promise; - // (undocumented) getTaskState({ taskId }: { taskId: string }): Promise< | { state: JsonObject; @@ -485,8 +483,6 @@ export interface TaskStore { // (undocumented) getTask(taskId: string): Promise; // (undocumented) - getTasks(taskIds: string[]): Promise; - // (undocumented) getTaskState?({ taskId }: { taskId: string }): Promise< | { state: JsonObject; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts index 28fdd5cb4e..9c1ed35b27 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts @@ -263,7 +263,7 @@ describe('DatabaseTaskStore', () => { const permissionFilters: PermissionCriteria = { not: { - property: 'createdBy', + key: 'created_by', values: ['user:default/three', 'user:default/four'], }, }; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts index 6a375b974b..f07d827f45 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts @@ -205,7 +205,7 @@ export class DatabaseTaskStore implements TaskStore { } private isTaskFilter(filter: any): filter is TaskFilter { - return filter.hasOwnProperty('property'); + return filter.hasOwnProperty('key'); } private parseFilter( @@ -214,20 +214,16 @@ export class DatabaseTaskStore implements TaskStore { db: Knex, negate: boolean = false, ): Knex.QueryBuilder { - // handle not criteria if (isNotCriteria(filter)) { return this.parseFilter(filter.not, query, db, !negate); } if (this.isTaskFilter(filter)) { const values: string[] = compact(filter.values) ?? []; - - if (filter.property === 'createdBy') { - if (negate) { - query.whereNotIn('created_by', [...new Set(values)]); - } else { - query.whereIn('created_by', [...new Set(values)]); - } + if (negate) { + query.whereNotIn(filter.key, values); + } else { + query.whereIn(filter.key, values); } return query; @@ -279,7 +275,7 @@ export class DatabaseTaskStore implements TaskStore { createdByValues.length > 0 ? { allOf: [ - { property: 'createdBy', values: createdByValues }, + { key: 'created_by', values: createdByValues }, ...(permissionFilters ? [permissionFilters] : []), ], } @@ -347,22 +343,6 @@ export class DatabaseTaskStore implements TaskStore { } } - async getTasks(taskIds: string[]): Promise { - const results = await this.db('tasks') - .whereIn('id', taskIds) - .select(); - - return results.map(result => { - try { - return this.parseTaskRow(result); - } catch (error) { - throw new Error( - `Failed to parse spec of task '${result.id}', ${error}`, - ); - } - }); - } - private parseTaskRow(result: RawDbTaskRow): SerializedTask { const spec = JSON.parse(result.spec); const secrets = result.secrets ? JSON.parse(result.secrets) : undefined; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts index 4d3c9414aa..9e154a6dfc 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts @@ -403,13 +403,6 @@ export class StorageTaskBroker implements TaskBroker { return this.storage.getTask(taskId); } - /** - * {@inheritdoc TaskBroker.getTasks} - */ - getTasks(taskIds: string[]): Promise { - return this.storage.getTasks(taskIds); - } - /** * {@inheritdoc TaskBroker.event$} */ diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts index f9d10ddb0f..62ffb171ff 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts @@ -110,8 +110,6 @@ export interface TaskStore { getTask(taskId: string): Promise; - getTasks(taskIds: string[]): Promise; - claimTask(): Promise; completeTask(options: { diff --git a/plugins/scaffolder-backend/src/service/permissions.ts b/plugins/scaffolder-backend/src/service/permissions.ts index cd10cabde0..e31e627b03 100644 --- a/plugins/scaffolder-backend/src/service/permissions.ts +++ b/plugins/scaffolder-backend/src/service/permissions.ts @@ -27,7 +27,6 @@ import { import { PermissionRuleParams } from '@backstage/plugin-permission-common'; import { SerializedTask, - TaskFilter, } from '@backstage/plugin-scaffolder-node'; /** @@ -82,8 +81,8 @@ export type TaskPermissionRuleInput< > = PermissionRule< SerializedTask, { - property: TaskFilter['property']; - values: any; + key: string; + values?: string[]; }, typeof RESOURCE_TYPE_SCAFFOLDER_TASK, TParams diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index ba476ae25f..21e4c8b688 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -136,6 +136,7 @@ import { } from '@backstage/plugin-scaffolder-node'; /** + * RouterOptions */ export interface RouterOptions { @@ -366,7 +367,6 @@ export async function createRouter( resourceType: RESOURCE_TYPE_SCAFFOLDER_TASK, permissions: scaffolderTaskPermissions, rules: taskRules, - getResources: resourceRefs => taskBroker.getTasks(resourceRefs), }, ], permissions: scaffolderPermissions, diff --git a/plugins/scaffolder-backend/src/service/rules.test.ts b/plugins/scaffolder-backend/src/service/rules.test.ts index 68b2930e32..92d3718496 100644 --- a/plugins/scaffolder-backend/src/service/rules.test.ts +++ b/plugins/scaffolder-backend/src/service/rules.test.ts @@ -586,7 +586,7 @@ describe('hasCreatedBy', () => { hasCreatedBy.toQuery({ createdBy: ['user:default/user-1'], }), - ).toEqual({ property: 'createdBy', values: ['user:default/user-1'] }); + ).toEqual({ key: 'created_by', values: ['user:default/user-1'] }); }); }); it('returns the correct query filter with values (multiple users in createdBy)', () => { @@ -595,7 +595,7 @@ describe('hasCreatedBy', () => { createdBy: ['user:default/user-1', 'user:default/user-2'], }), ).toEqual({ - property: 'createdBy', + key: 'created_by', values: ['user:default/user-1', 'user:default/user-2'], }); }); diff --git a/plugins/scaffolder-backend/src/service/rules.ts b/plugins/scaffolder-backend/src/service/rules.ts index d4f7afae89..69fb69bb73 100644 --- a/plugins/scaffolder-backend/src/service/rules.ts +++ b/plugins/scaffolder-backend/src/service/rules.ts @@ -27,7 +27,7 @@ import { TemplateParametersV1beta3, } from '@backstage/plugin-scaffolder-common'; -import { SerializedTask, TaskFilter } from '@backstage/plugin-scaffolder-node'; +import { SerializedTask } from '@backstage/plugin-scaffolder-node'; import { z } from 'zod'; import { JsonObject, JsonPrimitive } from '@backstage/types'; @@ -136,7 +136,7 @@ function buildHasProperty>({ export const createTaskPermissionRule = makeCreatePermissionRule< SerializedTask, { - property: TaskFilter['property']; + key: string; values: any; }, typeof RESOURCE_TYPE_SCAFFOLDER_TASK @@ -161,7 +161,7 @@ export const hasCreatedBy = createTaskPermissionRule({ }, toQuery: ({ createdBy }) => { return { - property: 'createdBy' as TaskFilter['property'], + key: 'created_by', values: createdBy, }; }, diff --git a/plugins/scaffolder-node/report.api.md b/plugins/scaffolder-node/report.api.md index ba8263cf62..8fe081ad13 100644 --- a/plugins/scaffolder-node/report.api.md +++ b/plugins/scaffolder-node/report.api.md @@ -361,8 +361,6 @@ export interface TaskBroker { // (undocumented) get(taskId: string): Promise; // (undocumented) - getTasks(taskIds: string[]): Promise; - // (undocumented) list?(options?: { filters?: { createdBy?: string | string[]; @@ -470,8 +468,8 @@ export type TaskEventType = 'completion' | 'log' | 'cancelled' | 'recovered'; // @public export type TaskFilter = { - property: 'createdBy'; - values: Array | undefined; + key: string; + values?: Array | undefined; }; // @public diff --git a/plugins/scaffolder-node/src/tasks/types.ts b/plugins/scaffolder-node/src/tasks/types.ts index c15832645d..96c081c4c8 100644 --- a/plugins/scaffolder-node/src/tasks/types.ts +++ b/plugins/scaffolder-node/src/tasks/types.ts @@ -110,8 +110,8 @@ export type TaskBrokerDispatchOptions = { * @public */ export type TaskFilter = { - property: 'createdBy'; - values: Array | undefined; + key: string; + values?: Array | undefined; }; /** @@ -204,8 +204,6 @@ export interface TaskBroker { get(taskId: string): Promise; - getTasks(taskIds: string[]): Promise; - list?(options?: { filters?: { createdBy?: string | string[]; diff --git a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.test.tsx b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.test.tsx index 2ef7ddc4d5..5e44d5cf43 100644 --- a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.test.tsx +++ b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.test.tsx @@ -161,16 +161,6 @@ describe('OngoingTask', () => { await expect(rendered.findByText('Hide Logs')).resolves.toBeInTheDocument(); }); - it('should render not found error page when user does not have permission to read the task', async () => { - const permissionApi = mockApis.permission({ - authorize: AuthorizeResult.DENY, - }); - - await expect(render(permissionApi)).rejects.toThrow( - 'Reached NotFound Page', - ); - }); - it('should have cancel button be disabled when user has read permission but lacks cancel permission', async () => { const permissionApi = mockApis.permission({ authorize: request => { diff --git a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx index b3c82dca19..bc0d5fe1aa 100644 --- a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx +++ b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx @@ -46,7 +46,7 @@ import { TaskSteps, } from '@backstage/plugin-scaffolder-react/alpha'; import { useAsync } from '@react-hookz/web'; -import { usePermission} from '@backstage/plugin-permission-react'; +import { usePermission } from '@backstage/plugin-permission-react'; import { taskCancelPermission, taskCreatePermission,