Skip to content

Comments

feat(scorecard): add the ability to drill down into aggregated scorecard data#2350

Open
PatAKnight wants to merge 17 commits intoredhat-developer:mainfrom
PatAKnight:scorecard-aggregated-drill-down
Open

feat(scorecard): add the ability to drill down into aggregated scorecard data#2350
PatAKnight wants to merge 17 commits intoredhat-developer:mainfrom
PatAKnight:scorecard-aggregated-drill-down

Conversation

@PatAKnight
Copy link
Member

@PatAKnight PatAKnight commented Feb 18, 2026

User description

Hey, I just made a Pull Request!

This PR 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.

Included is a new endpoint GET /metrics/:metricId/catalog/aggregations/entities that includes the ability to filter by status, owner, kind, and entity name. The owner parameter accepts multiple values (e.g., ?owner=a&owner=b, up to 50) so that frontends can implement "owned by me" scoping by passing the user's ownership refs directly. The endpoint also includes the ability to sort by entity name, owner, kind, timestamp, and metric value. Finally, offset-based pagination has been included.

I also added the entity_kind and entity_owner columns to the metric_values table. This allows all filtering and sorting to be performed at the database level, keeping queries efficient even at large entity counts. All filters, ORDER BY, and LIMIT/OFFSET are pushed to the database so only the requested page of rows is ever fetched.

The entityName filter performs a case-insensitive substring search against the full entity reference (kind:namespace/name), rather than just the entity name, which keeps it consistent with how references are stored and avoids an additional column.

I also batch-fetch from the catalog using getEntitiesByRefs to ensure metadata is accurate and up-to-date. This call also serves as the per-row authorization gate — entities the user doesn't have catalog.entity.read access to are returned as null by the catalog and are excluded from the response.

Which issue(s) does this PR fix

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or Updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)

Testing the changes

# Set environment variables for convenience
export BACKSTAGE_URL="http://localhost:7007"
export TOKEN="your-auth-token-here"

1. Get All Entities for a Metric (Default)

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities" \
  -H "Authorization: Bearer ${TOKEN}" \
  -H "Content-Type: application/json" | jq

2. Get Second Page (if you have multiple pages)

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?page=2&pageSize=5" \
  -H "Authorization: Bearer ${TOKEN}" | jq

3. Get More Results Per Page

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?pageSize=10" \
  -H "Authorization: Bearer ${TOKEN}" | jq

4. Get Entities in Error State

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?status=error" \
  -H "Authorization: Bearer ${TOKEN}" | jq

5. Get Only Components

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?kind=Component" \
  -H "Authorization: Bearer ${TOKEN}" | jq

6. Get Only Systems (lowercase — filter is case-insensitive)

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?kind=system" \
  -H "Authorization: Bearer ${TOKEN}" | jq

7. Get Entities Owned by Development Guests Team

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?owner=group:development/guests" \
  -H "Authorization: Bearer ${TOKEN}" | jq

8. Get Entities Owned by Guest User

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?owner=user:development/guest" \
  -H "Authorization: Bearer ${TOKEN}" | jq

9. Get Entities Owned by Multiple Teams (repeat the owner param)

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?owner=group:development/guests&owner=user:development/guest" \
  -H "Authorization: Bearer ${TOKEN}" | jq

10. Search for "scorecard" Services (substring match against full entity ref)

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?entityName=scorecard" \
  -H "Authorization: Bearer ${TOKEN}" | jq

11. Search Using the Full Ref Format

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?entityName=component:default/my-service" \
  -H "Authorization: Bearer ${TOKEN}" | jq

12. Sort by Entity Name (Alphabetically)

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?sortBy=entityName&sortOrder=asc" \
  -H "Authorization: Bearer ${TOKEN}" | jq

13. Sort by Metric Value (Highest First)

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?sortBy=metricValue&sortOrder=desc" \
  -H "Authorization: Bearer ${TOKEN}" | jq

