Crunchify shell for avatar URLs, handle user_avatar_url_field_optional client capability

[chrisbobbe]

Following #4171 and #4216, here's a new approach to grabbing the correct URL for a user's avatar in a bunch of places, at the right size, in a crunchy-shell way. It also makes it much easier to fix #4157 and fixes it.

Discussion on CZO around here.

Some questions for review:

• While writing tests for AvatarURL.fromMessageData and AvatarURL.fromUserOrBotData, I realized that they're quite similar. I didn't jump in and deduplicate them yet, though, in case that seeming similarity was deceptive. In particular, the rawAvatarUrl param is meant to be raw data straight from the server, and that has a slightly different form between users and messages. More importantly, maybe, it's reasonable to expect that those differences will remain and maybe grow. But maybe not dramatically, and maybe one will always be a subtype of the other.
• I think these changes might increase the typical rehydrate time; I'll post some numbers in a followup comment.
• This handles live-updating of the avatar URL (via EVENT_USER_UPDATE) for user objects in state.users, but not messages in state.messages. So if someone changes their avatar URL, you won't see the new one right away in the message list. There's some work to do before we get there; see discussion on implementation.
• For live-updating on a changed avatar_url, I'm having action.person on EVENT_USER_UPDATE only contain the fields we want to be overwritten in the user object. I've started out small, and I'm only ever having it overwrite the avatar_url property.
  • Does that sound right?
  • One thing I haven't done, which is considered in Handle user update events #3403, is to make our code (in eventToAction, I think) aware of the different well-defined chunks of properties we might get at a time. The nice new documentation for the event shows that a handful of properties will be present if the avatar URL is changed, a different handful will be present if the timezone changes, etc. In this branch, I haven't acknowledged that detail, thinking it's simpler to just go property by property: if it's there, we'll consider updating it; if not, we won't.

Fixes: #4157

---

EDIT:

Fixes: #4305

[chrisbobbe]

I think these changes might increase the typical rehydrate time; I'll post some numbers in a followup comment.

This was on my physical iPhone 6s device, on chat.zulip.org. I put a console.time('rehydrate') just before the timing.start call in src/boot/StoreProvider, and a console.timeEnd('rehydrate') just before the timing.end call there, and took consecutive readings from the console output.

At the tip (edcf172):

4607.139892578125ms
3987.748779296875ms
4329.47412109375ms
4138.198974609375ms
4238.33984375ms
4280.126953125ms
4645.26025390625ms
4009.550048828125ms
4247.946044921875ms
4693.374755859375ms

On master (after applying a dropCache migration to clear out data formed at the tip and loading once so it applies):

3194.426025390625ms
3198.646240234375ms
3151.398193359375ms
4964.14404296875ms
3210.5380859375ms
3419.843017578125ms
3380.66796875ms
3561.69287109375ms
4551.978759765625ms
3733.8232421875ms

[chrisbobbe]

Looks like an unintentional newline here; I'll fix this in my next revision.

[chrisbobbe]

Ok, I just updated this with the performance optimizations we discussed around here.

I'll do another set of measurements after rebasing atop #4235, once that's merged.

[chrisbobbe]

I'll do another set of measurements after rebasing atop #4235, once that's merged.

Marking as blocked by #4235.

[chrisbobbe]

OK, this is no longer blocked by #4235, since it was just merged! I've also gone and updated this branch with those changes, so it's ready for review. And I should do another set of measurements as well.

[chrisbobbe]

And I should do another set of measurements as well.

OK, these are here. It seems to me that they don't demonstrate a worrisome slowdown, but we'll see what @gnprice says. 🙂

[gnprice]

Thanks @chrisbobbe !

Below is a half-done review on this which I apparently didn't send -- just now spotted it in a tab. (Thankfully GitHub does save "pending" review comments.)

IIRC I hadn't finished reading through the branch when I apparently got distracted and forgot about this review. Not sure how far through I'd read; I'll have to figure that out when I return to it to complete the review. But I think these comments will be enough to be helpful, so wanted to send them without further delay.

[chrisbobbe]

Thanks for the review! I just pushed a new revision.

[gnprice]

Thanks @chrisbobbe ! Now finished a review -- comments below.

