narrow: Store stream IDs, not only names!

In the migration tests, it is kind of messy that we have to update
a previous migration's test.  That happens because we're running
all the migrations to completion.

As the comment at the "whole sequence" test says about a related
point: this is probably not a great design for continuing to add
more migrations in this sequence.  But pretty soon we're going to
cap it off and write future migrations in a new framework, which
will come with its own way of writing tests.  So for the moment
just live with it.

Fixes-partly: #4333

src/compose/ComposeBox.js

@@ -35,6 +35,7 @@ import {
   isStreamNarrow,
   isStreamOrTopicNarrow,
   isTopicNarrow,
+  streamIdOfNarrow,
   streamNameOfNarrow,
   topicNarrow,
   topicOfNarrow,
@@ -402,8 +403,9 @@ class ComposeBoxInner extends PureComponent<Props, State> {
     const { narrow, isEditing } = this.props;
     if (isStreamNarrow(narrow) || (isTopicNarrow(narrow) && isEditing)) {
       const streamName = streamNameOfNarrow(narrow);
+      const streamId = streamIdOfNarrow(narrow);
       const topic = this.state.topic.trim();
-      return topicNarrow(streamName, topic || apiConstants.NO_TOPIC_TOPIC);
+      return topicNarrow(streamName, streamId, topic || apiConstants.NO_TOPIC_TOPIC);
     }
     invariant(isConversationNarrow(narrow), 'destination narrow must be conversation');
     return narrow;

src/storage/__tests__/migrations-test.js

@@ -59,8 +59,8 @@ describe('migrations', () => {
     ],
   };
 
-  // What `base` becomes after all migrations.
-  const endBase = {
+  // What `base` becomes after migrations up through 37.
+  const base37 = {
     migrations: { version: 37 },
     accounts: [
       {
@@ -84,14 +84,20 @@ describe('migrations', () => {
     },
   };
 
+  // What `base` becomes after all migrations.
+  const endBase = {
+    ...base37,
+    migrations: { version: 38 },
+  };
+
   for (const [desc, before, after] of [
     // Test the behavior with no migration state, which doesn't apply any
     // of the specific migrations.
-    ['empty state -> just store version', {}, { migrations: { version: 37 } }],
+    ['empty state -> just store version', {}, { migrations: endBase.migrations }],
     [
       'no migration state -> just store version, leave everything else',
       { nonsense: [1, 2, 3] },
-      { migrations: { version: 37 }, nonsense: [1, 2, 3] },
+      { migrations: endBase.migrations, nonsense: [1, 2, 3] },
     ],
 
     // Test the whole sequence all together.  This covers many of the
@@ -144,9 +150,12 @@ describe('migrations', () => {
         migrations: { version: 21 },
         drafts: { 'pm:d:12:other@example.com': 'text', 'topic:s:general\x00stuff': 'other text' },
       },
-      { ...endBase, drafts: { 'pm:12': 'text', 'topic:s:general\x00stuff': 'other text' } },
-      // NB the original version of this migration was buggy; it resulted in:
-      //   { ...endBase, drafts: { 'topic:s:general\x00stuff': 'other text' } }, // WRONG
+      // This migration itself leaves the `topic:` draft in place.  But it
+      // gets dropped later by migration 38.
+      { ...endBase, drafts: { 'pm:12': 'text' } },
+      // NB the original version of this migration was buggy; it would drop
+      // the PM drafts entirely, so this test case would end up as:
+      //   { ...endBase, drafts: {} }, // WRONG
       // Should have written tests for it the first time. :-)
     ],
     [
@@ -204,6 +213,18 @@ describe('migrations', () => {
       { ...base15, settings: { ...base15.settings, doNotMarkMessagesAsRead: true } },
       { ...endBase, settings: { ...endBase.settings, doNotMarkMessagesAsRead: true } },
     ],
+    [
+      'check 38',
+      {
+        ...base37,
+        drafts: {
+          'topic:s:general\x00stuff': 'text',
+          'stream:s:general': 'more text',
+          'pm:12': 'pm text',
+        },
+      },
+      { ...endBase, drafts: { 'pm:12': 'pm text' } },
+    ],
   ]) {
     /* eslint-disable no-loop-func */
     test(desc, async () => {

src/storage/migrations.js

@@ -350,6 +350,18 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} =
     },
   }),
 
+  // Change format of keys representing stream and topic narrows, adding IDs.
+  '38': state => ({
+    ...state,
+    drafts: objectFromEntries(
+      // Just drop drafts for stream and topic narrows, for the same reasons
+      // as for PM narrows in migration 21 above.
+      Object.keys(state.drafts)
+        .filter(key => !key.startsWith('stream:') && !key.startsWith('topic:'))
+        .map(key => [key, state.drafts[key]]),
+    ),
+  }),
+
   // TIP: When adding a migration, consider just using `dropCache`.
 };
 

src/utils/__tests__/narrow-test.js

@@ -26,6 +26,7 @@ import {
   streamNameOfNarrow,
   topicOfNarrow,
   caseNarrowPartial,
+  streamIdOfNarrow,
 } from '../narrow';
 import type { Narrow, Message } from '../../types';
 import * as eg from '../../__tests__/lib/exampleData';
@@ -87,6 +88,7 @@ describe('streamNarrow', () => {
   test('narrows to messages from a specific stream', () => {
     const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id);
     expect(isStreamNarrow(narrow)).toBeTrue();
+    expect(streamIdOfNarrow(narrow)).toEqual(eg.stream.stream_id);
     expect(streamNameOfNarrow(narrow)).toEqual(eg.stream.name);
   });
 
@@ -101,6 +103,7 @@ describe('topicNarrow', () => {
   test('narrows to a specific topic within a specified stream', () => {
     const narrow = topicNarrow(eg.stream.name, eg.stream.stream_id, 'some topic');
     expect(isTopicNarrow(narrow)).toBeTrue();
+    expect(streamIdOfNarrow(narrow)).toEqual(eg.stream.stream_id);
     expect(streamNameOfNarrow(narrow)).toEqual(eg.stream.name);
     expect(topicOfNarrow(narrow)).toEqual('some topic');
   });

src/utils/narrow.js

@@ -34,8 +34,8 @@ import {
  *    server, and `apiNarrowOfNarrow` for converting to it.
  */
 export opaque type Narrow =
-  | {| type: 'stream', streamName: string |}
-  | {| type: 'topic', streamName: string, topic: string |}
+  | {| type: 'stream', streamName: string, streamId: number |}
+  | {| type: 'topic', streamName: string, streamId: number, topic: string |}
   | {| type: 'pm', userIds: PmKeyRecipients |}
   | {| type: 'search', query: string |}
   | {| type: 'all' | 'starred' | 'mentioned' | 'all-pm' |};
@@ -148,12 +148,11 @@ export const ALL_PRIVATE_NARROW: Narrow = specialNarrow('private');
 
 export const ALL_PRIVATE_NARROW_STR: string = keyFromNarrow(ALL_PRIVATE_NARROW);
 
-export const streamNarrow = (streamName: string, streamId?: number): Narrow =>
-  Object.freeze({ type: 'stream', streamName });
+export const streamNarrow = (streamName: string, streamId: number): Narrow =>
+  Object.freeze({ type: 'stream', streamName, streamId });
 
-export const topicNarrow = (streamName: string, ...args: [string] | [number, string]): Narrow =>
-  // $FlowFixMe[incompatible-cast]
-  Object.freeze({ type: 'topic', streamName, topic: (args[args.length - 1]: string) });
+export const topicNarrow = (streamName: string, streamId: number, topic: string): Narrow =>
+  Object.freeze({ type: 'topic', streamName, streamId, topic });
 
 export const SEARCH_NARROW = (query: string): Narrow => Object.freeze({ type: 'search', query });
 
@@ -163,8 +162,8 @@ type NarrowCases<T> = {|
   starred: () => T,
   mentioned: () => T,
   allPrivate: () => T,
-  stream: (name: string) => T,
-  topic: (streamName: string, topic: string) => T,
+  stream: (name: string, streamId: number) => T,
+  topic: (streamName: string, topic: string, streamId: number) => T,
   search: (query: string) => T,
 |};
 
@@ -175,8 +174,8 @@ export function caseNarrow<T>(narrow: Narrow, cases: NarrowCases<T>): T {
   };
 
   switch (narrow.type) {
-    case 'stream': return cases.stream(narrow.streamName);
-    case 'topic': return cases.topic(narrow.streamName, narrow.topic);
+    case 'stream': return cases.stream(narrow.streamName, narrow.streamId);
+    case 'topic': return cases.topic(narrow.streamName, narrow.topic, narrow.streamId);
     case 'pm': return cases.pm(narrow.userIds);
     case 'search': return cases.search(narrow.query);
     case 'all': return cases.home();
@@ -253,10 +252,13 @@ export function keyFromNarrow(narrow: Narrow): string {
     // NB if you're changing any of these: be sure to do a migration.
     // Take a close look at migration 19 and any later related migrations.
 
-    stream: name => `stream:s:${name}`,
+    // An earlier version had `stream:s:`.
+    stream: (name, streamId) => `stream:d:${streamId}:${name}`,
+
+    // An earlier version had `topic:s:`.
     // '\x00' is the one character not allowed in Zulip stream names.
     // (See `check_stream_name` in zulip.git:zerver/lib/streams.py.)
-    topic: (streamName, topic) => `topic:s:${streamName}\x00${topic}`,
+    topic: (streamName, topic, streamId) => `topic:d:${streamId}:${streamName}\x00${topic}`,
 
     // An earlier version had `pm:s:`.
     // Another had `pm:d:`.
@@ -281,22 +283,25 @@ export const parseNarrow = (narrowStr: string): Narrow => {
   // invariant: tag + rest === narrowStr
   switch (tag) {
     case 'stream:': {
-      if (!rest.startsWith('s:')) {
+      // The `/s` regexp flag means the `.` pattern matches absolutely
+      // anything.  By default it rejects certain "newline" characters,
+      // which in principle could appear in stream names.
+      const match = /^d:(\d+):(.*)/s.exec(rest);
+      if (!match) {
         throw makeError();
       }
-      const name = rest.substr('s:'.length);
-      return streamNarrow(name);
+      return streamNarrow(match[2], Number.parseInt(match[1], 10));
     }
 
     case 'topic:': {
       // The `/s` regexp flag means the `.` patterns match absolutely
       // anything.  By default they reject certain "newline" characters,
       // which in principle could appear in stream names or topics.
-      const match = /^s:(.*?)\x00(.*)/s.exec(rest);
+      const match = /^d:(\d+):(.*?)\x00(.*)/s.exec(rest);
       if (!match) {
         throw makeError();
       }
-      return topicNarrow(match[1], match[2]);
+      return topicNarrow(match[2], Number.parseInt(match[1], 10), match[3]);
     }
 
     case 'pm:': {
@@ -358,6 +363,16 @@ export const userIdsOfPmNarrow = (narrow: Narrow): PmKeyRecipients =>
 export const streamNameOfNarrow = (narrow: Narrow): string =>
   caseNarrowPartial(narrow, { stream: name => name, topic: streamName => streamName });
 
+/**
+ * The stream ID for a stream or topic narrow; else error.
+ *
+ * Most callers of this should probably be getting passed a stream ID
+ * instead of a Narrow in the first place; or if they do handle other kinds
+ * of narrows, should be using `caseNarrow`.
+ */
+export const streamIdOfNarrow = (narrow: Narrow): number =>
+  caseNarrowPartial(narrow, { stream: (n, id) => id, topic: (n, t, id) => id });
+
 /**
  * The topic for a topic narrow; else error.
  *