Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

events: Don't send avatar URLs of long term idle users. 

This adds a new client_capability that clients such as the mobile apps
can use to avoid unreasonable network bandwidth consumed sending
avatar URLs in organizations with 10,000s of users.

Clients don't strictly need this data, as they can always use the
/avatar/{user_id} endpoint to fetch the avatar if desired.

This will be more efficient especially for realms with
10,000+ users because the avatar URLs would increase the
payload size significantly and cost us more bandwidth.

Fixes #15287.
This commit is contained in:
Hashir Sarwar 2020-06-13 13:10:05 +05:00 committed by Tim Abbott
parent 9911ec3e6d
commit 5200598a31
9 changed files with 131 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
templates/zerver/api/changelog.md
View File

@ -10,6 +10,11 @@ below features are supported.
## Changes in Zulip 2.2
**Feature level 18**
* [`POST /register`](/api/register-queue): Added
`user_avatar_url_field_optional` to supported `client_capabilities`.
**Feature level 17**
* [`GET users/me/subscriptions`](/api/get-subscribed-streams), [`GET /streams`]

2
version.py
View File

@ -29,7 +29,7 @@ DESKTOP_WARNING_VERSION = "5.2.0"
#
# Changes should be accompanied by documentation explaining what the
# new level means in templates/zerver/api/changelog.md.
API_FEATURE_LEVEL = 17
API_FEATURE_LEVEL = 18
# Bump the minor PROVISION_VERSION to indicate that folks should provision
# only when going from an old version of the code to a newer version. Bump

4
zerver/lib/actions.py
View File

