refactor(db): JSON columns are decoded through one declared schema per column

decodeJson/decodeJsonResult/encodeJson read and write a JSON-in-TEXT column against the schema and fallback declared for it in db/json-columns.ts, and log a row that does not decode. reservations.metadata, the OAuth scope and redirect lists, consents, API token scopes, MFA backup codes, journey entry tags and pros/cons and poll options go through it. Readers that used to throw on a broken row (a hotel update, the ICS export, the price mirror, the OAuth token and session reads, a consent, a journey entry, a poll) now take the tolerant fallback the other readers of the same column already used.
2026-10-21 14:36:31 +00:00 · 2026-10-10 12:13:56 +02:00
parent a926990e4a
commit 91deedae8b
26 changed files with 510 additions and 106 deletions
@@ -227,6 +227,23 @@ describe('updateAccommodation', () => {
    spy.mockRestore();
  });

+  it('DAY-SVC-023b — a linked booking with unreadable metadata no longer fails the update; it gets the new times', async () => {
+    const { user } = createUser(testDb);
+    const trip = createTrip(testDb, user.id);
+    const day = createDay(testDb, trip.id) as { id: number };
+    const place = createPlace(testDb, trip.id, { name: 'Hotel' }) as { id: number };
+    const { accommodation: accom } = (await svc.createAccommodation(trip.id, {
+      place_id: place.id, start_day_id: day.id, end_day_id: day.id,
+    })) as { accommodation: { id: number } };
+    const before = await linkedReservation(accom.id);
+    await updateRows(await orm(), Reservations, { id: before.id }, { metadata: '{not json' });
+
+    const existing = (await svc.getAccommodation(accom.id, trip.id))!;
+    await svc.updateAccommodation(accom.id, existing as Parameters<typeof svc.updateAccommodation>[1], { check_in: '16:00' });
+
+    expect(JSON.parse((await linkedReservation(accom.id)).metadata)).toEqual({ check_in_time: '16:00' });
+  });
+
  it('DAY-SVC-023 — updates check-in and check-out times', async () => {
    const { user } = createUser(testDb);
    const trip = createTrip(testDb, user.id);
@@ -974,6 +974,17 @@ describe('linking existing expenses to bookings and places (#2084)', () => {
    expect(await metadataOf(reservationId)).toEqual({ seat: '12A', price: '30.3', priceCurrency: 'USD' });
  });

+  it('BUDGET-SVC-DB-072b: a booking whose metadata is not JSON gets the mirrored price instead of a logged failure', async () => {
+    const { trip, reservationId } = await tripWithBooking();
+    await updateRows(await orm(), Reservations, { id: reservationId }, { metadata: '{not json' });
+    const item = await budget.createBudgetItem(trip.id, { name: 'Fare', total_price: 42, currency: 'usd' });
+    await linkItems(reservationId, [item.id]);
+
+    await budget.resyncReservationPrice(trip.id, reservationId);
+
+    expect(await metadataOf(reservationId)).toEqual({ price: '42', priceCurrency: 'USD' });
+  });
+
  it('BUDGET-SVC-DB-052: an expense without a currency and one in the trip currency by code add up', async () => {
    // No currency means the trip's; the two rows are in the same money and the
    // booking shows their sum, not the first one alone.
@@ -523,6 +523,18 @@ describe('exportICS', () => {
    expect(ics).not.toContain('DESCRIPTION');
  });

+  it('CAL-017b: a booking whose metadata is not JSON still exports, without the metadata lines', async () => {
+    const { user } = createUser(testDb);
+    const trip = createTrip(testDb, user.id, { title: 'Broken' });
+    const reservation = createReservation(testDb, trip.id, { title: 'Garbled Flight', type: 'flight' });
+    await updateRows(orm, Reservations, { id: reservation.id }, { reservation_time: '2025-06-02T09:00', metadata: '{not json' });
+
+    const { ics } = await svc.exportICS(trip.id);
+
+    expect(ics).toContain('SUMMARY:Garbled Flight');
+    expect(ics).toContain('DESCRIPTION:Type: flight\r\n');
+  });
+
  it('CAL-018: flight metadata with only one airport emits only that side of the route', async () => {
    const { user } = createUser(testDb);
    const trip = createTrip(testDb, user.id, { title: 'Half Routes' });
@@ -78,6 +78,7 @@ import { RateLimitService } from '../../../src/nest/common/rate-limit.service';
 import { createTestUnitOfWork, createTestAppSettingsRepo, createTestTripsRepo, sharedTestOrm } from '../../helpers/test-uow';
 import type { TestOrm } from '../../helpers/test-orm';
 import { countRows, findRow, findRows, insertRow, updateRows } from '../../helpers/factories/rows';
+import { CollabPolls } from '../../../src/db/entities/CollabPolls.entity';
 import { dbNow } from '../../../src/db/types/db-timestamp.type';
 import { CollabMessageReactions } from '../../../src/db/entities/CollabMessageReactions.entity';
 import { CollabMessages } from '../../../src/db/entities/CollabMessages.entity';
@@ -221,6 +222,16 @@ describe('votePoll', () => {
    expect(result.error).toBe('invalid_index');
  });

+  it('COLLAB-SVC-006b: a poll whose stored options are not JSON lists with no options and refuses every vote', async () => {
+    const { user1, trip } = setup();
+    const poll = await svc.createPoll(trip.id, user1.id, { question: 'Q?', options: ['A', 'B'] });
+    await updateRows(orm, CollabPolls, { id: poll!.id }, { options: '{broken' });
+
+    const [listed] = (await svc.listPolls(trip.id)) as Array<{ options: unknown[] }>;
+    expect(listed.options).toEqual([]);
+    expect((await svc.votePoll(trip.id, poll!.id, user1.id, 0)).error).toBe('invalid_index');
+  });
+
  it('COLLAB-SVC-007: returns error "not_found" for nonexistent poll', async () => {
    const { user1, trip } = setup();
    const result = await svc.votePoll(trip.id, 9999, user1.id, 0);
@@ -0,0 +1,49 @@
+import { describe, expect, it, vi } from 'vitest';
+
+const logMock = vi.hoisted(() => ({ logInfo: vi.fn(), logError: vi.fn(), logWarn: vi.fn(), logDebug: vi.fn() }));
+vi.mock('../../../src/nest/audit/audit-log.logger', () => logMock);
+
+import { decodeEntryRow } from '../../../src/nest/journey/journey-entry-row';
+import type { JourneyEntry } from '../../../src/types';
+
+const row = (over: Partial<JourneyEntry> = {}): JourneyEntry =>
+  ({
+    id: 5,
+    journey_id: 1,
+    author_id: 1,
+    type: 'entry',
+    entry_date: '2026-01-01',
+    visibility: 'private',
+    sort_order: 0,
+    tags: null,
+    pros_cons: null,
+    stats_excluded: 0,
+    dismissed: 0,
+    is_draft: 1,
+    ...over,
+  }) as JourneyEntry;
+
+describe('decodeEntryRow', () => {
+  it('JENTRYROW-001: decodes the JSON columns and turns the flags into booleans', () => {
+    const wire = decodeEntryRow(row({ tags: '["beach","food"]', pros_cons: '{"pros":["sun"],"cons":["crowds"]}' }));
+    expect(wire.tags).toEqual(['beach', 'food']);
+    expect(wire.pros_cons).toEqual({ pros: ['sun'], cons: ['crowds'] });
+    expect(wire.stats_excluded).toBe(false);
+    expect(wire.dismissed).toBe(false);
+    expect(wire.is_draft).toBe(true);
+  });
+
+  it('JENTRYROW-002: empty columns are an empty tag list and no pros/cons', () => {
+    const wire = decodeEntryRow(row());
+    expect(wire.tags).toEqual([]);
+    expect(wire.pros_cons).toBeNull();
+    expect(logMock.logWarn).not.toHaveBeenCalled();
+  });
+
+  it('JENTRYROW-003: unreadable JSON no longer throws: it reads as empty and is logged', () => {
+    const wire = decodeEntryRow(row({ tags: '{oops', pros_cons: 'not json' }));
+    expect(wire.tags).toEqual([]);
+    expect(wire.pros_cons).toBeNull();
+    expect(logMock.logWarn).toHaveBeenCalledWith('[json] journey_entries.tags (journey entry 5): stored value is not valid JSON, using the fallback');
+  });
+});
@@ -1419,4 +1419,28 @@ describe('admin OAuth sessions', () => {
    expect(sessions).toHaveLength(1);
    expect(sessions[0].scopes).toBeNull();
  });
+
+  it('a row with malformed scopes no longer fails the user list, the token check or the refresh: it grants nothing', async () => {
+    const { user } = createUser(testDb);
+    const created = await makeClient(user.id);
+    const client = created.client as { client_id: string; client_secret: string };
+    const tokens = await issueTokens(client.client_id, user.id, ['trips:read']);
+    await updateRows(t, OauthTokens, { user: user.id }, { scopes: 'not-json{' });
+
+    const [session] = await listOAuthSessions(user.id);
+    expect(session.scopes).toEqual([]);
+    expect((await getUserByAccessToken(tokens.access_token))?.scopes).toEqual([]);
+  });
+
+  it('a malformed consent row counts as consent to nothing, and a new grant replaces it', async () => {
+    const { user } = createUser(testDb);
+    const created = await makeClient(user.id);
+    const clientId = created.client!.client_id as string;
+    await saveConsent(clientId, user.id, ['trips:read']);
+    await updateRows(t, OauthConsents, { user: user.id }, { scopes: '{broken' });
+
+    expect(await getConsent(clientId, user.id)).toEqual([]);
+    await saveConsent(clientId, user.id, ['trips:write']);
+    expect(await getConsent(clientId, user.id)).toEqual(['trips:write']);
+  });
 });
@@ -0,0 +1,118 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { z } from 'zod';
+
+const logMock = vi.hoisted(() => ({ logInfo: vi.fn(), logError: vi.fn(), logWarn: vi.fn(), logDebug: vi.fn() }));
+vi.mock('../../../src/nest/audit/audit-log.logger', () => logMock);
+
+import { decodeJson, decodeJsonResult, encodeJson, logJsonFailure, type JsonColumn } from '../../../src/utils/json-column';
+import {
+  COLLAB_POLL_OPTIONS,
+  JOURNEY_ENTRY_PROS_CONS,
+  JOURNEY_ENTRY_TAGS,
+  MCP_TOKEN_API_SCOPES,
+  OAUTH_CLIENT_ALLOWED_SCOPES,
+  OAUTH_CLIENT_REDIRECT_URIS,
+  OAUTH_CONSENT_SCOPES,
+  OAUTH_TOKEN_SCOPES,
+  RESERVATION_METADATA,
+  USER_MFA_BACKUP_CODES,
+} from '../../../src/db/json-columns';
+
+const LIST: JsonColumn<string[]> = { column: 't.list', schema: z.array(z.string()), fallback: () => [] };
+
+beforeEach(() => vi.clearAllMocks());
+
+describe('decodeJsonResult', () => {
+  it('JSONCOL-001: decodes text of the right shape', () => {
+    expect(decodeJsonResult(LIST, '["a","b"]')).toEqual({ ok: true, value: ['a', 'b'] });
+  });
+
+  it('JSONCOL-002: names why a value did not decode', () => {
+    expect(decodeJsonResult(LIST, null)).toEqual({ ok: false, reason: 'empty' });
+    expect(decodeJsonResult(LIST, undefined)).toEqual({ ok: false, reason: 'empty' });
+    expect(decodeJsonResult(LIST, '')).toEqual({ ok: false, reason: 'empty' });
+    expect(decodeJsonResult(LIST, '{not json')).toEqual({ ok: false, reason: 'invalid-json' });
+    expect(decodeJsonResult(LIST, '{"a":1}')).toEqual({ ok: false, reason: 'schema' });
+  });
+
+  it('JSONCOL-003: checks a value a driver handed back already parsed', () => {
+    expect(decodeJsonResult(LIST, ['x'])).toEqual({ ok: true, value: ['x'] });
+    expect(decodeJsonResult(LIST, { a: 1 })).toEqual({ ok: false, reason: 'schema' });
+  });
+
+  it('JSONCOL-004: unwraps one level of double encoding only where the column says so', () => {
+    const doubled = JSON.stringify(JSON.stringify({ price: '12' }));
+    expect(decodeJsonResult(RESERVATION_METADATA, doubled)).toEqual({ ok: true, value: { price: '12' } });
+    expect(decodeJsonResult(LIST, JSON.stringify(JSON.stringify(['a'])))).toEqual({ ok: false, reason: 'schema' });
+    expect(decodeJsonResult(RESERVATION_METADATA, JSON.stringify('{broken'))).toEqual({ ok: false, reason: 'invalid-json' });
+  });
+});
+
+describe('decodeJson', () => {
+  it('JSONCOL-010: returns the decoded value without logging', () => {
+    expect(decodeJson(LIST, '["a"]')).toEqual(['a']);
+    expect(logMock.logWarn).not.toHaveBeenCalled();
+  });
+
+  it('JSONCOL-011: an empty column falls back quietly', () => {
+    expect(decodeJson(LIST, null)).toEqual([]);
+    expect(logMock.logWarn).not.toHaveBeenCalled();
+  });
+
+  it('JSONCOL-012: broken text and the wrong shape fall back and are logged with the column and context', () => {
+    expect(decodeJson(LIST, '{not json', 'row 7')).toEqual([]);
+    expect(logMock.logWarn).toHaveBeenLastCalledWith('[json] t.list (row 7): stored value is not valid JSON, using the fallback');
+    expect(decodeJson(LIST, '{"a":1}')).toEqual([]);
+    expect(logMock.logWarn).toHaveBeenLastCalledWith('[json] t.list: stored value has the wrong shape, using the fallback');
+  });
+
+  it('JSONCOL-013: every fallback is a fresh value', () => {
+    const a = decodeJson(RESERVATION_METADATA, null);
+    a.price = '1';
+    expect(decodeJson(RESERVATION_METADATA, null)).toEqual({});
+  });
+});
+
+describe('logJsonFailure / encodeJson', () => {
+  it('JSONCOL-020: logs the column, the context and the reason', () => {
+    logJsonFailure(LIST, 'schema');
+    expect(logMock.logWarn).toHaveBeenCalledWith('[json] t.list: stored value has the wrong shape, using the fallback');
+  });
+
+  it('JSONCOL-021: encodes a value of the right shape and refuses another', () => {
+    expect(encodeJson(LIST, ['a', 'b'])).toBe('["a","b"]');
+    expect(() => encodeJson(LIST, [1] as unknown as string[])).toThrow();
+  });
+});
+
+describe('the declared columns', () => {
+  it('JSONCOL-030: reservation metadata is an object, and anything else reads as an empty one', () => {
+    expect(decodeJson(RESERVATION_METADATA, '{"airline":"LH","legs":[1,2]}')).toEqual({ airline: 'LH', legs: [1, 2] });
+    expect(decodeJson(RESERVATION_METADATA, '[1,2]')).toEqual({});
+    expect(decodeJson(RESERVATION_METADATA, 'null')).toEqual({});
+    expect(decodeJson(RESERVATION_METADATA, '{oops')).toEqual({});
+  });
+
+  it('JSONCOL-031: the OAuth lists fail closed to an empty list', () => {
+    for (const col of [OAUTH_TOKEN_SCOPES, OAUTH_CLIENT_ALLOWED_SCOPES, OAUTH_CLIENT_REDIRECT_URIS, OAUTH_CONSENT_SCOPES]) {
+      expect(decodeJson(col, '["trips:read"]')).toEqual(['trips:read']);
+      expect(decodeJson(col, 'garbage')).toEqual([]);
+      expect(decodeJson(col, '[1]')).toEqual([]);
+    }
+  });
+
+  it('JSONCOL-032: the lenient lists keep whatever list is stored', () => {
+    expect(decodeJson(MCP_TOKEN_API_SCOPES, '["a",1]')).toEqual(['a', 1]);
+    expect(decodeJson(USER_MFA_BACKUP_CODES, '["h1",null]')).toEqual(['h1', null]);
+    expect(decodeJson(COLLAB_POLL_OPTIONS, '["Yes",{"label":"No"}]')).toEqual(['Yes', { label: 'No' }]);
+    expect(decodeJson(COLLAB_POLL_OPTIONS, '{"a":1}')).toEqual([]);
+  });
+
+  it('JSONCOL-033: journey entry tags and pros/cons', () => {
+    expect(decodeJson(JOURNEY_ENTRY_TAGS, '["beach"]')).toEqual(['beach']);
+    expect(decodeJson(JOURNEY_ENTRY_TAGS, '{bad')).toEqual([]);
+    expect(decodeJson(JOURNEY_ENTRY_PROS_CONS, '{"pros":["sun"],"cons":[]}')).toEqual({ pros: ['sun'], cons: [] });
+    expect(decodeJson(JOURNEY_ENTRY_PROS_CONS, 'null')).toBeNull();
+    expect(decodeJson(JOURNEY_ENTRY_PROS_CONS, '{bad')).toBeNull();
+  });
+});