diff --git a/server/src/nest/journey/journey-domain.service.ts b/server/src/nest/journey/journey-domain.service.ts index 6e5dc8355..0a498500d 100644 --- a/server/src/nest/journey/journey-domain.service.ts +++ b/server/src/nest/journey/journey-domain.service.ts @@ -1,6 +1,7 @@ import { Injectable } from '@nestjs/common'; import { avatarUrl } from '../common/avatarUrl'; import type { Journey, JourneyEntry, JourneyPhoto, JourneyContributor } from '../../types'; +import { decodeEntryRow, type JourneyEntryWire } from './journey-entry-row'; import { DatabaseService } from '../database/database.service'; import { RealtimeService } from '../realtime/realtime.service'; import type { JourneyStats, JourneyTrack, TrekWsUserEventName } from '@trek/shared'; @@ -250,9 +251,7 @@ export class JourneyDomainService { .all(journeyId); const enrichedEntries = entries.map((e) => ({ - ...e, - tags: e.tags ? JSON.parse(e.tags) : [], - pros_cons: e.pros_cons ? JSON.parse(e.pros_cons) : null, + ...decodeEntryRow(e), photos: photosByEntry[e.id] || [], source_trip_name: e.source_trip_id ? (this.db.prepare('SELECT title FROM trips WHERE id = ?').get(e.source_trip_id) as { title: string } | undefined) @@ -1081,9 +1080,7 @@ export class JourneyDomainService { } return entries.map((e) => ({ - ...e, - tags: e.tags ? JSON.parse(e.tags) : [], - pros_cons: e.pros_cons ? JSON.parse(e.pros_cons) : null, + ...decodeEntryRow(e), photos: photosByEntry[e.id] || [], source_trip_name: e.source_trip_id ? (this.db.prepare('SELECT title FROM trips WHERE id = ?').get(e.source_trip_id) as { title: string } | undefined) @@ -1112,7 +1109,7 @@ export class JourneyDomainService { sort_order?: number; }, sid?: string, - ): JourneyEntry | null { + ): JourneyEntryWire | null { if (!this.canEdit(journeyId, userId)) return null; const now = this.ts(); @@ -1153,9 +1150,11 @@ export class JourneyDomainService { now, ); - const created = this.db - .prepare('SELECT * FROM journey_entries WHERE id = ?') - .get(Number(res.lastInsertRowid)) as JourneyEntry; + const created = decodeEntryRow( + this.db + .prepare('SELECT * FROM journey_entries WHERE id = ?') + .get(Number(res.lastInsertRowid)) as JourneyEntry, + ); this.broadcastJourneyEvent(journeyId, 'journey:entry:created', { entry: created }, sid); return created; } @@ -1180,7 +1179,7 @@ export class JourneyDomainService { sort_order: number; }>, sid?: string, - ): JourneyEntry | null { + ): JourneyEntryWire | null { const entry = this.db.prepare('SELECT * FROM journey_entries WHERE id = ?').get(entryId) as JourneyEntry | undefined; if (!entry) return null; if (!this.canEdit(entry.journey_id, userId)) return null; @@ -1229,7 +1228,7 @@ export class JourneyDomainService { values.push('entry'); } - if (fields.length === 0) return entry; + if (fields.length === 0) return decodeEntryRow(entry); fields.push('updated_at = ?'); values.push(this.ts()); @@ -1239,7 +1238,9 @@ export class JourneyDomainService { // touch the journey this.db.prepare('UPDATE journeys SET updated_at = ? WHERE id = ?').run(this.ts(), entry.journey_id); - const updated = this.db.prepare('SELECT * FROM journey_entries WHERE id = ?').get(entryId) as JourneyEntry; + const updated = decodeEntryRow( + this.db.prepare('SELECT * FROM journey_entries WHERE id = ?').get(entryId) as JourneyEntry, + ); this.broadcastJourneyEvent(entry.journey_id, 'journey:entry:updated', { entry: updated }, sid); return updated; } diff --git a/server/src/nest/journey/journey-entry-row.ts b/server/src/nest/journey/journey-entry-row.ts new file mode 100644 index 000000000..0ce00035f --- /dev/null +++ b/server/src/nest/journey/journey-entry-row.ts @@ -0,0 +1,37 @@ +import type { JourneyEntry } from '../../types'; + +/** + * A journey entry on the way out. + * + * `tags` and `pros_cons` are JSON held in TEXT columns, so a row read straight + * out of SQLite carries them as strings — and `JourneyEntry` types them that + * way, because it describes the row. What every consumer wants is the decoded + * shape, and it wants it under the same field names, so the two are one type + * apart and only this module knows the difference. + * + * That distinction is the whole bug this exists for. Three read paths decoded + * the columns by hand and the create and update paths did not, so an edit + * answered with `tags` as a string, the store spread it into an entry the + * client had typed as `string[]`, and the page threw `tags.map is not a + * function`. Neither compiler could see it: both sides were reading the same + * field name and only one of them was right. + * + * The broadcast is decoded for the same reason rather than because anything + * breaks on it today — the journey page reloads on any event and ignores the + * payload — but a payload that contradicts what the read paths answer is a trap + * for the first listener that decides to trust it. + */ +export interface JourneyEntryWire extends Omit { + tags: string[]; + pros_cons: { pros: string[]; cons: string[] } | null; +} + +/** Decode a row's JSON columns. The one place that knows they are JSON. */ +export function decodeEntryRow(row: JourneyEntry): JourneyEntryWire { + const { tags, pros_cons, ...rest } = row; + return { + ...rest, + tags: tags ? JSON.parse(tags) : [], + pros_cons: pros_cons ? JSON.parse(pros_cons) : null, + }; +} diff --git a/server/src/nest/journey/journey-share.service.ts b/server/src/nest/journey/journey-share.service.ts index e3a6f51e9..a0f120aeb 100644 --- a/server/src/nest/journey/journey-share.service.ts +++ b/server/src/nest/journey/journey-share.service.ts @@ -2,6 +2,7 @@ import { Injectable } from '@nestjs/common'; import crypto from 'crypto'; import { DatabaseService } from '../database/database.service'; import { JourneyDomainService } from './journey-domain.service'; +import { decodeEntryRow } from './journey-entry-row'; import { SettingsService } from '../settings/settings.service'; interface JourneySharePermissions { @@ -194,9 +195,7 @@ export class JourneyShareService { const enrichedEntries = entries .map(e => ({ - ...e, - tags: e.tags ? JSON.parse(e.tags) : [], - pros_cons: e.pros_cons ? JSON.parse(e.pros_cons) : null, + ...decodeEntryRow(e), photos: photosByEntry[e.id] || [], })); diff --git a/server/tests/unit/nest/journey-domain.service.test.ts b/server/tests/unit/nest/journey-domain.service.test.ts index b6d445715..63f6fa3a7 100644 --- a/server/tests/unit/nest/journey-domain.service.test.ts +++ b/server/tests/unit/nest/journey-domain.service.test.ts @@ -565,6 +565,84 @@ describe('updateEntry', () => { expect(updated!.story).toBe('Now I have a story!'); }); + /* + * The JSON columns come back decoded, on every path (#2085 follow-up). + * + * `tags` and `pros_cons` are JSON in TEXT columns. The read paths decoded + * them and create/update did not, so an edit answered with `tags` as a + * string, the client spread it into an entry it had typed as `string[]`, and + * the journey page threw `tags.map is not a function`. + */ + it('answers with tags decoded, not as the JSON string the column holds', () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const entry = svc.createEntry(journey.id, user.id, { + title: 'Beach Day', entry_date: '2026-03-10', tags: ['beach', 'sunset'], + }); + + expect(entry!.tags).toEqual(['beach', 'sunset']); + + const updated = svc.updateEntry(entry!.id, user.id, { title: 'Renamed' }); + expect(updated!.tags).toEqual(['beach', 'sunset']); + }); + + it('decodes an emptied tag list rather than answering the string "[]"', () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const entry = createJourneyEntry(testDb, journey.id, user.id, { entry_date: '2026-03-01' }); + + // The update path writes JSON.stringify([]) with no length check, so this + // is the ordinary way an entry ends up with a non-null tags column. + const updated = svc.updateEntry(entry.id, user.id, { tags: [] }); + expect(updated!.tags).toEqual([]); + }); + + it('decodes pros_cons the same way, on create and on update', () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const prosCons = { pros: ['warm'], cons: ['crowded'] }; + const entry = svc.createEntry(journey.id, user.id, { + title: 'Verdict', entry_date: '2026-03-10', pros_cons: prosCons, + }); + + expect(entry!.pros_cons).toEqual(prosCons); + expect(svc.updateEntry(entry!.id, user.id, { title: 'Renamed' })!.pros_cons).toEqual(prosCons); + }); + + /* An update that changes nothing takes its own early return out of the method. */ + it('decodes on the no-op update too', () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const entry = svc.createEntry(journey.id, user.id, { + title: 'Beach Day', entry_date: '2026-03-10', tags: ['beach'], + }); + + expect(svc.updateEntry(entry!.id, user.id, {})!.tags).toEqual(['beach']); + }); + + /* + * The broadcast carries the same shape as the answer. Nothing reads it today + * — the journey page reloads on any event — so this is about not leaving a + * payload that contradicts every read path for a listener to trust later. + */ + it('broadcasts the decoded entry, not the row', () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const entry = svc.createEntry(journey.id, user.id, { + title: 'Beach Day', entry_date: '2026-03-10', tags: ['beach', 'sunset'], + }); + + const spy = vi.spyOn(RealtimeService.prototype, 'broadcastToUser').mockImplementation(() => {}); + try { + svc.updateEntry(entry!.id, user.id, { title: 'Renamed' }); + const payload = spy.mock.calls.at(-1)?.[1] as { type: string; entry: { tags: unknown } }; + expect(payload.type).toBe('journey:entry:updated'); + expect(payload.entry.tags).toEqual(['beach', 'sunset']); + } finally { + spy.mockRestore(); + } + }); + it('JOURNEY-SVC-034: returns null for non-existent entry', () => { const { user } = createUser(testDb);