@ -504,8 +504,10 @@ def notify_created_user(user_profile: UserProfile) -> None:
person = format_user_row(user_profile.realm, user_profile, user_row,
# Since we don't know what the client
# supports at this point in the code, we
# just assume client_gravatar=False :(
# just assume client_gravatar and
# user_avatar_url_field_optional = False :(
client_gravatar=False,
user_avatar_url_field_optional=False,
# We assume there's no custom profile
# field data for a new user; initial
# values are expected to be added in a

6
zerver/lib/events.py
View File

@ -88,6 +88,7 @@ def always_want(msg_type: str) -> bool:
def fetch_initial_state_data(user_profile: UserProfile,
event_types: Optional[Iterable[str]],
queue_id: str, client_gravatar: bool,
user_avatar_url_field_optional: bool,
slim_presence: bool = False,
include_subscribers: bool = True) -> Dict[str, Any]:
state: Dict[str, Any] = {'queue_id': queue_id}
@ -208,7 +209,8 @@ def fetch_initial_state_data(user_profile: UserProfile,
if want('realm_user'):
state['raw_users'] = get_raw_user_data(realm, user_profile,
client_gravatar=client_gravatar)
client_gravatar=client_gravatar,
user_avatar_url_field_optional=user_avatar_url_field_optional)
# For the user's own avatar URL, we force
# client_gravatar=False, since that saves some unnecessary
@ -848,6 +850,7 @@ def do_events_register(user_profile: UserProfile, user_client: Client,
notification_settings_null = client_capabilities.get('notification_settings_null', False)
bulk_message_deletion = client_capabilities.get('bulk_message_deletion', False)
user_avatar_url_field_optional = client_capabilities.get('user_avatar_url_field_optional', False)
if user_profile.realm.email_address_visibility != Realm.EMAIL_ADDRESS_VISIBILITY_EVERYONE:
# If real email addresses are not available to the user, their
@ -877,6 +880,7 @@ def do_events_register(user_profile: UserProfile, user_client: Client,
ret = fetch_initial_state_data(user_profile, event_types_set, queue_id,
client_gravatar=client_gravatar,
user_avatar_url_field_optional=user_avatar_url_field_optional,
slim_presence=slim_presence,
include_subscribers=include_subscribers)

53
zerver/lib/users.py
View File

@ -303,7 +303,7 @@ def compute_show_invites_and_add_streams(user_profile: Optional[UserProfile]) ->
return True, True
def format_user_row(realm: Realm, acting_user: UserProfile, row: Dict[str, Any],
client_gravatar: bool,
client_gravatar: bool, user_avatar_url_field_optional: bool,
custom_profile_field_data: Optional[Dict[str, Any]] = None) -> Dict[str, Any]:
"""Formats a user row returned by a database fetch using
.values(*realm_user_dict_fields) into a dictionary representation
@ -311,23 +311,13 @@ def format_user_row(realm: Realm, acting_user: UserProfile, row: Dict[str, Any],
argument is used for permissions checks.
"""
avatar_url = get_avatar_field(user_id=row['id'],
realm_id=realm.id,
email=row['delivery_email'],
avatar_source=row['avatar_source'],
avatar_version=row['avatar_version'],
medium=False,
client_gravatar=client_gravatar)
is_admin = is_administrator_role(row['role'])
is_owner = row['role'] == UserProfile.ROLE_REALM_OWNER
is_guest = row['role'] == UserProfile.ROLE_GUEST
is_bot = row['is_bot']
# This format should align with get_cross_realm_dicts() and notify_created_user
result = dict(
email=row['email'],
user_id=row['id'],
avatar_url=avatar_url,
avatar_version=row['avatar_version'],
is_admin=is_admin,
is_owner=is_owner,
@ -338,6 +328,35 @@ def format_user_row(realm: Realm, acting_user: UserProfile, row: Dict[str, Any],
is_active = row['is_active'],
date_joined = row['date_joined'].isoformat(),
)
# Zulip clients that support using `GET /avatar/{user_id}` as a
# fallback if we didn't send an avatar URL in the user object pass
# user_avatar_url_field_optional in client_capabilities.
#
# This is a major network performance optimization for
# organizations with 10,000s of users where we would otherwise
# send avatar URLs in the payload (either because most users have
# uploaded avatars or because EMAIL_ADDRESS_VISIBILITY_ADMINS
# prevents the older client_gravatar optimization from helping).
# The performance impact is large largely because the hashes in
# avatar URLs structurally cannot compress well.
#
# The user_avatar_url_field_optional gives the server sole
# discretion in deciding for which users we want to send the
# avatar URL (Which saves clients an RTT at the cost of some
# bandwidth). At present, the server looks at `long_term_idle` to
# decide which users to include avatars for, piggy-backing on a
# different optimization for organizations with 10,000s of users.
include_avatar_url = not user_avatar_url_field_optional or not row['long_term_idle']
if include_avatar_url:
result['avatar_url'] = get_avatar_field(user_id=row['id'],
realm_id=realm.id,
email=row['delivery_email'],
avatar_source=row['avatar_source'],
avatar_version=row['avatar_version'],
medium=False,
client_gravatar=client_gravatar)
if (realm.email_address_visibility == Realm.EMAIL_ADDRESS_VISIBILITY_ADMINS and
acting_user.is_realm_admin):
result['delivery_email'] = row['delivery_email']
@ -396,6 +415,7 @@ def get_cross_realm_dicts() -> List[Dict[str, Any]]:
acting_user=user,
row=user_row,
client_gravatar=False,
user_avatar_url_field_optional=False,
custom_profile_field_data=None))
return result
@ -416,8 +436,8 @@ def get_custom_profile_field_values(custom_profile_field_values:
}
return profiles_by_user_id
def get_raw_user_data(realm: Realm, acting_user: UserProfile, client_gravatar: bool,
target_user: Optional[UserProfile]=None,
def get_raw_user_data(realm: Realm, acting_user: UserProfile, *, target_user: Optional[UserProfile]=None,
client_gravatar: bool, user_avatar_url_field_optional: bool,
include_custom_profile_fields: bool=True) -> Dict[int, Dict[str, str]]:
"""Fetches data about the target user(s) appropriate for sending to
acting_user via the standard format for the Zulip API. If
@ -447,9 +467,10 @@ def get_raw_user_data(realm: Realm, acting_user: UserProfile, client_gravatar: b
custom_profile_field_data = profiles_by_user_id.get(row['id'], {})
result[row['id']] = format_user_row(realm,
acting_user = acting_user,
acting_user=acting_user,
row=row,
client_gravatar= client_gravatar,
custom_profile_field_data = custom_profile_field_data,
client_gravatar=client_gravatar,
user_avatar_url_field_optional=user_avatar_url_field_optional,
custom_profile_field_data=custom_profile_field_data,
)
return result

9
zerver/openapi/zulip.yaml
View File

@ -3207,6 +3207,15 @@ paths:
in a loop. New in Zulip 2.2 (feature level 13). This
capability is for backwards-compatibility; it will be
required in a future server release.
* `user_avatar_url_field_optional`: Boolean for whether the
client required avatar URLs for all users, or supports
using `GET /avatar/{user_id}` to access user avatars. If the
client has this capability, the server may skip sending a
`avatar_url` field in the `realm_user` at its sole discretion
to optimize network performance. This is an important optimization
in organizations with 10,000s of users.
New in Zulip 2.2 (feature level 18).
schema:
type: object
example:

77
zerver/tests/test_events.py
View File

@ -503,8 +503,8 @@ class EventsRegisterTest(ZulipTestCase):
def do_test(self, action: Callable[[], object], event_types: Optional[List[str]]=None,
include_subscribers: bool=True, state_change_expected: bool=True,
notification_settings_null: bool=False,
client_gravatar: bool=True, slim_presence: bool=False,
notification_settings_null: bool=False, client_gravatar: bool=True,
user_avatar_url_field_optional: bool=False, slim_presence: bool=False,
num_events: int=1, bulk_message_deletion: bool=True) -> List[Dict[str, Any]]:
'''
Make sure we have a clean slate of client descriptors for these tests.
@ -536,6 +536,7 @@ class EventsRegisterTest(ZulipTestCase):
hybrid_state = fetch_initial_state_data(
self.user_profile, event_types, "",
client_gravatar=client_gravatar,
user_avatar_url_field_optional=user_avatar_url_field_optional,
slim_presence=slim_presence,
include_subscribers=include_subscribers,
)
@ -567,6 +568,7 @@ class EventsRegisterTest(ZulipTestCase):
normal_state = fetch_initial_state_data(
self.user_profile, event_types, "",
client_gravatar=client_gravatar,
user_avatar_url_field_optional=user_avatar_url_field_optional,
slim_presence=slim_presence,
include_subscribers=include_subscribers,
)
@ -2052,7 +2054,7 @@ class EventsRegisterTest(ZulipTestCase):
def test_realm_update_plan_type(self) -> None:
realm = self.user_profile.realm
state_data = fetch_initial_state_data(self.user_profile, None, "", False)
state_data = fetch_initial_state_data(self.user_profile, None, "", False, False)
self.assertEqual(state_data['realm_plan_type'], Realm.SELF_HOSTED)
self.assertEqual(state_data['zulip_plan_is_not_limited'], True)
@ -2069,7 +2071,7 @@ class EventsRegisterTest(ZulipTestCase):
error = schema_checker('events[0]', events[0])
self.assert_on_error(error)
state_data = fetch_initial_state_data(self.user_profile, None, "", False)
state_data = fetch_initial_state_data(self.user_profile, None, "", False, False)
self.assertEqual(state_data['realm_plan_type'], Realm.LIMITED)
self.assertEqual(state_data['zulip_plan_is_not_limited'], False)
@ -2846,7 +2848,7 @@ class EventsRegisterTest(ZulipTestCase):
lambda: do_delete_messages(self.user_profile.realm, [message]),
state_change_expected=True,
)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False)
self.assertEqual(result['max_message_id'], -1)
def test_add_attachment(self) -> None:
@ -3069,7 +3071,7 @@ class FetchInitialStateDataTest(ZulipTestCase):
def test_realm_bots_non_admin(self) -> None:
user_profile = self.example_user('cordelia')
self.assertFalse(user_profile.is_realm_admin)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False)
self.assert_length(result['realm_bots'], 0)
# additionally the API key for a random bot is not present in the data
@ -3081,14 +3083,14 @@ class FetchInitialStateDataTest(ZulipTestCase):
user_profile = self.example_user('hamlet')
do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR)
self.assertTrue(user_profile.is_realm_admin)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False)
self.assertTrue(len(result['realm_bots']) > 2)
def test_max_message_id_with_no_history(self) -> None:
user_profile = self.example_user('aaron')
# Delete all historical messages for this user
UserMessage.objects.filter(user_profile=user_profile).delete()
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False)
self.assertEqual(result['max_message_id'], -1)
def test_delivery_email_presence_for_non_admins(self) -> None:
@ -3097,13 +3099,13 @@ class FetchInitialStateDataTest(ZulipTestCase):
do_set_realm_property(user_profile.realm, "email_address_visibility",
Realm.EMAIL_ADDRESS_VISIBILITY_EVERYONE)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False)
for key, value in result['raw_users'].items():
self.assertNotIn('delivery_email', value)
do_set_realm_property(user_profile.realm, "email_address_visibility",
Realm.EMAIL_ADDRESS_VISIBILITY_ADMINS)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False)
for key, value in result['raw_users'].items():
self.assertNotIn('delivery_email', value)
@ -3113,16 +3115,62 @@ class FetchInitialStateDataTest(ZulipTestCase):
do_set_realm_property(user_profile.realm, "email_address_visibility",
Realm.EMAIL_ADDRESS_VISIBILITY_EVERYONE)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False)
for key, value in result['raw_users'].items():
self.assertNotIn('delivery_email', value)
do_set_realm_property(user_profile.realm, "email_address_visibility",
Realm.EMAIL_ADDRESS_VISIBILITY_ADMINS)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False)
result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False)
for key, value in result['raw_users'].items():
self.assertIn('delivery_email', value)
def test_user_avatar_url_field_optional(self) -> None:
hamlet = self.example_user('hamlet')
users = [
self.example_user('iago'),
self.example_user('cordelia'),
self.example_user('ZOE'),
self.example_user('othello'),
]
for user in users:
user.long_term_idle = True
user.save()
long_term_idle_users_ids = [user.id for user in users]
result = fetch_initial_state_data(user_profile=hamlet,
event_types=None,
queue_id='',
client_gravatar=False,
user_avatar_url_field_optional=True)
raw_users = result['raw_users']
for user_dict in raw_users.values():
if user_dict['user_id'] in long_term_idle_users_ids:
self.assertFalse('avatar_url' in user_dict)
else:
self.assertIsNotNone(user_dict['avatar_url'])
gravatar_users_id = [user_dict['user_id'] for user_dict in raw_users.values()
if 'avatar_url' in user_dict and 'gravatar.com' in user_dict['avatar_url']]
# Test again with client_gravatar = True
result = fetch_initial_state_data(user_profile=hamlet,
event_types=None,
queue_id='',
client_gravatar=True,
user_avatar_url_field_optional=True)
raw_users = result['raw_users']
for user_dict in raw_users.values():
if user_dict['user_id'] in gravatar_users_id:
self.assertIsNone(user_dict['avatar_url'])
else:
self.assertFalse('avatar_url' in user_dict)
class GetUnreadMsgsTest(ZulipTestCase):
def mute_stream(self, user_profile: UserProfile, stream: Stream) -> None:
@ -3836,6 +3884,7 @@ class FetchQueriesTest(ZulipTestCase):
event_types=None,
queue_id='x',
client_gravatar=False,
user_avatar_url_field_optional=False
)
self.assert_length(queries, 30)
@ -3892,6 +3941,7 @@ class FetchQueriesTest(ZulipTestCase):
event_types=event_types,
queue_id='x',
client_gravatar=False,
user_avatar_url_field_optional=False
)
self.assert_length(queries, count)
@ -3971,7 +4021,8 @@ class TestEventsRegisterNarrowDefaults(ZulipTestCase):
class TestGetRawUserDataSystemBotRealm(ZulipTestCase):
def test_get_raw_user_data_on_system_bot_realm(self) -> None:
result = get_raw_user_data(get_realm("zulipinternal"), self.example_user('hamlet'), True)
result = get_raw_user_data(get_realm("zulipinternal"), self.example_user('hamlet'),
client_gravatar=True, user_avatar_url_field_optional=True)
for bot_email in settings.CROSS_REALM_BOT_EMAILS:
bot_profile = get_system_bot(bot_email)

1
zerver/views/events_register.py
View File

@ -40,6 +40,7 @@ def events_register_backend(
], [
# Any new fields of `client_capabilities` should be optional. Add them here.
("bulk_message_deletion", check_bool),
('user_avatar_url_field_optional', check_bool),
]), default=None),
event_types: Optional[Iterable[str]]=REQ(validator=check_list(check_string), default=None),
fetch_event_types: Optional[Iterable[str]]=REQ(validator=check_list(check_string), default=None),

9
zerver/views/users.py
View File

@ -451,8 +451,9 @@ def get_members_backend(request: HttpRequest, user_profile: UserProfile, user_id
target_user = access_user_by_id(user_profile, user_id, allow_deactivated=True,
allow_bots=True, read_only=True)
members = get_raw_user_data(realm, user_profile, client_gravatar=client_gravatar,
target_user=target_user,
members = get_raw_user_data(realm, user_profile, target_user=target_user,
client_gravatar=client_gravatar,
user_avatar_url_field_optional=False,
include_custom_profile_fields=include_custom_profile_fields)
if target_user is not None:
@ -501,7 +502,9 @@ def create_user_backend(request: HttpRequest, user_profile: UserProfile,
def get_profile_backend(request: HttpRequest, user_profile: UserProfile) -> HttpResponse:
raw_user_data = get_raw_user_data(user_profile.realm, user_profile,
client_gravatar=False, target_user=user_profile)
target_user=user_profile,
client_gravatar=False,
user_avatar_url_field_optional=False)
result: Dict[str, Any] = raw_user_data[user_profile.id]
result['max_message_id'] = -1