[gnprice]

This 36 here looks like it changes some behavior. Previously the avatarUrl value here came from getAvatarFromMessage at the caller, and that used the default size of 80.

I think perhaps the root cause of having this behavior change happen in the same commit with the big refactor is that there are two separate implementations of "compute the actual URL from the pattern the server provides in avatar_url": there's the new classes with their methods, and there's the old helper functions like getAvatarFromMessage.

Can we do an NFC commit before this one that turns the latter into thin wrappers around the former? I think that would help isolate any behavior changes, and help confirm we're not making any that are unintended.

[chrisbobbe]

Can we do an NFC commit before this one that turns the latter into thin wrappers around the former?

Hmm, I'm probably missing something, so I'd better check. A commit that turns old helper functions into thin wrappers around new code doesn't sound NFC; is that right? In a commit like that, the old helper functions' old implementations would go from being used to not being used.

[gnprice]

Can we do an NFC commit before this one that turns the latter into thin wrappers around the former?

Hmm, I'm probably missing something, so I'd better check. A commit that turns old helper functions into thin wrappers around new code doesn't sound NFC; is that right? In a commit like that, the old helper functions' old implementations would go from being used to not being used.

Right, so such a commit would stop using the old implementation and start using the new implementation.

Whether that's NFC depends on whether there are differences in behavior between the old and the new implementations. If the new implementation is purely a new way of organizing the same ultimate logic as the old implementation has, so there's no change in behavior, then that commit is NFC.

I think this refactor is supposed to be at least mostly like that, right? At the end there's the commit that adds the fallback behavior -- but before that, IIUC it's not intended to change much behavior, if any. Is there some behavior it does intentionally change? Or, is there some behavior that it's hard to avoid having it change?

And another way to put that question is: if we do, before this commit, a change that looks something like:

 export const getAvatarFromUser = (user: UserOrBot, realm: URL, size?: number): string =>
-  getAvatarUrl(user.avatar_url, user.email, realm, size);
+  AvatarURL.fromUserOrBotData({ rawAvatarUrl: user.avatar_url, email: user.email, realm }).get(size).toString();
 
 export const getAvatarFromMessage = (
   message: Message | Outbox,
   realm: URL,
   size?: number,
-): string => getAvatarUrl(message.avatar_url, message.sender_email, realm, size);
+): string => {
+  const avatarUrl = AvatarURL.fromUserOrBotData({ rawAvatarUrl: message.avatar_url, email: message.sender_email, realm });
+  return avatarUrl.get(size).toString();
+};

then would that be NFC? And if not, why/how not?

If the answer is "yes", then arranging the series so that it contains that commit would be a nice way of demonstrating that. And if the answer is "no", then the detailed answer will be interesting for helping understand the branch... and rearranging things so that there is such a commit, if that's possible, would help make that detailed answer concrete, and help isolate the things that do change (in their own, focused commits) so that they're easier to see and to think about.

[chrisbobbe]

Ah OK, got it, I think—I hadn't been thinking of calling .toString() and returning a string, as you do in that example; that does make it easier to focus on how it might be NFC.

You've pointed to two exported functions, getAvatarFromUser and getAvatarFromMessage, being used to compute the actual URL from the pattern the server provides in avatar_url. Awkwardly, there's a third: getAvatarUrl. I have a hunch that it began its life as a helper function for those other two, but that UserAvatarWithPresence saw it as an attractive option because it's sometimes passed info about a user, and sometimes about a message. I suspect we can make the same kind of change to getAvatarUrl as you point out for those other two, though.

I find a few things that could be described as functional changes; maybe they're pretty minor (?):

In the old code, there's a tiny bug stemming from an assumption that, if avatar_url is a string, it won't be a Gravatar URL. There are in fact two cases where we'll get a string that represents a Gravatar URL. These are noted in comments on AvatarURL.fromUserOrBotData.

• If we don't announce client_gravatar. At that point in the branch, we don't announce it in getMessages. The server doesn't know that we can compute the Gravatar URL ourselves, so it just gives it to us.
• If the server doesn't have EMAIL_ADDRESS_VISIBILITY_EVERYONE set. When that's set, real email addresses don't get sent to the client. But I think real email addresses are still supposed to determine Gravatar URLs, since I might have told Gravatar what my email address is, and what image to show for me. The server can't expect the client to compute the Gravatar URL for the real email address, since the server isn't sending real email addresses. So they just give the client the Gravatar URL for the real email address.

