Skip to content
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

Move search operations to UserSubsystem #4188

Merged
merged 53 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
aa014bf
UserStore.Cassandra: Dedup embed call
akshaymankar Aug 6, 2024
cac29c2
Move all indexing operations to UserSearchSubsystem
akshaymankar Aug 6, 2024
082fdcd
Add metrics to UserSearchSubsystem and IndexedUserStore
akshaymankar Aug 15, 2024
aac6ed0
Run user serach index data migrations using subsystems
akshaymankar Aug 19, 2024
c234972
Remove explicit creds from ESConn
akshaymankar Aug 19, 2024
63afe93
Delete leftover code from Brig.Index.Eval
akshaymankar Aug 19, 2024
d732248
Move UserDoc tests to subsystem
akshaymankar Aug 20, 2024
e46d019
indexUserRow -> indexUser
akshaymankar Aug 20, 2024
e0c68fa
brig: Delete bulk reindex operations from internal API
akshaymankar Aug 20, 2024
d0272df
brig: Use the UserSearchSubsystem for syncing with index
akshaymankar Aug 20, 2024
9fab12e
regen nix
akshaymankar Aug 20, 2024
f170493
Rename an effect action
mdimjasevic Aug 20, 2024
46e8629
WIP: Move search code to subsystem
akshaymankar Aug 21, 2024
1a0f892
Move FederationConfigStore.Cassandra out of Brig
mdimjasevic Aug 21, 2024
b0391e4
Remove dead code from Brig.User.API.Search
mdimjasevic Aug 21, 2024
78d8856
Move browseTeam to wire-subsystem
mdimjasevic Aug 22, 2024
4ad948f
wire-subsystems: Fix compile errors with MiniBackend
akshaymankar Aug 26, 2024
67a3617
Merge UserSearchSubsystem into UserSubsystem
akshaymankar Aug 26, 2024
459c6b8
Wire.BlockListStore.Cassandra: Take ClientState as an arg instead of …
akshaymankar Aug 26, 2024
2560436
brig: Untangle sending user notifs and maintaining internal state
akshaymankar Aug 26, 2024
d77a0c1
Fix merge mistakes
akshaymankar Aug 27, 2024
39b4ea0
Delete leftover comment
akshaymankar Aug 27, 2024
01efe6a
UserStore.GetIndexUsersPaginated: Allow specifying page size
akshaymankar Aug 27, 2024
c664d34
wire-subsystems: Fix mock interpreters
akshaymankar Aug 27, 2024
4ec43a2
Dedup IndexError
akshaymankar Aug 27, 2024
f614653
Remove TODO
akshaymankar Aug 27, 2024
542f5ba
Reorganize Bulk operations
akshaymankar Aug 27, 2024
0ec354b
UserSearch.Types: Remove comment
akshaymankar Aug 27, 2024
0369090
Move expectedMigrationVersion to IndexedUserStore.Bulk
akshaymankar Aug 27, 2024
ec8a5d5
regen nix
akshaymankar Aug 27, 2024
3e25c07
Merge remote-tracking branch 'origin/develop' into wpb-8888/search-su…
elland Sep 16, 2024
559e154
Removed duplicated function.
elland Sep 16, 2024
63e3c4b
Brig.API.User.onActivated: update the index when email changes
akshaymankar Sep 16, 2024
b6f08d2
Promote suspected bug to confirmed bug, to be solved in a separate ti…
akshaymankar Sep 16, 2024
66a97d7
Another bug reported for another ticket
akshaymankar Sep 16, 2024
ede2aa1
hi ci
elland Sep 17, 2024
15a2c44
Merge branch 'develop' into wpb-8888/search-subsystem
elland Sep 17, 2024
1806e82
Resolve todo:
elland Sep 17, 2024
7d0d0c7
Update user search index on team changes.
elland Sep 17, 2024
99ab272
Pass casClient instead of embed.
elland Sep 17, 2024
8c73d31
Error for searcher doesn't exist.
elland Sep 17, 2024
1446cdb
Removed TODO, out of scope.
elland Sep 17, 2024
d8bac42
Upgraded TODO to FUTUREWORK.
elland Sep 17, 2024
1861511
Added changelogs.
elland Sep 17, 2024
69ce200
UserSubsystem: simplify folding over a domain
mdimjasevic Sep 18, 2024
6fc44e9
Merge remote-tracking branch 'origin/develop' into wpb-8888/search-su…
mdimjasevic Sep 18, 2024
7404df1
Bubble up liftSem'ing
mdimjasevic Sep 18, 2024
55cc084
Merge remote-tracking branch 'origin/develop' into wpb-8888/search-su…
mdimjasevic Sep 18, 2024
4b39d28
Remove commented out code
mdimjasevic Sep 18, 2024
d479ed6
Error messages for mocks/uninterpreted actions
mdimjasevic Sep 18, 2024
567a8d3
Remove a UserSubsystemError
mdimjasevic Sep 18, 2024
e7017f5
Merge remote-tracking branch 'origin/develop' into wpb-8888/search-su…
mdimjasevic Sep 18, 2024
01fa1bd
Remove unusued MapError instance
mdimjasevic Sep 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5-internal/WPB-888-2
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Removed `indexReindex` and `indexReindexIfSameOrNewer` from internal Brig/SearchIndex.
1 change: 1 addition & 0 deletions changelog.d/5-internal/WPB-8888
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Introduced ElasticSearch effects related to user search.
8 changes: 8 additions & 0 deletions integration/test/Test/Teams.hs
Original file line number Diff line number Diff line change
Expand Up @@ -49,31 +49,37 @@ testInvitePersonalUserToTeam = do
bindResponse (listInvitations owner tid) $ \resp -> do
resp.status `shouldMatchInt` 200
resp.json %. "invitations" `shouldMatch` ([] :: [()])

