From 960ea990588cc72f3f678748a8fe9993f3828af5 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 4 Oct 2023 15:09:51 -0700 Subject: [PATCH] mute: Keep out unexpected enum values from events, too This way we consistently maintain the invariant that we only see valid values. --- src/mute/__tests__/muteModel-test.js | 35 ++++++++++++++++++++++++ src/mute/muteModel.js | 25 ++++++++++++++--- src/storage/__tests__/migrations-test.js | 6 ++-- src/storage/migrations.js | 3 ++ 4 files changed, 62 insertions(+), 7 deletions(-) diff --git a/src/mute/__tests__/muteModel-test.js b/src/mute/__tests__/muteModel-test.js index c3b56ace2ab..090557affad 100644 --- a/src/mute/__tests__/muteModel-test.js +++ b/src/mute/__tests__/muteModel-test.js @@ -15,6 +15,7 @@ import { makeMuteState, makeUserTopic } from './mute-testlib'; import { tryGetActiveAccountState } from '../../selectors'; import { UserTopicVisibilityPolicy } from '../../api/modelTypes'; import { EventTypes } from '../../api/eventTypes'; +import * as logging from '../../utils/logging'; /* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "check"] }] */ @@ -139,6 +140,25 @@ describe('reducer', () => { ); }); + test('in modern user_topics format: invalid enum values discarded', () => { + // $FlowFixMe[prop-missing]: Jest mock + logging.warn.mockReturnValue(); + + const action1 = eg.mkActionRegisterComplete({ + user_topics: [ + // $FlowIgnore[incompatible-call]: simulates a future server + makeUserTopic(eg.stream, 'topic', 42), + makeUserTopic(eg.stream, 'other topic', UserTopicVisibilityPolicy.Muted), + ], + }); + const action2 = eg.mkActionRegisterComplete({ + user_topics: [makeUserTopic(eg.stream, 'other topic', UserTopicVisibilityPolicy.Muted)], + }); + expect(reducer(initialState, action1, eg.plusReduxState)).toEqual( + reducer(initialState, action2, eg.plusReduxState), + ); + }); + test('in old muted_topics format: unit test', () => { const action = eg.mkActionRegisterComplete({ muted_topics: [[eg.stream.name, 'topic']], @@ -231,6 +251,21 @@ describe('reducer', () => { initialState, ); }); + + test('treat invalid enum value as removing', () => { + // $FlowFixMe[prop-missing]: Jest mock + logging.warn.mockReturnValue(); + + check( + makeMuteState([ + [eg.stream, 'topic'], + [eg.stream, 'other topic'], + ]), + // $FlowIgnore[incompatible-call]: simulates a future server + makeUserTopic(eg.stream, 'topic', 999), + makeMuteState([[eg.stream, 'other topic']]), + ); + }); }); describe('EVENT_MUTED_TOPICS (legacy)', () => { diff --git a/src/mute/muteModel.js b/src/mute/muteModel.js index ed972b478ce..76447a6a415 100644 --- a/src/mute/muteModel.js +++ b/src/mute/muteModel.js @@ -100,6 +100,21 @@ export function isTopicVisible( const initialState: MuteState = Immutable.Map(); +/** + * Warn and return true on an unexpected value; else return false. + * + * This lets us keep out of our data structures any values we + * don't expect in our types. + */ +function warnInvalidVisibilityPolicy(visibility_policy: UserTopicVisibilityPolicy): boolean { + if (!UserTopicVisibilityPolicy.isValid((visibility_policy: number))) { + // Not a value we expect. Keep it out of our data structures. + logging.warn(`unexpected UserTopicVisibilityPolicy: ${(visibility_policy: number)}`); + return true; + } + return false; +} + /** Consume the old `muted_topics` format. */ function convertLegacy(data, streams): MuteState { // Same strategy as in convertInitial, below. @@ -128,9 +143,7 @@ function convertInitial(data): MuteState { // plain old Array. const byStream = new DefaultMap(() => []); for (const { stream_id, topic_name, visibility_policy } of data) { - if (!UserTopicVisibilityPolicy.isValid((visibility_policy: number))) { - // Not a value we expect. Keep it out of our data structures. - logging.warn(`unexpected UserTopicVisibilityPolicy: ${(visibility_policy: number)}`); + if (warnInvalidVisibilityPolicy(visibility_policy)) { continue; } byStream.getOrCreate(stream_id).push([topic_name, visibility_policy]); @@ -186,7 +199,11 @@ export const reducer = ( const { event } = action; switch (event.type) { case EventTypes.user_topic: { - const { stream_id, topic_name, visibility_policy } = event; + const { stream_id, topic_name } = event; + let { visibility_policy } = event; + if (warnInvalidVisibilityPolicy(visibility_policy)) { + visibility_policy = UserTopicVisibilityPolicy.None; + } if (visibility_policy === UserTopicVisibilityPolicy.None) { // This is the "zero value" for this type, which our MuteState // data structure represents by leaving the topic out entirely. diff --git a/src/storage/__tests__/migrations-test.js b/src/storage/__tests__/migrations-test.js index a152ff4c187..dc8dd963eff 100644 --- a/src/storage/__tests__/migrations-test.js +++ b/src/storage/__tests__/migrations-test.js @@ -105,7 +105,7 @@ describe('migrations', () => { // What `base` becomes after all migrations. const endBase = { ...base52, - migrations: { version: 59 }, + migrations: { version: 60 }, }; for (const [desc, before, after] of [ @@ -128,9 +128,9 @@ describe('migrations', () => { // redundant with this one, because none of the migration steps notice // whether any properties outside `storeKeys` are present or not. [ - 'check dropCache at 59', + 'check dropCache at 60', // Just before the `dropCache`, plus a `cacheKeys` property, plus junk. - { ...base52, migrations: { version: 58 }, mute: [], nonsense: [1, 2, 3] }, + { ...base52, migrations: { version: 59 }, mute: [], nonsense: [1, 2, 3] }, // Should wind up with the same result as without the extra properties. endBase, ], diff --git a/src/storage/migrations.js b/src/storage/migrations.js index 528665ac4cb..c0af14ff851 100644 --- a/src/storage/migrations.js +++ b/src/storage/migrations.js @@ -514,6 +514,9 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} = // Changed `state.presence` (twice), but no migration because that's in `discardKeys`. + // Discard invalid enum values from `state.mute`. + '60': dropCache, + // TIP: When adding a migration, consider just using `dropCache`. // (See its jsdoc for guidance on when that's the right answer.) };