Reading past the first conditional in getAvatarUrl, we appear to assume that the avatar_url string will correspond to a relative or absolute URL for an uploaded avatar. This code is mostly harmless to a Gravatar URL string that has snuck through for one of the above reasons. It's an absolute URL, so realm in new URL(avatarUrl, realm) will be ignored. And if size is greater than 100, it'll be run through a regex, but that's a no-op because Gravatar URLs don't have .png in them.

I say it's "mostly harmless" because there is some behavior we probably don't intend: a Gravatar URL string sneaking through in these cases won't ever be set to the desired or Zulip-default size (80). The way to set a Gravatar URL to a certain size is to pass a query param s, like s=100. And that's just not done in these cases because the code isn't written to be aware that it might be working with a Gravatar URL.

The new code handles these cases by recognizing that we're dealing with Gravatar URLs, and they will be sized appropriately.

The old code uses a default size of 80. The new code doesn't use a default size, but treats a not-provided size as saying "I don't really mind what size I get." For Gravatar URLs, this means not passing the s query param, and leaving it up to Gravatar to pick a size. For uploaded avatars, it means using the non--medium.png version, which is what we also do when we pass a size that's 100 or less.

[gnprice]

You've pointed to two exported functions, getAvatarFromUser and getAvatarFromMessage [...] I suspect we can make the same kind of change to getAvatarUrl as you point out for those other two, though.

Ah, good catch, yes. I think that means a switchover commit could just edit getAvatarUrl instead of those two callers of it.

---

• If the server doesn't have EMAIL_ADDRESS_VISIBILITY_EVERYONE set. When that's set, real email addresses don't get sent to the client. But I think real email addresses are still supposed to determine Gravatar URLs, since I might have told Gravatar what my email address is, and what image to show for me. The server can't expect the client to compute the Gravatar URL for the real email address, since the server isn't sending real email addresses. So they just give the client the Gravatar URL for the real email address.