ownerId <- owner %. "id" & asString
setTeamFeatureStatus domain tid "exposeInvitationURLsToTeamAdmin" "enabled" >>= assertSuccess
user <- createUser domain def >>= getJSON 201
uid <- user %. "id" >>= asString
email <- user %. "email" >>= asString

inv <- postInvitation owner (PostInvitation (Just email) Nothing) >>= getJSON 201
checkListInvitations owner tid email
code <- getInvitationCode owner inv >>= getJSON 200 >>= (%. "code") & asString
inv %. "url" & asString >>= assertUrlContainsCode code
acceptTeamInvitation user code Nothing >>= assertStatus 400
acceptTeamInvitation user code (Just "wrong-password") >>= assertStatus 403

void $ withWebSockets [user] $ \wss -> do
acceptTeamInvitation user code (Just defPassword) >>= assertSuccess
for wss $ \ws -> do
n <- awaitMatch isUserUpdatedNotif ws
n %. "payload.0.user.team" `shouldMatch` tid

bindResponse (getSelf user) $ \resp -> do
resp.status `shouldMatchInt` 200
resp.json %. "team" `shouldMatch` tid

-- a team member can now find the former personal user in the team
bindResponse (getTeamMembers tm tid) $ \resp -> do
resp.status `shouldMatchInt` 200
members <- resp.json %. "members" >>= asList
ids <- for members ((%. "user") >=> asString)
ids `shouldContain` [uid]

-- the former personal user can now see other team members
bindResponse (getTeamMembers user tid) $ \resp -> do
resp.status `shouldMatchInt` 200
Expand All @@ -82,12 +88,14 @@ testInvitePersonalUserToTeam = do
tmId <- tm %. "id" & asString
ids `shouldContain` [ownerId]
ids `shouldContain` [tmId]

-- the former personal user can now search for the owner
bindResponse (searchContacts user (owner %. "name") domain) $ \resp -> do
resp.status `shouldMatchInt` 200
documents <- resp.json %. "documents" >>= asList
ids <- for documents ((%. "id") >=> asString)
ids `shouldContain` [ownerId]

refreshIndex domain
-- a team member can now search for the former personal user
bindResponse (searchContacts tm (user %. "name") domain) $ \resp -> do
Expand Down
7 changes: 1 addition & 6 deletions libs/brig-types/brig-types.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ library
Brig.Types.Instances
Brig.Types.Intra
Brig.Types.Provider.Tag
Brig.Types.Search
Brig.Types.Team
Brig.Types.Team.LegalHold
Brig.Types.Test.Arbitrary
Expand Down Expand Up @@ -73,16 +72,12 @@ library
-funbox-strict-fields -Wredundant-constraints -Wunused-packages

