Fix: Cache key includes query parameters to prevent stale data after 422 errors (#250)

* Initial plan

* Fix: Update cache key to include query parameters and clear cache on errors

Co-authored-by: karpikpl <[email protected]>

* fix for caching

* error message

* Update shared/utils/metrics-util.ts

Co-authored-by: Copilot <[email protected]>

* code review comments

* fix test setup

---------

Co-authored-by: Copilot <[email protected]>
Co-authored-by: karpikpl <[email protected]>
Co-authored-by: Piotr Karpala <[email protected]>
Co-authored-by: Piotr Karpala <[email protected]>
Co-authored-by: Copilot <[email protected]>

Author: 3 people

app/components/MainComponent.vue

@@ -174,6 +174,8 @@ export default defineNuxtComponent({
       const config = useRuntimeConfig();
 
       this.isLoading = true;
+      // Clear previous API errors when making a new request
+      this.apiError = undefined;
 
       try {
         const options = Options.fromRoute(this.route, this.dateRange.since, this.dateRange.until);
@@ -216,7 +218,7 @@ export default defineNuxtComponent({
             this.apiError = `404 Not Found - is the ${this.config?.public?.scope || ''} org:"${this.config?.public?.githubOrg || ''}" ent:"${this.config?.public?.githubEnt || ''}" team:"${this.config?.public?.githubTeam}" correct? ${error.message}`;
             break;
           case 422:
-            this.apiError = `422 Unprocessable Entity - Is the Copilot Metrics API enabled for the Org/Ent? ${error.message}`;
+            this.apiError = `422 Unprocessable Entity - Is the Copilot Metrics API enabled for the Org/Ent? When changing filters, try adjusting the "from" date.  ${error.message}`;
             break;
           case 500:
             this.apiError = `500 Internal Server Error - most likely a bug in the app. Error: ${error.message}`;

server/api/teams.ts

@@ -1,12 +1,6 @@
 import { Options, type Scope } from '@/model/Options'
 import type { H3Event, EventHandlerRequest } from 'h3'
 
-const cache = new Map<string, CacheData>()
-
-interface CacheData {
-    data: Team[]
-    valid_until: number
-}
 interface Team { name: string; slug: string; description: string }
 interface GitHubTeam { name: string; slug: string; description?: string }
 
@@ -72,16 +66,6 @@ export async function getTeams(event: H3Event<EventHandlerRequest>): Promise<Tea
         return teams
     }
 
-    // Use cache if valid
-    if (cache.has(event.path)) {
-        const cachedData = cache.get(event.path)
-        if (cachedData && cachedData.valid_until > Math.floor(Date.now() / 1000)) {
-            logger.info(`Returning cached data for ${event.path}`)
-            return cachedData.data
-        }
-        cache.delete(event.path)
-    }
-
     if (!event.context.headers.has('Authorization')) {
         logger.error('No Authentication provided')
         throw new TeamsError('No Authentication provided', 401)
@@ -114,7 +98,5 @@ export async function getTeams(event: H3Event<EventHandlerRequest>): Promise<Tea
         page += 1
     }
 
-    // Cache for 60 seconds
-    cache.set(event.path, { data: allTeams, valid_until: Math.floor(Date.now() / 1000) + 60 })
     return allTeams
 }

shared/utils/metrics-util.ts

@@ -5,6 +5,7 @@ import { readFileSync } from 'fs';
 import { resolve } from 'path';
 import { getLocale } from "./getLocale";
 import { filterHolidaysFromMetrics, isHoliday, parseUtcDate } from '@/utils/dateUtils';
+import { createHash } from 'crypto';
 
 const cache = new Map<string, CacheData>();
 
@@ -24,6 +25,50 @@ class MetricsError extends Error {
   }
 }
 
+/**
+ * Builds a cache key for metrics data that is bound to the caller's Authorization header (hashed) + path + query.
+ * Exported for unit testing.
+ */
+type QueryParamValue = string | string[] | undefined;
+type QueryParams = Record<string, QueryParamValue>;
+
+export function buildMetricsCacheKey(path: string, query: QueryParams, authHeader: string): string {
+  // Split existing query params from provided path (if any)
+  const [rawPath, existingQueryString] = path.split('?');
+  const merged = new Map<string, string>();
+
+  // Add existing params first
+  if (existingQueryString) {
+    const existingParams = new URLSearchParams(existingQueryString);
+    existingParams.forEach((value, key) => {
+      merged.set(key, value);
+    });
+  }
+
+  // Merge in provided query object (overrides existing)
+  Object.entries(query).forEach(([k, v]) => {
+    if (v === undefined || v === null) return;
+    if (Array.isArray(v)) {
+      if (v.length === 0) return;
+      merged.set(k, v.join(','));
+    } else if (v !== '') {
+      merged.set(k, v);
+    }
+  });
+
+  // Build stable, sorted query string
+  const sortedKeys = Array.from(merged.keys()).sort();
+  const finalParams = new URLSearchParams();
+  sortedKeys.forEach(k => {
+    const val = merged.get(k);
+    if (val !== undefined) finalParams.set(k, val);
+  });
+  const finalQueryString = finalParams.toString();
+
+  const authFingerprint = createHash('sha256').update(authHeader).digest('hex').slice(0, 16); // short fingerprint
+  return `${authFingerprint}:${rawPath}${finalQueryString ? `?${finalQueryString}` : ''}`;
+}
+
 export async function getMetricsData(event: H3Event<EventHandlerRequest>): Promise<CopilotMetrics[]> {
   const logger = console;
   const query = getQuery(event);
@@ -52,26 +97,33 @@ export async function getMetricsData(event: H3Event<EventHandlerRequest>): Promi
     return usageData;
   }
 
-  if (cache.has(event.path)) {
-    const cachedData = cache.get(event.path);
-    if (cachedData && cachedData.valid_until > Date.now() / 1000) {
-      logger.info(`Returning cached data for ${event.path}`);
+  // Authorization must be validated BEFORE any cache lookup to prevent leakage of cached data
+  const authHeader = event.context.headers.get('Authorization');
+  if (!authHeader) {
+    logger.error('No Authentication provided');
+    throw new MetricsError('No Authentication provided', 401);
+  }
+
+  // Build auth-bound cache key
+  const path = event.path || '/api/metrics'; // fallback path (should always exist in practice)
+  const cacheKey = buildMetricsCacheKey(path, query as QueryParams, authHeader);
+
+  // Attempt cache lookup with auth fingerprint validation
+  const cachedData = cache.get(cacheKey);
+  if (cachedData) {
+    if (cachedData.valid_until > Date.now() / 1000) {
+      logger.info(`Returning cached data for ${cacheKey}`);
       return cachedData.data;
     } else {
-      logger.info(`Cached data for ${event.path} is expired, fetching new data`);
-      cache.delete(event.path);
+      logger.info(`Cached data for ${cacheKey} is expired or fingerprint mismatch, fetching new data`);
+      cache.delete(cacheKey);
     }
   }
 
-  if (!event.context.headers.has('Authorization')) {
-    logger.error('No Authentication provided');
-    throw new MetricsError('No Authentication provided', 401);
-  }
-
   logger.info(`Fetching metrics data from ${apiUrl}`);
 
   try {
-    const response = await $fetch(apiUrl, {
+  const response = await $fetch(apiUrl, {
       headers: event.context.headers
     }) as unknown[];
 
@@ -81,10 +133,12 @@ export async function getMetricsData(event: H3Event<EventHandlerRequest>): Promi
     const filteredUsageData = filterHolidaysFromMetrics(usageData, options.excludeHolidays || false, options.locale);
     // metrics is the old API format
     const validUntil = Math.floor(Date.now() / 1000) + 5 * 60; // Cache for 5 minutes
-    cache.set(event.path, { data: filteredUsageData, valid_until: validUntil });
+  cache.set(cacheKey, { data: filteredUsageData, valid_until: validUntil });
     return filteredUsageData;
   } catch (error: unknown) {
     logger.error('Error fetching metrics data:', error);
+    // Clear any cached data for this request to prevent stale data on retry
+    cache.delete(cacheKey);
     const errorMessage = error instanceof Error ? error.message : String(error);
     const statusCode = (error && typeof error === 'object' && 'statusCode' in error)
       ? (error as { statusCode: number }).statusCode
@@ -125,17 +179,12 @@ function updateMockDataDates(originalData: CopilotMetrics[], since?: string, unt
   }
 
   // Update dates in the dataset, copying existing entries when needed
-  const result = dateRange.map((date, index) => {
-    // Use existing data entries, cycling through them
+  const result: CopilotMetrics[] = dateRange.map((date, index) => {
     const dataIndex = index % originalData.length;
-    const entry = { ...originalData[dataIndex] };
-
-    // Update the date
-    entry.date = date.toISOString().split('T')[0];
-
-    return entry;
+    const src = originalData[dataIndex];
+    const newDate = date.toISOString().split('T')[0];
+    return { ...src, date: newDate };
   });
-
   return result;
 }
 

tests/metrics-auth-cache.nuxt.spec.ts

@@ -0,0 +1,63 @@
+// @vitest-environment nuxt
+import { describe, it, expect } from 'vitest'
+import { buildMetricsCacheKey } from '../shared/utils/metrics-util'
+
+/**
+ * These tests validate that the new cache key generation logic binds cache entries
+ * to the caller's Authorization header fingerprint and that different auth headers
+ * produce distinct keys even when path + query are identical.
+ */
+
+describe('Metrics Auth-Bound Cache Key', () => {
+  const path = '/api/metrics'
+  const query = { since: '2024-01-01', until: '2024-01-31', scope: 'organization', githubOrg: 'test-org' }
+
+  it('produces different keys for different auth headers', () => {
+    const keyA1 = buildMetricsCacheKey(path, query, 'token userA-1')
+    const keyA2 = buildMetricsCacheKey(path, query, 'token userA-2')
+    const keyB = buildMetricsCacheKey(path, query, 'token userB-1')
+
+    expect(keyA1).not.toBe(keyA2)
+    expect(keyA1.split(':')[0]).not.toBe(keyA2.split(':')[0])
+    expect(keyA1.split(':')[0]).not.toBe(keyB.split(':')[0])
+  })
+
+  it('is stable for same auth header + query', () => {
+    const key1 = buildMetricsCacheKey(path, query, 'token stable-user')
+    const key2 = buildMetricsCacheKey(path, query, 'token stable-user')
+    expect(key1).toBe(key2)
+  })
+
+  it('filters out undefined/empty query params', () => {
+    const key = buildMetricsCacheKey(path, { since: '2024-01-01', empty: '', undef: undefined }, 'token x')
+    expect(key).toContain('since=2024-01-01')
+    expect(key).not.toContain('empty=')
+    expect(key).not.toContain('undef=')
+  })
+
+  it('joins array query params deterministically', () => {
+    const key = buildMetricsCacheKey(path, { since: ['2024-01-01'], tag: ['a','b'] }, 'token y')
+    expect(key).toContain('since=2024-01-01')
+    expect(key).toMatch(/tag=a%2Cb|tag=b%2Ca/) // order preserved from input array join
+  })
+
+  it('merges existing path query params with provided query object without duplication', () => {
+    const pathWithQuery = '/api/metrics?since=2024-01-01&scope=organization'
+    const key = buildMetricsCacheKey(pathWithQuery, { githubOrg: 'test-org', since: '2024-01-01' }, 'token z')
+    // should not duplicate since param and should include githubOrg
+    const pieces = key.split(':')
+    expect(pieces.length).toBeGreaterThan(1)
+    const qs = pieces.slice(1).join(':').split('?')[1]
+    expect(qs?.match(/since=/g)?.length).toBe(1)
+    expect(qs).toContain('githubOrg=test-org')
+  })
+
+  it('sorts final query params for stable ordering', () => {
+    const key1 = buildMetricsCacheKey('/api/metrics?b=2&a=1', { c: '3' }, 'token sort')
+    const key2 = buildMetricsCacheKey('/api/metrics?a=1&b=2', { c: '3' }, 'token sort')
+    expect(key1).toBe(key2)
+  const qs = key1.split('?')[1]
+  expect(qs).toBeDefined()
+  expect(qs!.startsWith('a=1&b=2&c=3')).toBe(true)
+  })
+})

tests/metrics-cache.nuxt.spec.ts

@@ -0,0 +1,67 @@
+// @vitest-environment nuxt
+import { describe, it, expect, beforeEach } from 'vitest'
+
+describe('Metrics Cache Key Generation', () => {
+  it('should create unique cache keys for different query parameters', () => {
+    // Test the cache key logic that was implemented
+    const createCacheKey = (path: string, query: Record<string, string>) => {
+      const queryString = new URLSearchParams(query).toString()
+      return queryString ? `${path}?${queryString}` : path
+    }
+
+    const path = '/api/metrics'
+    
+    // Different date ranges should create different cache keys
+    const query1 = { since: '2024-01-01', until: '2024-01-31', scope: 'organization', githubOrg: 'test-org' }
+    const query2 = { since: '2024-02-01', until: '2024-02-28', scope: 'organization', githubOrg: 'test-org' }
+    const query3 = { since: '2024-01-01', until: '2024-01-31', scope: 'organization', githubOrg: 'test-org' }
+
+    const key1 = createCacheKey(path, query1)
+    const key2 = createCacheKey(path, query2)
+    const key3 = createCacheKey(path, query3)
+
+    // Different date ranges should have different keys
+    expect(key1).not.toBe(key2)
+    
+    // Same parameters should have same key
+    expect(key1).toBe(key3)
+    
+    // Keys should include query parameters
+    expect(key1).toContain('since=2024-01-01')
+    expect(key1).toContain('until=2024-01-31')
+    expect(key2).toContain('since=2024-02-01')
+    expect(key2).toContain('until=2024-02-28')
+  })
+
+  it('should handle empty query parameters', () => {
+    const createCacheKey = (path: string, query: Record<string, string>) => {
+      const queryString = new URLSearchParams(query).toString()
+      return queryString ? `${path}?${queryString}` : path
+    }
+
+    const path = '/api/metrics'
+    const emptyQuery = {}
+    
+    const key = createCacheKey(path, emptyQuery)
+    expect(key).toBe(path)
+  })
+
+  it('should handle undefined query values', () => {
+    const createCacheKey = (path: string, query: Record<string, any>) => {
+      // Filter out undefined values before creating query string
+      const filteredQuery = Object.fromEntries(
+        Object.entries(query).filter(([_, value]) => value !== undefined)
+      )
+      const queryString = new URLSearchParams(filteredQuery).toString()
+      return queryString ? `${path}?${queryString}` : path
+    }
+
+    const path = '/api/metrics'
+    const queryWithUndefined = { since: '2024-01-01', until: undefined, scope: 'organization' }
+    
+    const key = createCacheKey(path, queryWithUndefined)
+    expect(key).toContain('since=2024-01-01')
+    expect(key).toContain('scope=organization')
+    expect(key).not.toContain('until=')
+  })
+})

tests/test.setup.ts

@@ -0,0 +1,38 @@
+import * as urlLib from 'url';
+
+// Global test setup
+// Force mock mode so server-side handlers use local mock data instead of hitting GitHub APIs.
+process.env.NUXT_PUBLIC_IS_DATA_MOCKED = 'true';
+
+function isGitHub(url: string): boolean {
+    const host = urlLib.parse(url).host;
+    return host === 'api.github.com' || host === 'api.github.com:443';
+}
+
+// Block accidental real GitHub API calls if mock mode logic fails.
+const originalFetch: typeof globalThis.fetch | undefined = globalThis.fetch;
+if (originalFetch) {
+    globalThis.fetch = (async (...args: Parameters<typeof fetch>): Promise<Response> => {
+        const url = String(args[0]);
+        if (isGitHub(url)) {
+            throw new Error(`Blocked external GitHub API call during tests: ${url}`);
+        }
+        return originalFetch(...args);
+    }) as typeof fetch;
+}
+
+// Stub $fetch (ofetch) similarly if present at runtime.
+if (typeof globalThis.$fetch === 'function') {
+    const original$fetch = globalThis.$fetch;
+    const wrapped: typeof globalThis.$fetch = ((url: unknown, opts: unknown) => {
+        const str = String(url);
+        if (isGitHub(str)) {
+            return Promise.reject(new Error(`Blocked external GitHub API call during tests via $fetch: ${str}`));
+        }
+        return original$fetch(url as never, opts as never);
+    }) as typeof globalThis.$fetch;
+    // Preserve special properties if present
+    (wrapped as unknown as { raw?: unknown }).raw = (original$fetch as unknown as { raw?: () => unknown }).raw?.bind(original$fetch);
+    (wrapped as unknown as { create?: unknown }).create = (original$fetch as unknown as { create?: () => unknown }).create?.bind(original$fetch);
+    globalThis.$fetch = wrapped;
+}

vitest.config.ts

@@ -5,6 +5,7 @@ export default defineVitestConfig({
   test: {
     exclude: ['**/node_modules/**', '**/e2e-tests/**'],
     environment: 'nuxt',
-    globals: true // Use describe, test/expect, etc. without importing
+    globals: true, // Use describe, test/expect, etc. without importing
+    setupFiles: ['./tests/test.setup.ts']
   }
 })