narrow: Stop storing emails!#4382
Conversation
Indeed, thanks 🙂 |
gnprice
force-pushed
the
pr-narrow-userids
branch
from
908c6af to
c693640
Compare
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Great, thanks! This LGTM; just a small query and a comment below.
| } | ||
| emails.push(email); | ||
| } | ||
| // TODO(server-2.1): just send IDs instead |
There was a problem hiding this comment.
If it makes a big difference to the amount of work the server has to do (and if that's important to minimize), we could check the stored server version in Redux and courteously send IDs to the newer servers, while still sending emails to older ones. But it might not make much of a difference, and we may be dropping support for those older server versions soon-ish anyway. 🙂
There was a problem hiding this comment.
Yeah, I don't think it materially affects the work the server has to do. It'll want to look up the users in the database in any event (for one thing, to make sure they're valid users that are in your realm); and the database will have indexes so that looking up by ID or by email are both fast, because those are both common operations.
| ...dropCache(state), | ||
| drafts: objectFromEntries( | ||
| Object.keys(state.drafts) | ||
| .map(key => key.replace(/^pm:d:(.*?):.*/s, 'pm:$1')) |
There was a problem hiding this comment.
Good, I think '21' is safe from the kinds of potential issues mentioned in '19':
// Note this will migrate straight to our current format, even after
// that changes from when this migration was written! That saves us
// from duplicating `keyFromNarrow` here... but calls for care in
// migrations for future changes to `keyFromNarrow`.
In particular:
- If [
'21'] is the sequence of migrations that run at startup, drafts in'21''s input will be in thepm:d:format, which this string replacement handles correctly. - If [
'20','21'] is the sequence of migrations that run at startup, drafts in'21''s input will be...empty. (Drafts in'20''s input would have all hadpm:s:keys, and'20'would have removed them all.) Drafts being empty in'21''s input is fine; it just gives an empty output. - If [(...,)
'19','20','21'] is the sequence of migrations that run at startup, drafts in'21''s input will have keys that are already in the up-to-datepm:format (because of the quirk with'19'; then, following that, because'20'didn't filter out thepm:format). This is fine; the.replacein'21'neither throws an error with that input nor changes it at all.
There was a problem hiding this comment.
Thanks for checking this!
- If [
'20','21'] is the sequence of migrations that run at startup, drafts in'21''s input will be...empty.
(It'll have no PMs, that is -- it'll still have drafts for stream messages.)
- If [(...,)
'19','20','21'] is the sequence of migrations that run at startup, drafts in'21''s input will have keys that are already in the up-to-datepm:format
Hmm. I think there's actually a bug here, though! In migration 19:
Object.keys(state.drafts).map(key => [keyFromNarrow(JSON.parse(key)), state.drafts[key]]),the JSON.parse for a PM narrow will return something like { operator: 'pm-with', operand: 'foo@example.com,bar@example.org' }. In fact this is really two bugs:
- That's the old shape of
Narrow, whichkeyFromNarrowno longer accepts. - That has emails and no user IDs. So there's nothing
keyFromNarrowcould possibly do to convert it to the new format.
The bug was introduced with #4346, where Narrow changed and so the type expected by keyFromNarrow changed. The reason Flow didn't catch that there was this hole in the encapsulation is that JSON.parse there.
This probably means that my plan in #4339 (at #4339 (comment) ) wasn't the right one in retrospect. It would have been better to simply drop the drafts, or else bite the bullet and duplicate keyFromNarrow.
Happily no general users can have been affected at all, because we haven't made a release since this was introduced.
I think I'll fix this by rolling up migrations 19 and 20 into a new migration 21, which just drops drafts entirely (plus dropCache). That'll have the same effect in practice as if I'd gone with a "drop drafts" plan in #4339 from the beginning -- each user will have drafts dropped once. And the current migration 20 is already meant to drop all PM drafts, so only stream drafts are newly affected.
There was a problem hiding this comment.
Oh OK, great—I'm glad you found this! 🙂
I think I'll fix this by rolling up migrations 19 and 20 into a new migration 21
SGTM.
gnprice
force-pushed
the
pr-narrow-userids
branch
from
c693640 to
fbe04b8
Compare
|
New revision pushed! |
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks, @gnprice! LGTM except for one comment below.
| // that changes from when this migration was written! That saves us | ||
| // from duplicating `keyFromNarrow` here... but calls for care in | ||
| // migrations for future changes to `keyFromNarrow`. | ||
| Object.keys(state.drafts).map(key => [keyFromNarrow(JSON.parse(key)), state.drafts[key]]), |
There was a problem hiding this comment.
Looks like keyFromNarrow and objectFromEntries need their imports removed (but then don't forget to bring back objectFromEntries's for migration 22 🙂).
As the comment in migration 19 said, we were doing something a bit risky there in order to avoid duplicating the then-current version of `keyFromNarrow`. We simply invoked `keyFromNarrow` directly -- which called for being careful about migrations for any future change we made to `keyFromNarrow`. Then a bit later we changed `keyFromNarrow` quite dramatically, and didn't pay attention to migrations at all. Oops. Specifically, in cf4207f we changed the `Narrow` type to have quite a different structure. That's the type that `keyFromNarrow` accepts -- so the result is that `keyFromNarrow` stopped working on the old values. Which are exactly what `JSON.parse` is going to supply us when this migration is run. The result is probably that if the user has any drafts when they take an upgrade that spans migration 19 and commit cf4207f, the app will crash on startup. We'd probably have found this bug at the alpha stage when next making a release, so it wouldn't have made it to users other than ourselves. Better to fix ASAP, though. The plan for migration 19, as described here: zulip#4339 (comment) knowingly involved breaking the basic rule of migrations: don't invoke any of the app's model code, because it will change. I figured it was OK because there were changes coming up soon which I'd planned out how I'd add migrations for. Then later the same week I sent zulip#4346, which followed that plan... and I hadn't thought through this particular interaction, and it introduced this bug. So this is a fresh lesson in the basic rule of migrations: don't invoke any of the app's model code, because it will change. See discussion where we found this bug: zulip#4382 (comment)
Well, use one to get the other: we still need emails to send to the server, until we drop support for servers older than 2.1. Also add a TODO(server-2.1) comment to help us find that when we do. But with this change, we don't use the emails *from the Narrow value* -- which means we're now all clear to remove the emails from Narrow values entirely.
This check is itself the last remaining place where we consume the emails from a PM narrow and then try to find the users in our data structures; everything else relies on the user IDs. So there are no longer any potential crashes that this check could prevent. We're about to remove those emails entirely. As part of that, cut this check.
This completes a major objective of the long string of refactoring that appeared in the series of PRs zulip#4330, zulip#4332, zulip#4335, zulip#4339, zulip#4342, then zulip#4346, zulip#4356, zulip#4361, zulip#4364, and most recently zulip#4368. After this change, the portion of zulip#4333 that's about PMs, emails, and user IDs -- aka the portion of zulip#3764 that's about narrows -- is complete. Still open from zulip#4333 is to convert stream and topic narrows from stream names to stream IDs. I'm hopeful that that will be simpler: (a) unlike the situation for PMs, there's just one stream mentioned at a time, so there's no question of sorting, and (b) there isn't the "include self or not" complication that's bedeviled much of our code related to PMs. In other words, stream and topic narrows don't suffer from zulip#4035.
Since the previous commit this already doesn't contain anything meaningful -- and its type was `mixed`, to help confirm that no consumers were actually trying to use it. Now delete it.
This type exists only to be passed to Narrow constructors; and those no longer need emails, only user IDs. So we can simplify it to contain just the user IDs.
gnprice
force-pushed
the
pr-narrow-userids
branch
from
fbe04b8 to
a6798aa
Compare
|
Thanks for the review! Merged, with that fix. |
2 participants
This is the next in the long chain of PRs #4330, #4332, #4335, #4339, #4342, #4346, #4356, #4361, #4364, and most recently #4368. This one completes a major objective of all that refactoring: at the end of it, we no longer store emails in Narrow values.
After this change, the portion of #4333 that's about PMs, emails, and user IDs -- aka the portion of #3764 that's about narrows -- is complete.
Still open from #4333 is to convert stream and topic narrows from stream names to stream IDs. I'm hopeful that that will be simpler: (a) unlike the situation for PMs, there's just one stream mentioned at a time, so there's no question of sorting, and (b) there isn't the "include self or not" complication that's bedeviled much of our code related to PMs. In other words, stream and topic narrows don't suffer from #4035.
As discussed at #4368, in one of the spots this touches we still need emails, namely to make get-messages requests to the server (so long as we support servers older than 2.1). But we can handle that the same way we've handled such mismatches internally to the app during this refactoring, and the same way we've handled the reverse of them where we receive emails and now want to store user IDs: use the
allUsersByIdmap (or its inverse) to convert one form to the other. That means we don't need the emails from the Narrow value, and the rest of these changes can proceed.There's still plenty more places around the app where we can replace emails with user IDs, i.e. #3764. I still have a draft branch on top of this one with some of them. I'm hopeful that much of what remains will be easier now that emails are gone from narrows, because that's such a central data structure that a lot of code ultimately has to feed or is fed by.