build-depends:
aeson >=2.0.1.0
, attoparsec >=0.10
, base >=4 && <5
, bytestring
base >=4 && <5
, bytestring-conversion >=0.2
, cassandra-util
, containers >=0.5
, imports
, QuickCheck >=2.9
, text >=0.11
, types-common >=0.16
, wire-api

Expand Down
7 changes: 0 additions & 7 deletions libs/brig-types/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@
# dependencies are added or removed.
{ mkDerivation
, aeson
, attoparsec
, base
, bytestring
, bytestring-conversion
, cassandra-util
, containers
Expand All @@ -18,7 +16,6 @@
, tasty
, tasty-hunit
, tasty-quickcheck
, text
, types-common
, wire-api
}:
Expand All @@ -27,16 +24,12 @@ mkDerivation {
version = "1.35.0";
src = gitignoreSource ./.;
libraryHaskellDepends = [
aeson
attoparsec
base
bytestring
bytestring-conversion
cassandra-util
containers
imports
QuickCheck
text
types-common
wire-api
];
Expand Down
107 changes: 0 additions & 107 deletions libs/brig-types/src/Brig/Types/Search.hs

This file was deleted.

2 changes: 0 additions & 2 deletions libs/brig-types/test/unit/Test/Brig/Types/User.hs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ module Test.Brig.Types.User where

import Brig.Types.Connection (UpdateConnectionsInternal (..))
import Brig.Types.Intra (NewUserScimInvitation (..), UserAccount (..))
import Brig.Types.Search (SearchVisibilityInbound (..))
import Brig.Types.User (ManagedByUpdate (..), RichInfoUpdate (..))
import Data.Aeson
import Imports
Expand All @@ -50,7 +49,6 @@ roundtripTests =
testRoundTripWithSwagger @EJPDRequestBody,
testRoundTripWithSwagger @EJPDResponseBody,
testRoundTrip @UpdateConnectionsInternal,
testRoundTrip @SearchVisibilityInbound,
testRoundTripWithSwagger @UserAccount,
testGroup "golden tests" $
[testCaseUserAccount]
Expand Down
24 changes: 24 additions & 0 deletions libs/cassandra-util/src/Cassandra/Exec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ module Cassandra.Exec
paginateC,
PageWithState (..),
paginateWithState,
paginateWithStateC,
paramsPagingState,
pwsHasMore,
module C,
Expand Down Expand Up @@ -115,6 +116,29 @@ paginateWithState q p = do
pure $ PageWithState b (pagingState m)
_ -> throwM $ UnexpectedResponse (hrHost r) (hrResponse r)

-- | Like 'paginateWithState' but returns a conduit instead of one page.
--
-- This can be used with 'paginateWithState' like this:
-- @
-- main :: IO ()
-- main = do
-- runConduit $
-- paginateWithStateC getUsers
-- .| mapC doSomethingWithAPageOfUsers
-- where
-- getUsers state = paginateWithState getUsersQuery (paramsPagingState Quorum () 10000 state)
-- @
paginateWithStateC :: forall m a. (Monad m) => (Maybe Protocol.PagingState -> m (PageWithState a)) -> ConduitT () [a] m ()
mdimjasevic marked this conversation as resolved.
Show resolved Hide resolved
paginateWithStateC getPage = do
go =<< lift (getPage Nothing)
where
go :: PageWithState a -> ConduitT () [a] m ()
go page = do
unless (null page.pwsResults) $
yield (page.pwsResults)
when (pwsHasMore page) $
go =<< lift (getPage page.pwsState)

paramsPagingState :: Consistency -> a -> Int32 -> Maybe Protocol.PagingState -> QueryParams a
paramsPagingState c p n state = QueryParams c False p (Just n) state Nothing Nothing
{-# INLINE paramsPagingState #-}
Expand Down
1 change: 1 addition & 0 deletions libs/cassandra-util/src/Cassandra/Util.hs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ initCassandra settings Nothing logger = do
-- | Read cassandra's writetimes https://docs.datastax.com/en/dse/5.1/cql/cql/cql_using/useWritetime.html
-- as UTCTime values without any loss of precision
newtype Writetime a = Writetime {writetimeToUTC :: UTCTime}
deriving (Functor)

instance Cql (Writetime a) where
ctype = Tagged BigIntColumn
Expand Down
2 changes: 2 additions & 0 deletions libs/polysemy-wire-zoo/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
, polysemy
, polysemy-check
, polysemy-plugin
, prometheus-client
, QuickCheck
, saml2-web-sso
, time
Expand All @@ -45,6 +46,7 @@ mkDerivation {
polysemy
polysemy-check
polysemy-plugin
prometheus-client
QuickCheck
saml2-web-sso
time
Expand Down
6 changes: 5 additions & 1 deletion libs/polysemy-wire-zoo/polysemy-wire-zoo.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ license: AGPL-3
build-type: Simple

library
-- cabal-fmt: expand src
exposed-modules:
Polysemy.Testing
Polysemy.TinyLog
Expand All @@ -23,6 +24,8 @@ library
Wire.Sem.Logger
Wire.Sem.Logger.Level
Wire.Sem.Logger.TinyLog
Wire.Sem.Metrics
Wire.Sem.Metrics.IO
Wire.Sem.Now
Wire.Sem.Now.Input
Wire.Sem.Now.IO
Expand Down Expand Up @@ -83,7 +86,7 @@ library

build-depends:
aeson
, base >=4.6 && <5.0
, base >=4.6 && <5.0
, bytestring
, cassandra-util
, crypton
Expand All @@ -94,6 +97,7 @@ library
, polysemy
, polysemy-check
, polysemy-plugin
, prometheus-client
, QuickCheck
, saml2-web-sso
, time
Expand Down
21 changes: 21 additions & 0 deletions libs/polysemy-wire-zoo/src/Wire/Sem/Metrics.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{-# LANGUAGE TemplateHaskell #-}

module Wire.Sem.Metrics where

import Imports
import Polysemy
import Prometheus (Counter, Gauge)

-- | NOTE: Vectors would require non trival changes because
-- 'Prometheus.withLabel' take a paramter of type 'metric -> IO ()'.
data Metrics m a where
AddCounter :: Counter -> Double -> Metrics m ()
AddGauge :: Gauge -> Double -> Metrics m ()

makeSem ''Metrics

incCounter :: (Member Metrics r) => Counter -> Sem r ()
incCounter c = addCounter c 1

incGauge :: (Member Metrics r) => Gauge -> Sem r ()
incGauge c = addGauge c 1
16 changes: 16 additions & 0 deletions libs/polysemy-wire-zoo/src/Wire/Sem/Metrics/IO.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
module Wire.Sem.Metrics.IO where

import Imports
import Polysemy
import qualified Prometheus as Prom
import Wire.Sem.Metrics

runMetricsToIO :: (Member (Embed IO) r) => InterpreterFor Metrics r
runMetricsToIO = interpret $ \case
AddCounter c n -> embed . void $ Prom.addCounter @IO c n
AddGauge g n -> embed $ Prom.addGauge @IO g n

ignoreMetrics :: InterpreterFor Metrics r
ignoreMetrics = interpret $ \case
AddCounter _ _ -> pure ()
AddGauge _ _ -> pure ()
3 changes: 3 additions & 0 deletions libs/wire-api/src/Wire/API/Error/Brig.hs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ data BrigError
| NotConnected
| InvalidTransition
| NoIdentity
| NoUser
| HandleExists
| InvalidHandle
| HandleNotFound
Expand Down Expand Up @@ -170,6 +171,8 @@ type instance MapError 'InvalidTransition = 'StaticError 403 "bad-conn-update" "

type instance MapError 'NoIdentity = 'StaticError 403 "no-identity" "The user has no verified email"

type instance MapError 'NoUser = 'StaticError 403 "no-user" "The user does not exist"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the error before the refactoring?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"TODO: searcher doesn't exist" :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was asking about before this PR started. This PR is supposed to be a refactoring, so ideally we wouldn't be inventing new user visible errors. So it'd be great to know what was the previous behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we've introduced this error, but for no good reason. I'm refactoring the code so we don't need to throw the error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type instance is gone now and we don't throw a new error anymore compared to the old code.

type instance MapError 'HandleExists = 'StaticError 409 "handle-exists" "The given handle is already taken"

type instance MapError 'InvalidHandle = 'StaticError 400 "invalid-handle" "The given handle is invalid (less than 2 or more than 256 characters; chars not in \"a-z0-9_.-\"; or on the blocklist)"
Expand Down
Loading