14. Combine Filters — Team Errors Sorted by Metric Value

curl -X GET "${BACKSTAGE_URL}/api/scorecard/metrics/github.open_prs/catalog/aggregations/entities?owner=group:development/guests&status=error&sortBy=metricValue&sortOrder=desc&page=1&pageSize=10" \
  -H "Authorization: Bearer ${TOKEN}" | jq

PR Type

Enhancement


Description

  • Add drill-down endpoint to view individual entities contributing to aggregated scorecard metrics

  • Implement database-level filtering by status, owner, and kind for optimal performance

  • Add entity metadata columns (entity_kind, entity_owner) to metric_values table with indexes

  • Support application-level filtering by entity name, sorting, and pagination with comprehensive tests

  • Include batch catalog enrichment and graceful fallback when catalog metadata unavailable


Diagram Walkthrough

flowchart LR
  A["GET /metrics/:metricId/catalog/aggregations/entities"] -->|Query params| B["Router Handler"]
  B -->|Fetch metrics| C["CatalogMetricService.getEntityMetricDetails"]
  C -->|Database query| D["DatabaseMetricValues.readEntityMetricsByStatus"]
  D -->|Filter & paginate| E["metric_values table"]
  C -->|Batch fetch| F["Catalog Service"]
  F -->|Enrich with metadata| G["EntityMetricDetail[]"]
  G -->|Apply app-level filters| H["Sort & paginate"]
  H -->|Return response| I["EntityMetricDetailResponse"]
Loading

File Walkthrough

Relevant files
Enhancement
7 files
DatabaseMetricValues.ts
Add readEntityMetricsByStatus method for filtered entity metrics
+61/-0   
types.ts
Add entity_kind and entity_owner fields to metric types   
+4/-0     
CatalogMetricService.ts
Implement getEntityMetricDetails with filtering, sorting, pagination
+213/-1 
router.ts
Add GET endpoint for entity drill-down with permission checks
+85/-0   
PullMetricsByProviderTask.ts
Populate entity_kind and entity_owner during metric collection
+10/-0   
Metric.ts
Add EntityMetricDetail and EntityMetricDetailResponse types
+35/-0   
plugin.ts
Pass logger to CatalogMetricService constructor                   
+1/-0     
Configuration changes
1 files
20260217152637_add_entity_metadata_columns.js
Create migration to add entity_kind and entity_owner columns
+41/-0   
Tests
6 files
DatabaseMetricValues.test.ts
Add comprehensive tests for readEntityMetricsByStatus functionality
+603/-0 
CatalogMetricService.test.ts
Add tests for getEntityMetricDetails with filters and sorting
+451/-1 
router.test.ts
Add endpoint tests for entity drill-down with permissions
+358/-0 
PullMetricsByProviderTask.test.ts
Update tests to verify entity metadata population               
+4/-0     
mockDatabaseMetricValues.ts
Add mock for readEntityMetricsByStatus database method     
+8/-0     
mockMetricProvidersRegistry.ts
Add getMetric mock implementation to registry                       
+6/-0     
Documentation
2 files
drill-down.md
Add comprehensive documentation for drill-down endpoint   
+483/-0 
sour-coins-check.md
Add changeset for drill-down feature release                         
+6/-0     

@rhdh-gh-app
Copy link

rhdh-gh-app bot commented Feb 18, 2026

Important

This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@red-hat-developer-hub/backstage-plugin-scorecard-backend workspaces/scorecard/plugins/scorecard-backend minor v2.3.5
@red-hat-developer-hub/backstage-plugin-scorecard-common workspaces/scorecard/plugins/scorecard-common minor v2.3.5

@rhdh-qodo-merge
Copy link

rhdh-qodo-merge bot commented Feb 18, 2026

PR Compliance Guide 🔍

(Compliance updated until commit a14730f)

Below is a summary of compliance checks for this PR:

Security Compliance
🔴
Authorization count leakage