Yep, this is correct -- when emails are hidden, the server still sends Gravatar URLs based on the real emails. (I'm not sure this is documented anywhere; I don't see it in the user-facing docs. But it's been confirmed by Tim in chat threads.)

---

2. The old code uses a default size of 80. The new code doesn't use a default size, but treats a not-provided size as saying "I don't really mind what size I get." For Gravatar URLs, this means not passing the s query param, and leaving it up to Gravatar to pick a size. For uploaded avatars, it means using the non--medium.png version, which is what we also do when we pass a size that's 100 or less.

Ah, that summary is helpful, thanks.

Looking up what Gravatar does in that case... its default is 80! I'd guess that's where our code's default of 80 came from, now that I see that. 🙂 So I think that means this doesn't actually represent a change in behavior, once that small fact from the Gravatar API (which is the API we're effectively using when constructing these Gravatar URLs) is taken into account.

---

1. In the old code, there's a tiny bug stemming from an assumption that, if avatar_url is a string, it won't be a Gravatar URL. There are in fact two cases where we'll get a string that represents a Gravatar URL.

Mmm, yeah. And in master, or at the beginning of the branch, there's a third: the "double-processing" bug means we produce a Gravatar URL string prematurely, and then pass it through this avatar-URL logic again.

I say it's "mostly harmless" because there is some behavior we probably don't intend: a Gravatar URL string sneaking through in these cases won't ever be set to the desired [size]. The way to set a Gravatar URL to a certain size is to pass a query param s, like s=100. And that's just not done in these cases because the code isn't written to be aware that it might be working with a Gravatar URL.

Ah indeed. I think I learned about some of this shortly after I wrote the comment above, in fact: while writing up and repro'ing #4305 (which grew from this comment elsewhere on the present PR: #4230 (comment) ). It's one reason the symptoms of that issue are different between Gravatars and uploaded avatars; though both cases are buggy, because when we do specify a size it's always too small (that's the core of #4305.)

I think this point gives us an example of this idea I described above, then:

And if the answer is "no", then the detailed answer will be interesting for helping understand the branch... and rearranging things so that there is such a commit, if that's possible, would help make that detailed answer concrete, and help isolate the things that do change (in their own, focused commits) so that they're easier to see and to think about.

Specifically: how about a commit early in the branch that does the equivalent of this:

      if (tryParseUrl(rawAvatarUrl)?.origin === GravatarURL.ORIGIN) {
        const hashMatch = /[0-9a-fA-F]{32}$/.exec(new URL(rawAvatarUrl).pathname);
        if (hashMatch === null) {
          const msg = 'Unexpected Gravatar URL shape from server.';
          logging.error(msg, { value: rawAvatarUrl });
          throw new Error(msg);
        }
        return GravatarURL.validateAndConstructInstance({ email, hash: hashMatch[0] });
      }

but within the string-heavy, no-classes idiom of the existing code? I think that'd be a pretty small commit. The hard part here was in your sorting out the exact set of different situations that can occur. That commit would be a way of expressing this piece of what you learned; its commit message would probably reuse a chunk of the text from your comment here in this subthread.

---

Then with that change to fix the Gravatar-URL-string case, plus the "double-processing" fix... plus either a commit to adjust the old implementation's default-size behavior to match the new one's, or tweaking the new one's to be more like the old one's... I think at that point a switchover commit like the one I described would indeed be NFC. Does that sound right?

[chrisbobbe]

Does that sound right?

Yes, it does! Thanks for these helpful pointers.

[gnprice]

Could reduce the churn here by keeping the computation up at the const avatarUrl = line above. I think that also keeps this template a bit easier to read.

[chrisbobbe]

Sure, makes sense.

What do you think about the name of avatarUrl in the const avatarUrl = line you mentioned? Should we consider something to mark it as storing a specific usable URL with a size (like avatarUrlStr)? I'm wondering the same thing about certain props that are addressed in your comment below (below?), #4230 (comment).

[gnprice]

What do you think about the name of avatarUrl in the const avatarUrl = line you mentioned? Should we consider something to mark it as storing a specific usable URL with a size (like avatarUrlStr)? I'm wondering the same thing about certain props that are addressed in your comment below (below?), #4230 (comment).

I think in the setup we've been in, where an "avatar URL" might be a sort of template for a URL or might be an actual concrete URL to go fetch, and in either case would be a string or some nullish value, that kind of distinction could help a fair bit to reduce the confusion about when we have a template and when we have a specific usable URL.

But after the changes in this PR, we have AvatarURL for the templates, and we only use string -- apart from at the edge, taking data straight from the server -- for actual concrete URLs. A type distinction is easier to consistently keep up than a naming distinction, and I think using avatarUrl for both types of things is fine.

[gnprice]

Perhaps another angle on the last couple of comments: this commit has the job of changing the shape of Message, and that's enough to make it a significant commit.

When we're doing that, I think we should be able to leave unchanged which points in the code we take a user's avatar URL base and turn it into a specific usable URL with a size: we can call the new classes' get methods at the same spots where we've been calling getAvatarFromMessage and friends. So this is one of those spots.

Possibly then in a followup commit we'll want to change the types of things like this avatarUrl prop, and move around the get calls accordingly.

[chrisbobbe]

Thanks for the review! I've just pushed a new revision, but it doesn't yet address some concerns about preventing unintentional changes:

#4230 (comment)
#4230 (comment)
#4230 (comment)

Partly because I'll be at reduced capacity for a few days with my upcoming wrist surgery, so pushing some work seems better than pushing none, but also because I've posted a few questions on those comments that I'd appreciate your thoughts on.

[chrisbobbe]

Thanks for the review! I've just pushed a new revision, but it doesn't yet address some concerns about preventing unintentional changes

I've just pushed a revision that should hopefully make things clearer, following #4230 (comment) (but also please see #4230 (comment)).

[chrisbobbe]

Go ahead and merge at will after handling these. Since I think that'll happen soon, I won't do a partial merge now. (Merging as part of merging the whole PR is slightly nicer just because it means when someone looks up the commit on GitHub, it shows a subtle link to the PR it was part of.)

OK, done! Thanks again for the reviews! 🙂