From 7b7b63a9d3c380f9bb455b2247debb48a62ab42a Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 3 Aug 2020 17:58:01 -0700 Subject: [PATCH] registerForEvents: Start sending `user_avatar_url_field_optional`. See the point on `user_avatar_url_field_optional` at https://zulipchat.com/api/register-queue. This will reduce the volume of data that will have to be sent in the `/register` response, which should speed up the initial load. Now, the server may choose, at its own discretion, to omit the `avatar_url` field for users in the `/register` response [1], and we'll handle it by using the `/avatar/{user_id}` endpoint. Some of the boilerplate in the new FallbackAvatarURL class is due to a performance optimization [2] based on an observed ~1s added to the rehydrate time on CZO when URL objects are constructed, vs. about 200ms (and maybe a bit more in the upper tail) when they aren't. During rehydration, we don't need to expensively construct URL objects to validate raw URL strings, as long as we do so at the edge when we receive them from the network. [1] As of the introduction of the client capability (Zulip 3.0, feature level 18), it makes this decision based on whether the user has been inactive for a long time (using the `long_term_idle` field), but this is an implementation detail and should be expected to change without notice. [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/993660 Fixes: #4157 --- src/api/initialDataTypes.js | 4 +- src/api/messages/getMessages.js | 1 + src/api/registerForEvents.js | 17 ++++-- src/boot/store.js | 9 ++- src/events/eventToAction.js | 12 ++-- src/message/fetchActions.js | 1 + src/utils/__tests__/avatar-test.js | 44 +++++++++++++-- src/utils/avatar.js | 89 ++++++++++++++++++++++++++++-- 8 files changed, 155 insertions(+), 22 deletions(-) diff --git a/src/api/initialDataTypes.js b/src/api/initialDataTypes.js index c7c2bb04217..1e8dcee1438 100644 --- a/src/api/initialDataTypes.js +++ b/src/api/initialDataTypes.js @@ -111,8 +111,8 @@ export type RawInitialDataRealmUser = {| enter_sends: boolean, full_name: string, is_admin: boolean, - realm_users: Array<{| ...User, avatar_url: string | null |}>, - realm_non_active_users: Array<{| ...User, avatar_url: string | null |}>, + realm_users: Array<{| ...User, avatar_url: string | void | null |}>, + realm_non_active_users: Array<{| ...User, avatar_url: string | void | null |}>, user_id: number, |}; diff --git a/src/api/messages/getMessages.js b/src/api/messages/getMessages.js index a37a4859ba7..3e0ebc93589 100644 --- a/src/api/messages/getMessages.js +++ b/src/api/messages/getMessages.js @@ -54,6 +54,7 @@ export const migrateMessages = (messages: ServerMessage[], identity: Identity): avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, email: message.sender_email, + userId: message.sender_id, realm: identity.realm, }), reactions: reactions.map(reaction => { diff --git a/src/api/registerForEvents.js b/src/api/registerForEvents.js index be70ae5c56a..d0febbd6a9c 100644 --- a/src/api/registerForEvents.js +++ b/src/api/registerForEvents.js @@ -18,15 +18,24 @@ type RegisterForEventsParams = {| client_capabilities?: {| notification_settings_null: boolean, bulk_message_deletion: boolean, + user_avatar_url_field_optional: boolean, |}, |}; -const transformUser = (rawUser: {| ...User, avatar_url: string | null |}, realm: URL): User => { +const transformUser = ( + rawUser: {| ...User, avatar_url: string | void | null |}, + realm: URL, +): User => { const { avatar_url: rawAvatarUrl, email } = rawUser; return { ...rawUser, - avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm }), + avatar_url: AvatarURL.fromUserOrBotData({ + rawAvatarUrl, + email, + userId: rawUser.user_id, + realm, + }), }; }; @@ -34,11 +43,11 @@ const transformCrossRealmBot = ( rawCrossRealmBot: {| ...CrossRealmBot, avatar_url: string | void | null |}, realm: URL, ): CrossRealmBot => { - const { avatar_url: rawAvatarUrl, email } = rawCrossRealmBot; + const { avatar_url: rawAvatarUrl, user_id: userId, email } = rawCrossRealmBot; return { ...rawCrossRealmBot, - avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm }), + avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm }), }; }; diff --git a/src/boot/store.js b/src/boot/store.js index cfaff5905ef..4752f0980d6 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -11,7 +11,7 @@ import { persistStore, autoRehydrate } from '../third/redux-persist'; import type { Config } from '../third/redux-persist'; import { ZulipVersion } from '../utils/zulipVersion'; -import { GravatarURL, UploadedAvatarURL } from '../utils/avatar'; +import { GravatarURL, UploadedAvatarURL, FallbackAvatarURL } from '../utils/avatar'; import type { Action, GlobalState } from '../types'; import config from '../config'; import { REHYDRATE } from '../actionConstants'; @@ -298,6 +298,11 @@ const customReplacer = (key, value, defaultReplacer) => { data: UploadedAvatarURL.serialize(value), [SERIALIZED_TYPE_FIELD_NAME]: 'UploadedAvatarURL', }; + } else if (value instanceof FallbackAvatarURL) { + return { + data: FallbackAvatarURL.serialize(value), + [SERIALIZED_TYPE_FIELD_NAME]: 'FallbackAvatarURL', + }; } return defaultReplacer(key, value); }; @@ -314,6 +319,8 @@ const customReviver = (key, value, defaultReviver) => { return GravatarURL.deserialize(data); case 'UploadedAvatarURL': return UploadedAvatarURL.deserialize(data); + case 'FallbackAvatarURL': + return FallbackAvatarURL.deserialize(data); default: // Fall back to defaultReviver, below } diff --git a/src/events/eventToAction.js b/src/events/eventToAction.js index d798f88ba2f..174dba0d407 100644 --- a/src/events/eventToAction.js +++ b/src/events/eventToAction.js @@ -89,6 +89,7 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl: event.message.avatar_url, email: event.message.sender_email, + userId: event.message.sender_id, realm: getCurrentRealm(state), }), }, @@ -132,7 +133,7 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { switch (event.op) { case 'add': { - const { avatar_url: rawAvatarUrl, email } = event.person; + const { avatar_url: rawAvatarUrl, user_id: userId, email } = event.person; return { type: EVENT_USER_ADD, id: event.id, @@ -141,6 +142,7 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { ...event.person, avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, + userId, email, realm, }), @@ -149,14 +151,15 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { } case 'update': { - const existingUser = getAllUsersById(state).get(event.person.user_id); + const { user_id: userId } = event.person; + const existingUser = getAllUsersById(state).get(userId); if (!existingUser) { // If we get one of these events and don't have // information on the user, there's nothing to do about // it. But it's probably a bug, so, tell Sentry. logging.warn( "`realm_user` event with op `update` received for a user we don't know about", - { userId: event.person.user_id }, + { userId }, ); return { type: 'ignore' }; } @@ -164,7 +167,7 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { return { type: EVENT_USER_UPDATE, id: event.id, - userId: event.person.user_id, + userId, // Just the fields we want to overwrite. person: { // Note: The `avatar_url` field will be out of sync with @@ -175,6 +178,7 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { ? { avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl: event.person.avatar_url, + userId, email: existingUser.email, realm, }), diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index 90b17f919ae..8210f135b90 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -326,6 +326,7 @@ export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetStat client_capabilities: { notification_settings_null: true, bulk_message_deletion: true, + user_avatar_url_field_optional: true, }, }), ), diff --git a/src/utils/__tests__/avatar-test.js b/src/utils/__tests__/avatar-test.js index 8f264e2582d..a559b8acb0c 100644 --- a/src/utils/__tests__/avatar-test.js +++ b/src/utils/__tests__/avatar-test.js @@ -1,18 +1,25 @@ /* @flow strict-local */ import md5 from 'blueimp-md5'; -import { AvatarURL, GravatarURL, UploadedAvatarURL } from '../avatar'; +import { AvatarURL, GravatarURL, FallbackAvatarURL, UploadedAvatarURL } from '../avatar'; import * as eg from '../../__tests__/lib/exampleData'; describe('AvatarURL', () => { describe('fromUserOrBotData', () => { const user = eg.makeUser(); - const { email } = user; + const { email, user_id: userId } = user; const realm = eg.realm; + test('gives a `FallbackAvatarURL` if `rawAvatarURL` is undefined', () => { + const rawAvatarUrl = undefined; + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( + FallbackAvatarURL, + ); + }); + test('gives a `GravatarURL` if `rawAvatarURL` is null', () => { const rawAvatarUrl = null; - expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm })).toBeInstanceOf( + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( GravatarURL, ); }); @@ -20,7 +27,7 @@ describe('AvatarURL', () => { test('gives a `GravatarURL` if `rawAvatarURL` is a URL string on Gravatar origin', () => { const rawAvatarUrl = 'https://secure.gravatar.com/avatar/2efaec12efd9bea8a089299208117786?d=identicon&version=3'; - expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm })).toBeInstanceOf( + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( GravatarURL, ); }); @@ -28,7 +35,7 @@ describe('AvatarURL', () => { test('gives an `UploadedAvatarURL` if `rawAvatarURL` is a non-Gravatar absolute URL string', () => { const rawAvatarUrl = 'https://zulip-avatars.s3.amazonaws.com/13/430713047f2cffed661f84e139a64f864f17f286?x=x&version=5'; - expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm })).toBeInstanceOf( + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( UploadedAvatarURL, ); }); @@ -36,7 +43,7 @@ describe('AvatarURL', () => { test('gives an `UploadedAvatarURL` if `rawAvatarURL` is a relative URL string', () => { const rawAvatarUrl = '/user_avatars/2/08fb6d007eb10a56efee1d64760fbeb6111c4352.png?x=x&version=2'; - expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm })).toBeInstanceOf( + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( UploadedAvatarURL, ); }); @@ -144,3 +151,28 @@ describe('UploadedAvatarURL', () => { }); }); }); + +describe('FallbackAvatarURL', () => { + test('serializes/deserializes correctly', () => { + const instance = FallbackAvatarURL.validateAndConstructInstance({ + realm: eg.realm, + userId: eg.selfUser.user_id, + }); + + const roundTripped = FallbackAvatarURL.deserialize(FallbackAvatarURL.serialize(instance)); + + SIZES_WE_USE.forEach(size => { + expect(instance.get(size).toString()).toEqual(roundTripped.get(size).toString()); + }); + }); + + test('gives the `/avatar/{user_id}` URL, on the provided realm', () => { + const userId = eg.selfUser.user_id; + const instance = FallbackAvatarURL.validateAndConstructInstance({ + realm: new URL('https://chat.zulip.org'), + userId, + }); + + expect(instance.get().toString()).toEqual(`https://chat.zulip.org/avatar/${userId.toString()}`); + }); +}); diff --git a/src/utils/avatar.js b/src/utils/avatar.js index 49a117522b5..bf8b203ba47 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -18,15 +18,25 @@ export class AvatarURL { */ static fromUserOrBotData(args: {| rawAvatarUrl: string | void | null, + userId: number, email: string, realm: URL, |}): AvatarURL { - const { rawAvatarUrl, email, realm } = args; + const { rawAvatarUrl, userId, email, realm } = args; if (rawAvatarUrl === undefined) { - // `rawAvatarUrl` will be undefined for cross-realm bots from - // servers prior to 58ee3fa8c (1.9.0). Fall back to Gravatar for - // this case; it should be pretty rare. - return GravatarURL.validateAndConstructInstance({ email }); + // New in Zulip 3.0, feature level 18, the field may be missing + // on user objects in the register response, at the server's + // discretion, if we announce the + // `user_avatar_url_field_optional` client capability, which we + // do. See the note about `user_avatar_url_field_optional` at + // https://zulipchat.com/api/register-queue. + // + // It will also be absent on cross-realm bots from servers prior + // to 58ee3fa8c (1.9.0). The effect of using FallbackAvatarURL for + // this case isn't thoroughly considered, but at worst, it means a + // 404. We could plumb through the server version and + // conditionalize on that. + return FallbackAvatarURL.validateAndConstructInstance({ realm, userId }); } else if (rawAvatarUrl === null) { // If we announce `client_gravatar`, which we do, `rawAvatarUrl` // might be null. In that case, we take responsibility for @@ -156,6 +166,75 @@ export class GravatarURL extends AvatarURL { } } +/** + * The /avatar/{user_id} redirect. + * + * See the point on `user_avatar_url_field_optional` at + * https://zulipchat.com/api/register-queue. + * + * This endpoint does not currently support size customization. + */ +export class FallbackAvatarURL extends AvatarURL { + /** + * Serialize to a special string; reversible with `deserialize`. + */ + static serialize(instance: FallbackAvatarURL): string { + return instance._standardUrl instanceof URL + ? instance._standardUrl.toString() + : instance._standardUrl; + } + + /** + * Use a special string from `serialize` to make a new instance. + */ + static deserialize(serialized: string): FallbackAvatarURL { + return new FallbackAvatarURL(serialized); + } + + /** + * Construct from raw server data, or throw an error. + */ + static validateAndConstructInstance(args: {| realm: URL, userId: number |}): FallbackAvatarURL { + const { realm, userId } = args; + return new FallbackAvatarURL(new URL(`/avatar/${userId.toString()}`, realm.origin)); + } + + /** + * Standard URL from which to generate others. PRIVATE. + * + * May be a string if the instance was constructed at rehydrate + * time, when URL validation is unnecessary. + */ + _standardUrl: string | URL; + + /** + * PRIVATE: Make an instance from already-validated data. + * + * Not part of the public interface; use the static methods instead. + * + * It's private because we need a path to constructing an instance + * without constructing URL objects, which takes more time than is + * acceptable when we can avoid it, e.g., during rehydration. + * Constructing URL objects is a necessary part of validating data + * from the server, but we only need to validate the data once, when + * it's first received. + */ + constructor(standardUrl: string | URL) { + super(); + this._standardUrl = standardUrl; + } + + get(size: number | void): URL { + // `this._standardUrl` may have begun its life as a string, to + // avoid computing a URL object during rehydration + if (typeof this._standardUrl === 'string') { + this._standardUrl = new URL(this._standardUrl); + } + + return this._standardUrl; + } +} + /** * An avatar that was uploaded to the Zulip server. *