Description: The endpoint returns pagination.total and totalPages computed from the database total
(dbTotal) before per-entity authorization filtering via catalog.getEntitiesByRefs, which
can leak the existence/count of unauthorized entities matching the query (e.g., a user
sees total: 500 but only receives 2 authorized rows).
CatalogMetricService.ts [201-305]

Referred Code
async getEntityMetricDetails(
  metricId: string,
  credentials: BackstageCredentials,
  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<EntityMetricDetailResponse> {
  const dbPagination = {
    limit: options.limit,


 ... (clipped 84 lines)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
🟢
No codebase code duplication found New Components Detected (Top 5):
- describe: readEntityMetricsByStatus
- readEntityMetricsByStatus
- describe: getEntityMetricDetails
- it: should fetch entity metrics with default options
- it: should enrich entities with catalog metadata
Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Unauthorized count leak: The response pagination.total/totalPages are computed from the database dbTotal even
though unauthorized entities are removed via catalog.getEntitiesByRefs, which can leak the
existence/count of entities the user is not permitted to read.

Referred Code
// 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 => 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 ?? 'error', // default to error if status is null
    };
  });

// Format and return response
return {


 ... (clipped 14 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit logging: The new drill-down read endpoint does not appear to emit any audit log entry (who, what
metric, filters, outcome), so whether this read of potentially sensitive metric/entity
data is appropriately auditable cannot be verified from the diff.

Referred Code
router.get(
  '/metrics/:metricId/catalog/aggregations/entities',
  async (req, res) => {
    const { metricId } = req.params;

    const {
      page,
      pageSize,
      status,
      owner,
      kind,
      entityName,
      sortBy,
      sortOrder,
    } = validateDrillDownMetricsSchema(req.query, logger);

    const { conditions } = await authorizeConditional(
      req,
      scorecardMetricReadPermission,
    );



 ... (clipped 37 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Pagination edge mismatch: When catalog authorization filters out rows (or catalog fails), the returned
pagination.total/totalPages are still based on dbTotal, so clients may see inconsistent
pagination vs returned entities and may not be able to page reliably.

Referred Code
// 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 => 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 ?? 'error', // default to error if status is null
    };
  });

// Format and return response
return {


 ... (clipped 14 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Previous compliance checks

Compliance check up to commit 8dec549
Security Compliance
🔴
Authorization count leak

Description: The API returns pagination total/totalPages derived from the database row count (dbTotal)
even though unauthorized entities are filtered out after catalog.getEntitiesByRefs, which
can leak the existence/volume of entities the caller is not allowed to read (e.g.,
entities length is 2 but total may be 100).
CatalogMetricService.ts [226-305]

Referred Code
const { rows, total: dbTotal } =
  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);

// 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<string, Entity>();

if (entityRefsToFetch.length > 0) {


 ... (clipped 59 lines)
SQL injection risk

Description: The query uses orderByRaw with an interpolated ${direction} string, so if
readEntityMetricsByStatus is ever called with unvalidated sortOrder (outside the router’s
Zod validation), it could enable SQL injection via crafted sort direction.
DatabaseMetricValues.ts [170-181]

Referred Code
const column =
  (options.sortBy && sortColumnMap[options.sortBy]) ?? 'timestamp';
const direction = options.sortOrder ?? 'desc';

// Nulls last for metricValue (value can be null)
if (options.sortBy === 'metricValue') {
  query.orderByRaw(
    `value IS NULL, CAST(CAST(value AS TEXT) AS REAL) ${direction}`,
  );
} else {
  query.orderBy(column, direction);
}
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
🟢
No codebase code duplication found New Components Detected (Top 5):
- describe: readEntityMetricsByStatus
- readEntityMetricsByStatus
- describe: getEntityMetricDetails
- it: should fetch entity metrics with default options
- it: should enrich entities with catalog metadata
Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Incorrect pagination totals: The response pagination.total/totalPages are calculated from the database total before
catalog authorization filtering, so counts/pages can be incorrect and not reflect the
returned entities.

Referred Code
// 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 => 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 ?? 'error', // default to error if status is null
    };
  });

// Format and return response
return {


 ... (clipped 14 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Detailed validation errors: The thrown InputError includes ${parsed.error.message} which may expose internal
validation/schema details to end users instead of a generic message.

Referred Code
const parsed = drillDownSchema.safeParse(query);

if (!parsed.success) {
  throw new InputError(`Invalid query parameters: ${parsed.error.message}`);
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Unauthorized count leakage: The endpoint returns pagination.total derived from the DB (dbTotal) even when unauthorized
entities are filtered out (or when catalog lookup fails), which can leak information about
entities the user cannot read.

Referred Code
// 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 => 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 ?? 'error', // default to error if status is null
    };
  });

// Format and return response
return {


 ... (clipped 14 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing access logging: The new drill-down read endpoint does not emit any explicit audit log for access, and
whether this constitutes a “critical action” cannot be verified from the diff alone.

Referred Code
router.get(
  '/metrics/:metricId/catalog/aggregations/entities',
  async (req, res) => {
    const { metricId } = req.params;

    const {
      page,
      pageSize,
      status,
      owner,
      kind,
      entityName,
      sortBy,
      sortOrder,
    } = validateDrillDownMetricsSchema(req.query);

    const { conditions } = await authorizeConditional(
      req,
      scorecardMetricReadPermission,
    );



 ... (clipped 37 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance check up to commit 9d3523a
Security Compliance
🔴
Authorization bypass

Description: When catalog.getEntitiesByRefs fails, the code sets catalogAvailable = false and then
returns all DB rows without filtering unauthorized entities, which can leak entity metric
details to users who lack catalog.entity.read access (an authorization bypass triggered by
catalog outage or induced failure).
CatalogMetricService.ts [241-289]

Referred Code
// 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<string, Entity>();
let catalogAvailable = true;

if (entityRefsToFetch.length > 0) {
  try {
    const response = await this.catalog.getEntitiesByRefs(
      {
        entityRefs: entityRefsToFetch,
        fields: ['kind', 'metadata', 'spec'],
      },
      { credentials },
    );

    // Build map of ref -> entity (null entries = unauthorized, not added to map)
    entityRefsToFetch.forEach((ref, index) => {
      const entity = response.items[index];
      if (entity) {


 ... (clipped 28 lines)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
🟢
No codebase code duplication found New Components Detected (Top 5):
- describe: readEntityMetricsByStatus
- readEntityMetricsByStatus
- describe: getEntityMetricDetails
- it: should fetch entity metrics with default options
- it: should enrich entities with catalog metadata
Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Auth bypass on fallback: When catalog.getEntitiesByRefs fails the code returns all DB rows with fallback metadata,
which bypasses the catalog permission gate and can expose entity metric data to callers
who would otherwise be filtered as unauthorized.

Referred Code
// 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<string, Entity>();
let catalogAvailable = true;

if (entityRefsToFetch.length > 0) {
  try {
    const response = await this.catalog.getEntitiesByRefs(
      {
        entityRefs: entityRefsToFetch,
        fields: ['kind', 'metadata', 'spec'],
      },
      { credentials },
    );

    // Build map of ref -> entity (null entries = unauthorized, not added to map)
    entityRefsToFetch.forEach((ref, index) => {
      const entity = response.items[index];
      if (entity) {


 ... (clipped 28 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing access audit: The new drill-down read endpoint does not emit an audit-style log entry capturing who
accessed which metric and the outcome, so it is unclear if critical read access is
reconstructable from logs.

Referred Code
router.get(
  '/metrics/:metricId/catalog/aggregations/entities',
  async (req, res) => {
    const { metricId } = req.params;

    const {
      page,
      pageSize,
      status,
      owner,
      kind,
      entityName,
      sortBy,
      sortOrder,
    } = validateDrillDownMetricsSchema(req.query);

    const { conditions } = await authorizeConditional(
      req,
      scorecardMetricReadPermission,
    );



 ... (clipped 36 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance check up to commit 1ec8035
Security Compliance
Denial of service

Description: The new drill-down endpoint can be abused for resource exhaustion because it allows very
large page values (leading to huge DB OFFSETs) and may build extremely large
authorized-entity scopes (via catalog enumeration) that are then passed downstream,
potentially triggering expensive catalog pagination and DB queries per request.
router.ts [201-277]

Referred Code
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(


 ... (clipped 56 lines)
Catalog enumeration DoS

Description: getAuthorizedEntityRefs iterates through all catalog entities readable by the caller using
cursor pagination and collects every ref into memory, which can be exploited by repeated
requests (or large catalogs) to cause high CPU/memory usage and downstream query
amplification.
permissionUtils.ts [56-77]

Referred Code
export async function getAuthorizedEntityRefs(options: {
  catalog: CatalogService;
  credentials: BackstageCredentials;
}): Promise<string[]> {
  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);



 ... (clipped 1 lines)
Expensive DB query

Description: readEntityMetricsByStatus performs whereIn('catalog_entity_ref', catalog_entity_refs) with
a caller-supplied array that could become very large, creating oversized SQL IN clauses
and heavy window-count queries that are susceptible to performance-based denial-of-service
when invoked with large authorized-entity scopes.
DatabaseMetricValues.ts [137-186]

Referred Code
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 }> {
  if (catalog_entity_refs.length === 0) {
    return { rows: [], total: 0 };
  }

  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'))


 ... (clipped 29 lines)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
🟢
No codebase code duplication found New Components Detected (Top 5):
- describe: readEntityMetricsByStatus
- readEntityMetricsByStatus
- describe: getAuthorizedEntityRefs
- it: should return entity refs for entities the user is authorized to see
- it: should call queryEntities with user credentials
Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: 🏷️
Missing query validation: The new endpoint casts multiple query parameters (page, sortOrder, status, sortBy, etc.)
without validation/enforcement of allowed values or bounds, enabling invalid/negative
pagination inputs and unexpected parameter values to reach service/database logic.

Referred Code
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';

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: 🏷️
Missing access audit: The new drill-down read endpoint returns entity-level metric values but does not add any
audit logging of who accessed which metric/entity scope and the outcome.

Referred Code
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'


 ... (clipped 65 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: 🏷️
Unhandled catalog errors: getAuthorizedEntityRefs performs a paginated catalog query loop without adding contextual
error handling/logging, so catalog failures may surface as generic internal errors without
actionable context.

Referred Code
export async function getAuthorizedEntityRefs(options: {
  catalog: CatalogService;
  credentials: BackstageCredentials;
}): Promise<string[]> {
  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);



 ... (clipped 2 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance check up to commit 8398560
Security Compliance
🔴
Authorization bypass

Description: The new drill-down endpoint returns entity-level metric data for all entities when
ownedByMe is not true (by setting entityRefsToQuery = []) without performing per-entity
authorization checks, enabling users with metric read permission to access metric details
for entities they may not be allowed to read (IDOR / authorization bypass).
router.ts [195-277]

Referred Code
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'


 ... (clipped 62 lines)
Privilege escalation

Description: Entity metadata enrichment uses this.auth.getOwnServiceCredentials() to call
catalog.getEntitiesByRefs, which can bypass end-user catalog authorization and leak kind,
metadata, and spec.owner for entities the requesting user cannot access.
CatalogMetricService.ts [243-268]

Referred Code
// Batch-fetch entities from catalog
const entityRefsToFetch = rows.map(row => row.catalog_entity_ref);
const entityMap = new Map<string, Entity>();

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);
      }


 ... (clipped 5 lines)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
🟢
No codebase code duplication found New Components Detected (Top 5):
- describe: readEntityMetricsByStatus
- readEntityMetricsByStatus
- describe: getEntityMetricDetails
- it: should fetch entity metrics with default options
- it: should enrich entities with catalog metadata
Custom Compliance
🟢
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Inconsistent identifier casing: New TypeScript method parameters use snake_case (e.g., catalog_entity_refs, metric_id)
instead of conventional camelCase, reducing readability and consistency with surrounding
code.

Referred Code
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 }> {

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Pagination edge cases: The endpoint does not validate that page and pageSize are positive integers (e.g.,
negative pageSize can result in an invalid negative limit being passed downstream).

Referred Code
const page = Number(req.query.page) || 1;
const pageSize = Math.min(Number(req.query.pageSize) || 5, 100);
const status = req.query.status as
  | 'success'

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Missing query validation: Query parameters (page, pageSize, status, sortBy, sortOrder) are not strictly
validated/normalized at runtime before use, allowing invalid values (e.g., negative
pageSize) to reach service/database layers.

Referred Code
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';

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No access audit log: The new drill-down endpoint returns entity-level metric data but does not emit an audit
log entry identifying the caller, action, and outcome, which may be required if this data
is considered sensitive.

Referred Code
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'


 ... (clipped 62 lines)

Learn more about managing compliance generic rules or creating your own custom rules

@rhdh-qodo-merge
Copy link

rhdh-qodo-merge bot commented Feb 18, 2026

PR Code Suggestions ✨

Latest suggestions up to 8dec549

CategorySuggestion                                                                                                                                    Impact
Possible issue
Make metric sorting type-safe

Improve the sorting logic for metricValue to be database-agnostic and handle
boolean values correctly by using a CASE statement instead of casting to REAL.

workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts [174-181]

-// Nulls last for metricValue (value can be null)
+// Nulls last for metricValue (value can be null and can be boolean)
 if (options.sortBy === 'metricValue') {
   query.orderByRaw(
-    `value IS NULL, CAST(CAST(value AS TEXT) AS REAL) ${direction}`,
+    `
+    CASE WHEN value IS NULL THEN 1 ELSE 0 END ASC,
+    CASE
+      WHEN value = 'true' THEN 1
+      WHEN value = 'false' THEN 0
+      ELSE value
+    END ${direction}
+    `.trim(),
   );
 } else {
   query.orderBy(column, direction);
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that casting the value column to REAL is not robust, as the column can contain booleans, which could lead to sorting errors or incorrect behavior across different databases. The proposed CASE statement is a more portable and type-safe solution for sorting mixed-type data.

Medium
Select latest rows by timestamp

To ensure the latest metric data is always selected, modify the query to select
rows based on the maximum timestamp for each entity, rather than relying on the
maximum id.

workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts [151-160]

-const latestIdsSubquery = this.dbClient(this.tableName)
-  .max('id')
+const latestPerEntity = this.dbClient(this.tableName)
+  .select('catalog_entity_ref')
+  .max({ max_timestamp: 'timestamp' })
   .where('metric_id', metric_id)
-  .groupBy('metric_id', 'catalog_entity_ref');
+  .groupBy('catalog_entity_ref')
+  .as('latest_per_entity');
 
 const query = this.dbClient(this.tableName)
-  .select('*')
+  .select(`${this.tableName}.*`)
   .select(this.dbClient.raw('COUNT(*) OVER() as total_count'))
-  .whereIn('id', latestIdsSubquery)
-  .where('metric_id', metric_id);
+  .join(latestPerEntity, join => {
+    join
+      .on(`${this.tableName}.catalog_entity_ref`, '=', 'latest_per_entity.catalog_entity_ref')
+      .andOn(`${this.tableName}.timestamp`, '=', 'latest_per_entity.max_timestamp');
+  })
+  .where(`${this.tableName}.metric_id`, metric_id);
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies that using MAX(id) is not a reliable way to get the latest metric value if data can be inserted out of order. However, the proposed improved_code is flawed as it can return duplicate rows if multiple entries share the same latest timestamp for an entity, which would be a new bug.

Low
  • Update

Previous suggestions

✅ Suggestions up to commit 8398560
CategorySuggestion                                                                                                                                    Impact
High-level
Denormalization introduces data synchronization challenges

The denormalization of entity_kind and entity_owner in the metric_values table
will cause stale data issues when entity metadata changes. Implement a
synchronization mechanism, like a catalog processor, to update historical metric
records and maintain data consistency.

Examples:

workspaces/scorecard/plugins/scorecard-backend/migrations/20260217152637_add_entity_metadata_columns.js [18-28]
  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');

 ... (clipped 1 lines)
workspaces/scorecard/plugins/scorecard-backend/src/database/DatabaseMetricValues.ts [175-182]
    if (entityKind) {
      query.whereRaw('LOWER(entity_kind) = LOWER(?)', [entityKind]);
    }

    // Filter by entity_owner
    if (entityOwner) {
      query.whereRaw('LOWER(entity_owner) = LOWER(?)', [entityOwner]);
    }

Solution Walkthrough:

Before:

// In PullMetricsByProviderTask.ts
// When a new metric is pulled, current metadata is stored.
const metricValue = {
  // ... other fields
  entity_kind: entity.kind,
  entity_owner: entity.spec.owner,
};
database.createMetricValues([metricValue]);

// In DatabaseMetricValues.ts
// When filtering, the query uses the stored (potentially stale) data.
function readEntityMetricsByStatus(..., entityOwner, entityKind) {
  query.where('entity_owner', entityOwner); // Uses stale data
  query.where('entity_kind', entityKind);   // Uses stale data
  // ...
}
// No mechanism exists to update old metric_values records
// if entity.kind or entity.owner changes in the catalog.

After:

// Suggestion: Add a new mechanism, e.g., a Catalog Processor.

class MetricValueUpdaterProcessor implements CatalogProcessor {
  // ... constructor with database connection

  async postProcessEntity(entity, location) {
    const owner = entity.spec?.owner;
    const kind = entity.kind;
    const entityRef = stringifyEntityRef(entity);

    // Check if the stored metadata for this entity is outdated.
    const isOutdated = await this.database.isMetadataOutdated(entityRef, kind, owner);

    // If metadata has changed, update all historical records for this entity.
    if (isOutdated) {
       await this.database.updateHistoricalEntityMetadata(entityRef, kind, owner);
    }
    return entity;
  }
}
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical data consistency issue where stale entity_kind and entity_owner data will lead to incorrect filtering results, undermining the reliability of the new drill-down feature.

High
Possible issue
Fix dropIndex column args
Suggestion Impact:Instead of adding the missing column args to dropIndex, the commit removed the index creation in the up migration and removed the dropIndex calls entirely from the down migration, avoiding the failing rollback behavior.

code diff:

-    // 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');
-

Fix the down migration script by providing the column names (['entity_kind'] and
['entity_owner']) to the table.dropIndex calls, which is required for them to
function correctly.

workspaces/scorecard/plugins/scorecard-backend/migrations/20260217152637_add_entity_metadata_columns.js [31-41]

 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');
+    table.dropIndex(['entity_kind'], 'idx_metric_values_entity_kind');
+    table.dropIndex(['entity_owner'], 'idx_metric_values_entity_owner');
 
     // Drop columns
     table.dropColumn('entity_kind');
     table.dropColumn('entity_owner');
   });
 };

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: This suggestion fixes a critical bug in the database migration's down function. The dropIndex calls were missing the required column names, which would cause the rollback to fail. This is an important correctness fix.

High
Fix undefined value in catch

Fix a ReferenceError in the catch block by explicitly setting value: null in the
returned error object, as the value variable is out of scope.

workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts [167-178]

 } catch (error) {
   return {
     catalog_entity_ref: stringifyEntityRef(entity),
     metric_id: this.providerId,
-    value,
+    value: null,
     timestamp: new Date(),
     error_message:
       error instanceof Error ? error.message : String(error),
     entity_kind: entity.kind,
     entity_owner: normalizeOwner(entity?.spec?.owner),
   } as DbMetricValueCreate;
 }
Suggestion importance[1-10]: 9

__

Why: This suggestion identifies a critical ReferenceError bug where the value variable is used in a catch block where it is not defined. The fix is correct and essential for preventing the metric-pulling task from crashing.

High
Prevent storing invalid owner data

Update the normalizeOwner function to only process string values for owners and
return undefined for other types, preventing the storage of non-filterable
JSON-stringified owner data.

workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts [204-208]

 function normalizeOwner(owner: unknown): string | undefined {
-  if (!owner) return undefined;
-  if (typeof owner === 'string') return owner;
-  return JSON.stringify(owner);
+  if (typeof owner === 'string') {
+    return owner;
+  }
+  return undefined;
 }
Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies that storing stringified JSON for an entity owner will break the filtering logic. The proposed fix ensures only valid string owners are stored, which is critical for the correctness of the new drill-down feature.

Medium
General
Safe‐guard null status mapping

Replace the non-null assertion (!) on row.status with a nullish coalescing
operator (?? 'error') to safely handle cases where the status might be null and
prevent potential runtime errors.

workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts [272-284]

 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!,
+    status: row.status ?? 'error',
   };
 });
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies the unsafe use of a non-null assertion on row.status. Replacing it with a nullish coalescing operator (??) provides a safe fallback and prevents potential runtime errors, improving code robustness.

Medium
Sort nulls always last

Modify the sorting logic for metricValue to use Infinity for ascending and
-Infinity for descending order when the value is null, ensuring null values are
always sorted to the end.

workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts [318-322]

 case 'metricValue':
   // Handle null values - sort them to the end
-  aValue = a.metricValue ?? -Infinity;
-  bValue = b.metricValue ?? -Infinity;
+  aValue = a.metricValue ?? (options.sortOrder === 'asc' ? Infinity : -Infinity);
+  bValue = b.metricValue ?? (options.sortOrder === 'asc' ? Infinity : -Infinity);
   break;
Suggestion importance[1-10]: 7

__

Why: This suggestion provides an elegant and correct fix for sorting null metric values. The proposed change ensures nulls are consistently placed at the end of the sorted list for both ascending and descending orders, which fixes a bug in the original implementation.

Medium
Validate pagination query parameters

Add validation to ensure page and pageSize query parameters are positive
integers to prevent invalid pagination values.

workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts [200-201]

-const page = Number(req.query.page) || 1;
-const pageSize = Math.min(Number(req.query.pageSize) || 5, 100);
+const page = Math.max(1, Math.floor(Number(req.query.page) || 1));
+const pageSize = Math.min(
+  Math.max(1, Math.floor(Number(req.query.pageSize) || 5)),
+  100,
+);
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a lack of input validation for pagination parameters and proposes a robust fix using Math.floor and Math.max to prevent invalid values, improving the endpoint's reliability.

Low
Improve handling of missing entities

Improve the handling of missing catalog entities by changing the fallback
entityName from Unknown to [Deleted Entity] to more clearly indicate that the
entity no longer exists in the catalog.

workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts [278-279]

+entityName: entity?.metadata?.name ?? '[Deleted Entity]',
 entityKind: entity?.kind ?? row.entity_kind ?? 'Unknown',
 owner: (entity?.spec?.owner as string) ?? row.entity_owner ?? 'Unknown',
Suggestion importance[1-10]: 5

__

Why: The suggestion improves the user experience by making it clearer when an entity has been deleted from the catalog, changing the fallback name from Unknown to [Deleted Entity]. This is a useful but minor enhancement.

Low

…ard data

Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
…g fetch

Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
…ser credentials during catalog fetch

Signed-off-by: Patrick Knight <pknight@redhat.com>
…a param

Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
Signed-off-by: Patrick Knight <pknight@redhat.com>
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant