From 25fe30f2487d30477e958fc60466e3af86a6f1c3 Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Tue, 23 Dec 2025 21:36:42 +0100 Subject: [PATCH 01/11] 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 02/11] 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 03/11] 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, From 314b778183426cf638b600687ef81f95608bfdcd Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Thu, 8 Jan 2026 11:30:11 +0100 Subject: [PATCH 04/11] refactor(scorecard): update metric handling and introduce aggregated metrics functionality Signed-off-by: Ihor Mykhno --- .../__fixtures__/mockDatabaseMetricValues.ts | 16 +- .../__fixtures__/mockEntityBuilder.ts | 13 +- .../src/database/DatabaseMetricValues.test.ts | 53 ++-- .../src/database/DatabaseMetricValues.ts | 53 ++-- .../scorecard-backend/src/database/types.ts | 10 +- .../plugins/scorecard-backend/src/plugin.ts | 1 + .../tasks/PullMetricsByProviderTask.ts | 6 +- .../src/service/CatalogMetricService.test.ts | 70 +---- .../src/service/CatalogMetricService.ts | 43 ++- .../src/service/router.test.ts | 60 ++-- .../scorecard-backend/src/service/router.ts | 7 +- .../utils/aggregateMetricsByStatus.test.ts | 267 ----------------- .../src/utils/aggregateMetricsByStatus.ts | 44 --- .../src/utils/getEntitiesOwnedByUser.test.ts | 272 ++++++++++++------ .../src/utils/getEntitiesOwnedByUser.ts | 54 ++-- 15 files changed, 392 insertions(+), 577 deletions(-) delete mode 100644 workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.test.ts delete mode 100644 workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.ts diff --git a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts index 97d7d463af..3c614dc1e7 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts @@ -15,27 +15,27 @@ */ import { DatabaseMetricValues } from '../src/database/DatabaseMetricValues'; -import { DbMetricValue } from '../src/database/types'; +import { DbMetricValue, DbAggregatedMetric } from '../src/database/types'; type BuildMockDatabaseMetricValuesParams = { metricValues?: DbMetricValue[]; latestEntityMetric?: DbMetricValue[]; countOfExpiredMetrics?: number; - latestAggregatedEntityMetric?: DbMetricValue[]; + aggregatedMetrics?: DbAggregatedMetric[]; }; export const mockDatabaseMetricValues = { createMetricValues: jest.fn(), readLatestEntityMetricValues: jest.fn(), cleanupExpiredMetrics: jest.fn(), - readLatestEntityMetricValuesByEntityRefs: jest.fn(), + readAggregatedMetricsByEntityRefs: jest.fn(), } as unknown as jest.Mocked; export const buildMockDatabaseMetricValues = ({ metricValues, latestEntityMetric, countOfExpiredMetrics, - latestAggregatedEntityMetric, + aggregatedMetrics, }: BuildMockDatabaseMetricValuesParams) => { const createMetricValues = metricValues ? jest.fn().mockResolvedValue(metricValues) @@ -49,14 +49,14 @@ export const buildMockDatabaseMetricValues = ({ ? jest.fn().mockResolvedValue(countOfExpiredMetrics) : mockDatabaseMetricValues.cleanupExpiredMetrics; - const readLatestEntityMetricValuesByEntityRefs = latestAggregatedEntityMetric - ? jest.fn().mockResolvedValue(latestAggregatedEntityMetric) - : mockDatabaseMetricValues.readLatestEntityMetricValuesByEntityRefs; + const readAggregatedMetricsByEntityRefs = aggregatedMetrics + ? jest.fn().mockResolvedValue(aggregatedMetrics) + : mockDatabaseMetricValues.readAggregatedMetricsByEntityRefs; return { createMetricValues, readLatestEntityMetricValues, cleanupExpiredMetrics, - readLatestEntityMetricValuesByEntityRefs, + readAggregatedMetricsByEntityRefs, } as unknown as jest.Mocked; }; diff --git a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntityBuilder.ts b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntityBuilder.ts index aa09d001d7..7eb77fe0c3 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntityBuilder.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockEntityBuilder.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import type { Entity } from '@backstage/catalog-model'; +import type { Entity, EntityRelation } from '@backstage/catalog-model'; export class MockEntityBuilder { private kind: string = 'Component'; @@ -22,9 +22,8 @@ export class MockEntityBuilder { name: 'default-component', namespace: 'default', }; - private spec: Entity['spec'] = { - owner: 'guests', - }; + private spec: Entity['spec'] | undefined = undefined; + private relations: EntityRelation[] | undefined = undefined; withKind(kind: string): this { this.kind = kind; @@ -41,12 +40,18 @@ export class MockEntityBuilder { return this; } + withRelations(relations: EntityRelation[]): this { + this.relations = relations; + return this; + } + build(): Entity { return { apiVersion: 'backstage.io/v1alpha1', kind: this.kind, metadata: this.metadata, spec: this.spec, + relations: this.relations, }; } } diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts index 93b7005006..3e65bc6644 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts @@ -25,7 +25,7 @@ import { migrate } from './migration'; jest.setTimeout(60000); -const metricValues: Omit[] = [ +const metricValues: DbMetricValueCreate[] = [ { catalog_entity_ref: 'component:default/test-service', metric_id: 'github.metric1', @@ -191,9 +191,9 @@ describe('DatabaseMetricValues', () => { ); }); - describe('readLatestEntityMetricValuesByEntityRefs', () => { + describe('readAggregatedMetricsByEntityRefs', () => { it.each(databases.eachSupportedId())( - 'should return latest metric values for multiple entities and metrics - %p', + 'should return aggregated metrics by status for multiple entities and metrics - %p', async databaseId => { const { client, db } = await createDatabase(databaseId); @@ -204,25 +204,30 @@ describe('DatabaseMetricValues', () => { { ...metricValues[0], timestamp: baseTime, + status: 'success', }, { ...metricValues[1], timestamp: baseTime, + status: 'success', }, { ...metricValues[2], timestamp: laterTime, + status: 'warning', + value: 10, }, { - ...metricValues[2], + catalog_entity_ref: 'component:default/test-service', + metric_id: 'github.metric2', timestamp: laterTime, - value: 10, + value: 20, error_message: null, - status: 'success', + status: 'error', }, ]); - const result = await db.readLatestEntityMetricValuesByEntityRefs( + const result = await db.readAggregatedMetricsByEntityRefs( [ 'component:default/test-service', 'component:default/another-service', @@ -230,28 +235,26 @@ describe('DatabaseMetricValues', () => { ['github.metric1', 'github.metric2'], ); - expect(result).toHaveLength(3); + expect(result).toHaveLength(2); - 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 metric1Result = result.find( + r => r.metric_id === 'github.metric1', ); - - const anotherServiceMetric2 = result.find( - r => - r.metric_id === 'github.metric2' && - r.catalog_entity_ref === 'component:default/another-service', + const metric2Result = result.find( + r => r.metric_id === 'github.metric2', ); - expect(testServiceMetric1?.value).toBe(41); - expect(anotherServiceMetric1?.value).toBe(25); - expect(anotherServiceMetric2?.value).toBe(10); + expect(metric1Result).toBeDefined(); + expect(metric1Result?.total).toBe(2); + expect(metric1Result?.success).toBe(2); + expect(metric1Result?.warning).toBe(0); + expect(metric1Result?.error).toBe(0); + + expect(metric2Result).toBeDefined(); + expect(metric2Result?.total).toBe(2); + expect(metric2Result?.success).toBe(0); + expect(metric2Result?.warning).toBe(1); + expect(metric2Result?.error).toBe(1); }, ); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts index 5d81bf7c30..555f5a858f 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts @@ -15,7 +15,11 @@ */ import { Knex } from 'knex'; -import { DbMetricValueCreate, DbMetricValue } from './types'; +import { + DbMetricValueCreate, + DbMetricValue, + DbAggregatedMetric, +} from './types'; export class DatabaseMetricValues { private readonly tableName = 'metric_values'; @@ -25,9 +29,7 @@ export class DatabaseMetricValues { /** * Insert multiple metric values */ - async createMetricValues( - metricValues: Omit[], - ): Promise { + async createMetricValues(metricValues: DbMetricValueCreate[]): Promise { await this.dbClient(this.tableName).insert(metricValues); } @@ -60,21 +62,40 @@ export class DatabaseMetricValues { } /** - * Get the latest metric values for multiple entities and metrics + * Get aggregated metrics by status for multiple entities and metrics. */ - async readLatestEntityMetricValuesByEntityRefs( + async readAggregatedMetricsByEntityRefs( catalog_entity_refs: string[], metric_ids: string[], - ): Promise { + ): Promise { + const latestIdsSubquery = this.dbClient(this.tableName) + .max('id') + .whereIn('metric_id', metric_ids) + .whereIn('catalog_entity_ref', catalog_entity_refs) + .groupBy('metric_id', 'catalog_entity_ref'); + 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'), - ); + .select('metric_id') + .count('* as total') + .max('timestamp as max_timestamp') + .select( + this.dbClient.raw( + "SUM(CASE WHEN status = 'success' AND value IS NOT NULL THEN 1 ELSE 0 END) as success", + ), + ) + .select( + this.dbClient.raw( + "SUM(CASE WHEN status = 'warning' AND value IS NOT NULL THEN 1 ELSE 0 END) as warning", + ), + ) + .select( + this.dbClient.raw( + "SUM(CASE WHEN status = 'error' AND value IS NOT NULL THEN 1 ELSE 0 END) as error", + ), + ) + .whereIn('id', latestIdsSubquery) + .whereNotNull('status') + .whereNotNull('value') + .groupBy('metric_id'); } } diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts index 662749e82c..62c5576c8b 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts @@ -19,7 +19,6 @@ import { MetricValue } from '@red-hat-developer-hub/backstage-plugin-scorecard-c export type DbMetricValueStatus = 'success' | 'warning' | 'error'; export type DbMetricValueCreate = { - id: number; catalog_entity_ref: string; metric_id: string; value?: MetricValue; @@ -37,3 +36,12 @@ export type DbMetricValue = { error_message: string | null; status: DbMetricValueStatus | null; }; + +export type DbAggregatedMetric = { + metric_id: string; + total: number; + max_timestamp: Date; + success: number; + warning: number; + error: number; +}; diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts b/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts index 0aae93d76a..39d1840c34 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts @@ -115,6 +115,7 @@ export const scorecardPlugin = createBackendPlugin({ await createRouter({ metricProvidersRegistry, catalogMetricService, + catalog, httpAuth, permissions, }), diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts index 78b16ca899..d50db43152 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts @@ -161,7 +161,7 @@ export class PullMetricsByProviderTask implements SchedulerTask { value, timestamp: new Date(), status, - } as Omit; + } as DbMetricValueCreate; } catch (error) { return { catalog_entity_ref: stringifyEntityRef(entity), @@ -170,7 +170,7 @@ export class PullMetricsByProviderTask implements SchedulerTask { timestamp: new Date(), error_message: error instanceof Error ? error.message : String(error), - } as Omit; + } as DbMetricValueCreate; } }), ).then(promises => @@ -179,7 +179,7 @@ export class PullMetricsByProviderTask implements SchedulerTask { return [...acc, curr.value]; } return acc; - }, [] as Omit[]), + }, [] as DbMetricValueCreate[]), ); await this.database.createMetricValues(batchResults); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts index 590a332f53..e0e77f23e3 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts @@ -29,9 +29,8 @@ import { AuthService } from '@backstage/backend-plugin-api'; import * as permissionUtils from '../permissions/permissionUtils'; import { Metric } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import * as thresholdUtils from '../utils/mergeEntityAndProviderThresholds'; -import { DbMetricValue } from '../database/types'; +import { DbMetricValue, DbAggregatedMetric } from '../database/types'; import { mockThresholdRules } from '../../__fixtures__/mockThresholdRules'; -import * as aggregateMetricsByStatusModule from '../utils/aggregateMetricsByStatus'; import { MockEntityBuilder } from '../../__fixtures__/mockEntityBuilder'; import { PermissionCriteria, @@ -41,7 +40,6 @@ import { jest.mock('../utils/mergeEntityAndProviderThresholds'); jest.mock('../permissions/permissionUtils'); -jest.mock('../utils/aggregateMetricsByStatus'); const provider = new MockNumberProvider('github.important_metric', 'github'); @@ -57,26 +55,16 @@ const latestEntityMetric = [ } as DbMetricValue, ] as DbMetricValue[]; -const latestAggregatedEntityMetric = [ +const aggregatedMetrics: DbAggregatedMetric[] = [ { - 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: null, - 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: null, - status: 'warning', - } as DbMetricValue, -] as DbMetricValue[]; + total: 2, + max_timestamp: new Date('2024-01-15T12:00:00.000Z'), + success: 1, + warning: 1, + error: 0, + }, +]; const metricsList = [ { id: 'github.important_metric' }, @@ -99,7 +87,6 @@ describe('CatalogMetricService', () => { let mockedRegistry: jest.Mocked; let mockedDatabase: jest.Mocked; let service: CatalogMetricService; - let aggregateMetricsByStatusSpy: jest.SpyInstance; const mockEntity = new MockEntityBuilder().build(); @@ -120,7 +107,7 @@ describe('CatalogMetricService', () => { mockedDatabase = buildMockDatabaseMetricValues({ latestEntityMetric, - latestAggregatedEntityMetric, + aggregatedMetrics, }); (permissionUtils.filterAuthorizedMetrics as jest.Mock).mockReturnValue([ @@ -133,19 +120,6 @@ 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, @@ -169,12 +143,6 @@ 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( @@ -511,7 +479,7 @@ describe('CatalogMetricService', () => { ); }); - it('should read latest entity metric values by entity refs', async () => { + it('should read aggregated metrics by entity refs', async () => { await service.getAggregatedMetricsByEntityRefs( [ 'component:default/test-component', @@ -521,7 +489,7 @@ describe('CatalogMetricService', () => { ); expect( - mockedDatabase.readLatestEntityMetricValuesByEntityRefs, + mockedDatabase.readAggregatedMetricsByEntityRefs, ).toHaveBeenCalledWith( [ 'component:default/test-component', @@ -530,19 +498,5 @@ describe('CatalogMetricService', () => { ['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 a2536d3a6c..7b51a443ad 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -16,7 +16,6 @@ import { AggregatedMetricResult, - AggregatedMetricValue, MetricResult, ThresholdConfig, } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; @@ -32,7 +31,6 @@ import { import { CatalogService } from '@backstage/plugin-catalog-node'; import { DatabaseMetricValues } from '../database/DatabaseMetricValues'; import { mergeEntityAndProviderThresholds } from '../utils/mergeEntityAndProviderThresholds'; -import { aggregateMetricsByStatus } from '../utils/aggregateMetricsByStatus'; type CatalogMetricServiceOptions = { catalog: CatalogService; @@ -59,15 +57,6 @@ 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. * @@ -176,21 +165,21 @@ export class CatalogMetricService { filter, ); - const metricResults = - await this.database.readLatestEntityMetricValuesByEntityRefs( + const aggregatedMetrics = + await this.database.readAggregatedMetricsByEntityRefs( entityRefs, authorizedMetricsToFetch.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 }); - } + return aggregatedMetrics.map(row => { + const metricId = row.metric_id as string; + const success = row.success || 0; + const warning = row.warning || 0; + const error = row.error || 0; + const total = row.total || 0; + const timestamp = row.max_timestamp + ? new Date(row.max_timestamp).toISOString() + : new Date().toISOString(); const provider = this.registry.getProvider(metricId); const metric = provider.getMetric(); @@ -205,9 +194,13 @@ export class CatalogMetricService { history: metric.history, }, result: { - values, - total: metricData.total, - timestamp: new Date(metricResults[0].timestamp).toISOString(), + values: [ + { count: success, name: 'success' }, + { count: warning, name: 'warning' }, + { count: error, name: 'error' }, + ], + total, + timestamp, }, }; }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts index 46c67c188b..649294fc00 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -91,8 +91,9 @@ describe('createRouter', () => { beforeEach(async () => { metricProvidersRegistry = new MetricProvidersRegistry(); + const catalog = catalogServiceMock.mock(); catalogMetricService = new CatalogMetricService({ - catalog: catalogServiceMock.mock(), + catalog, registry: metricProvidersRegistry, auth: mockServices.auth(), database: mockDatabaseMetricValues, @@ -117,6 +118,7 @@ describe('createRouter', () => { const router = await createRouter({ metricProvidersRegistry, catalogMetricService, + catalog, httpAuth: httpAuthMock, permissions: permissionsMock, }); @@ -372,7 +374,7 @@ describe('createRouter', () => { }); }); - describe('GET /metrics/catalog/aggregated', () => { + describe('GET /metrics/catalog/aggregates', () => { const mockAggregatedMetricResults: AggregatedMetricResult[] = [ { id: 'github.open_prs', @@ -398,8 +400,9 @@ describe('createRouter', () => { let mockCatalog: ReturnType; let getEntitiesOwnedByUserSpy: jest.SpyInstance; let checkEntityAccessSpy: jest.SpyInstance; + let aggregatesApp: express.Express; - beforeEach(() => { + beforeEach(async () => { const githubProvider = new MockNumberProvider( 'github.open_prs', 'github', @@ -427,10 +430,6 @@ describe('createRouter', () => { .build(); mockCatalog.getEntities.mockResolvedValue({ items: [componentEntity] }); - jest - .spyOn(catalogMetricService, 'getCatalogService') - .mockReturnValue(mockCatalog); - jest .spyOn(catalogMetricService, 'getAggregatedMetricsByEntityRefs') .mockResolvedValue(mockAggregatedMetricResults); @@ -446,6 +445,17 @@ describe('createRouter', () => { permissionUtilsModule, 'checkEntityAccess', ); + + const router = await createRouter({ + metricProvidersRegistry, + catalogMetricService, + catalog: mockCatalog, + httpAuth: httpAuthMock, + permissions: permissionsMock, + }); + aggregatesApp = express(); + aggregatesApp.use(router); + aggregatesApp.use(mockErrorHandler()); }); afterEach(() => { @@ -456,7 +466,9 @@ describe('createRouter', () => { permissionsMock.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.DENY }, ]); - const result = await request(app).get('/metrics/catalog/aggregated'); + const result = await request(aggregatesApp).get( + '/metrics/catalog/aggregates', + ); expect(result.statusCode).toBe(403); expect(result.body.error.name).toEqual('NotAllowedError'); @@ -464,7 +476,9 @@ describe('createRouter', () => { 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'); + const result = await request(aggregatesApp).get( + '/metrics/catalog/aggregates', + ); console.log(result.body); expect(result.statusCode).toBe(403); @@ -472,8 +486,8 @@ describe('createRouter', () => { }); it('should handle multiple metricIds parameter', async () => { - const response = await request(app).get( - '/metrics/catalog/aggregated?metricIds=github.open_prs,jira.open_issues', + const response = await request(aggregatesApp).get( + '/metrics/catalog/aggregates?metricIds=github.open_prs,jira.open_issues', ); expect(response.status).toBe(200); @@ -488,8 +502,8 @@ describe('createRouter', () => { }); it('should handle single metricIds parameter', async () => { - const response = await request(app).get( - '/metrics/catalog/aggregated?metricIds=github.open_prs', + const response = await request(aggregatesApp).get( + '/metrics/catalog/aggregates?metricIds=github.open_prs', ); expect(response.status).toBe(200); @@ -504,7 +518,9 @@ describe('createRouter', () => { }); it('should get entities owned by user', async () => { - const response = await request(app).get('/metrics/catalog/aggregated'); + const response = await request(aggregatesApp).get( + '/metrics/catalog/aggregates', + ); expect(response.status).toBe(200); expect(getEntitiesOwnedByUserSpy).toHaveBeenCalledWith( @@ -518,14 +534,18 @@ describe('createRouter', () => { it('should return empty array when user owns no entities', async () => { getEntitiesOwnedByUserSpy.mockResolvedValue([]); - const response = await request(app).get('/metrics/catalog/aggregated'); + const response = await request(aggregatesApp).get( + '/metrics/catalog/aggregates', + ); 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'); + const response = await request(aggregatesApp).get( + '/metrics/catalog/aggregates', + ); expect(checkEntityAccessSpy).toHaveBeenCalledTimes(2); expect(response.status).toBe(200); @@ -533,8 +553,8 @@ describe('createRouter', () => { }); it('should parse metricIds parameter correctly', async () => { - const response = await request(app).get( - '/metrics/catalog/aggregated?metricIds=github.open_prs,jira.open_issues', + const response = await request(aggregatesApp).get( + '/metrics/catalog/aggregates?metricIds=github.open_prs,jira.open_issues', ); expect(response.status).toBe(200); @@ -548,8 +568,8 @@ describe('createRouter', () => { }); 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', + const response = await request(aggregatesApp).get( + '/metrics/catalog/aggregates?anotherParameter=value', ); expect(response.status).toBe(200); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index 251fa0d120..f8e5369c21 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -23,6 +23,7 @@ import { type HttpAuthService, type PermissionsService, } from '@backstage/backend-plugin-api'; +import type { CatalogService } from '@backstage/plugin-catalog-node'; import { AuthorizeResult, BasicPermission, @@ -42,6 +43,7 @@ import { parseCommaSeparatedString } from '../utils/parseCommaSeparatedString'; export type ScorecardRouterOptions = { metricProvidersRegistry: MetricProvidersRegistry; catalogMetricService: CatalogMetricService; + catalog: CatalogService; httpAuth: HttpAuthService; permissions: PermissionsService; }; @@ -49,6 +51,7 @@ export type ScorecardRouterOptions = { export async function createRouter({ metricProvidersRegistry, catalogMetricService, + catalog, httpAuth, permissions, }: ScorecardRouterOptions): Promise { @@ -147,7 +150,7 @@ export async function createRouter({ res.json(results); }); - router.get('/metrics/catalog/aggregated', async (req, res) => { + router.get('/metrics/catalog/aggregates', async (req, res) => { const { metricIds } = req.query; const { conditions } = await authorizeConditional( @@ -165,7 +168,7 @@ export async function createRouter({ } const entitiesOwnedByAUser = await getEntitiesOwnedByUser(userEntityRef, { - catalog: catalogMetricService.getCatalogService(), + catalog, credentials, }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.test.ts deleted file mode 100644 index 6c08dac73b..0000000000 --- a/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.test.ts +++ /dev/null @@ -1,267 +0,0 @@ -/* - * 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, DbMetricValueStatus } from '../database/types'; -import { MetricValue } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; - -describe('aggregateMetricsByStatus', () => { - const createMetric = ( - metricId: string, - 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, - }); - - 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', 98)]; - 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', 88)]; - 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', 77)]; - 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', 66), - createMetric('metric1', 'success', 16), - createMetric('metric1', 'warning', 55), - createMetric('metric1', 'error', 44), - ]; - const result = aggregateMetricsByStatus(metrics); - - expect(result).toEqual({ - metric1: { - values: { - success: 2, - warning: 1, - error: 1, - }, - total: 4, - }, - }); - }); - - it('should aggregate complex scenario with multiple metric_ids and statuses', () => { - const metrics = [ - 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); - - 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, - }, - }); - }); - }); - - 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 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', 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, - }, - total: 2, - }, - metric2: { - values: { - success: 0, - warning: 1, - error: 0, - }, - total: 1, - }, - }); - }); - - 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({}); - }); - - describe('edge cases', () => { - it('should handle zero value', () => { - 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 false value', () => { - 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 deleted file mode 100644 index ca1424b0f9..0000000000 --- a/workspaces/scorecard/plugins/scorecard-backend/src/utils/aggregateMetricsByStatus.ts +++ /dev/null @@ -1,44 +0,0 @@ -/* - * 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 index 992013d15e..6059bf9a23 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.test.ts @@ -18,14 +18,16 @@ 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'; +import { + RELATION_MEMBER_OF, + RELATION_OWNED_BY, +} from '@backstage/catalog-model'; jest.mock('@backstage/catalog-model', () => { const actual = jest.requireActual('@backstage/catalog-model'); return { ...actual, stringifyEntityRef: jest.fn(actual.stringifyEntityRef), - parseEntityRef: jest.fn(actual.parseEntityRef), }; }); @@ -44,12 +46,24 @@ describe('getEntitiesOwnedByUser', () => { userEntity = new MockEntityBuilder() .withKind('User') - .withMetadata({ name: 'test-user', namespace: 'default' }) + .withMetadata({ name: 'test-user', namespace: 'development' }) .build(); userOwnedEntity = new MockEntityBuilder() - .withMetadata({ name: 'user-component', namespace: 'default' }) - .withSpec({ owner: 'user:default/test-user' }) + .withMetadata({ name: 'user-component', namespace: 'development' }) .build(); + mockedCatalog.queryEntities.mockResolvedValue({ + items: [ + { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { name: 'user-component', namespace: 'development' }, + }, + ], + pageInfo: { + nextCursor: undefined, // No next page + }, + totalItems: 1, + }); }); afterEach(() => { @@ -61,7 +75,7 @@ describe('getEntitiesOwnedByUser', () => { mockedCatalog.getEntityByRef.mockResolvedValue(undefined); await expect( - getEntitiesOwnedByUser('user:default/test-user', { + getEntitiesOwnedByUser('user:development/test-user', { catalog: mockedCatalog, credentials: mockCredentials, }), @@ -71,74 +85,122 @@ describe('getEntitiesOwnedByUser', () => { 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', { + it('should call stringifyEntityRef once', async () => { + await getEntitiesOwnedByUser('user:development/test-user', { catalog: mockedCatalog, credentials: mockCredentials, }); - expect(catalogModel.stringifyEntityRef).toHaveBeenCalledTimes(2); + expect(catalogModel.stringifyEntityRef).toHaveBeenCalledTimes(1); 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', { + it('should not access memberOf relations when user has no groups', async () => { + await getEntitiesOwnedByUser('user:development/test-user', { catalog: mockedCatalog, credentials: mockCredentials, }); - expect(catalogModel.parseEntityRef).not.toHaveBeenCalled(); + expect(userEntity.relations).toBeUndefined(); }); - it('should call getEntities with owned by user relation', async () => { - await getEntitiesOwnedByUser('user:default/test-user', { + it('should call queryEntities with owned by user relation', async () => { + await getEntitiesOwnedByUser('user:development/test-user', { catalog: mockedCatalog, credentials: mockCredentials, }); - expect(mockedCatalog.getEntities).toHaveBeenCalledTimes(1); - expect(mockedCatalog.getEntities).toHaveBeenCalledWith( + expect(mockedCatalog.queryEntities).toHaveBeenCalledTimes(1); + expect(mockedCatalog.queryEntities).toHaveBeenCalledWith( { filter: { - [`relations.${RELATION_OWNED_BY}`]: 'user:default/test-user', + [`relations.${RELATION_OWNED_BY}`]: 'user:development/test-user', }, + fields: ['kind', 'metadata'], + limit: 50, }, { 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, - }); + const result = await getEntitiesOwnedByUser( + 'user:development/test-user', + { + catalog: mockedCatalog, + credentials: mockCredentials, + }, + ); - expect(result).toEqual(['component:default/user-component']); + expect(result).toEqual(['component:development/user-component']); }); it('should return empty array when user owns no entities', async () => { - mockedCatalog.getEntities.mockResolvedValue({ items: [] }); + mockedCatalog.queryEntities.mockResolvedValue({ + items: [], + pageInfo: { nextCursor: undefined }, + totalItems: 0, + }); + + const result = await getEntitiesOwnedByUser( + 'user:development/test-user', + { + catalog: mockedCatalog, + credentials: mockCredentials, + }, + ); + + expect(result).toEqual([]); + }); - const result = await getEntitiesOwnedByUser('user:default/test-user', { + it('should call queryEntities with cursor', async () => { + mockedCatalog.queryEntities.mockReset(); + mockedCatalog.queryEntities + .mockResolvedValueOnce({ + items: [userOwnedEntity], + pageInfo: { nextCursor: 'cursor1' }, + totalItems: 1, + }) + .mockResolvedValueOnce({ + items: [], + pageInfo: { nextCursor: undefined }, + totalItems: 0, + }); + + await getEntitiesOwnedByUser('user:development/test-user', { catalog: mockedCatalog, credentials: mockCredentials, }); - expect(result).toEqual([]); + expect(mockedCatalog.queryEntities).toHaveBeenCalledTimes(2); + expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( + 1, + { + filter: { + [`relations.${RELATION_OWNED_BY}`]: 'user:development/test-user', + }, + fields: ['kind', 'metadata'], + limit: 50, + }, + { credentials: mockCredentials }, + ); + expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( + 2, + { + cursor: 'cursor1', + filter: { + [`relations.${RELATION_OWNED_BY}`]: 'user:development/test-user', + }, + fields: ['kind', 'metadata'], + limit: 50, + }, + { credentials: mockCredentials }, + ); }); }); @@ -149,110 +211,156 @@ describe('getEntitiesOwnedByUser', () => { beforeEach(() => { userEntityWithMemberOf = new MockEntityBuilder() .withKind('User') - .withMetadata({ name: 'test-user', namespace: 'default' }) - .withSpec({ memberOf: ['group:default/developers'] }) + .withMetadata({ name: 'test-user', namespace: 'development' }) + .withRelations([ + { + type: RELATION_MEMBER_OF, + targetRef: 'group:development/developers', + }, + ]) .build(); groupOwnedEntity = new MockEntityBuilder() - .withMetadata({ name: 'group-component', namespace: 'default' }) + .withMetadata({ name: 'group-component', namespace: 'development' }) .build(); mockedCatalog.getEntityByRef.mockResolvedValue(userEntityWithMemberOf); - mockedCatalog.getEntities - .mockResolvedValueOnce({ items: [userOwnedEntity] }) - .mockResolvedValueOnce({ items: [groupOwnedEntity] }); + mockedCatalog.queryEntities + .mockResolvedValueOnce({ + items: [userOwnedEntity], + pageInfo: { nextCursor: undefined }, + totalItems: 1, + }) + .mockResolvedValueOnce({ + items: [groupOwnedEntity], + pageInfo: { nextCursor: undefined }, + totalItems: 1, + }); }); - it('should call stringifyEntityRef four times', async () => { - await getEntitiesOwnedByUser('user:default/test-user', { + it('should call stringifyEntityRef twice for owned entities', async () => { + await getEntitiesOwnedByUser('user:development/test-user', { catalog: mockedCatalog, credentials: mockCredentials, }); - expect(catalogModel.stringifyEntityRef).toHaveBeenCalledTimes(4); + expect(catalogModel.stringifyEntityRef).toHaveBeenCalledTimes(2); 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, + 2, groupOwnedEntity, ); }); - it('should call parseEntityRef once', async () => { - await getEntitiesOwnedByUser('user:default/test-user', { + it('should use relations to get group references', async () => { + await getEntitiesOwnedByUser('user:development/test-user', { catalog: mockedCatalog, credentials: mockCredentials, }); - expect(catalogModel.parseEntityRef).toHaveBeenCalledTimes(1); - expect(catalogModel.parseEntityRef).toHaveBeenCalledWith( - 'group:default/developers', - { defaultKind: 'Group', defaultNamespace: 'default' }, + expect(userEntityWithMemberOf.relations).toHaveLength(1); + expect(userEntityWithMemberOf.relations?.[0].type).toBe( + RELATION_MEMBER_OF, + ); + expect(userEntityWithMemberOf.relations?.[0].targetRef).toBe( + 'group:development/developers', ); }); - it('should call getEntities with owned by user relation', async () => { - await getEntitiesOwnedByUser('user:default/test-user', { + it('should call queryEntities with owned by user relation', async () => { + await getEntitiesOwnedByUser('user:development/test-user', { catalog: mockedCatalog, credentials: mockCredentials, }); - expect(mockedCatalog.getEntities).toHaveBeenCalledTimes(2); - expect(mockedCatalog.getEntities).toHaveBeenNthCalledWith( + expect(mockedCatalog.queryEntities).toHaveBeenCalledTimes(2); + expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( 1, { filter: { - [`relations.${RELATION_OWNED_BY}`]: 'user:default/test-user', + [`relations.${RELATION_OWNED_BY}`]: 'user:development/test-user', }, + fields: ['kind', 'metadata'], + limit: 50, }, { credentials: mockCredentials }, ); - expect(mockedCatalog.getEntities).toHaveBeenNthCalledWith( + expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( 2, { filter: { - [`relations.${RELATION_OWNED_BY}`]: 'group:default/developers', + [`relations.${RELATION_OWNED_BY}`]: 'group:development/developers', }, + fields: ['kind', 'metadata'], + limit: 50, }, { credentials: mockCredentials }, ); }); it('should return entities owned by user and groups', async () => { - const result = await getEntitiesOwnedByUser('user:default/test-user', { - catalog: mockedCatalog, - credentials: mockCredentials, - }); + const result = await getEntitiesOwnedByUser( + 'user:development/test-user', + { + catalog: mockedCatalog, + credentials: mockCredentials, + }, + ); expect(result).toEqual([ - 'component:default/user-component', - 'component:default/group-component', + 'component:development/user-component', + 'component:development/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, - }); + mockedCatalog.queryEntities.mockReset(); + mockedCatalog.queryEntities + .mockResolvedValueOnce({ + items: [], + pageInfo: { nextCursor: undefined }, + totalItems: 0, + }) + .mockResolvedValueOnce({ + items: [], + pageInfo: { nextCursor: undefined }, + totalItems: 0, + }); + + const result = await getEntitiesOwnedByUser( + 'user:development/test-user', + { + catalog: mockedCatalog, + credentials: mockCredentials, + }, + ); expect(result).toEqual([]); - expect(mockedCatalog.getEntities).toHaveBeenCalledTimes(2); + expect(mockedCatalog.queryEntities).toHaveBeenCalledTimes(2); }); + + // it('should call queryEntities with cursor', async () => { + // mockedCatalog.queryEntities.mockResolvedValue({ items: [userOwnedEntity], pageInfo: { nextCursor: 'cursor1' }, totalItems: 1 }); + // mockedCatalog.queryEntities.mockResolvedValue({ items: [groupOwnedEntity], pageInfo: { nextCursor: undefined }, totalItems: 1 }); + + // await getEntitiesOwnedByUser('user:development/test-user', { + // catalog: mockedCatalog, + // credentials: mockCredentials, + // }); + + // expect(mockedCatalog.queryEntities).toHaveBeenCalledTimes(2); + // expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( + // 1, + // { cursor: 'cursor1' }, + // { credentials: mockCredentials }, + // ); + // expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( + // 2, + // { cursor: 'cursor2' }, + // { credentials: mockCredentials }, + // ); + // }); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.ts b/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.ts index 163b76e438..b044c97885 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.ts @@ -18,11 +18,13 @@ import { NotFoundError } from '@backstage/errors'; import { BackstageCredentials } from '@backstage/backend-plugin-api'; import { CatalogService } from '@backstage/plugin-catalog-node'; import { - parseEntityRef, + RELATION_MEMBER_OF, RELATION_OWNED_BY, stringifyEntityRef, } from '@backstage/catalog-model'; +const QUERY_ENTITIES_BATCH_SIZE = 50; + export async function getEntitiesOwnedByUser( userEntityRef: string, options: { @@ -38,36 +40,44 @@ export async function getEntitiesOwnedByUser( throw new NotFoundError('User entity not found in catalog'); } - const entitiesOwnedByUser: string[] = [stringifyEntityRef(userEntity)]; + const ownerRefs: string[] = [userEntityRef]; - 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, - }); + const memberOfRelations = + userEntity.relations?.filter( + relation => relation.type === RELATION_MEMBER_OF, + ) ?? []; - entitiesOwnedByUser.push(stringifyEntityRef(parsedEntityRef)); - } + if (memberOfRelations.length > 0) { + for (const relation of memberOfRelations) { + ownerRefs.push(relation.targetRef); } } const entitiesOwnedByUserAndGroups: string[] = []; - for (const entityRef of entitiesOwnedByUser) { - const entities = await options.catalog.getEntities( - { - filter: { - [`relations.${RELATION_OWNED_BY}`]: entityRef, + for (const ownerRef of ownerRefs) { + let cursor: string | undefined = undefined; + + do { + const entities = await options.catalog.queryEntities( + { + filter: { + [`relations.${RELATION_OWNED_BY}`]: ownerRef, + }, + fields: ['kind', 'metadata'], + limit: QUERY_ENTITIES_BATCH_SIZE, + ...(cursor ? { cursor } : {}), }, - }, - { credentials: options.credentials }, - ); + { credentials: options.credentials }, + ); + + cursor = entities.pageInfo.nextCursor; - const entityRefs = entities.items.map(entity => stringifyEntityRef(entity)); - entitiesOwnedByUserAndGroups.push(...entityRefs); + const entityRefs = entities.items.map(entity => + stringifyEntityRef(entity), + ); + entitiesOwnedByUserAndGroups.push(...entityRefs); + } while (cursor !== undefined); } return entitiesOwnedByUserAndGroups; From 08daffd464169dcc481774b9a2e18f3275555fb2 Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Thu, 8 Jan 2026 11:37:45 +0100 Subject: [PATCH 05/11] refactor(scorecard): usage of the type for the frontend --- .../__fixtures__/aggregatedScorecardData.ts | 8 +++---- .../scorecard/plugins/scorecard/dev/index.tsx | 6 +++-- .../plugins/scorecard/src/api/index.ts | 6 +++-- .../ScorecardHomepageCard.tsx | 3 ++- .../ScorecardHomepageSection.tsx | 2 +- .../__tests__/ScorecardHomepageCard.test.tsx | 4 ++-- .../ScorecardHomepageSection.test.tsx | 5 ++--- .../src/hooks/useAggregatedScorecards.tsx | 2 +- .../plugins/scorecard/src/utils/utils.ts | 22 ------------------- 9 files changed, 19 insertions(+), 39 deletions(-) diff --git a/workspaces/scorecard/plugins/scorecard/__fixtures__/aggregatedScorecardData.ts b/workspaces/scorecard/plugins/scorecard/__fixtures__/aggregatedScorecardData.ts index 7b61ad0297..d881bee2be 100644 --- a/workspaces/scorecard/plugins/scorecard/__fixtures__/aggregatedScorecardData.ts +++ b/workspaces/scorecard/plugins/scorecard/__fixtures__/aggregatedScorecardData.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { AggregatedMetricResult } from '../src/utils/utils'; +import { AggregatedMetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; export const mockAggregatedScorecardSuccessData: AggregatedMetricResult[] = [ { @@ -24,7 +24,7 @@ export const mockAggregatedScorecardSuccessData: AggregatedMetricResult[] = [ title: 'GitHub open PRs', description: 'Current count of open Pull Requests for a given GitHub repository.', - type: 'object', + type: 'number', history: true, }, result: { @@ -35,7 +35,6 @@ export const mockAggregatedScorecardSuccessData: AggregatedMetricResult[] = [ ], total: 37, timestamp: '2024-01-15T10:30:00Z', - lastUpdated: '2024-01-15T10:30:00Z', }, }, { @@ -45,7 +44,7 @@ export const mockAggregatedScorecardSuccessData: AggregatedMetricResult[] = [ title: 'Jira open blocking tickets', description: 'Highlights the number of critical, blocking issues that are currently open in Jira.', - type: 'object', + type: 'number', history: true, }, result: { @@ -56,7 +55,6 @@ export const mockAggregatedScorecardSuccessData: AggregatedMetricResult[] = [ ], total: 4, timestamp: '2024-01-15T10:30:00Z', - lastUpdated: '2024-01-15T10:30:00Z', }, }, ]; diff --git a/workspaces/scorecard/plugins/scorecard/dev/index.tsx b/workspaces/scorecard/plugins/scorecard/dev/index.tsx index a385b88d2e..ecb9c56fc1 100644 --- a/workspaces/scorecard/plugins/scorecard/dev/index.tsx +++ b/workspaces/scorecard/plugins/scorecard/dev/index.tsx @@ -20,8 +20,10 @@ import { Page, Header, TabbedLayout } from '@backstage/core-components'; import { TestApiProvider } from '@backstage/test-utils'; import { getAllThemes } from '@red-hat-developer-hub/backstage-plugin-theme'; import type { Entity } from '@backstage/catalog-model'; -import type { MetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; -import type { AggregatedMetricResult } from '../src/utils/utils'; +import type { + MetricResult, + AggregatedMetricResult, +} from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { scorecardPlugin, EntityScorecardContent } from '../src/plugin'; import { scorecardTranslations } from '../src/translations'; diff --git a/workspaces/scorecard/plugins/scorecard/src/api/index.ts b/workspaces/scorecard/plugins/scorecard/src/api/index.ts index b8d71150cd..abe1eb4d69 100644 --- a/workspaces/scorecard/plugins/scorecard/src/api/index.ts +++ b/workspaces/scorecard/plugins/scorecard/src/api/index.ts @@ -20,9 +20,11 @@ import { DiscoveryApi, } from '@backstage/core-plugin-api'; import type { Entity } from '@backstage/catalog-model'; -import type { MetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; +import type { + MetricResult, + AggregatedMetricResult, +} from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { mockAggregatedScorecardSuccessData } from '../../__fixtures__/aggregatedScorecardData'; -import type { AggregatedMetricResult } from '../utils/utils'; export interface ScorecardApi { /** diff --git a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageCard.tsx b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageCard.tsx index 0039dadaf8..2301ab87be 100644 --- a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageCard.tsx +++ b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageCard.tsx @@ -31,7 +31,8 @@ import { useTheme } from '@mui/material/styles'; import { CardWrapper } from '../Common/CardWrapper'; import { CustomTooltip } from './CustomTooltip'; import CustomLegend from './CustomLegend'; -import type { AggregatedMetricResult, PieData } from '../../utils/utils'; +import type { PieData } from '../../utils/utils'; +import type { AggregatedMetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; export const ScorecardHomepageCard = ({ scorecard, diff --git a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageSection.tsx b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageSection.tsx index a051e28d9b..2b4de7f121 100644 --- a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageSection.tsx +++ b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/ScorecardHomepageSection.tsx @@ -23,7 +23,7 @@ import CircularProgress from '@mui/material/CircularProgress'; import { ScorecardHomepageCard } from './ScorecardHomepageCard'; import PermissionRequiredState from '../Common/PermissionRequiredState'; import { useAggregatedScorecards } from '../../hooks/useAggregatedScorecards'; -import type { AggregatedMetricResult } from '../../utils/utils'; +import type { AggregatedMetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; export const ScorecardHomepageSection = () => { const { aggregatedScorecards, loadingData, error } = diff --git a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageCard.test.tsx b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageCard.test.tsx index 47716ab9c5..8076e1b3e2 100644 --- a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageCard.test.tsx +++ b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageCard.test.tsx @@ -19,7 +19,7 @@ import { ThemeProvider, createTheme } from '@mui/material/styles'; import { ScorecardHomepageCard } from '../ScorecardHomepageCard'; import { mockAggregatedScorecardSuccessData } from '../../../../__fixtures__/aggregatedScorecardData'; -import type { AggregatedMetricResult } from '../../../utils/utils'; +import type { AggregatedMetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; // Mock the child components jest.mock('../../Common/CardWrapper', () => ({ @@ -194,7 +194,7 @@ describe('ScorecardHomepageCard Component', () => { ...mockAggregatedScorecardSuccessData[0], result: { ...mockAggregatedScorecardSuccessData[0].result, - values: undefined, + values: [], total: 0, }, }; diff --git a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageSection.test.tsx b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageSection.test.tsx index f292b7806b..bc28400882 100644 --- a/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageSection.test.tsx +++ b/workspaces/scorecard/plugins/scorecard/src/components/ScorecardHomepageSection/__tests__/ScorecardHomepageSection.test.tsx @@ -19,7 +19,7 @@ import { ThemeProvider, createTheme } from '@mui/material/styles'; import { ScorecardHomepageSection } from '../ScorecardHomepageSection'; import { mockAggregatedScorecardSuccessData } from '../../../../__fixtures__/aggregatedScorecardData'; -import type { AggregatedMetricResult } from '../../../utils/utils'; +import type { AggregatedMetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; // Mock the child components jest.mock('../../Common/PermissionRequiredState', () => { @@ -130,14 +130,13 @@ describe('ScorecardHomepageSection Component', () => { metadata: { title: 'Third Scorecard', description: 'Third description', - type: 'object', + type: 'number', history: true, }, result: { values: [{ count: 5, name: 'success' }], total: 5, timestamp: '2024-01-15T10:30:00Z', - lastUpdated: '2024-01-15T10:30:00Z', }, }, ]; diff --git a/workspaces/scorecard/plugins/scorecard/src/hooks/useAggregatedScorecards.tsx b/workspaces/scorecard/plugins/scorecard/src/hooks/useAggregatedScorecards.tsx index fb9dd979a6..9eba537880 100644 --- a/workspaces/scorecard/plugins/scorecard/src/hooks/useAggregatedScorecards.tsx +++ b/workspaces/scorecard/plugins/scorecard/src/hooks/useAggregatedScorecards.tsx @@ -19,7 +19,7 @@ import { useApi } from '@backstage/core-plugin-api'; import useAsync from 'react-use/lib/useAsync'; import { scorecardApiRef } from '../api'; -import { AggregatedMetricResult } from '../utils/utils'; +import { AggregatedMetricResult } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; export const useAggregatedScorecards = () => { const scorecardApi = useApi(scorecardApiRef); diff --git a/workspaces/scorecard/plugins/scorecard/src/utils/utils.ts b/workspaces/scorecard/plugins/scorecard/src/utils/utils.ts index da10291e4c..78e08d30f9 100644 --- a/workspaces/scorecard/plugins/scorecard/src/utils/utils.ts +++ b/workspaces/scorecard/plugins/scorecard/src/utils/utils.ts @@ -57,25 +57,3 @@ export const getStatusConfig = ({ return { color: 'success.main', icon: CheckCircleOutlineIcon }; } }; - -export type AggregatedMetricValue = { - count: number; - name: 'success' | 'warning' | 'error'; -}; - -export type AggregatedMetricResult = { - id: string; - status: 'success' | 'error'; - metadata: { - title: string; - description: string; - type: 'object'; - history?: boolean; - }; - result: { - values?: AggregatedMetricValue[]; - total: number; - timestamp: string; - lastUpdated: string; - }; -}; From a1095658cabd7ecc079a5c8738d1a28b666c454d Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Thu, 8 Jan 2026 12:25:46 +0100 Subject: [PATCH 06/11] feat(scorecard): implemented endpoint to return aggregated metrics for a specific metric across all entities owned by the authenticated user Signed-off-by: Ihor Mykhno --- .../scorecard/examples/all-scorecards.yaml | 2 +- workspaces/scorecard/examples/entities.yaml | 6 +- .../examples/github-scorecard-only.yaml | 2 +- .../examples/jira-scorecard-only.yaml | 2 +- .../scorecard/examples/no-scorecards.yaml | 2 +- .../template/content/catalog-info.yaml | 2 +- .../plugins/scorecard-backend/README.md | 14 ++ .../scorecard-backend/docs/aggregation.md | 134 ++++++++++ .../src/service/router.test.ts | 233 ++++++++++++++++++ .../scorecard-backend/src/service/router.ts | 46 ++++ 10 files changed, 435 insertions(+), 8 deletions(-) create mode 100644 workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md diff --git a/workspaces/scorecard/examples/all-scorecards.yaml b/workspaces/scorecard/examples/all-scorecards.yaml index 53714f2851..137e23b3da 100644 --- a/workspaces/scorecard/examples/all-scorecards.yaml +++ b/workspaces/scorecard/examples/all-scorecards.yaml @@ -10,5 +10,5 @@ metadata: jira/project-key: RSPT spec: type: service - owner: janus-authors + owner: user:development/guest lifecycle: production diff --git a/workspaces/scorecard/examples/entities.yaml b/workspaces/scorecard/examples/entities.yaml index 447e8b1f34..15964d2c9b 100644 --- a/workspaces/scorecard/examples/entities.yaml +++ b/workspaces/scorecard/examples/entities.yaml @@ -5,7 +5,7 @@ kind: System metadata: name: examples spec: - owner: guests + owner: group:development/guests --- # https://backstage.io/docs/features/software-catalog/descriptor-format#kind-component apiVersion: backstage.io/v1alpha1 @@ -15,7 +15,7 @@ metadata: spec: type: website lifecycle: experimental - owner: guests + owner: group:development/guests system: examples providesApis: [example-grpc-api] --- @@ -27,7 +27,7 @@ metadata: spec: type: grpc lifecycle: experimental - owner: guests + owner: group:development/guests system: examples definition: | syntax = "proto3"; diff --git a/workspaces/scorecard/examples/github-scorecard-only.yaml b/workspaces/scorecard/examples/github-scorecard-only.yaml index 4a0d63902c..92ab19d9a4 100644 --- a/workspaces/scorecard/examples/github-scorecard-only.yaml +++ b/workspaces/scorecard/examples/github-scorecard-only.yaml @@ -9,5 +9,5 @@ metadata: backstage.io/source-location: url:https://github.com/redhat-developer/rhdh-plugins spec: type: service - owner: janus-authors + owner: group:development/guests lifecycle: production diff --git a/workspaces/scorecard/examples/jira-scorecard-only.yaml b/workspaces/scorecard/examples/jira-scorecard-only.yaml index c405b56be2..679d232590 100644 --- a/workspaces/scorecard/examples/jira-scorecard-only.yaml +++ b/workspaces/scorecard/examples/jira-scorecard-only.yaml @@ -8,5 +8,5 @@ metadata: jira/project-key: RSPT spec: type: service - owner: janus-authors + owner: group:development/guests lifecycle: production diff --git a/workspaces/scorecard/examples/no-scorecards.yaml b/workspaces/scorecard/examples/no-scorecards.yaml index 626ac0683e..779efb4e06 100644 --- a/workspaces/scorecard/examples/no-scorecards.yaml +++ b/workspaces/scorecard/examples/no-scorecards.yaml @@ -6,5 +6,5 @@ metadata: name: no-scorecards-service spec: type: service - owner: janus-authors + owner: group:development/guests lifecycle: production diff --git a/workspaces/scorecard/examples/template/content/catalog-info.yaml b/workspaces/scorecard/examples/template/content/catalog-info.yaml index d4ccca42ef..1fdc15a5e7 100644 --- a/workspaces/scorecard/examples/template/content/catalog-info.yaml +++ b/workspaces/scorecard/examples/template/content/catalog-info.yaml @@ -4,5 +4,5 @@ metadata: name: ${{ values.name | dump }} spec: type: service - owner: user:guest + owner: user:development/guest lifecycle: experimental diff --git a/workspaces/scorecard/plugins/scorecard-backend/README.md b/workspaces/scorecard/plugins/scorecard-backend/README.md index ce21286518..bed4f1eeec 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/README.md +++ b/workspaces/scorecard/plugins/scorecard-backend/README.md @@ -97,6 +97,20 @@ Thresholds are evaluated in order, and the first matching rule determines the ca For comprehensive threshold configuration guide, examples, and best practices, see [thresholds.md](./docs/thresholds.md). +## Entity Aggregation + +The Scorecard plugin provides aggregation endpoints that return metrics for all entities owned by the authenticated user. This includes: + +- Entities directly owned by the user +- Entities owned by groups the user is a direct member of (Only direct parent groups are considered) + +### Available Endpoints + +- **`GET /metrics/catalog/aggregates`**: Returns aggregated metrics for all available metrics (optionally filtered by `metricIds` query parameter) +- **`GET /metrics/:metricId/catalog/aggregation`**: Returns aggregated metrics for a specific metric, with explicit access validation (returns `403` if the user doesn't have access to the metric) + +For comprehensive documentation on how entity aggregation works, API details, examples, and best practices, see [aggregation.md](./docs/aggregation.md). + ## Configuration cleanup Job The plugin has a predefined job that runs every day to check and clean old metrics. By default, metrics are saved for **365 days**, however, this period can be changed in the `app-config.yaml` file. Here is an example of how to do that: diff --git a/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md b/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md new file mode 100644 index 0000000000..bc8da9aea6 --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md @@ -0,0 +1,134 @@ +# Entity Aggregation + +The Scorecard plugin provides aggregation endpoints that return metrics aggregated across all entities owned by the authenticated user. This feature allows users to get a consolidated view of metrics across their entire portfolio of owned entities. + +## Overview + +The aggregation endpoints (`/metrics/catalog/aggregates` and `/metrics/:metricId/catalog/aggregation`) aggregate metrics from multiple entities based on entity ownership. They collect metrics from: + +- Entities directly owned by the user +- Entities owned by groups the user is a direct member of + +The aggregation counts how many entities fall into each threshold category (`success`, `warning`, `error`) for each metric, providing a high-level overview of the health status across all owned entities. + +### Important Limitation: Direct Parent Groups Only + +**Only direct parent groups are considered.** The aggregation does not traverse nested group hierarchies. + +**Example:** + +Consider the following group structure: + +- User `alice` is a member of `group:default/developers` +- `group:default/developers` is a member of `group:default/engineering` + +In this case: + +- ✅ Entities owned by `alice` directly are included +- ✅ Entities owned by `group:default/developers` are included +- ❌ Entities owned by `group:default/engineering` are **NOT** included + +## API Endpoints + +### `GET /metrics/catalog/aggregates` + +Returns aggregated metrics for all entities owned by the authenticated user. + +#### Query Parameters + +| Parameter | Type | Required | Description | +| ----------- | ------ | -------- | -------------------------------------------------------------------------------------------- | +| `metricIds` | string | No | Comma-separated list of metric IDs to filter. If not provided, returns all available metrics | + +#### Authentication + +Requires user authentication. The endpoint uses the authenticated user's entity reference to determine which entities to aggregate. + +#### Permissions + +Requires `scorecard.metric.read` permission. Additionally, the user must have `catalog.entity.read` permission for each entity that will be included in the aggregation. + +#### Example Request + +```bash +# Get all aggregated metrics +curl -X GET "{{url}}/api/scorecard/metrics/catalog/aggregates" \ + -H "Authorization: Bearer " + +# Get specific metrics +curl -X GET "{{url}}/api/scorecard/metrics/catalog/aggregates?metricIds=github.open_prs,jira.open_issues" \ + -H "Authorization: Bearer " +``` + +### `GET /metrics/:metricId/catalog/aggregation` + +Returns aggregated metrics for a specific metric across all entities owned by the authenticated user. This endpoint is useful when you need to check access to a specific metric and get its aggregation without requiring the `metricIds` query parameter. + +#### Path Parameters + +| Parameter | Type | Required | Description | +| ---------- | ------ | -------- | --------------------------------- | +| `metricId` | string | Yes | The ID of the metric to aggregate | + +#### Authentication + +Requires user authentication. The endpoint uses the authenticated user's entity reference to determine which entities to aggregate. + +#### Permissions + +Requires `scorecard.metric.read` permission. Additionally: + +- The user must have access to the specific metric (returns `403 Forbidden` if access is denied) +- The user must have `catalog.entity.read` permission for each entity that will be included in the aggregation + +#### Example Request + +```bash +# Get aggregated metrics for a specific metric +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregation" \ + -H "Authorization: Bearer " +``` + +#### Differences from `/metrics/catalog/aggregates` + +- **Metric Access Validation**: This endpoint explicitly validates that the user has access to the specified metric and returns `403 Forbidden` if access is denied +- **Single Metric Only**: Returns aggregation for only the specified metric (no need for `metricIds` query parameter) +- **Empty Results Handling**: Returns an empty array `[]` when the user owns no entities, avoiding errors when filtering by a single metric + +## Error Handling + +### Missing User Entity Reference + +If the authenticated user doesn't have an entity reference in the catalog: + +- **Status Code**: `403 Forbidden` +- **Error**: `NotAllowedError: User entity reference not found` + +### Permission Denied + +If the user doesn't have permission to read a specific entity: + +- **Status Code**: `403 Forbidden` +- **Error**: Permission denied for the specific entity + +### Metric Access Denied (for `/metrics/:metricId/catalog/aggregation`) + +If the user doesn't have access to the specified metric: + +- **Status Code**: `403 Forbidden` +- **Error**: `NotAllowedError: Access to metric "" denied` + +### Invalid Query Parameters + +If invalid query parameters are provided: + +- **Status Code**: `400 Bad Request` +- **Error**: Validation error details + +## Best Practices + +1. **Use Metric Filtering**: When you only need specific metrics, use the `metricIds` parameter to reduce response size and improve performance + +2. **Handle Empty Results**: Always check for empty arrays when the user owns no entities + +3. **Group Structure**: Be aware of the direct parent group limitation when designing your group hierarchy. If you need nested group aggregation, consider restructuring your groups or implementing custom logic diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts index 649294fc00..6d834c5b07 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -582,4 +582,237 @@ describe('createRouter', () => { ); }); }); + + describe('GET /metrics/:metricId/catalog/aggregation', () => { + const mockAggregatedMetricResults: AggregatedMetricResult[] = [ + { + id: 'github.open_prs', + status: 'success', + metadata: { + title: 'GitHub open PRs', + description: + 'Current count of open Pull Requests for a given GitHub repository.', + type: 'number', + history: true, + }, + result: { + values: [ + { count: 5, name: 'success' }, + { count: 4, name: 'warning' }, + { count: 3, name: 'error' }, + ], + total: 12, + timestamp: '2025-01-01T10:30:00.000Z', + }, + }, + ]; + let mockCatalog: ReturnType; + let getEntitiesOwnedByUserSpy: jest.SpyInstance; + let checkEntityAccessSpy: jest.SpyInstance; + let aggregationApp: express.Express; + + beforeEach(async () => { + const githubProvider = new MockNumberProvider( + 'github.open_prs', + 'github', + 'GitHub Open PRs', + ); + const jiraProvider = new MockNumberProvider( + 'jira.open_issues', + 'jira', + 'Jira Open Issues', + ); + metricProvidersRegistry.register(githubProvider); + metricProvidersRegistry.register(jiraProvider); + + mockCatalog = catalogServiceMock.mock(); + + const userEntity = new MockEntityBuilder() + .withKind('User') + .withMetadata({ name: 'test-user', namespace: 'default' }) + .build(); + mockCatalog.getEntityByRef.mockResolvedValue(userEntity); + + const componentEntity = new MockEntityBuilder() + .withKind('Component') + .withMetadata({ name: 'my-service', namespace: 'default' }) + .build(); + mockCatalog.getEntities.mockResolvedValue({ items: [componentEntity] }); + + jest + .spyOn(catalogMetricService, 'getAggregatedMetricsByEntityRefs') + .mockResolvedValue(mockAggregatedMetricResults); + + getEntitiesOwnedByUserSpy = jest + .spyOn(getEntitiesOwnedByUserModule, 'getEntitiesOwnedByUser') + .mockResolvedValue([ + 'component:default/my-service', + 'component:default/my-other-service', + ]); + + checkEntityAccessSpy = jest.spyOn( + permissionUtilsModule, + 'checkEntityAccess', + ); + + const router = await createRouter({ + metricProvidersRegistry, + catalogMetricService, + catalog: mockCatalog, + httpAuth: httpAuthMock, + permissions: permissionsMock, + }); + aggregationApp = express(); + aggregationApp.use(router); + aggregationApp.use(mockErrorHandler()); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should return 403 Unauthorized when DENY permissions', async () => { + permissionsMock.authorizeConditional.mockResolvedValue([ + { result: AuthorizeResult.DENY }, + ]); + const result = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(result.statusCode).toBe(403); + expect(result.body.error.name).toEqual('NotAllowedError'); + }); + + it('should return 403 NotAllowedError when user entity reference is not found', async () => { + httpAuthMock.credentials.mockResolvedValue({ + principal: {}, + } as any); + const result = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(result.statusCode).toBe(403); + expect(result.body.error.name).toEqual('NotAllowedError'); + expect(result.body.error.message).toContain( + 'User entity reference not found', + ); + }); + + it('should return 403 NotAllowedError when user does not have access to the metric', async () => { + permissionsMock.authorizeConditional.mockResolvedValue([ + CONDITIONAL_POLICY_DECISION, + ]); + const result = await request(aggregationApp).get( + '/metrics/jira.open_issues/catalog/aggregation', + ); + + expect(result.statusCode).toBe(403); + expect(result.body.error.name).toEqual('NotAllowedError'); + expect(result.body.error.message).toContain( + 'Access to metric "jira.open_issues" denied', + ); + }); + + it('should return aggregated metrics for a specific metric', async () => { + const response = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(response.status).toBe(200); + expect( + catalogMetricService.getAggregatedMetricsByEntityRefs, + ).toHaveBeenCalledWith( + ['component:default/my-service', 'component:default/my-other-service'], + ['github.open_prs'], + undefined, + ); + expect(response.body).toEqual(mockAggregatedMetricResults); + }); + + it('should get entities owned by user', async () => { + const response = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(response.status).toBe(200); + expect(getEntitiesOwnedByUserSpy).toHaveBeenCalledWith( + 'user:default/test-user', + expect.objectContaining({ + catalog: expect.any(Object), + credentials: expect.any(Object), + }), + ); + }); + + it('should return empty array when user owns no entities', async () => { + getEntitiesOwnedByUserSpy.mockResolvedValue([]); + const response = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(response.status).toBe(200); + expect(response.body).toEqual([]); + expect( + catalogMetricService.getAggregatedMetricsByEntityRefs, + ).not.toHaveBeenCalled(); + }); + + it('should check entity access for each entity owned by user', async () => { + const response = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(checkEntityAccessSpy).toHaveBeenCalledTimes(2); + expect(checkEntityAccessSpy).toHaveBeenCalledWith( + 'component:default/my-service', + expect.any(Object), + permissionsMock, + httpAuthMock, + ); + expect(checkEntityAccessSpy).toHaveBeenCalledWith( + 'component:default/my-other-service', + expect.any(Object), + permissionsMock, + httpAuthMock, + ); + expect(response.status).toBe(200); + expect(response.body).toEqual(mockAggregatedMetricResults); + }); + + it('should filter authorized metrics when CONDITIONAL permission', async () => { + permissionsMock.authorizeConditional.mockResolvedValue([ + CONDITIONAL_POLICY_DECISION, + ]); + const response = await request(aggregationApp).get( + '/metrics/github.open_prs/catalog/aggregation', + ); + + expect(response.status).toBe(200); + expect( + catalogMetricService.getAggregatedMetricsByEntityRefs, + ).toHaveBeenCalledWith( + ['component:default/my-service', 'component:default/my-other-service'], + ['github.open_prs'], + { + anyOf: [ + { + rule: 'HAS_METRIC_ID', + resourceType: 'scorecard-metric', + params: { metricIds: ['github.open_prs', 'github.open_issues'] }, + }, + ], + }, + ); + }); + + it('should return 404 NotFoundError when metric is not found', async () => { + const result = await request(aggregationApp).get( + '/metrics/non.existent.metric/catalog/aggregation', + ); + + expect(result.statusCode).toBe(404); + expect(result.body.error.name).toBe('NotFoundError'); + expect(result.body.error.message).toContain('Metric provider with ID'); + }); + }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index f8e5369c21..683c3eb88a 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -196,5 +196,51 @@ export async function createRouter({ res.json(aggregatedMetrics); }); + router.get('/metrics/:metricId/catalog/aggregation', async (req, res) => { + const { metricId } = req.params; + + const { conditions } = await authorizeConditional( + req, + scorecardMetricReadPermission, + ); + + const metric = metricProvidersRegistry.getMetric(metricId); + const authorizedMetrics = filterAuthorizedMetrics([metric], conditions); + + if (authorizedMetrics.length === 0) { + throw new NotAllowedError(`Access to metric "${metricId}" denied`); + } + + 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, + credentials, + }); + + if (entitiesOwnedByAUser.length === 0) { + res.json([]); + return; + } + + for (const entityRef of entitiesOwnedByAUser) { + await checkEntityAccess(entityRef, req, permissions, httpAuth); + } + + const aggregatedMetrics = + await catalogMetricService.getAggregatedMetricsByEntityRefs( + entitiesOwnedByAUser, + [metricId], + conditions, + ); + + res.json(aggregatedMetrics); + }); + return router; } From f5eb65981e2fdd71b0dd8af59f62142c49d1dba9 Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Thu, 8 Jan 2026 13:20:34 +0100 Subject: [PATCH 07/11] fix(scorecard) tests and sonarqube issues Signed-off-by: Ihor Mykhno --- .../src/database/DatabaseMetricValues.ts | 28 ++++++++++++++++++- .../src/service/CatalogMetricService.ts | 2 +- .../src/utils/getEntitiesOwnedByUser.test.ts | 22 --------------- 3 files changed, 28 insertions(+), 24 deletions(-) diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts index 555f5a858f..46652f51fc 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts @@ -74,7 +74,7 @@ export class DatabaseMetricValues { .whereIn('catalog_entity_ref', catalog_entity_refs) .groupBy('metric_id', 'catalog_entity_ref'); - return await this.dbClient(this.tableName) + const results = await this.dbClient(this.tableName) .select('metric_id') .count('* as total') .max('timestamp as max_timestamp') @@ -97,5 +97,31 @@ export class DatabaseMetricValues { .whereNotNull('status') .whereNotNull('value') .groupBy('metric_id'); + + // Normalize types for cross-database compatibility + // PostgreSQL returns COUNT/SUM as strings, SQLite returns numbers + // PostgreSQL returns MAX(timestamp) as Date, SQLite returns number (milliseconds) + return results.map(row => { + let maxTimestamp: Date; + if (row.max_timestamp instanceof Date) { + maxTimestamp = row.max_timestamp; + } else if ( + typeof row.max_timestamp === 'number' || + typeof row.max_timestamp === 'string' + ) { + maxTimestamp = new Date(row.max_timestamp as string | number); + } else { + maxTimestamp = new Date(); + } + + return { + metric_id: row.metric_id, + total: Number(row.total), + max_timestamp: maxTimestamp, + success: Number(row.success), + warning: Number(row.warning), + error: Number(row.error), + }; + }); } } diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts index 7b51a443ad..16ebc51163 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -172,7 +172,7 @@ export class CatalogMetricService { ); return aggregatedMetrics.map(row => { - const metricId = row.metric_id as string; + const metricId = row.metric_id; const success = row.success || 0; const warning = row.warning || 0; const error = row.error || 0; diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.test.ts index 6059bf9a23..0e66a739fe 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/getEntitiesOwnedByUser.test.ts @@ -340,27 +340,5 @@ describe('getEntitiesOwnedByUser', () => { expect(result).toEqual([]); expect(mockedCatalog.queryEntities).toHaveBeenCalledTimes(2); }); - - // it('should call queryEntities with cursor', async () => { - // mockedCatalog.queryEntities.mockResolvedValue({ items: [userOwnedEntity], pageInfo: { nextCursor: 'cursor1' }, totalItems: 1 }); - // mockedCatalog.queryEntities.mockResolvedValue({ items: [groupOwnedEntity], pageInfo: { nextCursor: undefined }, totalItems: 1 }); - - // await getEntitiesOwnedByUser('user:development/test-user', { - // catalog: mockedCatalog, - // credentials: mockCredentials, - // }); - - // expect(mockedCatalog.queryEntities).toHaveBeenCalledTimes(2); - // expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( - // 1, - // { cursor: 'cursor1' }, - // { credentials: mockCredentials }, - // ); - // expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( - // 2, - // { cursor: 'cursor2' }, - // { credentials: mockCredentials }, - // ); - // }); }); }); From e9a27542610b253ce809271841dab46aa50a444c Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Thu, 8 Jan 2026 17:05:36 +0100 Subject: [PATCH 08/11] refactor(scorecard): remove unused endpoint Signed-off-by: Ihor Mykhno --- .../src/service/router.test.ts | 209 ------------------ .../scorecard-backend/src/service/router.ts | 46 ---- 2 files changed, 255 deletions(-) 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 6d834c5b07..7ef8e0649a 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -374,215 +374,6 @@ describe('createRouter', () => { }); }); - describe('GET /metrics/catalog/aggregates', () => { - const mockAggregatedMetricResults: AggregatedMetricResult[] = [ - { - id: 'github.open_prs', - status: 'success', - metadata: { - title: 'GitHub open PRs', - description: - 'Current count of open Pull Requests for a given GitHub repository.', - type: 'number', - history: true, - }, - result: { - values: [ - { count: 5, name: 'success' }, - { count: 4, name: 'warning' }, - { count: 3, name: 'error' }, - ], - total: 12, - timestamp: '2025-01-01T10:30:00.000Z', - }, - }, - ]; - let mockCatalog: ReturnType; - let getEntitiesOwnedByUserSpy: jest.SpyInstance; - let checkEntityAccessSpy: jest.SpyInstance; - let aggregatesApp: express.Express; - - beforeEach(async () => { - const githubProvider = new MockNumberProvider( - 'github.open_prs', - 'github', - 'GitHub Open PRs', - ); - const jiraProvider = new MockNumberProvider( - 'jira.open_issues', - 'jira', - 'Jira Open Issues', - ); - metricProvidersRegistry.register(githubProvider); - metricProvidersRegistry.register(jiraProvider); - - mockCatalog = catalogServiceMock.mock(); - - const userEntity = new MockEntityBuilder() - .withKind('User') - .withMetadata({ name: 'test-user', namespace: 'default' }) - .build(); - mockCatalog.getEntityByRef.mockResolvedValue(userEntity); - - const componentEntity = new MockEntityBuilder() - .withKind('Component') - .withMetadata({ name: 'my-service', namespace: 'default' }) - .build(); - mockCatalog.getEntities.mockResolvedValue({ items: [componentEntity] }); - - jest - .spyOn(catalogMetricService, 'getAggregatedMetricsByEntityRefs') - .mockResolvedValue(mockAggregatedMetricResults); - - getEntitiesOwnedByUserSpy = jest - .spyOn(getEntitiesOwnedByUserModule, 'getEntitiesOwnedByUser') - .mockResolvedValue([ - 'component:default/my-service', - 'component:default/my-other-service', - ]); - - checkEntityAccessSpy = jest.spyOn( - permissionUtilsModule, - 'checkEntityAccess', - ); - - const router = await createRouter({ - metricProvidersRegistry, - catalogMetricService, - catalog: mockCatalog, - httpAuth: httpAuthMock, - permissions: permissionsMock, - }); - aggregatesApp = express(); - aggregatesApp.use(router); - aggregatesApp.use(mockErrorHandler()); - }); - - afterEach(() => { - jest.clearAllMocks(); - }); - - it('should return 403 Unauthorized when DENY permissions', async () => { - permissionsMock.authorizeConditional.mockResolvedValue([ - { result: AuthorizeResult.DENY }, - ]); - const result = await request(aggregatesApp).get( - '/metrics/catalog/aggregates', - ); - - 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(aggregatesApp).get( - '/metrics/catalog/aggregates', - ); - - 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(aggregatesApp).get( - '/metrics/catalog/aggregates?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'], - undefined, - ); - expect(response.body).toEqual(mockAggregatedMetricResults); - }); - - it('should handle single metricIds parameter', async () => { - const response = await request(aggregatesApp).get( - '/metrics/catalog/aggregates?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'], - undefined, - ); - expect(response.body).toEqual(mockAggregatedMetricResults); - }); - - it('should get entities owned by user', async () => { - const response = await request(aggregatesApp).get( - '/metrics/catalog/aggregates', - ); - - 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(aggregatesApp).get( - '/metrics/catalog/aggregates', - ); - - 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(aggregatesApp).get( - '/metrics/catalog/aggregates', - ); - - 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(aggregatesApp).get( - '/metrics/catalog/aggregates?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'], - undefined, - ); - }); - - it('should set parsed metricIds to undefined when metricIds parameter is not string', async () => { - const response = await request(aggregatesApp).get( - '/metrics/catalog/aggregates?anotherParameter=value', - ); - - expect(response.status).toBe(200); - expect( - catalogMetricService.getAggregatedMetricsByEntityRefs, - ).toHaveBeenCalledWith( - ['component:default/my-service', 'component:default/my-other-service'], - undefined, - undefined, - ); - }); - }); - describe('GET /metrics/:metricId/catalog/aggregation', () => { const mockAggregatedMetricResults: AggregatedMetricResult[] = [ { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index 683c3eb88a..6d0399026b 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -150,52 +150,6 @@ export async function createRouter({ res.json(results); }); - router.get('/metrics/catalog/aggregates', async (req, res) => { - const { metricIds } = req.query; - - const { conditions } = 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, - 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, - conditions, - ); - - res.json(aggregatedMetrics); - }); - router.get('/metrics/:metricId/catalog/aggregation', async (req, res) => { const { metricId } = req.params; From 0cbabf6f88952225ed66b8b63e2fcdda88c678b4 Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Fri, 9 Jan 2026 09:16:16 +0100 Subject: [PATCH 09/11] fix(scorecard): error types and message for aggregation endpoint Signed-off-by: Ihor Mykhno --- .../plugins/scorecard-backend/src/service/router.test.ts | 8 ++++---- .../plugins/scorecard-backend/src/service/router.ts | 8 +++++--- 2 files changed, 9 insertions(+), 7 deletions(-) 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 7ef8e0649a..d2c6b326b5 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -474,7 +474,7 @@ describe('createRouter', () => { expect(result.body.error.name).toEqual('NotAllowedError'); }); - it('should return 403 NotAllowedError when user entity reference is not found', async () => { + it('should return 404 NotFoundError when user entity reference is not found', async () => { httpAuthMock.credentials.mockResolvedValue({ principal: {}, } as any); @@ -482,8 +482,8 @@ describe('createRouter', () => { '/metrics/github.open_prs/catalog/aggregation', ); - expect(result.statusCode).toBe(403); - expect(result.body.error.name).toEqual('NotAllowedError'); + expect(result.statusCode).toBe(404); + expect(result.body.error.name).toEqual('NotFoundError'); expect(result.body.error.message).toContain( 'User entity reference not found', ); @@ -500,7 +500,7 @@ describe('createRouter', () => { expect(result.statusCode).toBe(403); expect(result.body.error.name).toEqual('NotAllowedError'); expect(result.body.error.message).toContain( - 'Access to metric "jira.open_issues" denied', + 'To view the scorecard metrics, your administrator must grant you the required permission.', ); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index 6d0399026b..819e2a1501 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { InputError, NotAllowedError } from '@backstage/errors'; +import { InputError, NotAllowedError, NotFoundError } from '@backstage/errors'; import { z } from 'zod'; import express, { Request } from 'express'; import Router from 'express-promise-router'; @@ -162,14 +162,16 @@ export async function createRouter({ const authorizedMetrics = filterAuthorizedMetrics([metric], conditions); if (authorizedMetrics.length === 0) { - throw new NotAllowedError(`Access to metric "${metricId}" denied`); + throw new NotAllowedError( + `To view the scorecard metrics, your administrator must grant you the required permission.`, + ); } const credentials = await httpAuth.credentials(req, { allow: ['user'] }); const userEntityRef = credentials?.principal?.userEntityRef; if (!userEntityRef) { - throw new NotAllowedError('User entity reference not found'); + throw new NotFoundError('User entity reference not found'); } const entitiesOwnedByAUser = await getEntitiesOwnedByUser(userEntityRef, { From 670e734998e8914fbcebbcece4d05fc2d043188b Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Fri, 9 Jan 2026 10:46:26 +0100 Subject: [PATCH 10/11] feat(scorecard): e2e tests by updating guest user membership Signed-off-by: Ihor Mykhno --- workspaces/scorecard/examples/org.yaml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/workspaces/scorecard/examples/org.yaml b/workspaces/scorecard/examples/org.yaml index c725294468..a928359991 100644 --- a/workspaces/scorecard/examples/org.yaml +++ b/workspaces/scorecard/examples/org.yaml @@ -6,7 +6,7 @@ metadata: name: guest namespace: development spec: - memberOf: [guests] + memberOf: [group:development/guests, group:default/red-hat] --- # https://backstage.io/docs/features/software-catalog/descriptor-format#kind-group apiVersion: backstage.io/v1alpha1 @@ -17,3 +17,12 @@ metadata: spec: type: team children: [] +--- +apiVersion: backstage.io/v1alpha1 +kind: Group +metadata: + name: red-hat + namespace: default +spec: + type: team + children: [] \ No newline at end of file From d3c2cdb7e0534763192b0268fb44837715ab54e7 Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Fri, 9 Jan 2026 10:52:56 +0100 Subject: [PATCH 11/11] fix(scorecard): prettier error --- workspaces/scorecard/examples/org.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workspaces/scorecard/examples/org.yaml b/workspaces/scorecard/examples/org.yaml index a928359991..f11c42cace 100644 --- a/workspaces/scorecard/examples/org.yaml +++ b/workspaces/scorecard/examples/org.yaml @@ -25,4 +25,4 @@ metadata: namespace: default spec: type: team - children: [] \ No newline at end of file + children: []