diff --git a/workspaces/scorecard/.changeset/seven-guests-search.md b/workspaces/scorecard/.changeset/seven-guests-search.md new file mode 100644 index 0000000000..3e3e6e9f0e --- /dev/null +++ b/workspaces/scorecard/.changeset/seven-guests-search.md @@ -0,0 +1,41 @@ +--- +'@red-hat-developer-hub/backstage-plugin-scorecard-backend': minor +'@red-hat-developer-hub/backstage-plugin-scorecard-common': minor +'@red-hat-developer-hub/backstage-plugin-scorecard': minor +--- + +Implemented endpoint to aggregate metrics for scorecard metrics + +**BREAKING** Update attribute `value` in the `MetricResult` type and update validation to support `null` instead `undefined` for the updated attribute + +```diff +export type MetricResult = { + id: string; + status: 'success' | 'error'; + metadata: { + title: string; + description: string; + type: MetricType; + history?: boolean; + }; + result: { +- value?: MetricValue; ++ value: MetricValue | null; + timestamp: string; + thresholdResult: ThresholdResult; + }; + error?: string; +}; +``` + +**BREAKING** Update attribute `evaluation` in the `ThresholdResult` type and update validation to support `null` instead `undefined` for the updated attribute + +```diff +export type ThresholdResult = { + status: 'success' | 'error'; +- definition: ThresholdConfig | undefined; ++ definition: ThresholdConfig | null; + evaluation: string | undefined; // threshold key the expression evaluated to + error?: string; +}; +``` diff --git a/workspaces/scorecard/examples/all-scorecards.yaml b/workspaces/scorecard/examples/all-scorecards.yaml index 53714f2851..137e23b3da 100644 --- a/workspaces/scorecard/examples/all-scorecards.yaml +++ b/workspaces/scorecard/examples/all-scorecards.yaml @@ -10,5 +10,5 @@ metadata: jira/project-key: RSPT spec: type: service - owner: janus-authors + owner: user:development/guest lifecycle: production diff --git a/workspaces/scorecard/examples/entities.yaml b/workspaces/scorecard/examples/entities.yaml index 447e8b1f34..15964d2c9b 100644 --- a/workspaces/scorecard/examples/entities.yaml +++ b/workspaces/scorecard/examples/entities.yaml @@ -5,7 +5,7 @@ kind: System metadata: name: examples spec: - owner: guests + owner: group:development/guests --- # https://backstage.io/docs/features/software-catalog/descriptor-format#kind-component apiVersion: backstage.io/v1alpha1 @@ -15,7 +15,7 @@ metadata: spec: type: website lifecycle: experimental - owner: guests + owner: group:development/guests system: examples providesApis: [example-grpc-api] --- @@ -27,7 +27,7 @@ metadata: spec: type: grpc lifecycle: experimental - owner: guests + owner: group:development/guests system: examples definition: | syntax = "proto3"; diff --git a/workspaces/scorecard/examples/github-scorecard-only.yaml b/workspaces/scorecard/examples/github-scorecard-only.yaml index 4a0d63902c..92ab19d9a4 100644 --- a/workspaces/scorecard/examples/github-scorecard-only.yaml +++ b/workspaces/scorecard/examples/github-scorecard-only.yaml @@ -9,5 +9,5 @@ metadata: backstage.io/source-location: url:https://github.com/redhat-developer/rhdh-plugins spec: type: service - owner: janus-authors + owner: group:development/guests lifecycle: production diff --git a/workspaces/scorecard/examples/jira-scorecard-only.yaml b/workspaces/scorecard/examples/jira-scorecard-only.yaml index c405b56be2..679d232590 100644 --- a/workspaces/scorecard/examples/jira-scorecard-only.yaml +++ b/workspaces/scorecard/examples/jira-scorecard-only.yaml @@ -8,5 +8,5 @@ metadata: jira/project-key: RSPT spec: type: service - owner: janus-authors + owner: group:development/guests lifecycle: production diff --git a/workspaces/scorecard/examples/no-scorecards.yaml b/workspaces/scorecard/examples/no-scorecards.yaml index 626ac0683e..779efb4e06 100644 --- a/workspaces/scorecard/examples/no-scorecards.yaml +++ b/workspaces/scorecard/examples/no-scorecards.yaml @@ -6,5 +6,5 @@ metadata: name: no-scorecards-service spec: type: service - owner: janus-authors + owner: group:development/guests lifecycle: production diff --git a/workspaces/scorecard/examples/org.yaml b/workspaces/scorecard/examples/org.yaml index c725294468..f11c42cace 100644 --- a/workspaces/scorecard/examples/org.yaml +++ b/workspaces/scorecard/examples/org.yaml @@ -6,7 +6,7 @@ metadata: name: guest namespace: development spec: - memberOf: [guests] + memberOf: [group:development/guests, group:default/red-hat] --- # https://backstage.io/docs/features/software-catalog/descriptor-format#kind-group apiVersion: backstage.io/v1alpha1 @@ -17,3 +17,12 @@ metadata: spec: type: team children: [] +--- +apiVersion: backstage.io/v1alpha1 +kind: Group +metadata: + name: red-hat + namespace: default +spec: + type: team + children: [] diff --git a/workspaces/scorecard/examples/template/content/catalog-info.yaml b/workspaces/scorecard/examples/template/content/catalog-info.yaml index d4ccca42ef..1fdc15a5e7 100644 --- a/workspaces/scorecard/examples/template/content/catalog-info.yaml +++ b/workspaces/scorecard/examples/template/content/catalog-info.yaml @@ -4,5 +4,5 @@ metadata: name: ${{ values.name | dump }} spec: type: service - owner: user:guest + owner: user:development/guest lifecycle: experimental diff --git a/workspaces/scorecard/plugins/scorecard-backend/README.md b/workspaces/scorecard/plugins/scorecard-backend/README.md index ce21286518..bed4f1eeec 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/README.md +++ b/workspaces/scorecard/plugins/scorecard-backend/README.md @@ -97,6 +97,20 @@ Thresholds are evaluated in order, and the first matching rule determines the ca For comprehensive threshold configuration guide, examples, and best practices, see [thresholds.md](./docs/thresholds.md). +## Entity Aggregation + +The Scorecard plugin provides aggregation endpoints that return metrics for all entities owned by the authenticated user. This includes: + +- Entities directly owned by the user +- Entities owned by groups the user is a direct member of (Only direct parent groups are considered) + +### Available Endpoints + +- **`GET /metrics/catalog/aggregates`**: Returns aggregated metrics for all available metrics (optionally filtered by `metricIds` query parameter) +- **`GET /metrics/:metricId/catalog/aggregation`**: Returns aggregated metrics for a specific metric, with explicit access validation (returns `403` if the user doesn't have access to the metric) + +For comprehensive documentation on how entity aggregation works, API details, examples, and best practices, see [aggregation.md](./docs/aggregation.md). + ## Configuration cleanup Job The plugin has a predefined job that runs every day to check and clean old metrics. By default, metrics are saved for **365 days**, however, this period can be changed in the `app-config.yaml` file. Here is an example of how to do that: diff --git a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts index 9acda55f14..3c614dc1e7 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts @@ -15,38 +15,48 @@ */ import { DatabaseMetricValues } from '../src/database/DatabaseMetricValues'; -import { DbMetricValue } from '../src/database/types'; +import { DbMetricValue, DbAggregatedMetric } from '../src/database/types'; type BuildMockDatabaseMetricValuesParams = { metricValues?: DbMetricValue[]; latestEntityMetric?: DbMetricValue[]; countOfExpiredMetrics?: number; + aggregatedMetrics?: DbAggregatedMetric[]; }; export const mockDatabaseMetricValues = { createMetricValues: jest.fn(), readLatestEntityMetricValues: jest.fn(), cleanupExpiredMetrics: jest.fn(), + readAggregatedMetricsByEntityRefs: jest.fn(), } as unknown as jest.Mocked; export const buildMockDatabaseMetricValues = ({ metricValues, latestEntityMetric, countOfExpiredMetrics, + aggregatedMetrics, }: BuildMockDatabaseMetricValuesParams) => { const createMetricValues = metricValues ? jest.fn().mockResolvedValue(metricValues) : mockDatabaseMetricValues.createMetricValues; + const readLatestEntityMetricValues = latestEntityMetric ? jest.fn().mockResolvedValue(latestEntityMetric) : mockDatabaseMetricValues.readLatestEntityMetricValues; + const cleanupExpiredMetrics = countOfExpiredMetrics ? jest.fn().mockResolvedValue(countOfExpiredMetrics) : mockDatabaseMetricValues.cleanupExpiredMetrics; + const readAggregatedMetricsByEntityRefs = aggregatedMetrics + ? jest.fn().mockResolvedValue(aggregatedMetrics) + : mockDatabaseMetricValues.readAggregatedMetricsByEntityRefs; + return { createMetricValues, readLatestEntityMetricValues, cleanupExpiredMetrics, + readAggregatedMetricsByEntityRefs, } as unknown as jest.Mocked; }; diff --git a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntityBuilder.ts b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntityBuilder.ts new file mode 100644 index 0000000000..7eb77fe0c3 --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntityBuilder.ts @@ -0,0 +1,57 @@ +/* + * Copyright Red Hat, Inc. + * + * Licensed 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. + */ + +import type { Entity, EntityRelation } from '@backstage/catalog-model'; + +export class MockEntityBuilder { + private kind: string = 'Component'; + private metadata: Entity['metadata'] = { + name: 'default-component', + namespace: 'default', + }; + private spec: Entity['spec'] | undefined = undefined; + private relations: EntityRelation[] | undefined = undefined; + + withKind(kind: string): this { + this.kind = kind; + return this; + } + + withMetadata(metadata: Entity['metadata']): this { + this.metadata = metadata; + return this; + } + + withSpec(spec: Entity['spec']): this { + this.spec = spec; + return this; + } + + withRelations(relations: EntityRelation[]): this { + this.relations = relations; + return this; + } + + build(): Entity { + return { + apiVersion: 'backstage.io/v1alpha1', + kind: this.kind, + metadata: this.metadata, + spec: this.spec, + relations: this.relations, + }; + } +} diff --git a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts index e551b28983..50900883fb 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts @@ -42,7 +42,12 @@ export const buildMockMetricProvidersRegistry = ({ ? jest.fn().mockReturnValue(provider) : jest.fn(); const listMetrics = metricsList - ? jest.fn().mockReturnValue(metricsList) + ? jest.fn().mockImplementation((metricIds?: string[]) => { + if (metricIds && metricIds.length !== 0) { + return metricsList.filter(metric => metricIds.includes(metric.id)); + } + return metricsList; + }) : jest.fn(); return { diff --git a/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md b/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md new file mode 100644 index 0000000000..bc8da9aea6 --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md @@ -0,0 +1,134 @@ +# Entity Aggregation + +The Scorecard plugin provides aggregation endpoints that return metrics aggregated across all entities owned by the authenticated user. This feature allows users to get a consolidated view of metrics across their entire portfolio of owned entities. + +## Overview + +The aggregation endpoints (`/metrics/catalog/aggregates` and `/metrics/:metricId/catalog/aggregation`) aggregate metrics from multiple entities based on entity ownership. They collect metrics from: + +- Entities directly owned by the user +- Entities owned by groups the user is a direct member of + +The aggregation counts how many entities fall into each threshold category (`success`, `warning`, `error`) for each metric, providing a high-level overview of the health status across all owned entities. + +### Important Limitation: Direct Parent Groups Only + +**Only direct parent groups are considered.** The aggregation does not traverse nested group hierarchies. + +**Example:** + +Consider the following group structure: + +- User `alice` is a member of `group:default/developers` +- `group:default/developers` is a member of `group:default/engineering` + +In this case: + +- ✅ Entities owned by `alice` directly are included +- ✅ Entities owned by `group:default/developers` are included +- ❌ Entities owned by `group:default/engineering` are **NOT** included + +## API Endpoints + +### `GET /metrics/catalog/aggregates` + +Returns aggregated metrics for all entities owned by the authenticated user. + +#### Query Parameters + +| Parameter | Type | Required | Description | +| ----------- | ------ | -------- | -------------------------------------------------------------------------------------------- | +| `metricIds` | string | No | Comma-separated list of metric IDs to filter. If not provided, returns all available metrics | + +#### Authentication + +Requires user authentication. The endpoint uses the authenticated user's entity reference to determine which entities to aggregate. + +#### Permissions + +Requires `scorecard.metric.read` permission. Additionally, the user must have `catalog.entity.read` permission for each entity that will be included in the aggregation. + +#### Example Request + +```bash +# Get all aggregated metrics +curl -X GET "{{url}}/api/scorecard/metrics/catalog/aggregates" \ + -H "Authorization: Bearer " + +# Get specific metrics +curl -X GET "{{url}}/api/scorecard/metrics/catalog/aggregates?metricIds=github.open_prs,jira.open_issues" \ + -H "Authorization: Bearer " +``` + +### `GET /metrics/:metricId/catalog/aggregation` + +Returns aggregated metrics for a specific metric across all entities owned by the authenticated user. This endpoint is useful when you need to check access to a specific metric and get its aggregation without requiring the `metricIds` query parameter. + +#### Path Parameters + +| Parameter | Type | Required | Description | +| ---------- | ------ | -------- | --------------------------------- | +| `metricId` | string | Yes | The ID of the metric to aggregate | + +#### Authentication + +Requires user authentication. The endpoint uses the authenticated user's entity reference to determine which entities to aggregate. + +#### Permissions + +Requires `scorecard.metric.read` permission. Additionally: + +- The user must have access to the specific metric (returns `403 Forbidden` if access is denied) +- The user must have `catalog.entity.read` permission for each entity that will be included in the aggregation + +#### Example Request + +```bash +# Get aggregated metrics for a specific metric +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregation" \ + -H "Authorization: Bearer " +``` + +#### Differences from `/metrics/catalog/aggregates` + +- **Metric Access Validation**: This endpoint explicitly validates that the user has access to the specified metric and returns `403 Forbidden` if access is denied +- **Single Metric Only**: Returns aggregation for only the specified metric (no need for `metricIds` query parameter) +- **Empty Results Handling**: Returns an empty array `[]` when the user owns no entities, avoiding errors when filtering by a single metric + +## Error Handling + +### Missing User Entity Reference + +If the authenticated user doesn't have an entity reference in the catalog: + +- **Status Code**: `403 Forbidden` +- **Error**: `NotAllowedError: User entity reference not found` + +### Permission Denied + +If the user doesn't have permission to read a specific entity: + +- **Status Code**: `403 Forbidden` +- **Error**: Permission denied for the specific entity + +### Metric Access Denied (for `/metrics/:metricId/catalog/aggregation`) + +If the user doesn't have access to the specified metric: + +- **Status Code**: `403 Forbidden` +- **Error**: `NotAllowedError: Access to metric "" denied` + +### Invalid Query Parameters + +If invalid query parameters are provided: + +- **Status Code**: `400 Bad Request` +- **Error**: Validation error details + +## Best Practices + +1. **Use Metric Filtering**: When you only need specific metrics, use the `metricIds` parameter to reduce response size and improve performance + +2. **Handle Empty Results**: Always check for empty arrays when the user owns no entities + +3. **Group Structure**: Be aware of the direct parent group limitation when designing your group hierarchy. If you need nested group aggregation, consider restructuring your groups or implementing custom logic diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts index 85c92b299f..3e65bc6644 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts @@ -20,30 +20,29 @@ import { TestDatabases, } from '@backstage/backend-test-utils'; import { DatabaseMetricValues } from './DatabaseMetricValues'; -import { DbMetricValue } from './types'; +import { DbMetricValueCreate } from './types'; import { migrate } from './migration'; jest.setTimeout(60000); -const metricValues: Omit[] = [ +const metricValues: DbMetricValueCreate[] = [ { catalog_entity_ref: 'component:default/test-service', metric_id: 'github.metric1', value: 41, timestamp: new Date('2023-01-01T00:00:00Z'), - error_message: undefined, + status: 'success', }, { catalog_entity_ref: 'component:default/another-service', metric_id: 'github.metric1', value: 25, timestamp: new Date('2023-01-01T00:00:00Z'), - error_message: undefined, + status: 'success', }, { catalog_entity_ref: 'component:default/another-service', metric_id: 'github.metric2', - value: undefined, timestamp: new Date('2023-01-01T00:00:00Z'), error_message: 'Failed to fetch metric', }, @@ -191,4 +190,72 @@ describe('DatabaseMetricValues', () => { }, ); }); + + describe('readAggregatedMetricsByEntityRefs', () => { + it.each(databases.eachSupportedId())( + 'should return aggregated metrics by status for multiple entities and metrics - %p', + async databaseId => { + const { client, db } = await createDatabase(databaseId); + + const baseTime = new Date('2023-01-01T00:00:00Z'); + const laterTime = new Date('2023-01-01T01:00:00Z'); + + await client('metric_values').insert([ + { + ...metricValues[0], + timestamp: baseTime, + status: 'success', + }, + { + ...metricValues[1], + timestamp: baseTime, + status: 'success', + }, + { + ...metricValues[2], + timestamp: laterTime, + status: 'warning', + value: 10, + }, + { + catalog_entity_ref: 'component:default/test-service', + metric_id: 'github.metric2', + timestamp: laterTime, + value: 20, + error_message: null, + status: 'error', + }, + ]); + + const result = await db.readAggregatedMetricsByEntityRefs( + [ + 'component:default/test-service', + 'component:default/another-service', + ], + ['github.metric1', 'github.metric2'], + ); + + expect(result).toHaveLength(2); + + const metric1Result = result.find( + r => r.metric_id === 'github.metric1', + ); + const metric2Result = result.find( + r => r.metric_id === 'github.metric2', + ); + + expect(metric1Result).toBeDefined(); + expect(metric1Result?.total).toBe(2); + expect(metric1Result?.success).toBe(2); + expect(metric1Result?.warning).toBe(0); + expect(metric1Result?.error).toBe(0); + + expect(metric2Result).toBeDefined(); + expect(metric2Result?.total).toBe(2); + expect(metric2Result?.success).toBe(0); + expect(metric2Result?.warning).toBe(1); + expect(metric2Result?.error).toBe(1); + }, + ); + }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts index 652a4b2d2a..46652f51fc 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts @@ -15,7 +15,11 @@ */ import { Knex } from 'knex'; -import { DbMetricValue } from './types'; +import { + DbMetricValueCreate, + DbMetricValue, + DbAggregatedMetric, +} from './types'; export class DatabaseMetricValues { private readonly tableName = 'metric_values'; @@ -25,9 +29,7 @@ export class DatabaseMetricValues { /** * Insert multiple metric values */ - async createMetricValues( - metricValues: Omit[], - ): Promise { + async createMetricValues(metricValues: DbMetricValueCreate[]): Promise { await this.dbClient(this.tableName).insert(metricValues); } @@ -58,4 +60,68 @@ export class DatabaseMetricValues { .where('timestamp', '<', olderThan) .del(); } + + /** + * Get aggregated metrics by status for multiple entities and metrics. + */ + async readAggregatedMetricsByEntityRefs( + catalog_entity_refs: string[], + metric_ids: string[], + ): Promise { + const latestIdsSubquery = this.dbClient(this.tableName) + .max('id') + .whereIn('metric_id', metric_ids) + .whereIn('catalog_entity_ref', catalog_entity_refs) + .groupBy('metric_id', 'catalog_entity_ref'); + + const results = await this.dbClient(this.tableName) + .select('metric_id') + .count('* as total') + .max('timestamp as max_timestamp') + .select( + this.dbClient.raw( + "SUM(CASE WHEN status = 'success' AND value IS NOT NULL THEN 1 ELSE 0 END) as success", + ), + ) + .select( + this.dbClient.raw( + "SUM(CASE WHEN status = 'warning' AND value IS NOT NULL THEN 1 ELSE 0 END) as warning", + ), + ) + .select( + this.dbClient.raw( + "SUM(CASE WHEN status = 'error' AND value IS NOT NULL THEN 1 ELSE 0 END) as error", + ), + ) + .whereIn('id', latestIdsSubquery) + .whereNotNull('status') + .whereNotNull('value') + .groupBy('metric_id'); + + // Normalize types for cross-database compatibility + // PostgreSQL returns COUNT/SUM as strings, SQLite returns numbers + // PostgreSQL returns MAX(timestamp) as Date, SQLite returns number (milliseconds) + return results.map(row => { + let maxTimestamp: Date; + if (row.max_timestamp instanceof Date) { + maxTimestamp = row.max_timestamp; + } else if ( + typeof row.max_timestamp === 'number' || + typeof row.max_timestamp === 'string' + ) { + maxTimestamp = new Date(row.max_timestamp as string | number); + } else { + maxTimestamp = new Date(); + } + + return { + metric_id: row.metric_id, + total: Number(row.total), + max_timestamp: maxTimestamp, + success: Number(row.success), + warning: Number(row.warning), + error: Number(row.error), + }; + }); + } } diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts index 7315606844..62c5576c8b 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts @@ -16,12 +16,32 @@ import { MetricValue } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; -export type DbMetricValue = { - id: number; +export type DbMetricValueStatus = 'success' | 'warning' | 'error'; + +export type DbMetricValueCreate = { catalog_entity_ref: string; metric_id: string; value?: MetricValue; timestamp: Date; error_message?: string; - status?: 'success' | 'warning' | 'error'; + status?: DbMetricValueStatus; +}; + +export type DbMetricValue = { + id: number; + catalog_entity_ref: string; + metric_id: string; + value: MetricValue | null; + timestamp: Date; + error_message: string | null; + status: DbMetricValueStatus | null; +}; + +export type DbAggregatedMetric = { + metric_id: string; + total: number; + max_timestamp: Date; + success: number; + warning: number; + error: number; }; diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.test.ts index 1149ef9264..1dde25f617 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.test.ts @@ -187,7 +187,11 @@ describe('permissionUtils', () => { mockPermissionsService, mockHttpAuthService, ), - ).rejects.toThrow(new NotAllowedError('Access to entity metrics denied')); + ).rejects.toThrow( + new NotAllowedError( + 'Access to "component:default/my-service" entity metrics denied', + ), + ); expect(mockPermissionsService.authorize).toHaveBeenCalledWith( [ diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.ts b/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.ts index 4269cfeab4..4bd81ab8aa 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.ts @@ -44,7 +44,7 @@ export const checkEntityAccess = async ( ); if (entityAccessDecision[0].result !== AuthorizeResult.ALLOW) { - throw new NotAllowedError('Access to entity metrics denied'); + throw new NotAllowedError(`Access to "${entityRef}" entity metrics denied`); } }; diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts b/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts index 0aae93d76a..39d1840c34 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts @@ -115,6 +115,7 @@ export const scorecardPlugin = createBackendPlugin({ await createRouter({ metricProvidersRegistry, catalogMetricService, + catalog, httpAuth, permissions, }), diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts index b05a5205c7..50b361814a 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts @@ -24,11 +24,13 @@ import { MockNumberProvider, MockBooleanProvider, } from '../../__fixtures__/mockProviders'; -import { mockEntity } from '../../__fixtures__/mockEntities'; +import { MockEntityBuilder } from '../../__fixtures__/mockEntityBuilder'; describe('MetricProvidersRegistry', () => { let registry: MetricProvidersRegistry; + const mockEntity = new MockEntityBuilder().build(); + beforeEach(() => { registry = new MetricProvidersRegistry(); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts index 0d4b05b9e3..d50db43152 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts @@ -28,9 +28,10 @@ import { MetricProvider } from '@red-hat-developer-hub/backstage-plugin-scorecar import { mergeEntityAndProviderThresholds } from '../../utils/mergeEntityAndProviderThresholds'; import { v4 as uuid } from 'uuid'; import { stringifyEntityRef } from '@backstage/catalog-model'; -import { DbMetricValue } from '../../database/types'; +import { DbMetricValueCreate } from '../../database/types'; import { SchedulerOptions, SchedulerTask } from '../types'; import { ThresholdEvaluator } from '../../threshold/ThresholdEvaluator'; +import { MetricValue } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; type Options = Pick< SchedulerOptions, @@ -138,12 +139,16 @@ export class PullMetricsByProviderTask implements SchedulerTask { const batchResults = await Promise.allSettled( entitiesResponse.items.map(async entity => { + let value: MetricValue | undefined; + try { + value = await provider.calculateMetric(entity); + const thresholds = mergeEntityAndProviderThresholds( entity, provider, ); - const value = await provider.calculateMetric(entity); + const status = this.thresholdEvaluator.getFirstMatchingThreshold( value, metricType, @@ -156,15 +161,16 @@ export class PullMetricsByProviderTask implements SchedulerTask { value, timestamp: new Date(), status, - } as Omit; + } as DbMetricValueCreate; } catch (error) { return { catalog_entity_ref: stringifyEntityRef(entity), metric_id: this.providerId, + value, timestamp: new Date(), error_message: error instanceof Error ? error.message : String(error), - } as Omit; + } as DbMetricValueCreate; } }), ).then(promises => @@ -173,7 +179,7 @@ export class PullMetricsByProviderTask implements SchedulerTask { return [...acc, curr.value]; } return acc; - }, [] as Omit[]), + }, [] as DbMetricValueCreate[]), ); await this.database.createMetricValues(batchResults); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts index f32c62abed..e0e77f23e3 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts @@ -20,7 +20,6 @@ import { catalogServiceMock } from '@backstage/plugin-catalog-node/testUtils'; import { CatalogMetricService } from './CatalogMetricService'; import { MetricProvidersRegistry } from '../providers/MetricProvidersRegistry'; import { MockNumberProvider } from '../../__fixtures__/mockProviders'; -import { mockEntity } from '../../__fixtures__/mockEntities'; import { buildMockDatabaseMetricValues, mockDatabaseMetricValues, @@ -30,8 +29,14 @@ import { AuthService } from '@backstage/backend-plugin-api'; import * as permissionUtils from '../permissions/permissionUtils'; import { Metric } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import * as thresholdUtils from '../utils/mergeEntityAndProviderThresholds'; -import { DbMetricValue } from '../database/types'; +import { DbMetricValue, DbAggregatedMetric } from '../database/types'; import { mockThresholdRules } from '../../__fixtures__/mockThresholdRules'; +import { MockEntityBuilder } from '../../__fixtures__/mockEntityBuilder'; +import { + PermissionCriteria, + PermissionCondition, + PermissionRuleParams, +} from '@backstage/plugin-permission-common'; jest.mock('../utils/mergeEntityAndProviderThresholds'); jest.mock('../permissions/permissionUtils'); @@ -45,16 +50,37 @@ const latestEntityMetric = [ metric_id: 'github.important_metric', value: 42, timestamp: new Date('2024-01-15T12:00:00.000Z'), - error_message: undefined, + error_message: null, status: 'success', } as DbMetricValue, ] as DbMetricValue[]; +const aggregatedMetrics: DbAggregatedMetric[] = [ + { + metric_id: 'github.important_metric', + total: 2, + max_timestamp: new Date('2024-01-15T12:00:00.000Z'), + success: 1, + warning: 1, + error: 0, + }, +]; + const metricsList = [ { id: 'github.important_metric' }, { id: 'github.number_metric' }, ] as Metric[]; +const permissionsFilter = { + anyOf: [ + { + rule: 'HAS_METRIC_ID', + resourceType: 'scorecard-metric', + params: { metricIds: ['github.important_metric'] }, + }, + ], +} as PermissionCriteria>; + describe('CatalogMetricService', () => { let mockedCatalog: ReturnType; let mockedAuth: jest.Mocked; @@ -62,6 +88,8 @@ describe('CatalogMetricService', () => { let mockedDatabase: jest.Mocked; let service: CatalogMetricService; + const mockEntity = new MockEntityBuilder().build(); + beforeEach(() => { mockedCatalog = catalogServiceMock.mock(); mockedCatalog.getEntityByRef.mockResolvedValue(mockEntity); @@ -77,7 +105,10 @@ describe('CatalogMetricService', () => { metricsList, }); - mockedDatabase = buildMockDatabaseMetricValues({ latestEntityMetric }); + mockedDatabase = buildMockDatabaseMetricValues({ + latestEntityMetric, + aggregatedMetrics, + }); (permissionUtils.filterAuthorizedMetrics as jest.Mock).mockReturnValue([ { id: 'github.important_metric' }, @@ -181,28 +212,12 @@ describe('CatalogMetricService', () => { await service.getLatestEntityMetrics( 'component:default/test-component', ['github.important_metric'], - { - anyOf: [ - { - rule: 'HAS_METRIC_ID', - resourceType: 'scorecard-metric', - params: { metricIds: ['github.important_metric'] }, - }, - ], - }, + permissionsFilter, ); expect(permissionUtils.filterAuthorizedMetrics).toHaveBeenCalledWith( [{ id: 'github.important_metric' }], - expect.objectContaining({ - anyOf: [ - { - rule: 'HAS_METRIC_ID', - resourceType: 'scorecard-metric', - params: { metricIds: ['github.important_metric'] }, - }, - ], - }), + permissionsFilter, ); }); @@ -210,28 +225,12 @@ describe('CatalogMetricService', () => { await service.getLatestEntityMetrics( 'component:default/test-component', undefined, - { - anyOf: [ - { - rule: 'HAS_METRIC_ID', - resourceType: 'scorecard-metric', - params: { metricIds: ['github.important_metric'] }, - }, - ], - }, + permissionsFilter, ); expect(permissionUtils.filterAuthorizedMetrics).toHaveBeenCalledWith( [{ id: 'github.important_metric' }, { id: 'github.number_metric' }], - expect.objectContaining({ - anyOf: [ - { - rule: 'HAS_METRIC_ID', - resourceType: 'scorecard-metric', - params: { metricIds: ['github.important_metric'] }, - }, - ], - }), + permissionsFilter, ); }); @@ -312,6 +311,51 @@ describe('CatalogMetricService', () => { expect(thresholdResult.error).toBe('Error: Merge thresholds failed'); }); + it('should set threshold error when value is null', async () => { + mockedDatabase.readLatestEntityMetricValues.mockResolvedValue([ + { + ...latestEntityMetric[0], + value: null, + error_message: 'Error message during metric calculation', + }, + ]); + + const newResult = await service.getLatestEntityMetrics( + 'component:default/test-component', + ['github.important_metric'], + ); + + expect(newResult[0].status).toBe('error'); + expect(newResult[0].error).toBe( + 'Error message during metric calculation', + ); + expect(newResult[0].result.thresholdResult.status).toBe('error'); + expect(newResult[0].result.thresholdResult.error).toBe( + 'Unable to evaluate thresholds, metric value is missing', + ); + }); + + it('should set threshold error when value is not null and error message exist', async () => { + mockedDatabase.readLatestEntityMetricValues.mockResolvedValue([ + { + ...latestEntityMetric[0], + error_message: 'Threshold error message during metric calculation', + }, + ]); + + const newResult = await service.getLatestEntityMetrics( + 'component:default/test-component', + ['github.important_metric'], + ); + + expect(newResult[0].status).toBe('success'); + expect(newResult[0].error).toBeUndefined(); + expect(newResult[0].result.thresholdResult.status).toBe('error'); + expect(newResult[0].result.thresholdResult.error).toBe( + 'Threshold error message during metric calculation', + ); + }); + it('should return metric result', async () => { const result = await service.getLatestEntityMetrics( 'component:default/test-component', @@ -351,11 +395,11 @@ describe('CatalogMetricService', () => { ); }); - it('should set status to error when error message is presented', async () => { + it('should set threshold result status to error when metric value is missing', async () => { mockedDatabase.readLatestEntityMetricValues.mockResolvedValue([ { ...latestEntityMetric[0], - error_message: 'Error message', + value: null, }, ]); @@ -364,44 +408,94 @@ describe('CatalogMetricService', () => { ['github.important_metric'], ); - expect(newResult[0].status).toBe('error'); - expect(newResult[0].error).toBe('Error message'); + const thresholdResult = newResult[0].result.thresholdResult; + expect(thresholdResult.status).toBe('error'); + expect(thresholdResult.error).toBe( + 'Unable to evaluate thresholds, metric value is missing', + ); }); + }); - it('should set status to error when metric value is undefined', async () => { - mockedDatabase.readLatestEntityMetricValues.mockResolvedValue([ + describe('getAggregatedMetricsByEntityRefs', () => { + it('should return aggregated metrics for multiple entities and metrics', async () => { + const result = await service.getAggregatedMetricsByEntityRefs( + [ + 'component:default/test-component', + 'component:default/test-component-2', + ], + ['github.important_metric'], + ); + + expect(result).toEqual([ { - ...latestEntityMetric[0], - value: undefined, + id: 'github.important_metric', + status: 'success', + metadata: { + title: 'Mock Number Metric', + description: 'Mock number description.', + type: 'number', + history: undefined, + }, + result: { + values: [ + { count: 1, name: 'success' }, + { count: 1, name: 'warning' }, + { count: 0, name: 'error' }, + ], + total: 2, + timestamp: '2024-01-15T12:00:00.000Z', + }, }, ]); + }); - const newResult = await service.getLatestEntityMetrics( - 'component:default/test-component', + it('should get list of metrics from registry', async () => { + await service.getAggregatedMetricsByEntityRefs( + [ + 'component:default/test-component', + 'component:default/test-component-2', + ], ['github.important_metric'], ); - expect(newResult[0].status).toBe('error'); - expect(newResult[0].error).toBe("Error: Metric value is 'undefined'"); + expect(mockedRegistry.listMetrics).toHaveBeenCalledWith([ + 'github.important_metric', + ]); }); - it('should set threshold result status to error when metric value is missing', async () => { - mockedDatabase.readLatestEntityMetricValues.mockResolvedValue([ - { - ...latestEntityMetric[0], - value: undefined, - }, - ]); + it('should filter authorized metrics for specific provider IDs', async () => { + await service.getAggregatedMetricsByEntityRefs( + [ + 'component:default/test-component', + 'component:default/test-component-2', + ], + ['github.important_metric', 'github.number_metric'], + permissionsFilter, + ); - const newResult = await service.getLatestEntityMetrics( - 'component:default/test-component', + expect(permissionUtils.filterAuthorizedMetrics).toHaveBeenCalledWith( + [{ id: 'github.important_metric' }, { id: 'github.number_metric' }], + permissionsFilter, + ); + }); + + it('should read aggregated metrics by entity refs', async () => { + await service.getAggregatedMetricsByEntityRefs( + [ + 'component:default/test-component', + 'component:default/test-component-2', + ], ['github.important_metric'], ); - const thresholdResult = newResult[0].result.thresholdResult; - expect(thresholdResult.status).toBe('error'); - expect(thresholdResult.error).toBe( - 'Unable to evaluate thresholds, metric value is missing', + expect( + mockedDatabase.readAggregatedMetricsByEntityRefs, + ).toHaveBeenCalledWith( + [ + 'component:default/test-component', + 'component:default/test-component-2', + ], + ['github.important_metric'], ); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts index 25096ac77d..16ebc51163 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -15,6 +15,7 @@ */ import { + AggregatedMetricResult, MetricResult, ThresholdConfig, } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; @@ -31,13 +32,18 @@ import { CatalogService } from '@backstage/plugin-catalog-node'; import { DatabaseMetricValues } from '../database/DatabaseMetricValues'; import { mergeEntityAndProviderThresholds } from '../utils/mergeEntityAndProviderThresholds'; -export type CatalogMetricServiceOptions = { +type CatalogMetricServiceOptions = { catalog: CatalogService; auth: AuthService; registry: MetricProvidersRegistry; database: DatabaseMetricValues; }; +export type AggregatedMetricsByStatus = Record< + string, + { values: { success: number; warning: number; error: number }; total: number } +>; + export class CatalogMetricService { private readonly catalog: CatalogService; private readonly auth: AuthService; @@ -74,9 +80,7 @@ export class CatalogMetricService { throw new NotFoundError(`Entity not found: ${entityRef}`); } - const metricsToFetch = providerIds - ? this.registry.listMetrics().filter(m => providerIds.includes(m.id)) - : this.registry.listMetrics(); + const metricsToFetch = this.registry.listMetrics(providerIds); const authorizedMetricsToFetch = filterAuthorizedMetrics( metricsToFetch, @@ -98,15 +102,17 @@ export class CatalogMetricService { try { thresholds = mergeEntityAndProviderThresholds(entity, provider); - if (value === undefined) { + if (value === null) { thresholdError = 'Unable to evaluate thresholds, metric value is missing'; + } else if (error_message) { + thresholdError = error_message; } } catch (error) { thresholdError = stringifyError(error); } - const isMetricCalcError = error_message || value === undefined; + const isMetricCalcError = error_message !== null && value === null; return { id: metric.id, @@ -136,4 +142,67 @@ export class CatalogMetricService { }, ); } + + /** + * Get aggregated metrics for multiple entities and metrics + * + * @param entityRefs - Array of entity references in format "kind:namespace/name" + * @param metricIds - Optional array of metric IDs to get aggregated metrics of. + * If not provided, gets all available aggregated metrics. + * @returns Aggregated metric results + */ + async getAggregatedMetricsByEntityRefs( + entityRefs: string[], + metricIds?: string[], + filter?: PermissionCriteria< + PermissionCondition + >, + ): Promise { + const metricsToFetch = this.registry.listMetrics(metricIds); + + const authorizedMetricsToFetch = filterAuthorizedMetrics( + metricsToFetch, + filter, + ); + + const aggregatedMetrics = + await this.database.readAggregatedMetricsByEntityRefs( + entityRefs, + authorizedMetricsToFetch.map(m => m.id), + ); + + return aggregatedMetrics.map(row => { + const metricId = row.metric_id; + const success = row.success || 0; + const warning = row.warning || 0; + const error = row.error || 0; + const total = row.total || 0; + const timestamp = row.max_timestamp + ? new Date(row.max_timestamp).toISOString() + : new Date().toISOString(); + + const provider = this.registry.getProvider(metricId); + const metric = provider.getMetric(); + + return { + id: metricId, + status: 'success', + metadata: { + title: metric.title, + description: metric.description, + type: metric.type, + history: metric.history, + }, + result: { + values: [ + { count: success, name: 'success' }, + { count: warning, name: 'warning' }, + { count: error, name: 'error' }, + ], + total, + timestamp, + }, + }; + }); + } } diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts index 6781989f6d..d2c6b326b5 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -29,6 +29,7 @@ import { githubNumberMetricMetadata, } from '../../__fixtures__/mockProviders'; import { + AggregatedMetricResult, Metric, MetricResult, } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; @@ -42,6 +43,22 @@ import { import { PermissionsService } from '@backstage/backend-plugin-api'; import { mockDatabaseMetricValues } from '../../__fixtures__/mockDatabaseMetricValues'; +jest.mock('../utils/getEntitiesOwnedByUser', () => ({ + getEntitiesOwnedByUser: jest.fn(), +})); + +jest.mock('../permissions/permissionUtils', () => { + const originalModule = jest.requireActual('../permissions/permissionUtils'); + return { + ...originalModule, + checkEntityAccess: jest.fn(), + }; +}); + +import * as getEntitiesOwnedByUserModule from '../utils/getEntitiesOwnedByUser'; +import * as permissionUtilsModule from '../permissions/permissionUtils'; +import { MockEntityBuilder } from '../../__fixtures__/mockEntityBuilder'; + const CONDITIONAL_POLICY_DECISION: PolicyDecision = { result: AuthorizeResult.CONDITIONAL, pluginId: 'scorecard', @@ -63,6 +80,9 @@ describe('createRouter', () => { let app: express.Express; let metricProvidersRegistry: MetricProvidersRegistry; let catalogMetricService: CatalogMetricService; + let httpAuthMock: ServiceMock< + import('@backstage/backend-plugin-api').HttpAuthService + >; const permissionsMock: ServiceMock = mockServices.permissions.mock({ authorizeConditional: jest.fn(), @@ -71,8 +91,9 @@ describe('createRouter', () => { beforeEach(async () => { metricProvidersRegistry = new MetricProvidersRegistry(); + const catalog = catalogServiceMock.mock(); catalogMetricService = new CatalogMetricService({ - catalog: catalogServiceMock.mock(), + catalog, registry: metricProvidersRegistry, auth: mockServices.auth(), database: mockDatabaseMetricValues, @@ -86,10 +107,19 @@ describe('createRouter', () => { { result: AuthorizeResult.ALLOW }, ]); + httpAuthMock = mockServices.httpAuth.mock({ + credentials: jest.fn().mockResolvedValue({ + principal: { + userEntityRef: 'user:default/test-user', + }, + }), + }); + const router = await createRouter({ metricProvidersRegistry, catalogMetricService, - httpAuth: mockServices.httpAuth.mock(), + catalog, + httpAuth: httpAuthMock, permissions: permissionsMock, }); app = express(); @@ -132,6 +162,7 @@ describe('createRouter', () => { it('should return all metrics', async () => { const response = await request(app).get('/metrics'); + console.log('response', response.body); expect(response.status).toBe(200); expect(response.body).toHaveProperty('metrics'); @@ -342,4 +373,237 @@ describe('createRouter', () => { expect(response.body.error.message).toContain('Invalid query parameters'); }); }); + + describe('GET /metrics/:metricId/catalog/aggregation', () => { + const mockAggregatedMetricResults: AggregatedMetricResult[] = [ + { + id: 'github.open_prs', + status: 'success', + metadata: { + title: 'GitHub open PRs', + description: + 'Current count of open Pull Requests for a given GitHub repository.', + type: 'number', + history: true, + }, + result: { + values: [ + { count: 5, name: 'success' }, + { count: 4, name: 'warning' }, + { count: 3, name: 'error' }, + ], + total: 12, + timestamp: '2025-01-01T10:30:00.000Z', + }, + }, + ]; + let mockCatalog: ReturnType; + let getEntitiesOwnedByUserSpy: jest.SpyInstance; + let checkEntityAccessSpy: jest.SpyInstance; + let aggregationApp: express.Express; + + beforeEach(async () => { + const githubProvider = new MockNumberProvider( + 'github.open_prs', + 'github', + 'GitHub Open PRs', + ); + const jiraProvider = new MockNumberProvider( + 'jira.open_issues', + 'jira', + 'Jira Open Issues', + ); + metricProvidersRegistry.register(githubProvider); + metricProvidersRegistry.register(jiraProvider); + + mockCatalog = catalogServiceMock.mock(); + + const userEntity = new MockEntityBuilder() + .withKind('User') + .withMetadata({ name: 'test-user', namespace: 'default' }) + .build(); + mockCatalog.getEntityByRef.mockResolvedValue(userEntity); + + const componentEntity = new MockEntityBuilder() + .withKind('Component') + .withMetadata({ name: 'my-service', namespace: 'default' }) + .build(); + mockCatalog.getEntities.mockResolvedValue({ items: [componentEntity] }); + + jest + .spyOn(catalogMetricService, 'getAggregatedMetricsByEntityRefs') + .mockResolvedValue(mockAggregatedMetricResults); + + getEntitiesOwnedByUserSpy = jest + .spyOn(getEntitiesOwnedByUserModule, 'getEntitiesOwnedByUser') + .mockResolvedValue([ + 'component:default/my-service', + 'component:default/my-other-service', + ]); + + checkEntityAccessSpy = jest.spyOn( + permissionUtilsModule, + 'checkEntityAccess', + ); + + const router = await createRouter({ + metricProvidersRegistry, + catalogMetricService, + catalog: mockCatalog, + httpAuth: httpAuthMock, + permissions: permissionsMock, + }); + aggregationApp = express(); + aggregationApp.use(router); + aggregationApp.use(mockErrorHandler()); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should return 403 Unauthorized when DENY permissions', async () => { + permissionsMock.authorizeConditional.mockResolvedValue([ + { result: AuthorizeResult.DENY }, + ]); + const result = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(result.statusCode).toBe(403); + expect(result.body.error.name).toEqual('NotAllowedError'); + }); + + it('should return 404 NotFoundError when user entity reference is not found', async () => { + httpAuthMock.credentials.mockResolvedValue({ + principal: {}, + } as any); + const result = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(result.statusCode).toBe(404); + expect(result.body.error.name).toEqual('NotFoundError'); + expect(result.body.error.message).toContain( + 'User entity reference not found', + ); + }); + + it('should return 403 NotAllowedError when user does not have access to the metric', async () => { + permissionsMock.authorizeConditional.mockResolvedValue([ + CONDITIONAL_POLICY_DECISION, + ]); + const result = await request(aggregationApp).get( + '/metrics/jira.open_issues/catalog/aggregation', + ); + + expect(result.statusCode).toBe(403); + expect(result.body.error.name).toEqual('NotAllowedError'); + expect(result.body.error.message).toContain( + 'To view the scorecard metrics, your administrator must grant you the required permission.', + ); + }); + + it('should return aggregated metrics for a specific metric', async () => { + const response = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(response.status).toBe(200); + expect( + catalogMetricService.getAggregatedMetricsByEntityRefs, + ).toHaveBeenCalledWith( + ['component:default/my-service', 'component:default/my-other-service'], + ['github.open_prs'], + undefined, + ); + expect(response.body).toEqual(mockAggregatedMetricResults); + }); + + it('should get entities owned by user', async () => { + const response = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(response.status).toBe(200); + expect(getEntitiesOwnedByUserSpy).toHaveBeenCalledWith( + 'user:default/test-user', + expect.objectContaining({ + catalog: expect.any(Object), + credentials: expect.any(Object), + }), + ); + }); + + it('should return empty array when user owns no entities', async () => { + getEntitiesOwnedByUserSpy.mockResolvedValue([]); + const response = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(response.status).toBe(200); + expect(response.body).toEqual([]); + expect( + catalogMetricService.getAggregatedMetricsByEntityRefs, + ).not.toHaveBeenCalled(); + }); + + it('should check entity access for each entity owned by user', async () => { + const response = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(checkEntityAccessSpy).toHaveBeenCalledTimes(2); + expect(checkEntityAccessSpy).toHaveBeenCalledWith( + 'component:default/my-service', + expect.any(Object), + permissionsMock, + httpAuthMock, + ); + expect(checkEntityAccessSpy).toHaveBeenCalledWith( + 'component:default/my-other-service', + expect.any(Object), + permissionsMock, + httpAuthMock, + ); + expect(response.status).toBe(200); + expect(response.body).toEqual(mockAggregatedMetricResults); + }); + + it('should filter authorized metrics when CONDITIONAL permission', async () => { + permissionsMock.authorizeConditional.mockResolvedValue([ + CONDITIONAL_POLICY_DECISION, + ]); + const response = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(response.status).toBe(200); + expect( + catalogMetricService.getAggregatedMetricsByEntityRefs, + ).toHaveBeenCalledWith( + ['component:default/my-service', 'component:default/my-other-service'], + ['github.open_prs'], + { + anyOf: [ + { + rule: 'HAS_METRIC_ID', + resourceType: 'scorecard-metric', + params: { metricIds: ['github.open_prs', 'github.open_issues'] }, + }, + ], + }, + ); + }); + + it('should return 404 NotFoundError when metric is not found', async () => { + const result = await request(aggregationApp).get( + '/metrics/non.existent.metric/catalog/aggregation', + ); + + expect(result.statusCode).toBe(404); + expect(result.body.error.name).toBe('NotFoundError'); + expect(result.body.error.message).toContain('Metric provider with ID'); + }); + }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index e94f452d77..819e2a1501 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { InputError, NotAllowedError } from '@backstage/errors'; +import { InputError, NotAllowedError, NotFoundError } from '@backstage/errors'; import { z } from 'zod'; import express, { Request } from 'express'; import Router from 'express-promise-router'; @@ -23,6 +23,7 @@ import { type HttpAuthService, type PermissionsService, } from '@backstage/backend-plugin-api'; +import type { CatalogService } from '@backstage/plugin-catalog-node'; import { AuthorizeResult, BasicPermission, @@ -35,10 +36,14 @@ import { checkEntityAccess, } from '../permissions/permissionUtils'; import { stringifyEntityRef } from '@backstage/catalog-model'; +import { validateCatalogMetricsSchema } from '../validation/validateCatalogMetricsSchema'; +import { getEntitiesOwnedByUser } from '../utils/getEntitiesOwnedByUser'; +import { parseCommaSeparatedString } from '../utils/parseCommaSeparatedString'; export type ScorecardRouterOptions = { metricProvidersRegistry: MetricProvidersRegistry; catalogMetricService: CatalogMetricService; + catalog: CatalogService; httpAuth: HttpAuthService; permissions: PermissionsService; }; @@ -46,6 +51,7 @@ export type ScorecardRouterOptions = { export async function createRouter({ metricProvidersRegistry, catalogMetricService, + catalog, httpAuth, permissions, }: ScorecardRouterOptions): Promise { @@ -124,23 +130,17 @@ export async function createRouter({ const { kind, namespace, name } = req.params; const { metricIds } = req.query; - const catalogMetricsSchema = z.object({ - metricIds: z.string().min(1).optional(), - }); - - const parsed = catalogMetricsSchema.safeParse(req.query); - if (!parsed.success) { - throw new InputError(`Invalid query parameters: ${parsed.error.message}`); - } + validateCatalogMetricsSchema(req.query); const entityRef = stringifyEntityRef({ kind, namespace, name }); // Check if user has permission to read this specific catalog entity await checkEntityAccess(entityRef, req, permissions, httpAuth); - const metricIdArray = metricIds - ? (metricIds as string).split(',').map(id => id.trim()) - : undefined; + const metricIdArray = + typeof metricIds === 'string' + ? parseCommaSeparatedString(metricIds) + : undefined; const results = await catalogMetricService.getLatestEntityMetrics( entityRef, @@ -150,5 +150,53 @@ export async function createRouter({ res.json(results); }); + router.get('/metrics/:metricId/catalog/aggregation', async (req, res) => { + const { metricId } = req.params; + + const { conditions } = await authorizeConditional( + req, + scorecardMetricReadPermission, + ); + + const metric = metricProvidersRegistry.getMetric(metricId); + const authorizedMetrics = filterAuthorizedMetrics([metric], conditions); + + if (authorizedMetrics.length === 0) { + throw new NotAllowedError( + `To view the scorecard metrics, your administrator must grant you the required permission.`, + ); + } + + const credentials = await httpAuth.credentials(req, { allow: ['user'] }); + const userEntityRef = credentials?.principal?.userEntityRef; + + if (!userEntityRef) { + throw new NotFoundError('User entity reference not found'); + } + + const entitiesOwnedByAUser = await getEntitiesOwnedByUser(userEntityRef, { + catalog, + credentials, + }); + + if (entitiesOwnedByAUser.length === 0) { + res.json([]); + return; + } + + for (const entityRef of entitiesOwnedByAUser) { + await checkEntityAccess(entityRef, req, permissions, httpAuth); + } + + const aggregatedMetrics = + await catalogMetricService.getAggregatedMetricsByEntityRefs( + entitiesOwnedByAUser, + [metricId], + conditions, + ); + + res.json(aggregatedMetrics); + }); + return router; } diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.test.ts new file mode 100644 index 0000000000..0e66a739fe --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.test.ts @@ -0,0 +1,344 @@ +/* + * Copyright Red Hat, Inc. + * + * Licensed 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. + */ + +import { catalogServiceMock } from '@backstage/plugin-catalog-node/testUtils'; +import type { BackstageCredentials } from '@backstage/backend-plugin-api'; +import * as catalogModel from '@backstage/catalog-model'; +import type { Entity } from '@backstage/catalog-model'; +import { + RELATION_MEMBER_OF, + RELATION_OWNED_BY, +} from '@backstage/catalog-model'; + +jest.mock('@backstage/catalog-model', () => { + const actual = jest.requireActual('@backstage/catalog-model'); + return { + ...actual, + stringifyEntityRef: jest.fn(actual.stringifyEntityRef), + }; +}); + +import { getEntitiesOwnedByUser } from './getEntitiesOwnedByUser'; +import { MockEntityBuilder } from '../../__fixtures__/mockEntityBuilder'; + +describe('getEntitiesOwnedByUser', () => { + let mockedCatalog: ReturnType; + let mockCredentials: BackstageCredentials; + let userEntity: catalogModel.Entity; + let userOwnedEntity: catalogModel.Entity; + + beforeEach(() => { + mockedCatalog = catalogServiceMock.mock(); + mockCredentials = {} as BackstageCredentials; + + userEntity = new MockEntityBuilder() + .withKind('User') + .withMetadata({ name: 'test-user', namespace: 'development' }) + .build(); + userOwnedEntity = new MockEntityBuilder() + .withMetadata({ name: 'user-component', namespace: 'development' }) + .build(); + mockedCatalog.queryEntities.mockResolvedValue({ + items: [ + { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { name: 'user-component', namespace: 'development' }, + }, + ], + pageInfo: { + nextCursor: undefined, // No next page + }, + totalItems: 1, + }); + }); + + afterEach(() => { + jest.clearAllMocks(); + jest.restoreAllMocks(); + }); + + it('should throw NotFoundError when user entity is not found', async () => { + mockedCatalog.getEntityByRef.mockResolvedValue(undefined); + + await expect( + getEntitiesOwnedByUser('user:development/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }), + ).rejects.toThrow('User entity not found in catalog'); + }); + + describe('when user has no memberOf groups', () => { + beforeEach(() => { + mockedCatalog.getEntityByRef.mockResolvedValue(userEntity); + }); + + it('should call stringifyEntityRef once', async () => { + await getEntitiesOwnedByUser('user:development/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(catalogModel.stringifyEntityRef).toHaveBeenCalledTimes(1); + expect(catalogModel.stringifyEntityRef).toHaveBeenNthCalledWith( + 1, + userOwnedEntity, + ); + }); + + it('should not access memberOf relations when user has no groups', async () => { + await getEntitiesOwnedByUser('user:development/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(userEntity.relations).toBeUndefined(); + }); + + it('should call queryEntities with owned by user relation', async () => { + await getEntitiesOwnedByUser('user:development/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(mockedCatalog.queryEntities).toHaveBeenCalledTimes(1); + expect(mockedCatalog.queryEntities).toHaveBeenCalledWith( + { + filter: { + [`relations.${RELATION_OWNED_BY}`]: 'user:development/test-user', + }, + fields: ['kind', 'metadata'], + limit: 50, + }, + { credentials: mockCredentials }, + ); + }); + + it('should return entities owned by user only', async () => { + const result = await getEntitiesOwnedByUser( + 'user:development/test-user', + { + catalog: mockedCatalog, + credentials: mockCredentials, + }, + ); + + expect(result).toEqual(['component:development/user-component']); + }); + + it('should return empty array when user owns no entities', async () => { + mockedCatalog.queryEntities.mockResolvedValue({ + items: [], + pageInfo: { nextCursor: undefined }, + totalItems: 0, + }); + + const result = await getEntitiesOwnedByUser( + 'user:development/test-user', + { + catalog: mockedCatalog, + credentials: mockCredentials, + }, + ); + + expect(result).toEqual([]); + }); + + it('should call queryEntities with cursor', async () => { + mockedCatalog.queryEntities.mockReset(); + mockedCatalog.queryEntities + .mockResolvedValueOnce({ + items: [userOwnedEntity], + pageInfo: { nextCursor: 'cursor1' }, + totalItems: 1, + }) + .mockResolvedValueOnce({ + items: [], + pageInfo: { nextCursor: undefined }, + totalItems: 0, + }); + + await getEntitiesOwnedByUser('user:development/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(mockedCatalog.queryEntities).toHaveBeenCalledTimes(2); + expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( + 1, + { + filter: { + [`relations.${RELATION_OWNED_BY}`]: 'user:development/test-user', + }, + fields: ['kind', 'metadata'], + limit: 50, + }, + { credentials: mockCredentials }, + ); + expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( + 2, + { + cursor: 'cursor1', + filter: { + [`relations.${RELATION_OWNED_BY}`]: 'user:development/test-user', + }, + fields: ['kind', 'metadata'], + limit: 50, + }, + { credentials: mockCredentials }, + ); + }); + }); + + describe('when user has memberOf groups', () => { + let userEntityWithMemberOf: Entity; + let groupOwnedEntity: Entity; + + beforeEach(() => { + userEntityWithMemberOf = new MockEntityBuilder() + .withKind('User') + .withMetadata({ name: 'test-user', namespace: 'development' }) + .withRelations([ + { + type: RELATION_MEMBER_OF, + targetRef: 'group:development/developers', + }, + ]) + .build(); + groupOwnedEntity = new MockEntityBuilder() + .withMetadata({ name: 'group-component', namespace: 'development' }) + .build(); + + mockedCatalog.getEntityByRef.mockResolvedValue(userEntityWithMemberOf); + mockedCatalog.queryEntities + .mockResolvedValueOnce({ + items: [userOwnedEntity], + pageInfo: { nextCursor: undefined }, + totalItems: 1, + }) + .mockResolvedValueOnce({ + items: [groupOwnedEntity], + pageInfo: { nextCursor: undefined }, + totalItems: 1, + }); + }); + + it('should call stringifyEntityRef twice for owned entities', async () => { + await getEntitiesOwnedByUser('user:development/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(catalogModel.stringifyEntityRef).toHaveBeenCalledTimes(2); + expect(catalogModel.stringifyEntityRef).toHaveBeenNthCalledWith( + 1, + userOwnedEntity, + ); + expect(catalogModel.stringifyEntityRef).toHaveBeenNthCalledWith( + 2, + groupOwnedEntity, + ); + }); + + it('should use relations to get group references', async () => { + await getEntitiesOwnedByUser('user:development/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(userEntityWithMemberOf.relations).toHaveLength(1); + expect(userEntityWithMemberOf.relations?.[0].type).toBe( + RELATION_MEMBER_OF, + ); + expect(userEntityWithMemberOf.relations?.[0].targetRef).toBe( + 'group:development/developers', + ); + }); + + it('should call queryEntities with owned by user relation', async () => { + await getEntitiesOwnedByUser('user:development/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(mockedCatalog.queryEntities).toHaveBeenCalledTimes(2); + expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( + 1, + { + filter: { + [`relations.${RELATION_OWNED_BY}`]: 'user:development/test-user', + }, + fields: ['kind', 'metadata'], + limit: 50, + }, + { credentials: mockCredentials }, + ); + expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( + 2, + { + filter: { + [`relations.${RELATION_OWNED_BY}`]: 'group:development/developers', + }, + fields: ['kind', 'metadata'], + limit: 50, + }, + { credentials: mockCredentials }, + ); + }); + + it('should return entities owned by user and groups', async () => { + const result = await getEntitiesOwnedByUser( + 'user:development/test-user', + { + catalog: mockedCatalog, + credentials: mockCredentials, + }, + ); + + expect(result).toEqual([ + 'component:development/user-component', + 'component:development/group-component', + ]); + }); + + it('should return empty array when user and groups owns no entities', async () => { + mockedCatalog.queryEntities.mockReset(); + mockedCatalog.queryEntities + .mockResolvedValueOnce({ + items: [], + pageInfo: { nextCursor: undefined }, + totalItems: 0, + }) + .mockResolvedValueOnce({ + items: [], + pageInfo: { nextCursor: undefined }, + totalItems: 0, + }); + + const result = await getEntitiesOwnedByUser( + 'user:development/test-user', + { + catalog: mockedCatalog, + credentials: mockCredentials, + }, + ); + + expect(result).toEqual([]); + expect(mockedCatalog.queryEntities).toHaveBeenCalledTimes(2); + }); + }); +}); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.ts b/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.ts new file mode 100644 index 0000000000..b044c97885 --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.ts @@ -0,0 +1,84 @@ +/* + * Copyright Red Hat, Inc. + * + * Licensed 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. + */ + +import { NotFoundError } from '@backstage/errors'; +import { BackstageCredentials } from '@backstage/backend-plugin-api'; +import { CatalogService } from '@backstage/plugin-catalog-node'; +import { + RELATION_MEMBER_OF, + RELATION_OWNED_BY, + stringifyEntityRef, +} from '@backstage/catalog-model'; + +const QUERY_ENTITIES_BATCH_SIZE = 50; + +export async function getEntitiesOwnedByUser( + userEntityRef: string, + options: { + catalog: CatalogService; + credentials: BackstageCredentials; + }, +): Promise { + const userEntity = await options.catalog.getEntityByRef(userEntityRef, { + credentials: options.credentials, + }); + + if (!userEntity) { + throw new NotFoundError('User entity not found in catalog'); + } + + const ownerRefs: string[] = [userEntityRef]; + + const memberOfRelations = + userEntity.relations?.filter( + relation => relation.type === RELATION_MEMBER_OF, + ) ?? []; + + if (memberOfRelations.length > 0) { + for (const relation of memberOfRelations) { + ownerRefs.push(relation.targetRef); + } + } + + const entitiesOwnedByUserAndGroups: string[] = []; + + for (const ownerRef of ownerRefs) { + let cursor: string | undefined = undefined; + + do { + const entities = await options.catalog.queryEntities( + { + filter: { + [`relations.${RELATION_OWNED_BY}`]: ownerRef, + }, + fields: ['kind', 'metadata'], + limit: QUERY_ENTITIES_BATCH_SIZE, + ...(cursor ? { cursor } : {}), + }, + { credentials: options.credentials }, + ); + + cursor = entities.pageInfo.nextCursor; + + const entityRefs = entities.items.map(entity => + stringifyEntityRef(entity), + ); + entitiesOwnedByUserAndGroups.push(...entityRefs); + } while (cursor !== undefined); + } + + return entitiesOwnedByUserAndGroups; +} diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/utils/parseCommaSeparatedString.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/utils/parseCommaSeparatedString.test.ts new file mode 100644 index 0000000000..83d6cdb8a6 --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/parseCommaSeparatedString.test.ts @@ -0,0 +1,47 @@ +/* + * Copyright Red Hat, Inc. + * + * Licensed 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. + */ + +import { parseCommaSeparatedString } from './parseCommaSeparatedString'; + +describe('parseCommaSeparatedString', () => { + it('should return array with single value for non-comma string', () => { + const result = parseCommaSeparatedString('github.open_prs'); + expect(result).toEqual(['github.open_prs']); + }); + + it('should return array with trimmed whitespace multiple values when values are comma and space separated', () => { + const result = parseCommaSeparatedString( + ' github.open_prs , github.open_issues ', + ); + + expect(result).toEqual(['github.open_prs', 'github.open_issues']); + }); + + it('should handle string with only whitespace', () => { + const result = parseCommaSeparatedString(' '); + expect(result).toEqual(['']); + }); + + it('should handle string with only commas', () => { + const result = parseCommaSeparatedString(' , , '); + expect(result).toEqual(['', '', '']); + }); + + it('should handle empty values between commas', () => { + const result = parseCommaSeparatedString('metric1, ,metric2'); + expect(result).toEqual(['metric1', '', 'metric2']); + }); +}); diff --git a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntities.ts b/workspaces/scorecard/plugins/scorecard-backend/src/utils/parseCommaSeparatedString.ts similarity index 68% rename from workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntities.ts rename to workspaces/scorecard/plugins/scorecard-backend/src/utils/parseCommaSeparatedString.ts index e35d524c85..1d3e83bc72 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntities.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/parseCommaSeparatedString.ts @@ -13,15 +13,13 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import type { Entity } from '@backstage/catalog-model'; -export const mockEntity: Entity = { - apiVersion: 'backstage.io/v1alpha1', - kind: 'Component', - metadata: { - name: 'test-component', - }, - spec: { - owner: 'guests', - }, -}; +/** + * Parse a comma separated string into an array of strings + * + * @param value - The comma separated string to parse + * @returns The array of strings + */ +export function parseCommaSeparatedString(value: string): string[] { + return value.split(',').map(id => id.trim()); +} diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts new file mode 100644 index 0000000000..ebc0f1da8f --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts @@ -0,0 +1,82 @@ +/* + * Copyright Red Hat, Inc. + * + * Licensed 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. + */ + +import { validateCatalogMetricsSchema } from './validateCatalogMetricsSchema'; +import { InputError } from '@backstage/errors'; + +describe('validateCatalogMetricsSchema', () => { + describe('valid query parameters', () => { + it('should validate empty object', () => { + expect(() => validateCatalogMetricsSchema({})).not.toThrow(); + }); + + it('should validate object with valid metricIds string', () => { + expect(() => + validateCatalogMetricsSchema({ metricIds: 'github.open_prs' }), + ).not.toThrow(); + }); + + it('should validate object with valid metricIds containing comma-separated values', () => { + expect(() => + validateCatalogMetricsSchema({ + metricIds: 'github.open_prs,github.open_issues', + }), + ).not.toThrow(); + }); + + it('should validate object with undefined metricIds', () => { + expect(() => + validateCatalogMetricsSchema({ metricIds: undefined }), + ).not.toThrow(); + }); + + it('should validate when query has additional properties along with valid metricIds', () => { + expect(() => + validateCatalogMetricsSchema({ + metricIds: 'github.open_prs', + invalidProp: 'value', + }), + ).not.toThrow(); + }); + + it('should validate when query has only additional properties', () => { + expect(() => + validateCatalogMetricsSchema({ invalidProp: 'value' }), + ).not.toThrow(); + }); + }); + + describe('invalid query parameters', () => { + it.each([ + { metricIds: '', description: 'empty string' }, + { metricIds: null, description: 'null' }, + { metricIds: 123, description: 'number' }, + { metricIds: true, description: 'boolean' }, + { metricIds: ['github.open_prs'], description: 'array' }, + { metricIds: { id: 'test' }, description: 'object' }, + ])( + 'should throw InputError when metricIds is $description', + ({ metricIds }) => { + expect(() => validateCatalogMetricsSchema({ metricIds })).toThrow( + InputError, + ); + expect(() => validateCatalogMetricsSchema({ metricIds })).toThrow( + 'Invalid query parameters', + ); + }, + ); + }); +}); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.ts b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.ts new file mode 100644 index 0000000000..65aff9832c --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.ts @@ -0,0 +1,30 @@ +/* + * Copyright Red Hat, Inc. + * + * Licensed 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. + */ + +import { z } from 'zod'; +import { InputError } from '@backstage/errors'; + +export function validateCatalogMetricsSchema(query: unknown): void { + const catalogMetricsSchema = z.object({ + metricIds: z.string().min(1).optional(), + }); + + const parsed = catalogMetricsSchema.safeParse(query); + + if (!parsed.success) { + throw new InputError(`Invalid query parameters: ${parsed.error.message}`); + } +} diff --git a/workspaces/scorecard/plugins/scorecard-common/report.api.md b/workspaces/scorecard/plugins/scorecard-common/report.api.md index abd8721a38..19db013864 100644 --- a/workspaces/scorecard/plugins/scorecard-common/report.api.md +++ b/workspaces/scorecard/plugins/scorecard-common/report.api.md @@ -5,6 +5,29 @@ ```ts import { ResourcePermission } from '@backstage/plugin-permission-common'; +// @public (undocumented) +export type AggregatedMetricResult = { + id: string; + status: 'success' | 'error'; + metadata: { + title: string; + description: string; + type: MetricType; + history?: boolean; + }; + result: { + values: AggregatedMetricValue[]; + total: number; + timestamp: string; + }; +}; + +// @public (undocumented) +export type AggregatedMetricValue = { + count: number; + name: 'success' | 'warning' | 'error'; +}; + // @public export const DEFAULT_NUMBER_THRESHOLDS: ThresholdConfig; @@ -28,7 +51,7 @@ export type MetricResult = { history?: boolean; }; result: { - value?: MetricValue; + value: MetricValue | null; timestamp: string; thresholdResult: ThresholdResult; }; @@ -68,7 +91,7 @@ export type ThresholdConfig = { export type ThresholdResult = { status: 'success' | 'error'; definition: ThresholdConfig | undefined; - evaluation: string | undefined; + evaluation: string | null; error?: string; }; diff --git a/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts b/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts index 6ec46e860e..6b16831b04 100644 --- a/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts +++ b/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts @@ -30,6 +30,14 @@ export type MetricValue = T extends 'number' ? boolean : never; +/** + * @public + */ +export type AggregatedMetricValue = { + count: number; + name: 'success' | 'warning' | 'error'; +}; + /** * @public */ @@ -54,9 +62,28 @@ export type MetricResult = { history?: boolean; }; result: { - value?: MetricValue; + value: MetricValue | null; timestamp: string; thresholdResult: ThresholdResult; }; error?: string; }; + +/** + * @public + */ +export type AggregatedMetricResult = { + id: string; + status: 'success' | 'error'; + metadata: { + title: string; + description: string; + type: MetricType; + history?: boolean; + }; + result: { + values: AggregatedMetricValue[]; + total: number; + timestamp: string; + }; +}; diff --git a/workspaces/scorecard/plugins/scorecard-common/src/types/threshold.ts b/workspaces/scorecard/plugins/scorecard-common/src/types/threshold.ts index 4ce1cf7a4b..eb904d0461 100644 --- a/workspaces/scorecard/plugins/scorecard-common/src/types/threshold.ts +++ b/workspaces/scorecard/plugins/scorecard-common/src/types/threshold.ts @@ -37,7 +37,7 @@ export type ThresholdConfig = { export type ThresholdResult = { status: 'success' | 'error'; definition: ThresholdConfig | undefined; - evaluation: string | undefined; // threshold key the expression evaluated to + evaluation: string | null; // threshold key the expression evaluated to error?: string; }; diff --git a/workspaces/scorecard/plugins/scorecard/__fixtures__/aggregatedScorecardData.ts b/workspaces/scorecard/plugins/scorecard/__fixtures__/aggregatedScorecardData.ts index 7b61ad0297..d881bee2be 100644 --- a/workspaces/scorecard/plugins/scorecard/__fixtures__/aggregatedScorecardData.ts +++ b/workspaces/scorecard/plugins/scorecard/__fixtures__/aggregatedScorecardData.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { AggregatedMetricResult } from '../src/utils/utils'; +import { AggregatedMetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; export const mockAggregatedScorecardSuccessData: AggregatedMetricResult[] = [ { @@ -24,7 +24,7 @@ export const mockAggregatedScorecardSuccessData: AggregatedMetricResult[] = [ title: 'GitHub open PRs', description: 'Current count of open Pull Requests for a given GitHub repository.', - type: 'object', + type: 'number', history: true, }, result: { @@ -35,7 +35,6 @@ export const mockAggregatedScorecardSuccessData: AggregatedMetricResult[] = [ ], total: 37, timestamp: '2024-01-15T10:30:00Z', - lastUpdated: '2024-01-15T10:30:00Z', }, }, { @@ -45,7 +44,7 @@ export const mockAggregatedScorecardSuccessData: AggregatedMetricResult[] = [ title: 'Jira open blocking tickets', description: 'Highlights the number of critical, blocking issues that are currently open in Jira.', - type: 'object', + type: 'number', history: true, }, result: { @@ -56,7 +55,6 @@ export const mockAggregatedScorecardSuccessData: AggregatedMetricResult[] = [ ], total: 4, timestamp: '2024-01-15T10:30:00Z', - lastUpdated: '2024-01-15T10:30:00Z', }, }, ]; diff --git a/workspaces/scorecard/plugins/scorecard/__fixtures__/scorecardData.ts b/workspaces/scorecard/plugins/scorecard/__fixtures__/scorecardData.ts index 57b6440910..d4605401a2 100644 --- a/workspaces/scorecard/plugins/scorecard/__fixtures__/scorecardData.ts +++ b/workspaces/scorecard/plugins/scorecard/__fixtures__/scorecardData.ts @@ -83,7 +83,7 @@ export const mockScorecardErrorData = [ history: true, }, result: { - value: undefined, + value: null, timestamp: '2025-08-08T10:00:00Z', thresholdResult: { definition: { diff --git a/workspaces/scorecard/plugins/scorecard/dev/index.tsx b/workspaces/scorecard/plugins/scorecard/dev/index.tsx index a385b88d2e..ecb9c56fc1 100644 --- a/workspaces/scorecard/plugins/scorecard/dev/index.tsx +++ b/workspaces/scorecard/plugins/scorecard/dev/index.tsx @@ -20,8 +20,10 @@ import { Page, Header, TabbedLayout } from '@backstage/core-components'; import { TestApiProvider } from '@backstage/test-utils'; import { getAllThemes } from '@red-hat-developer-hub/backstage-plugin-theme'; import type { Entity } from '@backstage/catalog-model'; -import type { MetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; -import type { AggregatedMetricResult } from '../src/utils/utils'; +import type { + MetricResult, + AggregatedMetricResult, +} from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { scorecardPlugin, EntityScorecardContent } from '../src/plugin'; import { scorecardTranslations } from '../src/translations'; diff --git a/workspaces/scorecard/plugins/scorecard/src/api/index.ts b/workspaces/scorecard/plugins/scorecard/src/api/index.ts index b8d71150cd..abe1eb4d69 100644 --- a/workspaces/scorecard/plugins/scorecard/src/api/index.ts +++ b/workspaces/scorecard/plugins/scorecard/src/api/index.ts @@ -20,9 +20,11 @@ import { DiscoveryApi, } from '@backstage/core-plugin-api'; import type { Entity } from '@backstage/catalog-model'; -import type { MetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; +import type { + MetricResult, + AggregatedMetricResult, +} from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { mockAggregatedScorecardSuccessData } from '../../__fixtures__/aggregatedScorecardData'; -import type { AggregatedMetricResult } from '../utils/utils'; export interface ScorecardApi { /** diff --git a/workspaces/scorecard/plugins/scorecard/src/components/Scorecard/EntityScorecardContent.tsx b/workspaces/scorecard/plugins/scorecard/src/components/Scorecard/EntityScorecardContent.tsx index 49b10bd3a8..6bfa059220 100644 --- a/workspaces/scorecard/plugins/scorecard/src/components/Scorecard/EntityScorecardContent.tsx +++ b/workspaces/scorecard/plugins/scorecard/src/components/Scorecard/EntityScorecardContent.tsx @@ -65,7 +65,7 @@ export const EntityScorecardContent = () => { {scorecards?.map((metric: MetricResult) => { // Check if metric data unavailable const isMetricDataError = - metric.status === 'error' || metric.result?.value === undefined; + metric.status === 'error' || metric.result?.value === null; // Check if threshold has an error const isThresholdError = diff --git a/workspaces/scorecard/plugins/scorecard/src/components/Scorecard/Scorecard.tsx b/workspaces/scorecard/plugins/scorecard/src/components/Scorecard/Scorecard.tsx index 619a9ada20..1ff515589a 100644 --- a/workspaces/scorecard/plugins/scorecard/src/components/Scorecard/Scorecard.tsx +++ b/workspaces/scorecard/plugins/scorecard/src/components/Scorecard/Scorecard.tsx @@ -37,7 +37,7 @@ interface ScorecardProps { loading: boolean; statusColor: string; StatusIcon: React.ElementType; - value?: MetricValue; + value: MetricValue | null; thresholds?: ThresholdResult; isMetricDataError?: boolean; metricDataError?: string; diff --git a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageCard.tsx b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageCard.tsx index 0039dadaf8..2301ab87be 100644 --- a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageCard.tsx +++ b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageCard.tsx @@ -31,7 +31,8 @@ import { useTheme } from '@mui/material/styles'; import { CardWrapper } from '../Common/CardWrapper'; import { CustomTooltip } from './CustomTooltip'; import CustomLegend from './CustomLegend'; -import type { AggregatedMetricResult, PieData } from '../../utils/utils'; +import type { PieData } from '../../utils/utils'; +import type { AggregatedMetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; export const ScorecardHomepageCard = ({ scorecard, diff --git a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageSection.tsx b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageSection.tsx index a051e28d9b..2b4de7f121 100644 --- a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageSection.tsx +++ b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageSection.tsx @@ -23,7 +23,7 @@ import CircularProgress from '@mui/material/CircularProgress'; import { ScorecardHomepageCard } from './ScorecardHomepageCard'; import PermissionRequiredState from '../Common/PermissionRequiredState'; import { useAggregatedScorecards } from '../../hooks/useAggregatedScorecards'; -import type { AggregatedMetricResult } from '../../utils/utils'; +import type { AggregatedMetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; export const ScorecardHomepageSection = () => { const { aggregatedScorecards, loadingData, error } = diff --git a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageCard.test.tsx b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageCard.test.tsx index 47716ab9c5..8076e1b3e2 100644 --- a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageCard.test.tsx +++ b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageCard.test.tsx @@ -19,7 +19,7 @@ import { ThemeProvider, createTheme } from '@mui/material/styles'; import { ScorecardHomepageCard } from '../ScorecardHomepageCard'; import { mockAggregatedScorecardSuccessData } from '../../../../__fixtures__/aggregatedScorecardData'; -import type { AggregatedMetricResult } from '../../../utils/utils'; +import type { AggregatedMetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; // Mock the child components jest.mock('../../Common/CardWrapper', () => ({ @@ -194,7 +194,7 @@ describe('ScorecardHomepageCard Component', () => { ...mockAggregatedScorecardSuccessData[0], result: { ...mockAggregatedScorecardSuccessData[0].result, - values: undefined, + values: [], total: 0, }, }; diff --git a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageSection.test.tsx b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageSection.test.tsx index f292b7806b..bc28400882 100644 --- a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageSection.test.tsx +++ b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageSection.test.tsx @@ -19,7 +19,7 @@ import { ThemeProvider, createTheme } from '@mui/material/styles'; import { ScorecardHomepageSection } from '../ScorecardHomepageSection'; import { mockAggregatedScorecardSuccessData } from '../../../../__fixtures__/aggregatedScorecardData'; -import type { AggregatedMetricResult } from '../../../utils/utils'; +import type { AggregatedMetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; // Mock the child components jest.mock('../../Common/PermissionRequiredState', () => { @@ -130,14 +130,13 @@ describe('ScorecardHomepageSection Component', () => { metadata: { title: 'Third Scorecard', description: 'Third description', - type: 'object', + type: 'number', history: true, }, result: { values: [{ count: 5, name: 'success' }], total: 5, timestamp: '2024-01-15T10:30:00Z', - lastUpdated: '2024-01-15T10:30:00Z', }, }, ]; diff --git a/workspaces/scorecard/plugins/scorecard/src/hooks/useAggregatedScorecards.tsx b/workspaces/scorecard/plugins/scorecard/src/hooks/useAggregatedScorecards.tsx index fb9dd979a6..9eba537880 100644 --- a/workspaces/scorecard/plugins/scorecard/src/hooks/useAggregatedScorecards.tsx +++ b/workspaces/scorecard/plugins/scorecard/src/hooks/useAggregatedScorecards.tsx @@ -19,7 +19,7 @@ import { useApi } from '@backstage/core-plugin-api'; import useAsync from 'react-use/lib/useAsync'; import { scorecardApiRef } from '../api'; -import { AggregatedMetricResult } from '../utils/utils'; +import { AggregatedMetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; export const useAggregatedScorecards = () => { const scorecardApi = useApi(scorecardApiRef); diff --git a/workspaces/scorecard/plugins/scorecard/src/utils/utils.ts b/workspaces/scorecard/plugins/scorecard/src/utils/utils.ts index 95c0ca9f13..78e08d30f9 100644 --- a/workspaces/scorecard/plugins/scorecard/src/utils/utils.ts +++ b/workspaces/scorecard/plugins/scorecard/src/utils/utils.ts @@ -39,7 +39,7 @@ export const getStatusConfig = ({ thresholdStatus, metricStatus, }: { - evaluation: string | undefined; + evaluation: string | null; thresholdStatus?: 'success' | 'error'; metricStatus?: 'success' | 'error'; }): StatusConfig => { @@ -57,25 +57,3 @@ export const getStatusConfig = ({ return { color: 'success.main', icon: CheckCircleOutlineIcon }; } }; - -export type AggregatedMetricValue = { - count: number; - name: 'success' | 'warning' | 'error'; -}; - -export type AggregatedMetricResult = { - id: string; - status: 'success' | 'error'; - metadata: { - title: string; - description: string; - type: 'object'; - history?: boolean; - }; - result: { - values?: AggregatedMetricValue[]; - total: number; - timestamp: string; - lastUpdated: string; - }; -};