From b0041f34ba7034835a1d018fc1d775e42a8ac69b Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 27 Jul 2026 23:03:59 +0200 Subject: [PATCH] fix(server): fix the quirks preserved by the exchange-rates DI migration - the Frankfurter fetch now carries an AbortSignal timeout (10s) and a 1 MB response-size cap, and the body is boundary-validated through a type guard instead of an as-cast - fetch failures are logged instead of silently swallowed (callers still get the graceful null) - the inflight-coalescing cleanup moved into .finally so a rejected fetch can no longer leak the key - the dead convertWithRates export (zero call sites) is dropped from the service and the bridge Kept on purpose: the beyond-TTL stale-cache fallback, the >1-keys empty-response heuristic and the || falsy-coercion defaults. --- .../src/nest/budget/exchange-rates.bridge.ts | 18 ++--- .../src/nest/budget/exchange-rates.service.ts | 72 ++++++++++--------- .../unit/nest/exchange-rates.service.test.ts | 69 +++++++----------- 3 files changed, 69 insertions(+), 90 deletions(-) diff --git a/server/src/nest/budget/exchange-rates.bridge.ts b/server/src/nest/budget/exchange-rates.bridge.ts index db66ddf5a..3ce43510f 100644 --- a/server/src/nest/budget/exchange-rates.bridge.ts +++ b/server/src/nest/budget/exchange-rates.bridge.ts @@ -5,9 +5,11 @@ import { ExchangeRatesService } from './exchange-rates.service'; * the Nest container (the legacy `services/budgetService.ts` FX-freeze/rebase * seams and the legacy `get_settlement_summary` registrar in * src/mcp/tools/budget.ts; the plugin RPC host injects ExchangeRatesService via - * PluginHostDepsFactory). Exports the legacy services/exchangeRateService names - * 1:1, so repointing a consumer is an import-path-only diff. Inside the - * container, inject ExchangeRatesService instead. + * PluginHostDepsFactory). Exports only the legacy services/exchangeRateService + * names still consumed outside the container (getRates — the dead + * convertWithRates export was dropped in the post-fold quirk fixes), so + * repointing a consumer is an import-path-only diff. Inside the container, + * inject ExchangeRatesService instead. * Delete this file when the budgetService migration lands. * * Module-level construction is safe: the service has no dependencies and no @@ -20,13 +22,3 @@ const fx = new ExchangeRatesService(); export function getRates(base: string) { return fx.getRates(base); } - -/** Unconsumed today (no call sites) — re-exported for 1:1 legacy surface parity. */ -export function convertWithRates( - amount: number, - from: string | null | undefined, - base: string, - rates: Record | null, -): number { - return fx.convertWithRates(amount, from, base, rates); -} diff --git a/server/src/nest/budget/exchange-rates.service.ts b/server/src/nest/budget/exchange-rates.service.ts index 01079671d..4b905a3d8 100644 --- a/server/src/nest/budget/exchange-rates.service.ts +++ b/server/src/nest/budget/exchange-rates.service.ts @@ -12,32 +12,53 @@ import { Injectable } from '@nestjs/common'; * callers get `null`/identity conversion and amounts are treated as already in * the base currency rather than throwing. * - * Moved 1:1 from the legacy `services/exchangeRateService.ts` (the budget-domain - * fold): identical fetch, cache/coalescing behavior, `||` falsy-coercion - * defaults and null degradation. The cache and inflight maps are deliberately - * MODULE-scoped, not instance fields — the DI singleton and the - * exchange-rates.bridge instance wrap the same upstream feed, so they must - * share one cache (same reasoning as the permissions.service.ts cache). + * Moved from the legacy `services/exchangeRateService.ts` (the budget-domain + * fold): identical cache/coalescing behavior, `||` falsy-coercion defaults and + * null degradation. The cache and inflight maps are deliberately MODULE-scoped, + * not instance fields — the DI singleton and the exchange-rates.bridge instance + * wrap the same upstream feed, so they must share one cache (same reasoning as + * the permissions.service.ts cache). + * + * Post-migration fixes on top of the relocated legacy behavior: the outbound + * fetch carries an AbortSignal timeout and a response-size cap, the response is + * boundary-validated instead of `as`-cast, failures are logged instead of + * silently swallowed (callers still get null), and the inflight cleanup is + * rejection-proof. Kept on purpose: the stale-cache fallback on upstream + * failure (stale beats nothing) and the ">1 keys" empty-response heuristic. */ const TTL_MS = 6 * 60 * 60 * 1000; // 6h +const FETCH_TIMEOUT_MS = 10_000; +// Frankfurter quotes a few dozen currencies (~2 KB); anything near this cap is +// not the API we think it is. +const MAX_RESPONSE_BYTES = 1024 * 1024; const cache = new Map; ts: number }>(); const inflight = new Map | null>>(); +const isRateEntry = (v: unknown): v is { quote: string; rate: number } => + typeof v === 'object' && v !== null && + typeof (v as { quote?: unknown }).quote === 'string' && + typeof (v as { rate?: unknown }).rate === 'number'; + async function fetchRates(base: string): Promise | null> { try { - const res = await fetch(`https://api.frankfurter.dev/v2/rates?base=${encodeURIComponent(base)}`); + const res = await fetch(`https://api.frankfurter.dev/v2/rates?base=${encodeURIComponent(base)}`, { + signal: AbortSignal.timeout(FETCH_TIMEOUT_MS), + }); if (!res.ok) return null; + const text = await res.text(); + if (text.length > MAX_RESPONSE_BYTES) return null; // Frankfurter returns an array of { date, base, quote, rate } and omits the // base's own self-rate, so seed the map with `base = 1` then index by quote. - const data = (await res.json()) as Array<{ quote?: string; rate?: number }>; + const data: unknown = JSON.parse(text); if (!Array.isArray(data)) return null; const rates: Record = { [base.toUpperCase()]: 1 }; for (const r of data) { - if (r && typeof r.quote === 'string' && typeof r.rate === 'number') rates[r.quote] = r.rate; + if (isRateEntry(r)) rates[r.quote] = r.rate; } return Object.keys(rates).length > 1 ? rates : null; - } catch { + } catch (err) { + console.error('[exchange-rates] rates fetch failed for', base, err); return null; } } @@ -54,11 +75,14 @@ export class ExchangeRatesService { // Coalesce concurrent fetches for the same base. let p = inflight.get(key); if (!p) { - p = fetchRates(key).then(rates => { - if (rates) cache.set(key, { rates, ts: Date.now() }); - inflight.delete(key); - return rates; - }); + p = fetchRates(key) + .then(rates => { + if (rates) cache.set(key, { rates, ts: Date.now() }); + return rates; + }) + .finally(() => { + inflight.delete(key); + }); inflight.set(key, p); } const rates = await p; @@ -66,22 +90,4 @@ export class ExchangeRatesService { if (!rates && hit) return hit.rates; return rates; } - - /** - * Convert `amount` from `from` currency into `base` using a rates map obtained - * from getRates(base). Identity when same currency or the rate is missing. - */ - convertWithRates( - amount: number, - from: string | null | undefined, - base: string, - rates: Record | null, - ): number { - const fromCur = (from || base).toUpperCase(); - const baseCur = base.toUpperCase(); - if (fromCur === baseCur || !rates) return amount; - const r = rates[fromCur]; - if (!r || r <= 0) return amount; - return amount / r; - } } diff --git a/server/tests/unit/nest/exchange-rates.service.test.ts b/server/tests/unit/nest/exchange-rates.service.test.ts index e7b8f042b..906420840 100644 --- a/server/tests/unit/nest/exchange-rates.service.test.ts +++ b/server/tests/unit/nest/exchange-rates.service.test.ts @@ -1,50 +1,55 @@ /** * Unit tests for the DI-native ExchangeRatesService — FX-SVC-001 through - * FX-SVC-020. New suite: the legacy services/exchangeRateService.ts had no - * dedicated tests; the budget-domain fold moves it inside the src/nest/** - * coverage gate. 001–018 pin the fetch/cache/convert behavior byte-for-byte - * (including the parity quirks: `|| 'EUR'` falsy coercion, stale-cache - * fallback, the `>1 keys` failure heuristic, silent catch → null); 019–020 pin - * the exchange-rates.bridge delegation and the module-scoped cache shared - * between the DI instance and the bridge instance. + * FX-SVC-022. New suite: the legacy services/exchangeRateService.ts had no + * dedicated tests; the budget-domain fold moved it inside the src/nest/** + * coverage gate. 001–012 pin the fetch/cache behavior (including the parity + * quirks kept on purpose: `|| 'EUR'` falsy coercion, stale-cache fallback, the + * `>1 keys` failure heuristic); 019 pins the exchange-rates.bridge delegation + * and the module-scoped cache shared between the DI instance and the bridge + * instance; 021–022 pin the post-fold quirk fixes (AbortSignal timeout, + * response-size cap, logged failures). 013–018 and 020 covered the dead + * convertWithRates export and were removed with it in the quirk fixes. * * The rate cache is deliberately MODULE-scoped, so it persists across tests in * this file — every case uses its own base currency to stay isolated. */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.service'; -import { getRates as bridgeGetRates, convertWithRates as bridgeConvertWithRates } from '../../../src/nest/budget/exchange-rates.bridge'; +import { getRates as bridgeGetRates } from '../../../src/nest/budget/exchange-rates.bridge'; const TTL_MS = 6 * 60 * 60 * 1000; // mirrors the service's 6h TTL // A minimal Frankfurter-shaped success response (array of { quote, rate }). -const okResponse = (data: unknown) => ({ ok: true, json: async () => data }); +const okResponse = (data: unknown) => ({ ok: true, text: async () => JSON.stringify(data) }); const svc = new ExchangeRatesService(); let fetchMock: ReturnType; +let errorSpy: ReturnType; beforeEach(() => { fetchMock = vi.fn(async () => okResponse([{ quote: 'USD', rate: 1.08 }])); vi.stubGlobal('fetch', fetchMock); + errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); }); afterEach(() => { vi.unstubAllGlobals(); vi.useRealTimers(); + errorSpy.mockRestore(); }); describe('ExchangeRatesService.getRates', () => { it('FX-SVC-001: falls back to EUR for a falsy base (|| coercion, not ??)', async () => { const rates = await svc.getRates(''); - expect(fetchMock).toHaveBeenCalledWith('https://api.frankfurter.dev/v2/rates?base=EUR'); + expect(fetchMock.mock.calls[0][0]).toBe('https://api.frankfurter.dev/v2/rates?base=EUR'); expect(rates).toEqual({ EUR: 1, USD: 1.08 }); }); it('FX-SVC-002: upper-cases the base for the request and the self-rate seed', async () => { fetchMock.mockResolvedValueOnce(okResponse([{ quote: 'GBP', rate: 0.85 }])); const rates = await svc.getRates('usd'); - expect(fetchMock).toHaveBeenCalledWith('https://api.frankfurter.dev/v2/rates?base=USD'); + expect(fetchMock.mock.calls[0][0]).toBe('https://api.frankfurter.dev/v2/rates?base=USD'); expect(rates).toEqual({ USD: 1, GBP: 0.85 }); }); @@ -70,9 +75,10 @@ describe('ExchangeRatesService.getRates', () => { expect(await svc.getRates('SEK')).toBeNull(); }); - it('FX-SVC-006: returns null when fetch throws (silent catch, no logging)', async () => { + it('FX-SVC-006: returns null when fetch throws — and logs the failure (quirk fix)', async () => { fetchMock.mockRejectedValueOnce(new Error('offline')); expect(await svc.getRates('DKK')).toBeNull(); + expect(errorSpy).toHaveBeenCalledWith('[exchange-rates] rates fetch failed for', 'DKK', expect.any(Error)); }); it('FX-SVC-007: treats a response that yields only the self-rate as failure (>1 keys heuristic)', async () => { @@ -120,36 +126,16 @@ describe('ExchangeRatesService.getRates', () => { fetchMock.mockResolvedValueOnce({ ok: false, json: async () => [] }); expect(await svc.getRates('BGN')).toBeNull(); }); -}); -describe('ExchangeRatesService.convertWithRates', () => { - const rates = { EUR: 1, USD: 1.08, BAD: 0, NEG: -2 }; - - it('FX-SVC-013: identity when from equals base (case-insensitive)', () => { - expect(svc.convertWithRates(10, 'eur', 'EUR', rates)).toBe(10); + it('FX-SVC-021: sends the request with an abort-timeout signal (quirk fix)', async () => { + await svc.getRates('MXN'); + const opts = fetchMock.mock.calls[0][1] as { signal?: unknown } | undefined; + expect(opts?.signal).toBeInstanceOf(AbortSignal); }); - it('FX-SVC-014: identity when the rates map is null', () => { - expect(svc.convertWithRates(10, 'USD', 'EUR', null)).toBe(10); - }); - - it('FX-SVC-015: a falsy from means "already in base" (|| coercion) → identity', () => { - expect(svc.convertWithRates(10, null, 'EUR', rates)).toBe(10); - expect(svc.convertWithRates(10, undefined, 'EUR', rates)).toBe(10); - expect(svc.convertWithRates(10, '', 'EUR', rates)).toBe(10); - }); - - it('FX-SVC-016: identity when the from-currency has no rate', () => { - expect(svc.convertWithRates(10, 'JPY', 'EUR', rates)).toBe(10); - }); - - it('FX-SVC-017: identity for a zero or negative rate', () => { - expect(svc.convertWithRates(10, 'BAD', 'EUR', rates)).toBe(10); - expect(svc.convertWithRates(10, 'NEG', 'EUR', rates)).toBe(10); - }); - - it('FX-SVC-018: divides by the rate ("units of X per 1 base")', () => { - expect(svc.convertWithRates(10.8, 'USD', 'EUR', rates)).toBeCloseTo(10); + it('FX-SVC-022: treats an oversized response body as failure (quirk fix)', async () => { + fetchMock.mockResolvedValueOnce({ ok: true, text: async () => 'x'.repeat(1024 * 1024 + 1) }); + expect(await svc.getRates('BRL')).toBeNull(); }); }); @@ -162,9 +148,4 @@ describe('exchange-rates.bridge', () => { expect(await bridgeGetRates('AUD')).toBe(primed); expect(fetchMock).toHaveBeenCalledTimes(1); }); - - it('FX-SVC-020: convertWithRates delegates 1:1', () => { - expect(bridgeConvertWithRates(21.6, 'USD', 'EUR', { EUR: 1, USD: 1.08 })).toBeCloseTo(20); - expect(bridgeConvertWithRates(5, 'EUR', 'EUR', null)).toBe(5); - }); });