From 25fe30f2487d30477e958fc60466e3af86a6f1c3 Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Tue, 23 Dec 2025 21:36:42 +0100 Subject: [PATCH 1/3] feat(scorecard): implement endpoint to aggregate metrics for scorecard --- .../scorecard/.changeset/cold-experts-stop.md | 6 + .../__fixtures__/mockDatabaseMetricValues.ts | 10 + .../__fixtures__/mockEntityBuilder.ts | 52 +++ .../mockMetricProvidersRegistry.ts | 7 +- .../src/database/DatabaseMetricValues.test.ts | 67 ++++ .../src/database/DatabaseMetricValues.ts | 19 + .../providers/MetricProvidersRegistry.test.ts | 4 +- .../src/service/CatalogMetricService.test.ts | 134 ++++++- .../src/service/CatalogMetricService.ts | 74 +++- .../src/service/router.test.ts | 218 +++++++++- .../scorecard-backend/src/service/router.ts | 61 ++- .../utils/aggregateMetricsByStatus.test.ts | 379 ++++++++++++++++++ .../src/utils/aggregateMetricsByStatus.ts | 44 ++ .../src/utils/getEntitiesOwnedByUser.test.ts | 258 ++++++++++++ .../src/utils/getEntitiesOwnedByUser.ts | 74 ++++ .../utils/parseCommaSeparatedString.test.ts | 47 +++ .../utils/parseCommaSeparatedString.ts} | 20 +- .../validateCatalogMetricsSchema.test.ts | 82 ++++ .../validateCatalogMetricsSchema.ts | 30 ++ .../plugins/scorecard-common/report.api.md | 23 ++ .../scorecard-common/src/types/Metric.ts | 27 ++ 21 files changed, 1605 insertions(+), 31 deletions(-) create mode 100644 workspaces/scorecard/.changeset/cold-experts-stop.md create mode 100644 workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntityBuilder.ts create mode 100644 workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.test.ts create mode 100644 workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.ts create mode 100644 workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.test.ts create mode 100644 workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.ts create mode 100644 workspaces/scorecard/plugins/scorecard-backend/src/utils/parseCommaSeparatedString.test.ts rename workspaces/scorecard/plugins/scorecard-backend/{__fixtures__/mockEntities.ts => src/utils/parseCommaSeparatedString.ts} (68%) create mode 100644 workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts create mode 100644 workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.ts diff --git a/workspaces/scorecard/.changeset/cold-experts-stop.md b/workspaces/scorecard/.changeset/cold-experts-stop.md new file mode 100644 index 0000000000..b70089144d --- /dev/null +++ b/workspaces/scorecard/.changeset/cold-experts-stop.md @@ -0,0 +1,6 @@ +--- +'@red-hat-developer-hub/backstage-plugin-scorecard-backend': minor +'@red-hat-developer-hub/backstage-plugin-scorecard-common': minor +--- + +Implemented endpoint to aggregate metrics for scorecard diff --git a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts index 9acda55f14..97d7d463af 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts @@ -21,32 +21,42 @@ type BuildMockDatabaseMetricValuesParams = { metricValues?: DbMetricValue[]; latestEntityMetric?: DbMetricValue[]; countOfExpiredMetrics?: number; + latestAggregatedEntityMetric?: DbMetricValue[]; }; export const mockDatabaseMetricValues = { createMetricValues: jest.fn(), readLatestEntityMetricValues: jest.fn(), cleanupExpiredMetrics: jest.fn(), + readLatestEntityMetricValuesByEntityRefs: jest.fn(), } as unknown as jest.Mocked; export const buildMockDatabaseMetricValues = ({ metricValues, latestEntityMetric, countOfExpiredMetrics, + latestAggregatedEntityMetric, }: 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 readLatestEntityMetricValuesByEntityRefs = latestAggregatedEntityMetric + ? jest.fn().mockResolvedValue(latestAggregatedEntityMetric) + : mockDatabaseMetricValues.readLatestEntityMetricValuesByEntityRefs; + return { createMetricValues, readLatestEntityMetricValues, cleanupExpiredMetrics, + readLatestEntityMetricValuesByEntityRefs, } 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..e201925413 --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntityBuilder.ts @@ -0,0 +1,52 @@ +/* + * 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 } from '@backstage/catalog-model'; + +export class MockEntityBuilder { + private kind: string = 'Component'; + private metadata: Entity['metadata'] = { + name: 'default-component', + namespace: 'default', + }; + private spec: Entity['spec'] = { + owner: 'guests', + }; + + withKind(kind: string): MockEntityBuilder { + this.kind = kind; + return this; + } + + withMetadata(metadata: Entity['metadata']): MockEntityBuilder { + this.metadata = metadata; + return this; + } + + withSpec(spec: Entity['spec']): MockEntityBuilder { + this.spec = spec; + return this; + } + + build(): Entity { + return { + apiVersion: 'backstage.io/v1alpha1', + kind: this.kind, + metadata: this.metadata, + spec: this.spec, + }; + } +} 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/src/database/DatabaseMetricValues.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts index 85c92b299f..4841307574 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts @@ -32,6 +32,7 @@ const metricValues: Omit[] = [ value: 41, timestamp: new Date('2023-01-01T00:00:00Z'), error_message: undefined, + status: 'success', }, { catalog_entity_ref: 'component:default/another-service', @@ -39,6 +40,7 @@ const metricValues: Omit[] = [ value: 25, timestamp: new Date('2023-01-01T00:00:00Z'), error_message: undefined, + status: 'success', }, { catalog_entity_ref: 'component:default/another-service', @@ -191,4 +193,69 @@ describe('DatabaseMetricValues', () => { }, ); }); + + describe('readLatestEntityMetricValuesByEntityRefs', () => { + it.each(databases.eachSupportedId())( + 'should return latest metric values 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, + }, + { + ...metricValues[1], + timestamp: baseTime, + }, + { + ...metricValues[2], + timestamp: laterTime, + }, + { + ...metricValues[2], + timestamp: laterTime, + value: 10, + error_message: undefined, + status: 'success', + }, + ]); + + const result = await db.readLatestEntityMetricValuesByEntityRefs( + [ + 'component:default/test-service', + 'component:default/another-service', + ], + ['github.metric1', 'github.metric2'], + ); + + expect(result).toHaveLength(3); + + const testServiceMetric1 = result.find( + r => + r.metric_id === 'github.metric1' && + r.catalog_entity_ref === 'component:default/test-service', + ); + const anotherServiceMetric1 = result.find( + r => + r.metric_id === 'github.metric1' && + r.catalog_entity_ref === 'component:default/another-service', + ); + + const anotherServiceMetric2 = result.find( + r => + r.metric_id === 'github.metric2' && + r.catalog_entity_ref === 'component:default/another-service', + ); + + expect(testServiceMetric1?.value).toBe(41); + expect(anotherServiceMetric1?.value).toBe(25); + expect(anotherServiceMetric2?.value).toBe(10); + }, + ); + }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts index 652a4b2d2a..c56fbd3fac 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts @@ -58,4 +58,23 @@ export class DatabaseMetricValues { .where('timestamp', '<', olderThan) .del(); } + + /** + * Get the latest metric values for multiple entities and metrics + */ + async readLatestEntityMetricValuesByEntityRefs( + catalog_entity_refs: string[], + metric_ids: string[], + ): Promise { + return await this.dbClient(this.tableName) + .select('*') + .whereIn( + 'id', + this.dbClient(this.tableName) + .max('id') + .whereIn('metric_id', metric_ids) + .whereIn('catalog_entity_ref', catalog_entity_refs) + .groupBy('metric_id', 'catalog_entity_ref'), + ); + } } 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/service/CatalogMetricService.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts index f32c62abed..5d01e64581 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, @@ -32,9 +31,12 @@ import { Metric } from '@red-hat-developer-hub/backstage-plugin-scorecard-common import * as thresholdUtils from '../utils/mergeEntityAndProviderThresholds'; import { DbMetricValue } from '../database/types'; import { mockThresholdRules } from '../../__fixtures__/mockThresholdRules'; +import * as aggregateMetricsByStatusModule from '../utils/aggregateMetricsByStatus'; +import { MockEntityBuilder } from '../../__fixtures__/mockEntityBuilder'; jest.mock('../utils/mergeEntityAndProviderThresholds'); jest.mock('../permissions/permissionUtils'); +jest.mock('../utils/aggregateMetricsByStatus'); const provider = new MockNumberProvider('github.important_metric', 'github'); @@ -50,6 +52,27 @@ const latestEntityMetric = [ } as DbMetricValue, ] as DbMetricValue[]; +const latestAggregatedEntityMetric = [ + { + id: 1, + catalog_entity_ref: 'component:default/test-component', + metric_id: 'github.important_metric', + value: 42, + timestamp: new Date('2024-01-15T12:00:00.000Z'), + error_message: undefined, + status: 'success', + } as DbMetricValue, + { + id: 2, + catalog_entity_ref: 'component:default/test-component-2', + metric_id: 'github.important_metric', + value: 11, + timestamp: new Date('2024-01-15T12:00:00.000Z'), + error_message: undefined, + status: 'warning', + } as DbMetricValue, +] as DbMetricValue[]; + const metricsList = [ { id: 'github.important_metric' }, { id: 'github.number_metric' }, @@ -61,6 +84,9 @@ describe('CatalogMetricService', () => { let mockedRegistry: jest.Mocked; let mockedDatabase: jest.Mocked; let service: CatalogMetricService; + let aggregateMetricsByStatusSpy: jest.SpyInstance; + + const mockEntity = new MockEntityBuilder().build(); beforeEach(() => { mockedCatalog = catalogServiceMock.mock(); @@ -77,7 +103,10 @@ describe('CatalogMetricService', () => { metricsList, }); - mockedDatabase = buildMockDatabaseMetricValues({ latestEntityMetric }); + mockedDatabase = buildMockDatabaseMetricValues({ + latestEntityMetric, + latestAggregatedEntityMetric, + }); (permissionUtils.filterAuthorizedMetrics as jest.Mock).mockReturnValue([ { id: 'github.important_metric' }, @@ -89,6 +118,19 @@ describe('CatalogMetricService', () => { rules: mockThresholdRules, }); + aggregateMetricsByStatusSpy = jest + .spyOn(aggregateMetricsByStatusModule, 'aggregateMetricsByStatus') + .mockReturnValue({ + 'github.important_metric': { + values: { + success: 1, + warning: 1, + error: 0, + }, + total: 2, + }, + }); + service = new CatalogMetricService({ catalog: mockedCatalog, auth: mockedAuth, @@ -112,6 +154,12 @@ describe('CatalogMetricService', () => { }); }); + describe('getCatalogService', () => { + it('should return the catalog service', () => { + expect(service.getCatalogService()).toBe(mockedCatalog); + }); + }); + describe('getLatestEntityMetrics', () => { it('should handle multiple metrics correctly', async () => { const secondProvider = new MockNumberProvider( @@ -405,4 +453,86 @@ describe('CatalogMetricService', () => { ); }); }); + + 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([ + { + 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', + }, + }, + ]); + }); + + 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(mockedRegistry.listMetrics).toHaveBeenCalledWith([ + 'github.important_metric', + ]); + }); + + it('should read latest entity metric values by entity refs', async () => { + await service.getAggregatedMetricsByEntityRefs( + [ + 'component:default/test-component', + 'component:default/test-component-2', + ], + ['github.important_metric'], + ); + + expect( + mockedDatabase.readLatestEntityMetricValuesByEntityRefs, + ).toHaveBeenCalledWith( + [ + 'component:default/test-component', + 'component:default/test-component-2', + ], + ['github.important_metric'], + ); + }); + + it('should aggregate metrics by status', async () => { + await service.getAggregatedMetricsByEntityRefs( + [ + 'component:default/test-component', + 'component:default/test-component-2', + ], + ['github.important_metric'], + ); + + expect(aggregateMetricsByStatusSpy).toHaveBeenCalledWith( + latestAggregatedEntityMetric, + ); + }); + }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts index 25096ac77d..4697282c77 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -15,6 +15,8 @@ */ import { + AggregatedMetricResult, + AggregatedMetricValue, MetricResult, ThresholdConfig, } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; @@ -30,14 +32,20 @@ import { import { CatalogService } from '@backstage/plugin-catalog-node'; import { DatabaseMetricValues } from '../database/DatabaseMetricValues'; import { mergeEntityAndProviderThresholds } from '../utils/mergeEntityAndProviderThresholds'; +import { aggregateMetricsByStatus } from '../utils/aggregateMetricsByStatus'; -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; @@ -51,6 +59,15 @@ export class CatalogMetricService { this.database = options.database; } + /** + * Get the catalog service + * + * @returns CatalogService + */ + getCatalogService(): CatalogService { + return this.catalog; + } + /** * Get latest metric results for a specific catalog entity and metric providers. * @@ -74,9 +91,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, @@ -136,4 +151,55 @@ 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[], + ): Promise { + const metricsToFetch = this.registry.listMetrics(metricIds); + + const metricResults = + await this.database.readLatestEntityMetricValuesByEntityRefs( + entityRefs, + metricsToFetch.map(m => m.id), + ); + + const aggregatedMetrics = aggregateMetricsByStatus(metricResults); + + return Object.entries(aggregatedMetrics).map(([metricId, metricData]) => { + const values: AggregatedMetricValue[] = []; + + for (const [key, count] of Object.entries(metricData.values)) { + const name = key as 'success' | 'warning' | 'error'; + values.push({ count, name }); + } + + 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, + total: metricData.total, + timestamp: new Date(metricResults[0].timestamp).toISOString(), + }, + }; + }); + } } 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..8539118b50 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(), @@ -86,10 +106,18 @@ 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(), + httpAuth: httpAuthMock, permissions: permissionsMock, }); app = express(); @@ -132,6 +160,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 +371,191 @@ describe('createRouter', () => { expect(response.body.error.message).toContain('Invalid query parameters'); }); }); + + describe('GET /metrics/catalog/aggregated', () => { + 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; + + beforeEach(() => { + 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, 'getCatalogService') + .mockReturnValue(mockCatalog); + + 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', + ); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should return 403 Unauthorized when DENY permissions', async () => { + permissionsMock.authorizeConditional.mockResolvedValue([ + { result: AuthorizeResult.DENY }, + ]); + const result = await request(app).get('/metrics/catalog/aggregated'); + + expect(result.statusCode).toBe(403); + expect(result.body.error.name).toEqual('NotAllowedError'); + }); + + it('should return 403 NotAllowedError when user entity reference is not found', async () => { + httpAuthMock.credentials.mockResolvedValue(undefined as any); + const result = await request(app).get('/metrics/catalog/aggregated'); + + console.log(result.body); + expect(result.statusCode).toBe(403); + expect(result.body.error.name).toEqual('NotAllowedError'); + }); + + it('should handle multiple metricIds parameter', async () => { + const response = await request(app).get( + '/metrics/catalog/aggregated?metricIds=github.open_prs,jira.open_issues', + ); + + expect(response.status).toBe(200); + expect( + catalogMetricService.getAggregatedMetricsByEntityRefs, + ).toHaveBeenCalledWith( + ['component:default/my-service', 'component:default/my-other-service'], + ['github.open_prs', 'jira.open_issues'], + ); + expect(response.body).toEqual(mockAggregatedMetricResults); + }); + + it('should handle single metricIds parameter', async () => { + const response = await request(app).get( + '/metrics/catalog/aggregated?metricIds=github.open_prs', + ); + + expect(response.status).toBe(200); + expect( + catalogMetricService.getAggregatedMetricsByEntityRefs, + ).toHaveBeenCalledWith( + ['component:default/my-service', 'component:default/my-other-service'], + ['github.open_prs'], + ); + expect(response.body).toEqual(mockAggregatedMetricResults); + }); + + it('should get entities owned by user', async () => { + const response = await request(app).get('/metrics/catalog/aggregated'); + + 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(app).get('/metrics/catalog/aggregated'); + + expect(response.status).toBe(200); + expect(response.body).toEqual([]); + }); + + it('should check entity access for each entity owned by user', async () => { + const response = await request(app).get('/metrics/catalog/aggregated'); + + expect(checkEntityAccessSpy).toHaveBeenCalledTimes(2); + expect(response.status).toBe(200); + expect(response.body).toEqual(mockAggregatedMetricResults); + }); + + it('should parse metricIds parameter correctly', async () => { + const response = await request(app).get( + '/metrics/catalog/aggregated?metricIds=github.open_prs,jira.open_issues', + ); + + expect(response.status).toBe(200); + expect( + catalogMetricService.getAggregatedMetricsByEntityRefs, + ).toHaveBeenCalledWith( + ['component:default/my-service', 'component:default/my-other-service'], + ['github.open_prs', 'jira.open_issues'], + ); + }); + + it('should set parsed metricIds to undefined when metricIds parameter is not string', async () => { + const response = await request(app).get( + '/metrics/catalog/aggregated?anotherParameter=value', + ); + + expect(response.status).toBe(200); + expect( + catalogMetricService.getAggregatedMetricsByEntityRefs, + ).toHaveBeenCalledWith( + ['component:default/my-service', 'component:default/my-other-service'], + undefined, + ); + }); + }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index e94f452d77..722b4c7a60 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -35,6 +35,9 @@ 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; @@ -124,23 +127,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 +147,47 @@ export async function createRouter({ res.json(results); }); + router.get('/metrics/catalog/aggregated', async (req, res) => { + const { metricIds } = req.query; + + await authorizeConditional(req, scorecardMetricReadPermission); + + validateCatalogMetricsSchema(req.query); + + const credentials = await httpAuth.credentials(req, { allow: ['user'] }); + const userEntityRef = credentials?.principal?.userEntityRef; + + if (!userEntityRef) { + throw new NotAllowedError('User entity reference not found'); + } + + const entitiesOwnedByAUser = await getEntitiesOwnedByUser(userEntityRef, { + catalog: catalogMetricService.getCatalogService(), + credentials, + }); + + if (entitiesOwnedByAUser.length === 0) { + res.json([]); + return; + } + + for (const entityRef of entitiesOwnedByAUser) { + await checkEntityAccess(entityRef, req, permissions, httpAuth); + } + + const metricIdsParsed = + typeof metricIds === 'string' + ? parseCommaSeparatedString(metricIds) + : undefined; + + const aggregatedMetrics = + await catalogMetricService.getAggregatedMetricsByEntityRefs( + entitiesOwnedByAUser, + metricIdsParsed, + ); + + res.json(aggregatedMetrics); + }); + return router; } diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.test.ts new file mode 100644 index 0000000000..aa75f73ede --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.test.ts @@ -0,0 +1,379 @@ +/* + * 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 { aggregateMetricsByStatus } from './aggregateMetricsByStatus'; +import { DbMetricValue } from '../database/types'; + +describe('aggregateMetricsByStatus', () => { + const createMetric = ( + metricId: string, + status: 'success' | 'warning' | 'error', + value: any = { count: 1 }, + ): DbMetricValue => ({ + id: 1, + catalog_entity_ref: 'component:default/test', + metric_id: metricId, + value, + timestamp: new Date(), + status, + }); + + it('should return empty object when metrics array is empty', () => { + const result = aggregateMetricsByStatus([]); + expect(result).toEqual({}); + }); + + describe('when metrics have valid status and value', () => { + it('should aggregate single success metric', () => { + const metrics = [createMetric('metric1', 'success')]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 1, + warning: 0, + error: 0, + }, + total: 1, + }, + }); + }); + + it('should aggregate single warning metric', () => { + const metrics = [createMetric('metric1', 'warning')]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 0, + warning: 1, + error: 0, + }, + total: 1, + }, + }); + }); + + it('should aggregate single error metric', () => { + const metrics = [createMetric('metric1', 'error')]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 0, + warning: 0, + error: 1, + }, + total: 1, + }, + }); + }); + + it('should aggregate multiple metrics with same metric_id', () => { + const metrics = [ + createMetric('metric1', 'success'), + createMetric('metric1', 'success'), + createMetric('metric1', 'warning'), + createMetric('metric1', 'error'), + ]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 2, + warning: 1, + error: 1, + }, + total: 4, + }, + }); + }); + + it('should aggregate multiple metrics with different metric_ids', () => { + const metrics = [ + createMetric('metric1', 'success'), + createMetric('metric2', 'warning'), + createMetric('metric3', 'error'), + ]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 1, + warning: 0, + error: 0, + }, + total: 1, + }, + metric2: { + values: { + success: 0, + warning: 1, + error: 0, + }, + total: 1, + }, + metric3: { + values: { + success: 0, + warning: 0, + error: 1, + }, + total: 1, + }, + }); + }); + + it('should aggregate complex scenario with multiple metric_ids and statuses', () => { + const metrics = [ + createMetric('metric1', 'success'), + createMetric('metric1', 'success'), + createMetric('metric1', 'warning'), + createMetric('metric2', 'error'), + createMetric('metric2', 'error'), + createMetric('metric2', 'success'), + createMetric('metric3', 'warning'), + ]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 2, + warning: 1, + error: 0, + }, + total: 3, + }, + metric2: { + values: { + success: 1, + warning: 0, + error: 2, + }, + total: 3, + }, + metric3: { + values: { + success: 0, + warning: 1, + error: 0, + }, + total: 1, + }, + }); + }); + }); + + describe('when metrics have invalid status or value', () => { + it('should skip metrics with null value', () => { + const metrics: DbMetricValue[] = [ + createMetric('metric1', 'success', null), + createMetric('metric1', 'warning', { count: 1 }), + ]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 0, + warning: 1, + error: 0, + }, + total: 1, + }, + }); + }); + + it('should skip metrics without status', () => { + const metrics: DbMetricValue[] = [ + // @ts-expect-error - for testing + createMetric('metric1', undefined, { count: 1 }), + createMetric('metric1', 'success'), + ]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 1, + warning: 0, + error: 0, + }, + total: 1, + }, + }); + }); + + it('should skip metrics with both null value and no status', () => { + const metrics: DbMetricValue[] = [ + { + id: 1, + catalog_entity_ref: 'component:default/test', + metric_id: 'metric1', + value: undefined, + timestamp: new Date(), + }, + createMetric('metric1', 'success'), + ]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 1, + warning: 0, + error: 0, + }, + total: 1, + }, + }); + }); + + it('should handle undefined value as valid (not null)', () => { + const metrics: DbMetricValue[] = [ + createMetric('metric1', 'success', undefined), + ]; + const result = aggregateMetricsByStatus(metrics); + + // undefined !== null, so it should be included + expect(result).toEqual({ + metric1: { + values: { + success: 1, + warning: 0, + error: 0, + }, + total: 1, + }, + }); + }); + + it('should handle mixed valid and invalid metrics', () => { + const metrics: DbMetricValue[] = [ + createMetric('metric1', 'success'), + createMetric('metric1', 'warning', null), + // @ts-expect-error - for testing + createMetric('metric1', undefined, { count: 1 }), + createMetric('metric1', 'error'), + createMetric('metric2', 'success', null), + createMetric('metric2', 'warning'), + ]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 1, + warning: 0, + error: 1, + }, + total: 2, + }, + metric2: { + values: { + success: 0, + warning: 1, + error: 0, + }, + total: 1, + }, + }); + }); + }); + + describe('when all metrics are invalid', () => { + it('should return empty object when all metrics have null value', () => { + const metrics: DbMetricValue[] = [ + createMetric('metric1', 'success', null), + createMetric('metric2', 'warning', null), + ]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({}); + }); + + it('should return empty object when all metrics have no status', () => { + const metrics: DbMetricValue[] = [ + // @ts-expect-error - for testing + createMetric('metric2', undefined, { count: 2 }), + // @ts-expect-error - for testing + createMetric('metric2', undefined, { count: 2 }), + ]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({}); + }); + }); + + describe('edge cases', () => { + it('should handle zero value as valid (not null)', () => { + const metrics: DbMetricValue[] = [createMetric('metric1', 'success', 0)]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 1, + warning: 0, + error: 0, + }, + total: 1, + }, + }); + }); + + it('should handle empty string value as valid (not null)', () => { + const metrics: DbMetricValue[] = [createMetric('metric1', 'success', '')]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 1, + warning: 0, + error: 0, + }, + total: 1, + }, + }); + }); + + it('should handle false value as valid (not null)', () => { + const metrics: DbMetricValue[] = [ + createMetric('metric1', 'success', false), + ]; + + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 1, + warning: 0, + error: 0, + }, + total: 1, + }, + }); + }); + }); +}); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.ts b/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.ts new file mode 100644 index 0000000000..ca1424b0f9 --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.ts @@ -0,0 +1,44 @@ +/* + * 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 { DbMetricValue } from '../database/types'; +import { AggregatedMetricsByStatus } from '../service/CatalogMetricService'; + +export function aggregateMetricsByStatus( + metrics: DbMetricValue[], +): AggregatedMetricsByStatus { + const aggregatedMetrics: AggregatedMetricsByStatus = {}; + + for (const metric of metrics) { + if (metric.status && metric.value !== null) { + if (!Object.hasOwn(aggregatedMetrics, metric.metric_id)) { + aggregatedMetrics[metric.metric_id] = { + values: { + success: 0, + warning: 0, + error: 0, + }, + total: 0, + }; + } + + aggregatedMetrics[metric.metric_id].values[metric.status]++; + aggregatedMetrics[metric.metric_id].total++; + } + } + + return aggregatedMetrics; +} 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..992013d15e --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.test.ts @@ -0,0 +1,258 @@ +/* + * 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_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), + parseEntityRef: jest.fn(actual.parseEntityRef), + }; +}); + +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: 'default' }) + .build(); + userOwnedEntity = new MockEntityBuilder() + .withMetadata({ name: 'user-component', namespace: 'default' }) + .withSpec({ owner: 'user:default/test-user' }) + .build(); + }); + + afterEach(() => { + jest.clearAllMocks(); + jest.restoreAllMocks(); + }); + + it('should throw NotFoundError when user entity is not found', async () => { + mockedCatalog.getEntityByRef.mockResolvedValue(undefined); + + await expect( + getEntitiesOwnedByUser('user:default/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); + mockedCatalog.getEntities.mockResolvedValue({ + items: [userOwnedEntity], + }); + }); + + it('should call stringifyEntityRef three times', async () => { + await getEntitiesOwnedByUser('user:default/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(catalogModel.stringifyEntityRef).toHaveBeenCalledTimes(2); + expect(catalogModel.stringifyEntityRef).toHaveBeenNthCalledWith( + 1, + userEntity, + ); + expect(catalogModel.stringifyEntityRef).toHaveBeenNthCalledWith( + 2, + userOwnedEntity, + ); + }); + + it('should not parse memberOf entities', async () => { + await getEntitiesOwnedByUser('user:default/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(catalogModel.parseEntityRef).not.toHaveBeenCalled(); + }); + + it('should call getEntities with owned by user relation', async () => { + await getEntitiesOwnedByUser('user:default/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(mockedCatalog.getEntities).toHaveBeenCalledTimes(1); + expect(mockedCatalog.getEntities).toHaveBeenCalledWith( + { + filter: { + [`relations.${RELATION_OWNED_BY}`]: 'user:default/test-user', + }, + }, + { credentials: mockCredentials }, + ); + }); + + it('should return entities owned by user only', async () => { + mockedCatalog.getEntities.mockResolvedValue({ items: [userOwnedEntity] }); + + const result = await getEntitiesOwnedByUser('user:default/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(result).toEqual(['component:default/user-component']); + }); + + it('should return empty array when user owns no entities', async () => { + mockedCatalog.getEntities.mockResolvedValue({ items: [] }); + + const result = await getEntitiesOwnedByUser('user:default/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(result).toEqual([]); + }); + }); + + describe('when user has memberOf groups', () => { + let userEntityWithMemberOf: Entity; + let groupOwnedEntity: Entity; + + beforeEach(() => { + userEntityWithMemberOf = new MockEntityBuilder() + .withKind('User') + .withMetadata({ name: 'test-user', namespace: 'default' }) + .withSpec({ memberOf: ['group:default/developers'] }) + .build(); + groupOwnedEntity = new MockEntityBuilder() + .withMetadata({ name: 'group-component', namespace: 'default' }) + .build(); + + mockedCatalog.getEntityByRef.mockResolvedValue(userEntityWithMemberOf); + mockedCatalog.getEntities + .mockResolvedValueOnce({ items: [userOwnedEntity] }) + .mockResolvedValueOnce({ items: [groupOwnedEntity] }); + }); + + it('should call stringifyEntityRef four times', async () => { + await getEntitiesOwnedByUser('user:default/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(catalogModel.stringifyEntityRef).toHaveBeenCalledTimes(4); + expect(catalogModel.stringifyEntityRef).toHaveBeenNthCalledWith( + 1, + userEntityWithMemberOf, + ); + expect(catalogModel.stringifyEntityRef).toHaveBeenNthCalledWith(2, { + kind: 'group', + name: 'developers', + namespace: 'default', + }); + expect(catalogModel.stringifyEntityRef).toHaveBeenNthCalledWith( + 3, + userOwnedEntity, + ); + expect(catalogModel.stringifyEntityRef).toHaveBeenNthCalledWith( + 4, + groupOwnedEntity, + ); + }); + + it('should call parseEntityRef once', async () => { + await getEntitiesOwnedByUser('user:default/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(catalogModel.parseEntityRef).toHaveBeenCalledTimes(1); + expect(catalogModel.parseEntityRef).toHaveBeenCalledWith( + 'group:default/developers', + { defaultKind: 'Group', defaultNamespace: 'default' }, + ); + }); + + it('should call getEntities with owned by user relation', async () => { + await getEntitiesOwnedByUser('user:default/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(mockedCatalog.getEntities).toHaveBeenCalledTimes(2); + expect(mockedCatalog.getEntities).toHaveBeenNthCalledWith( + 1, + { + filter: { + [`relations.${RELATION_OWNED_BY}`]: 'user:default/test-user', + }, + }, + { credentials: mockCredentials }, + ); + expect(mockedCatalog.getEntities).toHaveBeenNthCalledWith( + 2, + { + filter: { + [`relations.${RELATION_OWNED_BY}`]: 'group:default/developers', + }, + }, + { credentials: mockCredentials }, + ); + }); + + it('should return entities owned by user and groups', async () => { + const result = await getEntitiesOwnedByUser('user:default/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(result).toEqual([ + 'component:default/user-component', + 'component:default/group-component', + ]); + }); + + it('should return empty array when user and groups owns no entities', async () => { + mockedCatalog.getEntities.mockReset(); + mockedCatalog.getEntities + .mockResolvedValueOnce({ items: [] }) + .mockResolvedValueOnce({ items: [] }); + + const result = await getEntitiesOwnedByUser('user:default/test-user', { + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(result).toEqual([]); + expect(mockedCatalog.getEntities).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..163b76e438 --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.ts @@ -0,0 +1,74 @@ +/* + * 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 { + parseEntityRef, + RELATION_OWNED_BY, + stringifyEntityRef, +} from '@backstage/catalog-model'; + +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 entitiesOwnedByUser: string[] = [stringifyEntityRef(userEntity)]; + + const memberOf = userEntity.spec?.memberOf; + if (Array.isArray(memberOf) && memberOf.length > 0) { + for (const entityRef of memberOf) { + if (typeof entityRef === 'string') { + const parsedEntityRef = parseEntityRef(entityRef, { + defaultKind: 'Group', + defaultNamespace: userEntity.metadata.namespace, + }); + + entitiesOwnedByUser.push(stringifyEntityRef(parsedEntityRef)); + } + } + } + + const entitiesOwnedByUserAndGroups: string[] = []; + + for (const entityRef of entitiesOwnedByUser) { + const entities = await options.catalog.getEntities( + { + filter: { + [`relations.${RELATION_OWNED_BY}`]: entityRef, + }, + }, + { credentials: options.credentials }, + ); + + const entityRefs = entities.items.map(entity => stringifyEntityRef(entity)); + entitiesOwnedByUserAndGroups.push(...entityRefs); + } + + 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..98c210c8e5 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; diff --git a/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts b/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts index 6ec46e860e..1b01f41b74 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 */ @@ -60,3 +68,22 @@ export type MetricResult = { }; 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; + }; +}; From 4533e978652d55cc7391daeab4bf56407668c713 Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Mon, 5 Jan 2026 19:13:28 +0100 Subject: [PATCH 2/3] fix(scorecard): metric types and sonarqube issues --- .../scorecard/.changeset/cold-experts-stop.md | 6 - .../.changeset/seven-guests-search.md | 41 +++ .../__fixtures__/mockEntityBuilder.ts | 6 +- .../src/database/DatabaseMetricValues.test.ts | 9 +- .../src/database/DatabaseMetricValues.ts | 4 +- .../scorecard-backend/src/database/types.ts | 16 +- .../src/permissions/permissionUtils.test.ts | 6 +- .../src/permissions/permissionUtils.ts | 2 +- .../tasks/PullMetricsByProviderTask.ts | 8 +- .../src/service/CatalogMetricService.test.ts | 79 +++-- .../src/service/CatalogMetricService.ts | 14 +- .../src/service/router.test.ts | 4 + .../scorecard-backend/src/service/router.ts | 6 +- .../utils/aggregateMetricsByStatus.test.ts | 284 ++++++------------ .../plugins/scorecard-common/report.api.md | 6 +- .../scorecard-common/src/types/Metric.ts | 6 +- .../scorecard-common/src/types/threshold.ts | 2 +- .../scorecard/__fixtures__/scorecardData.ts | 2 +- .../Scorecard/EntityScorecardContent.tsx | 2 +- .../src/components/Scorecard/Scorecard.tsx | 2 +- .../plugins/scorecard/src/utils/utils.ts | 2 +- 21 files changed, 228 insertions(+), 279 deletions(-) delete mode 100644 workspaces/scorecard/.changeset/cold-experts-stop.md create mode 100644 workspaces/scorecard/.changeset/seven-guests-search.md diff --git a/workspaces/scorecard/.changeset/cold-experts-stop.md b/workspaces/scorecard/.changeset/cold-experts-stop.md deleted file mode 100644 index b70089144d..0000000000 --- a/workspaces/scorecard/.changeset/cold-experts-stop.md +++ /dev/null @@ -1,6 +0,0 @@ ---- -'@red-hat-developer-hub/backstage-plugin-scorecard-backend': minor -'@red-hat-developer-hub/backstage-plugin-scorecard-common': minor ---- - -Implemented endpoint to aggregate metrics for scorecard 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/plugins/scorecard-backend/__fixtures__/mockEntityBuilder.ts b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntityBuilder.ts index e201925413..aa09d001d7 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntityBuilder.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntityBuilder.ts @@ -26,17 +26,17 @@ export class MockEntityBuilder { owner: 'guests', }; - withKind(kind: string): MockEntityBuilder { + withKind(kind: string): this { this.kind = kind; return this; } - withMetadata(metadata: Entity['metadata']): MockEntityBuilder { + withMetadata(metadata: Entity['metadata']): this { this.metadata = metadata; return this; } - withSpec(spec: Entity['spec']): MockEntityBuilder { + withSpec(spec: Entity['spec']): this { this.spec = spec; return this; } 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 4841307574..93b7005006 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts @@ -20,18 +20,17 @@ 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: Omit[] = [ { 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', }, { @@ -39,13 +38,11 @@ const metricValues: Omit[] = [ 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', }, @@ -220,7 +217,7 @@ describe('DatabaseMetricValues', () => { ...metricValues[2], timestamp: laterTime, value: 10, - error_message: undefined, + error_message: null, status: 'success', }, ]); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts index c56fbd3fac..5d81bf7c30 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts @@ -15,7 +15,7 @@ */ import { Knex } from 'knex'; -import { DbMetricValue } from './types'; +import { DbMetricValueCreate, DbMetricValue } from './types'; export class DatabaseMetricValues { private readonly tableName = 'metric_values'; @@ -26,7 +26,7 @@ export class DatabaseMetricValues { * Insert multiple metric values */ async createMetricValues( - metricValues: Omit[], + metricValues: Omit[], ): Promise { await this.dbClient(this.tableName).insert(metricValues); } diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts index 7315606844..662749e82c 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts @@ -16,12 +16,24 @@ import { MetricValue } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; -export type DbMetricValue = { +export type DbMetricValueStatus = 'success' | 'warning' | 'error'; + +export type DbMetricValueCreate = { id: number; 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; }; 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/scheduler/tasks/PullMetricsByProviderTask.ts b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts index 0d4b05b9e3..4efbc53341 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts @@ -28,7 +28,7 @@ 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'; @@ -156,7 +156,7 @@ export class PullMetricsByProviderTask implements SchedulerTask { value, timestamp: new Date(), status, - } as Omit; + } as Omit; } catch (error) { return { catalog_entity_ref: stringifyEntityRef(entity), @@ -164,7 +164,7 @@ export class PullMetricsByProviderTask implements SchedulerTask { timestamp: new Date(), error_message: error instanceof Error ? error.message : String(error), - } as Omit; + } as Omit; } }), ).then(promises => @@ -173,7 +173,7 @@ export class PullMetricsByProviderTask implements SchedulerTask { return [...acc, curr.value]; } return acc; - }, [] as Omit[]), + }, [] as Omit[]), ); 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 5d01e64581..cb93ff3f71 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts @@ -33,6 +33,9 @@ import { DbMetricValue } from '../database/types'; import { mockThresholdRules } from '../../__fixtures__/mockThresholdRules'; import * as aggregateMetricsByStatusModule from '../utils/aggregateMetricsByStatus'; import { MockEntityBuilder } from '../../__fixtures__/mockEntityBuilder'; +import { PermissionCriteria } from '@backstage/plugin-permission-common'; +import { PermissionCondition } from '@backstage/plugin-permission-common'; +import { PermissionRuleParams } from '@backstage/plugin-permission-common'; jest.mock('../utils/mergeEntityAndProviderThresholds'); jest.mock('../permissions/permissionUtils'); @@ -47,7 +50,7 @@ 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[]; @@ -59,7 +62,7 @@ const latestAggregatedEntityMetric = [ 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, { @@ -68,7 +71,7 @@ const latestAggregatedEntityMetric = [ metric_id: 'github.important_metric', value: 11, timestamp: new Date('2024-01-15T12:00:00.000Z'), - error_message: undefined, + error_message: null, status: 'warning', } as DbMetricValue, ] as DbMetricValue[]; @@ -78,6 +81,16 @@ const metricsList = [ { 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; @@ -229,28 +242,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, ); }); @@ -258,28 +255,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, ); }); @@ -420,7 +401,7 @@ describe('CatalogMetricService', () => { mockedDatabase.readLatestEntityMetricValues.mockResolvedValue([ { ...latestEntityMetric[0], - value: undefined, + value: null, }, ]); @@ -437,7 +418,7 @@ describe('CatalogMetricService', () => { mockedDatabase.readLatestEntityMetricValues.mockResolvedValue([ { ...latestEntityMetric[0], - value: undefined, + value: null, }, ]); @@ -501,6 +482,22 @@ describe('CatalogMetricService', () => { ]); }); + 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, + ); + + expect(permissionUtils.filterAuthorizedMetrics).toHaveBeenCalledWith( + [{ id: 'github.important_metric' }, { id: 'github.number_metric' }], + permissionsFilter, + ); + }); + it('should read latest entity metric values by entity refs', async () => { await service.getAggregatedMetricsByEntityRefs( [ diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts index 4697282c77..b8ebaaed05 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -113,7 +113,7 @@ export class CatalogMetricService { try { thresholds = mergeEntityAndProviderThresholds(entity, provider); - if (value === undefined) { + if (value === null) { thresholdError = 'Unable to evaluate thresholds, metric value is missing'; } @@ -121,7 +121,7 @@ export class CatalogMetricService { thresholdError = stringifyError(error); } - const isMetricCalcError = error_message || value === undefined; + const isMetricCalcError = error_message || value === null; return { id: metric.id, @@ -163,13 +163,21 @@ export class CatalogMetricService { async getAggregatedMetricsByEntityRefs( entityRefs: string[], metricIds?: string[], + filter?: PermissionCriteria< + PermissionCondition + >, ): Promise { const metricsToFetch = this.registry.listMetrics(metricIds); + const authorizedMetricsToFetch = filterAuthorizedMetrics( + metricsToFetch, + filter, + ); + const metricResults = await this.database.readLatestEntityMetricValuesByEntityRefs( entityRefs, - metricsToFetch.map(m => m.id), + authorizedMetricsToFetch.map(m => m.id), ); const aggregatedMetrics = aggregateMetricsByStatus(metricResults); 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 8539118b50..46c67c188b 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -482,6 +482,7 @@ describe('createRouter', () => { ).toHaveBeenCalledWith( ['component:default/my-service', 'component:default/my-other-service'], ['github.open_prs', 'jira.open_issues'], + undefined, ); expect(response.body).toEqual(mockAggregatedMetricResults); }); @@ -497,6 +498,7 @@ describe('createRouter', () => { ).toHaveBeenCalledWith( ['component:default/my-service', 'component:default/my-other-service'], ['github.open_prs'], + undefined, ); expect(response.body).toEqual(mockAggregatedMetricResults); }); @@ -541,6 +543,7 @@ describe('createRouter', () => { ).toHaveBeenCalledWith( ['component:default/my-service', 'component:default/my-other-service'], ['github.open_prs', 'jira.open_issues'], + undefined, ); }); @@ -555,6 +558,7 @@ describe('createRouter', () => { ).toHaveBeenCalledWith( ['component:default/my-service', 'component:default/my-other-service'], undefined, + undefined, ); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index 722b4c7a60..251fa0d120 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -150,7 +150,10 @@ export async function createRouter({ router.get('/metrics/catalog/aggregated', async (req, res) => { const { metricIds } = req.query; - await authorizeConditional(req, scorecardMetricReadPermission); + const { conditions } = await authorizeConditional( + req, + scorecardMetricReadPermission, + ); validateCatalogMetricsSchema(req.query); @@ -184,6 +187,7 @@ export async function createRouter({ await catalogMetricService.getAggregatedMetricsByEntityRefs( entitiesOwnedByAUser, metricIdsParsed, + conditions, ); res.json(aggregatedMetrics); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.test.ts index aa75f73ede..6c08dac73b 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.test.ts @@ -15,19 +15,21 @@ */ import { aggregateMetricsByStatus } from './aggregateMetricsByStatus'; -import { DbMetricValue } from '../database/types'; +import { DbMetricValue, DbMetricValueStatus } from '../database/types'; +import { MetricValue } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; describe('aggregateMetricsByStatus', () => { const createMetric = ( metricId: string, - status: 'success' | 'warning' | 'error', - value: any = { count: 1 }, + status: DbMetricValueStatus | null = null, + value: MetricValue | null = null, ): DbMetricValue => ({ id: 1, catalog_entity_ref: 'component:default/test', metric_id: metricId, value, timestamp: new Date(), + error_message: null, status, }); @@ -38,7 +40,7 @@ describe('aggregateMetricsByStatus', () => { describe('when metrics have valid status and value', () => { it('should aggregate single success metric', () => { - const metrics = [createMetric('metric1', 'success')]; + const metrics = [createMetric('metric1', 'success', 98)]; const result = aggregateMetricsByStatus(metrics); expect(result).toEqual({ @@ -54,7 +56,7 @@ describe('aggregateMetricsByStatus', () => { }); it('should aggregate single warning metric', () => { - const metrics = [createMetric('metric1', 'warning')]; + const metrics = [createMetric('metric1', 'warning', 88)]; const result = aggregateMetricsByStatus(metrics); expect(result).toEqual({ @@ -70,7 +72,7 @@ describe('aggregateMetricsByStatus', () => { }); it('should aggregate single error metric', () => { - const metrics = [createMetric('metric1', 'error')]; + const metrics = [createMetric('metric1', 'error', 77)]; const result = aggregateMetricsByStatus(metrics); expect(result).toEqual({ @@ -87,10 +89,10 @@ describe('aggregateMetricsByStatus', () => { it('should aggregate multiple metrics with same metric_id', () => { const metrics = [ - createMetric('metric1', 'success'), - createMetric('metric1', 'success'), - createMetric('metric1', 'warning'), - createMetric('metric1', 'error'), + createMetric('metric1', 'success', 66), + createMetric('metric1', 'success', 16), + createMetric('metric1', 'warning', 55), + createMetric('metric1', 'error', 44), ]; const result = aggregateMetricsByStatus(metrics); @@ -106,51 +108,15 @@ describe('aggregateMetricsByStatus', () => { }); }); - it('should aggregate multiple metrics with different metric_ids', () => { - const metrics = [ - createMetric('metric1', 'success'), - createMetric('metric2', 'warning'), - createMetric('metric3', 'error'), - ]; - const result = aggregateMetricsByStatus(metrics); - - expect(result).toEqual({ - metric1: { - values: { - success: 1, - warning: 0, - error: 0, - }, - total: 1, - }, - metric2: { - values: { - success: 0, - warning: 1, - error: 0, - }, - total: 1, - }, - metric3: { - values: { - success: 0, - warning: 0, - error: 1, - }, - total: 1, - }, - }); - }); - it('should aggregate complex scenario with multiple metric_ids and statuses', () => { const metrics = [ - createMetric('metric1', 'success'), - createMetric('metric1', 'success'), - createMetric('metric1', 'warning'), - createMetric('metric2', 'error'), - createMetric('metric2', 'error'), - createMetric('metric2', 'success'), - createMetric('metric3', 'warning'), + createMetric('metric1', 'success', 16), + createMetric('metric1', 'success', 26), + createMetric('metric1', 'warning', 26), + createMetric('metric2', 'error', 36), + createMetric('metric2', 'error', 46), + createMetric('metric2', 'success', 56), + createMetric('metric3', 'warning', 66), ]; const result = aggregateMetricsByStatus(metrics); @@ -183,149 +149,87 @@ describe('aggregateMetricsByStatus', () => { }); }); - describe('when metrics have invalid status or value', () => { - it('should skip metrics with null value', () => { - const metrics: DbMetricValue[] = [ - createMetric('metric1', 'success', null), - createMetric('metric1', 'warning', { count: 1 }), - ]; - const result = aggregateMetricsByStatus(metrics); - - expect(result).toEqual({ - metric1: { - values: { - success: 0, - warning: 1, - error: 0, - }, - total: 1, - }, - }); - }); - - it('should skip metrics without status', () => { - const metrics: DbMetricValue[] = [ - // @ts-expect-error - for testing - createMetric('metric1', undefined, { count: 1 }), - createMetric('metric1', 'success'), - ]; - const result = aggregateMetricsByStatus(metrics); - - expect(result).toEqual({ - metric1: { - values: { - success: 1, - warning: 0, - error: 0, - }, - total: 1, + it('should skip metrics when value is null', () => { + const metrics: DbMetricValue[] = [ + createMetric('metric1', 'success'), + createMetric('metric1', 'warning', 1), + ]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 0, + warning: 1, + error: 0, }, - }); + total: 1, + }, }); + }); - it('should skip metrics with both null value and no status', () => { - const metrics: DbMetricValue[] = [ - { - id: 1, - catalog_entity_ref: 'component:default/test', - metric_id: 'metric1', - value: undefined, - timestamp: new Date(), - }, - createMetric('metric1', 'success'), - ]; - const result = aggregateMetricsByStatus(metrics); - - expect(result).toEqual({ - metric1: { - values: { - success: 1, - warning: 0, - error: 0, - }, - total: 1, - }, - }); - }); - - it('should handle undefined value as valid (not null)', () => { - const metrics: DbMetricValue[] = [ - createMetric('metric1', 'success', undefined), - ]; - const result = aggregateMetricsByStatus(metrics); - - // undefined !== null, so it should be included - expect(result).toEqual({ - metric1: { - values: { - success: 1, - warning: 0, - error: 0, - }, - total: 1, + it('should skip metrics when status is null', () => { + const metrics: DbMetricValue[] = [ + createMetric('metric1', null, 1), + createMetric('metric1', 'success', 4), + ]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 1, + warning: 0, + error: 0, }, - }); + total: 1, + }, }); + }); - it('should handle mixed valid and invalid metrics', () => { - const metrics: DbMetricValue[] = [ - createMetric('metric1', 'success'), - createMetric('metric1', 'warning', null), - // @ts-expect-error - for testing - createMetric('metric1', undefined, { count: 1 }), - createMetric('metric1', 'error'), - createMetric('metric2', 'success', null), - createMetric('metric2', 'warning'), - ]; - const result = aggregateMetricsByStatus(metrics); - - expect(result).toEqual({ - metric1: { - values: { - success: 1, - warning: 0, - error: 1, - }, - total: 2, + it('should handle mixed valid and invalid metrics', () => { + const metrics: DbMetricValue[] = [ + createMetric('metric1', 'success', 1), + createMetric('metric1', 'warning'), + createMetric('metric1', null, 2), + createMetric('metric1', 'error', 3), + createMetric('metric2', 'success'), + createMetric('metric2', 'warning', 4), + ]; + const result = aggregateMetricsByStatus(metrics); + + expect(result).toEqual({ + metric1: { + values: { + success: 1, + warning: 0, + error: 1, }, - metric2: { - values: { - success: 0, - warning: 1, - error: 0, - }, - total: 1, + total: 2, + }, + metric2: { + values: { + success: 0, + warning: 1, + error: 0, }, - }); + total: 1, + }, }); }); - describe('when all metrics are invalid', () => { - it('should return empty object when all metrics have null value', () => { - const metrics: DbMetricValue[] = [ - createMetric('metric1', 'success', null), - createMetric('metric2', 'warning', null), - ]; - const result = aggregateMetricsByStatus(metrics); - - expect(result).toEqual({}); - }); - - it('should return empty object when all metrics have no status', () => { - const metrics: DbMetricValue[] = [ - // @ts-expect-error - for testing - createMetric('metric2', undefined, { count: 2 }), - // @ts-expect-error - for testing - createMetric('metric2', undefined, { count: 2 }), - ]; - const result = aggregateMetricsByStatus(metrics); + it('should return empty object when all metrics are invalid', () => { + const metrics: DbMetricValue[] = [ + createMetric('metric1', 'success'), + createMetric('metric2', null, 11), + ]; + const result = aggregateMetricsByStatus(metrics); - expect(result).toEqual({}); - }); + expect(result).toEqual({}); }); describe('edge cases', () => { - it('should handle zero value as valid (not null)', () => { + it('should handle zero value', () => { const metrics: DbMetricValue[] = [createMetric('metric1', 'success', 0)]; const result = aggregateMetricsByStatus(metrics); @@ -341,23 +245,7 @@ describe('aggregateMetricsByStatus', () => { }); }); - it('should handle empty string value as valid (not null)', () => { - const metrics: DbMetricValue[] = [createMetric('metric1', 'success', '')]; - const result = aggregateMetricsByStatus(metrics); - - expect(result).toEqual({ - metric1: { - values: { - success: 1, - warning: 0, - error: 0, - }, - total: 1, - }, - }); - }); - - it('should handle false value as valid (not null)', () => { + it('should handle false value', () => { const metrics: DbMetricValue[] = [ createMetric('metric1', 'success', false), ]; diff --git a/workspaces/scorecard/plugins/scorecard-common/report.api.md b/workspaces/scorecard/plugins/scorecard-common/report.api.md index 98c210c8e5..19db013864 100644 --- a/workspaces/scorecard/plugins/scorecard-common/report.api.md +++ b/workspaces/scorecard/plugins/scorecard-common/report.api.md @@ -16,7 +16,7 @@ export type AggregatedMetricResult = { history?: boolean; }; result: { - values?: AggregatedMetricValue[]; + values: AggregatedMetricValue[]; total: number; timestamp: string; }; @@ -51,7 +51,7 @@ export type MetricResult = { history?: boolean; }; result: { - value?: MetricValue; + value: MetricValue | null; timestamp: string; thresholdResult: ThresholdResult; }; @@ -91,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 1b01f41b74..6b16831b04 100644 --- a/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts +++ b/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts @@ -62,14 +62,14 @@ export type MetricResult = { history?: boolean; }; result: { - value?: MetricValue; + value: MetricValue | null; timestamp: string; thresholdResult: ThresholdResult; }; error?: string; }; -/* +/** * @public */ export type AggregatedMetricResult = { @@ -82,7 +82,7 @@ export type AggregatedMetricResult = { history?: boolean; }; result: { - values?: AggregatedMetricValue[]; + 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__/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/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/utils/utils.ts b/workspaces/scorecard/plugins/scorecard/src/utils/utils.ts index 95c0ca9f13..da10291e4c 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 => { From fe366f70a8e402b3e1d30bd86a5e2d7431253673 Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Tue, 6 Jan 2026 13:38:42 +0100 Subject: [PATCH 3/3] fix(scorecard): issue with displaying card error message when threshold is invalid --- .../tasks/PullMetricsByProviderTask.ts | 8 +- .../src/service/CatalogMetricService.test.ts | 87 +++++++++++-------- .../src/service/CatalogMetricService.ts | 4 +- 3 files changed, 60 insertions(+), 39 deletions(-) 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 4efbc53341..78b16ca899 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts @@ -31,6 +31,7 @@ import { stringifyEntityRef } from '@backstage/catalog-model'; 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, @@ -161,6 +166,7 @@ export class PullMetricsByProviderTask implements SchedulerTask { return { catalog_entity_ref: stringifyEntityRef(entity), metric_id: this.providerId, + value, timestamp: new Date(), error_message: error instanceof Error ? error.message : String(error), 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 cb93ff3f71..590a332f53 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts @@ -33,9 +33,11 @@ import { DbMetricValue } from '../database/types'; import { mockThresholdRules } from '../../__fixtures__/mockThresholdRules'; import * as aggregateMetricsByStatusModule from '../utils/aggregateMetricsByStatus'; import { MockEntityBuilder } from '../../__fixtures__/mockEntityBuilder'; -import { PermissionCriteria } from '@backstage/plugin-permission-common'; -import { PermissionCondition } from '@backstage/plugin-permission-common'; -import { PermissionRuleParams } from '@backstage/plugin-permission-common'; +import { + PermissionCriteria, + PermissionCondition, + PermissionRuleParams, +} from '@backstage/plugin-permission-common'; jest.mock('../utils/mergeEntityAndProviderThresholds'); jest.mock('../permissions/permissionUtils'); @@ -341,6 +343,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', @@ -380,40 +427,6 @@ describe('CatalogMetricService', () => { ); }); - it('should set status to error when error message is presented', async () => { - mockedDatabase.readLatestEntityMetricValues.mockResolvedValue([ - { - ...latestEntityMetric[0], - error_message: 'Error message', - }, - ]); - - 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'); - }); - - it('should set status to error when metric value is undefined', async () => { - mockedDatabase.readLatestEntityMetricValues.mockResolvedValue([ - { - ...latestEntityMetric[0], - value: null, - }, - ]); - - const newResult = await service.getLatestEntityMetrics( - 'component:default/test-component', - ['github.important_metric'], - ); - - expect(newResult[0].status).toBe('error'); - expect(newResult[0].error).toBe("Error: Metric value is 'undefined'"); - }); - it('should set threshold result status to error when metric value is missing', async () => { mockedDatabase.readLatestEntityMetricValues.mockResolvedValue([ { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts index b8ebaaed05..a2536d3a6c 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -116,12 +116,14 @@ export class CatalogMetricService { 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 === null; + const isMetricCalcError = error_message !== null && value === null; return { id: metric.id,