From 63664f994771ede464762cd756db65362579da33 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 10:00:54 -0500 Subject: [PATCH 01/17] feat(scorecard): add the ability to drill down into aggregated scorecard data Signed-off-by: Patrick Knight --- ...60217152637_add_entity_metadata_columns.js | 41 ++++ .../src/database/DatabaseMetricValues.ts | 61 +++++ .../scorecard-backend/src/database/types.ts | 4 + .../plugins/scorecard-backend/src/plugin.ts | 1 + .../tasks/PullMetricsByProviderTask.ts | 4 + .../src/service/CatalogMetricService.ts | 214 +++++++++++++++++- .../scorecard-backend/src/service/router.ts | 85 +++++++ .../scorecard-common/src/types/Metric.ts | 35 +++ 8 files changed, 444 insertions(+), 1 deletion(-) create mode 100644 workspaces/scorecard/plugins/scorecard-backend/migrations/20260217152637_add_entity_metadata_columns.js diff --git a/workspaces/scorecard/plugins/scorecard-backend/migrations/20260217152637_add_entity_metadata_columns.js b/workspaces/scorecard/plugins/scorecard-backend/migrations/20260217152637_add_entity_metadata_columns.js new file mode 100644 index 0000000000..18fa31ac3d --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/migrations/20260217152637_add_entity_metadata_columns.js @@ -0,0 +1,41 @@ +/* + * 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. + */ + +exports.up = async function up(knex) { + await knex.schema.alterTable('metric_values', table => { + // Add entity_kind column (e.g., "Component", "API", "System") + table.string('entity_kind', 255).nullable(); + + // Add entity_owner column (stores the owner entity ref) + table.string('entity_owner', 255).nullable(); + + // Optional: Add index for better filtering performance + table.index(['entity_kind'], 'idx_metric_values_entity_kind'); + table.index(['entity_owner'], 'idx_metric_values_entity_owner'); + }); +}; + +exports.down = async function down(knex) { + await knex.schema.alterTable('metric_values', table => { + // Drop indexes first + table.dropIndex([], 'idx_metric_values_entity_kind'); + table.dropIndex([], 'idx_metric_values_entity_owner'); + + // Drop columns + table.dropColumn('entity_kind'); + table.dropColumn('entity_owner'); + }); +}; diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts index 5b40fbee15..18ca88e4c1 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts @@ -129,4 +129,65 @@ export class DatabaseMetricValues { return undefined; } + + /** + * Fetch entity metric values filtered by status with pagination + * Returns both the paginated rows and total count for pagination + */ + async readEntityMetricsByStatus( + catalog_entity_refs: string[], + metric_id: string, + status?: 'success' | 'warning' | 'error', + entityKind?: string, + entityOwner?: string, + pagination?: { limit: number; offset: number }, + ): Promise<{ rows: DbMetricValue[]; total: number }> { + // Build subquery for latest metric IDs + const latestIdsSubquery = this.dbClient(this.tableName) + .max('id') + .where('metric_id', metric_id); + + // Only add entity ref filter if provided (non-empty array) + if (catalog_entity_refs.length > 0) { + latestIdsSubquery.whereIn('catalog_entity_ref', catalog_entity_refs); + } + + latestIdsSubquery.groupBy('metric_id', 'catalog_entity_ref'); + + const query = this.dbClient(this.tableName) + .select('*') + .select(this.dbClient.raw('COUNT(*) OVER() as total_count')) + .whereIn('id', latestIdsSubquery) + .where('metric_id', metric_id); + + // Only add entity ref filter if provided + if (catalog_entity_refs.length > 0) { + query.whereIn('catalog_entity_ref', catalog_entity_refs); + } + + query.orderBy('timestamp', 'desc'); + + if (status) { + query.where('status', status); + } + + // Filter by entity_kind + if (entityKind) { + query.where('entity_kind', entityKind); + } + + // Filter by entity_owner + if (entityOwner) { + query.where('entity_owner', entityOwner); + } + + if (pagination) { + query.limit(pagination.limit).offset(pagination.offset); + } + + const rows = await query; + const total = rows.length > 0 ? Number(rows[0].total_count) : 0; + + return { rows, total }; + } } diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts index 62c5576c8b..aca204c46a 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/types.ts @@ -25,6 +25,8 @@ export type DbMetricValueCreate = { timestamp: Date; error_message?: string; status?: DbMetricValueStatus; + entity_kind?: string; + entity_owner?: string; }; export type DbMetricValue = { @@ -35,6 +37,8 @@ export type DbMetricValue = { timestamp: Date; error_message: string | null; status: DbMetricValueStatus | null; + entity_kind: string | null; + entity_owner: string | null; }; export type DbAggregatedMetric = { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts b/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts index 39d1840c34..6cc805e4ed 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts @@ -98,6 +98,7 @@ export const scorecardPlugin = createBackendPlugin({ auth, registry: metricProvidersRegistry, database: dbMetricValues, + logger: logger, }); Scheduler.create({ 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 230b5b86ad..041547b40e 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts @@ -161,6 +161,8 @@ export class PullMetricsByProviderTask implements SchedulerTask { value, timestamp: new Date(), status, + entity_kind: entity.kind, + entity_owner: JSON.stringify(entity?.spec?.owner) ?? undefined, } as DbMetricValueCreate; } catch (error) { return { @@ -170,6 +172,8 @@ export class PullMetricsByProviderTask implements SchedulerTask { timestamp: new Date(), error_message: error instanceof Error ? error.message : String(error), + entity_kind: entity.kind, + entity_owner: JSON.stringify(entity?.spec?.owner) ?? undefined, } as DbMetricValueCreate; } }), diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts index 1be8d6fd5d..305f53ba3b 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -18,10 +18,12 @@ import { MetricResult, ThresholdConfig, AggregatedMetric, + EntityMetricDetailResponse, + EntityMetricDetail, } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { MetricProvidersRegistry } from '../providers/MetricProvidersRegistry'; import { NotFoundError, stringifyError } from '@backstage/errors'; -import { AuthService } from '@backstage/backend-plugin-api'; +import { AuthService, LoggerService } from '@backstage/backend-plugin-api'; import { filterAuthorizedMetrics } from '../permissions/permissionUtils'; import { PermissionCondition, @@ -32,12 +34,14 @@ import { CatalogService } from '@backstage/plugin-catalog-node'; import { DatabaseMetricValues } from '../database/DatabaseMetricValues'; import { mergeEntityAndProviderThresholds } from '../utils/mergeEntityAndProviderThresholds'; import { AggregatedMetricMapper } from './mappers'; +import { Entity } from '@backstage/catalog-model'; type CatalogMetricServiceOptions = { catalog: CatalogService; auth: AuthService; registry: MetricProvidersRegistry; database: DatabaseMetricValues; + logger: LoggerService; }; export type AggregatedMetricsByStatus = Record< @@ -46,6 +50,8 @@ export type AggregatedMetricsByStatus = Record< >; export class CatalogMetricService { + private readonly logger: LoggerService; + private readonly catalog: CatalogService; private readonly auth: AuthService; private readonly registry: MetricProvidersRegistry; @@ -56,6 +62,7 @@ export class CatalogMetricService { this.auth = options.auth; this.registry = options.registry; this.database = options.database; + this.logger = options.logger; } /** @@ -167,4 +174,209 @@ export class CatalogMetricService { return AggregatedMetricMapper.toAggregatedMetric(); } + + /** + * Get detailed entity metrics for drill-down with filtering, sorting, and pagination. + * + * Fetches individual entity metric values and enriches them with catalog metadata. + * Supports database-level filtering (status, owner, kind) and application-level + * filtering (entityName). Falls back to database values if catalog is unavailable. + * + * @param entityRefs - Array of entity references to scope the query. Empty array fetches all entities. + * @param metricId - Metric ID to fetch (e.g., "github.open_prs") + * @param options - Query options for filtering, sorting, and pagination + * @param options.status - Filter by threshold status (database-level) + * @param options.owner - Filter by owner entity reference (database-level) + * @param options.kind - Filter by entity kind (database-level) + * @param options.entityName - Search entity names by substring (application-level) + * @param options.sortBy - Field to sort by (default: "timestamp") + * @param options.sortOrder - Sort direction: "asc" or "desc" (default: "desc") + * @param options.page - Page number (1-indexed) + * @param options.limit - Entities per page (max: 100) + * @returns Paginated entity metric details with metadata + */ + async getEntityMetricDetails( + entityRefs: string[], + metricId: string, + options: { + status?: 'success' | 'warning' | 'error'; + owner?: string; + kind?: string; + entityName?: string; + sortBy?: + | 'entityName' + | 'owner' + | 'entityKind' + | 'timestamp' + | 'metricValue'; + sortOrder?: 'asc' | 'desc'; + page: number; + limit: number; + }, + ): Promise { + // Determine if we need application-level filtering + const needsAppFiltering = !!options.entityName; + + // If we need app-level filtering (entityName), fetch ALL results + // Otherwise, paginate at DB (status, kind, owner are DB-filtered) + const dbPagination = needsAppFiltering + ? undefined // Fetch all for entityName filtering + : { + limit: options.limit, + offset: (options.page - 1) * options.limit, + }; + + // Fetch raw metric data from database + const { rows, total: dbTotal } = + await this.database.readEntityMetricsByStatus( + entityRefs, + metricId, + options.status, + options.kind, + options.owner, + dbPagination, + ); + + // Get metric metadata + const metric = this.registry.getMetric(metricId); + + // Batch-fetch entities from catalog + const entityRefsToFetch = rows.map(row => row.catalog_entity_ref); + const entityMap = new Map(); + + if (entityRefsToFetch.length > 0) { + try { + const response = await this.catalog.getEntitiesByRefs( + { + entityRefs: entityRefsToFetch, + fields: ['kind', 'metadata', 'spec'], // Only fetch needed fields + }, + { credentials: await this.auth.getOwnServiceCredentials() }, + ); + + // Build map of ref -> entity + // response.items is in same order as entityRefsToFetch + entityRefsToFetch.forEach((ref, index) => { + const entity = response.items[index]; + if (entity) { + entityMap.set(ref, entity); + } + }); + } catch (error) { + // Log error but continue with empty map (entities will show as "Unknown") + this.logger.warn('Failed to fetch entities from catalog', { error }); + } + } + + // Enrich rows with catalog data + const enrichedEntities: EntityMetricDetail[] = rows.map(row => { + const entity = entityMap.get(row.catalog_entity_ref); + + return { + entityRef: row.catalog_entity_ref, + entityName: entity?.metadata?.name ?? 'Unknown', + entityKind: entity?.kind ?? row.entity_kind ?? 'Unknown', + owner: (entity?.spec?.owner as string) ?? row.entity_owner ?? 'Unknown', + metricValue: row.value, + timestamp: new Date(row.timestamp).toISOString(), + status: row.status!, + }; + }); + + // Apply application-level filters + let filteredEntities = enrichedEntities; + + if (options.entityName) { + const searchTerm = options.entityName.toLowerCase(); + filteredEntities = filteredEntities.filter(e => + e.entityName.toLowerCase().includes(searchTerm), + ); + } + + if (options.sortBy) { + filteredEntities.sort((a, b) => { + let aValue: any; + let bValue: any; + + switch (options.sortBy) { + case 'entityName': + aValue = a.entityName.toLowerCase(); + bValue = b.entityName.toLowerCase(); + break; + case 'owner': + aValue = a.owner.toLowerCase(); + bValue = b.owner.toLowerCase(); + break; + case 'entityKind': + aValue = a.entityKind.toLowerCase(); + bValue = b.entityKind.toLowerCase(); + break; + case 'timestamp': + aValue = new Date(a.timestamp).getTime(); + bValue = new Date(b.timestamp).getTime(); + break; + case 'metricValue': + // Handle null values - sort them to the end + aValue = a.metricValue ?? -Infinity; + bValue = b.metricValue ?? -Infinity; + break; + default: + // Default to timestamp if invalid sortBy + aValue = new Date(a.timestamp).getTime(); + bValue = new Date(b.timestamp).getTime(); + } + + // Compare values + if (aValue < bValue) { + return options.sortOrder === 'asc' ? -1 : 1; + } + if (aValue > bValue) { + return options.sortOrder === 'asc' ? 1 : -1; + } + return 0; + }); + } else { + // Default: sort by timestamp DESC + filteredEntities.sort((a, b) => { + const aTime = new Date(a.timestamp).getTime(); + const bTime = new Date(b.timestamp).getTime(); + return bTime - aTime; // DESC + }); + } + + // Paginate at application level if we filtered, otherwise use DB results + let finalEntities: EntityMetricDetail[]; + let finalTotal: number; + + if (needsAppFiltering) { + // Paginate the filtered results + const startIndex = (options.page - 1) * options.limit; + finalEntities = filteredEntities.slice( + startIndex, + startIndex + options.limit, + ); + finalTotal = filteredEntities.length; + } else { + // Use database-paginated results as-is + finalEntities = filteredEntities; + finalTotal = dbTotal; + } + + // Format and return response + return { + metricId: metric.id, + metricMetadata: { + title: metric.title, + description: metric.description, + type: metric.type, + }, + entities: finalEntities, + pagination: { + page: options.page, + pageSize: options.limit, + total: finalTotal, + totalPages: Math.ceil(finalTotal / options.limit), + }, + }; + } } diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index 1d588921e6..5704186ecd 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -192,5 +192,90 @@ export async function createRouter({ ); }); + router.get( + '/metrics/:metricId/catalog/aggregations/entities', + async (req, res) => { + const { metricId } = req.params; + + const page = Number(req.query.page) || 1; + const pageSize = Math.min(Number(req.query.pageSize) || 5, 100); + const status = req.query.status as + | 'success' + | 'warning' + | 'error' + | undefined; + const ownedByMe = req.query.ownedByMe === 'true'; + const owner = req.query.owner as string | undefined; + const kind = req.query.kind as string | undefined; + const entityName = req.query.entityName as string | undefined; + const sortBy = req.query.sortBy as + | 'entityName' + | 'owner' + | 'entityKind' + | 'timestamp' + | 'metricValue' + | undefined; + const sortOrder = (req.query.sortOrder as 'asc' | 'desc') || 'desc'; + + const { conditions } = await authorizeConditional( + req, + scorecardMetricReadPermission, + ); + + const metric = metricProvidersRegistry.getMetric(metricId); + const authorizedMetrics = filterAuthorizedMetrics([metric], conditions); + + if (authorizedMetrics.length === 0) { + throw new NotAllowedError( + `To view the scorecard metrics, your administrator must grant you the required permission.`, + ); + } + + const credentials = await httpAuth.credentials(req, { allow: ['user'] }); + const userEntityRef = credentials?.principal?.userEntityRef; + + if (!userEntityRef) { + throw new AuthenticationError('User entity reference not found'); + } + + // Determine entity scope based on filters + let entityRefsToQuery: string[]; + + if (ownedByMe) { + // Use getEntitiesOwnedByUser scoping + entityRefsToQuery = await getEntitiesOwnedByUser(userEntityRef, { + catalog, + credentials, + }); + + // Check entity access for owned entities + for (const entityRef of entityRefsToQuery) { + await checkEntityAccess(entityRef, req, permissions, httpAuth); + } + } else { + // Default: Get ALL entity refs from metric_values table for this metric + // (Service layer will fetch from database without entity scoping) + entityRefsToQuery = []; // Empty array signals "fetch all" + } + + const entityMetrics = await catalogMetricService.getEntityMetricDetails( + entityRefsToQuery, // Empty = all, populated = scoped + metricId, + { + status, + owner, + kind, + entityName, + sortBy, + sortOrder, + page, + limit: pageSize, + }, + ); + + res.json(entityMetrics); + }, + ); + return router; } diff --git a/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts b/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts index 84001a5e9e..58e042d037 100644 --- a/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts +++ b/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts @@ -92,3 +92,38 @@ export type AggregatedMetricResult = { }; result: AggregatedMetric; }; + +/** + * Individual entity metric detail for drill-down + * @public + */ +export type EntityMetricDetail = { + entityRef: string; + entityName: string; + entityKind: string; + owner: string; + metricValue: number | boolean | null; + timestamp: string; + status: 'success' | 'warning' | 'error'; + score?: string; +}; + +/** + * Paginated response for entity metrics drill-down + * @public + */ +export type EntityMetricDetailResponse = { + metricId: string; + metricMetadata: { + title: string; + description: string; + type: MetricType; + }; + entities: EntityMetricDetail[]; + pagination: { + page: number; + pageSize: number; + total: number; + totalPages: number; + }; +}; From 7a7ae749450f942536d0d1bb8e62c5a5bbf55533 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 10:02:21 -0500 Subject: [PATCH 02/17] feat(scorecard): add tests for drill down functionality Signed-off-by: Patrick Knight --- .../__fixtures__/mockDatabaseMetricValues.ts | 8 + .../mockMetricProvidersRegistry.ts | 6 + .../src/database/DatabaseMetricValues.test.ts | 603 ++++++++++++++++++ .../tasks/PullMetricsByProviderTask.test.ts | 4 + .../src/service/CatalogMetricService.test.ts | 452 ++++++++++++- .../src/service/router.test.ts | 358 +++++++++++ 6 files changed, 1430 insertions(+), 1 deletion(-) diff --git a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts index 5e9493bc50..7e1e8424ff 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockDatabaseMetricValues.ts @@ -22,6 +22,7 @@ type BuildMockDatabaseMetricValuesParams = { latestEntityMetric?: DbMetricValue[]; countOfExpiredMetrics?: number; aggregatedMetric?: DbAggregatedMetric; + entityMetricsByStatus?: { rows: DbMetricValue[]; total: number }; }; export const mockDatabaseMetricValues = { @@ -29,6 +30,7 @@ export const mockDatabaseMetricValues = { readLatestEntityMetricValues: jest.fn(), cleanupExpiredMetrics: jest.fn(), readAggregatedMetricByEntityRefs: jest.fn(), + readEntityMetricsByStatus: jest.fn(), } as unknown as jest.Mocked; export const buildMockDatabaseMetricValues = ({ @@ -36,6 +38,7 @@ export const buildMockDatabaseMetricValues = ({ latestEntityMetric, countOfExpiredMetrics, aggregatedMetric, + entityMetricsByStatus, }: BuildMockDatabaseMetricValuesParams) => { const createMetricValues = metricValues ? jest.fn().mockResolvedValue(metricValues) @@ -53,10 +56,15 @@ export const buildMockDatabaseMetricValues = ({ ? jest.fn().mockResolvedValue(aggregatedMetric) : mockDatabaseMetricValues.readAggregatedMetricByEntityRefs; + const readEntityMetricsByStatus = entityMetricsByStatus + ? jest.fn().mockResolvedValue(entityMetricsByStatus) + : mockDatabaseMetricValues.readEntityMetricsByStatus; + return { createMetricValues, readLatestEntityMetricValues, cleanupExpiredMetrics, readAggregatedMetricByEntityRefs, + readEntityMetricsByStatus, } as unknown as jest.Mocked; }; diff --git a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts index 50900883fb..84be73a096 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts @@ -49,10 +49,16 @@ export const buildMockMetricProvidersRegistry = ({ return metricsList; }) : jest.fn(); + const getMetric = provider + ? jest.fn().mockImplementation((_metricId: string) => { + return provider.getMetric(); // Returns the metric from the provider + }) + : jest.fn(); return { ...mockMetricProvidersRegistry, getProvider, listMetrics, + getMetric, } as unknown as jest.Mocked; }; 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 099d0f0334..f704055000 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts @@ -258,4 +258,607 @@ describe('DatabaseMetricValues', () => { }, ); }); + + describe('readEntityMetricsByStatus', () => { + it.each(databases.eachSupportedId())( + 'should return paginated entity metrics filtered by status - %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'); + + // Insert test data with different statuses + await client('metric_values').insert([ + // Older value for service1 - should be ignored + { + catalog_entity_ref: 'component:default/service1', + metric_id: 'github.metric1', + value: 999, + timestamp: baseTime, + status: 'success', + }, + { + catalog_entity_ref: 'component:default/service1', + metric_id: 'github.metric1', + value: 10, + timestamp: laterTime, + status: 'error', + }, + { + catalog_entity_ref: 'component:default/service2', + metric_id: 'github.metric1', + value: 5, + timestamp: laterTime, + status: 'success', + }, + { + catalog_entity_ref: 'component:default/service3', + metric_id: 'github.metric1', + value: 15, + timestamp: laterTime, + status: 'error', + }, + { + catalog_entity_ref: 'component:default/service4', + metric_id: 'github.metric1', + value: 3, + timestamp: laterTime, + status: 'warning', + }, + ]); + + const result = await db.readEntityMetricsByStatus( + [ + 'component:default/service1', + 'component:default/service2', + 'component:default/service3', + 'component:default/service4', + ], + 'github.metric1', + 'error', + undefined, + undefined, + { limit: 10, offset: 0 }, + ); + + // Should return 2 entities with error status + expect(result.rows).toHaveLength(2); + expect(result.total).toBe(2); + + // Check that both are error status + expect(result.rows[0].status).toBe('error'); + expect(result.rows[1].status).toBe('error'); + + // Verify it's the latest values (not the old one for service1) + const service1Result = result.rows.find( + r => r.catalog_entity_ref === 'component:default/service1', + ); + expect(service1Result?.value).toBe(10); // Not 999 from older entry + }, + ); + + it.each(databases.eachSupportedId())( + 'should return all statuses when no filter provided - %p', + async databaseId => { + const { client, db } = await createDatabase(databaseId); + + const timestamp = new Date('2023-01-01T00:00:00Z'); + + await client('metric_values').insert([ + { + catalog_entity_ref: 'component:default/service1', + metric_id: 'github.metric1', + value: 10, + timestamp, + status: 'error', + }, + { + catalog_entity_ref: 'component:default/service2', + metric_id: 'github.metric1', + value: 5, + timestamp, + status: 'success', + }, + { + catalog_entity_ref: 'component:default/service3', + metric_id: 'github.metric1', + value: 15, + timestamp, + status: 'warning', + }, + ]); + + const result = await db.readEntityMetricsByStatus( + [ + 'component:default/service1', + 'component:default/service2', + 'component:default/service3', + ], + 'github.metric1', + undefined, + undefined, + undefined, + { limit: 10, offset: 0 }, + ); + + expect(result.rows).toHaveLength(3); + expect(result.total).toBe(3); + }, + ); + + it.each(databases.eachSupportedId())( + 'should handle pagination correctly - %p', + async databaseId => { + const { client, db } = await createDatabase(databaseId); + + const timestamp = new Date('2023-01-01T00:00:00Z'); + + // Insert 5 entities with same status + await client('metric_values').insert([ + { + catalog_entity_ref: 'component:default/service1', + metric_id: 'github.metric1', + value: 1, + timestamp, + status: 'error', + }, + { + catalog_entity_ref: 'component:default/service2', + metric_id: 'github.metric1', + value: 2, + timestamp, + status: 'error', + }, + { + catalog_entity_ref: 'component:default/service3', + metric_id: 'github.metric1', + value: 3, + timestamp, + status: 'error', + }, + { + catalog_entity_ref: 'component:default/service4', + metric_id: 'github.metric1', + value: 4, + timestamp, + status: 'error', + }, + { + catalog_entity_ref: 'component:default/service5', + metric_id: 'github.metric1', + value: 5, + timestamp, + status: 'error', + }, + ]); + + // Page 1: limit 2 + const page1 = await db.readEntityMetricsByStatus( + [ + 'component:default/service1', + 'component:default/service2', + 'component:default/service3', + 'component:default/service4', + 'component:default/service5', + ], + 'github.metric1', + 'error', + undefined, + undefined, + { limit: 2, offset: 0 }, + ); + + expect(page1.rows).toHaveLength(2); + expect(page1.total).toBe(5); + + // Page 2: limit 2, offset 2 + const page2 = await db.readEntityMetricsByStatus( + [ + 'component:default/service1', + 'component:default/service2', + 'component:default/service3', + 'component:default/service4', + 'component:default/service5', + ], + 'github.metric1', + 'error', + undefined, + undefined, + { limit: 2, offset: 2 }, + ); + + expect(page2.rows).toHaveLength(2); + expect(page2.total).toBe(5); // Total should stay the same + + // Page 3: limit 2, offset 4 + const page3 = await db.readEntityMetricsByStatus( + [ + 'component:default/service1', + 'component:default/service2', + 'component:default/service3', + 'component:default/service4', + 'component:default/service5', + ], + 'github.metric1', + 'error', + undefined, + undefined, + { limit: 2, offset: 4 }, + ); + + expect(page3.rows).toHaveLength(1); // Only 1 left on page 3 + expect(page3.total).toBe(5); + }, + ); + + it.each(databases.eachSupportedId())( + 'should return empty result for no matching entity refs - %p', + async databaseId => { + const { db } = await createDatabase(databaseId); + + const result = await db.readEntityMetricsByStatus( + [], + 'github.metric1', + 'error', + undefined, + undefined, + { limit: 10, offset: 0 }, + ); + + expect(result.rows).toHaveLength(0); + expect(result.total).toBe(0); + }, + ); + + it.each(databases.eachSupportedId())( + 'should filter by entity kind - %p', + async databaseId => { + const { client, db } = await createDatabase(databaseId); + + const timestamp = new Date('2023-01-01T00:00:00Z'); + + // Insert entities with different kinds + await client('metric_values').insert([ + { + catalog_entity_ref: 'component:default/service1', + metric_id: 'github.metric1', + value: 10, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + { + catalog_entity_ref: 'api:default/api1', + metric_id: 'github.metric1', + value: 5, + timestamp, + status: 'error', + entity_kind: 'API', + entity_owner: 'team:default/platform', + }, + { + catalog_entity_ref: 'component:default/service2', + metric_id: 'github.metric1', + value: 15, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/backend', + }, + ]); + + const result = await db.readEntityMetricsByStatus( + [ + 'component:default/service1', + 'api:default/api1', + 'component:default/service2', + ], + 'github.metric1', + 'error', + 'Component', // Filter by kind + undefined, + { limit: 10, offset: 0 }, + ); + + // Should only return Component entities + expect(result.rows).toHaveLength(2); + expect(result.total).toBe(2); + expect(result.rows[0].entity_kind).toBe('Component'); + expect(result.rows[1].entity_kind).toBe('Component'); + }, + ); + + it.each(databases.eachSupportedId())( + 'should filter by entity owner - %p', + async databaseId => { + const { client, db } = await createDatabase(databaseId); + + const timestamp = new Date('2023-01-01T00:00:00Z'); + + // Insert entities with different owners + await client('metric_values').insert([ + { + catalog_entity_ref: 'component:default/service1', + metric_id: 'github.metric1', + value: 10, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + { + catalog_entity_ref: 'component:default/service2', + metric_id: 'github.metric1', + value: 5, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/backend', + }, + { + catalog_entity_ref: 'component:default/service3', + metric_id: 'github.metric1', + value: 15, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + ]); + + const result = await db.readEntityMetricsByStatus( + [ + 'component:default/service1', + 'component:default/service2', + 'component:default/service3', + ], + 'github.metric1', + 'error', + undefined, + 'team:default/platform', // Filter by owner + { limit: 10, offset: 0 }, + ); + + // Should only return entities owned by team:default/platform + expect(result.rows).toHaveLength(2); + expect(result.total).toBe(2); + expect(result.rows[0].entity_owner).toBe('team:default/platform'); + expect(result.rows[1].entity_owner).toBe('team:default/platform'); + }, + ); + + it.each(databases.eachSupportedId())( + 'should filter by status, kind, and owner together - %p', + async databaseId => { + const { client, db } = await createDatabase(databaseId); + + const timestamp = new Date('2023-01-01T00:00:00Z'); + + // Insert diverse test data + await client('metric_values').insert([ + { + catalog_entity_ref: 'component:default/service1', + metric_id: 'github.metric1', + value: 10, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + { + catalog_entity_ref: 'api:default/api1', + metric_id: 'github.metric1', + value: 5, + timestamp, + status: 'error', + entity_kind: 'API', + entity_owner: 'team:default/platform', + }, + { + catalog_entity_ref: 'component:default/service2', + metric_id: 'github.metric1', + value: 15, + timestamp, + status: 'warning', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + { + catalog_entity_ref: 'component:default/service3', + metric_id: 'github.metric1', + value: 20, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/backend', + }, + ]); + + const result = await db.readEntityMetricsByStatus( + [ + 'component:default/service1', + 'api:default/api1', + 'component:default/service2', + 'component:default/service3', + ], + 'github.metric1', + 'error', // Only error status + 'Component', // Only Component kind + 'team:default/platform', // Only platform team + { limit: 10, offset: 0 }, + ); + + // Should only return service1 (Component, error, platform) + expect(result.rows).toHaveLength(1); + expect(result.total).toBe(1); + expect(result.rows[0].catalog_entity_ref).toBe( + 'component:default/service1', + ); + expect(result.rows[0].status).toBe('error'); + expect(result.rows[0].entity_kind).toBe('Component'); + expect(result.rows[0].entity_owner).toBe('team:default/platform'); + }, + ); + + it.each(databases.eachSupportedId())( + 'should work without pagination (fetch all) - %p', + async databaseId => { + const { client, db } = await createDatabase(databaseId); + + const timestamp = new Date('2023-01-01T00:00:00Z'); + + await client('metric_values').insert([ + { + catalog_entity_ref: 'component:default/service1', + metric_id: 'github.metric1', + value: 10, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + { + catalog_entity_ref: 'component:default/service2', + metric_id: 'github.metric1', + value: 5, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + { + catalog_entity_ref: 'component:default/service3', + metric_id: 'github.metric1', + value: 15, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + ]); + + // No pagination parameter - should return all + const result = await db.readEntityMetricsByStatus( + [ + 'component:default/service1', + 'component:default/service2', + 'component:default/service3', + ], + 'github.metric1', + 'error', + undefined, + undefined, + undefined, // No pagination + ); + + expect(result.rows).toHaveLength(3); + expect(result.total).toBe(3); + }, + ); + + it.each(databases.eachSupportedId())( + 'should handle null entity_kind and entity_owner - %p', + async databaseId => { + const { client, db } = await createDatabase(databaseId); + + const timestamp = new Date('2023-01-01T00:00:00Z'); + + // Insert entity with null kind/owner (legacy data) + await client('metric_values').insert([ + { + catalog_entity_ref: 'component:default/service1', + metric_id: 'github.metric1', + value: 10, + timestamp, + status: 'error', + entity_kind: null, + entity_owner: null, + }, + { + catalog_entity_ref: 'component:default/service2', + metric_id: 'github.metric1', + value: 5, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + ]); + + // Should return both when no filters + const result = await db.readEntityMetricsByStatus( + ['component:default/service1', 'component:default/service2'], + 'github.metric1', + 'error', + undefined, + undefined, + { limit: 10, offset: 0 }, + ); + + expect(result.rows).toHaveLength(2); + expect(result.total).toBe(2); + + // Should only return service2 when filtering by kind + const filteredResult = await db.readEntityMetricsByStatus( + ['component:default/service1', 'component:default/service2'], + 'github.metric1', + 'error', + 'Component', + undefined, + { limit: 10, offset: 0 }, + ); + + expect(filteredResult.rows).toHaveLength(1); + expect(filteredResult.rows[0].catalog_entity_ref).toBe( + 'component:default/service2', + ); + }, + ); + + it.each(databases.eachSupportedId())( + 'should handle empty entity refs list (fetch all entities) - %p', + async databaseId => { + const { client, db } = await createDatabase(databaseId); + + const timestamp = new Date('2023-01-01T00:00:00Z'); + + // Insert entities + await client('metric_values').insert([ + { + catalog_entity_ref: 'component:default/service1', + metric_id: 'github.metric1', + value: 10, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + { + catalog_entity_ref: 'component:default/service2', + metric_id: 'github.metric1', + value: 5, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/backend', + }, + ]); + + // Empty array = fetch all entities for this metric + const result = await db.readEntityMetricsByStatus( + [], // Empty array + 'github.metric1', + 'error', + undefined, + undefined, + { limit: 10, offset: 0 }, + ); + + expect(result.rows).toHaveLength(2); + expect(result.total).toBe(2); + }, + ); + }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.test.ts index 65895c2f03..926e030732 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.test.ts @@ -267,6 +267,8 @@ describe('PullMetricsByProviderTask', () => { const metricValues = [ { catalog_entity_ref: 'component:default/test1', + entity_kind: 'Component', + entity_owner: undefined, metric_id: 'github.test_metric', timestamp: new Date('2024-01-15T12:00:00.000Z'), value: 42, @@ -275,6 +277,8 @@ describe('PullMetricsByProviderTask', () => { { catalog_entity_ref: 'component:default/test2', metric_id: 'github.test_metric', + entity_kind: 'Component', + entity_owner: undefined, status: 'success', timestamp: new Date('2024-01-15T12:00:00.000Z'), value: 42, 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 abfdae96b7..6f36077437 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts @@ -25,7 +25,7 @@ import { mockDatabaseMetricValues, } from '../../__fixtures__/mockDatabaseMetricValues'; import { buildMockMetricProvidersRegistry } from '../../__fixtures__/mockMetricProvidersRegistry'; -import { AuthService } from '@backstage/backend-plugin-api'; +import { AuthService, LoggerService } from '@backstage/backend-plugin-api'; import * as permissionUtils from '../permissions/permissionUtils'; import { AggregatedMetric, @@ -88,6 +88,7 @@ describe('CatalogMetricService', () => { let mockedAuth: jest.Mocked; let mockedRegistry: jest.Mocked; let mockedDatabase: jest.Mocked; + let mockedLogger: jest.Mocked; let service: CatalogMetricService; let toAggregatedMetricSpy: jest.SpyInstance; @@ -97,6 +98,10 @@ describe('CatalogMetricService', () => { mockedCatalog = catalogServiceMock.mock(); mockedCatalog.getEntityByRef.mockResolvedValue(mockEntity); + mockedCatalog.getEntitiesByRefs = jest + .fn() + .mockResolvedValue({ items: [] }); + mockedAuth = mockServices.auth.mock({ getOwnServiceCredentials: jest.fn().mockResolvedValue({ token: 'test-token', @@ -113,6 +118,8 @@ describe('CatalogMetricService', () => { aggregatedMetric, }); + mockedLogger = mockServices.logger.mock(); + (permissionUtils.filterAuthorizedMetrics as jest.Mock).mockReturnValue([ { id: 'github.important_metric' }, ]); @@ -133,6 +140,7 @@ describe('CatalogMetricService', () => { auth: mockedAuth, registry: mockedRegistry, database: mockedDatabase, + logger: mockedLogger, }); jest.useFakeTimers(); @@ -506,4 +514,446 @@ describe('CatalogMetricService', () => { }); }); }); + + describe('getEntityMetricDetails', () => { + const mockMetricRows: DbMetricValue[] = [ + { + id: 1, + catalog_entity_ref: 'component:default/service-a', + metric_id: 'github.important_metric', + value: 15, + timestamp: new Date('2024-01-15T12:00:00.000Z'), + error_message: null, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + { + id: 2, + catalog_entity_ref: 'component:default/service-b', + metric_id: 'github.important_metric', + value: 8, + timestamp: new Date('2024-01-15T11:00:00.000Z'), + error_message: null, + status: 'warning', + entity_kind: 'Component', + entity_owner: 'team:default/backend', + }, + { + id: 3, + catalog_entity_ref: 'component:default/service-c', + metric_id: 'github.important_metric', + value: 3, + timestamp: new Date('2024-01-15T10:00:00.000Z'), + error_message: null, + status: 'success', + entity_kind: 'API', + entity_owner: 'team:default/platform', + }, + ]; + + const mockEntities = { + items: [ + new MockEntityBuilder() + .withKind('Component') + .withMetadata({ name: 'service-a', namespace: 'default' }) + .withSpec({ owner: 'team:default/platform' }) + .build(), + new MockEntityBuilder() + .withKind('Component') + .withMetadata({ name: 'service-b', namespace: 'default' }) + .withSpec({ owner: 'team:default/backend' }) + .build(), + new MockEntityBuilder() + .withKind('API') + .withMetadata({ name: 'service-c', namespace: 'default' }) + .withSpec({ owner: 'team:default/platform' }) + .build(), + ], + }; + + beforeEach(() => { + mockedDatabase.readEntityMetricsByStatus.mockResolvedValue({ + rows: mockMetricRows, + total: 3, + }); + + mockedCatalog.getEntitiesByRefs.mockReset(); + mockedCatalog.getEntitiesByRefs.mockResolvedValue(mockEntities); + }); + + it('should fetch entity metrics with default options', async () => { + const result = await service.getEntityMetricDetails( + [], + 'github.important_metric', + { + page: 1, + limit: 10, + }, + ); + + expect(result.metricId).toBe('github.important_metric'); + expect(result.entities).toHaveLength(3); + expect(result.pagination).toEqual({ + page: 1, + pageSize: 10, + total: 3, + totalPages: 1, + }); + }); + + it('should enrich entities with catalog metadata', async () => { + const result = await service.getEntityMetricDetails( + [], + 'github.important_metric', + { + page: 1, + limit: 10, + }, + ); + + expect(result.entities[0]).toEqual({ + entityRef: 'component:default/service-a', + entityName: 'service-a', + entityKind: 'Component', + owner: 'team:default/platform', + metricValue: 15, + timestamp: '2024-01-15T12:00:00.000Z', + status: 'error', + }); + }); + + it('should call database with correct pagination parameters', async () => { + await service.getEntityMetricDetails([], 'github.important_metric', { + page: 2, + limit: 5, + }); + + expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( + [], + 'github.important_metric', + undefined, + undefined, + undefined, + { limit: 5, offset: 5 }, + ); + }); + + it('should filter by status at database level', async () => { + await service.getEntityMetricDetails([], 'github.important_metric', { + status: 'error', + page: 1, + limit: 10, + }); + + expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( + [], + 'github.important_metric', + 'error', + undefined, + undefined, + { limit: 10, offset: 0 }, + ); + }); + + it('should filter by kind at database level', async () => { + await service.getEntityMetricDetails([], 'github.important_metric', { + kind: 'Component', + page: 1, + limit: 10, + }); + + expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( + [], + 'github.important_metric', + undefined, + 'Component', + undefined, + { limit: 10, offset: 0 }, + ); + }); + + it('should filter by owner at database level', async () => { + await service.getEntityMetricDetails([], 'github.important_metric', { + owner: 'team:default/platform', + page: 1, + limit: 10, + }); + + expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( + [], + 'github.important_metric', + undefined, + undefined, + 'team:default/platform', + { limit: 10, offset: 0 }, + ); + }); + + it('should filter by entityName at application level', async () => { + const result = await service.getEntityMetricDetails( + [], + 'github.important_metric', + { + entityName: 'service-a', + page: 1, + limit: 10, + }, + ); + + // Should fetch all from DB (no pagination) + expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( + [], + 'github.important_metric', + undefined, + undefined, + undefined, + undefined, + ); + + // Should filter in application + expect(result.entities).toHaveLength(1); + expect(result.entities[0].entityName).toBe('service-a'); + expect(result.pagination.total).toBe(1); + }); + + it('should perform case-insensitive entityName search', async () => { + const result = await service.getEntityMetricDetails( + [], + 'github.important_metric', + { + entityName: 'SERVICE', + page: 1, + limit: 10, + }, + ); + + expect(result.entities).toHaveLength(3); + }); + + it('should sort by entityName ascending', async () => { + const result = await service.getEntityMetricDetails( + [], + 'github.important_metric', + { + sortBy: 'entityName', + sortOrder: 'asc', + page: 1, + limit: 10, + }, + ); + + expect(result.entities[0].entityName).toBe('service-a'); + expect(result.entities[1].entityName).toBe('service-b'); + expect(result.entities[2].entityName).toBe('service-c'); + }); + + it('should sort by metricValue descending', async () => { + const result = await service.getEntityMetricDetails( + [], + 'github.important_metric', + { + sortBy: 'metricValue', + sortOrder: 'desc', + page: 1, + limit: 10, + }, + ); + + expect(result.entities[0].metricValue).toBe(15); + expect(result.entities[1].metricValue).toBe(8); + expect(result.entities[2].metricValue).toBe(3); + }); + + it('should sort by timestamp descending by default', async () => { + const result = await service.getEntityMetricDetails( + [], + 'github.important_metric', + { + page: 1, + limit: 10, + }, + ); + + // Most recent first + expect(result.entities[0].timestamp).toBe('2024-01-15T12:00:00.000Z'); + expect(result.entities[1].timestamp).toBe('2024-01-15T11:00:00.000Z'); + expect(result.entities[2].timestamp).toBe('2024-01-15T10:00:00.000Z'); + }); + + it('should handle null metric values in sorting', async () => { + mockedDatabase.readEntityMetricsByStatus.mockResolvedValue({ + rows: [{ ...mockMetricRows[0], value: null }, mockMetricRows[1]], + total: 2, + }); + + const result = await service.getEntityMetricDetails( + [], + 'github.important_metric', + { + sortBy: 'metricValue', + sortOrder: 'desc', + page: 1, + limit: 10, + }, + ); + + // Null should be sorted to the end + expect(result.entities[0].metricValue).toBe(8); + expect(result.entities[1].metricValue).toBe(null); + }); + + it('should batch-fetch entities using getEntitiesByRefs', async () => { + await service.getEntityMetricDetails([], 'github.important_metric', { + page: 1, + limit: 10, + }); + + expect(mockedCatalog.getEntitiesByRefs).toHaveBeenCalledWith( + { + entityRefs: [ + 'component:default/service-a', + 'component:default/service-b', + 'component:default/service-c', + ], + fields: ['kind', 'metadata', 'spec'], + }, + { credentials: expect.any(Object) }, + ); + }); + + it('should handle missing catalog entities gracefully', async () => { + mockedCatalog.getEntitiesByRefs.mockResolvedValue({ + items: [mockEntities.items[0], undefined, mockEntities.items[2]], + }); + + const result = await service.getEntityMetricDetails( + [], + 'github.important_metric', + { + page: 1, + limit: 10, + }, + ); + + expect(result.entities[1].entityName).toBe('Unknown'); + expect(result.entities[1].entityKind).toBe('Component'); + expect(result.entities[1].owner).toBe('team:default/backend'); + }); + + it('should handle catalog API failures gracefully', async () => { + mockedCatalog.getEntitiesByRefs.mockRejectedValue( + new Error('Catalog API error'), + ); + + const result = await service.getEntityMetricDetails( + [], + 'github.important_metric', + { + page: 1, + limit: 10, + }, + ); + + expect(mockedLogger.warn).toHaveBeenCalledWith( + 'Failed to fetch entities from catalog', + expect.objectContaining({ error: expect.any(Error) }), + ); + + // Should still return results with fallback data + expect(result.entities).toHaveLength(3); + expect(result.entities[0].entityName).toBe('Unknown'); + }); + + it('should pass entity refs to database query', async () => { + await service.getEntityMetricDetails( + ['component:default/service-a', 'component:default/service-b'], + 'github.important_metric', + { page: 1, limit: 10 }, + ); + + expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( + ['component:default/service-a', 'component:default/service-b'], + 'github.important_metric', + undefined, + undefined, + undefined, + { limit: 10, offset: 0 }, + ); + }); + + it('should combine filters, sorting, and pagination', async () => { + mockedDatabase.readEntityMetricsByStatus.mockResolvedValue({ + rows: [mockMetricRows[0]], + total: 1, + }); + + const result = await service.getEntityMetricDetails( + [], + 'github.important_metric', + { + status: 'error', + kind: 'Component', + owner: 'team:default/platform', + sortBy: 'metricValue', + sortOrder: 'desc', + page: 1, + limit: 5, + }, + ); + + expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( + [], + 'github.important_metric', + 'error', + 'Component', + 'team:default/platform', + { limit: 5, offset: 0 }, + ); + + expect(result.entities).toHaveLength(1); + expect(result.pagination.total).toBe(1); + }); + + it('should return empty results when no entities match', async () => { + mockedDatabase.readEntityMetricsByStatus.mockResolvedValue({ + rows: [], + total: 0, + }); + + const result = await service.getEntityMetricDetails( + [], + 'github.important_metric', + { + page: 1, + limit: 10, + }, + ); + + expect(result.entities).toEqual([]); + expect(result.pagination).toEqual({ + page: 1, + pageSize: 10, + total: 0, + totalPages: 0, + }); + }); + + it('should include metric metadata in response', async () => { + const result = await service.getEntityMetricDetails( + [], + 'github.important_metric', + { + page: 1, + limit: 10, + }, + ); + + expect(result.metricMetadata).toEqual({ + title: provider.getMetric().title, + description: provider.getMetric().description, + type: provider.getMetric().type, + }); + }); + }); }); 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 e46e561048..2b98937df5 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -81,6 +81,7 @@ describe('createRouter', () => { let app: express.Express; let metricProvidersRegistry: MetricProvidersRegistry; let catalogMetricService: CatalogMetricService; + let mockLogger: ReturnType; let httpAuthMock: ServiceMock< import('@backstage/backend-plugin-api').HttpAuthService >; @@ -93,11 +94,13 @@ describe('createRouter', () => { beforeEach(async () => { metricProvidersRegistry = new MetricProvidersRegistry(); const catalog = catalogServiceMock.mock(); + mockLogger = mockServices.logger.mock(); catalogMetricService = new CatalogMetricService({ catalog, registry: metricProvidersRegistry, auth: mockServices.auth(), database: mockDatabaseMetricValues, + logger: mockLogger, }); permissionsMock.authorizeConditional.mockResolvedValue([ @@ -725,4 +728,359 @@ describe('createRouter', () => { ); }); }); + + describe('GET /metrics/:metricId/catalog/aggregations/entities', () => { + const mockEntityMetricDetailResponse = { + metricId: 'github.open_prs', + metricMetadata: { + title: 'GitHub Open PRs', + description: 'Mock number description.', + type: 'number', + }, + entities: [ + { + entityRef: 'component:default/my-service', + entityName: 'my-service', + entityKind: 'Component', + owner: 'team:default/platform', + metricValue: 15, + timestamp: '2025-01-01T10:30:00.000Z', + status: 'error', + }, + { + entityRef: 'component:default/another-service', + entityName: 'another-service', + entityKind: 'Component', + owner: 'team:default/backend', + metricValue: 8, + timestamp: '2025-01-01T10:25:00.000Z', + status: 'warning', + }, + ], + pagination: { + page: 1, + pageSize: 10, + total: 2, + totalPages: 1, + }, + }; + + let drillDownApp: express.Express; + let getEntityMetricDetailsSpy: jest.SpyInstance; + let getEntitiesOwnedByUserSpy: jest.SpyInstance; + let checkEntityAccessSpy: jest.SpyInstance; + + beforeEach(async () => { + const githubProvider = new MockNumberProvider( + 'github.open_prs', + 'github', + 'GitHub Open PRs', + ); + metricProvidersRegistry.register(githubProvider); + + const jiraProvider = new MockNumberProvider( + 'jira.open_issues', + 'jira', + 'Jira Open Issues', + ); + metricProvidersRegistry.register(jiraProvider); + + getEntityMetricDetailsSpy = jest + .spyOn(catalogMetricService, 'getEntityMetricDetails') + .mockResolvedValue(mockEntityMetricDetailResponse as any); + + getEntitiesOwnedByUserSpy = jest + .spyOn(getEntitiesOwnedByUserModule, 'getEntitiesOwnedByUser') + .mockResolvedValue([ + 'component:default/my-service', + 'component:default/another-service', + ]); + + checkEntityAccessSpy = jest.spyOn( + permissionUtilsModule, + 'checkEntityAccess', + ); + + const mockCatalog = catalogServiceMock.mock(); + const router = await createRouter({ + metricProvidersRegistry, + catalogMetricService, + catalog: mockCatalog, + httpAuth: httpAuthMock, + permissions: permissionsMock, + }); + + drillDownApp = express(); + drillDownApp.use(router); + drillDownApp.use(mockErrorHandler()); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should return entity metric details with default pagination', async () => { + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities', + ); + + expect(response.status).toBe(200); + expect(response.body).toEqual(mockEntityMetricDetailResponse); + expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( + [], + 'github.open_prs', + { + status: undefined, + owner: undefined, + kind: undefined, + entityName: undefined, + sortBy: undefined, + sortOrder: 'desc', + page: 1, + limit: 5, + }, + ); + }); + + it('should handle custom page and pageSize', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?page=2&pageSize=20', + ); + + expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( + [], + 'github.open_prs', + expect.objectContaining({ + page: 2, + limit: 20, + }), + ); + }); + + it('should enforce max pageSize of 100', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?pageSize=200', + ); + + expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( + [], + 'github.open_prs', + expect.objectContaining({ + limit: 100, + }), + ); + }); + + it('should filter by status', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?status=error', + ); + + expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( + [], + 'github.open_prs', + expect.objectContaining({ + status: 'error', + }), + ); + }); + + it('should filter by owner', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?owner=team:default/platform', + ); + + expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( + [], + 'github.open_prs', + expect.objectContaining({ + owner: 'team:default/platform', + }), + ); + }); + + it('should filter by kind', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?kind=Component', + ); + + expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( + [], + 'github.open_prs', + expect.objectContaining({ + kind: 'Component', + }), + ); + }); + + it('should filter by entityName', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?entityName=service', + ); + + expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( + [], + 'github.open_prs', + expect.objectContaining({ + entityName: 'service', + }), + ); + }); + + it('should handle ownedByMe=true and call getEntitiesOwnedByUser', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=true', + ); + + expect(getEntitiesOwnedByUserSpy).toHaveBeenCalledWith( + 'user:default/test-user', + expect.objectContaining({ + catalog: expect.any(Object), + credentials: expect.any(Object), + }), + ); + + expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( + ['component:default/my-service', 'component:default/another-service'], + 'github.open_prs', + expect.any(Object), + ); + }); + + it('should check entity access when ownedByMe=true', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=true', + ); + + expect(checkEntityAccessSpy).toHaveBeenCalledTimes(2); + expect(checkEntityAccessSpy).toHaveBeenCalledWith( + 'component:default/my-service', + expect.any(Object), + permissionsMock, + httpAuthMock, + ); + expect(checkEntityAccessSpy).toHaveBeenCalledWith( + 'component:default/another-service', + expect.any(Object), + permissionsMock, + httpAuthMock, + ); + }); + + it('should sort by entityName ascending', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?sortBy=entityName&sortOrder=asc', + ); + + expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( + [], + 'github.open_prs', + expect.objectContaining({ + sortBy: 'entityName', + sortOrder: 'asc', + }), + ); + }); + + it('should sort by metricValue descending', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?sortBy=metricValue&sortOrder=desc', + ); + + expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( + [], + 'github.open_prs', + expect.objectContaining({ + sortBy: 'metricValue', + sortOrder: 'desc', + }), + ); + }); + + it('should combine multiple filters', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?status=error&kind=Component&owner=team:default/platform&sortBy=metricValue&sortOrder=desc', + ); + + expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( + [], + 'github.open_prs', + expect.objectContaining({ + status: 'error', + kind: 'Component', + owner: 'team:default/platform', + sortBy: 'metricValue', + sortOrder: 'desc', + }), + ); + }); + + it('should return 403 when user does not have permission', async () => { + permissionsMock.authorizeConditional.mockResolvedValue([ + { result: AuthorizeResult.DENY }, + ]); + + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities', + ); + + expect(response.status).toBe(403); + expect(response.body.error.name).toEqual('NotAllowedError'); + }); + + it('should return 401 when user entity reference is not found', async () => { + httpAuthMock.credentials.mockResolvedValue({ + principal: {}, + } as any); + + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities', + ); + + expect(response.status).toBe(401); + expect(response.body.error.name).toEqual('AuthenticationError'); + expect(response.body.error.message).toContain( + 'User entity reference not found', + ); + }); + + it('should return 403 when user does not have access to metric (conditional)', async () => { + permissionsMock.authorizeConditional.mockResolvedValue([ + CONDITIONAL_POLICY_DECISION, + ]); + + const response = await request(drillDownApp).get( + '/metrics/jira.open_issues/catalog/aggregations/entities', + ); + + expect(response.status).toBe(403); + expect(response.body.error.name).toEqual('NotAllowedError'); + }); + + it('should return 404 when metric does not exist', async () => { + const response = await request(drillDownApp).get( + '/metrics/non.existent.metric/catalog/aggregations/entities', + ); + + expect(response.status).toBe(404); + expect(response.body.error.name).toBe('NotFoundError'); + }); + + it('should return empty entities array when no results', async () => { + getEntityMetricDetailsSpy.mockResolvedValue({ + metricId: 'github.open_prs', + metricMetadata: mockEntityMetricDetailResponse.metricMetadata, + entities: [], + pagination: { page: 1, pageSize: 10, total: 0, totalPages: 0 }, + }); + + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities', + ); + + expect(response.status).toBe(200); + expect(response.body.entities).toEqual([]); + expect(response.body.pagination.total).toBe(0); + }); + }); }); From 7901fae80c2e9f3d867b738e08eeeed995f133a4 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 10:02:50 -0500 Subject: [PATCH 03/17] feat(scorecard): add docs for drill down functionality Signed-off-by: Patrick Knight --- .../scorecard-backend/docs/drill-down.md | 483 ++++++++++++++++++ 1 file changed, 483 insertions(+) create mode 100644 workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md diff --git a/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md b/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md new file mode 100644 index 0000000000..1e7061fcc3 --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md @@ -0,0 +1,483 @@ +# Entity Drill-Down + +The Scorecard plugin provides a drill-down endpoint that returns detailed entity-level metrics with filtering, sorting, and pagination capabilities. This feature allows users to investigate the individual entities that contribute to aggregated scorecard metrics, enabling detailed analysis and troubleshooting. + +## Overview + +The drill-down endpoint (`/metrics/:metricId/catalog/aggregations/entities`) provides a detailed view of entities and their metric values. It allows managers and platform engineers to: + +- See individual entities contributing to aggregated scores +- Filter entities by status (success/warning/error), owner, kind, or name +- Sort by any column (entity name, owner, kind, timestamp, metric value) +- Paginate through large result sets +- Understand data freshness through per-entity timestamps + +This endpoint transforms the scorecard from a passive reporting tool into an actionable diagnostic interface. + +## API Endpoint + +### `GET /metrics/:metricId/catalog/aggregations/entities` + +Returns a paginated list of entities with their metric values, enriched with catalog metadata. + +#### Path Parameters + +| Parameter | Type | Required | Description | +| ---------- | ------ | -------- | ---------------------------------------------- | +| `metricId` | string | Yes | The ID of the metric (e.g., `github.open_prs`) | + +#### Query Parameters + +| Parameter | Type | Required | Default | Description | +| ------------ | ------- | -------- | ----------- | -------------------------------------------------------------------------------------------- | +| `status` | string | No | - | Filter by threshold status: `success`, `warning`, or `error` | +| `ownedByMe` | boolean | No | `false` | If `true`, only show entities owned by the authenticated user and their direct parent groups | +| `owner` | string | No | - | Filter by specific owner entity ref (e.g., `team:default/platform`) | +| `kind` | string | No | - | Filter by entity kind (e.g., `Component`, `API`, `System`) | +| `entityName` | string | No | - | Search for entities with names containing this string (case-insensitive) | +| `sortBy` | string | No | `timestamp` | Sort by: `entityName`, `owner`, `entityKind`, `timestamp`, or `metricValue` | +| `sortOrder` | string | No | `desc` | Sort direction: `asc` or `desc` | +| `page` | number | No | `1` | Page number (1-indexed) | +| `pageSize` | number | No | `5` | Number of entities per page (max: 100) | + +#### Authentication + +Requires user authentication. The endpoint uses the authenticated user's entity reference when `ownedByMe=true` is specified. + +#### 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 results + +#### Response Schema + +```typescript +{ + metricId: string; + metricMetadata: { + title: string; + description: string; + type: 'number' | 'boolean'; + }; + entities: EntityMetricDetail[]; + pagination: { + page: number; + pageSize: number; + total: number; + totalPages: number; + }; +} + +type EntityMetricDetail = { + entityRef: string; // Full entity reference (e.g., "component:default/my-service") + entityName: string; // Entity name from catalog + entityKind: string; // Entity kind (e.g., "Component", "API") + owner: string; // Owner entity reference or name + metricValue: number | boolean | null; // The actual metric value + timestamp: string; // ISO 8601 timestamp of when metric was synced + status: 'success' | 'warning' | 'error'; // Threshold evaluation status +}; +``` + +## Usage Examples + +### Basic Drill-Down + +Get the first page of entities for a metric: + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?page=1&pageSize=10" \ + -H "Authorization: Bearer " +``` + +### Filter by Status + +Get only entities in error state: + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?status=error&page=1&pageSize=10" \ + -H "Authorization: Bearer " +``` + +### Filter by Ownership + +Get only entities owned by the authenticated user: + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=true&status=error" \ + -H "Authorization: Bearer " +``` + +Get entities owned by a specific team: + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?owner=team:default/platform&page=1&pageSize=10" \ + -H "Authorization: Bearer " +``` + +### Filter by Entity Kind + +Get only Component entities: + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?kind=Component&page=1&pageSize=10" \ + -H "Authorization: Bearer " +``` + +### Search by Entity Name + +Search for entities with "service" in their name: + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?entityName=service&page=1&pageSize=10" \ + -H "Authorization: Bearer " +``` + +### Sorting + +Sort by metric value (highest first): + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?sortBy=metricValue&sortOrder=desc&page=1&pageSize=10" \ + -H "Authorization: Bearer " +``` + +Sort by entity name alphabetically: + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?sortBy=entityName&sortOrder=asc&page=1&pageSize=10" \ + -H "Authorization: Bearer " +``` + +### Combining Filters + +Get my Component entities with errors, sorted by metric value: + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=true&status=error&kind=Component&sortBy=metricValue&sortOrder=desc&page=1&pageSize=10" \ + -H "Authorization: Bearer " +``` + +## Response Example + +```json +{ + "metricId": "github.open_prs", + "metricMetadata": { + "title": "Open Pull Requests", + "description": "Number of open pull requests in GitHub", + "type": "number" + }, + "entities": [ + { + "entityRef": "component:default/my-service", + "entityName": "my-service", + "entityKind": "Component", + "owner": "team:default/platform", + "metricValue": 15, + "timestamp": "2026-02-17T10:30:00Z", + "status": "error" + }, + { + "entityRef": "component:default/another-service", + "entityName": "another-service", + "entityKind": "Component", + "owner": "team:default/backend", + "metricValue": 8, + "timestamp": "2026-02-17T10:25:00Z", + "status": "warning" + } + ], + "pagination": { + "page": 1, + "pageSize": 10, + "total": 42, + "totalPages": 5 + } +} +``` + +## Filtering Behavior + +### Entity Ownership Scoping + +The `ownedByMe` parameter controls which entities are included in the results: + +**Default (`ownedByMe` not specified or `false`)**: Returns all entities in the system that have metric values for the specified metric, subject to the user's catalog read permissions. + +**`ownedByMe=true`**: Returns only entities owned by the authenticated user and their direct parent groups. This uses the same ownership logic as the aggregation endpoint (see [Entity Aggregation](./aggregation.md) for details on direct parent groups vs transitive ownership). + +### Status Filtering + +When `status` is specified, only entities with that threshold evaluation are returned: + +- `status=success`: Entities meeting success thresholds +- `status=warning`: Entities meeting warning thresholds +- `status=error`: Entities failing thresholds or in error state + +Status filtering is performed at the database level for optimal performance. + +### Owner Filtering + +The `owner` parameter filters entities by their catalog owner (`spec.owner`): + +```bash +# Get entities owned by a specific team +?owner=team:default/platform + +# Get entities owned by a specific user +?owner=user:default/alice +``` + +This filter is applied at the database level and works independently of `ownedByMe`. + +### Kind Filtering + +Filter by entity kind to narrow results to specific entity types: + +```bash +# Only Components +?kind=Component + +# Only APIs +?kind=API + +# Only Systems +?kind=System +``` + +Kind filtering is performed at the database level for optimal performance. + +### Entity Name Search + +The `entityName` parameter performs a case-insensitive substring search on entity names: + +```bash +# Find entities with "auth" in the name +?entityName=auth + +# Find entities with "service" in the name +?entityName=service +``` + +Entity name filtering requires catalog metadata and is performed at the application level after enrichment. + +## Sorting + +Results can be sorted by any column in ascending or descending order: + +### Sort Options + +| Sort By | Description | Example Values | +| ------------- | ----------------------------------------------- | ------------------------ | +| `entityName` | Entity name alphabetically | "api-service", "web-app" | +| `owner` | Owner entity reference alphabetically | "team:default/platform" | +| `entityKind` | Entity kind alphabetically | "API", "Component" | +| `timestamp` | Metric sync timestamp (most/least recent) | ISO 8601 timestamps | +| `metricValue` | Metric value numerically (highest/lowest first) | 5, 15, 25, 100 | + +### Default Sorting + +If no `sortBy` is specified, results are sorted by `timestamp` in descending order (most recent first). + +### Null Value Handling + +When sorting by `metricValue`, entities with `null` values are sorted to the end regardless of sort order. + +## Pagination + +The endpoint uses offset-based pagination: + +- **Default page size**: 5 entities +- **Maximum page size**: 100 entities +- **Page numbering**: 1-indexed (first page is `page=1`) + +The response includes pagination metadata: + +```json +{ + "pagination": { + "page": 1, // Current page + "pageSize": 10, // Entities per page + "total": 42, // Total matching entities across all pages + "totalPages": 5 // Total number of pages + } +} +``` + +### Pagination Performance + +- **Database-level pagination**: Used when only `status`, `owner`, or `kind` filters are applied (optimal performance) +- **Application-level pagination**: Used when `entityName` search is applied (fetches all results, then filters and paginates in memory) + +For best performance with large datasets, use database-level filters when possible. + +## Error Handling + +### Invalid Metric ID + +If the specified metric does not exist: + +- **Status Code**: `404 Not Found` +- **Error**: `NotFoundError: Metric not found` + +### Missing User Entity Reference + +If the authenticated user doesn't have an entity reference: + +- **Status Code**: `401 Unauthorized` +- **Error**: `AuthenticationError: User entity reference not found` + +### Permission Denied + +If the user doesn't have access to the metric: + +- **Status Code**: `403 Forbidden` +- **Error**: `NotAllowedError: To view the scorecard metrics, your administrator must grant you the required permission.` + +### Invalid Query Parameters + +If query parameters are invalid (e.g., `pageSize > 100`): + +- **Status Code**: `400 Bad Request` +- **Error**: Description of the validation error + +### Empty Results + +When no entities match the filters: + +- **Status Code**: `200 OK` +- **Response**: Empty entities array with `total: 0` + +```json +{ + "metricId": "github.open_prs", + "metricMetadata": { ... }, + "entities": [], + "pagination": { + "page": 1, + "pageSize": 10, + "total": 0, + "totalPages": 0 + } +} +``` + +## Data Freshness + +Each entity includes a `timestamp` field indicating when the metric value was last synced. This helps users understand data recency and identify stale metrics. + +The timestamp represents when the metric provider last successfully fetched and evaluated the metric for that specific entity. Timestamps may vary across entities depending on: + +- When entities were added to the catalog +- Individual metric sync schedules +- Failures or errors in previous sync attempts + +## Use Cases + +### Use Case 1: Identify Services in Error State + +A manager sees an aggregated scorecard showing 50 entities with errors. They drill down to see which specific services need attention: + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?status=error&sortBy=metricValue&sortOrder=desc&page=1&pageSize=20" \ + -H "Authorization: Bearer " +``` + +This returns the 20 entities with the most severe issues (highest metric values in error state). + +### Use Case 2: Review Team-Specific Metrics + +A team lead wants to see only their team's entities: + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/jira.open_issues/catalog/aggregations/entities?owner=team:default/backend&sortBy=timestamp&sortOrder=asc" \ + -H "Authorization: Bearer " +``` + +This shows the team's entities sorted by staleness (oldest data first), helping identify entities that may need attention. + +### Use Case 3: Audit Specific Entity Type + +An architect wants to review all API entities: + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/openssf.score/catalog/aggregations/entities?kind=API&status=warning&page=1&pageSize=25" \ + -H "Authorization: Bearer " +``` + +This returns API entities with security warnings, helping prioritize security improvements. + +### Use Case 4: Personal Dashboard + +An engineer wants to see only their owned entities with issues: + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=true&page=1&pageSize=10" \ + -H "Authorization: Bearer " +``` + +This returns a personalized view of entities they're responsible for. + +## Limitations + +### Direct Parent Groups Only (when using `ownedByMe=true`) + +Similar to the aggregation endpoint, `ownedByMe=true` only includes entities owned by direct parent groups, not nested hierarchies. See [Entity Aggregation](./aggregation.md) for details on enabling transitive parent groups. + +### Entity Metadata Freshness + +Entity metadata (name, kind, owner) is fetched from the catalog at request time and reflects the current state. However, metric values and timestamps represent historical data from the last sync. This means: + +- If an entity was renamed, the new name appears +- If ownership changed, the new owner appears +- But the metric value and timestamp are from the last sync, not re-calculated + +## Troubleshooting + +### Empty Results + +**Symptom**: `total: 0` even though aggregation shows entities in that category + +**Possible causes**: + +1. **Stale aggregation data**: Aggregation was cached, entities have since changed status +2. **Permission changes**: User lost access to entities between viewing aggregation and drill-down +3. **Incorrect filters**: Check filter parameters match the aggregation criteria + +### Missing Entity Metadata + +**Symptom**: Entities show "Unknown" for name, kind, or owner + +**Possible causes**: + +1. **Entity deleted from catalog**: Entity ref exists in metrics but entity removed +2. **Permission denied**: User lacks `catalog.entity.read` for that entity +3. **Catalog API error**: Temporary failure fetching catalog metadata + +**Resolution**: Check console logs for warnings about failed entity fetches. + +### Slow Responses + +**Symptom**: Response times > 5 seconds + +**Possible causes**: + +1. **Large result set**: Too many entities match the filters +2. **Entity name search**: Using `entityName` filter fetches all entities before filtering +3. **No filters applied**: Returning all entities in the system + +**Resolution**: + +- Use more specific filters (status, kind, owner) +- Reduce page size +- Avoid `entityName` search on large datasets +- Consider using `ownedByMe=true` to scope results + +## Related Documentation + +- [Entity Aggregation](./aggregation.md) - Parent aggregation endpoint that shows summary counts +- [Thresholds](./thresholds.md) - How threshold evaluation determines success/warning/error status +- [Metric Providers](./providers.md) - How metrics are collected and stored From 64020c440d12b75ae3ba82fce8e5354c77f3b5da Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 10:30:32 -0500 Subject: [PATCH 04/17] feat(scorecard): normalize owners returned from catalog Signed-off-by: Patrick Knight --- .../src/scheduler/tasks/PullMetricsByProviderTask.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 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 041547b40e..ce3399414b 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts @@ -162,7 +162,7 @@ export class PullMetricsByProviderTask implements SchedulerTask { timestamp: new Date(), status, entity_kind: entity.kind, - entity_owner: JSON.stringify(entity?.spec?.owner) ?? undefined, + entity_owner: normalizeOwner(entity?.spec?.owner), } as DbMetricValueCreate; } catch (error) { return { @@ -173,7 +173,7 @@ export class PullMetricsByProviderTask implements SchedulerTask { error_message: error instanceof Error ? error.message : String(error), entity_kind: entity.kind, - entity_owner: JSON.stringify(entity?.spec?.owner) ?? undefined, + entity_owner: normalizeOwner(entity?.spec?.owner), } as DbMetricValueCreate; } }), @@ -200,3 +200,9 @@ export class PullMetricsByProviderTask implements SchedulerTask { } } } + +function normalizeOwner(owner: unknown): string | undefined { + if (!owner) return undefined; + if (typeof owner === 'string') return owner; + return JSON.stringify(owner); +} From ebfda2ffc7efd5432e45270c79bb1bc2d328f45e Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 10:31:38 -0500 Subject: [PATCH 05/17] feat(scorecard): ensure kind and owner are case insensitive Signed-off-by: Patrick Knight --- .../scorecard-backend/src/database/DatabaseMetricValues.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts index 18ca88e4c1..662b963ad8 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts @@ -173,12 +173,12 @@ export class DatabaseMetricValues { // Filter by entity_kind if (entityKind) { - query.where('entity_kind', entityKind); + query.whereRaw('LOWER(entity_kind) = LOWER(?)', [entityKind]); } // Filter by entity_owner if (entityOwner) { - query.where('entity_owner', entityOwner); + query.whereRaw('LOWER(entity_owner) = LOWER(?)', [entityOwner]); } if (pagination) { From 69a44c94aef017d7e509cd12bc8cbbb857d1d5b0 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 10:48:37 -0500 Subject: [PATCH 06/17] feat(scorecard): add changeset Signed-off-by: Patrick Knight --- workspaces/scorecard/.changeset/sour-coins-check.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 workspaces/scorecard/.changeset/sour-coins-check.md diff --git a/workspaces/scorecard/.changeset/sour-coins-check.md b/workspaces/scorecard/.changeset/sour-coins-check.md new file mode 100644 index 0000000000..5a8f2bfe37 --- /dev/null +++ b/workspaces/scorecard/.changeset/sour-coins-check.md @@ -0,0 +1,6 @@ +--- +'@red-hat-developer-hub/backstage-plugin-scorecard-backend': minor +'@red-hat-developer-hub/backstage-plugin-scorecard-common': minor +--- + +Adds the ability to drill down from aggregated scorecard KPIs to view the individual entities that contribute to the overall score. This enables managers and platform engineers to identify specific services impacting metrics and troubleshoot issues at the entity level. From 63820e2d495149df3ab312acfccec01d567f9485 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 10:55:24 -0500 Subject: [PATCH 07/17] feat(scorecard): remove indexes from migration Signed-off-by: Patrick Knight --- .../20260217152637_add_entity_metadata_columns.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/workspaces/scorecard/plugins/scorecard-backend/migrations/20260217152637_add_entity_metadata_columns.js b/workspaces/scorecard/plugins/scorecard-backend/migrations/20260217152637_add_entity_metadata_columns.js index 18fa31ac3d..f3f26b3b9a 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/migrations/20260217152637_add_entity_metadata_columns.js +++ b/workspaces/scorecard/plugins/scorecard-backend/migrations/20260217152637_add_entity_metadata_columns.js @@ -21,19 +21,11 @@ exports.up = async function up(knex) { // Add entity_owner column (stores the owner entity ref) table.string('entity_owner', 255).nullable(); - - // Optional: Add index for better filtering performance - table.index(['entity_kind'], 'idx_metric_values_entity_kind'); - table.index(['entity_owner'], 'idx_metric_values_entity_owner'); }); }; exports.down = async function down(knex) { await knex.schema.alterTable('metric_values', table => { - // Drop indexes first - table.dropIndex([], 'idx_metric_values_entity_kind'); - table.dropIndex([], 'idx_metric_values_entity_owner'); - // Drop columns table.dropColumn('entity_kind'); table.dropColumn('entity_owner'); From 6ac78063fefdb7c4938858155159dbcc74e21f1a Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 10:57:18 -0500 Subject: [PATCH 08/17] feat(scorecard): update report.api.md Signed-off-by: Patrick Knight --- .../plugins/scorecard-common/report.api.md | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/workspaces/scorecard/plugins/scorecard-common/report.api.md b/workspaces/scorecard/plugins/scorecard-common/report.api.md index 589032df52..ba8423db80 100644 --- a/workspaces/scorecard/plugins/scorecard-common/report.api.md +++ b/workspaces/scorecard/plugins/scorecard-common/report.api.md @@ -34,6 +34,35 @@ export type AggregatedMetricValue = { // @public export const DEFAULT_NUMBER_THRESHOLDS: ThresholdConfig; +// @public +export type EntityMetricDetail = { + entityRef: string; + entityName: string; + entityKind: string; + owner: string; + metricValue: number | boolean | null; + timestamp: string; + status: 'success' | 'warning' | 'error'; + score?: string; +}; + +// @public +export type EntityMetricDetailResponse = { + metricId: string; + metricMetadata: { + title: string; + description: string; + type: MetricType; + }; + entities: EntityMetricDetail[]; + pagination: { + page: number; + pageSize: number; + total: number; + totalPages: number; + }; +}; + // @public (undocumented) export type Metric = { id: string; From d5c6ae7455fef1a092acead7af530ee9ec4bb421 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 11:27:40 -0500 Subject: [PATCH 09/17] feat(scorecard): ensure we do not bypass catalog conditional permissions Signed-off-by: Patrick Knight --- .../src/database/DatabaseMetricValues.test.ts | 13 +- .../src/database/DatabaseMetricValues.ts | 24 +-- .../src/permissions/permissionUtils.test.ts | 154 ++++++++++++++++++ .../src/permissions/permissionUtils.ts | 29 ++++ .../src/service/router.test.ts | 94 +++++++++-- .../scorecard-backend/src/service/router.ts | 12 +- 6 files changed, 291 insertions(+), 35 deletions(-) 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 f704055000..224f1d54c8 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts @@ -818,13 +818,13 @@ describe('DatabaseMetricValues', () => { ); it.each(databases.eachSupportedId())( - 'should handle empty entity refs list (fetch all entities) - %p', + 'should return empty result when entity refs is empty, not bypass to fetch all - %p', async databaseId => { const { client, db } = await createDatabase(databaseId); const timestamp = new Date('2023-01-01T00:00:00Z'); - // Insert entities + // Insert entities that would previously be returned when passing an empty array await client('metric_values').insert([ { catalog_entity_ref: 'component:default/service1', @@ -846,9 +846,10 @@ describe('DatabaseMetricValues', () => { }, ]); - // Empty array = fetch all entities for this metric + // Empty array must return no results — an empty scope means no authorized entities, + // not an unscoped "fetch all" that would bypass catalog read permissions. const result = await db.readEntityMetricsByStatus( - [], // Empty array + [], 'github.metric1', 'error', undefined, @@ -856,8 +857,8 @@ describe('DatabaseMetricValues', () => { { limit: 10, offset: 0 }, ); - expect(result.rows).toHaveLength(2); - expect(result.total).toBe(2); + expect(result.rows).toHaveLength(0); + expect(result.total).toBe(0); }, ); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts index 662b963ad8..e46c1cc23c 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts @@ -142,28 +142,22 @@ export class DatabaseMetricValues { entityOwner?: string, pagination?: { limit: number; offset: number }, ): Promise<{ rows: DbMetricValue[]; total: number }> { - // Build subquery for latest metric IDs - const latestIdsSubquery = this.dbClient(this.tableName) - .max('id') - .where('metric_id', metric_id); - - // Only add entity ref filter if provided (non-empty array) - if (catalog_entity_refs.length > 0) { - latestIdsSubquery.whereIn('catalog_entity_ref', catalog_entity_refs); + if (catalog_entity_refs.length === 0) { + return { rows: [], total: 0 }; } - latestIdsSubquery.groupBy('metric_id', 'catalog_entity_ref'); + const latestIdsSubquery = this.dbClient(this.tableName) + .max('id') + .where('metric_id', metric_id) + .whereIn('catalog_entity_ref', catalog_entity_refs) + .groupBy('metric_id', 'catalog_entity_ref'); const query = this.dbClient(this.tableName) .select('*') .select(this.dbClient.raw('COUNT(*) OVER() as total_count')) .whereIn('id', latestIdsSubquery) - .where('metric_id', metric_id); - - // Only add entity ref filter if provided - if (catalog_entity_refs.length > 0) { - query.whereIn('catalog_entity_ref', catalog_entity_refs); - } + .where('metric_id', metric_id) + .whereIn('catalog_entity_ref', catalog_entity_refs); query.orderBy('timestamp', 'desc'); 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 1dde25f617..094d0d8294 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.test.ts @@ -24,6 +24,7 @@ import { Request } from 'express'; import { NotAllowedError } from '@backstage/errors'; import { catalogEntityReadPermission } from '@backstage/plugin-catalog-common/alpha'; import type { + BackstageCredentials, HttpAuthService, PermissionsService, } from '@backstage/backend-plugin-api'; @@ -32,8 +33,10 @@ import { filterAuthorizedMetrics, matches, checkEntityAccess, + getAuthorizedEntityRefs, } from './permissionUtils'; import { mockServices } from '@backstage/backend-test-utils'; +import { catalogServiceMock } from '@backstage/plugin-catalog-node/testUtils'; const createMockMetric = ( id: string, @@ -228,4 +231,155 @@ describe('permissionUtils', () => { ); }); }); + + describe('getAuthorizedEntityRefs', () => { + let mockedCatalog: ReturnType; + let mockCredentials: BackstageCredentials; + + beforeEach(() => { + mockedCatalog = catalogServiceMock.mock(); + mockCredentials = {} as BackstageCredentials; + + mockedCatalog.queryEntities.mockResolvedValue({ + items: [ + { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { name: 'my-service', namespace: 'default' }, + }, + ], + pageInfo: { nextCursor: undefined }, + totalItems: 1, + }); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should return entity refs for entities the user is authorized to see', async () => { + const result = await getAuthorizedEntityRefs({ + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(result).toEqual(['component:default/my-service']); + }); + + it('should call queryEntities with user credentials', async () => { + await getAuthorizedEntityRefs({ + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(mockedCatalog.queryEntities).toHaveBeenCalledWith( + expect.any(Object), + { credentials: mockCredentials }, + ); + }); + + it('should call queryEntities with correct fields and batch size limit', async () => { + await getAuthorizedEntityRefs({ + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(mockedCatalog.queryEntities).toHaveBeenCalledWith( + { + fields: ['kind', 'metadata.name', 'metadata.namespace'], + limit: 50, + }, + { credentials: mockCredentials }, + ); + }); + + it('should return empty array when catalog has no entities', async () => { + mockedCatalog.queryEntities.mockResolvedValue({ + items: [], + pageInfo: { nextCursor: undefined }, + totalItems: 0, + }); + + const result = await getAuthorizedEntityRefs({ + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(result).toEqual([]); + }); + + it('should handle cursor-based pagination across multiple pages', async () => { + const entity1 = { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { name: 'service-1', namespace: 'default' }, + }; + const entity2 = { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { name: 'service-2', namespace: 'default' }, + }; + + mockedCatalog.queryEntities + .mockResolvedValueOnce({ + items: [entity1], + pageInfo: { nextCursor: 'cursor1' }, + totalItems: 2, + }) + .mockResolvedValueOnce({ + items: [entity2], + pageInfo: { nextCursor: undefined }, + totalItems: 2, + }); + + const result = await getAuthorizedEntityRefs({ + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(result).toEqual([ + 'component:default/service-1', + 'component:default/service-2', + ]); + expect(mockedCatalog.queryEntities).toHaveBeenCalledTimes(2); + expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( + 2, + { + fields: ['kind', 'metadata.name', 'metadata.namespace'], + limit: 50, + cursor: 'cursor1', + }, + { credentials: mockCredentials }, + ); + }); + + it('should return entity refs for multiple entity kinds', async () => { + mockedCatalog.queryEntities.mockResolvedValue({ + items: [ + { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { name: 'my-service', namespace: 'default' }, + }, + { + apiVersion: 'backstage.io/v1alpha1', + kind: 'API', + metadata: { name: 'my-api', namespace: 'default' }, + }, + ], + pageInfo: { nextCursor: undefined }, + totalItems: 2, + }); + + const result = await getAuthorizedEntityRefs({ + catalog: mockedCatalog, + credentials: mockCredentials, + }); + + expect(result).toEqual([ + 'component:default/my-service', + 'api:default/my-api', + ]); + }); + }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.ts b/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.ts index 4bd81ab8aa..ce3e60c292 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.ts @@ -24,6 +24,7 @@ import { Request } from 'express'; import { NotAllowedError } from '@backstage/errors'; import { catalogEntityReadPermission } from '@backstage/plugin-catalog-common/alpha'; import type { + BackstageCredentials, HttpAuthService, PermissionsService, } from '@backstage/backend-plugin-api'; @@ -31,6 +32,10 @@ import type { import { Metric } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { rules as scorecardRules } from './rules'; +import { CatalogService } from '@backstage/plugin-catalog-node'; +import { stringifyEntityRef } from '@backstage/catalog-model'; + +const QUERY_ENTITIES_BATCH_SIZE = 50; export const checkEntityAccess = async ( entityRef: string, @@ -48,6 +53,30 @@ export const checkEntityAccess = async ( } }; +export async function getAuthorizedEntityRefs(options: { + catalog: CatalogService; + credentials: BackstageCredentials; +}): Promise { + const entityRefs: string[] = []; + let cursor: string | undefined = undefined; + + do { + const result = await options.catalog.queryEntities( + { + fields: ['kind', 'metadata.name', 'metadata.namespace'], + limit: QUERY_ENTITIES_BATCH_SIZE, + ...(cursor ? { cursor } : {}), + }, + { credentials: options.credentials }, // user credentials — enforces conditional policies + ); + + cursor = result.pageInfo.nextCursor; + entityRefs.push(...result.items.map(e => stringifyEntityRef(e))); + } while (cursor !== undefined); + + return entityRefs; +} + export const matches = ( metric: Metric, filters?: PermissionCriteria< 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 2b98937df5..f2c5c9d293 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -52,6 +52,7 @@ jest.mock('../permissions/permissionUtils', () => { return { ...originalModule, checkEntityAccess: jest.fn(), + getAuthorizedEntityRefs: jest.fn(), }; }); @@ -730,6 +731,11 @@ describe('createRouter', () => { }); describe('GET /metrics/:metricId/catalog/aggregations/entities', () => { + const AUTHORIZED_ENTITY_REFS = [ + 'component:default/my-service', + 'component:default/another-service', + ]; + const mockEntityMetricDetailResponse = { metricId: 'github.open_prs', metricMetadata: { @@ -769,6 +775,7 @@ describe('createRouter', () => { let getEntityMetricDetailsSpy: jest.SpyInstance; let getEntitiesOwnedByUserSpy: jest.SpyInstance; let checkEntityAccessSpy: jest.SpyInstance; + let getAuthorizedEntityRefsSpy: jest.SpyInstance; beforeEach(async () => { const githubProvider = new MockNumberProvider( @@ -801,6 +808,10 @@ describe('createRouter', () => { 'checkEntityAccess', ); + getAuthorizedEntityRefsSpy = jest + .spyOn(permissionUtilsModule, 'getAuthorizedEntityRefs') + .mockResolvedValue(AUTHORIZED_ENTITY_REFS); + const mockCatalog = catalogServiceMock.mock(); const router = await createRouter({ metricProvidersRegistry, @@ -827,7 +838,7 @@ describe('createRouter', () => { expect(response.status).toBe(200); expect(response.body).toEqual(mockEntityMetricDetailResponse); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - [], + AUTHORIZED_ENTITY_REFS, 'github.open_prs', { status: undefined, @@ -848,7 +859,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - [], + AUTHORIZED_ENTITY_REFS, 'github.open_prs', expect.objectContaining({ page: 2, @@ -863,7 +874,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - [], + AUTHORIZED_ENTITY_REFS, 'github.open_prs', expect.objectContaining({ limit: 100, @@ -877,7 +888,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - [], + AUTHORIZED_ENTITY_REFS, 'github.open_prs', expect.objectContaining({ status: 'error', @@ -891,7 +902,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - [], + AUTHORIZED_ENTITY_REFS, 'github.open_prs', expect.objectContaining({ owner: 'team:default/platform', @@ -905,7 +916,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - [], + AUTHORIZED_ENTITY_REFS, 'github.open_prs', expect.objectContaining({ kind: 'Component', @@ -919,7 +930,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - [], + AUTHORIZED_ENTITY_REFS, 'github.open_prs', expect.objectContaining({ entityName: 'service', @@ -973,7 +984,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - [], + AUTHORIZED_ENTITY_REFS, 'github.open_prs', expect.objectContaining({ sortBy: 'entityName', @@ -988,7 +999,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - [], + AUTHORIZED_ENTITY_REFS, 'github.open_prs', expect.objectContaining({ sortBy: 'metricValue', @@ -1003,7 +1014,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - [], + AUTHORIZED_ENTITY_REFS, 'github.open_prs', expect.objectContaining({ status: 'error', @@ -1082,5 +1093,68 @@ describe('createRouter', () => { expect(response.body.entities).toEqual([]); expect(response.body.pagination.total).toBe(0); }); + + it('should call getAuthorizedEntityRefs when ownedByMe is not set', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities', + ); + + expect(getAuthorizedEntityRefsSpy).toHaveBeenCalledTimes(1); + expect(getAuthorizedEntityRefsSpy).toHaveBeenCalledWith( + expect.objectContaining({ + catalog: expect.any(Object), + credentials: expect.any(Object), + }), + ); + }); + + it('should call getAuthorizedEntityRefs when ownedByMe=false', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=false', + ); + + expect(getAuthorizedEntityRefsSpy).toHaveBeenCalledTimes(1); + expect(getEntitiesOwnedByUserSpy).not.toHaveBeenCalled(); + }); + + it('should not call getEntitiesOwnedByUser when ownedByMe is not set', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities', + ); + + expect(getEntitiesOwnedByUserSpy).not.toHaveBeenCalled(); + }); + + it('should not call checkEntityAccess when ownedByMe is not set', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities', + ); + + expect(checkEntityAccessSpy).not.toHaveBeenCalled(); + }); + + it('should pass authorized entity refs from getAuthorizedEntityRefs to getEntityMetricDetails', async () => { + const restrictedRefs = ['component:default/allowed-service']; + getAuthorizedEntityRefsSpy.mockResolvedValueOnce(restrictedRefs); + + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities', + ); + + expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( + restrictedRefs, + 'github.open_prs', + expect.any(Object), + ); + }); + + it('should not call getAuthorizedEntityRefs when ownedByMe=true', async () => { + await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=true', + ); + + expect(getAuthorizedEntityRefsSpy).not.toHaveBeenCalled(); + expect(getEntitiesOwnedByUserSpy).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index 5704186ecd..eafdf9e5b0 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -37,6 +37,7 @@ import { scorecardMetricReadPermission } from '@red-hat-developer-hub/backstage- import { filterAuthorizedMetrics, checkEntityAccess, + getAuthorizedEntityRefs, } from '../permissions/permissionUtils'; import { stringifyEntityRef } from '@backstage/catalog-model'; import { validateCatalogMetricsSchema } from '../validation/validateCatalogMetricsSchema'; @@ -253,13 +254,16 @@ export async function createRouter({ await checkEntityAccess(entityRef, req, permissions, httpAuth); } } else { - // Default: Get ALL entity refs from metric_values table for this metric - // (Service layer will fetch from database without entity scoping) - entityRefsToQuery = []; // Empty array signals "fetch all" + // Scope to entities the user is authorized to read from the catalog. + // Passing user credentials means catalog enforces conditional policies natively. + entityRefsToQuery = await getAuthorizedEntityRefs({ + catalog, + credentials, + }); } const entityMetrics = await catalogMetricService.getEntityMetricDetails( - entityRefsToQuery, // Empty = all, populated = scoped + entityRefsToQuery, metricId, { status, From 844c14f3820babe6dd4633474dac61a5ded039c9 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 11:56:06 -0500 Subject: [PATCH 10/17] feat(scorecard): ensure we utilize users credentials for batch catalog fetch Signed-off-by: Patrick Knight --- .../src/service/CatalogMetricService.test.ts | 94 ++++++++++++++----- .../src/service/CatalogMetricService.ts | 9 +- .../src/service/router.test.ts | 24 ++++- .../scorecard-backend/src/service/router.ts | 1 + 4 files changed, 101 insertions(+), 27 deletions(-) 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 6f36077437..0860047e1e 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts @@ -25,7 +25,11 @@ import { mockDatabaseMetricValues, } from '../../__fixtures__/mockDatabaseMetricValues'; import { buildMockMetricProvidersRegistry } from '../../__fixtures__/mockMetricProvidersRegistry'; -import { AuthService, LoggerService } from '@backstage/backend-plugin-api'; +import { + AuthService, + BackstageCredentials, + LoggerService, +} from '@backstage/backend-plugin-api'; import * as permissionUtils from '../permissions/permissionUtils'; import { AggregatedMetric, @@ -572,6 +576,8 @@ describe('CatalogMetricService', () => { ], }; + let mockCredentials: BackstageCredentials; + beforeEach(() => { mockedDatabase.readEntityMetricsByStatus.mockResolvedValue({ rows: mockMetricRows, @@ -580,12 +586,14 @@ describe('CatalogMetricService', () => { mockedCatalog.getEntitiesByRefs.mockReset(); mockedCatalog.getEntitiesByRefs.mockResolvedValue(mockEntities); + mockCredentials = {} as BackstageCredentials; }); it('should fetch entity metrics with default options', async () => { const result = await service.getEntityMetricDetails( [], 'github.important_metric', + mockCredentials, { page: 1, limit: 10, @@ -606,6 +614,7 @@ describe('CatalogMetricService', () => { const result = await service.getEntityMetricDetails( [], 'github.important_metric', + mockCredentials, { page: 1, limit: 10, @@ -624,10 +633,15 @@ describe('CatalogMetricService', () => { }); it('should call database with correct pagination parameters', async () => { - await service.getEntityMetricDetails([], 'github.important_metric', { - page: 2, - limit: 5, - }); + await service.getEntityMetricDetails( + [], + 'github.important_metric', + mockCredentials, + { + page: 2, + limit: 5, + }, + ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( [], @@ -640,11 +654,16 @@ describe('CatalogMetricService', () => { }); it('should filter by status at database level', async () => { - await service.getEntityMetricDetails([], 'github.important_metric', { - status: 'error', - page: 1, - limit: 10, - }); + await service.getEntityMetricDetails( + [], + 'github.important_metric', + mockCredentials, + { + status: 'error', + page: 1, + limit: 10, + }, + ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( [], @@ -657,11 +676,16 @@ describe('CatalogMetricService', () => { }); it('should filter by kind at database level', async () => { - await service.getEntityMetricDetails([], 'github.important_metric', { - kind: 'Component', - page: 1, - limit: 10, - }); + await service.getEntityMetricDetails( + [], + 'github.important_metric', + mockCredentials, + { + kind: 'Component', + page: 1, + limit: 10, + }, + ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( [], @@ -674,11 +698,16 @@ describe('CatalogMetricService', () => { }); it('should filter by owner at database level', async () => { - await service.getEntityMetricDetails([], 'github.important_metric', { - owner: 'team:default/platform', - page: 1, - limit: 10, - }); + await service.getEntityMetricDetails( + [], + 'github.important_metric', + mockCredentials, + { + owner: 'team:default/platform', + page: 1, + limit: 10, + }, + ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( [], @@ -694,6 +723,7 @@ describe('CatalogMetricService', () => { const result = await service.getEntityMetricDetails( [], 'github.important_metric', + mockCredentials, { entityName: 'service-a', page: 1, @@ -721,6 +751,7 @@ describe('CatalogMetricService', () => { const result = await service.getEntityMetricDetails( [], 'github.important_metric', + mockCredentials, { entityName: 'SERVICE', page: 1, @@ -735,6 +766,7 @@ describe('CatalogMetricService', () => { const result = await service.getEntityMetricDetails( [], 'github.important_metric', + mockCredentials, { sortBy: 'entityName', sortOrder: 'asc', @@ -752,6 +784,7 @@ describe('CatalogMetricService', () => { const result = await service.getEntityMetricDetails( [], 'github.important_metric', + mockCredentials, { sortBy: 'metricValue', sortOrder: 'desc', @@ -769,6 +802,7 @@ describe('CatalogMetricService', () => { const result = await service.getEntityMetricDetails( [], 'github.important_metric', + mockCredentials, { page: 1, limit: 10, @@ -790,6 +824,7 @@ describe('CatalogMetricService', () => { const result = await service.getEntityMetricDetails( [], 'github.important_metric', + mockCredentials, { sortBy: 'metricValue', sortOrder: 'desc', @@ -804,10 +839,15 @@ describe('CatalogMetricService', () => { }); it('should batch-fetch entities using getEntitiesByRefs', async () => { - await service.getEntityMetricDetails([], 'github.important_metric', { - page: 1, - limit: 10, - }); + await service.getEntityMetricDetails( + [], + 'github.important_metric', + mockCredentials, + { + page: 1, + limit: 10, + }, + ); expect(mockedCatalog.getEntitiesByRefs).toHaveBeenCalledWith( { @@ -830,6 +870,7 @@ describe('CatalogMetricService', () => { const result = await service.getEntityMetricDetails( [], 'github.important_metric', + mockCredentials, { page: 1, limit: 10, @@ -849,6 +890,7 @@ describe('CatalogMetricService', () => { const result = await service.getEntityMetricDetails( [], 'github.important_metric', + mockCredentials, { page: 1, limit: 10, @@ -869,6 +911,7 @@ describe('CatalogMetricService', () => { await service.getEntityMetricDetails( ['component:default/service-a', 'component:default/service-b'], 'github.important_metric', + mockCredentials, { page: 1, limit: 10 }, ); @@ -891,6 +934,7 @@ describe('CatalogMetricService', () => { const result = await service.getEntityMetricDetails( [], 'github.important_metric', + mockCredentials, { status: 'error', kind: 'Component', @@ -924,6 +968,7 @@ describe('CatalogMetricService', () => { const result = await service.getEntityMetricDetails( [], 'github.important_metric', + mockCredentials, { page: 1, limit: 10, @@ -943,6 +988,7 @@ describe('CatalogMetricService', () => { const result = await service.getEntityMetricDetails( [], 'github.important_metric', + mockCredentials, { page: 1, limit: 10, diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts index 305f53ba3b..5f3ae8b0bb 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -23,7 +23,11 @@ import { } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { MetricProvidersRegistry } from '../providers/MetricProvidersRegistry'; import { NotFoundError, stringifyError } from '@backstage/errors'; -import { AuthService, LoggerService } from '@backstage/backend-plugin-api'; +import { + AuthService, + BackstageCredentials, + LoggerService, +} from '@backstage/backend-plugin-api'; import { filterAuthorizedMetrics } from '../permissions/permissionUtils'; import { PermissionCondition, @@ -198,6 +202,7 @@ export class CatalogMetricService { async getEntityMetricDetails( entityRefs: string[], metricId: string, + credentials: BackstageCredentials, options: { status?: 'success' | 'warning' | 'error'; owner?: string; @@ -251,7 +256,7 @@ export class CatalogMetricService { entityRefs: entityRefsToFetch, fields: ['kind', 'metadata', 'spec'], // Only fetch needed fields }, - { credentials: await this.auth.getOwnServiceCredentials() }, + { credentials }, ); // Build map of ref -> entity 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 f2c5c9d293..6df2f27d08 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -40,7 +40,10 @@ import { AuthorizeResult, PolicyDecision, } from '@backstage/plugin-permission-common'; -import { PermissionsService } from '@backstage/backend-plugin-api'; +import { + BackstageCredentials, + PermissionsService, +} from '@backstage/backend-plugin-api'; import { mockDatabaseMetricValues } from '../../__fixtures__/mockDatabaseMetricValues'; jest.mock('../utils/getEntitiesOwnedByUser', () => ({ @@ -776,6 +779,7 @@ describe('createRouter', () => { let getEntitiesOwnedByUserSpy: jest.SpyInstance; let checkEntityAccessSpy: jest.SpyInstance; let getAuthorizedEntityRefsSpy: jest.SpyInstance; + let mockCredentials: BackstageCredentials; beforeEach(async () => { const githubProvider = new MockNumberProvider( @@ -824,6 +828,11 @@ describe('createRouter', () => { drillDownApp = express(); drillDownApp.use(router); drillDownApp.use(mockErrorHandler()); + mockCredentials = { + principal: { + userEntityRef: 'user:default/test-user', + }, + } as BackstageCredentials; }); afterEach(() => { @@ -840,6 +849,7 @@ describe('createRouter', () => { expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( AUTHORIZED_ENTITY_REFS, 'github.open_prs', + mockCredentials, { status: undefined, owner: undefined, @@ -861,6 +871,7 @@ describe('createRouter', () => { expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( AUTHORIZED_ENTITY_REFS, 'github.open_prs', + mockCredentials, expect.objectContaining({ page: 2, limit: 20, @@ -876,6 +887,7 @@ describe('createRouter', () => { expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( AUTHORIZED_ENTITY_REFS, 'github.open_prs', + mockCredentials, expect.objectContaining({ limit: 100, }), @@ -890,6 +902,7 @@ describe('createRouter', () => { expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( AUTHORIZED_ENTITY_REFS, 'github.open_prs', + mockCredentials, expect.objectContaining({ status: 'error', }), @@ -904,6 +917,7 @@ describe('createRouter', () => { expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( AUTHORIZED_ENTITY_REFS, 'github.open_prs', + mockCredentials, expect.objectContaining({ owner: 'team:default/platform', }), @@ -918,6 +932,7 @@ describe('createRouter', () => { expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( AUTHORIZED_ENTITY_REFS, 'github.open_prs', + mockCredentials, expect.objectContaining({ kind: 'Component', }), @@ -932,6 +947,7 @@ describe('createRouter', () => { expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( AUTHORIZED_ENTITY_REFS, 'github.open_prs', + mockCredentials, expect.objectContaining({ entityName: 'service', }), @@ -954,6 +970,7 @@ describe('createRouter', () => { expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( ['component:default/my-service', 'component:default/another-service'], 'github.open_prs', + mockCredentials, expect.any(Object), ); }); @@ -986,6 +1003,7 @@ describe('createRouter', () => { expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( AUTHORIZED_ENTITY_REFS, 'github.open_prs', + mockCredentials, expect.objectContaining({ sortBy: 'entityName', sortOrder: 'asc', @@ -1001,6 +1019,7 @@ describe('createRouter', () => { expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( AUTHORIZED_ENTITY_REFS, 'github.open_prs', + mockCredentials, expect.objectContaining({ sortBy: 'metricValue', sortOrder: 'desc', @@ -1016,6 +1035,7 @@ describe('createRouter', () => { expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( AUTHORIZED_ENTITY_REFS, 'github.open_prs', + mockCredentials, expect.objectContaining({ status: 'error', kind: 'Component', @@ -1080,6 +1100,7 @@ describe('createRouter', () => { it('should return empty entities array when no results', async () => { getEntityMetricDetailsSpy.mockResolvedValue({ metricId: 'github.open_prs', + mockCredentials, metricMetadata: mockEntityMetricDetailResponse.metricMetadata, entities: [], pagination: { page: 1, pageSize: 10, total: 0, totalPages: 0 }, @@ -1144,6 +1165,7 @@ describe('createRouter', () => { expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( restrictedRefs, 'github.open_prs', + mockCredentials, expect.any(Object), ); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index eafdf9e5b0..fbdbf3a94a 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -265,6 +265,7 @@ export async function createRouter({ const entityMetrics = await catalogMetricService.getEntityMetricDetails( entityRefsToQuery, metricId, + credentials, { status, owner, From 56a6474392f1383cfd69d0715f271555136ce5e0 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 12:37:55 -0500 Subject: [PATCH 11/17] feat(scorecard): add input validation to the new endpoint Signed-off-by: Patrick Knight --- .../src/service/router.test.ts | 161 ++++++++++++++- .../scorecard-backend/src/service/router.ts | 35 ++-- .../validateCatalogMetricsSchema.test.ts | 184 +++++++++++++++++- .../validateCatalogMetricsSchema.ts | 27 +++ 4 files changed, 384 insertions(+), 23 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 6df2f27d08..3fa1eaa37c 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -879,11 +879,23 @@ describe('createRouter', () => { ); }); - it('should enforce max pageSize of 100', async () => { - await request(drillDownApp).get( + it('should return 400 when pageSize exceeds max of 100', async () => { + const response = await request(drillDownApp).get( '/metrics/github.open_prs/catalog/aggregations/entities?pageSize=200', ); + expect(response.status).toBe(400); + expect(response.body.error.name).toBe('InputError'); + expect(response.body.error.message).toContain('Invalid query parameters'); + expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); + }); + + it('should accept pageSize at max boundary of 100', async () => { + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?pageSize=100', + ); + + expect(response.status).toBe(200); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( AUTHORIZED_ENTITY_REFS, 'github.open_prs', @@ -1178,5 +1190,150 @@ describe('createRouter', () => { expect(getAuthorizedEntityRefsSpy).not.toHaveBeenCalled(); expect(getEntitiesOwnedByUserSpy).toHaveBeenCalledTimes(1); }); + + describe('input validation', () => { + it('should return 400 when page is 0', async () => { + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?page=0', + ); + + expect(response.status).toBe(400); + expect(response.body.error.name).toBe('InputError'); + expect(response.body.error.message).toContain( + 'Invalid query parameters', + ); + expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); + }); + + it('should return 400 when page is negative', async () => { + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?page=-1', + ); + + expect(response.status).toBe(400); + expect(response.body.error.name).toBe('InputError'); + expect(response.body.error.message).toContain( + 'Invalid query parameters', + ); + expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); + }); + + it('should return 400 when page exceeds max of 10000', async () => { + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?page=10001', + ); + + expect(response.status).toBe(400); + expect(response.body.error.name).toBe('InputError'); + expect(response.body.error.message).toContain( + 'Invalid query parameters', + ); + expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); + }); + + it('should return 400 when pageSize is 0', async () => { + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?pageSize=0', + ); + + expect(response.status).toBe(400); + expect(response.body.error.name).toBe('InputError'); + expect(response.body.error.message).toContain( + 'Invalid query parameters', + ); + expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); + }); + + it('should return 400 when status is an invalid value', async () => { + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?status=unknown', + ); + + expect(response.status).toBe(400); + expect(response.body.error.name).toBe('InputError'); + expect(response.body.error.message).toContain( + 'Invalid query parameters', + ); + expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); + }); + + it('should return 400 when sortBy is an invalid value', async () => { + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?sortBy=invalid', + ); + + expect(response.status).toBe(400); + expect(response.body.error.name).toBe('InputError'); + expect(response.body.error.message).toContain( + 'Invalid query parameters', + ); + expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); + }); + + it('should return 400 when sortOrder is an invalid value', async () => { + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?sortOrder=random', + ); + + expect(response.status).toBe(400); + expect(response.body.error.name).toBe('InputError'); + expect(response.body.error.message).toContain( + 'Invalid query parameters', + ); + expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); + }); + + it('should return 400 when ownedByMe is an invalid value', async () => { + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=yes', + ); + + expect(response.status).toBe(400); + expect(response.body.error.name).toBe('InputError'); + expect(response.body.error.message).toContain( + 'Invalid query parameters', + ); + expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); + }); + + it('should return 400 when owner is an empty string', async () => { + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?owner=', + ); + + expect(response.status).toBe(400); + expect(response.body.error.name).toBe('InputError'); + expect(response.body.error.message).toContain( + 'Invalid query parameters', + ); + expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); + }); + + it('should return 400 when kind is an empty string', async () => { + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?kind=', + ); + + expect(response.status).toBe(400); + expect(response.body.error.name).toBe('InputError'); + expect(response.body.error.message).toContain( + 'Invalid query parameters', + ); + expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); + }); + + it('should return 400 when entityName is an empty string', async () => { + const response = await request(drillDownApp).get( + '/metrics/github.open_prs/catalog/aggregations/entities?entityName=', + ); + + expect(response.status).toBe(400); + expect(response.body.error.name).toBe('InputError'); + expect(response.body.error.message).toContain( + 'Invalid query parameters', + ); + expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); + }); + }); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index fbdbf3a94a..744a6b9c38 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -40,7 +40,10 @@ import { getAuthorizedEntityRefs, } from '../permissions/permissionUtils'; import { stringifyEntityRef } from '@backstage/catalog-model'; -import { validateCatalogMetricsSchema } from '../validation/validateCatalogMetricsSchema'; +import { + validateCatalogMetricsSchema, + validateDrillDownMetricsSchema, +} from '../validation/validateCatalogMetricsSchema'; import { getEntitiesOwnedByUser } from '../utils/getEntitiesOwnedByUser'; import { parseCommaSeparatedString } from '../utils/parseCommaSeparatedString'; import { validateMetricsSchema } from '../validation/validateMetricsSchema'; @@ -198,25 +201,17 @@ export async function createRouter({ async (req, res) => { const { metricId } = req.params; - const page = Number(req.query.page) || 1; - const pageSize = Math.min(Number(req.query.pageSize) || 5, 100); - const status = req.query.status as - | 'success' - | 'warning' - | 'error' - | undefined; - const ownedByMe = req.query.ownedByMe === 'true'; - const owner = req.query.owner as string | undefined; - const kind = req.query.kind as string | undefined; - const entityName = req.query.entityName as string | undefined; - const sortBy = req.query.sortBy as - | 'entityName' - | 'owner' - | 'entityKind' - | 'timestamp' - | 'metricValue' - | undefined; - const sortOrder = (req.query.sortOrder as 'asc' | 'desc') || 'desc'; + const { + page, + pageSize, + status, + ownedByMe, + owner, + kind, + entityName, + sortBy, + sortOrder, + } = validateDrillDownMetricsSchema(req.query); const { conditions } = await authorizeConditional( req, diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts index 331fc49ad2..ac4e0f5ab0 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts @@ -14,7 +14,10 @@ * limitations under the License. */ -import { validateCatalogMetricsSchema } from './validateCatalogMetricsSchema'; +import { + validateCatalogMetricsSchema, + validateDrillDownMetricsSchema, +} from './validateCatalogMetricsSchema'; import { InputError } from '@backstage/errors'; describe('validateCatalogMetricsSchema', () => { @@ -84,3 +87,182 @@ describe('validateCatalogMetricsSchema', () => { ); }); }); + +describe('validateDrillDownMetricsSchema', () => { + describe('valid query parameters', () => { + it('should return defaults when given an empty object', () => { + expect(validateDrillDownMetricsSchema({})).toEqual({ + page: 1, + pageSize: 5, + sortOrder: 'desc', + }); + }); + + it('should coerce page and pageSize from strings to numbers', () => { + const result = validateDrillDownMetricsSchema({ + page: '3', + pageSize: '20', + }); + expect(result.page).toBe(3); + expect(result.pageSize).toBe(20); + }); + + it('should accept page at max boundary of 10000', () => { + const result = validateDrillDownMetricsSchema({ page: '10000' }); + expect(result.page).toBe(10000); + }); + + it('should accept pageSize at max boundary of 100', () => { + const result = validateDrillDownMetricsSchema({ pageSize: '100' }); + expect(result.pageSize).toBe(100); + }); + + it.each(['success', 'warning', 'error'] as const)( + 'should accept status=%s', + status => { + const result = validateDrillDownMetricsSchema({ status }); + expect(result.status).toBe(status); + }, + ); + + it.each([ + 'entityName', + 'owner', + 'entityKind', + 'timestamp', + 'metricValue', + ] as const)('should accept sortBy=%s', sortBy => { + const result = validateDrillDownMetricsSchema({ sortBy }); + expect(result.sortBy).toBe(sortBy); + }); + + it.each(['asc', 'desc'] as const)( + 'should accept sortOrder=%s', + sortOrder => { + const result = validateDrillDownMetricsSchema({ sortOrder }); + expect(result.sortOrder).toBe(sortOrder); + }, + ); + + it('should transform ownedByMe="true" to boolean true', () => { + const result = validateDrillDownMetricsSchema({ ownedByMe: 'true' }); + expect(result.ownedByMe).toBe(true); + }); + + it('should transform ownedByMe="false" to boolean false', () => { + const result = validateDrillDownMetricsSchema({ ownedByMe: 'false' }); + expect(result.ownedByMe).toBe(false); + }); + + it('should accept a valid owner string', () => { + const result = validateDrillDownMetricsSchema({ + owner: 'team:default/platform', + }); + expect(result.owner).toBe('team:default/platform'); + }); + + it('should accept a valid kind string', () => { + const result = validateDrillDownMetricsSchema({ kind: 'Component' }); + expect(result.kind).toBe('Component'); + }); + + it('should accept a valid entityName string', () => { + const result = validateDrillDownMetricsSchema({ + entityName: 'my-service', + }); + expect(result.entityName).toBe('my-service'); + }); + + it('should accept all valid parameters together', () => { + const result = validateDrillDownMetricsSchema({ + page: '2', + pageSize: '10', + status: 'error', + sortBy: 'metricValue', + sortOrder: 'asc', + ownedByMe: 'true', + owner: 'team:default/backend', + kind: 'Component', + entityName: 'my-service', + }); + + expect(result).toEqual({ + page: 2, + pageSize: 10, + status: 'error', + sortBy: 'metricValue', + sortOrder: 'asc', + ownedByMe: true, + owner: 'team:default/backend', + kind: 'Component', + entityName: 'my-service', + }); + }); + + it('should strip unknown properties', () => { + const result = validateDrillDownMetricsSchema({ unknownProp: 'value' }); + expect(result).not.toHaveProperty('unknownProp'); + }); + }); + + describe('invalid query parameters', () => { + it('should throw InputError when page is 0', () => { + expect(() => validateDrillDownMetricsSchema({ page: '0' })).toThrow( + InputError, + ); + }); + + it('should throw InputError when page is negative', () => { + expect(() => validateDrillDownMetricsSchema({ page: '-1' })).toThrow( + InputError, + ); + }); + + it('should throw InputError when page exceeds 10000', () => { + expect(() => validateDrillDownMetricsSchema({ page: '10001' })).toThrow( + InputError, + ); + }); + + it('should throw InputError when pageSize is 0', () => { + expect(() => validateDrillDownMetricsSchema({ pageSize: '0' })).toThrow( + InputError, + ); + }); + + it('should throw InputError when pageSize exceeds 100', () => { + expect(() => validateDrillDownMetricsSchema({ pageSize: '101' })).toThrow( + InputError, + ); + }); + + it.each([ + { field: 'status', value: 'unknown' }, + { field: 'sortBy', value: 'invalid' }, + { field: 'sortOrder', value: 'random' }, + { field: 'ownedByMe', value: 'yes' }, + ])( + 'should throw InputError when $field has invalid value "$value"', + ({ field, value }) => { + expect(() => + validateDrillDownMetricsSchema({ [field]: value }), + ).toThrow(InputError); + expect(() => + validateDrillDownMetricsSchema({ [field]: value }), + ).toThrow('Invalid query parameters'); + }, + ); + + it.each(['owner', 'kind', 'entityName'])( + 'should throw InputError when %s is an empty string', + field => { + expect(() => validateDrillDownMetricsSchema({ [field]: '' })).toThrow( + InputError, + ); + expect(() => validateDrillDownMetricsSchema({ [field]: '' })).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 index 4c56d83bd4..a7a45afd6f 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.ts @@ -32,3 +32,30 @@ export function validateCatalogMetricsSchema(query: unknown): { return parsed.data; } + +export function validateDrillDownMetricsSchema(query: unknown) { + const drillDownSchema = z.object({ + page: z.coerce.number().int().min(1).max(10000).optional().default(1), + pageSize: z.coerce.number().int().min(1).max(100).optional().default(5), + status: z.enum(['success', 'warning', 'error']).optional(), + sortBy: z + .enum(['entityName', 'owner', 'entityKind', 'timestamp', 'metricValue']) + .optional(), + sortOrder: z.enum(['asc', 'desc']).optional().default('desc'), + ownedByMe: z + .enum(['true', 'false']) + .transform(v => v === 'true') + .optional(), + owner: z.string().min(1).max(255).optional(), + kind: z.string().min(1).max(100).optional(), + entityName: z.string().min(1).max(255).optional(), + }); + + const parsed = drillDownSchema.safeParse(query); + + if (!parsed.success) { + throw new InputError(`Invalid query parameters: ${parsed.error.message}`); + } + + return parsed.data; +} From d530fc48ae4e3964c15e390ae77a975592438034 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 19:43:47 -0500 Subject: [PATCH 12/17] feat(scorecard): remove getAuthorizedEntityRefs as we are utilizing user credentials during catalog fetch Signed-off-by: Patrick Knight --- .../src/database/DatabaseMetricValues.test.ts | 44 +++++ .../src/database/DatabaseMetricValues.ts | 25 ++- .../src/permissions/permissionUtils.test.ts | 154 ------------------ .../src/permissions/permissionUtils.ts | 29 ---- .../src/service/CatalogMetricService.test.ts | 106 ++++++++---- .../src/service/CatalogMetricService.ts | 50 +++--- .../src/service/router.test.ts | 64 +++----- .../scorecard-backend/src/service/router.ts | 12 +- 8 files changed, 191 insertions(+), 293 deletions(-) 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 224f1d54c8..ecd01cf94d 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts @@ -817,6 +817,50 @@ describe('DatabaseMetricValues', () => { }, ); + it.each(databases.eachSupportedId())( + 'should return all matching rows when entity refs is null (unscoped path) - %p', + async databaseId => { + const { client, db } = await createDatabase(databaseId); + + const timestamp = new Date('2023-01-01T00:00:00Z'); + + await client('metric_values').insert([ + { + catalog_entity_ref: 'component:default/service1', + metric_id: 'github.metric1', + value: 10, + timestamp, + status: 'success', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + { + catalog_entity_ref: 'component:default/service2', + metric_id: 'github.metric1', + value: 5, + timestamp, + status: 'warning', + entity_kind: 'Component', + entity_owner: 'team:default/backend', + }, + ]); + + // null means unscoped — all rows for the metric are returned. + // Per-row authorization is enforced downstream by catalog.getEntitiesByRefs. + const result = await db.readEntityMetricsByStatus( + null, + 'github.metric1', + undefined, + undefined, + undefined, + { limit: 10, offset: 0 }, + ); + + expect(result.rows).toHaveLength(2); + expect(result.total).toBe(2); + }, + ); + it.each(databases.eachSupportedId())( 'should return empty result when entity refs is empty, not bypass to fetch all - %p', async databaseId => { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts index e46c1cc23c..6a71418708 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts @@ -135,29 +135,40 @@ export class DatabaseMetricValues { * Returns both the paginated rows and total count for pagination */ async readEntityMetricsByStatus( - catalog_entity_refs: string[], + catalog_entity_refs: string[] | null, metric_id: string, status?: 'success' | 'warning' | 'error', entityKind?: string, entityOwner?: string, pagination?: { limit: number; offset: number }, ): Promise<{ rows: DbMetricValue[]; total: number }> { - if (catalog_entity_refs.length === 0) { + if ( + Array.isArray(catalog_entity_refs) && + catalog_entity_refs.length === 0 + ) { return { rows: [], total: 0 }; } + // Build subquery — only scope to refs when provided const latestIdsSubquery = this.dbClient(this.tableName) .max('id') - .where('metric_id', metric_id) - .whereIn('catalog_entity_ref', catalog_entity_refs) - .groupBy('metric_id', 'catalog_entity_ref'); + .where('metric_id', metric_id); + + if (catalog_entity_refs !== null) { + latestIdsSubquery.whereIn('catalog_entity_ref', catalog_entity_refs); + } + + latestIdsSubquery.groupBy('metric_id', 'catalog_entity_ref'); const query = this.dbClient(this.tableName) .select('*') .select(this.dbClient.raw('COUNT(*) OVER() as total_count')) .whereIn('id', latestIdsSubquery) - .where('metric_id', metric_id) - .whereIn('catalog_entity_ref', catalog_entity_refs); + .where('metric_id', metric_id); + + if (catalog_entity_refs !== null) { + query.whereIn('catalog_entity_ref', catalog_entity_refs); + } query.orderBy('timestamp', 'desc'); 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 094d0d8294..1dde25f617 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.test.ts @@ -24,7 +24,6 @@ import { Request } from 'express'; import { NotAllowedError } from '@backstage/errors'; import { catalogEntityReadPermission } from '@backstage/plugin-catalog-common/alpha'; import type { - BackstageCredentials, HttpAuthService, PermissionsService, } from '@backstage/backend-plugin-api'; @@ -33,10 +32,8 @@ import { filterAuthorizedMetrics, matches, checkEntityAccess, - getAuthorizedEntityRefs, } from './permissionUtils'; import { mockServices } from '@backstage/backend-test-utils'; -import { catalogServiceMock } from '@backstage/plugin-catalog-node/testUtils'; const createMockMetric = ( id: string, @@ -231,155 +228,4 @@ describe('permissionUtils', () => { ); }); }); - - describe('getAuthorizedEntityRefs', () => { - let mockedCatalog: ReturnType; - let mockCredentials: BackstageCredentials; - - beforeEach(() => { - mockedCatalog = catalogServiceMock.mock(); - mockCredentials = {} as BackstageCredentials; - - mockedCatalog.queryEntities.mockResolvedValue({ - items: [ - { - apiVersion: 'backstage.io/v1alpha1', - kind: 'Component', - metadata: { name: 'my-service', namespace: 'default' }, - }, - ], - pageInfo: { nextCursor: undefined }, - totalItems: 1, - }); - }); - - afterEach(() => { - jest.clearAllMocks(); - }); - - it('should return entity refs for entities the user is authorized to see', async () => { - const result = await getAuthorizedEntityRefs({ - catalog: mockedCatalog, - credentials: mockCredentials, - }); - - expect(result).toEqual(['component:default/my-service']); - }); - - it('should call queryEntities with user credentials', async () => { - await getAuthorizedEntityRefs({ - catalog: mockedCatalog, - credentials: mockCredentials, - }); - - expect(mockedCatalog.queryEntities).toHaveBeenCalledWith( - expect.any(Object), - { credentials: mockCredentials }, - ); - }); - - it('should call queryEntities with correct fields and batch size limit', async () => { - await getAuthorizedEntityRefs({ - catalog: mockedCatalog, - credentials: mockCredentials, - }); - - expect(mockedCatalog.queryEntities).toHaveBeenCalledWith( - { - fields: ['kind', 'metadata.name', 'metadata.namespace'], - limit: 50, - }, - { credentials: mockCredentials }, - ); - }); - - it('should return empty array when catalog has no entities', async () => { - mockedCatalog.queryEntities.mockResolvedValue({ - items: [], - pageInfo: { nextCursor: undefined }, - totalItems: 0, - }); - - const result = await getAuthorizedEntityRefs({ - catalog: mockedCatalog, - credentials: mockCredentials, - }); - - expect(result).toEqual([]); - }); - - it('should handle cursor-based pagination across multiple pages', async () => { - const entity1 = { - apiVersion: 'backstage.io/v1alpha1', - kind: 'Component', - metadata: { name: 'service-1', namespace: 'default' }, - }; - const entity2 = { - apiVersion: 'backstage.io/v1alpha1', - kind: 'Component', - metadata: { name: 'service-2', namespace: 'default' }, - }; - - mockedCatalog.queryEntities - .mockResolvedValueOnce({ - items: [entity1], - pageInfo: { nextCursor: 'cursor1' }, - totalItems: 2, - }) - .mockResolvedValueOnce({ - items: [entity2], - pageInfo: { nextCursor: undefined }, - totalItems: 2, - }); - - const result = await getAuthorizedEntityRefs({ - catalog: mockedCatalog, - credentials: mockCredentials, - }); - - expect(result).toEqual([ - 'component:default/service-1', - 'component:default/service-2', - ]); - expect(mockedCatalog.queryEntities).toHaveBeenCalledTimes(2); - expect(mockedCatalog.queryEntities).toHaveBeenNthCalledWith( - 2, - { - fields: ['kind', 'metadata.name', 'metadata.namespace'], - limit: 50, - cursor: 'cursor1', - }, - { credentials: mockCredentials }, - ); - }); - - it('should return entity refs for multiple entity kinds', async () => { - mockedCatalog.queryEntities.mockResolvedValue({ - items: [ - { - apiVersion: 'backstage.io/v1alpha1', - kind: 'Component', - metadata: { name: 'my-service', namespace: 'default' }, - }, - { - apiVersion: 'backstage.io/v1alpha1', - kind: 'API', - metadata: { name: 'my-api', namespace: 'default' }, - }, - ], - pageInfo: { nextCursor: undefined }, - totalItems: 2, - }); - - const result = await getAuthorizedEntityRefs({ - catalog: mockedCatalog, - credentials: mockCredentials, - }); - - expect(result).toEqual([ - 'component:default/my-service', - 'api:default/my-api', - ]); - }); - }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.ts b/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.ts index ce3e60c292..4bd81ab8aa 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.ts @@ -24,7 +24,6 @@ import { Request } from 'express'; import { NotAllowedError } from '@backstage/errors'; import { catalogEntityReadPermission } from '@backstage/plugin-catalog-common/alpha'; import type { - BackstageCredentials, HttpAuthService, PermissionsService, } from '@backstage/backend-plugin-api'; @@ -32,10 +31,6 @@ import type { import { Metric } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { rules as scorecardRules } from './rules'; -import { CatalogService } from '@backstage/plugin-catalog-node'; -import { stringifyEntityRef } from '@backstage/catalog-model'; - -const QUERY_ENTITIES_BATCH_SIZE = 50; export const checkEntityAccess = async ( entityRef: string, @@ -53,30 +48,6 @@ export const checkEntityAccess = async ( } }; -export async function getAuthorizedEntityRefs(options: { - catalog: CatalogService; - credentials: BackstageCredentials; -}): Promise { - const entityRefs: string[] = []; - let cursor: string | undefined = undefined; - - do { - const result = await options.catalog.queryEntities( - { - fields: ['kind', 'metadata.name', 'metadata.namespace'], - limit: QUERY_ENTITIES_BATCH_SIZE, - ...(cursor ? { cursor } : {}), - }, - { credentials: options.credentials }, // user credentials — enforces conditional policies - ); - - cursor = result.pageInfo.nextCursor; - entityRefs.push(...result.items.map(e => stringifyEntityRef(e))); - } while (cursor !== undefined); - - return entityRefs; -} - export const matches = ( metric: Metric, filters?: PermissionCriteria< 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 0860047e1e..b7cd7e4122 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts @@ -591,7 +591,7 @@ describe('CatalogMetricService', () => { it('should fetch entity metrics with default options', async () => { const result = await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -612,7 +612,7 @@ describe('CatalogMetricService', () => { it('should enrich entities with catalog metadata', async () => { const result = await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -634,7 +634,7 @@ describe('CatalogMetricService', () => { it('should call database with correct pagination parameters', async () => { await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -644,7 +644,7 @@ describe('CatalogMetricService', () => { ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - [], + null, 'github.important_metric', undefined, undefined, @@ -655,7 +655,7 @@ describe('CatalogMetricService', () => { it('should filter by status at database level', async () => { await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -666,7 +666,7 @@ describe('CatalogMetricService', () => { ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - [], + null, 'github.important_metric', 'error', undefined, @@ -677,7 +677,7 @@ describe('CatalogMetricService', () => { it('should filter by kind at database level', async () => { await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -688,7 +688,7 @@ describe('CatalogMetricService', () => { ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - [], + null, 'github.important_metric', undefined, 'Component', @@ -699,7 +699,7 @@ describe('CatalogMetricService', () => { it('should filter by owner at database level', async () => { await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -710,7 +710,7 @@ describe('CatalogMetricService', () => { ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - [], + null, 'github.important_metric', undefined, undefined, @@ -721,7 +721,7 @@ describe('CatalogMetricService', () => { it('should filter by entityName at application level', async () => { const result = await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -733,7 +733,7 @@ describe('CatalogMetricService', () => { // Should fetch all from DB (no pagination) expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - [], + null, 'github.important_metric', undefined, undefined, @@ -749,7 +749,7 @@ describe('CatalogMetricService', () => { it('should perform case-insensitive entityName search', async () => { const result = await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -764,7 +764,7 @@ describe('CatalogMetricService', () => { it('should sort by entityName ascending', async () => { const result = await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -782,7 +782,7 @@ describe('CatalogMetricService', () => { it('should sort by metricValue descending', async () => { const result = await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -800,7 +800,7 @@ describe('CatalogMetricService', () => { it('should sort by timestamp descending by default', async () => { const result = await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -822,7 +822,7 @@ describe('CatalogMetricService', () => { }); const result = await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -840,7 +840,7 @@ describe('CatalogMetricService', () => { it('should batch-fetch entities using getEntitiesByRefs', async () => { await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -862,13 +862,14 @@ describe('CatalogMetricService', () => { ); }); - it('should handle missing catalog entities gracefully', async () => { + it('should exclude entities that the catalog returns null for (unauthorized)', async () => { + // service-b (index 1) returns undefined/null — catalog enforces no access mockedCatalog.getEntitiesByRefs.mockResolvedValue({ items: [mockEntities.items[0], undefined, mockEntities.items[2]], }); const result = await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -877,18 +878,22 @@ describe('CatalogMetricService', () => { }, ); - expect(result.entities[1].entityName).toBe('Unknown'); - expect(result.entities[1].entityKind).toBe('Component'); - expect(result.entities[1].owner).toBe('team:default/backend'); + // service-b is filtered out; only the two authorized entities are returned + expect(result.entities).toHaveLength(2); + expect(result.entities.map(e => e.entityRef)).not.toContain( + 'component:default/service-b', + ); + expect(result.entities[0].entityRef).toBe('component:default/service-a'); + expect(result.entities[1].entityRef).toBe('component:default/service-c'); }); - it('should handle catalog API failures gracefully', async () => { + it('should handle catalog API failures gracefully by falling back to DB metadata', async () => { mockedCatalog.getEntitiesByRefs.mockRejectedValue( new Error('Catalog API error'), ); const result = await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -902,9 +907,12 @@ describe('CatalogMetricService', () => { expect.objectContaining({ error: expect.any(Error) }), ); - // Should still return results with fallback data + // When catalog is unavailable, all DB rows are returned with fallback metadata + // so a transient outage doesn't silently hide all results from the user. expect(result.entities).toHaveLength(3); expect(result.entities[0].entityName).toBe('Unknown'); + expect(result.entities[0].entityKind).toBe('Component'); // from DB row.entity_kind + expect(result.entities[0].owner).toBe('team:default/platform'); // from DB row.entity_owner }); it('should pass entity refs to database query', async () => { @@ -925,6 +933,44 @@ describe('CatalogMetricService', () => { ); }); + it('should pass null to database for unscoped query (avoids catalog enumeration)', async () => { + await service.getEntityMetricDetails( + null, + 'github.important_metric', + mockCredentials, + { page: 1, limit: 10 }, + ); + + expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( + null, + 'github.important_metric', + undefined, + undefined, + undefined, + { limit: 10, offset: 0 }, + ); + }); + + it('should use catalog.getEntitiesByRefs as the sole authorization gate for the unscoped path', async () => { + // Simulate catalog returning null for service-b (no access) and real entities for others + mockedCatalog.getEntitiesByRefs.mockResolvedValue({ + items: [mockEntities.items[0], undefined, mockEntities.items[2]], + }); + + const result = await service.getEntityMetricDetails( + null, + 'github.important_metric', + mockCredentials, + { page: 1, limit: 10 }, + ); + + // service-b should be filtered out because catalog returned null (unauthorized) + expect(result.entities).toHaveLength(2); + expect(result.entities.map(e => e.entityRef)).not.toContain( + 'component:default/service-b', + ); + }); + it('should combine filters, sorting, and pagination', async () => { mockedDatabase.readEntityMetricsByStatus.mockResolvedValue({ rows: [mockMetricRows[0]], @@ -932,7 +978,7 @@ describe('CatalogMetricService', () => { }); const result = await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -947,7 +993,7 @@ describe('CatalogMetricService', () => { ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - [], + null, 'github.important_metric', 'error', 'Component', @@ -966,7 +1012,7 @@ describe('CatalogMetricService', () => { }); const result = await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { @@ -986,7 +1032,7 @@ describe('CatalogMetricService', () => { it('should include metric metadata in response', async () => { const result = await service.getEntityMetricDetails( - [], + null, 'github.important_metric', mockCredentials, { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts index 5f3ae8b0bb..f560bbd52e 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -186,7 +186,8 @@ export class CatalogMetricService { * Supports database-level filtering (status, owner, kind) and application-level * filtering (entityName). Falls back to database values if catalog is unavailable. * - * @param entityRefs - Array of entity references to scope the query. Empty array fetches all entities. + * @param entityRefs - Entity refs to scope the DB query, or null for unscoped. + * Empty array signals no authorized entities (returns empty). * @param metricId - Metric ID to fetch (e.g., "github.open_prs") * @param options - Query options for filtering, sorting, and pagination * @param options.status - Filter by threshold status (database-level) @@ -200,7 +201,7 @@ export class CatalogMetricService { * @returns Paginated entity metric details with metadata */ async getEntityMetricDetails( - entityRefs: string[], + entityRefs: string[] | null, metricId: string, credentials: BackstageCredentials, options: { @@ -245,22 +246,24 @@ export class CatalogMetricService { // Get metric metadata const metric = this.registry.getMetric(metricId); - // Batch-fetch entities from catalog + // Batch-fetch entities from catalog using user credentials. + // The catalog enforces catalog.entity.read permissions — entities the user + // cannot access are returned as null in response.items. const entityRefsToFetch = rows.map(row => row.catalog_entity_ref); const entityMap = new Map(); + let catalogAvailable = true; if (entityRefsToFetch.length > 0) { try { const response = await this.catalog.getEntitiesByRefs( { entityRefs: entityRefsToFetch, - fields: ['kind', 'metadata', 'spec'], // Only fetch needed fields + fields: ['kind', 'metadata', 'spec'], }, { credentials }, ); - // Build map of ref -> entity - // response.items is in same order as entityRefsToFetch + // Build map of ref -> entity (null entries = unauthorized, not added to map) entityRefsToFetch.forEach((ref, index) => { const entity = response.items[index]; if (entity) { @@ -268,25 +271,30 @@ export class CatalogMetricService { } }); } catch (error) { - // Log error but continue with empty map (entities will show as "Unknown") + // Catalog is unavailable — fall back to DB-only metadata rather than + // returning empty results, so a transient outage doesn't silently hide data. + catalogAvailable = false; this.logger.warn('Failed to fetch entities from catalog', { error }); } } - // Enrich rows with catalog data - const enrichedEntities: EntityMetricDetail[] = rows.map(row => { - const entity = entityMap.get(row.catalog_entity_ref); - - return { - entityRef: row.catalog_entity_ref, - entityName: entity?.metadata?.name ?? 'Unknown', - entityKind: entity?.kind ?? row.entity_kind ?? 'Unknown', - owner: (entity?.spec?.owner as string) ?? row.entity_owner ?? 'Unknown', - metricValue: row.value, - timestamp: new Date(row.timestamp).toISOString(), - status: row.status!, - }; - }); + // When catalog is available: filter to only authorized entities (null response = no access). + // When catalog is unavailable: include all rows with fallback DB metadata for resilience. + const enrichedEntities: EntityMetricDetail[] = rows + .filter(row => !catalogAvailable || entityMap.has(row.catalog_entity_ref)) + .map(row => { + const entity = entityMap.get(row.catalog_entity_ref); + return { + entityRef: row.catalog_entity_ref, + entityName: entity?.metadata?.name ?? 'Unknown', + entityKind: entity?.kind ?? row.entity_kind ?? 'Unknown', + owner: + (entity?.spec?.owner as string) ?? row.entity_owner ?? 'Unknown', + metricValue: row.value, + timestamp: new Date(row.timestamp).toISOString(), + status: row.status!, + }; + }); // Apply application-level filters let filteredEntities = enrichedEntities; 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 3fa1eaa37c..0bd8aa63f1 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -55,7 +55,6 @@ jest.mock('../permissions/permissionUtils', () => { return { ...originalModule, checkEntityAccess: jest.fn(), - getAuthorizedEntityRefs: jest.fn(), }; }); @@ -734,11 +733,6 @@ describe('createRouter', () => { }); describe('GET /metrics/:metricId/catalog/aggregations/entities', () => { - const AUTHORIZED_ENTITY_REFS = [ - 'component:default/my-service', - 'component:default/another-service', - ]; - const mockEntityMetricDetailResponse = { metricId: 'github.open_prs', metricMetadata: { @@ -778,7 +772,6 @@ describe('createRouter', () => { let getEntityMetricDetailsSpy: jest.SpyInstance; let getEntitiesOwnedByUserSpy: jest.SpyInstance; let checkEntityAccessSpy: jest.SpyInstance; - let getAuthorizedEntityRefsSpy: jest.SpyInstance; let mockCredentials: BackstageCredentials; beforeEach(async () => { @@ -812,10 +805,6 @@ describe('createRouter', () => { 'checkEntityAccess', ); - getAuthorizedEntityRefsSpy = jest - .spyOn(permissionUtilsModule, 'getAuthorizedEntityRefs') - .mockResolvedValue(AUTHORIZED_ENTITY_REFS); - const mockCatalog = catalogServiceMock.mock(); const router = await createRouter({ metricProvidersRegistry, @@ -847,7 +836,7 @@ describe('createRouter', () => { expect(response.status).toBe(200); expect(response.body).toEqual(mockEntityMetricDetailResponse); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - AUTHORIZED_ENTITY_REFS, + null, 'github.open_prs', mockCredentials, { @@ -869,7 +858,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - AUTHORIZED_ENTITY_REFS, + null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -897,7 +886,7 @@ describe('createRouter', () => { expect(response.status).toBe(200); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - AUTHORIZED_ENTITY_REFS, + null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -912,7 +901,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - AUTHORIZED_ENTITY_REFS, + null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -927,7 +916,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - AUTHORIZED_ENTITY_REFS, + null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -942,7 +931,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - AUTHORIZED_ENTITY_REFS, + null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -957,7 +946,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - AUTHORIZED_ENTITY_REFS, + null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -1013,7 +1002,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - AUTHORIZED_ENTITY_REFS, + null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -1029,7 +1018,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - AUTHORIZED_ENTITY_REFS, + null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -1045,7 +1034,7 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - AUTHORIZED_ENTITY_REFS, + null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -1127,26 +1116,11 @@ describe('createRouter', () => { expect(response.body.pagination.total).toBe(0); }); - it('should call getAuthorizedEntityRefs when ownedByMe is not set', async () => { - await request(drillDownApp).get( - '/metrics/github.open_prs/catalog/aggregations/entities', - ); - - expect(getAuthorizedEntityRefsSpy).toHaveBeenCalledTimes(1); - expect(getAuthorizedEntityRefsSpy).toHaveBeenCalledWith( - expect.objectContaining({ - catalog: expect.any(Object), - credentials: expect.any(Object), - }), - ); - }); - - it('should call getAuthorizedEntityRefs when ownedByMe=false', async () => { + it('should not call getEntitiesOwnedByUser when ownedByMe=false', async () => { await request(drillDownApp).get( '/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=false', ); - expect(getAuthorizedEntityRefsSpy).toHaveBeenCalledTimes(1); expect(getEntitiesOwnedByUserSpy).not.toHaveBeenCalled(); }); @@ -1166,29 +1140,31 @@ describe('createRouter', () => { expect(checkEntityAccessSpy).not.toHaveBeenCalled(); }); - it('should pass authorized entity refs from getAuthorizedEntityRefs to getEntityMetricDetails', async () => { - const restrictedRefs = ['component:default/allowed-service']; - getAuthorizedEntityRefsSpy.mockResolvedValueOnce(restrictedRefs); - + it('should pass null as entity scope for unscoped path (avoids catalog enumeration)', async () => { await request(drillDownApp).get( '/metrics/github.open_prs/catalog/aggregations/entities', ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - restrictedRefs, + null, 'github.open_prs', mockCredentials, expect.any(Object), ); }); - it('should not call getAuthorizedEntityRefs when ownedByMe=true', async () => { + it('should call getEntitiesOwnedByUser and pass its result to getEntityMetricDetails when ownedByMe=true', async () => { await request(drillDownApp).get( '/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=true', ); - expect(getAuthorizedEntityRefsSpy).not.toHaveBeenCalled(); expect(getEntitiesOwnedByUserSpy).toHaveBeenCalledTimes(1); + expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( + ['component:default/my-service', 'component:default/another-service'], + 'github.open_prs', + mockCredentials, + expect.any(Object), + ); }); describe('input validation', () => { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index 744a6b9c38..ca2e82fcb7 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -37,7 +37,6 @@ import { scorecardMetricReadPermission } from '@red-hat-developer-hub/backstage- import { filterAuthorizedMetrics, checkEntityAccess, - getAuthorizedEntityRefs, } from '../permissions/permissionUtils'; import { stringifyEntityRef } from '@backstage/catalog-model'; import { @@ -235,7 +234,7 @@ export async function createRouter({ } // Determine entity scope based on filters - let entityRefsToQuery: string[]; + let entityRefsToQuery: string[] | null; if (ownedByMe) { // Use getEntitiesOwnedByUser scoping @@ -249,12 +248,9 @@ export async function createRouter({ await checkEntityAccess(entityRef, req, permissions, httpAuth); } } else { - // Scope to entities the user is authorized to read from the catalog. - // Passing user credentials means catalog enforces conditional policies natively. - entityRefsToQuery = await getAuthorizedEntityRefs({ - catalog, - credentials, - }); + // Unscoped: DB returns the page, getEntitiesByRefs with user credentials + // in getEntityMetricDetails enforces catalog read permissions per-row. + entityRefsToQuery = null; } const entityMetrics = await catalogMetricService.getEntityMetricDetails( From 71ac5b08184f679cf2c8217c14f223885655cf33 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 20:20:42 -0500 Subject: [PATCH 13/17] feat(scorecard): remove ownedByMe in favor of passing owner array as a param Signed-off-by: Patrick Knight --- .../scorecard-backend/docs/drill-down.md | 62 +++-- .../src/database/DatabaseMetricValues.test.ts | 93 ++------ .../src/database/DatabaseMetricValues.ts | 30 +-- .../src/service/CatalogMetricService.test.ts | 53 +---- .../src/service/CatalogMetricService.ts | 6 +- .../src/service/router.test.ts | 129 +--------- .../scorecard-backend/src/service/router.ts | 31 +-- .../validateCatalogMetricsSchema.test.ts | 184 +-------------- .../validateCatalogMetricsSchema.ts | 27 --- .../validateDrillDownMetricsSchema.test.ts | 220 ++++++++++++++++++ .../validateDrillDownMetricsSchema.ts | 44 ++++ 11 files changed, 336 insertions(+), 543 deletions(-) create mode 100644 workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.test.ts create mode 100644 workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.ts diff --git a/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md b/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md index 1e7061fcc3..dd304b524b 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md +++ b/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md @@ -28,21 +28,20 @@ Returns a paginated list of entities with their metric values, enriched with cat #### Query Parameters -| Parameter | Type | Required | Default | Description | -| ------------ | ------- | -------- | ----------- | -------------------------------------------------------------------------------------------- | -| `status` | string | No | - | Filter by threshold status: `success`, `warning`, or `error` | -| `ownedByMe` | boolean | No | `false` | If `true`, only show entities owned by the authenticated user and their direct parent groups | -| `owner` | string | No | - | Filter by specific owner entity ref (e.g., `team:default/platform`) | -| `kind` | string | No | - | Filter by entity kind (e.g., `Component`, `API`, `System`) | -| `entityName` | string | No | - | Search for entities with names containing this string (case-insensitive) | -| `sortBy` | string | No | `timestamp` | Sort by: `entityName`, `owner`, `entityKind`, `timestamp`, or `metricValue` | -| `sortOrder` | string | No | `desc` | Sort direction: `asc` or `desc` | -| `page` | number | No | `1` | Page number (1-indexed) | -| `pageSize` | number | No | `5` | Number of entities per page (max: 100) | +| Parameter | Type | Required | Default | Description | +| ------------ | ---------------- | -------- | ----------- | --------------------------------------------------------------------------------------- | +| `status` | string | No | - | Filter by threshold status: `success`, `warning`, or `error` | +| `owner` | string/string\[] | No | - | Filter by owner entity ref. Repeat to supply multiple values (e.g., `?owner=a&owner=b`) | +| `kind` | string | No | - | Filter by entity kind (e.g., `Component`, `API`, `System`) | +| `entityName` | string | No | - | Search for entities with names containing this string (case-insensitive) | +| `sortBy` | string | No | `timestamp` | Sort by: `entityName`, `owner`, `entityKind`, `timestamp`, or `metricValue` | +| `sortOrder` | string | No | `desc` | Sort direction: `asc` or `desc` | +| `page` | number | No | `1` | Page number (1-indexed) | +| `pageSize` | number | No | `5` | Number of entities per page (max: 100) | #### Authentication -Requires user authentication. The endpoint uses the authenticated user's entity reference when `ownedByMe=true` is specified. +Requires user authentication. #### Permissions @@ -103,17 +102,17 @@ curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/ ### Filter by Ownership -Get only entities owned by the authenticated user: +Get entities owned by a specific team: ```bash -curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=true&status=error" \ +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?owner=team:default/platform&page=1&pageSize=10" \ -H "Authorization: Bearer " ``` -Get entities owned by a specific team: +Get entities owned by multiple teams (repeat the `owner` parameter): ```bash -curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?owner=team:default/platform&page=1&pageSize=10" \ +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?owner=team:default/platform&owner=team:default/backend&page=1&pageSize=10" \ -H "Authorization: Bearer " ``` @@ -153,10 +152,10 @@ curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/ ### Combining Filters -Get my Component entities with errors, sorted by metric value: +Get Component entities with errors for a specific team, sorted by metric value: ```bash -curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=true&status=error&kind=Component&sortBy=metricValue&sortOrder=desc&page=1&pageSize=10" \ +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?owner=team:default/platform&status=error&kind=Component&sortBy=metricValue&sortOrder=desc&page=1&pageSize=10" \ -H "Authorization: Bearer " ``` @@ -201,14 +200,6 @@ curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/ ## Filtering Behavior -### Entity Ownership Scoping - -The `ownedByMe` parameter controls which entities are included in the results: - -**Default (`ownedByMe` not specified or `false`)**: Returns all entities in the system that have metric values for the specified metric, subject to the user's catalog read permissions. - -**`ownedByMe=true`**: Returns only entities owned by the authenticated user and their direct parent groups. This uses the same ownership logic as the aggregation endpoint (see [Entity Aggregation](./aggregation.md) for details on direct parent groups vs transitive ownership). - ### Status Filtering When `status` is specified, only entities with that threshold evaluation are returned: @@ -221,7 +212,7 @@ Status filtering is performed at the database level for optimal performance. ### Owner Filtering -The `owner` parameter filters entities by their catalog owner (`spec.owner`): +The `owner` parameter filters entities by their catalog owner (`spec.owner`). Repeat the parameter to match any of several owners (up to 50): ```bash # Get entities owned by a specific team @@ -229,9 +220,12 @@ The `owner` parameter filters entities by their catalog owner (`spec.owner`): # Get entities owned by a specific user ?owner=user:default/alice + +# Get entities owned by either of two teams +?owner=team:default/platform&owner=team:default/backend ``` -This filter is applied at the database level and works independently of `ownedByMe`. +This filter is applied at the database level for optimal performance. Frontends can implement "owned by me" scoping by passing the user's `identityApi.ownershipEntityRefs` (user ref + direct group refs) as repeated `owner` values. ### Kind Filtering @@ -412,21 +406,17 @@ This returns API entities with security warnings, helping prioritize security im ### Use Case 4: Personal Dashboard -An engineer wants to see only their owned entities with issues: +An engineer wants to see only their owned entities with issues. The frontend passes the user's `ownershipEntityRefs` (user ref + group memberships) as repeated `owner` params: ```bash -curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=true&page=1&pageSize=10" \ +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?owner=user:default/alice&owner=team:default/platform&page=1&pageSize=10" \ -H "Authorization: Bearer " ``` -This returns a personalized view of entities they're responsible for. +This returns a personalized view scoped to the entities the engineer and their teams are responsible for. ## Limitations -### Direct Parent Groups Only (when using `ownedByMe=true`) - -Similar to the aggregation endpoint, `ownedByMe=true` only includes entities owned by direct parent groups, not nested hierarchies. See [Entity Aggregation](./aggregation.md) for details on enabling transitive parent groups. - ### Entity Metadata Freshness Entity metadata (name, kind, owner) is fetched from the catalog at request time and reflects the current state. However, metric values and timestamps represent historical data from the last sync. This means: @@ -474,7 +464,7 @@ Entity metadata (name, kind, owner) is fetched from the catalog at request time - Use more specific filters (status, kind, owner) - Reduce page size - Avoid `entityName` search on large datasets -- Consider using `ownedByMe=true` to scope results +- Use the `owner` filter to scope results to specific teams ## Related Documentation 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 ecd01cf94d..666c08dfb8 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts @@ -309,12 +309,6 @@ describe('DatabaseMetricValues', () => { ]); const result = await db.readEntityMetricsByStatus( - [ - 'component:default/service1', - 'component:default/service2', - 'component:default/service3', - 'component:default/service4', - ], 'github.metric1', 'error', undefined, @@ -370,11 +364,6 @@ describe('DatabaseMetricValues', () => { ]); const result = await db.readEntityMetricsByStatus( - [ - 'component:default/service1', - 'component:default/service2', - 'component:default/service3', - ], 'github.metric1', undefined, undefined, @@ -435,13 +424,6 @@ describe('DatabaseMetricValues', () => { // Page 1: limit 2 const page1 = await db.readEntityMetricsByStatus( - [ - 'component:default/service1', - 'component:default/service2', - 'component:default/service3', - 'component:default/service4', - 'component:default/service5', - ], 'github.metric1', 'error', undefined, @@ -454,13 +436,6 @@ describe('DatabaseMetricValues', () => { // Page 2: limit 2, offset 2 const page2 = await db.readEntityMetricsByStatus( - [ - 'component:default/service1', - 'component:default/service2', - 'component:default/service3', - 'component:default/service4', - 'component:default/service5', - ], 'github.metric1', 'error', undefined, @@ -473,13 +448,6 @@ describe('DatabaseMetricValues', () => { // Page 3: limit 2, offset 4 const page3 = await db.readEntityMetricsByStatus( - [ - 'component:default/service1', - 'component:default/service2', - 'component:default/service3', - 'component:default/service4', - 'component:default/service5', - ], 'github.metric1', 'error', undefined, @@ -493,12 +461,11 @@ describe('DatabaseMetricValues', () => { ); it.each(databases.eachSupportedId())( - 'should return empty result for no matching entity refs - %p', + 'should return empty result when database has no rows for the metric - %p', async databaseId => { const { db } = await createDatabase(databaseId); const result = await db.readEntityMetricsByStatus( - [], 'github.metric1', 'error', undefined, @@ -550,11 +517,6 @@ describe('DatabaseMetricValues', () => { ]); const result = await db.readEntityMetricsByStatus( - [ - 'component:default/service1', - 'api:default/api1', - 'component:default/service2', - ], 'github.metric1', 'error', 'Component', // Filter by kind @@ -609,15 +571,10 @@ describe('DatabaseMetricValues', () => { ]); const result = await db.readEntityMetricsByStatus( - [ - 'component:default/service1', - 'component:default/service2', - 'component:default/service3', - ], 'github.metric1', 'error', undefined, - 'team:default/platform', // Filter by owner + ['team:default/platform'], // Filter by owner { limit: 10, offset: 0 }, ); @@ -677,16 +634,10 @@ describe('DatabaseMetricValues', () => { ]); const result = await db.readEntityMetricsByStatus( - [ - 'component:default/service1', - 'api:default/api1', - 'component:default/service2', - 'component:default/service3', - ], 'github.metric1', 'error', // Only error status 'Component', // Only Component kind - 'team:default/platform', // Only platform team + ['team:default/platform'], // Only platform team { limit: 10, offset: 0 }, ); @@ -741,11 +692,6 @@ describe('DatabaseMetricValues', () => { // No pagination parameter - should return all const result = await db.readEntityMetricsByStatus( - [ - 'component:default/service1', - 'component:default/service2', - 'component:default/service3', - ], 'github.metric1', 'error', undefined, @@ -789,7 +735,6 @@ describe('DatabaseMetricValues', () => { // Should return both when no filters const result = await db.readEntityMetricsByStatus( - ['component:default/service1', 'component:default/service2'], 'github.metric1', 'error', undefined, @@ -802,7 +747,6 @@ describe('DatabaseMetricValues', () => { // Should only return service2 when filtering by kind const filteredResult = await db.readEntityMetricsByStatus( - ['component:default/service1', 'component:default/service2'], 'github.metric1', 'error', 'Component', @@ -818,7 +762,7 @@ describe('DatabaseMetricValues', () => { ); it.each(databases.eachSupportedId())( - 'should return all matching rows when entity refs is null (unscoped path) - %p', + 'should return all rows when no owner filter is applied - %p', async databaseId => { const { client, db } = await createDatabase(databaseId); @@ -845,10 +789,9 @@ describe('DatabaseMetricValues', () => { }, ]); - // null means unscoped — all rows for the metric are returned. + // No owner filter — all rows for the metric are returned. // Per-row authorization is enforced downstream by catalog.getEntitiesByRefs. const result = await db.readEntityMetricsByStatus( - null, 'github.metric1', undefined, undefined, @@ -862,13 +805,12 @@ describe('DatabaseMetricValues', () => { ); it.each(databases.eachSupportedId())( - 'should return empty result when entity refs is empty, not bypass to fetch all - %p', + 'should filter by multiple owner refs - %p', async databaseId => { const { client, db } = await createDatabase(databaseId); const timestamp = new Date('2023-01-01T00:00:00Z'); - // Insert entities that would previously be returned when passing an empty array await client('metric_values').insert([ { catalog_entity_ref: 'component:default/service1', @@ -888,21 +830,32 @@ describe('DatabaseMetricValues', () => { entity_kind: 'Component', entity_owner: 'team:default/backend', }, + { + catalog_entity_ref: 'component:default/service3', + metric_id: 'github.metric1', + value: 8, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/other', + }, ]); - // Empty array must return no results — an empty scope means no authorized entities, - // not an unscoped "fetch all" that would bypass catalog read permissions. + // Passing two owners returns only those two teams' entities. const result = await db.readEntityMetricsByStatus( - [], 'github.metric1', 'error', undefined, - undefined, + ['team:default/platform', 'team:default/backend'], { limit: 10, offset: 0 }, ); - expect(result.rows).toHaveLength(0); - expect(result.total).toBe(0); + expect(result.rows).toHaveLength(2); + expect(result.total).toBe(2); + expect(result.rows.map(r => r.entity_owner).sort()).toEqual([ + 'team:default/backend', + 'team:default/platform', + ]); }, ); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts index 6a71418708..0d06b41c4f 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts @@ -135,30 +135,16 @@ export class DatabaseMetricValues { * Returns both the paginated rows and total count for pagination */ async readEntityMetricsByStatus( - catalog_entity_refs: string[] | null, metric_id: string, status?: 'success' | 'warning' | 'error', entityKind?: string, - entityOwner?: string, + entityOwner?: string[], pagination?: { limit: number; offset: number }, ): Promise<{ rows: DbMetricValue[]; total: number }> { - if ( - Array.isArray(catalog_entity_refs) && - catalog_entity_refs.length === 0 - ) { - return { rows: [], total: 0 }; - } - - // Build subquery — only scope to refs when provided const latestIdsSubquery = this.dbClient(this.tableName) .max('id') - .where('metric_id', metric_id); - - if (catalog_entity_refs !== null) { - latestIdsSubquery.whereIn('catalog_entity_ref', catalog_entity_refs); - } - - latestIdsSubquery.groupBy('metric_id', 'catalog_entity_ref'); + .where('metric_id', metric_id) + .groupBy('metric_id', 'catalog_entity_ref'); const query = this.dbClient(this.tableName) .select('*') @@ -166,24 +152,18 @@ export class DatabaseMetricValues { .whereIn('id', latestIdsSubquery) .where('metric_id', metric_id); - if (catalog_entity_refs !== null) { - query.whereIn('catalog_entity_ref', catalog_entity_refs); - } - query.orderBy('timestamp', 'desc'); if (status) { query.where('status', status); } - // Filter by entity_kind if (entityKind) { query.whereRaw('LOWER(entity_kind) = LOWER(?)', [entityKind]); } - // Filter by entity_owner - if (entityOwner) { - query.whereRaw('LOWER(entity_owner) = LOWER(?)', [entityOwner]); + if (entityOwner && entityOwner.length > 0) { + query.whereIn('entity_owner', entityOwner); } if (pagination) { 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 b7cd7e4122..259c60ded5 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts @@ -591,7 +591,6 @@ describe('CatalogMetricService', () => { it('should fetch entity metrics with default options', async () => { const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -612,7 +611,6 @@ describe('CatalogMetricService', () => { it('should enrich entities with catalog metadata', async () => { const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -634,7 +632,6 @@ describe('CatalogMetricService', () => { it('should call database with correct pagination parameters', async () => { await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -644,7 +641,6 @@ describe('CatalogMetricService', () => { ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - null, 'github.important_metric', undefined, undefined, @@ -655,7 +651,6 @@ describe('CatalogMetricService', () => { it('should filter by status at database level', async () => { await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -666,7 +661,6 @@ describe('CatalogMetricService', () => { ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - null, 'github.important_metric', 'error', undefined, @@ -677,7 +671,6 @@ describe('CatalogMetricService', () => { it('should filter by kind at database level', async () => { await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -688,7 +681,6 @@ describe('CatalogMetricService', () => { ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - null, 'github.important_metric', undefined, 'Component', @@ -699,29 +691,26 @@ describe('CatalogMetricService', () => { it('should filter by owner at database level', async () => { await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { - owner: 'team:default/platform', + owner: ['team:default/platform'], page: 1, limit: 10, }, ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - null, 'github.important_metric', undefined, undefined, - 'team:default/platform', + ['team:default/platform'], { limit: 10, offset: 0 }, ); }); it('should filter by entityName at application level', async () => { const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -733,7 +722,6 @@ describe('CatalogMetricService', () => { // Should fetch all from DB (no pagination) expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - null, 'github.important_metric', undefined, undefined, @@ -749,7 +737,6 @@ describe('CatalogMetricService', () => { it('should perform case-insensitive entityName search', async () => { const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -764,7 +751,6 @@ describe('CatalogMetricService', () => { it('should sort by entityName ascending', async () => { const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -782,7 +768,6 @@ describe('CatalogMetricService', () => { it('should sort by metricValue descending', async () => { const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -800,7 +785,6 @@ describe('CatalogMetricService', () => { it('should sort by timestamp descending by default', async () => { const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -822,7 +806,6 @@ describe('CatalogMetricService', () => { }); const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -840,7 +823,6 @@ describe('CatalogMetricService', () => { it('should batch-fetch entities using getEntitiesByRefs', async () => { await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -869,7 +851,6 @@ describe('CatalogMetricService', () => { }); const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -893,7 +874,6 @@ describe('CatalogMetricService', () => { ); const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -915,34 +895,14 @@ describe('CatalogMetricService', () => { expect(result.entities[0].owner).toBe('team:default/platform'); // from DB row.entity_owner }); - it('should pass entity refs to database query', async () => { - await service.getEntityMetricDetails( - ['component:default/service-a', 'component:default/service-b'], - 'github.important_metric', - mockCredentials, - { page: 1, limit: 10 }, - ); - - expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - ['component:default/service-a', 'component:default/service-b'], - 'github.important_metric', - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, - ); - }); - it('should pass null to database for unscoped query (avoids catalog enumeration)', async () => { await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { page: 1, limit: 10 }, ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - null, 'github.important_metric', undefined, undefined, @@ -958,7 +918,6 @@ describe('CatalogMetricService', () => { }); const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { page: 1, limit: 10 }, @@ -978,13 +937,12 @@ describe('CatalogMetricService', () => { }); const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { status: 'error', kind: 'Component', - owner: 'team:default/platform', + owner: ['team:default/platform'], sortBy: 'metricValue', sortOrder: 'desc', page: 1, @@ -993,11 +951,10 @@ describe('CatalogMetricService', () => { ); expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( - null, 'github.important_metric', 'error', 'Component', - 'team:default/platform', + ['team:default/platform'], { limit: 5, offset: 0 }, ); @@ -1012,7 +969,6 @@ describe('CatalogMetricService', () => { }); const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { @@ -1032,7 +988,6 @@ describe('CatalogMetricService', () => { it('should include metric metadata in response', async () => { const result = await service.getEntityMetricDetails( - null, 'github.important_metric', mockCredentials, { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts index f560bbd52e..e9578b03cd 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -186,8 +186,6 @@ export class CatalogMetricService { * Supports database-level filtering (status, owner, kind) and application-level * filtering (entityName). Falls back to database values if catalog is unavailable. * - * @param entityRefs - Entity refs to scope the DB query, or null for unscoped. - * Empty array signals no authorized entities (returns empty). * @param metricId - Metric ID to fetch (e.g., "github.open_prs") * @param options - Query options for filtering, sorting, and pagination * @param options.status - Filter by threshold status (database-level) @@ -201,12 +199,11 @@ export class CatalogMetricService { * @returns Paginated entity metric details with metadata */ async getEntityMetricDetails( - entityRefs: string[] | null, metricId: string, credentials: BackstageCredentials, options: { status?: 'success' | 'warning' | 'error'; - owner?: string; + owner?: string[]; kind?: string; entityName?: string; sortBy?: @@ -235,7 +232,6 @@ export class CatalogMetricService { // Fetch raw metric data from database const { rows, total: dbTotal } = await this.database.readEntityMetricsByStatus( - entityRefs, metricId, options.status, options.kind, 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 0bd8aa63f1..450e8f4bc7 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -770,8 +770,6 @@ describe('createRouter', () => { let drillDownApp: express.Express; let getEntityMetricDetailsSpy: jest.SpyInstance; - let getEntitiesOwnedByUserSpy: jest.SpyInstance; - let checkEntityAccessSpy: jest.SpyInstance; let mockCredentials: BackstageCredentials; beforeEach(async () => { @@ -793,18 +791,6 @@ describe('createRouter', () => { .spyOn(catalogMetricService, 'getEntityMetricDetails') .mockResolvedValue(mockEntityMetricDetailResponse as any); - getEntitiesOwnedByUserSpy = jest - .spyOn(getEntitiesOwnedByUserModule, 'getEntitiesOwnedByUser') - .mockResolvedValue([ - 'component:default/my-service', - 'component:default/another-service', - ]); - - checkEntityAccessSpy = jest.spyOn( - permissionUtilsModule, - 'checkEntityAccess', - ); - const mockCatalog = catalogServiceMock.mock(); const router = await createRouter({ metricProvidersRegistry, @@ -836,7 +822,6 @@ describe('createRouter', () => { expect(response.status).toBe(200); expect(response.body).toEqual(mockEntityMetricDetailResponse); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - null, 'github.open_prs', mockCredentials, { @@ -858,7 +843,6 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -886,7 +870,6 @@ describe('createRouter', () => { expect(response.status).toBe(200); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -901,7 +884,6 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -916,11 +898,10 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - null, 'github.open_prs', mockCredentials, expect.objectContaining({ - owner: 'team:default/platform', + owner: ['team:default/platform'], }), ); }); @@ -931,7 +912,6 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -946,7 +926,6 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -955,54 +934,12 @@ describe('createRouter', () => { ); }); - it('should handle ownedByMe=true and call getEntitiesOwnedByUser', async () => { - await request(drillDownApp).get( - '/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=true', - ); - - expect(getEntitiesOwnedByUserSpy).toHaveBeenCalledWith( - 'user:default/test-user', - expect.objectContaining({ - catalog: expect.any(Object), - credentials: expect.any(Object), - }), - ); - - expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - ['component:default/my-service', 'component:default/another-service'], - 'github.open_prs', - mockCredentials, - expect.any(Object), - ); - }); - - it('should check entity access when ownedByMe=true', async () => { - await request(drillDownApp).get( - '/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=true', - ); - - expect(checkEntityAccessSpy).toHaveBeenCalledTimes(2); - expect(checkEntityAccessSpy).toHaveBeenCalledWith( - 'component:default/my-service', - expect.any(Object), - permissionsMock, - httpAuthMock, - ); - expect(checkEntityAccessSpy).toHaveBeenCalledWith( - 'component:default/another-service', - expect.any(Object), - permissionsMock, - httpAuthMock, - ); - }); - it('should sort by entityName ascending', async () => { await request(drillDownApp).get( '/metrics/github.open_prs/catalog/aggregations/entities?sortBy=entityName&sortOrder=asc', ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -1018,7 +955,6 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - null, 'github.open_prs', mockCredentials, expect.objectContaining({ @@ -1034,13 +970,12 @@ describe('createRouter', () => { ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - null, 'github.open_prs', mockCredentials, expect.objectContaining({ status: 'error', kind: 'Component', - owner: 'team:default/platform', + owner: ['team:default/platform'], sortBy: 'metricValue', sortOrder: 'desc', }), @@ -1116,54 +1051,17 @@ describe('createRouter', () => { expect(response.body.pagination.total).toBe(0); }); - it('should not call getEntitiesOwnedByUser when ownedByMe=false', async () => { - await request(drillDownApp).get( - '/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=false', - ); - - expect(getEntitiesOwnedByUserSpy).not.toHaveBeenCalled(); - }); - - it('should not call getEntitiesOwnedByUser when ownedByMe is not set', async () => { - await request(drillDownApp).get( - '/metrics/github.open_prs/catalog/aggregations/entities', - ); - - expect(getEntitiesOwnedByUserSpy).not.toHaveBeenCalled(); - }); - - it('should not call checkEntityAccess when ownedByMe is not set', async () => { - await request(drillDownApp).get( - '/metrics/github.open_prs/catalog/aggregations/entities', - ); - - expect(checkEntityAccessSpy).not.toHaveBeenCalled(); - }); - - it('should pass null as entity scope for unscoped path (avoids catalog enumeration)', async () => { + it('should normalize multi-value owner params to an array', async () => { await request(drillDownApp).get( - '/metrics/github.open_prs/catalog/aggregations/entities', + '/metrics/github.open_prs/catalog/aggregations/entities?owner=team:default/platform&owner=user:default/alice', ); expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - null, 'github.open_prs', mockCredentials, - expect.any(Object), - ); - }); - - it('should call getEntitiesOwnedByUser and pass its result to getEntityMetricDetails when ownedByMe=true', async () => { - await request(drillDownApp).get( - '/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=true', - ); - - expect(getEntitiesOwnedByUserSpy).toHaveBeenCalledTimes(1); - expect(getEntityMetricDetailsSpy).toHaveBeenCalledWith( - ['component:default/my-service', 'component:default/another-service'], - 'github.open_prs', - mockCredentials, - expect.any(Object), + expect.objectContaining({ + owner: ['team:default/platform', 'user:default/alice'], + }), ); }); @@ -1259,19 +1157,6 @@ describe('createRouter', () => { expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); }); - it('should return 400 when ownedByMe is an invalid value', async () => { - const response = await request(drillDownApp).get( - '/metrics/github.open_prs/catalog/aggregations/entities?ownedByMe=yes', - ); - - expect(response.status).toBe(400); - expect(response.body.error.name).toBe('InputError'); - expect(response.body.error.message).toContain( - 'Invalid query parameters', - ); - expect(getEntityMetricDetailsSpy).not.toHaveBeenCalled(); - }); - it('should return 400 when owner is an empty string', async () => { const response = await request(drillDownApp).get( '/metrics/github.open_prs/catalog/aggregations/entities?owner=', diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index ca2e82fcb7..9aa2d3bd58 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -39,14 +39,12 @@ import { checkEntityAccess, } from '../permissions/permissionUtils'; import { stringifyEntityRef } from '@backstage/catalog-model'; -import { - validateCatalogMetricsSchema, - validateDrillDownMetricsSchema, -} from '../validation/validateCatalogMetricsSchema'; +import { validateCatalogMetricsSchema } from '../validation/validateCatalogMetricsSchema'; import { getEntitiesOwnedByUser } from '../utils/getEntitiesOwnedByUser'; import { parseCommaSeparatedString } from '../utils/parseCommaSeparatedString'; import { validateMetricsSchema } from '../validation/validateMetricsSchema'; import { AggregatedMetricMapper } from './mappers'; +import { validateDrillDownMetricsSchema } from '../validation/validateDrillDownMetricsSchema'; export type ScorecardRouterOptions = { metricProvidersRegistry: MetricProvidersRegistry; @@ -204,7 +202,6 @@ export async function createRouter({ page, pageSize, status, - ownedByMe, owner, kind, entityName, @@ -233,28 +230,10 @@ export async function createRouter({ throw new AuthenticationError('User entity reference not found'); } - // Determine entity scope based on filters - let entityRefsToQuery: string[] | null; - - if (ownedByMe) { - // Use getEntitiesOwnedByUser scoping - entityRefsToQuery = await getEntitiesOwnedByUser(userEntityRef, { - catalog, - credentials, - }); - - // Check entity access for owned entities - for (const entityRef of entityRefsToQuery) { - await checkEntityAccess(entityRef, req, permissions, httpAuth); - } - } else { - // Unscoped: DB returns the page, getEntitiesByRefs with user credentials - // in getEntityMetricDetails enforces catalog read permissions per-row. - entityRefsToQuery = null; - } - + // Per-row authorization is enforced by catalog.getEntitiesByRefs in the service. + // For "owned by me" scoping, the frontend passes identityApi.ownershipEntityRefs + // as repeated ?owner= params. const entityMetrics = await catalogMetricService.getEntityMetricDetails( - entityRefsToQuery, metricId, credentials, { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts index ac4e0f5ab0..331fc49ad2 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts @@ -14,10 +14,7 @@ * limitations under the License. */ -import { - validateCatalogMetricsSchema, - validateDrillDownMetricsSchema, -} from './validateCatalogMetricsSchema'; +import { validateCatalogMetricsSchema } from './validateCatalogMetricsSchema'; import { InputError } from '@backstage/errors'; describe('validateCatalogMetricsSchema', () => { @@ -87,182 +84,3 @@ describe('validateCatalogMetricsSchema', () => { ); }); }); - -describe('validateDrillDownMetricsSchema', () => { - describe('valid query parameters', () => { - it('should return defaults when given an empty object', () => { - expect(validateDrillDownMetricsSchema({})).toEqual({ - page: 1, - pageSize: 5, - sortOrder: 'desc', - }); - }); - - it('should coerce page and pageSize from strings to numbers', () => { - const result = validateDrillDownMetricsSchema({ - page: '3', - pageSize: '20', - }); - expect(result.page).toBe(3); - expect(result.pageSize).toBe(20); - }); - - it('should accept page at max boundary of 10000', () => { - const result = validateDrillDownMetricsSchema({ page: '10000' }); - expect(result.page).toBe(10000); - }); - - it('should accept pageSize at max boundary of 100', () => { - const result = validateDrillDownMetricsSchema({ pageSize: '100' }); - expect(result.pageSize).toBe(100); - }); - - it.each(['success', 'warning', 'error'] as const)( - 'should accept status=%s', - status => { - const result = validateDrillDownMetricsSchema({ status }); - expect(result.status).toBe(status); - }, - ); - - it.each([ - 'entityName', - 'owner', - 'entityKind', - 'timestamp', - 'metricValue', - ] as const)('should accept sortBy=%s', sortBy => { - const result = validateDrillDownMetricsSchema({ sortBy }); - expect(result.sortBy).toBe(sortBy); - }); - - it.each(['asc', 'desc'] as const)( - 'should accept sortOrder=%s', - sortOrder => { - const result = validateDrillDownMetricsSchema({ sortOrder }); - expect(result.sortOrder).toBe(sortOrder); - }, - ); - - it('should transform ownedByMe="true" to boolean true', () => { - const result = validateDrillDownMetricsSchema({ ownedByMe: 'true' }); - expect(result.ownedByMe).toBe(true); - }); - - it('should transform ownedByMe="false" to boolean false', () => { - const result = validateDrillDownMetricsSchema({ ownedByMe: 'false' }); - expect(result.ownedByMe).toBe(false); - }); - - it('should accept a valid owner string', () => { - const result = validateDrillDownMetricsSchema({ - owner: 'team:default/platform', - }); - expect(result.owner).toBe('team:default/platform'); - }); - - it('should accept a valid kind string', () => { - const result = validateDrillDownMetricsSchema({ kind: 'Component' }); - expect(result.kind).toBe('Component'); - }); - - it('should accept a valid entityName string', () => { - const result = validateDrillDownMetricsSchema({ - entityName: 'my-service', - }); - expect(result.entityName).toBe('my-service'); - }); - - it('should accept all valid parameters together', () => { - const result = validateDrillDownMetricsSchema({ - page: '2', - pageSize: '10', - status: 'error', - sortBy: 'metricValue', - sortOrder: 'asc', - ownedByMe: 'true', - owner: 'team:default/backend', - kind: 'Component', - entityName: 'my-service', - }); - - expect(result).toEqual({ - page: 2, - pageSize: 10, - status: 'error', - sortBy: 'metricValue', - sortOrder: 'asc', - ownedByMe: true, - owner: 'team:default/backend', - kind: 'Component', - entityName: 'my-service', - }); - }); - - it('should strip unknown properties', () => { - const result = validateDrillDownMetricsSchema({ unknownProp: 'value' }); - expect(result).not.toHaveProperty('unknownProp'); - }); - }); - - describe('invalid query parameters', () => { - it('should throw InputError when page is 0', () => { - expect(() => validateDrillDownMetricsSchema({ page: '0' })).toThrow( - InputError, - ); - }); - - it('should throw InputError when page is negative', () => { - expect(() => validateDrillDownMetricsSchema({ page: '-1' })).toThrow( - InputError, - ); - }); - - it('should throw InputError when page exceeds 10000', () => { - expect(() => validateDrillDownMetricsSchema({ page: '10001' })).toThrow( - InputError, - ); - }); - - it('should throw InputError when pageSize is 0', () => { - expect(() => validateDrillDownMetricsSchema({ pageSize: '0' })).toThrow( - InputError, - ); - }); - - it('should throw InputError when pageSize exceeds 100', () => { - expect(() => validateDrillDownMetricsSchema({ pageSize: '101' })).toThrow( - InputError, - ); - }); - - it.each([ - { field: 'status', value: 'unknown' }, - { field: 'sortBy', value: 'invalid' }, - { field: 'sortOrder', value: 'random' }, - { field: 'ownedByMe', value: 'yes' }, - ])( - 'should throw InputError when $field has invalid value "$value"', - ({ field, value }) => { - expect(() => - validateDrillDownMetricsSchema({ [field]: value }), - ).toThrow(InputError); - expect(() => - validateDrillDownMetricsSchema({ [field]: value }), - ).toThrow('Invalid query parameters'); - }, - ); - - it.each(['owner', 'kind', 'entityName'])( - 'should throw InputError when %s is an empty string', - field => { - expect(() => validateDrillDownMetricsSchema({ [field]: '' })).toThrow( - InputError, - ); - expect(() => validateDrillDownMetricsSchema({ [field]: '' })).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 index a7a45afd6f..4c56d83bd4 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.ts @@ -32,30 +32,3 @@ export function validateCatalogMetricsSchema(query: unknown): { return parsed.data; } - -export function validateDrillDownMetricsSchema(query: unknown) { - const drillDownSchema = z.object({ - page: z.coerce.number().int().min(1).max(10000).optional().default(1), - pageSize: z.coerce.number().int().min(1).max(100).optional().default(5), - status: z.enum(['success', 'warning', 'error']).optional(), - sortBy: z - .enum(['entityName', 'owner', 'entityKind', 'timestamp', 'metricValue']) - .optional(), - sortOrder: z.enum(['asc', 'desc']).optional().default('desc'), - ownedByMe: z - .enum(['true', 'false']) - .transform(v => v === 'true') - .optional(), - owner: z.string().min(1).max(255).optional(), - kind: z.string().min(1).max(100).optional(), - entityName: z.string().min(1).max(255).optional(), - }); - - const parsed = drillDownSchema.safeParse(query); - - if (!parsed.success) { - throw new InputError(`Invalid query parameters: ${parsed.error.message}`); - } - - return parsed.data; -} diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.test.ts new file mode 100644 index 0000000000..5fa1cbffd4 --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.test.ts @@ -0,0 +1,220 @@ +/* + * 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 { InputError } from '@backstage/errors'; +import { validateDrillDownMetricsSchema } from './validateDrillDownMetricsSchema'; + +describe('validateDrillDownMetricsSchema', () => { + describe('valid query parameters', () => { + it('should return defaults when given an empty object', () => { + expect(validateDrillDownMetricsSchema({})).toEqual({ + page: 1, + pageSize: 5, + sortOrder: 'desc', + }); + }); + + it('should coerce page and pageSize from strings to numbers', () => { + const result = validateDrillDownMetricsSchema({ + page: '3', + pageSize: '20', + }); + expect(result.page).toBe(3); + expect(result.pageSize).toBe(20); + }); + + it('should accept page at max boundary of 10000', () => { + const result = validateDrillDownMetricsSchema({ page: '10000' }); + expect(result.page).toBe(10000); + }); + + it('should accept pageSize at max boundary of 100', () => { + const result = validateDrillDownMetricsSchema({ pageSize: '100' }); + expect(result.pageSize).toBe(100); + }); + + it.each(['success', 'warning', 'error'] as const)( + 'should accept status=%s', + status => { + const result = validateDrillDownMetricsSchema({ status }); + expect(result.status).toBe(status); + }, + ); + + it.each([ + 'entityName', + 'owner', + 'entityKind', + 'timestamp', + 'metricValue', + ] as const)('should accept sortBy=%s', sortBy => { + const result = validateDrillDownMetricsSchema({ sortBy }); + expect(result.sortBy).toBe(sortBy); + }); + + it.each(['asc', 'desc'] as const)( + 'should accept sortOrder=%s', + sortOrder => { + const result = validateDrillDownMetricsSchema({ sortOrder }); + expect(result.sortOrder).toBe(sortOrder); + }, + ); + + it('should normalize a single owner string to an array', () => { + const result = validateDrillDownMetricsSchema({ + owner: 'team:default/platform', + }); + expect(result.owner).toEqual(['team:default/platform']); + }); + + it('should accept an array of owner strings', () => { + const result = validateDrillDownMetricsSchema({ + owner: ['team:default/platform', 'user:default/alice'], + }); + expect(result.owner).toEqual([ + 'team:default/platform', + 'user:default/alice', + ]); + }); + + it('should return undefined when owner is not provided', () => { + const result = validateDrillDownMetricsSchema({}); + expect(result.owner).toBeUndefined(); + }); + + it('should accept a valid kind string', () => { + const result = validateDrillDownMetricsSchema({ kind: 'Component' }); + expect(result.kind).toBe('Component'); + }); + + it('should accept a valid entityName string', () => { + const result = validateDrillDownMetricsSchema({ + entityName: 'my-service', + }); + expect(result.entityName).toBe('my-service'); + }); + + it('should accept all valid parameters together', () => { + const result = validateDrillDownMetricsSchema({ + page: '2', + pageSize: '10', + status: 'error', + sortBy: 'metricValue', + sortOrder: 'asc', + owner: 'team:default/backend', + kind: 'Component', + entityName: 'my-service', + }); + + expect(result).toEqual({ + page: 2, + pageSize: 10, + status: 'error', + sortBy: 'metricValue', + sortOrder: 'asc', + owner: ['team:default/backend'], + kind: 'Component', + entityName: 'my-service', + }); + }); + + it('should strip unknown properties', () => { + const result = validateDrillDownMetricsSchema({ unknownProp: 'value' }); + expect(result).not.toHaveProperty('unknownProp'); + }); + }); + + describe('invalid query parameters', () => { + it('should throw InputError when page is 0', () => { + expect(() => validateDrillDownMetricsSchema({ page: '0' })).toThrow( + InputError, + ); + }); + + it('should throw InputError when page is negative', () => { + expect(() => validateDrillDownMetricsSchema({ page: '-1' })).toThrow( + InputError, + ); + }); + + it('should throw InputError when page exceeds 10000', () => { + expect(() => validateDrillDownMetricsSchema({ page: '10001' })).toThrow( + InputError, + ); + }); + + it('should throw InputError when pageSize is 0', () => { + expect(() => validateDrillDownMetricsSchema({ pageSize: '0' })).toThrow( + InputError, + ); + }); + + it('should throw InputError when pageSize exceeds 100', () => { + expect(() => validateDrillDownMetricsSchema({ pageSize: '101' })).toThrow( + InputError, + ); + }); + + it('should throw InputError when more than 50 owner values are provided', () => { + const tooManyOwners = Array.from( + { length: 51 }, + (_, i) => `team:default/team-${i}`, + ); + expect(() => + validateDrillDownMetricsSchema({ owner: tooManyOwners }), + ).toThrow(InputError); + expect(() => + validateDrillDownMetricsSchema({ owner: tooManyOwners }), + ).toThrow('Invalid query parameters'); + }); + + it.each([ + { field: 'status', value: 'unknown' }, + { field: 'sortBy', value: 'invalid' }, + { field: 'sortOrder', value: 'random' }, + ])( + 'should throw InputError when $field has invalid value "$value"', + ({ field, value }) => { + expect(() => + validateDrillDownMetricsSchema({ [field]: value }), + ).toThrow(InputError); + expect(() => + validateDrillDownMetricsSchema({ [field]: value }), + ).toThrow('Invalid query parameters'); + }, + ); + + it.each(['kind', 'entityName'])( + 'should throw InputError when %s is an empty string', + field => { + expect(() => validateDrillDownMetricsSchema({ [field]: '' })).toThrow( + InputError, + ); + expect(() => validateDrillDownMetricsSchema({ [field]: '' })).toThrow( + 'Invalid query parameters', + ); + }, + ); + + it('should throw InputError when owner contains an empty string', () => { + expect(() => validateDrillDownMetricsSchema({ owner: [''] })).toThrow( + InputError, + ); + expect(() => validateDrillDownMetricsSchema({ owner: [''] })).toThrow( + 'Invalid query parameters', + ); + }); + }); +}); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.ts b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.ts new file mode 100644 index 0000000000..1340295c02 --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.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 { z } from 'zod'; +import { InputError } from '@backstage/errors'; + +export function validateDrillDownMetricsSchema(query: unknown) { + const drillDownSchema = z.object({ + page: z.coerce.number().int().min(1).max(10000).optional().default(1), + pageSize: z.coerce.number().int().min(1).max(100).optional().default(5), + status: z.enum(['success', 'warning', 'error']).optional(), + sortBy: z + .enum(['entityName', 'owner', 'entityKind', 'timestamp', 'metricValue']) + .optional(), + sortOrder: z.enum(['asc', 'desc']).optional().default('desc'), + owner: z.preprocess(val => { + if (val === undefined) return val; + if (Array.isArray(val)) return val; + return [val]; + }, z.array(z.string().min(1).max(255)).max(50).optional()), + kind: z.string().min(1).max(100).optional(), + entityName: z.string().min(1).max(255).optional(), + }); + + const parsed = drillDownSchema.safeParse(query); + + if (!parsed.success) { + throw new InputError(`Invalid query parameters: ${parsed.error.message}`); + } + + return parsed.data; +} From 8083f2ff79d235d751a017fd864a53177b424fc4 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 21:01:15 -0500 Subject: [PATCH 14/17] feat(scorecard): move entityName filtering to the database Signed-off-by: Patrick Knight --- .../scorecard-backend/docs/drill-down.md | 46 ++++++------ .../src/database/DatabaseMetricValues.test.ts | 70 +++++++++++++++++++ .../src/database/DatabaseMetricValues.ts | 7 ++ .../src/service/CatalogMetricService.test.ts | 31 ++++++-- .../src/service/CatalogMetricService.ts | 49 +++---------- 5 files changed, 134 insertions(+), 69 deletions(-) diff --git a/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md b/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md index dd304b524b..86103ebd68 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md +++ b/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md @@ -7,7 +7,7 @@ The Scorecard plugin provides a drill-down endpoint that returns detailed entity The drill-down endpoint (`/metrics/:metricId/catalog/aggregations/entities`) provides a detailed view of entities and their metric values. It allows managers and platform engineers to: - See individual entities contributing to aggregated scores -- Filter entities by status (success/warning/error), owner, kind, or name +- Filter entities by status (success/warning/error), owner, kind, or entity ref substring - Sort by any column (entity name, owner, kind, timestamp, metric value) - Paginate through large result sets - Understand data freshness through per-entity timestamps @@ -28,16 +28,16 @@ Returns a paginated list of entities with their metric values, enriched with cat #### Query Parameters -| Parameter | Type | Required | Default | Description | -| ------------ | ---------------- | -------- | ----------- | --------------------------------------------------------------------------------------- | -| `status` | string | No | - | Filter by threshold status: `success`, `warning`, or `error` | -| `owner` | string/string\[] | No | - | Filter by owner entity ref. Repeat to supply multiple values (e.g., `?owner=a&owner=b`) | -| `kind` | string | No | - | Filter by entity kind (e.g., `Component`, `API`, `System`) | -| `entityName` | string | No | - | Search for entities with names containing this string (case-insensitive) | -| `sortBy` | string | No | `timestamp` | Sort by: `entityName`, `owner`, `entityKind`, `timestamp`, or `metricValue` | -| `sortOrder` | string | No | `desc` | Sort direction: `asc` or `desc` | -| `page` | number | No | `1` | Page number (1-indexed) | -| `pageSize` | number | No | `5` | Number of entities per page (max: 100) | +| Parameter | Type | Required | Default | Description | +| ------------ | ---------------- | -------- | ----------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `status` | string | No | - | Filter by threshold status: `success`, `warning`, or `error` | +| `owner` | string/string\[] | No | - | Filter by owner entity ref. Repeat to supply multiple values (e.g., `?owner=a&owner=b`) | +| `kind` | string | No | - | Filter by entity kind (e.g., `Component`, `API`, `System`) | +| `entityName` | string | No | - | Substring search against the entity ref (`kind:namespace/name`). Matches any part of the ref (case-insensitive). Use the name portion for simple searches (e.g., `auth` matches `component:default/auth-service`) | +| `sortBy` | string | No | `timestamp` | Sort by: `entityName`, `owner`, `entityKind`, `timestamp`, or `metricValue` | +| `sortOrder` | string | No | `desc` | Sort direction: `asc` or `desc` | +| `page` | number | No | `1` | Page number (1-indexed) | +| `pageSize` | number | No | `5` | Number of entities per page (max: 100) | #### Authentication @@ -246,17 +246,22 @@ Kind filtering is performed at the database level for optimal performance. ### Entity Name Search -The `entityName` parameter performs a case-insensitive substring search on entity names: +The `entityName` parameter performs a case-insensitive substring search against the full entity reference, which has the format `kind:namespace/name` (e.g., `component:default/auth-service`). ```bash -# Find entities with "auth" in the name +# Match by name fragment — matches component:default/auth-service, api:default/auth-api, etc. ?entityName=auth -# Find entities with "service" in the name +# Match by name fragment — matches component:default/my-service, component:default/service-api, etc. ?entityName=service + +# Match more precisely using the full ref format +?entityName=component:default/auth-service ``` -Entity name filtering requires catalog metadata and is performed at the application level after enrichment. +Because the search runs against the entire ref string, searching by just the name portion (the part after `/`) is the most common and natural usage. Be aware that the search term could also match the kind or namespace prefix if those happen to contain the search string (e.g., `?entityName=default` would match all entities in the `default` namespace). + +Entity name filtering is performed at the database level for consistent pagination and accurate total counts. ## Sorting @@ -303,10 +308,9 @@ The response includes pagination metadata: ### Pagination Performance -- **Database-level pagination**: Used when only `status`, `owner`, or `kind` filters are applied (optimal performance) -- **Application-level pagination**: Used when `entityName` search is applied (fetches all results, then filters and paginates in memory) +All filters (`status`, `owner`, `kind`, and `entityName`) are applied at the database level before pagination. The `LIMIT`/`OFFSET` is always pushed to the database, so only the requested page of rows is fetched regardless of which filters are active. -For best performance with large datasets, use database-level filters when possible. +For best performance with large datasets, combine specific filters to reduce the result set size before paginating. ## Error Handling @@ -456,14 +460,12 @@ Entity metadata (name, kind, owner) is fetched from the catalog at request time **Possible causes**: 1. **Large result set**: Too many entities match the filters -2. **Entity name search**: Using `entityName` filter fetches all entities before filtering -3. **No filters applied**: Returning all entities in the system +2. **No filters applied**: Returning all entities in the system **Resolution**: -- Use more specific filters (status, kind, owner) +- Use more specific filters (status, kind, owner, entityName) - Reduce page size -- Avoid `entityName` search on large datasets - Use the `owner` filter to scope results to specific teams ## Related Documentation 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 666c08dfb8..fa506ab607 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts @@ -313,6 +313,7 @@ describe('DatabaseMetricValues', () => { 'error', undefined, undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -368,6 +369,7 @@ describe('DatabaseMetricValues', () => { undefined, undefined, undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -428,6 +430,7 @@ describe('DatabaseMetricValues', () => { 'error', undefined, undefined, + undefined, { limit: 2, offset: 0 }, ); @@ -440,6 +443,7 @@ describe('DatabaseMetricValues', () => { 'error', undefined, undefined, + undefined, { limit: 2, offset: 2 }, ); @@ -452,6 +456,7 @@ describe('DatabaseMetricValues', () => { 'error', undefined, undefined, + undefined, { limit: 2, offset: 4 }, ); @@ -470,6 +475,7 @@ describe('DatabaseMetricValues', () => { 'error', undefined, undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -519,6 +525,7 @@ describe('DatabaseMetricValues', () => { const result = await db.readEntityMetricsByStatus( 'github.metric1', 'error', + undefined, 'Component', // Filter by kind undefined, { limit: 10, offset: 0 }, @@ -574,6 +581,7 @@ describe('DatabaseMetricValues', () => { 'github.metric1', 'error', undefined, + undefined, ['team:default/platform'], // Filter by owner { limit: 10, offset: 0 }, ); @@ -636,6 +644,7 @@ describe('DatabaseMetricValues', () => { const result = await db.readEntityMetricsByStatus( 'github.metric1', 'error', // Only error status + undefined, 'Component', // Only Component kind ['team:default/platform'], // Only platform team { limit: 10, offset: 0 }, @@ -696,6 +705,7 @@ describe('DatabaseMetricValues', () => { 'error', undefined, undefined, + undefined, undefined, // No pagination ); @@ -739,6 +749,7 @@ describe('DatabaseMetricValues', () => { 'error', undefined, undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -749,6 +760,7 @@ describe('DatabaseMetricValues', () => { const filteredResult = await db.readEntityMetricsByStatus( 'github.metric1', 'error', + undefined, 'Component', undefined, { limit: 10, offset: 0 }, @@ -796,6 +808,7 @@ describe('DatabaseMetricValues', () => { undefined, undefined, undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -846,6 +859,7 @@ describe('DatabaseMetricValues', () => { 'github.metric1', 'error', undefined, + undefined, ['team:default/platform', 'team:default/backend'], { limit: 10, offset: 0 }, ); @@ -858,5 +872,61 @@ describe('DatabaseMetricValues', () => { ]); }, ); + + it.each(databases.eachSupportedId())( + 'should filter by entityName substring via catalog_entity_ref LIKE - %p', + async databaseId => { + const { client, db } = await createDatabase(databaseId); + + const timestamp = new Date('2023-01-01T00:00:00Z'); + + await client('metric_values').insert([ + { + catalog_entity_ref: 'component:default/my-service', + metric_id: 'github.metric1', + value: 10, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + { + catalog_entity_ref: 'component:default/service-api', + metric_id: 'github.metric1', + value: 5, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + { + catalog_entity_ref: 'component:default/unrelated', + metric_id: 'github.metric1', + value: 15, + timestamp, + status: 'error', + entity_kind: 'Component', + entity_owner: 'team:default/platform', + }, + ]); + + // 'service' should match 'my-service' and 'service-api' but not 'unrelated' + const result = await db.readEntityMetricsByStatus( + 'github.metric1', + undefined, + 'service', + undefined, + undefined, + { limit: 10, offset: 0 }, + ); + + expect(result.rows).toHaveLength(2); + expect(result.total).toBe(2); + expect(result.rows.map(r => r.catalog_entity_ref).sort()).toEqual([ + 'component:default/my-service', + 'component:default/service-api', + ]); + }, + ); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts index 0d06b41c4f..65f33f45ea 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts @@ -137,6 +137,7 @@ export class DatabaseMetricValues { async readEntityMetricsByStatus( metric_id: string, status?: 'success' | 'warning' | 'error', + entityName?: string, entityKind?: string, entityOwner?: string[], pagination?: { limit: number; offset: number }, @@ -158,6 +159,12 @@ export class DatabaseMetricValues { query.where('status', status); } + if (entityName) { + query.whereRaw('LOWER(catalog_entity_ref) LIKE LOWER(?)', [ + `%${entityName}%`, + ]); + } + if (entityKind) { query.whereRaw('LOWER(entity_kind) = LOWER(?)', [entityKind]); } 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 259c60ded5..422530298d 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts @@ -645,6 +645,7 @@ describe('CatalogMetricService', () => { undefined, undefined, undefined, + undefined, { limit: 5, offset: 5 }, ); }); @@ -665,6 +666,7 @@ describe('CatalogMetricService', () => { 'error', undefined, undefined, + undefined, { limit: 10, offset: 0 }, ); }); @@ -683,6 +685,7 @@ describe('CatalogMetricService', () => { expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', undefined, + undefined, 'Component', undefined, { limit: 10, offset: 0 }, @@ -704,12 +707,18 @@ describe('CatalogMetricService', () => { 'github.important_metric', undefined, undefined, + undefined, ['team:default/platform'], { limit: 10, offset: 0 }, ); }); - it('should filter by entityName at application level', async () => { + it('should filter by entityName at database level', async () => { + mockedDatabase.readEntityMetricsByStatus.mockResolvedValueOnce({ + rows: [mockMetricRows[0]], + total: 1, + }); + const result = await service.getEntityMetricDetails( 'github.important_metric', mockCredentials, @@ -720,23 +729,22 @@ describe('CatalogMetricService', () => { }, ); - // Should fetch all from DB (no pagination) expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', undefined, + 'service-a', undefined, undefined, - undefined, + { limit: 10, offset: 0 }, ); - // Should filter in application expect(result.entities).toHaveLength(1); expect(result.entities[0].entityName).toBe('service-a'); expect(result.pagination.total).toBe(1); }); - it('should perform case-insensitive entityName search', async () => { - const result = await service.getEntityMetricDetails( + it('should pass entityName to database for filtering', async () => { + await service.getEntityMetricDetails( 'github.important_metric', mockCredentials, { @@ -746,7 +754,14 @@ describe('CatalogMetricService', () => { }, ); - expect(result.entities).toHaveLength(3); + expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( + 'github.important_metric', + undefined, + 'SERVICE', + undefined, + undefined, + { limit: 10, offset: 0 }, + ); }); it('should sort by entityName ascending', async () => { @@ -907,6 +922,7 @@ describe('CatalogMetricService', () => { undefined, undefined, undefined, + undefined, { limit: 10, offset: 0 }, ); }); @@ -953,6 +969,7 @@ describe('CatalogMetricService', () => { expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', 'error', + undefined, 'Component', ['team:default/platform'], { limit: 5, offset: 0 }, diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts index e9578b03cd..ea780cd59f 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -217,23 +217,17 @@ export class CatalogMetricService { limit: number; }, ): Promise { - // Determine if we need application-level filtering - const needsAppFiltering = !!options.entityName; - - // If we need app-level filtering (entityName), fetch ALL results - // Otherwise, paginate at DB (status, kind, owner are DB-filtered) - const dbPagination = needsAppFiltering - ? undefined // Fetch all for entityName filtering - : { - limit: options.limit, - offset: (options.page - 1) * options.limit, - }; + const dbPagination = { + limit: options.limit, + offset: (options.page - 1) * options.limit, + }; // Fetch raw metric data from database const { rows, total: dbTotal } = await this.database.readEntityMetricsByStatus( metricId, options.status, + options.entityName, options.kind, options.owner, dbPagination, @@ -292,18 +286,8 @@ export class CatalogMetricService { }; }); - // Apply application-level filters - let filteredEntities = enrichedEntities; - - if (options.entityName) { - const searchTerm = options.entityName.toLowerCase(); - filteredEntities = filteredEntities.filter(e => - e.entityName.toLowerCase().includes(searchTerm), - ); - } - if (options.sortBy) { - filteredEntities.sort((a, b) => { + enrichedEntities.sort((a, b) => { let aValue: any; let bValue: any; @@ -346,30 +330,15 @@ export class CatalogMetricService { }); } else { // Default: sort by timestamp DESC - filteredEntities.sort((a, b) => { + enrichedEntities.sort((a, b) => { const aTime = new Date(a.timestamp).getTime(); const bTime = new Date(b.timestamp).getTime(); return bTime - aTime; // DESC }); } - // Paginate at application level if we filtered, otherwise use DB results - let finalEntities: EntityMetricDetail[]; - let finalTotal: number; - - if (needsAppFiltering) { - // Paginate the filtered results - const startIndex = (options.page - 1) * options.limit; - finalEntities = filteredEntities.slice( - startIndex, - startIndex + options.limit, - ); - finalTotal = filteredEntities.length; - } else { - // Use database-paginated results as-is - finalEntities = filteredEntities; - finalTotal = dbTotal; - } + const finalEntities: EntityMetricDetail[] = enrichedEntities; + const finalTotal = dbTotal; // Format and return response return { From 9d3523a09f2574b24b268d64bab9209c0e933e05 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 21:10:32 -0500 Subject: [PATCH 15/17] feat(scorecard): move sorting down to the database layer Signed-off-by: Patrick Knight --- .../scorecard-backend/docs/drill-down.md | 18 +-- .../src/database/DatabaseMetricValues.test.ts | 133 ++++++++++++++++++ .../src/database/DatabaseMetricValues.ts | 25 +++- .../src/service/CatalogMetricService.test.ts | 79 ++++++++--- .../src/service/CatalogMetricService.ts | 68 ++------- 5 files changed, 236 insertions(+), 87 deletions(-) diff --git a/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md b/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md index 86103ebd68..de40351024 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md +++ b/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md @@ -265,17 +265,17 @@ Entity name filtering is performed at the database level for consistent paginati ## Sorting -Results can be sorted by any column in ascending or descending order: +Results can be sorted by any column in ascending or descending order. Sorting is applied at the database level, so the correct order is guaranteed across all pages regardless of which filters are active. ### Sort Options -| Sort By | Description | Example Values | -| ------------- | ----------------------------------------------- | ------------------------ | -| `entityName` | Entity name alphabetically | "api-service", "web-app" | -| `owner` | Owner entity reference alphabetically | "team:default/platform" | -| `entityKind` | Entity kind alphabetically | "API", "Component" | -| `timestamp` | Metric sync timestamp (most/least recent) | ISO 8601 timestamps | -| `metricValue` | Metric value numerically (highest/lowest first) | 5, 15, 25, 100 | +| Sort By | Description | Example Values | +| ------------- | ---------------------------------------------------------------------------------------------- | ------------------------------------------------------ | +| `entityName` | Full entity ref (`kind:namespace/name`) alphabetically — equivalent to sorting by the full ref | "api:default/api-service", "component:default/web-app" | +| `owner` | Owner entity reference alphabetically | "team:default/platform" | +| `entityKind` | Entity kind alphabetically | "API", "Component" | +| `timestamp` | Metric sync timestamp (most/least recent) | ISO 8601 timestamps | +| `metricValue` | Metric value numerically (highest/lowest first) | 5, 15, 25, 100 | ### Default Sorting @@ -308,7 +308,7 @@ The response includes pagination metadata: ### Pagination Performance -All filters (`status`, `owner`, `kind`, and `entityName`) are applied at the database level before pagination. The `LIMIT`/`OFFSET` is always pushed to the database, so only the requested page of rows is fetched regardless of which filters are active. +All filters (`status`, `owner`, `kind`, and `entityName`) and sorting (`sortBy`, `sortOrder`) are applied at the database level before pagination. The `ORDER BY` and `LIMIT`/`OFFSET` are always pushed to the database, so only the requested page of rows is fetched in the correct order regardless of which filters are active. For best performance with large datasets, combine specific filters to reduce the result set size before paginating. 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 fa506ab607..8cb2f66320 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts @@ -314,6 +314,8 @@ describe('DatabaseMetricValues', () => { undefined, undefined, undefined, + undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -370,6 +372,8 @@ describe('DatabaseMetricValues', () => { undefined, undefined, undefined, + undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -431,6 +435,8 @@ describe('DatabaseMetricValues', () => { undefined, undefined, undefined, + undefined, + undefined, { limit: 2, offset: 0 }, ); @@ -444,6 +450,8 @@ describe('DatabaseMetricValues', () => { undefined, undefined, undefined, + undefined, + undefined, { limit: 2, offset: 2 }, ); @@ -457,6 +465,8 @@ describe('DatabaseMetricValues', () => { undefined, undefined, undefined, + undefined, + undefined, { limit: 2, offset: 4 }, ); @@ -476,6 +486,8 @@ describe('DatabaseMetricValues', () => { undefined, undefined, undefined, + undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -528,6 +540,8 @@ describe('DatabaseMetricValues', () => { undefined, 'Component', // Filter by kind undefined, + undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -583,6 +597,8 @@ describe('DatabaseMetricValues', () => { undefined, undefined, ['team:default/platform'], // Filter by owner + undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -647,6 +663,8 @@ describe('DatabaseMetricValues', () => { undefined, 'Component', // Only Component kind ['team:default/platform'], // Only platform team + undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -706,6 +724,7 @@ describe('DatabaseMetricValues', () => { undefined, undefined, undefined, + undefined, undefined, // No pagination ); @@ -750,6 +769,8 @@ describe('DatabaseMetricValues', () => { undefined, undefined, undefined, + undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -763,6 +784,8 @@ describe('DatabaseMetricValues', () => { undefined, 'Component', undefined, + undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -809,6 +832,8 @@ describe('DatabaseMetricValues', () => { undefined, undefined, undefined, + undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -861,6 +886,8 @@ describe('DatabaseMetricValues', () => { undefined, undefined, ['team:default/platform', 'team:default/backend'], + undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -917,6 +944,8 @@ describe('DatabaseMetricValues', () => { 'service', undefined, undefined, + undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -928,5 +957,109 @@ describe('DatabaseMetricValues', () => { ]); }, ); + + it.each(databases.eachSupportedId())( + 'should sort by catalog_entity_ref ascending when sortBy=entityName - %p', + async databaseId => { + const { client, db } = await createDatabase(databaseId); + + const timestamp = new Date('2023-01-01T00:00:00Z'); + + await client('metric_values').insert([ + { + catalog_entity_ref: 'component:default/service-c', + metric_id: 'github.metric1', + value: 1, + timestamp, + status: 'success', + }, + { + catalog_entity_ref: 'component:default/service-a', + metric_id: 'github.metric1', + value: 2, + timestamp, + status: 'success', + }, + { + catalog_entity_ref: 'component:default/service-b', + metric_id: 'github.metric1', + value: 3, + timestamp, + status: 'success', + }, + ]); + + const result = await db.readEntityMetricsByStatus( + 'github.metric1', + undefined, + undefined, + undefined, + undefined, + 'entityName', + 'asc', + { limit: 10, offset: 0 }, + ); + + expect(result.rows).toHaveLength(3); + expect(result.rows[0].catalog_entity_ref).toBe( + 'component:default/service-a', + ); + expect(result.rows[1].catalog_entity_ref).toBe( + 'component:default/service-b', + ); + expect(result.rows[2].catalog_entity_ref).toBe( + 'component:default/service-c', + ); + }, + ); + + it.each(databases.eachSupportedId())( + 'should sort by value descending with nulls last when sortBy=metricValue - %p', + async databaseId => { + const { client, db } = await createDatabase(databaseId); + + const timestamp = new Date('2023-01-01T00:00:00Z'); + + await client('metric_values').insert([ + { + catalog_entity_ref: 'component:default/service-a', + metric_id: 'github.metric1', + value: null, + timestamp, + status: 'error', + }, + { + catalog_entity_ref: 'component:default/service-b', + metric_id: 'github.metric1', + value: 5, + timestamp, + status: 'error', + }, + { + catalog_entity_ref: 'component:default/service-c', + metric_id: 'github.metric1', + value: 15, + timestamp, + status: 'error', + }, + ]); + + const result = await db.readEntityMetricsByStatus( + 'github.metric1', + undefined, + undefined, + undefined, + undefined, + 'metricValue', + 'desc', + { limit: 10, offset: 0 }, + ); + + expect(result.rows).toHaveLength(3); + expect(result.rows[0].value).toBe(15); + expect(result.rows[1].value).toBe(5); + expect(result.rows[2].value).toBeNull(); // null sorted last + }, + ); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts index 65f33f45ea..733454735a 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts @@ -140,6 +140,13 @@ export class DatabaseMetricValues { entityName?: string, entityKind?: string, entityOwner?: string[], + sortBy?: + | 'entityName' + | 'owner' + | 'entityKind' + | 'timestamp' + | 'metricValue', + sortOrder?: 'asc' | 'desc', pagination?: { limit: number; offset: number }, ): Promise<{ rows: DbMetricValue[]; total: number }> { const latestIdsSubquery = this.dbClient(this.tableName) @@ -153,7 +160,23 @@ export class DatabaseMetricValues { .whereIn('id', latestIdsSubquery) .where('metric_id', metric_id); - query.orderBy('timestamp', 'desc'); + const sortColumnMap: Record = { + entityName: 'catalog_entity_ref', + owner: 'entity_owner', + entityKind: 'entity_kind', + timestamp: 'timestamp', + metricValue: 'value', + }; + + const column = (sortBy && sortColumnMap[sortBy]) ?? 'timestamp'; + const direction = sortOrder ?? 'desc'; + + // Nulls last for metricValue (value can be null) + if (sortBy === 'metricValue') { + query.orderByRaw(`${column} IS NULL, ${column} ${direction}`); + } else { + query.orderBy(column, direction); + } if (status) { query.where('status', status); 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 422530298d..ac0ac21b77 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts @@ -646,6 +646,8 @@ describe('CatalogMetricService', () => { undefined, undefined, undefined, + undefined, + undefined, { limit: 5, offset: 5 }, ); }); @@ -667,6 +669,8 @@ describe('CatalogMetricService', () => { undefined, undefined, undefined, + undefined, + undefined, { limit: 10, offset: 0 }, ); }); @@ -688,6 +692,8 @@ describe('CatalogMetricService', () => { undefined, 'Component', undefined, + undefined, + undefined, { limit: 10, offset: 0 }, ); }); @@ -709,6 +715,8 @@ describe('CatalogMetricService', () => { undefined, undefined, ['team:default/platform'], + undefined, + undefined, { limit: 10, offset: 0 }, ); }); @@ -735,6 +743,8 @@ describe('CatalogMetricService', () => { 'service-a', undefined, undefined, + undefined, + undefined, { limit: 10, offset: 0 }, ); @@ -760,12 +770,14 @@ describe('CatalogMetricService', () => { 'SERVICE', undefined, undefined, + undefined, + undefined, { limit: 10, offset: 0 }, ); }); it('should sort by entityName ascending', async () => { - const result = await service.getEntityMetricDetails( + await service.getEntityMetricDetails( 'github.important_metric', mockCredentials, { @@ -776,13 +788,20 @@ describe('CatalogMetricService', () => { }, ); - expect(result.entities[0].entityName).toBe('service-a'); - expect(result.entities[1].entityName).toBe('service-b'); - expect(result.entities[2].entityName).toBe('service-c'); + expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( + 'github.important_metric', + undefined, + undefined, + undefined, + undefined, + 'entityName', + 'asc', + { limit: 10, offset: 0 }, + ); }); it('should sort by metricValue descending', async () => { - const result = await service.getEntityMetricDetails( + await service.getEntityMetricDetails( 'github.important_metric', mockCredentials, { @@ -793,13 +812,20 @@ describe('CatalogMetricService', () => { }, ); - expect(result.entities[0].metricValue).toBe(15); - expect(result.entities[1].metricValue).toBe(8); - expect(result.entities[2].metricValue).toBe(3); + expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( + 'github.important_metric', + undefined, + undefined, + undefined, + undefined, + 'metricValue', + 'desc', + { limit: 10, offset: 0 }, + ); }); it('should sort by timestamp descending by default', async () => { - const result = await service.getEntityMetricDetails( + await service.getEntityMetricDetails( 'github.important_metric', mockCredentials, { @@ -808,10 +834,17 @@ describe('CatalogMetricService', () => { }, ); - // Most recent first - expect(result.entities[0].timestamp).toBe('2024-01-15T12:00:00.000Z'); - expect(result.entities[1].timestamp).toBe('2024-01-15T11:00:00.000Z'); - expect(result.entities[2].timestamp).toBe('2024-01-15T10:00:00.000Z'); + // When no sortBy/sortOrder are supplied the DB defaults to timestamp desc + expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( + 'github.important_metric', + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + { limit: 10, offset: 0 }, + ); }); it('should handle null metric values in sorting', async () => { @@ -820,7 +853,7 @@ describe('CatalogMetricService', () => { total: 2, }); - const result = await service.getEntityMetricDetails( + await service.getEntityMetricDetails( 'github.important_metric', mockCredentials, { @@ -831,9 +864,17 @@ describe('CatalogMetricService', () => { }, ); - // Null should be sorted to the end - expect(result.entities[0].metricValue).toBe(8); - expect(result.entities[1].metricValue).toBe(null); + // Null handling (nulls-last) is delegated to the DB via orderByRaw + expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( + 'github.important_metric', + undefined, + undefined, + undefined, + undefined, + 'metricValue', + 'desc', + { limit: 10, offset: 0 }, + ); }); it('should batch-fetch entities using getEntitiesByRefs', async () => { @@ -923,6 +964,8 @@ describe('CatalogMetricService', () => { undefined, undefined, undefined, + undefined, + undefined, { limit: 10, offset: 0 }, ); }); @@ -972,6 +1015,8 @@ describe('CatalogMetricService', () => { undefined, 'Component', ['team:default/platform'], + 'metricValue', + 'desc', { limit: 5, offset: 0 }, ); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts index ea780cd59f..bf0ec1994c 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -183,15 +183,15 @@ export class CatalogMetricService { * Get detailed entity metrics for drill-down with filtering, sorting, and pagination. * * Fetches individual entity metric values and enriches them with catalog metadata. - * Supports database-level filtering (status, owner, kind) and application-level - * filtering (entityName). Falls back to database values if catalog is unavailable. + * Supports database-level filtering (status, owner, kind, entityName) and + * database-level sorting and pagination. Falls back to database values if catalog is unavailable. * * @param metricId - Metric ID to fetch (e.g., "github.open_prs") * @param options - Query options for filtering, sorting, and pagination * @param options.status - Filter by threshold status (database-level) * @param options.owner - Filter by owner entity reference (database-level) * @param options.kind - Filter by entity kind (database-level) - * @param options.entityName - Search entity names by substring (application-level) + * @param options.entityName - Substring search against the entity ref `kind:namespace/name` (database-level) * @param options.sortBy - Field to sort by (default: "timestamp") * @param options.sortOrder - Sort direction: "asc" or "desc" (default: "desc") * @param options.page - Page number (1-indexed) @@ -230,6 +230,8 @@ export class CatalogMetricService { options.entityName, options.kind, options.owner, + options.sortBy, + options.sortOrder, dbPagination, ); @@ -286,60 +288,6 @@ export class CatalogMetricService { }; }); - if (options.sortBy) { - enrichedEntities.sort((a, b) => { - let aValue: any; - let bValue: any; - - switch (options.sortBy) { - case 'entityName': - aValue = a.entityName.toLowerCase(); - bValue = b.entityName.toLowerCase(); - break; - case 'owner': - aValue = a.owner.toLowerCase(); - bValue = b.owner.toLowerCase(); - break; - case 'entityKind': - aValue = a.entityKind.toLowerCase(); - bValue = b.entityKind.toLowerCase(); - break; - case 'timestamp': - aValue = new Date(a.timestamp).getTime(); - bValue = new Date(b.timestamp).getTime(); - break; - case 'metricValue': - // Handle null values - sort them to the end - aValue = a.metricValue ?? -Infinity; - bValue = b.metricValue ?? -Infinity; - break; - default: - // Default to timestamp if invalid sortBy - aValue = new Date(a.timestamp).getTime(); - bValue = new Date(b.timestamp).getTime(); - } - - // Compare values - if (aValue < bValue) { - return options.sortOrder === 'asc' ? -1 : 1; - } - if (aValue > bValue) { - return options.sortOrder === 'asc' ? 1 : -1; - } - return 0; - }); - } else { - // Default: sort by timestamp DESC - enrichedEntities.sort((a, b) => { - const aTime = new Date(a.timestamp).getTime(); - const bTime = new Date(b.timestamp).getTime(); - return bTime - aTime; // DESC - }); - } - - const finalEntities: EntityMetricDetail[] = enrichedEntities; - const finalTotal = dbTotal; - // Format and return response return { metricId: metric.id, @@ -348,12 +296,12 @@ export class CatalogMetricService { description: metric.description, type: metric.type, }, - entities: finalEntities, + entities: enrichedEntities, pagination: { page: options.page, pageSize: options.limit, - total: finalTotal, - totalPages: Math.ceil(finalTotal / options.limit), + total: dbTotal, + totalPages: Math.ceil(dbTotal / options.limit), }, }; } From eb209d929cb3eda639e33c162c428bbe4189165a Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 22:20:17 -0500 Subject: [PATCH 16/17] feat(scorecard): address sonarqube findings Signed-off-by: Patrick Knight --- .../scorecard-backend/docs/drill-down.md | 15 +- .../src/database/DatabaseMetricValues.test.ts | 255 ++++++------------ .../src/database/DatabaseMetricValues.ts | 54 ++-- .../src/service/CatalogMetricService.test.ts | 130 +++------ .../src/service/CatalogMetricService.ts | 37 ++- 5 files changed, 180 insertions(+), 311 deletions(-) diff --git a/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md b/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md index de40351024..3a79992509 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md +++ b/workspaces/scorecard/plugins/scorecard-backend/docs/drill-down.md @@ -447,11 +447,20 @@ Entity metadata (name, kind, owner) is fetched from the catalog at request time **Possible causes**: -1. **Entity deleted from catalog**: Entity ref exists in metrics but entity removed +1. **Entity deleted from catalog**: Entity ref exists in metrics but entity was removed from the catalog 2. **Permission denied**: User lacks `catalog.entity.read` for that entity -3. **Catalog API error**: Temporary failure fetching catalog metadata -**Resolution**: Check console logs for warnings about failed entity fetches. +**Resolution**: Check whether the entity still exists in the catalog and that the user has the appropriate read permissions. + +### Catalog Unavailable + +**Symptom**: Empty entity list despite knowing entities with metric data exist + +**Possible causes**: + +1. **Catalog API unreachable**: The endpoint could not contact the catalog to verify entity access. To protect against unauthorized data exposure, results are not returned when authorization cannot be confirmed. + +**Resolution**: Check backend logs for `Failed to fetch entities from catalog` error entries and confirm the catalog service is healthy. ### Slow Responses 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 8cb2f66320..87d789c584 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.test.ts @@ -308,16 +308,10 @@ describe('DatabaseMetricValues', () => { }, ]); - const result = await db.readEntityMetricsByStatus( - 'github.metric1', - 'error', - undefined, - undefined, - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, - ); + const result = await db.readEntityMetricsByStatus('github.metric1', { + status: 'error', + pagination: { limit: 10, offset: 0 }, + }); // Should return 2 entities with error status expect(result.rows).toHaveLength(2); @@ -366,16 +360,9 @@ describe('DatabaseMetricValues', () => { }, ]); - const result = await db.readEntityMetricsByStatus( - 'github.metric1', - undefined, - undefined, - undefined, - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, - ); + const result = await db.readEntityMetricsByStatus('github.metric1', { + pagination: { limit: 10, offset: 0 }, + }); expect(result.rows).toHaveLength(3); expect(result.total).toBe(3); @@ -429,46 +416,28 @@ describe('DatabaseMetricValues', () => { ]); // Page 1: limit 2 - const page1 = await db.readEntityMetricsByStatus( - 'github.metric1', - 'error', - undefined, - undefined, - undefined, - undefined, - undefined, - { limit: 2, offset: 0 }, - ); + const page1 = await db.readEntityMetricsByStatus('github.metric1', { + status: 'error', + pagination: { limit: 2, offset: 0 }, + }); expect(page1.rows).toHaveLength(2); expect(page1.total).toBe(5); // Page 2: limit 2, offset 2 - const page2 = await db.readEntityMetricsByStatus( - 'github.metric1', - 'error', - undefined, - undefined, - undefined, - undefined, - undefined, - { limit: 2, offset: 2 }, - ); + const page2 = await db.readEntityMetricsByStatus('github.metric1', { + status: 'error', + pagination: { limit: 2, offset: 2 }, + }); expect(page2.rows).toHaveLength(2); expect(page2.total).toBe(5); // Total should stay the same // Page 3: limit 2, offset 4 - const page3 = await db.readEntityMetricsByStatus( - 'github.metric1', - 'error', - undefined, - undefined, - undefined, - undefined, - undefined, - { limit: 2, offset: 4 }, - ); + const page3 = await db.readEntityMetricsByStatus('github.metric1', { + status: 'error', + pagination: { limit: 2, offset: 4 }, + }); expect(page3.rows).toHaveLength(1); // Only 1 left on page 3 expect(page3.total).toBe(5); @@ -480,16 +449,10 @@ describe('DatabaseMetricValues', () => { async databaseId => { const { db } = await createDatabase(databaseId); - const result = await db.readEntityMetricsByStatus( - 'github.metric1', - 'error', - undefined, - undefined, - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, - ); + const result = await db.readEntityMetricsByStatus('github.metric1', { + status: 'error', + pagination: { limit: 10, offset: 0 }, + }); expect(result.rows).toHaveLength(0); expect(result.total).toBe(0); @@ -534,16 +497,11 @@ describe('DatabaseMetricValues', () => { }, ]); - const result = await db.readEntityMetricsByStatus( - 'github.metric1', - 'error', - undefined, - 'Component', // Filter by kind - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, - ); + const result = await db.readEntityMetricsByStatus('github.metric1', { + status: 'error', + entityKind: 'Component', // Filter by kind + pagination: { limit: 10, offset: 0 }, + }); // Should only return Component entities expect(result.rows).toHaveLength(2); @@ -591,16 +549,11 @@ describe('DatabaseMetricValues', () => { }, ]); - const result = await db.readEntityMetricsByStatus( - 'github.metric1', - 'error', - undefined, - undefined, - ['team:default/platform'], // Filter by owner - undefined, - undefined, - { limit: 10, offset: 0 }, - ); + const result = await db.readEntityMetricsByStatus('github.metric1', { + status: 'error', + entityOwner: ['team:default/platform'], // Filter by owner + pagination: { limit: 10, offset: 0 }, + }); // Should only return entities owned by team:default/platform expect(result.rows).toHaveLength(2); @@ -657,16 +610,12 @@ describe('DatabaseMetricValues', () => { }, ]); - const result = await db.readEntityMetricsByStatus( - 'github.metric1', - 'error', // Only error status - undefined, - 'Component', // Only Component kind - ['team:default/platform'], // Only platform team - undefined, - undefined, - { limit: 10, offset: 0 }, - ); + const result = await db.readEntityMetricsByStatus('github.metric1', { + status: 'error', // Only error status + entityKind: 'Component', // Only Component kind + entityOwner: ['team:default/platform'], // Only platform team + pagination: { limit: 10, offset: 0 }, + }); // Should only return service1 (Component, error, platform) expect(result.rows).toHaveLength(1); @@ -718,15 +667,9 @@ describe('DatabaseMetricValues', () => { ]); // No pagination parameter - should return all - const result = await db.readEntityMetricsByStatus( - 'github.metric1', - 'error', - undefined, - undefined, - undefined, - undefined, - undefined, // No pagination - ); + const result = await db.readEntityMetricsByStatus('github.metric1', { + status: 'error', + }); expect(result.rows).toHaveLength(3); expect(result.total).toBe(3); @@ -763,16 +706,10 @@ describe('DatabaseMetricValues', () => { ]); // Should return both when no filters - const result = await db.readEntityMetricsByStatus( - 'github.metric1', - 'error', - undefined, - undefined, - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, - ); + const result = await db.readEntityMetricsByStatus('github.metric1', { + status: 'error', + pagination: { limit: 10, offset: 0 }, + }); expect(result.rows).toHaveLength(2); expect(result.total).toBe(2); @@ -780,13 +717,11 @@ describe('DatabaseMetricValues', () => { // Should only return service2 when filtering by kind const filteredResult = await db.readEntityMetricsByStatus( 'github.metric1', - 'error', - undefined, - 'Component', - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, + { + status: 'error', + entityKind: 'Component', + pagination: { limit: 10, offset: 0 }, + }, ); expect(filteredResult.rows).toHaveLength(1); @@ -826,16 +761,9 @@ describe('DatabaseMetricValues', () => { // No owner filter — all rows for the metric are returned. // Per-row authorization is enforced downstream by catalog.getEntitiesByRefs. - const result = await db.readEntityMetricsByStatus( - 'github.metric1', - undefined, - undefined, - undefined, - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, - ); + const result = await db.readEntityMetricsByStatus('github.metric1', { + pagination: { limit: 10, offset: 0 }, + }); expect(result.rows).toHaveLength(2); expect(result.total).toBe(2); @@ -880,23 +808,20 @@ describe('DatabaseMetricValues', () => { ]); // Passing two owners returns only those two teams' entities. - const result = await db.readEntityMetricsByStatus( - 'github.metric1', - 'error', - undefined, - undefined, - ['team:default/platform', 'team:default/backend'], - undefined, - undefined, - { limit: 10, offset: 0 }, - ); + const result = await db.readEntityMetricsByStatus('github.metric1', { + status: 'error', + entityOwner: ['team:default/platform', 'team:default/backend'], + pagination: { limit: 10, offset: 0 }, + }); expect(result.rows).toHaveLength(2); expect(result.total).toBe(2); - expect(result.rows.map(r => r.entity_owner).sort()).toEqual([ - 'team:default/backend', - 'team:default/platform', - ]); + expect( + result.rows + .map(r => r.entity_owner) + .filter((o): o is string => o !== null) + .sort((a, b) => a.localeCompare(b)), + ).toEqual(['team:default/backend', 'team:default/platform']); }, ); @@ -938,20 +863,18 @@ describe('DatabaseMetricValues', () => { ]); // 'service' should match 'my-service' and 'service-api' but not 'unrelated' - const result = await db.readEntityMetricsByStatus( - 'github.metric1', - undefined, - 'service', - undefined, - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, - ); + const result = await db.readEntityMetricsByStatus('github.metric1', { + entityName: 'service', + pagination: { limit: 10, offset: 0 }, + }); expect(result.rows).toHaveLength(2); expect(result.total).toBe(2); - expect(result.rows.map(r => r.catalog_entity_ref).sort()).toEqual([ + expect( + result.rows + .map(r => r.catalog_entity_ref) + .sort((a, b) => a.localeCompare(b)), + ).toEqual([ 'component:default/my-service', 'component:default/service-api', ]); @@ -989,16 +912,11 @@ describe('DatabaseMetricValues', () => { }, ]); - const result = await db.readEntityMetricsByStatus( - 'github.metric1', - undefined, - undefined, - undefined, - undefined, - 'entityName', - 'asc', - { limit: 10, offset: 0 }, - ); + const result = await db.readEntityMetricsByStatus('github.metric1', { + sortBy: 'entityName', + sortOrder: 'asc', + pagination: { limit: 10, offset: 0 }, + }); expect(result.rows).toHaveLength(3); expect(result.rows[0].catalog_entity_ref).toBe( @@ -1044,16 +962,11 @@ describe('DatabaseMetricValues', () => { }, ]); - const result = await db.readEntityMetricsByStatus( - 'github.metric1', - undefined, - undefined, - undefined, - undefined, - 'metricValue', - 'desc', - { limit: 10, offset: 0 }, - ); + const result = await db.readEntityMetricsByStatus('github.metric1', { + sortBy: 'metricValue', + sortOrder: 'desc', + pagination: { limit: 10, offset: 0 }, + }); expect(result.rows).toHaveLength(3); expect(result.rows[0].value).toBe(15); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts index 733454735a..54846b8ce4 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts @@ -21,6 +21,16 @@ import { DbAggregatedMetric, } from './types'; +type ReadEntityMetricsByStatusOptions = { + status?: 'success' | 'warning' | 'error'; + entityName?: string; + entityKind?: string; + entityOwner?: string[]; + sortBy?: 'entityName' | 'owner' | 'entityKind' | 'timestamp' | 'metricValue'; + sortOrder?: 'asc' | 'desc'; + pagination?: { limit: number; offset: number }; +}; + export class DatabaseMetricValues { private readonly tableName = 'metric_values'; @@ -136,18 +146,7 @@ export class DatabaseMetricValues { */ async readEntityMetricsByStatus( metric_id: string, - status?: 'success' | 'warning' | 'error', - entityName?: string, - entityKind?: string, - entityOwner?: string[], - sortBy?: - | 'entityName' - | 'owner' - | 'entityKind' - | 'timestamp' - | 'metricValue', - sortOrder?: 'asc' | 'desc', - pagination?: { limit: number; offset: number }, + options: ReadEntityMetricsByStatusOptions, ): Promise<{ rows: DbMetricValue[]; total: number }> { const latestIdsSubquery = this.dbClient(this.tableName) .max('id') @@ -168,36 +167,39 @@ export class DatabaseMetricValues { metricValue: 'value', }; - const column = (sortBy && sortColumnMap[sortBy]) ?? 'timestamp'; - const direction = sortOrder ?? 'desc'; + const column = + (options.sortBy && sortColumnMap[options.sortBy]) ?? 'timestamp'; + const direction = options.sortOrder === 'asc' ? 'asc' : 'desc'; // Nulls last for metricValue (value can be null) - if (sortBy === 'metricValue') { - query.orderByRaw(`${column} IS NULL, ${column} ${direction}`); + if (options.sortBy === 'metricValue') { + query.orderByRaw( + `value IS NULL, CAST(CAST(value AS TEXT) AS REAL) ${direction}`, + ); } else { query.orderBy(column, direction); } - if (status) { - query.where('status', status); + if (options.status) { + query.where('status', options.status); } - if (entityName) { + if (options.entityName) { query.whereRaw('LOWER(catalog_entity_ref) LIKE LOWER(?)', [ - `%${entityName}%`, + `%${options.entityName}%`, ]); } - if (entityKind) { - query.whereRaw('LOWER(entity_kind) = LOWER(?)', [entityKind]); + if (options.entityKind) { + query.whereRaw('LOWER(entity_kind) = LOWER(?)', [options.entityKind]); } - if (entityOwner && entityOwner.length > 0) { - query.whereIn('entity_owner', entityOwner); + if (options.entityOwner && options.entityOwner.length > 0) { + query.whereIn('entity_owner', options.entityOwner); } - if (pagination) { - query.limit(pagination.limit).offset(pagination.offset); + if (options.pagination) { + query.limit(options.pagination.limit).offset(options.pagination.offset); } const rows = await query; 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 ac0ac21b77..42ae423b3a 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts @@ -642,13 +642,7 @@ describe('CatalogMetricService', () => { expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', - undefined, - undefined, - undefined, - undefined, - undefined, - undefined, - { limit: 5, offset: 5 }, + { pagination: { limit: 5, offset: 5 } }, ); }); @@ -665,13 +659,7 @@ describe('CatalogMetricService', () => { expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', - 'error', - undefined, - undefined, - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, + { status: 'error', pagination: { limit: 10, offset: 0 } }, ); }); @@ -688,13 +676,7 @@ describe('CatalogMetricService', () => { expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', - undefined, - undefined, - 'Component', - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, + { entityKind: 'Component', pagination: { limit: 10, offset: 0 } }, ); }); @@ -711,13 +693,10 @@ describe('CatalogMetricService', () => { expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', - undefined, - undefined, - undefined, - ['team:default/platform'], - undefined, - undefined, - { limit: 10, offset: 0 }, + { + entityOwner: ['team:default/platform'], + pagination: { limit: 10, offset: 0 }, + }, ); }); @@ -739,13 +718,7 @@ describe('CatalogMetricService', () => { expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', - undefined, - 'service-a', - undefined, - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, + { entityName: 'service-a', pagination: { limit: 10, offset: 0 } }, ); expect(result.entities).toHaveLength(1); @@ -766,13 +739,7 @@ describe('CatalogMetricService', () => { expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', - undefined, - 'SERVICE', - undefined, - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, + { entityName: 'SERVICE', pagination: { limit: 10, offset: 0 } }, ); }); @@ -790,13 +757,11 @@ describe('CatalogMetricService', () => { expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', - undefined, - undefined, - undefined, - undefined, - 'entityName', - 'asc', - { limit: 10, offset: 0 }, + { + sortBy: 'entityName', + sortOrder: 'asc', + pagination: { limit: 10, offset: 0 }, + }, ); }); @@ -814,13 +779,11 @@ describe('CatalogMetricService', () => { expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', - undefined, - undefined, - undefined, - undefined, - 'metricValue', - 'desc', - { limit: 10, offset: 0 }, + { + sortBy: 'metricValue', + sortOrder: 'desc', + pagination: { limit: 10, offset: 0 }, + }, ); }); @@ -837,13 +800,7 @@ describe('CatalogMetricService', () => { // When no sortBy/sortOrder are supplied the DB defaults to timestamp desc expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', - undefined, - undefined, - undefined, - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, + { pagination: { limit: 10, offset: 0 } }, ); }); @@ -867,13 +824,11 @@ describe('CatalogMetricService', () => { // Null handling (nulls-last) is delegated to the DB via orderByRaw expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', - undefined, - undefined, - undefined, - undefined, - 'metricValue', - 'desc', - { limit: 10, offset: 0 }, + { + sortBy: 'metricValue', + sortOrder: 'desc', + pagination: { limit: 10, offset: 0 }, + }, ); }); @@ -924,7 +879,7 @@ describe('CatalogMetricService', () => { expect(result.entities[1].entityRef).toBe('component:default/service-c'); }); - it('should handle catalog API failures gracefully by falling back to DB metadata', async () => { + it('should handle catalog API failures by logging an error and not returning information from the database', async () => { mockedCatalog.getEntitiesByRefs.mockRejectedValue( new Error('Catalog API error'), ); @@ -938,17 +893,13 @@ describe('CatalogMetricService', () => { }, ); - expect(mockedLogger.warn).toHaveBeenCalledWith( + expect(mockedLogger.error).toHaveBeenCalledWith( 'Failed to fetch entities from catalog', expect.objectContaining({ error: expect.any(Error) }), ); - // When catalog is unavailable, all DB rows are returned with fallback metadata - // so a transient outage doesn't silently hide all results from the user. - expect(result.entities).toHaveLength(3); - expect(result.entities[0].entityName).toBe('Unknown'); - expect(result.entities[0].entityKind).toBe('Component'); // from DB row.entity_kind - expect(result.entities[0].owner).toBe('team:default/platform'); // from DB row.entity_owner + // When catalog is unavailable, do not bypass and instead log error + expect(result.entities).toHaveLength(0); }); it('should pass null to database for unscoped query (avoids catalog enumeration)', async () => { @@ -960,13 +911,7 @@ describe('CatalogMetricService', () => { expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', - undefined, - undefined, - undefined, - undefined, - undefined, - undefined, - { limit: 10, offset: 0 }, + { pagination: { limit: 10, offset: 0 } }, ); }); @@ -1011,13 +956,14 @@ describe('CatalogMetricService', () => { expect(mockedDatabase.readEntityMetricsByStatus).toHaveBeenCalledWith( 'github.important_metric', - 'error', - undefined, - 'Component', - ['team:default/platform'], - 'metricValue', - 'desc', - { limit: 5, offset: 0 }, + { + status: 'error', + entityKind: 'Component', + entityOwner: ['team:default/platform'], + sortBy: 'metricValue', + sortOrder: 'desc', + pagination: { limit: 5, offset: 0 }, + }, ); expect(result.entities).toHaveLength(1); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts index bf0ec1994c..81de477eb6 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -224,16 +224,15 @@ export class CatalogMetricService { // Fetch raw metric data from database const { rows, total: dbTotal } = - await this.database.readEntityMetricsByStatus( - metricId, - options.status, - options.entityName, - options.kind, - options.owner, - options.sortBy, - options.sortOrder, - dbPagination, - ); + await this.database.readEntityMetricsByStatus(metricId, { + status: options.status, + entityName: options.entityName, + entityKind: options.kind, + entityOwner: options.owner, + sortBy: options.sortBy, + sortOrder: options.sortOrder, + pagination: dbPagination, + }); // Get metric metadata const metric = this.registry.getMetric(metricId); @@ -243,7 +242,6 @@ export class CatalogMetricService { // cannot access are returned as null in response.items. const entityRefsToFetch = rows.map(row => row.catalog_entity_ref); const entityMap = new Map(); - let catalogAvailable = true; if (entityRefsToFetch.length > 0) { try { @@ -263,17 +261,18 @@ export class CatalogMetricService { } }); } catch (error) { - // Catalog is unavailable — fall back to DB-only metadata rather than - // returning empty results, so a transient outage doesn't silently hide data. - catalogAvailable = false; - this.logger.warn('Failed to fetch entities from catalog', { error }); + // Catalog unavailable — entityMap stays empty, so the filter below removes all rows. + // Fail secure: authorization cannot be confirmed without the catalog, so no results + // are returned rather than risking exposure of unauthorized entity metric data. + this.logger.error('Failed to fetch entities from catalog', { error }); } } - // When catalog is available: filter to only authorized entities (null response = no access). - // When catalog is unavailable: include all rows with fallback DB metadata for resilience. + // Only include entities the catalog confirmed the user can access. + // Unauthorized entities are returned as null by getEntitiesByRefs and are never added + // to entityMap, so they are silently excluded here. const enrichedEntities: EntityMetricDetail[] = rows - .filter(row => !catalogAvailable || entityMap.has(row.catalog_entity_ref)) + .filter(row => entityMap.has(row.catalog_entity_ref)) .map(row => { const entity = entityMap.get(row.catalog_entity_ref); return { @@ -284,7 +283,7 @@ export class CatalogMetricService { (entity?.spec?.owner as string) ?? row.entity_owner ?? 'Unknown', metricValue: row.value, timestamp: new Date(row.timestamp).toISOString(), - status: row.status!, + status: row.status ?? 'error', // default to error if status is null }; }); From a14730f200d1d46cb661bb4ed518b6d358f2fd68 Mon Sep 17 00:00:00 2001 From: Patrick Knight Date: Wed, 18 Feb 2026 22:54:14 -0500 Subject: [PATCH 17/17] feat(scorecard): logger full error but throw generic error Signed-off-by: Patrick Knight --- .../plugins/scorecard-backend/src/plugin.ts | 1 + .../src/service/router.test.ts | 3 + .../scorecard-backend/src/service/router.ts | 5 +- .../validateDrillDownMetricsSchema.test.ts | 207 ++++++++++++------ .../validateDrillDownMetricsSchema.ts | 17 +- 5 files changed, 167 insertions(+), 66 deletions(-) diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts b/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts index 6cc805e4ed..10ee3abe63 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts @@ -119,6 +119,7 @@ export const scorecardPlugin = createBackendPlugin({ catalog, httpAuth, permissions, + logger, }), ); }, 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 450e8f4bc7..a4895aa497 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -128,6 +128,7 @@ describe('createRouter', () => { catalog, httpAuth: httpAuthMock, permissions: permissionsMock, + logger: mockServices.logger.mock(), }); app = express(); app.use(router); @@ -552,6 +553,7 @@ describe('createRouter', () => { catalog: mockCatalog, httpAuth: httpAuthMock, permissions: permissionsMock, + logger: mockServices.logger.mock(), }); aggregationApp = express(); aggregationApp.use(router); @@ -798,6 +800,7 @@ describe('createRouter', () => { catalog: mockCatalog, httpAuth: httpAuthMock, permissions: permissionsMock, + logger: mockServices.logger.mock(), }); drillDownApp = express(); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index 9aa2d3bd58..b54529940f 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 Router from 'express-promise-router'; import type { CatalogMetricService } from './CatalogMetricService'; import type { MetricProvidersRegistry } from '../providers/MetricProvidersRegistry'; import { + LoggerService, type HttpAuthService, type PermissionsService, } from '@backstage/backend-plugin-api'; @@ -52,6 +53,7 @@ export type ScorecardRouterOptions = { catalog: CatalogService; httpAuth: HttpAuthService; permissions: PermissionsService; + logger: LoggerService; }; export async function createRouter({ @@ -60,6 +62,7 @@ export async function createRouter({ catalog, httpAuth, permissions, + logger, }: ScorecardRouterOptions): Promise { const router = Router(); router.use(express.json()); @@ -207,7 +210,7 @@ export async function createRouter({ entityName, sortBy, sortOrder, - } = validateDrillDownMetricsSchema(req.query); + } = validateDrillDownMetricsSchema(req.query, logger); const { conditions } = await authorizeConditional( req, diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.test.ts index 5fa1cbffd4..eed6110ff3 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.test.ts @@ -15,11 +15,14 @@ */ import { InputError } from '@backstage/errors'; import { validateDrillDownMetricsSchema } from './validateDrillDownMetricsSchema'; +import { mockServices } from '@backstage/backend-test-utils'; describe('validateDrillDownMetricsSchema', () => { describe('valid query parameters', () => { it('should return defaults when given an empty object', () => { - expect(validateDrillDownMetricsSchema({})).toEqual({ + expect( + validateDrillDownMetricsSchema({}, mockServices.logger.mock()), + ).toEqual({ page: 1, pageSize: 5, sortOrder: 'desc', @@ -27,28 +30,40 @@ describe('validateDrillDownMetricsSchema', () => { }); it('should coerce page and pageSize from strings to numbers', () => { - const result = validateDrillDownMetricsSchema({ - page: '3', - pageSize: '20', - }); + const result = validateDrillDownMetricsSchema( + { + page: '3', + pageSize: '20', + }, + mockServices.logger.mock(), + ); expect(result.page).toBe(3); expect(result.pageSize).toBe(20); }); it('should accept page at max boundary of 10000', () => { - const result = validateDrillDownMetricsSchema({ page: '10000' }); + const result = validateDrillDownMetricsSchema( + { page: '10000' }, + mockServices.logger.mock(), + ); expect(result.page).toBe(10000); }); it('should accept pageSize at max boundary of 100', () => { - const result = validateDrillDownMetricsSchema({ pageSize: '100' }); + const result = validateDrillDownMetricsSchema( + { pageSize: '100' }, + mockServices.logger.mock(), + ); expect(result.pageSize).toBe(100); }); it.each(['success', 'warning', 'error'] as const)( 'should accept status=%s', status => { - const result = validateDrillDownMetricsSchema({ status }); + const result = validateDrillDownMetricsSchema( + { status }, + mockServices.logger.mock(), + ); expect(result.status).toBe(status); }, ); @@ -60,29 +75,41 @@ describe('validateDrillDownMetricsSchema', () => { 'timestamp', 'metricValue', ] as const)('should accept sortBy=%s', sortBy => { - const result = validateDrillDownMetricsSchema({ sortBy }); + const result = validateDrillDownMetricsSchema( + { sortBy }, + mockServices.logger.mock(), + ); expect(result.sortBy).toBe(sortBy); }); it.each(['asc', 'desc'] as const)( 'should accept sortOrder=%s', sortOrder => { - const result = validateDrillDownMetricsSchema({ sortOrder }); + const result = validateDrillDownMetricsSchema( + { sortOrder }, + mockServices.logger.mock(), + ); expect(result.sortOrder).toBe(sortOrder); }, ); it('should normalize a single owner string to an array', () => { - const result = validateDrillDownMetricsSchema({ - owner: 'team:default/platform', - }); + const result = validateDrillDownMetricsSchema( + { + owner: 'team:default/platform', + }, + mockServices.logger.mock(), + ); expect(result.owner).toEqual(['team:default/platform']); }); it('should accept an array of owner strings', () => { - const result = validateDrillDownMetricsSchema({ - owner: ['team:default/platform', 'user:default/alice'], - }); + const result = validateDrillDownMetricsSchema( + { + owner: ['team:default/platform', 'user:default/alice'], + }, + mockServices.logger.mock(), + ); expect(result.owner).toEqual([ 'team:default/platform', 'user:default/alice', @@ -90,33 +117,45 @@ describe('validateDrillDownMetricsSchema', () => { }); it('should return undefined when owner is not provided', () => { - const result = validateDrillDownMetricsSchema({}); + const result = validateDrillDownMetricsSchema( + {}, + mockServices.logger.mock(), + ); expect(result.owner).toBeUndefined(); }); it('should accept a valid kind string', () => { - const result = validateDrillDownMetricsSchema({ kind: 'Component' }); + const result = validateDrillDownMetricsSchema( + { kind: 'Component' }, + mockServices.logger.mock(), + ); expect(result.kind).toBe('Component'); }); it('should accept a valid entityName string', () => { - const result = validateDrillDownMetricsSchema({ - entityName: 'my-service', - }); + const result = validateDrillDownMetricsSchema( + { + entityName: 'my-service', + }, + mockServices.logger.mock(), + ); expect(result.entityName).toBe('my-service'); }); it('should accept all valid parameters together', () => { - const result = validateDrillDownMetricsSchema({ - page: '2', - pageSize: '10', - status: 'error', - sortBy: 'metricValue', - sortOrder: 'asc', - owner: 'team:default/backend', - kind: 'Component', - entityName: 'my-service', - }); + const result = validateDrillDownMetricsSchema( + { + page: '2', + pageSize: '10', + status: 'error', + sortBy: 'metricValue', + sortOrder: 'asc', + owner: 'team:default/backend', + kind: 'Component', + entityName: 'my-service', + }, + mockServices.logger.mock(), + ); expect(result).toEqual({ page: 2, @@ -131,40 +170,58 @@ describe('validateDrillDownMetricsSchema', () => { }); it('should strip unknown properties', () => { - const result = validateDrillDownMetricsSchema({ unknownProp: 'value' }); + const result = validateDrillDownMetricsSchema( + { unknownProp: 'value' }, + mockServices.logger.mock(), + ); expect(result).not.toHaveProperty('unknownProp'); }); }); describe('invalid query parameters', () => { it('should throw InputError when page is 0', () => { - expect(() => validateDrillDownMetricsSchema({ page: '0' })).toThrow( - InputError, - ); + expect(() => + validateDrillDownMetricsSchema( + { page: '0' }, + mockServices.logger.mock(), + ), + ).toThrow(InputError); }); it('should throw InputError when page is negative', () => { - expect(() => validateDrillDownMetricsSchema({ page: '-1' })).toThrow( - InputError, - ); + expect(() => + validateDrillDownMetricsSchema( + { page: '-1' }, + mockServices.logger.mock(), + ), + ).toThrow(InputError); }); it('should throw InputError when page exceeds 10000', () => { - expect(() => validateDrillDownMetricsSchema({ page: '10001' })).toThrow( - InputError, - ); + expect(() => + validateDrillDownMetricsSchema( + { page: '10001' }, + mockServices.logger.mock(), + ), + ).toThrow(InputError); }); it('should throw InputError when pageSize is 0', () => { - expect(() => validateDrillDownMetricsSchema({ pageSize: '0' })).toThrow( - InputError, - ); + expect(() => + validateDrillDownMetricsSchema( + { pageSize: '0' }, + mockServices.logger.mock(), + ), + ).toThrow(InputError); }); it('should throw InputError when pageSize exceeds 100', () => { - expect(() => validateDrillDownMetricsSchema({ pageSize: '101' })).toThrow( - InputError, - ); + expect(() => + validateDrillDownMetricsSchema( + { pageSize: '101' }, + mockServices.logger.mock(), + ), + ).toThrow(InputError); }); it('should throw InputError when more than 50 owner values are provided', () => { @@ -173,10 +230,16 @@ describe('validateDrillDownMetricsSchema', () => { (_, i) => `team:default/team-${i}`, ); expect(() => - validateDrillDownMetricsSchema({ owner: tooManyOwners }), + validateDrillDownMetricsSchema( + { owner: tooManyOwners }, + mockServices.logger.mock(), + ), ).toThrow(InputError); expect(() => - validateDrillDownMetricsSchema({ owner: tooManyOwners }), + validateDrillDownMetricsSchema( + { owner: tooManyOwners }, + mockServices.logger.mock(), + ), ).toThrow('Invalid query parameters'); }); @@ -188,10 +251,16 @@ describe('validateDrillDownMetricsSchema', () => { 'should throw InputError when $field has invalid value "$value"', ({ field, value }) => { expect(() => - validateDrillDownMetricsSchema({ [field]: value }), + validateDrillDownMetricsSchema( + { [field]: value }, + mockServices.logger.mock(), + ), ).toThrow(InputError); expect(() => - validateDrillDownMetricsSchema({ [field]: value }), + validateDrillDownMetricsSchema( + { [field]: value }, + mockServices.logger.mock(), + ), ).toThrow('Invalid query parameters'); }, ); @@ -199,22 +268,34 @@ describe('validateDrillDownMetricsSchema', () => { it.each(['kind', 'entityName'])( 'should throw InputError when %s is an empty string', field => { - expect(() => validateDrillDownMetricsSchema({ [field]: '' })).toThrow( - InputError, - ); - expect(() => validateDrillDownMetricsSchema({ [field]: '' })).toThrow( - 'Invalid query parameters', - ); + expect(() => + validateDrillDownMetricsSchema( + { [field]: '' }, + mockServices.logger.mock(), + ), + ).toThrow(InputError); + expect(() => + validateDrillDownMetricsSchema( + { [field]: '' }, + mockServices.logger.mock(), + ), + ).toThrow('Invalid query parameters'); }, ); it('should throw InputError when owner contains an empty string', () => { - expect(() => validateDrillDownMetricsSchema({ owner: [''] })).toThrow( - InputError, - ); - expect(() => validateDrillDownMetricsSchema({ owner: [''] })).toThrow( - 'Invalid query parameters', - ); + expect(() => + validateDrillDownMetricsSchema( + { owner: [''] }, + mockServices.logger.mock(), + ), + ).toThrow(InputError); + expect(() => + validateDrillDownMetricsSchema( + { owner: [''] }, + mockServices.logger.mock(), + ), + ).toThrow('Invalid query parameters'); }); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.ts b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.ts index 1340295c02..190585c623 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateDrillDownMetricsSchema.ts @@ -15,8 +15,12 @@ */ import { z } from 'zod'; import { InputError } from '@backstage/errors'; +import { LoggerService } from '@backstage/backend-plugin-api'; -export function validateDrillDownMetricsSchema(query: unknown) { +export function validateDrillDownMetricsSchema( + query: unknown, + logger: LoggerService, +) { const drillDownSchema = z.object({ page: z.coerce.number().int().min(1).max(10000).optional().default(1), pageSize: z.coerce.number().int().min(1).max(100).optional().default(5), @@ -37,7 +41,16 @@ export function validateDrillDownMetricsSchema(query: unknown) { const parsed = drillDownSchema.safeParse(query); if (!parsed.success) { - throw new InputError(`Invalid query parameters: ${parsed.error.message}`); + logger.warn('Invalid drill-down query parameters', { + errors: JSON.stringify( + parsed.error.errors.map(e => ({ + path: e.path.join('.'), + message: e.message, + code: e.code, + })), + ), + }); + throw new InputError('Invalid query parameters'); } return parsed.data;