-
-
Notifications
You must be signed in to change notification settings - Fork 642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
api: Stop assuming fetched messages' reactions have deprecated user
field
#5773
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking care of this! Comments below.
src/utils/avatar.js
Outdated
ensureUnreachable(rawAvatarUrl); | ||
const msg = 'Unexpected value for `rawAvatarUrl` in `AvatarURL.fromUserOrBotData`'; | ||
logging.error(msg, { value: rawAvatarUrl }); | ||
throw new Error(msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avatar: Remove redundant logging.error when we're already throwing an error
This would produce log noise in the Jest tests in an upcoming commit.
Hmm, how do we end up reaching this code in the tests? The ensureUnreachable
means the type-checker should verify it can't be reached given our types, and we generally write our tests to be well-typed.
I think the reason the logging.error
is there in addition to the throw is that it adds the extra data of what the unexpected rawAvatarUrl
was; that's what makes it not redundant with the throw. Happy to remove it if it's getting in the way somehow, though — it looks like we see zero cases of this error in Sentry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Answered below: #5773 (comment))
const { user, ...restReaction } = reaction; | ||
return { | ||
...restReaction, | ||
user_id: user.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof. Yeah, what we should have done long ago would be to make this say user.id ?? restReaction.user_id
(or perhaps, equivalently, user.id ?? user_id
after saying const { user_id, user, ...restReaction }
above… or perhaps better yet, write the fallback the other way around with modern first, like user_id ?? user.id
).
Then it would have already been forward-compatible with an eventual server version that dropped the old user
property, without really being materially more complicated.
// Flow would complain at `faultyReaction` if it | ||
// Flow would complain at `avatar_url` if it | ||
// type-checked `response`, but we should ignore it if that | ||
// day comes. It's badly shaped on purpose. | ||
messages: [{ ...fetchedMessage1, reactions: [faultyReaction] }], | ||
// day comes. It's badly typed on purpose. | ||
messages: [{ ...fetchedMessage1, avatar_url: 123 }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see now the answer to my first question (in utils/avatar.js).
// Simulate #4156, a real-life problem that a user at server commit | ||
// 0af2f9d838 ran into [1], by having `user` be missing on reactions | ||
// on a message: | ||
// https://github.com/zulip/zulip-mobile/issues/4156#issuecomment-655905093 | ||
const store = mockStore<GlobalState, Action>(baseState); | ||
|
||
// Missing `user` (and `user_id`, for good measure), to | ||
// simulate #4156. | ||
const faultyReaction = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the comment "Simulate #…, a real-life problem that a user ran into [link]" is helpful — this is otherwise a pretty weird-looking test.
Given that context, and given this latter comment too — which says user_id
is missing for good measure — what happens if we leave the test unchanged, with user_id
still missing? … Ah, I guess the point is that this is testing what happens if transformFetchedMessage
(or anything else within the migrateResponse
helper of api.getMessages
) throws an exception. Which doesn't happen for just any malformed data, only for malformations in the data it actually has any transformations to perform on.
// day comes. It's badly shaped on purpose. | ||
messages: [{ ...fetchedMessage1, reactions: [faultyReaction] }], | ||
// day comes. It's badly typed on purpose. | ||
messages: [{ ...fetchedMessage1, avatar_url: 123 }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From reading the avatars code (since that's now the only part of transformFetchedMessages
that seems like it could throw an error on bad server data), here's a different fun way to cause an exception:
messages: [{ ...fetchedMessage1, avatar_url: null, sender_email: undefined }],
That causes the email.toLowerCase()
here to throw:
`${GravatarURL.ORIGIN}/avatar/${md5(email.toLowerCase())}?d=identicon`,
and doesn't provoke any logging
calls.
(The sender_email: undefined
is a crude way to get the same effect in the consuming code as if sender_email
were omitted altogether. Could also use a line or two more code here and do that properly.)
c63396b
to
bacded8
Compare
Thanks for the review! Revision pushed. Do you still see something to do for #5773 (comment), to make it a less weird-looking test? |
Thanks!
I think the comment was helpful for that. Here's an updated comment: + // Regression test for #4156. There was a server bug that caused
+ // message data to be malformed, and though it was only briefly in
+ // main, a user did run into it in real life:
+ // https://github.com/zulip/zulip-mobile/issues/4156#issuecomment-655905093 Otherwise all looks good — I'll add that comment and merge. |
… field Our code at the edge for transforming the old format of reactions, with `user`, into the new format, with `user_id`, was depending on `user` always being present. Since `user_id` is documented as being added in Zulip 3.0: https://zulip.com/api/get-messages#response , and since zulip#5747 we've been refusing to connect to servers pre-3.0, we can assume `user_id` is always present and stop reading `user`. Fixes: zulip#4072
bacded8
to
df1dd79
Compare
But this isn't a regression test for #4156, is it? That bug was that we expected a With this PR, we stop expecting The test itself doesn't do anything interesting with When we added this test in #4205, I wrote in a comment:
I guess we removed that bit, in 505f78f. I don't remember why, and I think it's not clear from the PR. |
On the other hand, I guess it makes sense as a regression test for the kind of issue #4156 was: bad behavior that could be prevented by catching and handling malformed server data in a certain way. If I squint at #4156, blurring over the specifics of how it was about |
Yeah, exactly. It's a regression test for a somewhat generalized version of #4156, though not for the exact circumstances that bit us then (because with this PR's changes, we happen to no longer be vulnerable to those exact circumstances in the first place). |
From looking just at that commit, my guess is that the reason we deleted it was mostly about the means the comment was proposing ("See if we can mock |
Our code at the edge for transforming the old format of reactions,
with
user
, into the new format, withuser_id
, was depending onuser
always being present.Since
user_id
is documented as being added in Zulip 3.0:https://zulip.com/api/get-messages#response
, and since #5747 we've been refusing to connect to servers pre-3.0,
we can assume
user_id
is always present and stop readinguser
.Fixes: #4072