mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(journey): answer an entry edit with tags decoded, not the JSON string (#2155)
tags and pros_cons are JSON held in TEXT columns. Three read paths decoded them by hand and createEntry and updateEntry did not, so an edit answered with the raw row: tags came back as '["beach","sunset"]', the store spread it into an entry the client types as string[], and the journey page threw "tags.map is not a function" behind an error boundary. Any edit was enough to arm it. The update path writes JSON.stringify(val) with no length check, so even clearing an entry's tags stores '[]' and answers with that string. Neither compiler could see it: JourneyEntry describes the row, tags and all, and both sides read the same field name while only one of them was right. So the decoded shape gets a type of its own and one decoder, which is what turned up the third case - the early return for an update that changes nothing handed back a raw row too. Fixes #2085
This commit is contained in:
committed by
GitHub
parent
e70d7397dd
commit
e615011e73
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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<JourneyEntry, 'tags' | 'pros_cons'> {
|
||||
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,
|
||||
};
|
||||
}
|
||||
@@ -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] || [],
|
||||
}));
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user