From 1c8bf4f050475073d641f6f4b5f27b143e8b1dd6 Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 16 Jun 2023 18:53:45 +0530 Subject: [PATCH] people: Make `get_by_user_id` type-safe. We should make `get_by_user_id` type-safe in the sense that it's caller should not expect undefined values and hence it's caller should not need to validate the user ids. To do this I extracted a method `maybe_get_user_by_id` which is type-unsafe version of this function which will allow undefined values and will behave exactly the same as the previous version. So the callers which expects the undefined values from this function should use this new function `maybe_get_user_by_id`. Probably about half of these callers are implicitly iterating through all users in the people.js data set and thus by construction should never fail, but it's simpler to just convert all existing callers than do that audit. --- web/src/buddy_data.js | 6 +++--- web/src/compose_validate.js | 2 +- web/src/message_helper.js | 2 +- web/src/message_store.js | 2 +- web/src/peer_data.js | 2 +- web/src/people.js | 21 +++++++++++++++------ web/src/popovers.js | 2 +- web/src/presence.js | 2 +- web/src/rendered_markdown.js | 2 +- web/src/settings_bots.js | 2 +- web/src/settings_emoji.js | 2 +- web/src/settings_users.js | 6 +++--- web/src/stream_create_subscribers_data.js | 2 +- web/src/user_events.js | 2 +- web/src/user_group_create_members_data.js | 2 +- web/tests/buddy_data.test.js | 2 +- web/tests/compose_validate.test.js | 2 +- web/tests/message_store.test.js | 2 +- web/tests/peer_data.test.js | 2 +- web/tests/people.test.js | 2 +- web/tests/people_errors.test.js | 2 +- web/tests/presence.test.js | 1 + web/tests/user_events.test.js | 2 +- 23 files changed, 41 insertions(+), 31 deletions(-) diff --git a/web/src/buddy_data.js b/web/src/buddy_data.js index d1f071432d..12a9247b3e 100644 --- a/web/src/buddy_data.js +++ b/web/src/buddy_data.js @@ -75,8 +75,8 @@ export function compare_function(a, b) { } // Sort equivalent direct message names alphabetically - const person_a = people.get_by_user_id(a); - const person_b = people.get_by_user_id(b); + const person_a = people.maybe_get_user_by_id(a); + const person_b = people.maybe_get_user_by_id(b); const full_name_a = person_a ? person_a.full_name : ""; const full_name_b = person_b ? person_b.full_name : ""; @@ -253,7 +253,7 @@ function filter_user_ids(user_filter_text, user_ids) { // This first filter is for whether the user is eligible to be // displayed in the right sidebar at all. user_ids = user_ids.filter((user_id) => { - const person = people.get_by_user_id(user_id); + const person = people.maybe_get_user_by_id(user_id); if (!person) { blueslip.warn("Got user_id in presence but not people: " + user_id); diff --git a/web/src/compose_validate.js b/web/src/compose_validate.js index e779733693..3aa6e5a615 100644 --- a/web/src/compose_validate.js +++ b/web/src/compose_validate.js @@ -39,7 +39,7 @@ export function needs_subscribe_warning(user_id, stream_id) { // We expect the caller to already have verified that we're // sending to a valid stream and trying to mention the user. - const user = people.get_by_user_id(user_id); + const user = people.maybe_get_user_by_id(user_id); if (!user) { return false; diff --git a/web/src/message_helper.js b/web/src/message_helper.js index bdeffa946f..4ac7cbc91c 100644 --- a/web/src/message_helper.js +++ b/web/src/message_helper.js @@ -29,7 +29,7 @@ export function process_new_message(message) { people.extract_people_from_message(message); people.maybe_incr_recipient_count(message); - const sender = people.get_by_user_id(message.sender_id); + const sender = people.maybe_get_user_by_id(message.sender_id); if (sender) { message.sender_full_name = sender.full_name; message.sender_email = sender.email; diff --git a/web/src/message_store.js b/web/src/message_store.js index 2961b17f08..eaeeef6e69 100644 --- a/web/src/message_store.js +++ b/web/src/message_store.js @@ -39,7 +39,7 @@ export function get_pm_emails(message) { const user_ids = people.pm_with_user_ids(message); const emails = user_ids .map((user_id) => { - const person = people.get_by_user_id(user_id); + const person = people.maybe_get_user_by_id(user_id); if (!person) { blueslip.error("Unknown user id", {user_id}); return "?"; diff --git a/web/src/peer_data.js b/web/src/peer_data.js index ab85c2e4ca..bf37e07a23 100644 --- a/web/src/peer_data.js +++ b/web/src/peer_data.js @@ -98,7 +98,7 @@ export function add_subscriber(stream_id, user_id) { // 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); + const person = people.maybe_get_user_by_id(user_id); if (person === undefined) { blueslip.warn("We tried to add invalid subscriber: " + user_id); } diff --git a/web/src/people.js b/web/src/people.js index 19534aed68..bc3220c7d3 100644 --- a/web/src/people.js +++ b/web/src/people.js @@ -1,5 +1,6 @@ import md5 from "blueimp-md5"; import {format, utcToZonedTime} from "date-fns-tz"; +import assert from "minimalistic-assert"; import * as typeahead from "../shared/src/typeahead"; @@ -58,9 +59,17 @@ export function get_users_from_ids(user_ids) { return user_ids.map((user_id) => get_by_user_id(user_id)); } -export function get_by_user_id(user_id, ignore_missing) { - if (!people_by_user_id_dict.has(user_id) && !ignore_missing) { - blueslip.error("Unknown user_id in get_by_user_id", {user_id}); +// Use this function only when you are sure that user_id is valid. +export function get_by_user_id(user_id) { + const person = people_by_user_id_dict.get(user_id); + assert(person, `Unknown user_id in get_by_user_id: ${user_id}`); + return person; +} + +// This is type unsafe version of get_by_user_id for the callers that expects undefined values. +export function maybe_get_user_by_id(user_id) { + if (!people_by_user_id_dict.has(user_id)) { + blueslip.error("Unknown user_id in maybe_get_user_by_id", {user_id}); return undefined; } return people_by_user_id_dict.get(user_id); @@ -347,7 +356,7 @@ export function get_full_names_for_poll_option(user_ids) { } export function get_display_full_name(user_id) { - const person = get_by_user_id(user_id); + const person = maybe_get_user_by_id(user_id); if (!person) { blueslip.error("Unknown user id", {user_id}); return "?"; @@ -528,7 +537,7 @@ export function pm_with_url(message) { if (user_ids.length > 1) { suffix = "group"; } else { - const person = get_by_user_id(user_ids[0]); + const person = maybe_get_user_by_id(user_ids[0]); if (person && person.full_name) { suffix = person.full_name.replaceAll(/[ "%/<>`\p{C}]+/gu, "-"); } else { @@ -772,7 +781,7 @@ export function small_avatar_url(message) { // We should always have message.sender_id, except for in the // tutorial, where it's ok to fall back to the URL in the fake // messages. - person = get_by_user_id(message.sender_id); + person = maybe_get_user_by_id(message.sender_id); } // The first time we encounter a sender in a message, we may diff --git a/web/src/popovers.js b/web/src/popovers.js index 8313c1e9c7..6f9144767e 100644 --- a/web/src/popovers.js +++ b/web/src/popovers.js @@ -450,7 +450,7 @@ function get_user_info_popover_manage_menu_items() { function fetch_group_members(member_ids) { return member_ids - .map((m) => people.get_by_user_id(m)) + .map((m) => people.maybe_get_user_by_id(m)) .filter((m) => m !== undefined) .map((p) => ({ ...p, diff --git a/web/src/presence.js b/web/src/presence.js index 58c95fa638..6b240e69a2 100644 --- a/web/src/presence.js +++ b/web/src/presence.js @@ -175,7 +175,7 @@ export function set_info(presences, server_timestamp) { // reload_state.is_in_progress, because races where presence // returns data on users not yet received via the server_events // system are common in both situations. - const person = people.get_by_user_id(user_id, true); + const person = people.maybe_get_user_by_id(user_id); if (person === undefined) { if (!(watchdog.suspects_user_is_offline() || reload_state.is_in_progress())) { // If we're online, and we get a user who we don't diff --git a/web/src/rendered_markdown.js b/web/src/rendered_markdown.js index df05aad7d7..764ebe3df4 100644 --- a/web/src/rendered_markdown.js +++ b/web/src/rendered_markdown.js @@ -107,7 +107,7 @@ export const update_elements = ($content) => { // mention text to show the user's current name, // assuming that you're not searching for text // inside the highlight. - const person = people.get_by_user_id(user_id, true); + const person = people.maybe_get_user_by_id(user_id); if (person !== undefined) { // Note that person might be undefined in some // unpleasant corner cases involving data import. diff --git a/web/src/settings_bots.js b/web/src/settings_bots.js index cb07a7fe4a..cfb4b4d03f 100644 --- a/web/src/settings_bots.js +++ b/web/src/settings_bots.js @@ -340,7 +340,7 @@ export function confirm_bot_deactivation(bot_id, handle_confirm, loading_spinner } export function show_edit_bot_info_modal(user_id, from_user_info_popover) { - const bot = people.get_by_user_id(user_id); + const bot = people.maybe_get_user_by_id(user_id); const owner_id = bot_data.get(user_id).owner_id; const owner_full_name = people.get_full_name(owner_id); diff --git a/web/src/settings_emoji.js b/web/src/settings_emoji.js index 97acc96ca5..fae0bd5e4e 100644 --- a/web/src/settings_emoji.js +++ b/web/src/settings_emoji.js @@ -102,7 +102,7 @@ export function populate_emoji() { for (const emoji of Object.values(emoji_data)) { // Add people.js data for the user here. if (emoji.author_id !== null) { - emoji.author = people.get_by_user_id(emoji.author_id); + emoji.author = people.maybe_get_user_by_id(emoji.author_id); } else { emoji.author = null; } diff --git a/web/src/settings_users.js b/web/src/settings_users.js index 93408538d3..170bebd769 100644 --- a/web/src/settings_users.js +++ b/web/src/settings_users.js @@ -191,7 +191,7 @@ function bot_owner_full_name(owner_id) { return undefined; } - const bot_owner = people.get_by_user_id(owner_id); + const bot_owner = people.maybe_get_user_by_id(owner_id); if (!bot_owner) { return undefined; } @@ -200,7 +200,7 @@ function bot_owner_full_name(owner_id) { } function bot_info(bot_user_id) { - const bot_user = people.get_by_user_id(bot_user_id); + const bot_user = people.maybe_get_user_by_id(bot_user_id); if (!bot_user) { return undefined; @@ -616,7 +616,7 @@ function handle_reactivation($tbody) { } export function show_edit_user_info_modal(user_id, from_user_info_popover) { - const person = people.get_by_user_id(user_id); + const person = people.maybe_get_user_by_id(user_id); if (!person) { return; diff --git a/web/src/stream_create_subscribers_data.js b/web/src/stream_create_subscribers_data.js index 33648755f4..53e4a7f03e 100644 --- a/web/src/stream_create_subscribers_data.js +++ b/web/src/stream_create_subscribers_data.js @@ -40,7 +40,7 @@ export function must_be_subscribed(user_id) { export function add_user_ids(user_ids) { for (const user_id of user_ids) { if (!user_id_set.has(user_id)) { - const user = people.get_by_user_id(user_id); + const user = people.maybe_get_user_by_id(user_id); if (user) { user_id_set.add(user_id); } diff --git a/web/src/user_events.js b/web/src/user_events.js index ca9af2a815..caea60b91b 100644 --- a/web/src/user_events.js +++ b/web/src/user_events.js @@ -22,7 +22,7 @@ import * as settings_streams from "./settings_streams"; import * as settings_users from "./settings_users"; export const update_person = function update(person) { - const person_obj = people.get_by_user_id(person.user_id); + const person_obj = people.maybe_get_user_by_id(person.user_id); if (!person_obj) { blueslip.error("Got update_person event for unexpected user", {user_id: person.user_id}); diff --git a/web/src/user_group_create_members_data.js b/web/src/user_group_create_members_data.js index 66e34973d4..c24a871548 100644 --- a/web/src/user_group_create_members_data.js +++ b/web/src/user_group_create_members_data.js @@ -36,7 +36,7 @@ export function get_potential_members() { export function add_user_ids(user_ids) { for (const user_id of user_ids) { if (!user_id_set.has(user_id)) { - const user = people.get_by_user_id(user_id); + const user = people.maybe_get_user_by_id(user_id); if (user) { user_id_set.add(user_id); } diff --git a/web/tests/buddy_data.test.js b/web/tests/buddy_data.test.js index f85fb47be6..e9bec489f5 100644 --- a/web/tests/buddy_data.test.js +++ b/web/tests/buddy_data.test.js @@ -549,7 +549,7 @@ test("get_items_for_users", () => { test("error handling", () => { presence.presence_info.set(42, {status: "active"}); - blueslip.expect("error", "Unknown user_id in get_by_user_id"); + blueslip.expect("error", "Unknown user_id in maybe_get_user_by_id"); blueslip.expect("warn", "Got user_id in presence but not people: 42"); buddy_data.get_filtered_and_sorted_user_ids(); }); diff --git a/web/tests/compose_validate.test.js b/web/tests/compose_validate.test.js index cbc6cba280..15467b7140 100644 --- a/web/tests/compose_validate.test.js +++ b/web/tests/compose_validate.test.js @@ -616,7 +616,7 @@ test_ui("needs_subscribe_warning", () => { stream_data.add_sub(sub); peer_data.set_subscribers(sub.stream_id, [bob.user_id, me.user_id]); - blueslip.expect("error", "Unknown user_id in get_by_user_id"); + blueslip.expect("error", "Unknown user_id in maybe_get_user_by_id"); // Test with an invalid user id. assert.equal(compose_validate.needs_subscribe_warning(invalid_user_id, sub.stream_id), false); diff --git a/web/tests/message_store.test.js b/web/tests/message_store.test.js index f3b7771d37..32238cb2bf 100644 --- a/web/tests/message_store.test.js +++ b/web/tests/message_store.test.js @@ -212,7 +212,7 @@ test("errors", ({disallow_rewire}) => { display_recipient: [{id: 92714}], }; - blueslip.expect("error", "Unknown user_id in get_by_user_id", 2); + blueslip.expect("error", "Unknown user_id in maybe_get_user_by_id", 2); blueslip.expect("error", "Unknown user id", 2); // From person.js // Expect each to throw two blueslip errors diff --git a/web/tests/peer_data.test.js b/web/tests/peer_data.test.js index 894226ca18..a92704de1b 100644 --- a/web/tests/peer_data.test.js +++ b/web/tests/peer_data.test.js @@ -204,7 +204,7 @@ test("subscribers", () => { blueslip.reset(); // Verify that we don't crash for a bad user id. - blueslip.expect("error", "Unknown user_id in get_by_user_id"); + blueslip.expect("error", "Unknown user_id in maybe_get_user_by_id"); blueslip.expect("warn", "We tried to add invalid subscriber: 88888"); peer_data.add_subscriber(stream_id, 88888); blueslip.reset(); diff --git a/web/tests/people.test.js b/web/tests/people.test.js index 50a849df8a..656a259f82 100644 --- a/web/tests/people.test.js +++ b/web/tests/people.test.js @@ -795,7 +795,7 @@ test_people("message_methods", () => { "https://secure.gravatar.com/avatar/6dbdd7946b58d8b11351fcb27e5cdd55?d=identicon&s=50", ); - blueslip.expect("error", "Unknown user_id in get_by_user_id"); + blueslip.expect("error", "Unknown user_id in maybe_get_user_by_id"); message = { avatar_url: undefined, sender_email: "foo@example.com", diff --git a/web/tests/people_errors.test.js b/web/tests/people_errors.test.js index 03e76ec063..597528cab2 100644 --- a/web/tests/people_errors.test.js +++ b/web/tests/people_errors.test.js @@ -101,7 +101,7 @@ run_test("blueslip", () => { const reply_to = people.pm_reply_to(message); assert.ok(reply_to.includes("?")); - blueslip.expect("error", "Unknown user_id in get_by_user_id"); + blueslip.expect("error", "Unknown user_id in maybe_get_user_by_id"); blueslip.expect("error", "Unknown people in message"); const url = people.pm_with_url({type: "private", display_recipient: [{id: 42}]}); assert.equal(url.indexOf("unk"), url.length - 3); diff --git a/web/tests/presence.test.js b/web/tests/presence.test.js index 8fb2c5362f..89745a0e18 100644 --- a/web/tests/presence.test.js +++ b/web/tests/presence.test.js @@ -95,6 +95,7 @@ test("unknown user", ({override}) => { const presences = {}; presences[unknown_user_id.toString()] = "does-not-matter"; + blueslip.expect("error", "Unknown user_id in maybe_get_user_by_id", 3); blueslip.expect("error", "Unknown user ID in presence data"); presence.set_info(presences, now); diff --git a/web/tests/user_events.test.js b/web/tests/user_events.test.js index 3172bb5880..33ce685b8e 100644 --- a/web/tests/user_events.test.js +++ b/web/tests/user_events.test.js @@ -217,7 +217,7 @@ run_test("updates", () => { assert.ok(person.timezone); blueslip.expect("error", "Got update_person event for unexpected user"); - blueslip.expect("error", "Unknown user_id in get_by_user_id"); + blueslip.expect("error", "Unknown user_id in maybe_get_user_by_id"); assert.ok(!user_events.update_person({user_id: 29, full_name: "Sir Isaac Newton"})); me.profile_data = {};