diff --git a/apps/backend/controllers/library.controller.ts b/apps/backend/controllers/library.controller.ts index b758b85d..f6d02628 100644 --- a/apps/backend/controllers/library.controller.ts +++ b/apps/backend/controllers/library.controller.ts @@ -307,12 +307,43 @@ export const getRotation: RequestHandler = async (req, res) => { }; export type RotationAddRequest = Omit; + +/** + * Pick only the fields the client is allowed to write through the public + * `POST /library/rotation` endpoint (BS#1380). Mirrors + * `pickUpdateEntryFields()` in flowsheet.controller.ts (BS#1099). + * + * Server-derived columns (`legacy_rotation_id`, `legacy_library_release_id`, + * `discogs_release_id`, `discogs_release_id_source`, `lml_identity_id`, + * `tracklist_lookup_attempted_at`, `kill_date`) and tubafrenzy-ETL-only + * snapshot columns (`add_date`, `artist_name`, `album_title`, + * `record_label`) must never be client-supplied through this endpoint — + * `addToRotation` derives the LML-handle columns from `library_identity` + * and the synchronous `resolveIdentity` hop, and tubafrenzy is the only + * legitimate source for the snapshot columns. + * + * Phrased as an allowlist (signature-typed accept list) so a future column + * addition to `rotation` is implicitly rejected by typecheck until + * explicitly added to the signature. Matches dj-site's `RotationParams` + * (`{ album_id, rotation_bin }`); widen the signature here when a future + * caller legitimately needs another field. + */ +type AddRotationAllowlist = Pick; + +export function pickAddRotationFields(body: Partial): AddRotationAllowlist { + const picked = {} as AddRotationAllowlist; + if (body.album_id !== undefined) picked.album_id = body.album_id; + if (body.rotation_bin !== undefined) picked.rotation_bin = body.rotation_bin; + return picked; +} + export const addRotation: RequestHandler = async (req, res) => { if (req.body.album_id === undefined || req.body.rotation_bin === undefined) { throw new WxycError('Missing Parameters: album_id or rotation_bin', 400); } - const rotationRelease: RotationRelease = await libraryService.addToRotation(req.body); + const picked = pickAddRotationFields(req.body); + const rotationRelease: RotationRelease = await libraryService.addToRotation(picked); res.status(201).json(rotationRelease); }; diff --git a/apps/backend/services/library.service.ts b/apps/backend/services/library.service.ts index f0f86d5a..f3f8e36c 100644 --- a/apps/backend/services/library.service.ts +++ b/apps/backend/services/library.service.ts @@ -36,6 +36,7 @@ import { isLmlConfigured, envInt, LmlClientError, + resolveIdentity, type LookupResponse, type DiscogsTrackItem, type DiscogsReleaseMetadata, @@ -340,8 +341,136 @@ export const getRotationFromDB = async (): Promise => { return rows.map((row) => serializeReconciledIdentity(row)); }; +/** + * BS#1380: Sentry counter `lml.resolve.fallback_to_null` reason buckets. + * Each value signals a different operational response: + * - `timeout`: LML latency/capacity (look at LML p95) + * - `5xx`: LML deploy regression (look at LML release notes) + * - `4xx`: BS-side caller bug (the library_identity row pointed at + * a sentinel ID LML rejects, etc.) + * - `network`: infra noise (transient TCP/DNS failures) + * - `other`: uncategorised — investigate per-event + * + * Surfaced separately so a per-bucket regression doesn't get masked by the + * rollup. Revisit (split the knob or tune the value) if Sentry shows p95 + * user-facing add-to-rotation > 2.5 s OR fallback rate > 5% in any bucket. + */ +export type LmlResolveFallbackReason = 'timeout' | '5xx' | '4xx' | 'network' | 'other'; + +/** + * Classify the error caught from `resolveIdentity(...)` into one of the + * five operational buckets above. Pulled out so the unit test can + * exhaustively exercise every branch without spinning up an LML server. + * + * `LmlClientError` is the wrapper `lmlFetch` throws; its `statusCode` was + * already translated upstream (5xx → 502, AbortError → 504). The catch + * here untranslates so the Sentry attribute reads as the original + * operational signal. + */ +export function classifyLmlResolveError(err: unknown): LmlResolveFallbackReason { + if (!err || typeof err !== 'object') return 'other'; + const e = err as { name?: string; code?: string; statusCode?: number; message?: string }; + if (e.name === 'AbortError') return 'timeout'; + if (err instanceof LmlClientError) { + // `lmlFetch` translates `AbortError` → 504 and upstream 5xx → 502; restore + // the operational bucket from those translations + the raw status range. + if (e.statusCode === 504) return 'timeout'; + if (e.statusCode === 502) { + // 502 is overloaded: it's the wrapper for both upstream 5xx and a raw + // fetch failure (the catch-all in `lmlFetch`). Disambiguate from the + // message — "LML request failed" is the fetch-threw path. + return typeof e.message === 'string' && e.message.startsWith('LML request failed') ? 'network' : '5xx'; + } + if (typeof e.statusCode === 'number') { + if (e.statusCode >= 500) return '5xx'; + if (e.statusCode >= 400) return '4xx'; + } + return 'other'; + } + if (e.code === 'ECONNRESET' || e.code === 'ENOTFOUND' || e.code === 'ECONNREFUSED') { + return 'network'; + } + return 'other'; +} + +/** + * Add an album to rotation (dj-site path, `POST /library/rotation`). + * + * Synchronously triple-writes `(discogs_release_id, discogs_release_id_source = + * 'library_identity', lml_identity_id)` when the album's `library_identity` + * row supplies a non-NULL `discogs_release_id` (BS#1380): + * + * 1. Read `library_identity.discogs_release_id` for the requested + * `album_id`. `library_identity.library_id` is the PRIMARY KEY + * (see schema.ts:1391-1393); `LIMIT 1` is defensive. + * 2. If non-NULL, `await resolveIdentity({kind:'release', + * source:'discogs_release', external_id})` to mint a stable + * `lml_identity_id`. The hop runs inside this handler — the response + * row carries the populated columns without needing a follow-up + * patch. + * 3. INSERT all three columns. On resolve failure (timeout / 5xx / + * network), `lml_identity_id` falls back to NULL but + * `discogs_release_id` + `discogs_release_id_source = 'library_identity'` + * still land — the *source of the Discogs id* is library_identity + * regardless of whether the mint succeeded. The daily backfill cron + * catches up within ~24h. + * + * Rationale for synchronous-before-INSERT: dj-site's optimistic UI + * (WXYC/dj-site#648) covers the latency tail at the client side — the row + * appears in-rotation immediately on click. Blocking the music director on + * an LML outage is worse than a temporarily-incomplete row, so the catch + * arm proceeds rather than rethrowing. + */ export const addToRotation = async (newRotation: RotationAddRequest) => { - const insertedRotation: RotationRelease[] = await db.insert(rotation).values(newRotation).returning(); + const values: RotationAddRequest = { ...newRotation }; + + // Allowlist guard already runs at the controller layer, but the server- + // derived fields below must always come from this function, not the + // request — defense in depth. + if (values.album_id != null) { + const [identityRow] = await db + .select({ discogs_release_id: library_identity.discogs_release_id }) + .from(library_identity) + .where(eq(library_identity.library_id, values.album_id)) + .limit(1); + const discogsReleaseId = identityRow?.discogs_release_id ?? null; + + if (discogsReleaseId != null) { + // The source-of-the-Discogs-id is library_identity, regardless of + // whether the LML mint that follows succeeds. + values.discogs_release_id = discogsReleaseId; + values.discogs_release_id_source = 'library_identity'; + + let lmlIdentityId: number | null = null; + try { + const resolved = await resolveIdentity({ + kind: 'release', + source: 'discogs_release', + external_id: String(discogsReleaseId), + }); + lmlIdentityId = resolved.identity_id; + } catch (err) { + const reason = classifyLmlResolveError(err); + // BS#1380: classify once at fallback time so each reason bucket + // signals a different operational response (timeout → LML + // latency, 5xx → LML deploy regression, 4xx → BS-side caller bug, + // network → infra noise). v10 SDK shape — `Sentry.metrics.count( + // name, value, {attributes})`. Pre-v8 + // `Sentry.metrics.increment(..., {tags})` was removed. + try { + Sentry.metrics.count('lml.resolve.fallback_to_null', 1, { + attributes: { caller: 'add_to_rotation', reason }, + }); + } catch { + // Observability must never break the request path; swallow. + } + // lml_identity_id stays NULL; INSERT proceeds. + } + values.lml_identity_id = lmlIdentityId; + } + } + + const insertedRotation: RotationRelease[] = await db.insert(rotation).values(values).returning(); return insertedRotation[0]; }; diff --git a/tests/integration/library.spec.js b/tests/integration/library.spec.js index 7016da5e..137a068a 100644 --- a/tests/integration/library.spec.js +++ b/tests/integration/library.spec.js @@ -346,6 +346,61 @@ describe('Library Rotation', () => { expectErrorContains(res, 'Missing Parameters'); }); + + // BS#1380 — pickAddRotationFields allowlist. A client that includes + // server-derived (`discogs_release_id`, `discogs_release_id_source`, + // `lml_identity_id`, `legacy_rotation_id`, `legacy_library_release_id`, + // `tracklist_lookup_attempted_at`, `kill_date`) or ETL-only + // (`add_date`, `artist_name`, `album_title`, `record_label`) fields + // in the request body must not be able to set them through this + // endpoint. Persisted row must reflect server defaults / NULL. + test('allowlist drops client-supplied non-allowlisted fields', async () => { + const res = await auth + .post('/library/rotation') + .send({ + album_id: 3, + rotation_bin: 'L', + // Forbidden fields the allowlist must drop. + discogs_release_id: 999777, + discogs_release_id_source: 'discogs_direct_backfill', + lml_identity_id: 8888888, + legacy_rotation_id: 555_000_001, + legacy_library_release_id: 555_000_002, + kill_date: '2027-01-01', + artist_name: 'Forged Artist', + album_title: 'Forged Album', + record_label: 'Forged Label', + }) + .expect(201); + + // Server-derived fields land NULL when library_identity has no row + // (seed_db.sql ships no library_identity rows for album_id=3) and + // operator-only fields stay NULL. + expect(res.body.discogs_release_id).toBeNull(); + expect(res.body.lml_identity_id).toBeNull(); + expect(res.body.legacy_rotation_id).toBeNull(); + expect(res.body.legacy_library_release_id).toBeNull(); + expect(res.body.kill_date).toBeNull(); + // ETL-only denormalized snapshot fields stay NULL. + expect(res.body.artist_name).toBeNull(); + expect(res.body.album_title).toBeNull(); + expect(res.body.record_label).toBeNull(); + // The two allowed fields land verbatim. + expect(res.body.album_id).toBe(3); + expect(res.body.rotation_bin).toBe('L'); + // discogs_release_id_source falls back to the column default + // ('tubafrenzy_paste') when addToRotation doesn't supply + // library_identity. This is acceptable today for the no- + // library_identity path; the BS#1380 plan is honest about it + // (the library_identity branch tags 'library_identity' explicitly, + // the no-library_identity branch leaves the column default). + expect(res.body.discogs_release_id_source).toBe('tubafrenzy_paste'); + + // Clean up. + if (res.body.id) { + await auth.patch('/library/rotation').send({ rotation_id: res.body.id }); + } + }); }); describe('PATCH /library/rotation (Kill Rotation)', () => { diff --git a/tests/unit/services/library.service.addToRotation.test.ts b/tests/unit/services/library.service.addToRotation.test.ts new file mode 100644 index 00000000..f263cd1e --- /dev/null +++ b/tests/unit/services/library.service.addToRotation.test.ts @@ -0,0 +1,278 @@ +/** + * Unit tests for `addToRotation` + `classifyLmlResolveError` (BS#1380). + * + * Two surfaces under test: + * + * 1. `classifyLmlResolveError(err)` — pure function. Every catch-block + * branch (timeout / 5xx / 4xx / network / other) maps to the right + * `LmlResolveFallbackReason`. The Sentry counter `lml.resolve + * .fallback_to_null` carries this value as an attribute, and a + * per-bucket regression dashboard depends on the classifier producing + * the same string the prose contract promises. Exhaustive coverage + * lives here so a future refactor doesn't quietly collapse two buckets + * together. + * + * 2. `addToRotation` — when `library_identity.discogs_release_id` is + * non-NULL, the row is triple-written (discogs_release_id, + * discogs_release_id_source='library_identity', lml_identity_id). + * On resolve failure the first two still land; `lml_identity_id` is + * NULL. The Sentry counter fires once with `caller=add_to_rotation` + * and the classified `reason`. + * + * Tests don't reach the integration layer — Drizzle is mocked via the + * established `database.mock` so we can assert on the INSERT values + * directly. + */ + +import { jest } from '@jest/globals'; +import { db, createMockQueryChain, rotation, library_identity } from '../../mocks/database.mock'; + +// Mirror the real `LmlClientError` shape so the classifier's `err instanceof +// LmlClientError` branch fires. The wrapper sets `statusCode` to one of +// {422, 502, 504, ...} after translating from the upstream response. +class MockLmlClientError extends Error { + statusCode: number; + constructor(message: string, statusCode: number) { + super(message); + this.name = 'LmlClientError'; + this.statusCode = statusCode; + } +} + +const mockResolveIdentity = jest.fn<() => Promise<{ identity_id: number; kind: 'release'; minted: boolean }>>(); +const mockLookupMetadata = jest.fn<() => Promise>(); +const mockLookupBySong = jest.fn<() => Promise>(); +const mockIsLmlConfigured = jest.fn<() => boolean>(); +const mockGetRelease = jest.fn<() => Promise>(); + +jest.mock('@wxyc/lml-client', () => ({ + resolveIdentity: mockResolveIdentity, + lookupMetadata: mockLookupMetadata, + lookupBySong: mockLookupBySong, + isLmlConfigured: mockIsLmlConfigured, + getRelease: mockGetRelease, + envInt: (_name: string, fallback: number) => fallback, + LmlClientError: MockLmlClientError, +})); + +jest.mock('../../../apps/backend/services/lml/lookup-coordinator', () => ({ + lmlLookupCoordinator: { lookup: () => Promise.resolve(null) }, +})); + +const mockSentryMetricsCount = jest.fn<(name: string, value: number, opts: unknown) => void>(); +jest.mock('@sentry/node', () => ({ + startSpan: (_opts: unknown, callback: () => T | Promise): Promise => Promise.resolve(callback()), + getActiveSpan: () => ({ setAttribute: jest.fn(), setAttributes: jest.fn() }), + metrics: { count: mockSentryMetricsCount }, +})); + +import { addToRotation, classifyLmlResolveError } from '../../../apps/backend/services/library.service'; + +describe('classifyLmlResolveError', () => { + test('AbortError → timeout', () => { + const err = new Error('aborted'); + err.name = 'AbortError'; + expect(classifyLmlResolveError(err)).toBe('timeout'); + }); + + test('LmlClientError statusCode 504 → timeout (lmlFetch translates AbortError)', () => { + const err = new MockLmlClientError('LML request timed out', 504); + expect(classifyLmlResolveError(err)).toBe('timeout'); + }); + + test('LmlClientError statusCode 502 with upstream-5xx message → 5xx', () => { + // `lmlFetch` translates upstream 5xx to 502 via `LmlClientError` + // (`response.status >= 500 ? 502 : response.status`). The classifier + // disambiguates that from a fetch-threw 502 (also wrapped) via the + // message prefix `LML responded with`. + const err = new MockLmlClientError('LML responded with 503: Service Unavailable', 502); + expect(classifyLmlResolveError(err)).toBe('5xx'); + }); + + test('LmlClientError statusCode 502 with fetch-threw message → network', () => { + // The fetch-catch path in `lmlFetch` raises + // `LmlClientError('LML request failed: ', 502)`. That's the + // "network" bucket — operator should look at DNS / TCP, not at LML's + // deploy state. + const err = new MockLmlClientError('LML request failed: ECONNRESET', 502); + expect(classifyLmlResolveError(err)).toBe('network'); + }); + + test('LmlClientError statusCode 422 → 4xx (sentinel rejection)', () => { + const err = new MockLmlClientError('Discogs id <= 0 rejected', 422); + expect(classifyLmlResolveError(err)).toBe('4xx'); + }); + + test('LmlClientError statusCode 400 → 4xx', () => { + const err = new MockLmlClientError('Bad Request', 400); + expect(classifyLmlResolveError(err)).toBe('4xx'); + }); + + test('LmlClientError statusCode 500 → 5xx (raw, not via lmlFetch translation)', () => { + const err = new MockLmlClientError('Internal Server Error', 500); + expect(classifyLmlResolveError(err)).toBe('5xx'); + }); + + test('Node ECONNRESET → network', () => { + const err = Object.assign(new Error('socket hang up'), { code: 'ECONNRESET' }); + expect(classifyLmlResolveError(err)).toBe('network'); + }); + + test('Node ENOTFOUND → network', () => { + const err = Object.assign(new Error('getaddrinfo ENOTFOUND lml'), { code: 'ENOTFOUND' }); + expect(classifyLmlResolveError(err)).toBe('network'); + }); + + test('Node ECONNREFUSED → network', () => { + const err = Object.assign(new Error('connect ECONNREFUSED'), { code: 'ECONNREFUSED' }); + expect(classifyLmlResolveError(err)).toBe('network'); + }); + + test('plain Error → other', () => { + expect(classifyLmlResolveError(new Error('something else'))).toBe('other'); + }); + + test('non-Error throwables → other', () => { + expect(classifyLmlResolveError('a string')).toBe('other'); + expect(classifyLmlResolveError(null)).toBe('other'); + expect(classifyLmlResolveError(undefined)).toBe('other'); + expect(classifyLmlResolveError(42)).toBe('other'); + }); +}); + +describe('addToRotation (BS#1380)', () => { + const ALBUM_ID = 100; + const DISCOGS_RELEASE_ID = 12345; + const LML_IDENTITY_ID = 7700100; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + test('library_identity hit + LML resolve success → triple-write (discogs_release_id, source, lml_identity_id)', async () => { + // Library-identity row supplies a non-NULL discogs_release_id. + const selectChain = createMockQueryChain([{ discogs_release_id: DISCOGS_RELEASE_ID }]); + selectChain.limit = jest.fn().mockResolvedValue([{ discogs_release_id: DISCOGS_RELEASE_ID }]); + db.select.mockReturnValue(selectChain); + + // LML mints/returns a stable identity_id. + mockResolveIdentity.mockResolvedValue({ identity_id: LML_IDENTITY_ID, kind: 'release', minted: true }); + + // INSERT returns the persisted row. + const insertChain = createMockQueryChain([ + { + id: 1, + album_id: ALBUM_ID, + rotation_bin: 'M', + discogs_release_id: DISCOGS_RELEASE_ID, + discogs_release_id_source: 'library_identity', + lml_identity_id: LML_IDENTITY_ID, + }, + ]); + db.insert.mockReturnValue(insertChain); + + const result = await addToRotation({ album_id: ALBUM_ID, rotation_bin: 'M' }); + + expect(mockResolveIdentity).toHaveBeenCalledWith({ + kind: 'release', + source: 'discogs_release', + external_id: String(DISCOGS_RELEASE_ID), + }); + + // The INSERT was called with the three triple-write fields populated. + const valuesArg = insertChain.values.mock.calls[0][0] as Record; + expect(valuesArg.album_id).toBe(ALBUM_ID); + expect(valuesArg.rotation_bin).toBe('M'); + expect(valuesArg.discogs_release_id).toBe(DISCOGS_RELEASE_ID); + expect(valuesArg.discogs_release_id_source).toBe('library_identity'); + expect(valuesArg.lml_identity_id).toBe(LML_IDENTITY_ID); + + expect(result?.lml_identity_id).toBe(LML_IDENTITY_ID); + expect(mockSentryMetricsCount).not.toHaveBeenCalled(); + }); + + test('library_identity hit + LML resolve failure → discogs_release_id + source still land, lml_identity_id = NULL, counter fires', async () => { + const selectChain = createMockQueryChain([{ discogs_release_id: DISCOGS_RELEASE_ID }]); + selectChain.limit = jest.fn().mockResolvedValue([{ discogs_release_id: DISCOGS_RELEASE_ID }]); + db.select.mockReturnValue(selectChain); + + // Simulate LML timeout — `lmlFetch` raises `LmlClientError(..., 504)`. + mockResolveIdentity.mockRejectedValue(new MockLmlClientError('LML request timed out', 504)); + + const insertChain = createMockQueryChain([ + { + id: 2, + album_id: ALBUM_ID, + rotation_bin: 'L', + discogs_release_id: DISCOGS_RELEASE_ID, + discogs_release_id_source: 'library_identity', + lml_identity_id: null, + }, + ]); + db.insert.mockReturnValue(insertChain); + + const result = await addToRotation({ album_id: ALBUM_ID, rotation_bin: 'L' }); + + const valuesArg = insertChain.values.mock.calls[0][0] as Record; + // The source-of-the-Discogs-id is still library_identity (the issue's + // explicit contract — provenance is honest even when the mint fails). + expect(valuesArg.discogs_release_id).toBe(DISCOGS_RELEASE_ID); + expect(valuesArg.discogs_release_id_source).toBe('library_identity'); + expect(valuesArg.lml_identity_id).toBeNull(); + + expect(result?.lml_identity_id).toBeNull(); + + // Counter fires once with the correct reason bucket. + expect(mockSentryMetricsCount).toHaveBeenCalledTimes(1); + expect(mockSentryMetricsCount).toHaveBeenCalledWith('lml.resolve.fallback_to_null', 1, { + attributes: { caller: 'add_to_rotation', reason: 'timeout' }, + }); + }); + + test('library_identity miss (no row, or row with NULL discogs_release_id) → no LML call, no triple-write', async () => { + // The library_identity lookup returns no row. + const selectChain = createMockQueryChain([]); + selectChain.limit = jest.fn().mockResolvedValue([]); + db.select.mockReturnValue(selectChain); + + const insertChain = createMockQueryChain([{ id: 3, album_id: ALBUM_ID, rotation_bin: 'H' }]); + db.insert.mockReturnValue(insertChain); + + await addToRotation({ album_id: ALBUM_ID, rotation_bin: 'H' }); + + expect(mockResolveIdentity).not.toHaveBeenCalled(); + + const valuesArg = insertChain.values.mock.calls[0][0] as Record; + // Server doesn't supply any of the LML-handle fields. The column + // default ('tubafrenzy_paste') applies to discogs_release_id_source + // when not set, but BS#1380's acceptance criterion is that we do NOT + // tag dj-site rows as if they came from tubafrenzy. So for the + // no-library-identity path, we leave the field unset — the default + // 'tubafrenzy_paste' applies but no LML resolve fires and no + // discogs_release_id lands either. + expect(valuesArg.discogs_release_id).toBeUndefined(); + expect(valuesArg.discogs_release_id_source).toBeUndefined(); + expect(valuesArg.lml_identity_id).toBeUndefined(); + }); + + test('reads library_identity by library_id (PRIMARY KEY)', async () => { + // Defensive: confirms we're using the schema's PRIMARY KEY column, + // not falling back to a wrong join key. The library_identity table's + // PRIMARY KEY is library_id (schema.ts:1391-1393). + const selectChain = createMockQueryChain([{ discogs_release_id: null }]); + selectChain.limit = jest.fn().mockResolvedValue([{ discogs_release_id: null }]); + db.select.mockReturnValue(selectChain); + + const insertChain = createMockQueryChain([{ id: 4 }]); + db.insert.mockReturnValue(insertChain); + + await addToRotation({ album_id: ALBUM_ID, rotation_bin: 'M' }); + + // The select chain reads from library_identity. + expect(db.select).toHaveBeenCalled(); + expect(selectChain.from).toHaveBeenCalledWith(library_identity); + expect(insertChain.values).toHaveBeenCalled(); + // INSERT target was rotation (not library_identity). + expect(db.insert).toHaveBeenCalledWith(rotation); + }); +});