diff --git a/frontend_tests/node_tests/compose.js b/frontend_tests/node_tests/compose.js index f7f987b477..27a949f7b9 100644 --- a/frontend_tests/node_tests/compose.js +++ b/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); diff --git a/frontend_tests/node_tests/peer_data.js b/frontend_tests/node_tests/peer_data.js index af323e51b4..abf69b225e 100644 --- a/frontend_tests/node_tests/peer_data.js +++ b/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); + blueslip.reset(); - // 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 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 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); + // 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(); }); diff --git a/frontend_tests/node_tests/stream_events.js b/frontend_tests/node_tests/stream_events.js index 003375059b..3977d4ea4c 100644 --- a/frontend_tests/node_tests/stream_events.js +++ b/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); diff --git a/static/js/peer_data.js b/static/js/peer_data.js index 4e3816c0ae..88677f110c 100644 --- a/static/js/peer_data.js +++ b/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); + const sub1_set = get_user_set(stream_id1); + const sub2_set = get_user_set(stream_id2); - if (sub1_set && sub2_set) { - return Array.from(sub1_set.keys()).every((key) => sub2_set.has(key)); - } - - 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); }