Add *AndRole indexes to usersChannels #4340

mxstbr · 2018-11-26T10:48:20Z

This adds two new indexes to usersChannels and rewrites all queries to use them:

userIdAndRole
channelIdAndRole

"Role" is one of the roles a user can have in a channel, and is based on the is{Role} fields, i.e. "Role" can be one of "blocked", "pending", "owner", "moderator", "owner".

Note that the index prefers roles in that order, so even if a write query is inconsistent and a member has isMember: true but also isBlocked: true the user will be counted as blocked.

This is modeled after the same thing we did for usersCommunities, but I realized that adding 5 indexes to a table as big as usersCommunities and usersChannels is pretty expensive storage-wise. (I could've also implemented the same indexes as usersCommunities, i.e. userIdAndIsMember, userIdAndIsBlocked, etc.)

This achieves the same thing and makes all queries be indexed, but requires a lot less storage. We should probably also move the usersCommunities indexes to this *AndRole system...

Status

WIP
Ready for review
Needs testing

Deploy after merge (delete what needn't be deployed)

api
analytics
athena
vulcan
mercury
hermes
chronos

Run database migrations (delete if no migration was added)
YES

Related issues
Based on #4339

This was leftover behavior from the move to create-query, but is actually unnecessary.

This adds two new indexes to `usersChannels` and rewrites all queries to use them: - `userIdAndRole` - `channelIdAndRole` "Role" is one of the roles a user can have in a channel, and is based on the `is{Role}` fields, i.e. "Role" can be one of "blocked", "pending", "owner", "moderator", "owner". Note that the index prefers roles in that order, so even if a write query is inconsistent and a member has `isMember: true` but also `isBlocked: true` the user will be counted as blocked. ---- This is modeled after the same thing we did for `usersCommunities`, but I realized that adding 5 indexes to a table as big as `usersCommunities` and `usersChannels` is pretty expensive storage-wise. (I could've also implemented the same indexes as `usersCommunities`, i.e. `userIdAndIsMember`, `userIdAndIsBlocked`, etc.) This achieves the same thing and makes all queries be indexed, but requires a lot less storage. We should probably also move the usersCommunities indexes to this `*AndRole` system...

Still the same content, just ordered differently due to new queries.

brianlovin · 2018-11-26T18:09:28Z

api/models/thread.js

-    .getAll(currentUser, { index: 'userId' })
-    .filter({ isBlocked: false, isMember: true })
+    .getAll(
+      [evalUser, 'member'],


Shouldn't this be currentUser not evalUser?

brianlovin · 2018-11-26T18:09:42Z

api/models/thread.js

-    .getAll(currentUser, { index: 'userId' })
-    .filter({ isBlocked: false, isMember: true })
+    .getAll(
+      [evalUser, 'member'],


currentUser not evalUser right?

brianlovin · 2018-11-26T18:10:52Z

api/models/usersChannels.js

          .table('usersChannels')
-          .getAll(userId, { index: 'userId' })
-          .filter({ channelId, isBlocked: false })
+          .getAll([userId, channelId], { index: 'userIdAndChannelId' })


I think we still want the isBlocked filter here right? Otherwise someone could spoof joining a channel via a direct POST request

brianlovin · 2018-11-26T18:12:29Z

api/models/usersChannels.js

-    .getAll(channelId, { index: 'channelId' })
-    .filter({ isMember: true })
+    .getAll(
+      [channelId, 'member'],


To clarify: this automatically handles duplication? e.g. someone is a moderator and a member, it should only count as one membership

Yes, since the role index can only have one value the users that have a role of "member" are exclusive of other users who have the roles "moderator" or "owner". See my comment below: #4340 (comment)

brianlovin · 2018-11-26T18:14:51Z

chronos/models/usersChannels.js

    .table('usersChannels')
-    .getAll(userId, { index: 'userId' })
-    .filter({ isMember: true })
+    .getAll([userId, 'member'], [userId, 'moderator'], [userId, 'owner'], {


I've noticed you've done this check in a few places. If we're just trying to get members, why are you using all 3 indexes?

Because owners and moderators are also members, but their roles are "owner" and "moderator"!

This isn't true, so we can simplify this! If you are a moderator or owner you also are isMember: true

We can't, unfortunately!

Indexes can only have a single value, not multiple values. The "role" index is set to the highest one based on this precedence:

Blocked

Pending

Owner

Moderator

Member

This means:

If both isMember and isOwner are true, the users role will be "owner".

If both isMember and isModerator are true, the users role will be "moderator".

If both isMember and isBlocked are true (for whatever reason, bad db data most likely) the the role will be "blocked".

That means if we want to get all members we also have to fetch the users who have moderator and owner roles, on top of the ones that have member roles.

Does that make sense?

mxstbr · 2018-11-26T20:21:03Z

Great reviews, will dig further into this tomorrow unless you want to dig into this and finish it off today!

brianlovin · 2018-11-26T20:22:37Z

Great reviews, will dig further into this tomorrow unless you want to dig into this and finish it off today!

Will be testing some other work today and trying to fix some other issues with Chronos, so feel free to finish up as you have time this week ;)

mxstbr · 2018-11-27T09:19:45Z

Cleaned up those copy-and-paste goofs in the queries with evalUser and currentUser, want to give this another good review and ship if you're happy with it?

brianlovin · 2018-11-27T16:40:35Z

Sure thing @mxstbr. One question about this new index: how is the performance? Do all the branches end up taking longer than a normal query without an index?

mxstbr · 2018-11-27T16:42:48Z

Indexes are computed at write-time, rather than read-time, so those r.branches won't have any effect on the read-time of queries and a very neglible impact on write-time, although I admittedly haven't tested that (and don't even know how I would...).

brianlovin · 2018-11-27T17:49:18Z

api/models/usersChannels.js

    .table('usersChannels')
-    .getAll(channelId, { index: 'channelId' })
-    .filter({ userId, isBlocked: true })
+    .getAll([userId, channelId], { index: 'userIdAndChannelId' })


Sorry, one more spot: I think we want to filter isBlocked: true to ensure that it's only approving a blocked user

brianlovin · 2018-11-27T17:51:34Z

Afaict in my local testing everything is working well! I double checked search filtering and profile filtering based on private communities and channels, and everything is holding up.

One thing I noticed: at some point along the way we made it impossible to fetch a private channel where you're not a member. This means we never render the 'request to join private channel' view :P Not high pri, but we probably want that request-to-join screen back at some point.

brianlovin · 2018-11-27T18:14:25Z

Gonna merge and add this to 2.7.8

mxstbr added 3 commits November 26, 2018 10:15

Don't double-connect to db from workers

c3db8b8

This was leftover behavior from the move to create-query, but is actually unnecessary.

Improve some old queries

8ebdc9e

spectrum-bot bot added the Ready for Review label Nov 26, 2018

mxstbr added 3 commits November 26, 2018 12:00

Fix a goof

a7094c6

Fix all member queries to also include mods and owners, like before

b55d39c

Switch to new ordering in snapshot tests

24abff0

Still the same content, just ordered differently due to new queries.

brianlovin reviewed Nov 26, 2018

View reviewed changes

mxstbr mentioned this pull request Nov 27, 2018

Use message threadIdAndTimestamp index for getNewMessageCount query #4350

Merged

3 tasks

Clean up bad copy-and-paste queries

66f63c2

brianlovin reviewed Nov 27, 2018

View reviewed changes

Fix nit

d523ff7

brianlovin merged commit d832592 into alpha Nov 27, 2018

brianlovin deleted the users-channels-role-index branch November 27, 2018 18:14

Add *AndRole indexes to usersChannels #4340

Add *AndRole indexes to usersChannels #4340

Uh oh!

Conversation

mxstbr commented Nov 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxstbr commented Nov 26, 2018

Uh oh!

brianlovin commented Nov 26, 2018

Uh oh!

mxstbr commented Nov 27, 2018

Uh oh!

brianlovin commented Nov 27, 2018

Uh oh!

mxstbr commented Nov 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianlovin commented Nov 27, 2018

Uh oh!

brianlovin commented Nov 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants