Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,16 @@ describe('the start event', () => {
expect(event).toMatchObject({ start: { analysisType: null } });
});

it('takes the release as the string the wire sends', () => {
it('takes the release as an int, which is what the field is becoming', () => {
// The service is changing this from a string to an int to match
// `/api/answer`, which always sent one. Both spellings are accepted so that
// neither side of that change breaks the panel.
expect(parseFrame(frame('start', { release: 97 }))).toMatchObject({
start: { release: 97 },
});
});

it('still takes the string form the field had before that', () => {
// `"97"`, not `97`. Requiring a number here silently nulled every release,
// and the field is what tells one release's cached summary from another's.
expect(parseFrame(frame('start', { release: '97' }))).toMatchObject({
Expand All @@ -57,6 +66,16 @@ describe('the start event', () => {
});
});

it('never turns an absent release into release 0', () => {
// `Number(null)`, `Number('')` and `Number(false)` are all 0, and 0 is a
// number that caches and compares perfectly happily while naming no release
// that has ever existed.
for (const release of [null, '', ' ', false, []]) {
expect(parseFrame(frame('start', { release }))).toMatchObject({ start: { release: null } });
}
expect(parseFrame(frame('start', {}))).toMatchObject({ start: { release: null } });
});

it('treats a missing cached flag as not cached', () => {
expect(parseFrame(frame('start', {}))).toMatchObject({ start: { cached: false } });
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,19 @@ function asReason(value: unknown): RefusalReason | null {
}

/** Turns one complete SSE frame into an event, or null if it is not one we act on. */
/**
* The release number, from either spelling, or null.
*
* Only a number or a non-empty string can be one, and it must be positive: the
* field's whole job is telling one release's cached summary from another's, and
* a 0 conjured out of `null` would do that job wrongly rather than not at all.
*/
function releaseOf(value: unknown): number | null {
if (typeof value !== 'number' && (typeof value !== 'string' || value.trim() === '')) return null;
const release = Number(value);
return Number.isFinite(release) && release > 0 ? release : null;
}

export function parseFrame(frame: string): SummaryEvent | null {
let name = '';
const dataLines: string[] = [];
Expand All @@ -151,10 +164,16 @@ export function parseFrame(frame: string): SummaryEvent | null {
return {
kind: 'start',
start: {
// A string on the wire ("97"), not a number. Coerced rather than
// required, because what this is for is telling one release's cache
// from another's, not arithmetic.
release: Number.isFinite(Number(data['release'])) ? Number(data['release']) : null,
// Accepts both spellings on purpose. It arrived as the string "97"
// while `/api/answer` sent an int for the same field; the service is
// making it an int here too, and a parser that accepted only one of
// them would have broken on one side of that change or the other.
//
// Not a bare `Number()`: `Number(null)`, `Number('')` and
// `Number(false)` are all 0, so an absent or empty release would
// become release 0 -- a number that compares, sorts and caches
// perfectly well while naming no release that exists.
release: releaseOf(data['release']),
analysisType:
typeof data['analysis_type'] === 'string' && TYPES.includes(data['analysis_type'])
? (data['analysis_type'] as AnalysisType)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ export class SummaryService {
*/
readonly expired = computed(() => this._state() === 'gone');

private release: number | null = null;
private readonly cache = new Map<string, Cached>();
private inFlight: AbortController | null = null;
/** What to ask again for once a challenge is solved. */
Expand Down Expand Up @@ -302,7 +301,12 @@ export class SummaryService {
switch (event.kind) {
case 'start':
this._started.set(true);
this.release = event.start.release;
// `start.release` is deliberately not kept. It was held in a field
// nothing ever read, which the dead-code gate cannot see because a
// private field is not an export. The cache key does not use it --
// this cache lives in one page's memory, and a release during a
// session would replace the process holding it -- so storing it
// only invited a later reader to trust a value nobody maintains.
this._analysisType.set(event.start.analysisType);
this._applied.set(event.start.disclosure);
break;
Expand Down
Loading