refactor: Extract get_user_set in peer_data.

We now use the same code in all places to
get the bucket of user_ids that correspond
to a stream, and we consistently treat
a stream as having zero subscribers, not
an undefined number of subscribers, in
the hypothetical case of us asking about
a stream that we're not tracking.

The behavior for untracked streams has
always been problematic, since if a
stream is untracked, all bets are off.

So now if we don't "track" the stream,
the subscriber count is zero.  None of
our callers distinguish between undefined
and zero.

And we just consider the stream to be subscribed
by a user when add_subscriber is called,
even if we haven't been told by stream_data
to track the stream.  (We also stop
returning true/false from add_subscriber,
since only test code was looking at it.)

We protect against the most likely source
of internal-to-the-frontend bugs by adding
the assert_number() call.

We generally have to assume that the server
is sending us sensible data at page load
time, or all bets are off.

And we have good protections in place
for unknown ids in our dispatch code
for peer_add/peer_remove events.

Author: showell

frontend_tests/node_tests/compose.js

@@ -1018,7 +1018,9 @@ run_test("warn_if_private_stream_is_linked", () => {
     denmark = {
         invite_only: true,
         name: "Denmark",
+        stream_id: 22,
     };
+    stream_data.add_sub(denmark);
 
     compose.warn_if_private_stream_is_linked(denmark);
     assert.equal($("#compose_private_stream_alert").visible(), true);

frontend_tests/node_tests/peer_data.js

@@ -8,7 +8,7 @@
 
 const {strict: assert} = require("assert");
 
-const {set_global, with_field, zrequire} = require("../zjsunit/namespace");
+const {set_global, zrequire} = require("../zjsunit/namespace");
 const {run_test} = require("../zjsunit/test");
 
 const peer_data = zrequire("peer_data");
@@ -118,8 +118,7 @@ run_test("subscribers", () => {
     assert(!stream_data.is_user_subscribed(sub.stream_id, brutus.user_id));
 
     // add
-    let ok = peer_data.add_subscriber(sub.stream_id, brutus.user_id);
-    assert(ok);
+    peer_data.add_subscriber(sub.stream_id, brutus.user_id);
     assert(stream_data.is_user_subscribed(sub.stream_id, brutus.user_id));
     sub = stream_data.get_sub("Rome");
     assert.equal(peer_data.get_subscriber_count(sub.stream_id), 1);
@@ -134,7 +133,7 @@ run_test("subscribers", () => {
     assert.equal(peer_data.get_subscriber_count(sub.stream_id), 1);
 
     // remove
-    ok = peer_data.remove_subscriber(sub.stream_id, brutus.user_id);
+    let ok = peer_data.remove_subscriber(sub.stream_id, brutus.user_id);
     assert(ok);
     assert(!stream_data.is_user_subscribed(sub.stream_id, brutus.user_id));
     sub = stream_data.get_sub("Rome");
@@ -144,15 +143,15 @@ run_test("subscribers", () => {
 
     blueslip.expect("warn", "Undefined user_id passed to function is_user_subscribed");
     assert.equal(stream_data.is_user_subscribed(sub.stream_id, undefined), undefined);
+    blueslip.reset();
 
     // Verify noop for bad stream when removing subscriber
     const bad_stream_id = 999999;
-    blueslip.expect(
-        "warn",
-        "We got a remove_subscriber call for an untracked stream " + bad_stream_id,
-    );
+    blueslip.expect("warn", "We called get_user_set for an untracked stream: " + bad_stream_id);
+    blueslip.expect("warn", "We tried to remove invalid subscriber: 104");
     ok = peer_data.remove_subscriber(bad_stream_id, brutus.user_id);
     assert(!ok);
+    blueslip.reset();
 
     // verify that removing an already-removed subscriber is a noop
     blueslip.expect("warn", "We tried to remove invalid subscriber: 104");
@@ -161,6 +160,7 @@ run_test("subscribers", () => {
     assert(!stream_data.is_user_subscribed(sub.stream_id, brutus.user_id));
     sub = stream_data.get_sub("Rome");
     assert.equal(peer_data.get_subscriber_count(sub.stream_id), 0);
+    blueslip.reset();
 
     // Verify defensive code in set_subscribers, where the second parameter
     // can be undefined.
@@ -172,8 +172,7 @@ run_test("subscribers", () => {
     // Verify that we noop and don't crash when unsubscribed.
     sub.subscribed = false;
     stream_data.update_calculated_fields(sub);
-    ok = peer_data.add_subscriber(sub.stream_id, brutus.user_id);
-    assert(ok);
+    peer_data.add_subscriber(sub.stream_id, brutus.user_id);
     assert.equal(stream_data.is_user_subscribed(sub.stream_id, brutus.user_id), true);
     peer_data.remove_subscriber(sub.stream_id, brutus.user_id);
     assert.equal(stream_data.is_user_subscribed(sub.stream_id, brutus.user_id), false);
@@ -190,17 +189,18 @@ run_test("subscribers", () => {
     assert.equal(stream_data.is_user_subscribed(sub.stream_id, brutus.user_id), undefined);
     peer_data.remove_subscriber(sub.stream_id, brutus.user_id);
     assert.equal(stream_data.is_user_subscribed(sub.stream_id, brutus.user_id), undefined);
-
-    // Verify that we don't crash and return false for a bad stream.
-    blueslip.expect("warn", "We got an add_subscriber call for an untracked stream: 9999999");
-    ok = peer_data.add_subscriber(9999999, brutus.user_id);
-    assert(!ok);
-
-    // Verify that we don't crash and return false for a bad user id.
-    blueslip.expect("error", "Unknown user_id in get_by_user_id: 9999999");
-    blueslip.expect("error", "We tried to add invalid subscriber: 9999999");
-    ok = peer_data.add_subscriber(sub.stream_id, 9999999);
-    assert(!ok);
+    blueslip.reset();
+
+    // Verify that we don't crash for a bad stream.
+    blueslip.expect("warn", "We called get_user_set for an untracked stream: 9999999");
+    peer_data.add_subscriber(9999999, brutus.user_id);
+    blueslip.reset();
+
+    // Verify that we don't crash for a bad user id.
+    blueslip.expect("error", "Unknown user_id in get_by_user_id: 88888");
+    blueslip.expect("warn", "We tried to add invalid subscriber: 88888");
+    peer_data.add_subscriber(sub.stream_id, 88888);
+    blueslip.reset();
 });
 
 run_test("get_subscriber_count", () => {
@@ -211,8 +211,8 @@ run_test("get_subscriber_count", () => {
     };
     stream_data.clear_subscriptions();
 
-    blueslip.expect("warn", "We got a get_subscriber_count call for an untracked stream: 102");
-    assert.equal(peer_data.get_subscriber_count(india.stream_id), undefined);
+    blueslip.expect("warn", "We called get_user_set for an untracked stream: 102");
+    assert.equal(peer_data.get_subscriber_count(india.stream_id), 0);
 
     stream_data.add_sub(india);
     assert.equal(peer_data.get_subscriber_count(india.stream_id), 0);
@@ -249,13 +249,6 @@ run_test("is_subscriber_subset", () => {
     const sub_b = make_sub(302, [2, 3]);
     const sub_c = make_sub(303, [1, 2, 3]);
 
-    // The bogus case should not come up in normal
-    // use.
-    // We simply punt on any calculation if
-    // a stream has no subscriber info (like
-    // maybe Zephyr?).
-    const bogus = {}; // no subscribers
-
     const matrix = [
         [sub_a, sub_a, true],
         [sub_a, sub_b, false],
@@ -266,33 +259,21 @@ run_test("is_subscriber_subset", () => {
         [sub_c, sub_a, false],
         [sub_c, sub_b, false],
         [sub_c, sub_c, true],
-        [bogus, bogus, false],
     ];
 
     for (const row of matrix) {
         assert.equal(peer_data.is_subscriber_subset(row[0].stream_id, row[1].stream_id), row[2]);
     }
-});
 
-run_test("warn if subscribers are missing", () => {
-    // This should only happen in this contrived test situation.
-    stream_data.clear_subscriptions();
-    const sub = {
-        name: "test",
-        stream_id: 3,
-        can_access_subscribers: true,
-    };
-
-    with_field(
-        stream_data,
-        "get_sub_by_id",
-        () => sub,
-        () => {
-            blueslip.expect("warn", "We called is_user_subscribed for an untracked stream: 3");
-            stream_data.is_user_subscribed(sub.stream_id, me.user_id);
-
-            blueslip.expect("warn", "We called get_subscribers for an untracked stream: 3");
-            assert.deepEqual(peer_data.get_subscribers(sub.stream_id), []);
-        },
-    );
+    // Two untracked streams should never be passed into us.
+    blueslip.expect("warn", "We called get_user_set for an untracked stream: 88888");
+    blueslip.expect("warn", "We called get_user_set for an untracked stream: 99999");
+    peer_data.is_subscriber_subset(99999, 88888);
+    blueslip.reset();
+
+    // Warn about hypothetical undefined stream_ids.
+    blueslip.expect("error", "You must pass ids as numbers to peer_data. id = undefined");
+    blueslip.expect("warn", "We called get_user_set for an untracked stream: undefined");
+    peer_data.is_subscriber_subset(undefined, sub_a.stream_id);
+    blueslip.reset();
 });

frontend_tests/node_tests/stream_events.js

@@ -420,7 +420,7 @@ run_test("remove_deactivated_user_from_all_streams", () => {
     assert(!stream_data.is_user_subscribed(dev_help.stream_id, george.user_id));
 
     // verify that deactivating user should unsubscribe user from all streams
-    assert(peer_data.add_subscriber(dev_help.stream_id, george.user_id));
+    peer_data.add_subscriber(dev_help.stream_id, george.user_id);
     assert(stream_data.is_user_subscribed(dev_help.stream_id, george.user_id));
 
     stream_events.remove_deactivated_user_from_all_streams(george.user_id);

static/js/peer_data.js

@@ -7,6 +7,28 @@ const people = require("./people");
 // clear_subscriptions.
 let stream_subscribers;
 
+function assert_number(id) {
+    if (typeof id !== "number") {
+        blueslip.error(`You must pass ids as numbers to peer_data. id = ${id}`);
+    }
+}
+
+function get_user_set(stream_id) {
+    assert_number(stream_id);
+
+    // This is an internal function to get the LazySet of users.
+    // We create one on the fly as necessary, but we warn in that case.
+    let subscribers = stream_subscribers.get(stream_id);
+
+    if (subscribers === undefined) {
+        blueslip.warn("We called get_user_set for an untracked stream: " + stream_id);
+        subscribers = new LazySet([]);
+        stream_subscribers.set(stream_id, subscribers);
+    }
+
+    return subscribers;
+}
+
 export function clear() {
     stream_subscribers = new Map();
 }
@@ -18,14 +40,10 @@ export function maybe_clear_subscribers(stream_id) {
 }
 
 export function is_subscriber_subset(stream_id1, stream_id2) {
-    const sub1_set = stream_subscribers.get(stream_id1);
-    const sub2_set = stream_subscribers.get(stream_id2);
-
-    if (sub1_set && sub2_set) {
-        return Array.from(sub1_set.keys()).every((key) => sub2_set.has(key));
-    }
+    const sub1_set = get_user_set(stream_id1);
+    const sub2_set = get_user_set(stream_id2);
 
-    return false;
+    return Array.from(sub1_set.keys()).every((key) => sub2_set.has(key));
 }
 
 export function potential_subscribers(stream_id) {
@@ -63,23 +81,14 @@ export function potential_subscribers(stream_id) {
 }
 
 export function get_subscriber_count(stream_id) {
-    const subscribers = stream_subscribers.get(stream_id);
-
-    if (!subscribers) {
-        blueslip.warn("We got a get_subscriber_count call for an untracked stream: " + stream_id);
-        return undefined;
-    }
-
+    const subscribers = get_user_set(stream_id);
     return subscribers.size;
 }
 
 export function get_subscribers(stream_id) {
-    const subscribers = stream_subscribers.get(stream_id);
-
-    if (typeof subscribers === "undefined") {
-        blueslip.warn("We called get_subscribers for an untracked stream: " + stream_id);
-        return [];
-    }
+    // This is our external interface for callers who just
+    // want an array of user_ids who are subscribed to a stream.
+    const subscribers = get_user_set(stream_id);
 
     return Array.from(subscribers.keys());
 }
@@ -91,27 +100,18 @@ export function set_subscribers(stream_id, user_ids) {
 }
 
 export function add_subscriber(stream_id, user_id) {
-    const subscribers = stream_subscribers.get(stream_id);
-    if (typeof subscribers === "undefined") {
-        blueslip.warn("We got an add_subscriber call for an untracked stream: " + stream_id);
-        return false;
-    }
+    // If stream_id/user_id are unknown to us, we will
+    // still track it, but we will warn.
+    const subscribers = get_user_set(stream_id);
     const person = people.get_by_user_id(user_id);
     if (person === undefined) {
-        blueslip.error("We tried to add invalid subscriber: " + user_id);
-        return false;
+        blueslip.warn("We tried to add invalid subscriber: " + user_id);
     }
     subscribers.add(user_id);
-
-    return true;
 }
 
 export function remove_subscriber(stream_id, user_id) {
-    const subscribers = stream_subscribers.get(stream_id);
-    if (typeof subscribers === "undefined") {
-        blueslip.warn("We got a remove_subscriber call for an untracked stream " + stream_id);
-        return false;
-    }
+    const subscribers = get_user_set(stream_id);
     if (!subscribers.has(user_id)) {
         blueslip.warn("We tried to remove invalid subscriber: " + user_id);
         return false;
@@ -125,7 +125,7 @@ export function remove_subscriber(stream_id, user_id) {
 export function bulk_add_subscribers({stream_ids, user_ids}) {
     // We rely on our callers to validate stream_ids and user_ids.
     for (const stream_id of stream_ids) {
-        const subscribers = stream_subscribers.get(stream_id) || set_subscribers(stream_id);
+        const subscribers = get_user_set(stream_id);
         for (const user_id of user_ids) {
             subscribers.add(user_id);
         }
@@ -135,7 +135,7 @@ export function bulk_add_subscribers({stream_ids, user_ids}) {
 export function bulk_remove_subscribers({stream_ids, user_ids}) {
     // We rely on our callers to validate stream_ids and user_ids.
     for (const stream_id of stream_ids) {
-        const subscribers = stream_subscribers.get(stream_id) || set_subscribers(stream_id);
+        const subscribers = get_user_set(stream_id);
         for (const user_id of user_ids) {
             subscribers.delete(user_id);
         }
@@ -146,11 +146,6 @@ export function is_user_subscribed(stream_id, user_id) {
     // Most callers should call stream_data.is_user_subscribed,
     // which does additional checks.
 
-    const subscribers = stream_subscribers.get(stream_id);
-    if (typeof subscribers === "undefined") {
-        blueslip.warn("We called is_user_subscribed for an untracked stream: " + stream_id);
-        return false;
-    }
-
+    const subscribers = get_user_set(stream_id);
     return subscribers.has(user_id);
 }