Skip to content

Commit

Permalink
registerForEvents: Start sending user_avatar_url_field_optional.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
chrisbobbe committed Sep 22, 2020
1 parent a40d4d3 commit 57c3fdf
Show file tree
Hide file tree
Showing 6 changed files with 140 additions and 16 deletions.
5 changes: 3 additions & 2 deletions src/api/registerForEvents.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,19 @@ type RegisterForEventsParams = {|
client_capabilities?: {|
notification_settings_null: boolean,
bulk_message_deletion: boolean,
user_avatar_url_field_optional: boolean,
|},
|};

export const migrateUserOrBot = <T: User | CrossRealmBot>(
rawUserOrBot: $FlowFixMe, // server data pre-transformation
realm: URL,
): T => {
const { avatar_url: rawAvatarUrl, email } = rawUserOrBot;
const { avatar_url: rawAvatarUrl, user_id: userId, email } = rawUserOrBot;

return {
...rawUserOrBot,
avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm }),
avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm }),
};
};

Expand Down
9 changes: 8 additions & 1 deletion src/boot/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -299,6 +299,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);
};
Expand All @@ -315,6 +320,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
}
Expand Down
9 changes: 6 additions & 3 deletions src/events/eventToAction.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,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,
Expand All @@ -141,16 +141,18 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => {
...event.person,
avatar_url: AvatarURL.fromUserOrBotData({
rawAvatarUrl,
userId,
email,
realm,
}),
},
};
}
case 'update': {
const { user_id: userId } = event.person;
let existingUser: UserOrBot;
try {
existingUser = getUserForId(state, event.person.user_id);
existingUser = getUserForId(state, userId);
} catch (e) {
// We don't store deactivated users. We don't know whether
// these events are actually emitted for deactivated users
Expand All @@ -165,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
Expand All @@ -176,6 +178,7 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => {
? {
avatar_url: AvatarURL.fromUserOrBotData({
rawAvatarUrl: event.person.avatar_url,
userId,
email: existingUser.email,
realm,
}),
Expand Down
1 change: 1 addition & 0 deletions src/message/fetchActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,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,
},
}),
),
Expand Down
37 changes: 31 additions & 6 deletions src/utils/__tests__/avatar-test.js
Original file line number Diff line number Diff line change
@@ -1,42 +1,42 @@
/* @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 `GravatarURL` if `rawAvatarURL` is null', () => {
const rawAvatarUrl = null;
expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm })).toBeInstanceOf(
expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf(
GravatarURL,
);
});

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,
);
});

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,
);
});

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,
);
});
Expand Down Expand Up @@ -181,3 +181,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()}`);
});
});
95 changes: 91 additions & 4 deletions src/utils/avatar.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ export class AvatarURL {
* defined.
*/
static fromUserOrBotData: (args: {|
rawAvatarUrl: string | null,
rawAvatarUrl: string | void | null,
userId: number,
email: string,
realm: URL,
|}) => AvatarURL;
Expand Down Expand Up @@ -125,6 +126,78 @@ 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 locally, or to an S3 backend.
*
Expand Down Expand Up @@ -218,12 +291,26 @@ export class UploadedAvatarURL extends AvatarURL {
}

AvatarURL.fromUserOrBotData = (args: {|
rawAvatarUrl: string | null,
rawAvatarUrl: string | void | null,
userId: number,
email: string,
realm: URL,
|}): AvatarURL => {
const { rawAvatarUrl, email, realm } = args;
if (rawAvatarUrl === null) {
const { rawAvatarUrl, userId, email, realm } = args;
if (rawAvatarUrl === undefined) {
// New in Zulip 2.2, feature level 18, the field may be missing 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
// computing a hash for the user's email and using it to form a
Expand Down

0 comments on commit 57c3fdf

Please sign in to comment.