From 797b5a114142d052a679a034b75eba896351ad45 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Tue, 10 Aug 2021 18:05:52 +0200 Subject: [PATCH 01/29] Break POST /list-conversations API and adapt code The API will only support getting metadata of a given list of ids --- libs/wire-api/src/Wire/API/Conversation.hs | 34 ++++-- .../src/Wire/API/Routes/Public/Galley.hs | 5 +- .../testObject_ListConversations_1.json | 1 - .../testObject_ConversationsResponse_1.json | 107 ++++++++++++++++++ .../testObject_ListConversations_1.json | 12 ++ .../unit/Test/Wire/API/Golden/FromJSON.hs | 4 - .../test/unit/Test/Wire/API/Golden/Manual.hs | 9 +- .../Golden/Manual/ConversationsResponse.hs | 103 +++++++++++++++++ .../API/Golden/Manual/ListConversations.hs | 12 +- libs/wire-api/wire-api.cabal | 3 +- services/galley/src/Galley/API/Query.hs | 36 +----- services/galley/test/integration/API.hs | 84 +++++++------- services/galley/test/integration/API/Util.hs | 15 --- 13 files changed, 315 insertions(+), 110 deletions(-) delete mode 100644 libs/wire-api/test/golden/fromJSON/testObject_ListConversations_1.json create mode 100644 libs/wire-api/test/golden/testObject_ConversationsResponse_1.json create mode 100644 libs/wire-api/test/golden/testObject_ListConversations_1.json create mode 100644 libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ConversationsResponse.hs diff --git a/libs/wire-api/src/Wire/API/Conversation.hs b/libs/wire-api/src/Wire/API/Conversation.hs index 55e9c50fa7..29bb21f4a7 100644 --- a/libs/wire-api/src/Wire/API/Conversation.hs +++ b/libs/wire-api/src/Wire/API/Conversation.hs @@ -31,6 +31,7 @@ module Wire.API.Conversation ConversationPagingState (..), ConversationPagingTable (..), ConvIdsPage (..), + ConversationsResponse (..), -- * Conversation properties Access (..), @@ -343,11 +344,8 @@ instance ToSchema GetPaginatedConversationIds where <*> gpciSize .= (fieldWithDocModifier "size" addSizeDoc schema <|> pure (toRange (Proxy @1000))) -- | Used on the POST /list-conversations endpoint --- FUTUREWORK: add to golden tests (how to generate them?) -data ListConversations = ListConversations - { lQualifiedIds :: Maybe (NonEmpty (Qualified ConvId)), - lStartId :: Maybe (Qualified ConvId), - lSize :: Maybe (Range 1 500 Int32) +newtype ListConversations = ListConversations + { lQualifiedIds :: NonEmpty (Qualified ConvId) } deriving stock (Eq, Show, Generic) deriving (FromJSON, ToJSON, S.ToSchema) via Schema ListConversations @@ -356,11 +354,29 @@ instance ToSchema ListConversations where schema = objectWithDocModifier "ListConversations" - (description ?~ "A request to list some or all of a user's conversations, including remote ones") + (description ?~ "A request to list some of a user's conversations, including remote ones") $ ListConversations - <$> lQualifiedIds .= optField "qualified_ids" Nothing (nonEmptyArray schema) - <*> lStartId .= optField "start_id" Nothing schema - <*> lSize .= optField "size" Nothing schema + <$> lQualifiedIds .= field "qualified_ids" (nonEmptyArray schema) + +data ConversationsResponse = ConversationsResponse + { crFound :: [Conversation], + crNotFound :: [Qualified ConvId], + crFailed :: [Qualified ConvId] + } + deriving stock (Eq, Show) + deriving (FromJSON, ToJSON, S.ToSchema) via Schema ConversationsResponse + +instance ToSchema ConversationsResponse where + schema = + let notFoundDoc = description ?~ "These conversations either don't exist or are deleted." + failedDoc = description ?~ "The server failed to fetch these conversations, most likely due to network issues while contacting a remote server" + in objectWithDocModifier + "ConversationsResponse" + (description ?~ "Response object for getting metadata of a list of conversations") + $ ConversationsResponse + <$> crFound .= field "found" (array schema) + <*> crNotFound .= fieldWithDocModifier "not_found" notFoundDoc (array schema) + <*> crFailed .= fieldWithDocModifier "failed" failedDoc (array schema) -------------------------------------------------------------------------------- -- Conversation properties diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Galley.hs b/libs/wire-api/src/Wire/API/Routes/Public/Galley.hs index 53780ca0b8..70b0362904 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Galley.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Galley.hs @@ -188,12 +188,11 @@ data Api routes = Api :> Get '[Servant.JSON] (Public.ConversationList Public.Conversation), listConversations :: routes - :- Summary "Get all conversations (also returns remote conversations)" - :> Description "Like GET /conversations, but allows specifying a list of remote conversations in its request body. Will return all or the requested qualified conversations, including remote ones. WIP: Size parameter is not yet honoured for remote conversations." + :- Summary "Get conversation metadata for a list of conversation ids" :> ZUser :> "list-conversations" :> ReqBody '[Servant.JSON] Public.ListConversations - :> Post '[Servant.JSON] (Public.ConversationList Public.Conversation), + :> Post '[Servant.JSON] Public.ConversationsResponse, -- This endpoint can lead to the following events being sent: -- - ConvCreate event to members getConversationByReusableCode :: diff --git a/libs/wire-api/test/golden/fromJSON/testObject_ListConversations_1.json b/libs/wire-api/test/golden/fromJSON/testObject_ListConversations_1.json deleted file mode 100644 index 0967ef424b..0000000000 --- a/libs/wire-api/test/golden/fromJSON/testObject_ListConversations_1.json +++ /dev/null @@ -1 +0,0 @@ -{} diff --git a/libs/wire-api/test/golden/testObject_ConversationsResponse_1.json b/libs/wire-api/test/golden/testObject_ConversationsResponse_1.json new file mode 100644 index 0000000000..70bd669d4c --- /dev/null +++ b/libs/wire-api/test/golden/testObject_ConversationsResponse_1.json @@ -0,0 +1,107 @@ +{ + "found": [ + { + "access": [], + "creator": "00000001-0000-0001-0000-000200000001", + "access_role": "private", + "members": { + "self": { + "hidden_ref": "", + "status": 0, + "service": null, + "otr_muted_ref": null, + "conversation_role": "rhhdzf0j0njilixx0g0vzrp06b_5us", + "status_time": "1970-01-01T00:00:00.000Z", + "hidden": false, + "status_ref": "0.0", + "id": "00000001-0000-0001-0000-000100000000", + "otr_archived": false, + "otr_muted_status": null, + "otr_muted": true, + "otr_archived_ref": "" + }, + "others": [] + }, + "qualified_id": { + "domain": "golden.example.com", + "id": "00000001-0000-0000-0000-000000000000" + }, + "name": " 0", + "team": "00000001-0000-0001-0000-000100000002", + "id": "00000001-0000-0000-0000-000000000000", + "type": 2, + "receipt_mode": -2, + "last_event_time": "1970-01-01T00:00:00.000Z", + "message_timer": null, + "last_event": "0.0" + }, + { + "access": [ + "invite", + "invite", + "code", + "link", + "invite", + "private", + "link", + "code", + "code", + "link", + "private", + "invite" + ], + "creator": "00000000-0000-0000-0000-000200000001", + "access_role": "non_activated", + "members": { + "self": { + "hidden_ref": "", + "status": 0, + "service": null, + "otr_muted_ref": null, + "conversation_role": "9b2d3thyqh4ptkwtq2n2v9qsni_ln1ca66et_z8dlhfs9oamp328knl3rj9kcj", + "status_time": "1970-01-01T00:00:00.000Z", + "hidden": true, + "status_ref": "0.0", + "id": "00000000-0000-0001-0000-000100000001", + "otr_archived": false, + "otr_muted_status": -1, + "otr_muted": true, + "otr_archived_ref": null + }, + "others": [] + }, + "qualified_id": { + "domain": "golden.example.com", + "id": "00000000-0000-0000-0000-000000000002" + }, + "name": "", + "team": "00000000-0000-0001-0000-000200000000", + "id": "00000000-0000-0000-0000-000000000002", + "type": 1, + "receipt_mode": 2, + "last_event_time": "1970-01-01T00:00:00.000Z", + "message_timer": 1319272593797015, + "last_event": "0.0" + } + ], + "not_found": [ + { + "domain": "golden.example.com", + "id": "00000018-0000-0020-0000-000e00000002" + }, + { + "domain": "golden2.example.com", + "id": "00000018-0000-0020-0000-111111111112" + } + ], + "failed": [ + { + "domain": "golden.example.com", + "id": "00000018-4444-0020-0000-000e00000002" + }, + { + "domain": "golden3.example.com", + "id": "99999999-0000-0020-0000-111111111112" + } + ] +} \ No newline at end of file diff --git a/libs/wire-api/test/golden/testObject_ListConversations_1.json b/libs/wire-api/test/golden/testObject_ListConversations_1.json new file mode 100644 index 0000000000..e2b47c2fc1 --- /dev/null +++ b/libs/wire-api/test/golden/testObject_ListConversations_1.json @@ -0,0 +1,12 @@ +{ + "qualified_ids": [ + { + "domain": "domain.example.com", + "id": "00000018-0000-0020-0000-000e00000002" + }, + { + "domain": "domain2.example.com", + "id": "00000018-0000-0020-0000-111111111112" + } + ] +} \ No newline at end of file diff --git a/libs/wire-api/test/unit/Test/Wire/API/Golden/FromJSON.hs b/libs/wire-api/test/unit/Test/Wire/API/Golden/FromJSON.hs index c3ed87b42b..c13aa0a5e9 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Golden/FromJSON.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Golden/FromJSON.hs @@ -24,7 +24,6 @@ import Test.Wire.API.Golden.Generated.NewConvUnmanaged_user import Test.Wire.API.Golden.Generated.NewOtrMessage_user import Test.Wire.API.Golden.Generated.RmClient_user import Test.Wire.API.Golden.Generated.SimpleMember_user -import Test.Wire.API.Golden.Manual.ListConversations import Test.Wire.API.Golden.Runner import Wire.API.Conversation (Conversation) import Wire.API.User.Client (RmClient) @@ -49,9 +48,6 @@ tests = [(testObject_RmClient_user_4, "testObject_RmClient_user_4.json")], testCase "RmClient failure" $ testFromJSONFailure @RmClient "testObject_RmClient_failure.json", - testCase "ListConversations" $ - testFromJSONObjects - [(testObject_ListConversations_1, "testObject_ListConversations_1.json")], testCase "QualifiedConversationId" $ testFromJSONFailure @Conversation "testObject_Conversation_qualifiedId.json" ] diff --git a/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual.hs b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual.hs index e13083c4dc..d5b463b079 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual.hs @@ -25,8 +25,10 @@ import Test.Wire.API.Golden.Manual.ClientCapabilityList import Test.Wire.API.Golden.Manual.ConvIdsPage import Test.Wire.API.Golden.Manual.ConversationCoverView import Test.Wire.API.Golden.Manual.ConversationPagingState +import Test.Wire.API.Golden.Manual.ConversationsResponse import Test.Wire.API.Golden.Manual.FeatureConfigEvent import Test.Wire.API.Golden.Manual.GetPaginatedConversationIds +import Test.Wire.API.Golden.Manual.ListConversations import Test.Wire.API.Golden.Manual.QualifiedUserClientPrekeyMap import Test.Wire.API.Golden.Manual.UserClientPrekeyMap import Test.Wire.API.Golden.Manual.UserIdList @@ -94,5 +96,10 @@ tests = testObjects [ (testObject_UserIdList_1, "testObject_UserIdList_1.json"), (testObject_UserIdList_2, "testObject_UserIdList_2.json") - ] + ], + testCase "ListConversations" $ + testObjects + [(testObject_ListConversations_1, "testObject_ListConversations_1.json")], + testCase "ConversationsResponse" $ + testObjects [(testObject_ConversationsResponse_1, "testObject_ConversationsResponse_1.json")] ] diff --git a/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ConversationsResponse.hs b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ConversationsResponse.hs new file mode 100644 index 0000000000..d4733ff41a --- /dev/null +++ b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ConversationsResponse.hs @@ -0,0 +1,103 @@ +module Test.Wire.API.Golden.Manual.ConversationsResponse + ( testObject_ConversationsResponse_1, + ) +where + +import Data.Domain +import Data.Id (Id (Id)) +import Data.Misc +import Data.Qualified +import qualified Data.UUID as UUID +import Imports +import Wire.API.Conversation +import Wire.API.Conversation.Role + +testObject_ConversationsResponse_1 :: ConversationsResponse +testObject_ConversationsResponse_1 = + ConversationsResponse + { crFound = [conv1, conv2], + crNotFound = + [ Qualified (Id (fromJust (UUID.fromString "00000018-0000-0020-0000-000e00000002"))) (Domain "golden.example.com"), + Qualified (Id (fromJust (UUID.fromString "00000018-0000-0020-0000-111111111112"))) (Domain "golden2.example.com") + ], + crFailed = + [ Qualified (Id (fromJust (UUID.fromString "00000018-4444-0020-0000-000e00000002"))) (Domain "golden.example.com"), + Qualified (Id (fromJust (UUID.fromString "99999999-0000-0020-0000-111111111112"))) (Domain "golden3.example.com") + ] + } + +conv1 :: Conversation +conv1 = + Conversation + { cnvQualifiedId = Qualified (Id (fromJust (UUID.fromString "00000001-0000-0000-0000-000000000000"))) (Domain "golden.example.com"), + cnvType = One2OneConv, + cnvCreator = Id (fromJust (UUID.fromString "00000001-0000-0001-0000-000200000001")), + cnvAccess = [], + cnvAccessRole = PrivateAccessRole, + cnvName = Just " 0", + cnvMembers = + ConvMembers + { cmSelf = + Member + { memId = Id (fromJust (UUID.fromString "00000001-0000-0001-0000-000100000000")), + memService = Nothing, + memOtrMuted = True, + memOtrMutedStatus = Nothing, + memOtrMutedRef = Nothing, + memOtrArchived = False, + memOtrArchivedRef = Just "", + memHidden = False, + memHiddenRef = Just "", + memConvRoleName = fromJust (parseRoleName "rhhdzf0j0njilixx0g0vzrp06b_5us") + }, + cmOthers = [] + }, + cnvTeam = Just (Id (fromJust (UUID.fromString "00000001-0000-0001-0000-000100000002"))), + cnvMessageTimer = Nothing, + cnvReceiptMode = Just (ReceiptMode {unReceiptMode = -2}) + } + +conv2 :: Conversation +conv2 = + Conversation + { cnvQualifiedId = Qualified (Id (fromJust (UUID.fromString "00000000-0000-0000-0000-000000000002"))) (Domain "golden.example.com"), + cnvType = SelfConv, + cnvCreator = Id (fromJust (UUID.fromString "00000000-0000-0000-0000-000200000001")), + cnvAccess = + [ InviteAccess, + InviteAccess, + CodeAccess, + LinkAccess, + InviteAccess, + PrivateAccess, + LinkAccess, + CodeAccess, + CodeAccess, + LinkAccess, + PrivateAccess, + InviteAccess + ], + cnvAccessRole = NonActivatedAccessRole, + cnvName = Just "", + cnvMembers = + ConvMembers + { cmSelf = + Member + { memId = Id (fromJust (UUID.fromString "00000000-0000-0001-0000-000100000001")), + memService = Nothing, + memOtrMuted = True, + memOtrMutedStatus = Just (MutedStatus {fromMutedStatus = -1}), + memOtrMutedRef = Nothing, + memOtrArchived = False, + memOtrArchivedRef = Nothing, + memHidden = True, + memHiddenRef = Just "", + memConvRoleName = + fromJust (parseRoleName "9b2d3thyqh4ptkwtq2n2v9qsni_ln1ca66et_z8dlhfs9oamp328knl3rj9kcj") + }, + cmOthers = [] + }, + cnvTeam = Just (Id (fromJust (UUID.fromString "00000000-0000-0001-0000-000200000000"))), + cnvMessageTimer = Just (Ms {ms = 1319272593797015}), + cnvReceiptMode = Just (ReceiptMode {unReceiptMode = 2}) + } diff --git a/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversations.hs b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversations.hs index cb4580d6c2..31f116b999 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversations.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversations.hs @@ -17,8 +17,18 @@ module Test.Wire.API.Golden.Manual.ListConversations where +import Data.Domain (Domain (Domain)) +import Data.Id (Id (Id)) +import Data.List.NonEmpty (NonEmpty (..)) +import Data.Qualified (Qualified (Qualified)) +import qualified Data.UUID as UUID import Imports import Wire.API.Conversation (ListConversations (..)) testObject_ListConversations_1 :: ListConversations -testObject_ListConversations_1 = ListConversations Nothing Nothing Nothing +testObject_ListConversations_1 = + ListConversations + ( Qualified (Id (fromJust (UUID.fromString "00000018-0000-0020-0000-000e00000002"))) (Domain "domain.example.com") + :| [ Qualified (Id (fromJust (UUID.fromString "00000018-0000-0020-0000-111111111112"))) (Domain "domain2.example.com") + ] + ) diff --git a/libs/wire-api/wire-api.cabal b/libs/wire-api/wire-api.cabal index a70062305e..0fd5601f8c 100644 --- a/libs/wire-api/wire-api.cabal +++ b/libs/wire-api/wire-api.cabal @@ -4,7 +4,7 @@ cabal-version: 1.12 -- -- see: https://github.com/sol/hpack -- --- hash: 2e4bca56fcdff432834ff4ef0e7678b73cafc509219dd2b1b0f6308dca6a2588 +-- hash: 8852dcbba2a141b5116810b08f8ace7078fa29585f490c15de2ddf9b6f403b97 name: wire-api version: 0.1.0 @@ -401,6 +401,7 @@ test-suite wire-api-tests Test.Wire.API.Golden.Manual.ClientCapabilityList Test.Wire.API.Golden.Manual.ConversationCoverView Test.Wire.API.Golden.Manual.ConversationPagingState + Test.Wire.API.Golden.Manual.ConversationsResponse Test.Wire.API.Golden.Manual.ConvIdsPage Test.Wire.API.Golden.Manual.FeatureConfigEvent Test.Wire.API.Golden.Manual.GetPaginatedConversationIds diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index d401ed4730..ee49881719 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -208,22 +208,13 @@ getConversationsInternal user mids mstart msize = do | Data.isConvDeleted c = Data.deleteConversation (Data.convId c) >> pure False | otherwise = pure True --- FUTUREWORK: pagination support for remote conversations, or should *all* of them be returned always? -- FUTUREWORK: optimize cassandra requests when retrieving conversations (avoid large IN queries, prefer parallel/chunked requests) -listConversations :: UserId -> Public.ListConversations -> Galley (Public.ConversationList Public.Conversation) -listConversations user (Public.ListConversations mIds qstart msize) = do +listConversations :: UserId -> Public.ListConversations -> Galley Public.ConversationsResponse +listConversations user (Public.ListConversations ids) = do localDomain <- viewFederationDomain - when (isJust mIds && isJust qstart) $ - throwM (invalidPayload "'start' and 'qualified_ids' are mutually exclusive") - (localMore, localConvIds, remoteConvIds) <- case mIds of - Just xs -> do - let (remoteConvIds, localIds) = partitionRemoteOrLocalIds' localDomain (toList xs) - (localMore, localConvIds) <- getIdsAndMore localIds - pure (localMore, localConvIds, remoteConvIds) - Nothing -> do - (localMore, localConvIds) <- getAll (localstart localDomain) - remoteConvIds <- Data.conversationsRemote user - pure (localMore, localConvIds, remoteConvIds) + + let (remoteConvIds, localIds) = partitionRemoteOrLocalIds' localDomain (toList ids) + localConvIds <- Data.conversationIdsOf user localIds localInternalConversations <- Data.conversations localConvIds @@ -233,23 +224,8 @@ listConversations user (Public.ListConversations mIds qstart msize) = do remoteConversations <- getRemoteConversations user remoteConvIds let allConvs = localConversations <> remoteConversations - pure $ Public.ConversationList allConvs localMore + pure $ Public.ConversationsResponse allConvs [] [] where - localstart localDomain = case qstart of - Just start | qDomain start == localDomain -> Just (qUnqualified start) - _ -> Nothing - - size = fromMaybe (toRange (Proxy @32)) msize - - getIdsAndMore :: [ConvId] -> Galley (Bool, [ConvId]) - getIdsAndMore ids = (False,) <$> Data.conversationIdsOf user ids - - getAll :: Maybe ConvId -> Galley (Bool, [ConvId]) - getAll mstart = do - r <- Data.conversationIdsFrom user mstart (rcast size) - let hasMore = Data.resultSetType r == Data.ResultSetTruncated - pure (hasMore, Data.resultSetResult r) - removeDeleted :: Data.Conversation -> Galley Bool removeDeleted c | Data.isConvDeleted c = Data.deleteConversation (Data.convId c) >> pure False diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index 05423fa558..9a6c1fee20 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -113,7 +113,6 @@ tests s = test s "metrics" metrics, test s "create conversation" postConvOk, test s "get empty conversations" getConvsOk, - test s "list-conversations empty" listConvsOk, test s "get conversations by ids" getConvsOk2, test s "list-conversations by ids" listConvsOk2, test s "fail to get >500 conversations" getConvsFailMaxSize, @@ -124,7 +123,6 @@ tests s = test s "paginate through /conversations/list-ids - page ending at locals and remote domain" paginateConvListIdsPageEndingAtLocalsAndDomain, test s "fail to get >1000 conversation ids" getConvIdsFailMaxSize, test s "page through conversations" getConvsPagingOk, - test s "page through list-conversations (local conversations only)" listConvsPagingOk, test s "fail to create conversation when not connected" postConvFailNotConnected, test s "fail to create conversation with qualified users when not connected" postConvQualifiedFailNotConnected, test s "M:N conversation creation must have postO2OConv alice bob (Just "gossip1") - let req1 = ListConversations (Just (cnvQualifiedId cnv1 :| [])) Nothing Nothing + let req1 = ListConversations (cnvQualifiedId cnv1 :| []) listConvs alice req1 !!! do const 200 === statusCode - const (Just [cnvQualifiedId cnv1]) === fmap (map cnvQualifiedId . convList) . responseJsonUnsafe + const (Just [cnvQualifiedId cnv1]) === fmap (map cnvQualifiedId . crFound) . responseJsonUnsafe -- create & get group conv carl <- randomUser connectUsers alice (singleton carl) cnv2 <- responseJsonUnsafeWithMsg "conversation" <$> postConv alice [bob, carl] (Just "gossip2") [] Nothing Nothing - let req2 = ListConversations (Just (cnvQualifiedId cnv2 :| [])) Nothing Nothing + let req2 = ListConversations (cnvQualifiedId cnv2 :| []) listConvs alice req2 !!! do const 200 === statusCode - const (Just [cnvQualifiedId cnv2]) === fmap (map cnvQualifiedId . convList) . responseJsonUnsafe + const (Just [cnvQualifiedId cnv2]) === fmap (map cnvQualifiedId . crFound) . responseJsonUnsafe -- get both - rs <- listAllConvs alice responseJsonUnsafe rs + let req3 = ListConversations (cnvQualifiedId cnv1 :| [cnvQualifiedId cnv2]) + rs <- listConvs alice req3 responseJsonUnsafe rs let c1 = convs >>= find ((== cnvQualifiedId cnv1) . cnvQualifiedId) let c2 = convs >>= find ((== cnvQualifiedId cnv2) . cnvQualifiedId) liftIO . forM_ [(cnv1, c1), (cnv2, c2)] $ \(expected, actual) -> do @@ -1424,31 +1415,31 @@ getConvsPagingOk = do -- same test as getConvsPagingOk, but using the listConversations endpoint -- (only tests pagination behaviour for local conversations) -- FUTUREWORK: pagination for remote conversations -listConvsPagingOk :: TestM () -listConvsPagingOk = do - [ally, bill, carl] <- randomUsers 3 - connectUsers ally (list1 bill [carl]) - replicateM_ 11 $ postConv ally [bill, carl] (Just "gossip") [] Nothing Nothing - walk ally [3, 3, 3, 3, 2] -- 11 (group) + 2 (1:1) + 1 (self) - walk bill [3, 3, 3, 3, 1] -- 11 (group) + 1 (1:1) + 1 (self) - walk carl [3, 3, 3, 3, 1] -- 11 (group) + 1 (1:1) + 1 (self) - where - walk :: Foldable t => UserId -> t Int -> TestM () - walk u = foldM_ (next u 3) Nothing - next :: UserId -> Int32 -> Maybe ConvId -> Int -> TestM (Maybe ConvId) - next u step start n = do - -- FUTUREWORK: support an endpoint to get qualified conversation IDs - -- (without all the conversation metadata) - r1 <- getConvIds u (Right <$> start) (Just step) responseJsonUnsafe r1 - liftIO $ assertEqual "unexpected length (getConvIds)" (Just n) (length <$> ids1) - localDomain <- viewFederationDomain - let requestBody = ListConversations Nothing (flip Qualified localDomain <$> start) (Just (unsafeRange step)) - r2 <- listConvs u requestBody responseJsonUnsafe r2 - liftIO $ assertEqual "unexpected length (getConvs)" (Just n) (length <$> ids3) - liftIO $ assertBool "getConvIds /= getConvs" (ids1 == ids3) - return $ ids1 >>= listToMaybe . reverse +-- listConvsPagingOk :: TestM () +-- listConvsPagingOk = do +-- [ally, bill, carl] <- randomUsers 3 +-- connectUsers ally (list1 bill [carl]) +-- replicateM_ 11 $ postConv ally [bill, carl] (Just "gossip") [] Nothing Nothing +-- walk ally [3, 3, 3, 3, 2] -- 11 (group) + 2 (1:1) + 1 (self) +-- walk bill [3, 3, 3, 3, 1] -- 11 (group) + 1 (1:1) + 1 (self) +-- walk carl [3, 3, 3, 3, 1] -- 11 (group) + 1 (1:1) + 1 (self) +-- where +-- walk :: Foldable t => UserId -> t Int -> TestM () +-- walk u = foldM_ (next u 3) Nothing +-- next :: UserId -> Int32 -> Maybe ConvId -> Int -> TestM (Maybe ConvId) +-- next u step start n = do +-- -- FUTUREWORK: support an endpoint to get qualified conversation IDs +-- -- (without all the conversation metadata) +-- r1 <- getConvIds u (Right <$> start) (Just step) responseJsonUnsafe r1 +-- liftIO $ assertEqual "unexpected length (getConvIds)" (Just n) (length <$> ids1) +-- localDomain <- viewFederationDomain +-- let requestBody = ListConversations Nothing (flip Qualified localDomain <$> start) (Just (unsafeRange step)) +-- r2 <- listConvs u requestBody responseJsonUnsafe r2 +-- liftIO $ assertEqual "unexpected length (getConvs)" (Just n) (length <$> ids3) +-- liftIO $ assertBool "getConvIds /= getConvs" (ids1 == ids3) +-- return $ ids1 >>= listToMaybe . reverse postConvFailNotConnected :: TestM () postConvFailNotConnected = do @@ -1829,6 +1820,9 @@ testAddRemoteMember = do toJSON [mkProfile bob (Name "bob")] | otherwise = toJSON () +-- TODO: Extend this test to include local convs, remote failures, local non +-- exsitent convs, local convs which the requesting user is not part of and +-- remote not founds. testGetRemoteConversations :: TestM () testGetRemoteConversations = do -- alice on local domain @@ -1876,16 +1870,17 @@ testGetRemoteConversations = do -- FUTUREWORK: Do this test with more than one remote domains -- test POST /list-conversations + let req = ListConversations (remoteConvId :| []) (respAll, _) <- withTempMockFederator opts remoteDomain (const remoteConversationResponse) - (listAllConvs alice) - convs :: ConversationList Conversation <- responseJsonUnsafe <$> (pure respAll (pure respAll cmOthers (cnvMembers c) \\ cmOthers (cnvMembers expected)) <$> actual) - assertEqual "expecting two conversation: Alice's self conversation and remote one with Bob" 2 (length (convList convs)) testAddRemoteMemberFailure :: TestM () testAddRemoteMemberFailure = do diff --git a/services/galley/test/integration/API/Util.hs b/services/galley/test/integration/API/Util.hs index 4287e62c5c..bf84e38aa0 100644 --- a/services/galley/test/integration/API/Util.hs +++ b/services/galley/test/integration/API/Util.hs @@ -33,7 +33,6 @@ import Control.Monad.Except (ExceptT, runExceptT) import Control.Retry (constantDelay, exponentialBackoff, limitRetries, retrying) import Data.Aeson hiding (json) import Data.Aeson.Lens (key, _String) -import Data.Aeson.Types (emptyObject) import qualified Data.ByteString as BS import qualified Data.ByteString.Base64 as B64 import qualified Data.ByteString.Char8 as C @@ -775,20 +774,6 @@ getConvs u r s = do . zType "access" . convRange r s --- (should be) equivalent to --- listConvs u (ListConversations [] Nothing Nothing) --- (if the schema of ListConversations is correct) -listAllConvs :: (MonadIO m, MonadHttp m, HasGalley m) => UserId -> m ResponseLBS -listAllConvs u = do - g <- viewGalley - post $ - g - . path "/list-conversations" - . zUser u - . zConn "conn" - . zType "access" - . json emptyObject - listConvs :: (MonadIO m, MonadHttp m, HasGalley m) => UserId -> ListConversations -> m ResponseLBS listConvs u req = do -- when using servant-client (pending #1605), this would become: From 28e8ddd75050af5d23c61edacb00df7209a64e5f Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Wed, 11 Aug 2021 10:18:48 +0200 Subject: [PATCH 02/29] Restore old endpoint, create v2 --- libs/wire-api/src/Wire/API/Conversation.hs | 29 +++- .../src/Wire/API/Routes/Public/Galley.hs | 15 +- ... => testObject_ListConversationsV2_1.json} | 0 .../test/unit/Test/Wire/API/Golden/Manual.hs | 6 +- ...onversations.hs => ListConversationsV2.hs} | 10 +- libs/wire-api/wire-api.cabal | 4 +- services/galley/src/Galley/API/Public.hs | 1 + services/galley/src/Galley/API/Query.hs | 53 +++++- services/galley/test/integration/API.hs | 157 +++++++++++++----- services/galley/test/integration/API/Util.hs | 26 +++ 10 files changed, 245 insertions(+), 56 deletions(-) rename libs/wire-api/test/golden/{testObject_ListConversations_1.json => testObject_ListConversationsV2_1.json} (100%) rename libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/{ListConversations.hs => ListConversationsV2.hs} (84%) diff --git a/libs/wire-api/src/Wire/API/Conversation.hs b/libs/wire-api/src/Wire/API/Conversation.hs index 29bb21f4a7..0f3922f966 100644 --- a/libs/wire-api/src/Wire/API/Conversation.hs +++ b/libs/wire-api/src/Wire/API/Conversation.hs @@ -27,6 +27,7 @@ module Wire.API.Conversation ConversationCoverView (..), ConversationList (..), ListConversations (..), + ListConversationsV2 (..), GetPaginatedConversationIds (..), ConversationPagingState (..), ConversationPagingTable (..), @@ -343,9 +344,10 @@ instance ToSchema GetPaginatedConversationIds where <$> gpciPagingState .= optFieldWithDocModifier "paging_state" Nothing addPagingStateDoc schema <*> gpciSize .= (fieldWithDocModifier "size" addSizeDoc schema <|> pure (toRange (Proxy @1000))) --- | Used on the POST /list-conversations endpoint -newtype ListConversations = ListConversations - { lQualifiedIds :: NonEmpty (Qualified ConvId) +data ListConversations = ListConversations + { lQualifiedIds :: Maybe (NonEmpty (Qualified ConvId)), + lStartId :: Maybe (Qualified ConvId), + lSize :: Maybe (Range 1 500 Int32) } deriving stock (Eq, Show, Generic) deriving (FromJSON, ToJSON, S.ToSchema) via Schema ListConversations @@ -354,9 +356,26 @@ instance ToSchema ListConversations where schema = objectWithDocModifier "ListConversations" - (description ?~ "A request to list some of a user's conversations, including remote ones") + (description ?~ "A request to list some or all of a user's conversations, including remote ones") $ ListConversations - <$> lQualifiedIds .= field "qualified_ids" (nonEmptyArray schema) + <$> lQualifiedIds .= optField "qualified_ids" Nothing (nonEmptyArray schema) + <*> lStartId .= optField "start_id" Nothing schema + <*> lSize .= optField "size" Nothing schema + +-- | Used on the POST /list-conversations endpoint +newtype ListConversationsV2 = ListConversationsV2 + { lcQualifiedIds :: NonEmpty (Qualified ConvId) + } + deriving stock (Eq, Show, Generic) + deriving (FromJSON, ToJSON, S.ToSchema) via Schema ListConversationsV2 + +instance ToSchema ListConversationsV2 where + schema = + objectWithDocModifier + "ListConversations" + (description ?~ "A request to list some of a user's conversations, including remote ones") + $ ListConversationsV2 + <$> lcQualifiedIds .= field "qualified_ids" (nonEmptyArray schema) data ConversationsResponse = ConversationsResponse { crFound :: [Conversation], diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Galley.hs b/libs/wire-api/src/Wire/API/Routes/Public/Galley.hs index 70b0362904..38c991ff95 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Galley.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Galley.hs @@ -188,10 +188,23 @@ data Api routes = Api :> Get '[Servant.JSON] (Public.ConversationList Public.Conversation), listConversations :: routes - :- Summary "Get conversation metadata for a list of conversation ids" + :- Summary "[deprecated] Get all conversations (also returns remote conversations)" + :> Description "Like GET /conversations, but allows specifying a list of remote conversations in its request body. \ + \Will return all or the requested qualified conversations, including remote ones. \ + \Size parameter is not yet honoured for remote conversations.\n\ + \**NOTE** This endpoint will soon be removed." :> ZUser :> "list-conversations" :> ReqBody '[Servant.JSON] Public.ListConversations + :> Post '[Servant.JSON] (Public.ConversationList Public.Conversation), + listConversationsV2 :: + routes + :- Summary "Get conversation metadata for a list of conversation ids" + :> ZUser + :> "conversations" + :> "list" + :> "v2" + :> ReqBody '[Servant.JSON] Public.ListConversationsV2 :> Post '[Servant.JSON] Public.ConversationsResponse, -- This endpoint can lead to the following events being sent: -- - ConvCreate event to members diff --git a/libs/wire-api/test/golden/testObject_ListConversations_1.json b/libs/wire-api/test/golden/testObject_ListConversationsV2_1.json similarity index 100% rename from libs/wire-api/test/golden/testObject_ListConversations_1.json rename to libs/wire-api/test/golden/testObject_ListConversationsV2_1.json diff --git a/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual.hs b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual.hs index d5b463b079..c5d09a6c73 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual.hs @@ -28,7 +28,7 @@ import Test.Wire.API.Golden.Manual.ConversationPagingState import Test.Wire.API.Golden.Manual.ConversationsResponse import Test.Wire.API.Golden.Manual.FeatureConfigEvent import Test.Wire.API.Golden.Manual.GetPaginatedConversationIds -import Test.Wire.API.Golden.Manual.ListConversations +import Test.Wire.API.Golden.Manual.ListConversationsV2 import Test.Wire.API.Golden.Manual.QualifiedUserClientPrekeyMap import Test.Wire.API.Golden.Manual.UserClientPrekeyMap import Test.Wire.API.Golden.Manual.UserIdList @@ -97,9 +97,9 @@ tests = [ (testObject_UserIdList_1, "testObject_UserIdList_1.json"), (testObject_UserIdList_2, "testObject_UserIdList_2.json") ], - testCase "ListConversations" $ + testCase "ListConversationsV2" $ testObjects - [(testObject_ListConversations_1, "testObject_ListConversations_1.json")], + [(testObject_ListConversationsV2_1, "testObject_ListConversationsV2_1.json")], testCase "ConversationsResponse" $ testObjects [(testObject_ConversationsResponse_1, "testObject_ConversationsResponse_1.json")] ] diff --git a/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversations.hs b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversationsV2.hs similarity index 84% rename from libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversations.hs rename to libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversationsV2.hs index 31f116b999..cabf0925cf 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversations.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversationsV2.hs @@ -15,7 +15,7 @@ -- You should have received a copy of the GNU Affero General Public License along -- with this program. If not, see . -module Test.Wire.API.Golden.Manual.ListConversations where +module Test.Wire.API.Golden.Manual.ListConversationsV2 where import Data.Domain (Domain (Domain)) import Data.Id (Id (Id)) @@ -23,11 +23,11 @@ import Data.List.NonEmpty (NonEmpty (..)) import Data.Qualified (Qualified (Qualified)) import qualified Data.UUID as UUID import Imports -import Wire.API.Conversation (ListConversations (..)) +import Wire.API.Conversation (ListConversationsV2 (..)) -testObject_ListConversations_1 :: ListConversations -testObject_ListConversations_1 = - ListConversations +testObject_ListConversationsV2_1 :: ListConversationsV2 +testObject_ListConversationsV2_1 = + ListConversationsV2 ( Qualified (Id (fromJust (UUID.fromString "00000018-0000-0020-0000-000e00000002"))) (Domain "domain.example.com") :| [ Qualified (Id (fromJust (UUID.fromString "00000018-0000-0020-0000-111111111112"))) (Domain "domain2.example.com") ] diff --git a/libs/wire-api/wire-api.cabal b/libs/wire-api/wire-api.cabal index 0fd5601f8c..30a59c829a 100644 --- a/libs/wire-api/wire-api.cabal +++ b/libs/wire-api/wire-api.cabal @@ -4,7 +4,7 @@ cabal-version: 1.12 -- -- see: https://github.com/sol/hpack -- --- hash: 8852dcbba2a141b5116810b08f8ace7078fa29585f490c15de2ddf9b6f403b97 +-- hash: dc88ad9211d31a3b215c3a2db745d7c55c0b91d2c9b0105de04ab959f18a5b68 name: wire-api version: 0.1.0 @@ -405,7 +405,7 @@ test-suite wire-api-tests Test.Wire.API.Golden.Manual.ConvIdsPage Test.Wire.API.Golden.Manual.FeatureConfigEvent Test.Wire.API.Golden.Manual.GetPaginatedConversationIds - Test.Wire.API.Golden.Manual.ListConversations + Test.Wire.API.Golden.Manual.ListConversationsV2 Test.Wire.API.Golden.Manual.QualifiedUserClientPrekeyMap Test.Wire.API.Golden.Manual.UserClientPrekeyMap Test.Wire.API.Golden.Manual.UserIdList diff --git a/services/galley/src/Galley/API/Public.hs b/services/galley/src/Galley/API/Public.hs index cba48a48c7..7f93785ce3 100644 --- a/services/galley/src/Galley/API/Public.hs +++ b/services/galley/src/Galley/API/Public.hs @@ -85,6 +85,7 @@ servantSitemap = GalleyAPI.getConversations = Query.getConversations, GalleyAPI.getConversationByReusableCode = Query.getConversationByReusableCode, GalleyAPI.listConversations = Query.listConversations, + GalleyAPI.listConversationsV2 = Query.listConversationsV2, GalleyAPI.createGroupConversation = Create.createGroupConversation, GalleyAPI.createSelfConversation = Create.createSelfConversation, GalleyAPI.createOne2OneConversation = Create.createOne2OneConversation, diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index ee49881719..09f6dfe937 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -25,6 +25,7 @@ module Galley.API.Query conversationIdsPageFrom, getConversations, listConversations, + listConversationsV2, iterateConversations, getSelfH, internalGetMemberH, @@ -208,9 +209,57 @@ getConversationsInternal user mids mstart msize = do | Data.isConvDeleted c = Data.deleteConversation (Data.convId c) >> pure False | otherwise = pure True + +-- | Deprecated +-- FUTUREWORK(federation): Delete this endpoint +listConversations :: UserId -> Public.ListConversations -> Galley (Public.ConversationList Public.Conversation) +listConversations user (Public.ListConversations mIds qstart msize) = do + localDomain <- viewFederationDomain + when (isJust mIds && isJust qstart) $ + throwM (invalidPayload "'start' and 'qualified_ids' are mutually exclusive") + (localMore, localConvIds, remoteConvIds) <- case mIds of + Just xs -> do + let (remoteConvIds, localIds) = partitionRemoteOrLocalIds' localDomain (toList xs) + (localMore, localConvIds) <- getIdsAndMore localIds + pure (localMore, localConvIds, remoteConvIds) + Nothing -> do + (localMore, localConvIds) <- getAll (localstart localDomain) + remoteConvIds <- Data.conversationsRemote user + pure (localMore, localConvIds, remoteConvIds) + + localInternalConversations <- + Data.conversations localConvIds + >>= filterM removeDeleted + >>= filterM (pure . isMember user . Data.convLocalMembers) + localConversations <- mapM (Mapping.conversationView user) localInternalConversations + + remoteConversations <- getRemoteConversations user remoteConvIds + let allConvs = localConversations <> remoteConversations + pure $ Public.ConversationList allConvs localMore + where + localstart localDomain = case qstart of + Just start | qDomain start == localDomain -> Just (qUnqualified start) + _ -> Nothing + + size = fromMaybe (toRange (Proxy @32)) msize + + getIdsAndMore :: [ConvId] -> Galley (Bool, [ConvId]) + getIdsAndMore ids = (False,) <$> Data.conversationIdsOf user ids + + getAll :: Maybe ConvId -> Galley (Bool, [ConvId]) + getAll mstart = do + r <- Data.conversationIdsFrom user mstart (rcast size) + let hasMore = Data.resultSetType r == Data.ResultSetTruncated + pure (hasMore, Data.resultSetResult r) + + removeDeleted :: Data.Conversation -> Galley Bool + removeDeleted c + | Data.isConvDeleted c = Data.deleteConversation (Data.convId c) >> pure False + | otherwise = pure True + -- FUTUREWORK: optimize cassandra requests when retrieving conversations (avoid large IN queries, prefer parallel/chunked requests) -listConversations :: UserId -> Public.ListConversations -> Galley Public.ConversationsResponse -listConversations user (Public.ListConversations ids) = do +listConversationsV2 :: UserId -> Public.ListConversationsV2 -> Galley Public.ConversationsResponse +listConversationsV2 user (Public.ListConversationsV2 ids) = do localDomain <- viewFederationDomain let (remoteConvIds, localIds) = partitionRemoteOrLocalIds' localDomain (toList ids) diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index 9a6c1fee20..382147d498 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -113,6 +113,7 @@ tests s = test s "metrics" metrics, test s "create conversation" postConvOk, test s "get empty conversations" getConvsOk, + test s "list-conversations empty" listConvsOk, test s "get conversations by ids" getConvsOk2, test s "list-conversations by ids" listConvsOk2, test s "fail to get >500 conversations" getConvsFailMaxSize, @@ -123,6 +124,7 @@ tests s = test s "paginate through /conversations/list-ids - page ending at locals and remote domain" paginateConvListIdsPageEndingAtLocalsAndDomain, test s "fail to get >1000 conversation ids" getConvIdsFailMaxSize, test s "page through conversations" getConvsPagingOk, + test s "page through list-conversations (local conversations only)" listConvsPagingOk, test s "fail to create conversation when not connected" postConvFailNotConnected, test s "fail to create conversation with qualified users when not connected" postConvQualifiedFailNotConnected, test s "M:N conversation creation must have postO2OConv alice bob (Just "gossip1") - let req1 = ListConversations (cnvQualifiedId cnv1 :| []) + let req1 = ListConversations (Just (cnvQualifiedId cnv1 :| [])) Nothing Nothing listConvs alice req1 !!! do const 200 === statusCode - const (Just [cnvQualifiedId cnv1]) === fmap (map cnvQualifiedId . crFound) . responseJsonUnsafe + const (Just [cnvQualifiedId cnv1]) === fmap (map cnvQualifiedId . convList) . responseJsonUnsafe -- create & get group conv carl <- randomUser connectUsers alice (singleton carl) cnv2 <- responseJsonUnsafeWithMsg "conversation" <$> postConv alice [bob, carl] (Just "gossip2") [] Nothing Nothing - let req2 = ListConversations (cnvQualifiedId cnv2 :| []) + let req2 = ListConversations (Just (cnvQualifiedId cnv2 :| [])) Nothing Nothing listConvs alice req2 !!! do const 200 === statusCode - const (Just [cnvQualifiedId cnv2]) === fmap (map cnvQualifiedId . crFound) . responseJsonUnsafe + const (Just [cnvQualifiedId cnv2]) === fmap (map cnvQualifiedId . convList) . responseJsonUnsafe -- get both - let req3 = ListConversations (cnvQualifiedId cnv1 :| [cnvQualifiedId cnv2]) - rs <- listConvs alice req3 responseJsonUnsafe rs + rs <- listAllConvs alice responseJsonUnsafe rs let c1 = convs >>= find ((== cnvQualifiedId cnv1) . cnvQualifiedId) let c2 = convs >>= find ((== cnvQualifiedId cnv2) . cnvQualifiedId) liftIO . forM_ [(cnv1, c1), (cnv2, c2)] $ \(expected, actual) -> do @@ -1415,31 +1425,31 @@ getConvsPagingOk = do -- same test as getConvsPagingOk, but using the listConversations endpoint -- (only tests pagination behaviour for local conversations) -- FUTUREWORK: pagination for remote conversations --- listConvsPagingOk :: TestM () --- listConvsPagingOk = do --- [ally, bill, carl] <- randomUsers 3 --- connectUsers ally (list1 bill [carl]) --- replicateM_ 11 $ postConv ally [bill, carl] (Just "gossip") [] Nothing Nothing --- walk ally [3, 3, 3, 3, 2] -- 11 (group) + 2 (1:1) + 1 (self) --- walk bill [3, 3, 3, 3, 1] -- 11 (group) + 1 (1:1) + 1 (self) --- walk carl [3, 3, 3, 3, 1] -- 11 (group) + 1 (1:1) + 1 (self) --- where --- walk :: Foldable t => UserId -> t Int -> TestM () --- walk u = foldM_ (next u 3) Nothing --- next :: UserId -> Int32 -> Maybe ConvId -> Int -> TestM (Maybe ConvId) --- next u step start n = do --- -- FUTUREWORK: support an endpoint to get qualified conversation IDs --- -- (without all the conversation metadata) --- r1 <- getConvIds u (Right <$> start) (Just step) responseJsonUnsafe r1 --- liftIO $ assertEqual "unexpected length (getConvIds)" (Just n) (length <$> ids1) --- localDomain <- viewFederationDomain --- let requestBody = ListConversations Nothing (flip Qualified localDomain <$> start) (Just (unsafeRange step)) --- r2 <- listConvs u requestBody responseJsonUnsafe r2 --- liftIO $ assertEqual "unexpected length (getConvs)" (Just n) (length <$> ids3) --- liftIO $ assertBool "getConvIds /= getConvs" (ids1 == ids3) --- return $ ids1 >>= listToMaybe . reverse +listConvsPagingOk :: TestM () +listConvsPagingOk = do + [ally, bill, carl] <- randomUsers 3 + connectUsers ally (list1 bill [carl]) + replicateM_ 11 $ postConv ally [bill, carl] (Just "gossip") [] Nothing Nothing + walk ally [3, 3, 3, 3, 2] -- 11 (group) + 2 (1:1) + 1 (self) + walk bill [3, 3, 3, 3, 1] -- 11 (group) + 1 (1:1) + 1 (self) + walk carl [3, 3, 3, 3, 1] -- 11 (group) + 1 (1:1) + 1 (self) + where + walk :: Foldable t => UserId -> t Int -> TestM () + walk u = foldM_ (next u 3) Nothing + next :: UserId -> Int32 -> Maybe ConvId -> Int -> TestM (Maybe ConvId) + next u step start n = do + -- FUTUREWORK: support an endpoint to get qualified conversation IDs + -- (without all the conversation metadata) + r1 <- getConvIds u (Right <$> start) (Just step) responseJsonUnsafe r1 + liftIO $ assertEqual "unexpected length (getConvIds)" (Just n) (length <$> ids1) + localDomain <- viewFederationDomain + let requestBody = ListConversations Nothing (flip Qualified localDomain <$> start) (Just (unsafeRange step)) + r2 <- listConvs u requestBody responseJsonUnsafe r2 + liftIO $ assertEqual "unexpected length (getConvs)" (Just n) (length <$> ids3) + liftIO $ assertBool "getConvIds /= getConvs" (ids1 == ids3) + return $ ids1 >>= listToMaybe . reverse postConvFailNotConnected :: TestM () postConvFailNotConnected = do @@ -1820,11 +1830,82 @@ testAddRemoteMember = do toJSON [mkProfile bob (Name "bob")] | otherwise = toJSON () +testGetRemoteConversations :: TestM () +testGetRemoteConversations = do + -- alice on local domain + -- bob and the conversation on the remote domain + aliceQ <- randomQualifiedUser + let alice = qUnqualified aliceQ + bobId <- randomId + convId <- randomId + let remoteDomain = Domain "far-away.example.com" + remoteConvId = Qualified convId remoteDomain + + let aliceAsOtherMember = OtherMember aliceQ Nothing roleNameWireAdmin + bobAsMember = Member bobId Nothing False Nothing Nothing False Nothing False Nothing roleNameWireAdmin + mockConversation = + Conversation + { cnvQualifiedId = remoteConvId, + cnvType = RegularConv, + cnvCreator = alice, + cnvAccess = [], + cnvAccessRole = ActivatedAccessRole, + cnvName = Just "federated gossip", + cnvMembers = ConvMembers bobAsMember [aliceAsOtherMember], + cnvTeam = Nothing, + cnvMessageTimer = Nothing, + cnvReceiptMode = Nothing + } + remoteConversationResponse = GetConversationsResponse [mockConversation] + opts <- view tsGConf + -- test GET /conversations/:domain/:cnv for single conversation + (respOne, _) <- + withTempMockFederator + opts + remoteDomain + (const remoteConversationResponse) + (getConvQualified alice remoteConvId) + conv :: Conversation <- responseJsonUnsafe <$> (pure respOne (pure respAll actual) + assertEqual + "self member mismatch" + (Just . cmSelf $ cnvMembers expected) + (cmSelf . cnvMembers <$> actual) + assertEqual + "other members mismatch" + (Just []) + ((\c -> cmOthers (cnvMembers c) \\ cmOthers (cnvMembers expected)) <$> actual) + assertEqual "expecting two conversation: Alice's self conversation and remote one with Bob" 2 (length (convList convs)) + -- TODO: Extend this test to include local convs, remote failures, local non -- exsitent convs, local convs which the requesting user is not part of and -- remote not founds. -testGetRemoteConversations :: TestM () -testGetRemoteConversations = do +testGetRemoteConversationsV2 :: TestM () +testGetRemoteConversationsV2 = do -- alice on local domain -- bob and the conversation on the remote domain aliceQ <- randomQualifiedUser @@ -1870,17 +1951,17 @@ testGetRemoteConversations = do -- FUTUREWORK: Do this test with more than one remote domains -- test POST /list-conversations - let req = ListConversations (remoteConvId :| []) + let req = ListConversationsV2 (remoteConvId :| []) (respAll, _) <- withTempMockFederator opts remoteDomain (const remoteConversationResponse) - (listConvs alice req) - convs <- responseJsonUnsafe <$> (pure respAll (pure respAll UserId -> m ResponseLBS +listAllConvs u = do + g <- viewGalley + post $ + g + . path "/list-conversations" + . zUser u + . zConn "conn" + . zType "access" + . json emptyObject + listConvs :: (MonadIO m, MonadHttp m, HasGalley m) => UserId -> ListConversations -> m ResponseLBS listConvs u req = do -- when using servant-client (pending #1605), this would become: @@ -788,6 +803,17 @@ listConvs u req = do . zType "access" . json req +listConvsV2 :: (MonadIO m, MonadHttp m, HasGalley m) => UserId -> ListConversationsV2 -> m ResponseLBS +listConvsV2 u req = do + g <- viewGalley + post $ + g + . path "/list-conversations" + . zUser u + . zConn "conn" + . zType "access" + . json req + getConv :: (MonadIO m, MonadHttp m, HasGalley m, HasCallStack) => UserId -> ConvId -> m ResponseLBS getConv u c = do g <- viewGalley From c0a5707b47ae109a875a174e2c4b8378e05e6920 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Wed, 11 Aug 2021 10:31:51 +0200 Subject: [PATCH 03/29] Ormolu --- libs/wire-api/src/Wire/API/Routes/Public/Galley.hs | 9 +++++---- services/galley/src/Galley/API/Query.hs | 1 - 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Galley.hs b/libs/wire-api/src/Wire/API/Routes/Public/Galley.hs index 38c991ff95..3e8fa9921b 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Galley.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Galley.hs @@ -189,10 +189,11 @@ data Api routes = Api listConversations :: routes :- Summary "[deprecated] Get all conversations (also returns remote conversations)" - :> Description "Like GET /conversations, but allows specifying a list of remote conversations in its request body. \ - \Will return all or the requested qualified conversations, including remote ones. \ - \Size parameter is not yet honoured for remote conversations.\n\ - \**NOTE** This endpoint will soon be removed." + :> Description + "Like GET /conversations, but allows specifying a list of remote conversations in its request body. \ + \Will return all or the requested qualified conversations, including remote ones. \ + \Size parameter is not yet honoured for remote conversations.\n\ + \**NOTE** This endpoint will soon be removed." :> ZUser :> "list-conversations" :> ReqBody '[Servant.JSON] Public.ListConversations diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index 09f6dfe937..1941dd448c 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -209,7 +209,6 @@ getConversationsInternal user mids mstart msize = do | Data.isConvDeleted c = Data.deleteConversation (Data.convId c) >> pure False | otherwise = pure True - -- | Deprecated -- FUTUREWORK(federation): Delete this endpoint listConversations :: UserId -> Public.ListConversations -> Galley (Public.ConversationList Public.Conversation) From 72e2ed8d62b2beacaf803d80fbacaa00697389cc Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Wed, 11 Aug 2021 13:24:38 +0200 Subject: [PATCH 04/29] Test the right endpoint! --- services/galley/test/integration/API.hs | 4 ++-- services/galley/test/integration/API/Util.hs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index 382147d498..0f2f12d452 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -1958,10 +1958,10 @@ testGetRemoteConversationsV2 = do remoteDomain (const remoteConversationResponse) (listConvsV2 alice req) - convs :: ConversationList Conversation <- responseJsonUnsafe <$> (pure respAll (pure respAll Date: Wed, 11 Aug 2021 15:08:07 +0200 Subject: [PATCH 05/29] Limit number of Ids to 1000 --- libs/wire-api/package.yaml | 1 + libs/wire-api/src/Wire/API/Conversation.hs | 7 ++++--- .../Test/Wire/API/Golden/Manual/ListConversationsV2.hs | 9 +++++---- libs/wire-api/wire-api.cabal | 3 ++- services/galley/src/Galley/API/Query.hs | 2 +- 5 files changed, 13 insertions(+), 9 deletions(-) diff --git a/libs/wire-api/package.yaml b/libs/wire-api/package.yaml index 82cc58a66f..d32a04a4e6 100644 --- a/libs/wire-api/package.yaml +++ b/libs/wire-api/package.yaml @@ -66,6 +66,7 @@ library: - servant-server - servant-swagger - schema-profunctor + - singletons - sop-core - string-conversions - swagger >=0.1 diff --git a/libs/wire-api/src/Wire/API/Conversation.hs b/libs/wire-api/src/Wire/API/Conversation.hs index 0f3922f966..0087e4c9a7 100644 --- a/libs/wire-api/src/Wire/API/Conversation.hs +++ b/libs/wire-api/src/Wire/API/Conversation.hs @@ -89,8 +89,9 @@ import Data.List1 import Data.Misc import Data.Proxy (Proxy (Proxy)) import Data.Qualified (Qualified (qUnqualified), deprecatedSchema) -import Data.Range (Range, toRange) +import Data.Range (Range, rangedSchema, toRange, fromRange) import Data.Schema +import Data.Singletons (sing) import qualified Data.Set as Set import Data.String.Conversions (cs) import qualified Data.Swagger as S @@ -364,7 +365,7 @@ instance ToSchema ListConversations where -- | Used on the POST /list-conversations endpoint newtype ListConversationsV2 = ListConversationsV2 - { lcQualifiedIds :: NonEmpty (Qualified ConvId) + { lcQualifiedIds :: Range 1 1000 [Qualified ConvId] } deriving stock (Eq, Show, Generic) deriving (FromJSON, ToJSON, S.ToSchema) via Schema ListConversationsV2 @@ -375,7 +376,7 @@ instance ToSchema ListConversationsV2 where "ListConversations" (description ?~ "A request to list some of a user's conversations, including remote ones") $ ListConversationsV2 - <$> lcQualifiedIds .= field "qualified_ids" (nonEmptyArray schema) + <$> (fromRange . lcQualifiedIds) .= field "qualified_ids" (rangedSchema sing sing (array schema)) data ConversationsResponse = ConversationsResponse { crFound :: [Conversation], diff --git a/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversationsV2.hs b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversationsV2.hs index cabf0925cf..788267f9b0 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversationsV2.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversationsV2.hs @@ -19,16 +19,17 @@ module Test.Wire.API.Golden.Manual.ListConversationsV2 where import Data.Domain (Domain (Domain)) import Data.Id (Id (Id)) -import Data.List.NonEmpty (NonEmpty (..)) import Data.Qualified (Qualified (Qualified)) import qualified Data.UUID as UUID import Imports import Wire.API.Conversation (ListConversationsV2 (..)) +import Data.Range (unsafeRange) testObject_ListConversationsV2_1 :: ListConversationsV2 testObject_ListConversationsV2_1 = ListConversationsV2 - ( Qualified (Id (fromJust (UUID.fromString "00000018-0000-0020-0000-000e00000002"))) (Domain "domain.example.com") - :| [ Qualified (Id (fromJust (UUID.fromString "00000018-0000-0020-0000-111111111112"))) (Domain "domain2.example.com") - ] + ( unsafeRange + [ Qualified (Id (fromJust (UUID.fromString "00000018-0000-0020-0000-000e00000002"))) (Domain "domain.example.com"), + Qualified (Id (fromJust (UUID.fromString "00000018-0000-0020-0000-111111111112"))) (Domain "domain2.example.com") + ] ) diff --git a/libs/wire-api/wire-api.cabal b/libs/wire-api/wire-api.cabal index 30a59c829a..dfa774db99 100644 --- a/libs/wire-api/wire-api.cabal +++ b/libs/wire-api/wire-api.cabal @@ -4,7 +4,7 @@ cabal-version: 1.12 -- -- see: https://github.com/sol/hpack -- --- hash: dc88ad9211d31a3b215c3a2db745d7c55c0b91d2c9b0105de04ab959f18a5b68 +-- hash: efca28ca2d2ca3ccfcaf3b543293e01d05d18802dd38283d7658ec50939231d9 name: wire-api version: 0.1.0 @@ -148,6 +148,7 @@ library , servant-multipart , servant-server , servant-swagger + , singletons , sop-core , string-conversions , swagger >=0.1 diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index 1941dd448c..defb0560f9 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -261,7 +261,7 @@ listConversationsV2 :: UserId -> Public.ListConversationsV2 -> Galley Public.Con listConversationsV2 user (Public.ListConversationsV2 ids) = do localDomain <- viewFederationDomain - let (remoteConvIds, localIds) = partitionRemoteOrLocalIds' localDomain (toList ids) + let (remoteConvIds, localIds) = partitionRemoteOrLocalIds' localDomain (fromRange ids) localConvIds <- Data.conversationIdsOf user localIds localInternalConversations <- From efbd7c33a07a4c1fce35ef4b66a8cf2e9f95ef52 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Wed, 11 Aug 2021 15:11:45 +0200 Subject: [PATCH 06/29] Add a TODO to verify old code --- services/galley/src/Galley/API/Query.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index defb0560f9..1994438cfb 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -95,7 +95,7 @@ getConversation zusr cnv = do localDomain <- viewFederationDomain if qDomain cnv == localDomain then getUnqualifiedConversation zusr (qUnqualified cnv) - else getRemoteConversation zusr (toRemote cnv) + else getRemoteConversation zusr (toRemote cnv) -- TODO: This looks wrong, we shouldn't get a remote conversation is the requesting user is not part of that conversation. getRemoteConversation :: UserId -> Remote ConvId -> Galley Public.Conversation getRemoteConversation zusr remoteConvId = do From d92c44ba8df8a0521be3f6c4f2ae471a0eeece21 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Wed, 11 Aug 2021 15:14:48 +0200 Subject: [PATCH 07/29] Reorganize tests --- services/galley/test/integration/API.hs | 141 ++++++++++--------- services/galley/test/integration/API/Util.hs | 27 +++- 2 files changed, 100 insertions(+), 68 deletions(-) diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index 0f2f12d452..4750b4c1e3 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -1,3 +1,4 @@ +{-# LANGUAGE RecordWildCards #-} {-# OPTIONS_GHC -Wno-incomplete-uni-patterns #-} -- This file is part of the Wire Server implementation. @@ -35,7 +36,6 @@ import API.Util import Bilge hiding (timeout) import Bilge.Assert import Brig.Types -import qualified Cassandra as Cql import qualified Control.Concurrent.Async as Async import Control.Lens (at, ix, preview, view, (.~), (?~), (^.)) import Control.Monad.Except (MonadError (throwError)) @@ -59,7 +59,6 @@ import Data.String.Conversions (cs) import qualified Data.Text as T import qualified Data.Text.Ascii as Ascii import Data.Time.Clock (getCurrentTime) -import qualified Galley.Data as Cql import Galley.Options (Opts, optFederator) import Galley.Types hiding (InternalMember (..)) import Galley.Types.Conversations.Roles @@ -153,8 +152,10 @@ tests s = test s "fail to add members when not connected" postMembersFail, test s "fail to add too many members" postTooManyMembersFail, test s "add remote members" testAddRemoteMember, - test s "get and list remote conversations" testGetRemoteConversations, - test s "get and list remote conversations v2" testGetRemoteConversationsV2, + test s "get conversations/:domain/:cnv - local" testGetQualifiedLocalConv, + test s "get conversations/:domain/:cnv - remote" testGetQualifiedRemoteConv, + test s "post list-conversations" testListRemoteConvs, + test s "post conversations/list/v2 - success" testBulkGetQualifiedConvsSuccess, test s "add non-existing remote members" testAddRemoteMemberFailure, test s "add deleted remote members" testAddDeletedRemoteUser, test s "add remote members on invalid domain" testAddRemoteMemberInvalidDomain, @@ -1830,8 +1831,60 @@ testAddRemoteMember = do toJSON [mkProfile bob (Name "bob")] | otherwise = toJSON () -testGetRemoteConversations :: TestM () -testGetRemoteConversations = do +testGetQualifiedLocalConv :: TestM () +testGetQualifiedLocalConv = do + alice <- randomUser + convId <- decodeQualifiedConvId <$> postConv alice [] (Just "gossip") [] Nothing Nothing + conv :: Conversation <- fmap responseJsonUnsafe $ getConvQualified alice convId (pure respAll (pure respOne (pure respAll (pure respAll actual) - assertEqual - "self member mismatch" - (Just . cmSelf $ cnvMembers expected) - (cmSelf . cnvMembers <$> actual) - assertEqual - "other members mismatch" - (Just []) - ((\c -> cmOthers (cnvMembers c) \\ cmOthers (cnvMembers expected)) <$> actual) + assertEqual "conversations" (Just expected) actual assertEqual "expecting two conversation: Alice's self conversation and remote one with Bob" 2 (length (convList convs)) -- TODO: Extend this test to include local convs, remote failures, local non -- exsitent convs, local convs which the requesting user is not part of and -- remote not founds. -testGetRemoteConversationsV2 :: TestM () -testGetRemoteConversationsV2 = do +testBulkGetQualifiedConvsSuccess :: TestM () +testBulkGetQualifiedConvsSuccess = do -- alice on local domain -- bob and the conversation on the remote domain aliceQ <- randomQualifiedUser @@ -1913,10 +1943,12 @@ testGetRemoteConversationsV2 = do bobId <- randomId convId <- randomId let remoteDomain = Domain "far-away.example.com" + bobQ = Qualified bobId remoteDomain remoteConvId = Qualified convId remoteDomain - let aliceAsOtherMember = OtherMember aliceQ Nothing roleNameWireAdmin - bobAsMember = Member bobId Nothing False Nothing Nothing False Nothing False Nothing roleNameWireAdmin + let aliceAsSelfMember = Member (qUnqualified aliceQ) Nothing False Nothing Nothing False Nothing False Nothing roleNameWireAdmin + aliceAsOtherMember = OtherMember aliceQ Nothing roleNameWireAdmin + bobAsOtherMember = OtherMember bobQ Nothing roleNameWireAdmin mockConversation = Conversation { cnvQualifiedId = remoteConvId, @@ -1925,33 +1957,19 @@ testGetRemoteConversationsV2 = do cnvAccess = [], cnvAccessRole = ActivatedAccessRole, cnvName = Just "federated gossip", - cnvMembers = ConvMembers bobAsMember [aliceAsOtherMember], + cnvMembers = ConvMembers aliceAsSelfMember [bobAsOtherMember], cnvTeam = Nothing, cnvMessageTimer = Nothing, cnvReceiptMode = Nothing } remoteConversationResponse = GetConversationsResponse [mockConversation] opts <- view tsGConf - -- test GET /conversations/:domain/:cnv for single conversation - (respOne, _) <- - withTempMockFederator - opts - remoteDomain - (const remoteConversationResponse) - (getConvQualified alice remoteConvId) - conv :: Conversation <- responseJsonUnsafe <$> (pure respOne actual) - assertEqual - "self member mismatch" - (Just . cmSelf $ cnvMembers expected) - (cmSelf . cnvMembers <$> actual) - assertEqual - "other members mismatch" - (Just []) - ((\c -> cmOthers (cnvMembers c) \\ cmOthers (cnvMembers expected)) <$> actual) + assertEqual "conversation" (Just expected) actual testAddRemoteMemberFailure :: TestM () testAddRemoteMemberFailure = do diff --git a/services/galley/test/integration/API/Util.hs b/services/galley/test/integration/API/Util.hs index dff77cda4e..c183af6807 100644 --- a/services/galley/test/integration/API/Util.hs +++ b/services/galley/test/integration/API/Util.hs @@ -1,4 +1,5 @@ {-# OPTIONS_GHC -Wno-incomplete-uni-patterns #-} +{-# LANGUAGE RecordWildCards #-} -- This file is part of the Wire Server implementation. -- @@ -117,6 +118,7 @@ import Wire.API.Message import qualified Wire.API.Message.Proto as Proto import Wire.API.User.Client (ClientCapability (..), UserClientsFull (UserClientsFull)) import qualified Wire.API.User.Client as Client +import Data.Time (getCurrentTime) ------------------------------------------------------------------------------- -- API Operations @@ -1139,6 +1141,26 @@ getTeamQueue' zusr msince msize onlyLast = do ] ) +registerRemoteConv :: Qualified ConvId -> Qualified UserId -> Maybe Text -> Set OtherMember -> TestM () +registerRemoteConv convId originUser name othMembers = do + fedGalleyClient <- view tsFedGalleyClient + now <- liftIO getCurrentTime + FederatedGalley.registerConversation + fedGalleyClient + ( FederatedGalley.MkRegisterConversation + { rcTime = now, + rcOrigUserId = originUser, + rcCnvId = convId, + rcCnvType = RegularConv, + rcCnvAccess = [], + rcCnvAccessRole = ActivatedAccessRole, + rcCnvName = name, + rcMembers = othMembers, + rcMessageTimer = Nothing, + rcReceiptMode = Nothing + } + ) + ------------------------------------------------------------------------------- -- Common Assertions @@ -1366,7 +1388,10 @@ decodeConvCodeEvent r = case responseJsonUnsafe r of _ -> error "Failed to parse ConversationCode from Event" decodeConvId :: HasCallStack => Response (Maybe Lazy.ByteString) -> ConvId -decodeConvId = qUnqualified . cnvQualifiedId . responseJsonUnsafe +decodeConvId = qUnqualified . decodeQualifiedConvId + +decodeQualifiedConvId :: HasCallStack => Response (Maybe Lazy.ByteString) -> Qualified ConvId +decodeQualifiedConvId = cnvQualifiedId . responseJsonUnsafe decodeConvList :: Response (Maybe Lazy.ByteString) -> [Conversation] decodeConvList = convList . responseJsonUnsafeWithMsg "conversations" From d69d5ee8563ca619332a10069f81dbf97770900a Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Wed, 11 Aug 2021 15:26:35 +0200 Subject: [PATCH 08/29] Also test getting local conversation --- services/galley/test/integration/API.hs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index 4750b4c1e3..b3b149e337 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -1967,9 +1967,12 @@ testBulkGetQualifiedConvsSuccess = do registerRemoteConv remoteConvId bobQ Nothing (Set.fromList [aliceAsOtherMember]) + localConv <- responseJsonUnsafe <$> postConv alice [] (Just "gossip") [] Nothing Nothing + let localConvId = cnvQualifiedId localConv + -- FUTUREWORK: Do this test with more than one remote domains -- test POST /list-conversations - let req = ListConversationsV2 (unsafeRange [remoteConvId]) + let req = ListConversationsV2 (unsafeRange [localConvId, remoteConvId]) (respAll, _) <- withTempMockFederator opts @@ -1978,9 +1981,14 @@ testBulkGetQualifiedConvsSuccess = do (listConvsV2 alice req) convs :: ConversationsResponse <- responseJsonUnsafe <$> (pure respAll Date: Wed, 11 Aug 2021 15:58:30 +0200 Subject: [PATCH 09/29] Test with multiple remotes --- services/galley/test/integration/API.hs | 70 ++++++++++---------- services/galley/test/integration/API/Util.hs | 19 +++++- 2 files changed, 53 insertions(+), 36 deletions(-) diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index b3b149e337..ac21b38402 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -1931,9 +1931,8 @@ testListRemoteConvs = do assertEqual "conversations" (Just expected) actual assertEqual "expecting two conversation: Alice's self conversation and remote one with Bob" 2 (length (convList convs)) --- TODO: Extend this test to include local convs, remote failures, local non --- exsitent convs, local convs which the requesting user is not part of and --- remote not founds. +-- TODO: Write another test for remote failures, local non exsitent convs, local +-- convs which the requesting user is not part of and remote not founds. testBulkGetQualifiedConvsSuccess :: TestM () testBulkGetQualifiedConvsSuccess = do -- alice on local domain @@ -1941,52 +1940,55 @@ testBulkGetQualifiedConvsSuccess = do aliceQ <- randomQualifiedUser let alice = qUnqualified aliceQ bobId <- randomId + carlId <- randomId convId <- randomId - let remoteDomain = Domain "far-away.example.com" - bobQ = Qualified bobId remoteDomain - remoteConvId = Qualified convId remoteDomain - - let aliceAsSelfMember = Member (qUnqualified aliceQ) Nothing False Nothing Nothing False Nothing False Nothing roleNameWireAdmin - aliceAsOtherMember = OtherMember aliceQ Nothing roleNameWireAdmin - bobAsOtherMember = OtherMember bobQ Nothing roleNameWireAdmin - mockConversation = - Conversation - { cnvQualifiedId = remoteConvId, - cnvType = RegularConv, - cnvCreator = alice, - cnvAccess = [], - cnvAccessRole = ActivatedAccessRole, - cnvName = Just "federated gossip", - cnvMembers = ConvMembers aliceAsSelfMember [bobAsOtherMember], - cnvTeam = Nothing, - cnvMessageTimer = Nothing, - cnvReceiptMode = Nothing - } - remoteConversationResponse = GetConversationsResponse [mockConversation] - opts <- view tsGConf - - registerRemoteConv remoteConvId bobQ Nothing (Set.fromList [aliceAsOtherMember]) + let remoteDomainA = Domain "a.far-away.example.com" + remoteDomainB = Domain "b.far-away.example.com" + bobQ = Qualified bobId remoteDomainA + carlQ = Qualified carlId remoteDomainB + remoteConvIdA = Qualified convId remoteDomainA + remoteConvIdB = Qualified convId remoteDomainB localConv <- responseJsonUnsafe <$> postConv alice [] (Just "gossip") [] Nothing Nothing let localConvId = cnvQualifiedId localConv + -- TODO: Make this necessary + let aliceAsOtherMember = OtherMember aliceQ Nothing roleNameWireAdmin + registerRemoteConv remoteConvIdA bobQ Nothing (Set.fromList [aliceAsOtherMember]) + registerRemoteConv remoteConvIdB carlQ Nothing (Set.fromList [aliceAsOtherMember]) + -- FUTUREWORK: Do this test with more than one remote domains -- test POST /list-conversations - let req = ListConversationsV2 (unsafeRange [localConvId, remoteConvId]) + let aliceAsSelfMember = Member (qUnqualified aliceQ) Nothing False Nothing Nothing False Nothing False Nothing roleNameWireAdmin + bobAsOtherMember = OtherMember bobQ Nothing roleNameWireAdmin + carlAsOtherMember = OtherMember carlQ Nothing roleNameWireAdmin + mockConversationA = mkConv remoteConvIdA bobId aliceAsSelfMember [bobAsOtherMember] + mockConversationB = mkConv remoteConvIdB carlId aliceAsSelfMember [carlAsOtherMember] + req = ListConversationsV2 (unsafeRange [localConvId, remoteConvIdA, remoteConvIdB]) + opts <- view tsGConf (respAll, _) <- withTempMockFederator opts - remoteDomain - (const remoteConversationResponse) + remoteDomainA + (\fedReq -> + case F.domain fedReq of + d | d == domainText remoteDomainA -> GetConversationsResponse [mockConversationA] + d | d == domainText remoteDomainB -> GetConversationsResponse [mockConversationB] + _ -> GetConversationsResponse [] + ) (listConvsV2 alice req) - convs :: ConversationsResponse <- responseJsonUnsafe <$> (pure respAll (pure respAll UserId -> Member -> [OtherMember] -> Conversation +mkConv cnvId creator selfMember otherMembers = + Conversation + { cnvQualifiedId = cnvId, + cnvType = RegularConv, + cnvCreator = creator, + cnvAccess = [], + cnvAccessRole = ActivatedAccessRole, + cnvName = Just "federated gossip", + cnvMembers = ConvMembers selfMember otherMembers, + cnvTeam = Nothing, + cnvMessageTimer = Nothing, + cnvReceiptMode = Nothing + } + -- | ES is only refreshed occasionally; we don't want to wait for that in tests. refreshIndex :: TestM () refreshIndex = do From 4d70f2cbd8c53e6002456d1f9f4bce7571d46cc3 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Wed, 11 Aug 2021 17:01:06 +0200 Subject: [PATCH 10/29] Ensure no unauthorized access to remote conversation metadata --- libs/wire-api/src/Wire/API/Conversation.hs | 4 +- .../API/Golden/Manual/ListConversationsV2.hs | 2 +- services/galley/src/Galley/API/Query.hs | 13 +++-- services/galley/src/Galley/Data.hs | 24 +++++++-- services/galley/test/integration/API.hs | 51 +++++++++++++++---- 5 files changed, 73 insertions(+), 21 deletions(-) diff --git a/libs/wire-api/src/Wire/API/Conversation.hs b/libs/wire-api/src/Wire/API/Conversation.hs index 0087e4c9a7..c8588be636 100644 --- a/libs/wire-api/src/Wire/API/Conversation.hs +++ b/libs/wire-api/src/Wire/API/Conversation.hs @@ -89,10 +89,10 @@ import Data.List1 import Data.Misc import Data.Proxy (Proxy (Proxy)) import Data.Qualified (Qualified (qUnqualified), deprecatedSchema) -import Data.Range (Range, rangedSchema, toRange, fromRange) +import Data.Range (Range, fromRange, rangedSchema, toRange) import Data.Schema -import Data.Singletons (sing) import qualified Data.Set as Set +import Data.Singletons (sing) import Data.String.Conversions (cs) import qualified Data.Swagger as S import qualified Data.Swagger.Build.Api as Doc diff --git a/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversationsV2.hs b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversationsV2.hs index 788267f9b0..6cac2a29ca 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversationsV2.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Golden/Manual/ListConversationsV2.hs @@ -20,10 +20,10 @@ module Test.Wire.API.Golden.Manual.ListConversationsV2 where import Data.Domain (Domain (Domain)) import Data.Id (Id (Id)) import Data.Qualified (Qualified (Qualified)) +import Data.Range (unsafeRange) import qualified Data.UUID as UUID import Imports import Wire.API.Conversation (ListConversationsV2 (..)) -import Data.Range (unsafeRange) testObject_ListConversationsV2_1 :: ListConversationsV2 testObject_ListConversationsV2_1 = diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index 1994438cfb..c352b4fa27 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -44,6 +44,7 @@ import Data.Id as Id import Data.Proxy import Data.Qualified (Qualified (..), Remote, partitionRemote, partitionRemoteOrLocalIds', toRemote) import Data.Range +import Data.Tagged (unTagged) import Galley.API.Error import qualified Galley.API.Mapping as Mapping import Galley.API.Util @@ -261,8 +262,9 @@ listConversationsV2 :: UserId -> Public.ListConversationsV2 -> Galley Public.Con listConversationsV2 user (Public.ListConversationsV2 ids) = do localDomain <- viewFederationDomain - let (remoteConvIds, localIds) = partitionRemoteOrLocalIds' localDomain (fromRange ids) + let (remoteIds, localIds) = partitionRemoteOrLocalIds' localDomain (fromRange ids) localConvIds <- Data.conversationIdsOf user localIds + (foundRemoteIds, notFoundRemoteIds) <- Data.remoteConversationIdOf user remoteIds localInternalConversations <- Data.conversations localConvIds @@ -270,9 +272,14 @@ listConversationsV2 user (Public.ListConversationsV2 ids) = do >>= filterM (pure . isMember user . Data.convLocalMembers) localConversations <- mapM (Mapping.conversationView user) localInternalConversations - remoteConversations <- getRemoteConversations user remoteConvIds + remoteConversations <- getRemoteConversations user foundRemoteIds let allConvs = localConversations <> remoteConversations - pure $ Public.ConversationsResponse allConvs [] [] + pure $ + Public.ConversationsResponse + { crFound = allConvs, + crNotFound = map unTagged notFoundRemoteIds, + crFailed = [] + } where removeDeleted :: Data.Conversation -> Galley Bool removeDeleted c diff --git a/services/galley/src/Galley/Data.hs b/services/galley/src/Galley/Data.hs index 32e5797d3f..34c4da7b11 100644 --- a/services/galley/src/Galley/Data.hs +++ b/services/galley/src/Galley/Data.hs @@ -58,6 +58,7 @@ module Galley.Data acceptConnect, conversation, conversationIdsFrom, + remoteConversationIdOf, localConversationIdsPageFrom, conversationIdRowsForPagination, conversationIdsOf, @@ -575,13 +576,26 @@ conversationIdRowsForPagination usr start (fromRange -> max) = Just c -> paginate Cql.selectUserConvsFrom (paramsP Quorum (usr, c) max) Nothing -> paginate Cql.selectUserConvs (paramsP Quorum (Identity usr) max) -conversationIdsOf :: - (MonadClient m, Log.MonadLogger m, MonadThrow m) => - UserId -> - [ConvId] -> - m [ConvId] +conversationIdsOf :: (MonadClient m) => UserId -> [ConvId] -> m [ConvId] conversationIdsOf usr cids = runIdentity <$$> retry x1 (query Cql.selectUserConvsIn (params Quorum (usr, cids))) +-- | Takes a list of conversation ids and splits them by those found for the +-- given user and those which are not found. +remoteConversationIdOf :: forall m. (MonadClient m, MonadLogger m, MonadUnliftIO m) => UserId -> [Remote ConvId] -> m ([Remote ConvId], [Remote ConvId]) +remoteConversationIdOf usr cids = + concatTuples <$> pooledMapConcurrentlyN 8 splitOne cids + where + concatTuples :: [(Maybe a, Maybe b)] -> ([a], [b]) + concatTuples xs = (mapMaybe fst xs, mapMaybe snd xs) + + splitOne :: Remote ConvId -> m (Maybe (Remote ConvId), Maybe (Remote ConvId)) + splitOne remoteConvId = do + let (Qualified conv domain) = unTagged remoteConvId + mbMembership <- query1 Cql.selectRemoteConvMembership (params Quorum (usr, domain, conv)) + case mbMembership of + Nothing -> pure (Nothing, Just remoteConvId) + Just _ -> pure (Just remoteConvId, Nothing) + conversationsRemote :: (MonadClient m) => UserId -> m [Remote ConvId] conversationsRemote usr = do (\(d, c) -> toRemote $ Qualified c d) <$$> retry x1 (query Cql.selectUserRemoteConvs (params Quorum (Identity usr))) diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index ac21b38402..4f54794d52 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -1933,20 +1933,44 @@ testListRemoteConvs = do -- TODO: Write another test for remote failures, local non exsitent convs, local -- convs which the requesting user is not part of and remote not founds. + +-- | Tests getting many converations given their ids. +-- +-- In this test: +-- +-- - Alice is a local user, who will be part of these convs and be asking for +-- their metadata +-- +-- - Bob is on a.far-away.example.com, there is 1 conversation between Alice and +-- Bob +-- +-- - Carl is on b.far-away.example.com, there is 1 conversation between Alice +-- and Carl +-- +-- - Alice tries to get 1 conversation from a.far-away.example.com, but is not +-- found in the local DB +-- +-- - TODO: Alice tries to get 1 conversation from b.far-away.example.com, it is +-- found in the local DB but remote does not return it +-- +-- - TODO: Alice tries to get 1 conversation from c.far-away.example.com, but +-- the federated call fails +-- - TODO: Alice tries to get 1 local conversation which doesn't exist +-- - TODO: Alice tries to get 1 local conersation which they're not part of testBulkGetQualifiedConvsSuccess :: TestM () testBulkGetQualifiedConvsSuccess = do - -- alice on local domain - -- bob and the conversation on the remote domain aliceQ <- randomQualifiedUser let alice = qUnqualified aliceQ bobId <- randomId carlId <- randomId convId <- randomId + convIdNotFound <- randomId let remoteDomainA = Domain "a.far-away.example.com" remoteDomainB = Domain "b.far-away.example.com" bobQ = Qualified bobId remoteDomainA carlQ = Qualified carlId remoteDomainB remoteConvIdA = Qualified convId remoteDomainA + remoteConvIdALocallyNotFound = Qualified convIdNotFound remoteDomainA remoteConvIdB = Qualified convId remoteDomainB localConv <- responseJsonUnsafe <$> postConv alice [] (Just "gossip") [] Nothing Nothing @@ -1964,17 +1988,17 @@ testBulkGetQualifiedConvsSuccess = do carlAsOtherMember = OtherMember carlQ Nothing roleNameWireAdmin mockConversationA = mkConv remoteConvIdA bobId aliceAsSelfMember [bobAsOtherMember] mockConversationB = mkConv remoteConvIdB carlId aliceAsSelfMember [carlAsOtherMember] - req = ListConversationsV2 (unsafeRange [localConvId, remoteConvIdA, remoteConvIdB]) + req = ListConversationsV2 (unsafeRange [localConvId, remoteConvIdA, remoteConvIdB, remoteConvIdALocallyNotFound]) opts <- view tsGConf - (respAll, _) <- + (respAll, receivedRequests) <- withTempMockFederator opts remoteDomainA - (\fedReq -> - case F.domain fedReq of - d | d == domainText remoteDomainA -> GetConversationsResponse [mockConversationA] - d | d == domainText remoteDomainB -> GetConversationsResponse [mockConversationB] - _ -> GetConversationsResponse [] + ( \fedReq -> + case F.domain fedReq of + d | d == domainText remoteDomainA -> GetConversationsResponse [mockConversationA] + d | d == domainText remoteDomainB -> GetConversationsResponse [mockConversationB] + _ -> GetConversationsResponse [] ) (listConvsV2 alice req) convs <- responseJsonUnsafe <$> (pure respAll Date: Thu, 12 Aug 2021 09:35:30 +0200 Subject: [PATCH 11/29] Ensure local not founds are reported --- services/galley/src/Galley/API/Query.hs | 8 ++-- services/galley/src/Galley/Data.hs | 18 ++++++-- services/galley/src/Galley/Data/Queries.hs | 3 ++ services/galley/test/integration/API.hs | 48 ++++++++++++++-------- 4 files changed, 53 insertions(+), 24 deletions(-) diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index c352b4fa27..40d873d919 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -263,11 +263,11 @@ listConversationsV2 user (Public.ListConversationsV2 ids) = do localDomain <- viewFederationDomain let (remoteIds, localIds) = partitionRemoteOrLocalIds' localDomain (fromRange ids) - localConvIds <- Data.conversationIdsOf user localIds + (foundLocalIds, notFoundLocalIds) <- Data.localConversationIdsOf user localIds (foundRemoteIds, notFoundRemoteIds) <- Data.remoteConversationIdOf user remoteIds localInternalConversations <- - Data.conversations localConvIds + Data.conversations foundLocalIds >>= filterM removeDeleted >>= filterM (pure . isMember user . Data.convLocalMembers) localConversations <- mapM (Mapping.conversationView user) localInternalConversations @@ -277,7 +277,9 @@ listConversationsV2 user (Public.ListConversationsV2 ids) = do pure $ Public.ConversationsResponse { crFound = allConvs, - crNotFound = map unTagged notFoundRemoteIds, + crNotFound = + map unTagged notFoundRemoteIds + <> map (`Qualified` localDomain) notFoundLocalIds, crFailed = [] } where diff --git a/services/galley/src/Galley/Data.hs b/services/galley/src/Galley/Data.hs index 34c4da7b11..291e6d39be 100644 --- a/services/galley/src/Galley/Data.hs +++ b/services/galley/src/Galley/Data.hs @@ -58,6 +58,7 @@ module Galley.Data acceptConnect, conversation, conversationIdsFrom, + localConversationIdsOf, remoteConversationIdOf, localConversationIdsPageFrom, conversationIdRowsForPagination, @@ -576,18 +577,26 @@ conversationIdRowsForPagination usr start (fromRange -> max) = Just c -> paginate Cql.selectUserConvsFrom (paramsP Quorum (usr, c) max) Nothing -> paginate Cql.selectUserConvs (paramsP Quorum (Identity usr) max) +-- Deprecated conversationIdsOf :: (MonadClient m) => UserId -> [ConvId] -> m [ConvId] conversationIdsOf usr cids = runIdentity <$$> retry x1 (query Cql.selectUserConvsIn (params Quorum (usr, cids))) +localConversationIdsOf :: forall m. (MonadClient m, MonadUnliftIO m) => UserId -> [ConvId] -> m ([ConvId], [ConvId]) +localConversationIdsOf usr cids = do + concatTuples <$> pooledMapConcurrentlyN 8 splitOne cids + where + splitOne :: ConvId -> m (Maybe ConvId, Maybe ConvId) + splitOne conv = do + mbMembership <- query1 Cql.selectUserConvMembership (params Quorum (usr, conv)) + case mbMembership of + Nothing -> pure (Nothing, Just conv) + Just _ -> pure (Just conv, Nothing) -- | Takes a list of conversation ids and splits them by those found for the -- given user and those which are not found. remoteConversationIdOf :: forall m. (MonadClient m, MonadLogger m, MonadUnliftIO m) => UserId -> [Remote ConvId] -> m ([Remote ConvId], [Remote ConvId]) remoteConversationIdOf usr cids = concatTuples <$> pooledMapConcurrentlyN 8 splitOne cids where - concatTuples :: [(Maybe a, Maybe b)] -> ([a], [b]) - concatTuples xs = (mapMaybe fst xs, mapMaybe snd xs) - splitOne :: Remote ConvId -> m (Maybe (Remote ConvId), Maybe (Remote ConvId)) splitOne remoteConvId = do let (Qualified conv domain) = unTagged remoteConvId @@ -596,6 +605,9 @@ remoteConversationIdOf usr cids = Nothing -> pure (Nothing, Just remoteConvId) Just _ -> pure (Just remoteConvId, Nothing) +concatTuples :: [(Maybe a, Maybe b)] -> ([a], [b]) +concatTuples xs = (mapMaybe fst xs, mapMaybe snd xs) + conversationsRemote :: (MonadClient m) => UserId -> m [Remote ConvId] conversationsRemote usr = do (\(d, c) -> toRemote $ Qualified c d) <$$> retry x1 (query Cql.selectUserRemoteConvs (params Quorum (Identity usr))) diff --git a/services/galley/src/Galley/Data/Queries.hs b/services/galley/src/Galley/Data/Queries.hs index cdc2bc3128..106de5b697 100644 --- a/services/galley/src/Galley/Data/Queries.hs +++ b/services/galley/src/Galley/Data/Queries.hs @@ -240,6 +240,9 @@ deleteCode = "DELETE FROM conversation_codes WHERE key = ? AND scope = ?" selectUserConvs :: PrepQuery R (Identity UserId) (Identity ConvId) selectUserConvs = "select conv from user where user = ? order by conv" +selectUserConvMembership :: PrepQuery R (UserId, ConvId) (Identity UserId) +selectUserConvMembership = "select user from user where user = ? and conv = ?" + selectUserConvsIn :: PrepQuery R (UserId, [ConvId]) (Identity ConvId) selectUserConvsIn = "select conv from user where user = ? and conv in ? order by conv" diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index 4f54794d52..6e7e344230 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -1955,27 +1955,33 @@ testListRemoteConvs = do -- -- - TODO: Alice tries to get 1 conversation from c.far-away.example.com, but -- the federated call fails --- - TODO: Alice tries to get 1 local conversation which doesn't exist --- - TODO: Alice tries to get 1 local conersation which they're not part of +-- +-- - Alice tries to get 1 local conversation which doesn't exist +-- +-- - Alice tries to get 1 local conersation which they're not part of testBulkGetQualifiedConvsSuccess :: TestM () testBulkGetQualifiedConvsSuccess = do + localDomain <- viewFederationDomain aliceQ <- randomQualifiedUser let alice = qUnqualified aliceQ bobId <- randomId carlId <- randomId convId <- randomId - convIdNotFound <- randomId let remoteDomainA = Domain "a.far-away.example.com" remoteDomainB = Domain "b.far-away.example.com" bobQ = Qualified bobId remoteDomainA carlQ = Qualified carlId remoteDomainB remoteConvIdA = Qualified convId remoteDomainA - remoteConvIdALocallyNotFound = Qualified convIdNotFound remoteDomainA remoteConvIdB = Qualified convId remoteDomainB localConv <- responseJsonUnsafe <$> postConv alice [] (Just "gossip") [] Nothing Nothing let localConvId = cnvQualifiedId localConv + remoteConvIdALocallyNotFound <- flip Qualified remoteDomainA <$> randomId + localConvIdNotFound <- flip Qualified localDomain <$> randomId + eve <- randomQualifiedUser + localConvIdNotParticipating <- decodeQualifiedConvId <$> postConv (qUnqualified eve) [] (Just "gossip about alice!") [] Nothing Nothing + -- TODO: Make this necessary let aliceAsOtherMember = OtherMember aliceQ Nothing roleNameWireAdmin registerRemoteConv remoteConvIdA bobQ Nothing (Set.fromList [aliceAsOtherMember]) @@ -1988,7 +1994,15 @@ testBulkGetQualifiedConvsSuccess = do carlAsOtherMember = OtherMember carlQ Nothing roleNameWireAdmin mockConversationA = mkConv remoteConvIdA bobId aliceAsSelfMember [bobAsOtherMember] mockConversationB = mkConv remoteConvIdB carlId aliceAsSelfMember [carlAsOtherMember] - req = ListConversationsV2 (unsafeRange [localConvId, remoteConvIdA, remoteConvIdB, remoteConvIdALocallyNotFound]) + req = + ListConversationsV2 . unsafeRange $ + [ localConvId, + remoteConvIdA, + remoteConvIdB, + remoteConvIdALocallyNotFound, + localConvIdNotFound, + localConvIdNotParticipating + ] opts <- view tsGConf (respAll, receivedRequests) <- withTempMockFederator @@ -2004,25 +2018,23 @@ testBulkGetQualifiedConvsSuccess = do convs <- responseJsonUnsafe <$> (pure respAll Date: Thu, 12 Aug 2021 09:36:35 +0200 Subject: [PATCH 12/29] Remote tackled TODO --- services/galley/test/integration/API.hs | 1 - 1 file changed, 1 deletion(-) diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index 6e7e344230..ad393dd759 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -1982,7 +1982,6 @@ testBulkGetQualifiedConvsSuccess = do eve <- randomQualifiedUser localConvIdNotParticipating <- decodeQualifiedConvId <$> postConv (qUnqualified eve) [] (Just "gossip about alice!") [] Nothing Nothing - -- TODO: Make this necessary let aliceAsOtherMember = OtherMember aliceQ Nothing roleNameWireAdmin registerRemoteConv remoteConvIdA bobQ Nothing (Set.fromList [aliceAsOtherMember]) registerRemoteConv remoteConvIdB carlQ Nothing (Set.fromList [aliceAsOtherMember]) From 91d67833e084ee7c9232ae69ef1dc5c419935589 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Thu, 12 Aug 2021 17:35:21 +0200 Subject: [PATCH 13/29] Ensure remote not founds are reported --- services/galley/src/Galley/API/Query.hs | 14 ++++++++++++-- services/galley/test/integration/API.hs | 19 +++++++++++-------- services/galley/test/integration/API/Util.hs | 3 +++ 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index 40d873d919..af83591940 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -44,6 +44,7 @@ import Data.Id as Id import Data.Proxy import Data.Qualified (Qualified (..), Remote, partitionRemote, partitionRemoteOrLocalIds', toRemote) import Data.Range +import qualified Data.Set as Set import Data.Tagged (unTagged) import Galley.API.Error import qualified Galley.API.Mapping as Mapping @@ -58,6 +59,7 @@ import Network.HTTP.Types import Network.Wai import Network.Wai.Predicate hiding (result, setStatus) import Network.Wai.Utilities +import qualified System.Logger.Class as Logger import UnliftIO (pooledForConcurrentlyN) import Wire.API.Conversation (ConversationCoverView (..)) import qualified Wire.API.Conversation as Public @@ -264,7 +266,7 @@ listConversationsV2 user (Public.ListConversationsV2 ids) = do let (remoteIds, localIds) = partitionRemoteOrLocalIds' localDomain (fromRange ids) (foundLocalIds, notFoundLocalIds) <- Data.localConversationIdsOf user localIds - (foundRemoteIds, notFoundRemoteIds) <- Data.remoteConversationIdOf user remoteIds + (foundRemoteIds, locallyNotFoundRemoteIds) <- Data.remoteConversationIdOf user remoteIds localInternalConversations <- Data.conversations foundLocalIds @@ -273,12 +275,20 @@ listConversationsV2 user (Public.ListConversationsV2 ids) = do localConversations <- mapM (Mapping.conversationView user) localInternalConversations remoteConversations <- getRemoteConversations user foundRemoteIds + let fetchedRemoteIds = Set.fromList $ map Public.cnvQualifiedId remoteConversations + remoteNotFoundRemoteIds = filter (`Set.notMember` fetchedRemoteIds) $ map unTagged foundRemoteIds + unless (null remoteNotFoundRemoteIds) $ + Logger.info $ + Logger.msg ("Some locally found conversation ids were not returned by remotes" :: ByteString) + . Logger.field "convIds" (show remoteNotFoundRemoteIds) + let allConvs = localConversations <> remoteConversations pure $ Public.ConversationsResponse { crFound = allConvs, crNotFound = - map unTagged notFoundRemoteIds + map unTagged locallyNotFoundRemoteIds + <> remoteNotFoundRemoteIds <> map (`Qualified` localDomain) notFoundLocalIds, crFailed = [] } diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index ad393dd759..1a0d1350fb 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -1950,7 +1950,7 @@ testListRemoteConvs = do -- - Alice tries to get 1 conversation from a.far-away.example.com, but is not -- found in the local DB -- --- - TODO: Alice tries to get 1 conversation from b.far-away.example.com, it is +-- - Alice tries to get 1 conversation from b.far-away.example.com, it is -- found in the local DB but remote does not return it -- -- - TODO: Alice tries to get 1 conversation from c.far-away.example.com, but @@ -1966,25 +1966,27 @@ testBulkGetQualifiedConvsSuccess = do let alice = qUnqualified aliceQ bobId <- randomId carlId <- randomId - convId <- randomId let remoteDomainA = Domain "a.far-away.example.com" remoteDomainB = Domain "b.far-away.example.com" bobQ = Qualified bobId remoteDomainA carlQ = Qualified carlId remoteDomainB - remoteConvIdA = Qualified convId remoteDomainA - remoteConvIdB = Qualified convId remoteDomainB localConv <- responseJsonUnsafe <$> postConv alice [] (Just "gossip") [] Nothing Nothing let localConvId = cnvQualifiedId localConv - remoteConvIdALocallyNotFound <- flip Qualified remoteDomainA <$> randomId - localConvIdNotFound <- flip Qualified localDomain <$> randomId + remoteConvIdA <- randomQualifiedId remoteDomainA + remoteConvIdB <- randomQualifiedId remoteDomainB + remoteConvIdALocallyNotFound <- randomQualifiedId remoteDomainA + remoteConvIdBNotFoundOnRemote <- randomQualifiedId remoteDomainB + localConvIdNotFound <- randomQualifiedId localDomain + eve <- randomQualifiedUser localConvIdNotParticipating <- decodeQualifiedConvId <$> postConv (qUnqualified eve) [] (Just "gossip about alice!") [] Nothing Nothing let aliceAsOtherMember = OtherMember aliceQ Nothing roleNameWireAdmin registerRemoteConv remoteConvIdA bobQ Nothing (Set.fromList [aliceAsOtherMember]) registerRemoteConv remoteConvIdB carlQ Nothing (Set.fromList [aliceAsOtherMember]) + registerRemoteConv remoteConvIdBNotFoundOnRemote carlQ Nothing (Set.fromList [aliceAsOtherMember]) -- FUTUREWORK: Do this test with more than one remote domains -- test POST /list-conversations @@ -2000,7 +2002,8 @@ testBulkGetQualifiedConvsSuccess = do remoteConvIdB, remoteConvIdALocallyNotFound, localConvIdNotFound, - localConvIdNotParticipating + localConvIdNotParticipating, + remoteConvIdBNotFoundOnRemote ] opts <- view tsGConf (respAll, receivedRequests) <- @@ -2029,7 +2032,7 @@ testBulkGetQualifiedConvsSuccess = do =<< find ((== domainText remoteDomainA) . F.domain) receivedRequests assertEqual "only locally found conversations should be queried" (Just [qUnqualified remoteConvIdA]) requestedConvIdsA - let expectedNotFound = sort [localConvIdNotFound, localConvIdNotParticipating, remoteConvIdALocallyNotFound] + let expectedNotFound = sort [localConvIdNotFound, localConvIdNotParticipating, remoteConvIdALocallyNotFound, remoteConvIdBNotFoundOnRemote] actualNotFound = sort $ crNotFound convs assertEqual "not founds" expectedNotFound actualNotFound assertEqual "no failures" [] (crFailed convs) diff --git a/services/galley/test/integration/API/Util.hs b/services/galley/test/integration/API/Util.hs index 5c183fd8e7..361aede80a 100644 --- a/services/galley/test/integration/API/Util.hs +++ b/services/galley/test/integration/API/Util.hs @@ -1547,6 +1547,9 @@ randomUser = qUnqualified <$> randomUser' False True True randomQualifiedUser :: HasCallStack => TestM (Qualified UserId) randomQualifiedUser = randomUser' False True True +randomQualifiedId :: MonadIO m => Domain -> m (Qualified (Id a)) +randomQualifiedId domain = flip Qualified domain <$> randomId + randomTeamCreator :: HasCallStack => TestM UserId randomTeamCreator = qUnqualified <$> randomUser' True True True From bdea5d0344940a30a1db5918e80ffae14d1e0c4e Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Thu, 12 Aug 2021 18:33:03 +0200 Subject: [PATCH 14/29] Ensure failed conversation fetches are logged and reported --- .../src/Wire/API/Federation/Client.hs | 1 + services/galley/src/Galley/API/Query.hs | 34 ++++++++++++++++--- services/galley/test/integration/API.hs | 22 +++++++----- 3 files changed, 45 insertions(+), 12 deletions(-) diff --git a/libs/wire-api-federation/src/Wire/API/Federation/Client.hs b/libs/wire-api-federation/src/Wire/API/Federation/Client.hs index 50b7172637..ee39c5ed59 100644 --- a/libs/wire-api-federation/src/Wire/API/Federation/Client.hs +++ b/libs/wire-api-federation/src/Wire/API/Federation/Client.hs @@ -123,6 +123,7 @@ data FederationError | FederationNotImplemented | FederationNotConfigured | FederationCallFailure FederationClientFailure + deriving (Show, Eq) data FederationClientFailure = FederationClientFailure { fedFailDomain :: Domain, diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index af83591940..95dd3f4624 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -36,6 +36,7 @@ where import qualified Cassandra as C import Control.Monad.Catch (throwM) +import Control.Monad.Except (ExceptT, runExceptT) import qualified Data.ByteString.Lazy as LBS import Data.Code import Data.CommaSeparatedList @@ -67,6 +68,7 @@ import qualified Wire.API.Conversation.Role as Public import Wire.API.ErrorDescription (convNotFound) import Wire.API.Federation.API.Galley (gcresConvs) import qualified Wire.API.Federation.API.Galley as FederatedGalley +import Wire.API.Federation.Client (FederationError, executeFederated) import Wire.API.Federation.Error import qualified Wire.API.Provider.Bot as Public @@ -119,6 +121,30 @@ getRemoteConversations zusr remoteConvs = do gcresConvs <$> runFederatedGalley remoteDomain rpc pure $ concat convs +getRemoteConversationsWithFailures :: UserId -> [Remote ConvId] -> Galley ([Qualified ConvId], [Public.Conversation]) +getRemoteConversationsWithFailures zusr remoteConvs = do + localDomain <- viewFederationDomain + let qualifiedZUser = Qualified zusr localDomain + let convsByDomain = partitionRemote remoteConvs + convs <- pooledForConcurrentlyN 8 convsByDomain $ \(remoteDomain, convIds) -> handleFailures remoteDomain convIds $ do + let req = FederatedGalley.GetConversationsRequest qualifiedZUser convIds + rpc = FederatedGalley.getConversations FederatedGalley.clientRoutes req + gcresConvs <$> executeFederated remoteDomain rpc + pure $ concatEithers convs + where + handleFailures :: Domain -> [ConvId] -> ExceptT FederationError Galley a -> Galley (Either [Qualified ConvId] a) + handleFailures domain convIds action = do + res <- runExceptT action + case res of + Right a -> pure $ Right a + Left e -> do + Logger.warn $ + Logger.msg ("Error occurred while fetch remote conversations" :: ByteString) + . Logger.field "error" (show e) + pure . Left $ map (`Qualified` domain) convIds + concatEithers :: (Monoid a, Monoid b) => [Either a b] -> (a, b) + concatEithers = bimap mconcat mconcat . partitionEithers + getConversationRoles :: UserId -> ConvId -> Galley Public.ConversationRolesList getConversationRoles zusr cnv = do void $ getConversationAndCheckMembership zusr cnv @@ -274,9 +300,9 @@ listConversationsV2 user (Public.ListConversationsV2 ids) = do >>= filterM (pure . isMember user . Data.convLocalMembers) localConversations <- mapM (Mapping.conversationView user) localInternalConversations - remoteConversations <- getRemoteConversations user foundRemoteIds - let fetchedRemoteIds = Set.fromList $ map Public.cnvQualifiedId remoteConversations - remoteNotFoundRemoteIds = filter (`Set.notMember` fetchedRemoteIds) $ map unTagged foundRemoteIds + (remoteFailures, remoteConversations) <- getRemoteConversationsWithFailures user foundRemoteIds + let fetchedOrFailedRemoteIds = Set.fromList $ map Public.cnvQualifiedId remoteConversations <> remoteFailures + remoteNotFoundRemoteIds = filter (`Set.notMember` fetchedOrFailedRemoteIds) $ map unTagged foundRemoteIds unless (null remoteNotFoundRemoteIds) $ Logger.info $ Logger.msg ("Some locally found conversation ids were not returned by remotes" :: ByteString) @@ -290,7 +316,7 @@ listConversationsV2 user (Public.ListConversationsV2 ids) = do map unTagged locallyNotFoundRemoteIds <> remoteNotFoundRemoteIds <> map (`Qualified` localDomain) notFoundLocalIds, - crFailed = [] + crFailed = remoteFailures } where removeDeleted :: Data.Conversation -> Galley Bool diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index 1a0d1350fb..06cef6376b 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -1953,7 +1953,7 @@ testListRemoteConvs = do -- - Alice tries to get 1 conversation from b.far-away.example.com, it is -- found in the local DB but remote does not return it -- --- - TODO: Alice tries to get 1 conversation from c.far-away.example.com, but +-- - Alice tries to get 1 conversation from c.far-away.example.com, but -- the federated call fails -- -- - Alice tries to get 1 local conversation which doesn't exist @@ -1968,6 +1968,7 @@ testBulkGetQualifiedConvsSuccess = do carlId <- randomId let remoteDomainA = Domain "a.far-away.example.com" remoteDomainB = Domain "b.far-away.example.com" + remoteDomainC = Domain "c.far-away.example.com" bobQ = Qualified bobId remoteDomainA carlQ = Qualified carlId remoteDomainB @@ -1979,6 +1980,7 @@ testBulkGetQualifiedConvsSuccess = do remoteConvIdALocallyNotFound <- randomQualifiedId remoteDomainA remoteConvIdBNotFoundOnRemote <- randomQualifiedId remoteDomainB localConvIdNotFound <- randomQualifiedId localDomain + remoteConvIdCFailure <- randomQualifiedId remoteDomainC eve <- randomQualifiedUser localConvIdNotParticipating <- decodeQualifiedConvId <$> postConv (qUnqualified eve) [] (Just "gossip about alice!") [] Nothing Nothing @@ -1987,6 +1989,7 @@ testBulkGetQualifiedConvsSuccess = do registerRemoteConv remoteConvIdA bobQ Nothing (Set.fromList [aliceAsOtherMember]) registerRemoteConv remoteConvIdB carlQ Nothing (Set.fromList [aliceAsOtherMember]) registerRemoteConv remoteConvIdBNotFoundOnRemote carlQ Nothing (Set.fromList [aliceAsOtherMember]) + registerRemoteConv remoteConvIdCFailure carlQ Nothing (Set.fromList [aliceAsOtherMember]) -- FUTUREWORK: Do this test with more than one remote domains -- test POST /list-conversations @@ -2003,18 +2006,21 @@ testBulkGetQualifiedConvsSuccess = do remoteConvIdALocallyNotFound, localConvIdNotFound, localConvIdNotParticipating, - remoteConvIdBNotFoundOnRemote + remoteConvIdBNotFoundOnRemote, + remoteConvIdCFailure ] opts <- view tsGConf (respAll, receivedRequests) <- - withTempMockFederator + withTempMockFederator' opts remoteDomainA - ( \fedReq -> + ( \fedReq -> do + let success = pure . F.OutwardResponseBody . LBS.toStrict . encode case F.domain fedReq of - d | d == domainText remoteDomainA -> GetConversationsResponse [mockConversationA] - d | d == domainText remoteDomainB -> GetConversationsResponse [mockConversationB] - _ -> GetConversationsResponse [] + d | d == domainText remoteDomainA -> success $ GetConversationsResponse [mockConversationA] + d | d == domainText remoteDomainB -> success $ GetConversationsResponse [mockConversationB] + d | d == domainText remoteDomainC -> pure . F.OutwardResponseError $ F.OutwardError F.DiscoveryFailed Nothing + _ -> assertFailure $ "Unrecognized domain: " <> show fedReq ) (listConvsV2 alice req) convs <- responseJsonUnsafe <$> (pure respAll Date: Tue, 17 Aug 2021 16:21:55 +0200 Subject: [PATCH 15/29] Ormolu --- services/galley/src/Galley/Data.hs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/services/galley/src/Galley/Data.hs b/services/galley/src/Galley/Data.hs index 291e6d39be..1cee2b9d87 100644 --- a/services/galley/src/Galley/Data.hs +++ b/services/galley/src/Galley/Data.hs @@ -585,12 +585,13 @@ localConversationIdsOf :: forall m. (MonadClient m, MonadUnliftIO m) => UserId - localConversationIdsOf usr cids = do concatTuples <$> pooledMapConcurrentlyN 8 splitOne cids where - splitOne :: ConvId -> m (Maybe ConvId, Maybe ConvId) + splitOne :: ConvId -> m (Maybe ConvId, Maybe ConvId) splitOne conv = do mbMembership <- query1 Cql.selectUserConvMembership (params Quorum (usr, conv)) case mbMembership of Nothing -> pure (Nothing, Just conv) Just _ -> pure (Just conv, Nothing) + -- | Takes a list of conversation ids and splits them by those found for the -- given user and those which are not found. remoteConversationIdOf :: forall m. (MonadClient m, MonadLogger m, MonadUnliftIO m) => UserId -> [Remote ConvId] -> m ([Remote ConvId], [Remote ConvId]) From 046ada54e6bd20927850047e03e076a3fc4756e1 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Tue, 17 Aug 2021 16:40:19 +0200 Subject: [PATCH 16/29] Remove tackled TODO --- services/galley/test/integration/API.hs | 3 --- 1 file changed, 3 deletions(-) diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index 06cef6376b..a561e4d53d 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -1931,9 +1931,6 @@ testListRemoteConvs = do assertEqual "conversations" (Just expected) actual assertEqual "expecting two conversation: Alice's self conversation and remote one with Bob" 2 (length (convList convs)) --- TODO: Write another test for remote failures, local non exsitent convs, local --- convs which the requesting user is not part of and remote not founds. - -- | Tests getting many converations given their ids. -- -- In this test: From f3f79586e94295bcf4af6cdf58e6f5509d8c6d69 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Tue, 17 Aug 2021 16:42:36 +0200 Subject: [PATCH 17/29] Remove tackled FUTUREWORK --- services/galley/src/Galley/API/Query.hs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index 95dd3f4624..bd972d212a 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -238,8 +238,7 @@ getConversationsInternal user mids mstart msize = do | Data.isConvDeleted c = Data.deleteConversation (Data.convId c) >> pure False | otherwise = pure True --- | Deprecated --- FUTUREWORK(federation): Delete this endpoint +-- | Deprecated. FUTUREWORK(federation): Delete this endpoint listConversations :: UserId -> Public.ListConversations -> Galley (Public.ConversationList Public.Conversation) listConversations user (Public.ListConversations mIds qstart msize) = do localDomain <- viewFederationDomain @@ -285,7 +284,6 @@ listConversations user (Public.ListConversations mIds qstart msize) = do | Data.isConvDeleted c = Data.deleteConversation (Data.convId c) >> pure False | otherwise = pure True --- FUTUREWORK: optimize cassandra requests when retrieving conversations (avoid large IN queries, prefer parallel/chunked requests) listConversationsV2 :: UserId -> Public.ListConversationsV2 -> Galley Public.ConversationsResponse listConversationsV2 user (Public.ListConversationsV2 ids) = do localDomain <- viewFederationDomain From e688135143823dd6a6089e82cd5a6b2b68d16fef Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Tue, 17 Aug 2021 16:44:56 +0200 Subject: [PATCH 18/29] Add FUTUREWORK to deal with backendds being out of sync --- services/galley/src/Galley/API/Query.hs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index bd972d212a..92063e1e08 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -302,6 +302,9 @@ listConversationsV2 user (Public.ListConversationsV2 ids) = do let fetchedOrFailedRemoteIds = Set.fromList $ map Public.cnvQualifiedId remoteConversations <> remoteFailures remoteNotFoundRemoteIds = filter (`Set.notMember` fetchedOrFailedRemoteIds) $ map unTagged foundRemoteIds unless (null remoteNotFoundRemoteIds) $ + -- FUTUREWORK: This implies that the backends are out of sync. Maybe the + -- current user should be considered removed from this conversation at this + -- point. Logger.info $ Logger.msg ("Some locally found conversation ids were not returned by remotes" :: ByteString) . Logger.field "convIds" (show remoteNotFoundRemoteIds) From 4fad988413f1ca2445987be0de7a2be1ebcd4543 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Tue, 17 Aug 2021 16:50:12 +0200 Subject: [PATCH 19/29] Fix endpoint path in comment --- libs/wire-api/src/Wire/API/Conversation.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/wire-api/src/Wire/API/Conversation.hs b/libs/wire-api/src/Wire/API/Conversation.hs index c8588be636..c90884cef2 100644 --- a/libs/wire-api/src/Wire/API/Conversation.hs +++ b/libs/wire-api/src/Wire/API/Conversation.hs @@ -363,7 +363,7 @@ instance ToSchema ListConversations where <*> lStartId .= optField "start_id" Nothing schema <*> lSize .= optField "size" Nothing schema --- | Used on the POST /list-conversations endpoint +-- | Used on the POST /conversations/list/v2 endpoint newtype ListConversationsV2 = ListConversationsV2 { lcQualifiedIds :: Range 1 1000 [Qualified ConvId] } From bab7fc0da2e214ea0d475557b0dc0e887dbdbdd9 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Tue, 17 Aug 2021 18:36:42 +0200 Subject: [PATCH 20/29] Simplify checking membership --- services/galley/src/Galley/Data.hs | 33 ++++++++++++++++-------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/services/galley/src/Galley/Data.hs b/services/galley/src/Galley/Data.hs index 1cee2b9d87..ccd89bca8c 100644 --- a/services/galley/src/Galley/Data.hs +++ b/services/galley/src/Galley/Data.hs @@ -581,33 +581,36 @@ conversationIdRowsForPagination usr start (fromRange -> max) = conversationIdsOf :: (MonadClient m) => UserId -> [ConvId] -> m [ConvId] conversationIdsOf usr cids = runIdentity <$$> retry x1 (query Cql.selectUserConvsIn (params Quorum (usr, cids))) +-- | Takes a list of conversation ids and splits them by those found for the +-- given user and those which are not found. +-- +-- Returns @(foundConvs, notFoundConvs)@. localConversationIdsOf :: forall m. (MonadClient m, MonadUnliftIO m) => UserId -> [ConvId] -> m ([ConvId], [ConvId]) localConversationIdsOf usr cids = do - concatTuples <$> pooledMapConcurrentlyN 8 splitOne cids + partitionEithers <$> pooledMapConcurrentlyN 8 findConv cids where - splitOne :: ConvId -> m (Maybe ConvId, Maybe ConvId) - splitOne conv = do + findConv :: ConvId -> m (Either ConvId ConvId) + findConv conv = do mbMembership <- query1 Cql.selectUserConvMembership (params Quorum (usr, conv)) - case mbMembership of - Nothing -> pure (Nothing, Just conv) - Just _ -> pure (Just conv, Nothing) + pure $ case mbMembership of + Nothing -> Right conv + Just _ -> Left conv -- | Takes a list of conversation ids and splits them by those found for the -- given user and those which are not found. +-- +-- Returns @(foundConvs, notFoundConvs)@. remoteConversationIdOf :: forall m. (MonadClient m, MonadLogger m, MonadUnliftIO m) => UserId -> [Remote ConvId] -> m ([Remote ConvId], [Remote ConvId]) remoteConversationIdOf usr cids = - concatTuples <$> pooledMapConcurrentlyN 8 splitOne cids + partitionEithers <$> pooledMapConcurrentlyN 8 findConv cids where - splitOne :: Remote ConvId -> m (Maybe (Remote ConvId), Maybe (Remote ConvId)) - splitOne remoteConvId = do + findConv :: Remote ConvId -> m (Either (Remote ConvId) (Remote ConvId)) + findConv remoteConvId = do let (Qualified conv domain) = unTagged remoteConvId mbMembership <- query1 Cql.selectRemoteConvMembership (params Quorum (usr, domain, conv)) - case mbMembership of - Nothing -> pure (Nothing, Just remoteConvId) - Just _ -> pure (Just remoteConvId, Nothing) - -concatTuples :: [(Maybe a, Maybe b)] -> ([a], [b]) -concatTuples xs = (mapMaybe fst xs, mapMaybe snd xs) + pure $ case mbMembership of + Nothing -> Right remoteConvId + Just _ -> Left remoteConvId conversationsRemote :: (MonadClient m) => UserId -> m [Remote ConvId] conversationsRemote usr = do From 414b282522563e47164757242cb5ac525376df67 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Thu, 19 Aug 2021 11:57:56 +0200 Subject: [PATCH 21/29] Update changelog-draft --- CHANGELOG-draft.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG-draft.md b/CHANGELOG-draft.md index 1e1ad95529..084803eea8 100644 --- a/CHANGELOG-draft.md +++ b/CHANGELOG-draft.md @@ -33,6 +33,8 @@ THIS FILE ACCUMULATES THE RELEASE NOTES FOR THE UPCOMING RELEASE. * Deprecate `DELETE /conversations/:cnv/members/:usr` (#1697) * Add `DELETE /conversations/:cnv/members/:domain/:usr` (#1697) +* Add `POST /conversations/list/v2` (#1703) +* Deprecate `POST /list-conversations` (#1703) ## Features @@ -60,3 +62,4 @@ THIS FILE ACCUMULATES THE RELEASE NOTES FOR THE UPCOMING RELEASE. * Implemented full server-to-server authentication (#1687) * Add an endpoint for removing a qualified user from a local conversation (#1697) * The update conversation membership federation endpoint takes OriginDomainHeader (#1719) +* Added new endpoint to allow fetching conversation metadata by qualified ids (#1703) From b6c53866c5736ce2af0598afca1aa79f84bb88d8 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Thu, 19 Aug 2021 12:04:18 +0200 Subject: [PATCH 22/29] Remove TODO, in favour of solving it separately --- services/galley/src/Galley/API/Query.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index 92063e1e08..079f016f4c 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -100,7 +100,7 @@ getConversation zusr cnv = do localDomain <- viewFederationDomain if qDomain cnv == localDomain then getUnqualifiedConversation zusr (qUnqualified cnv) - else getRemoteConversation zusr (toRemote cnv) -- TODO: This looks wrong, we shouldn't get a remote conversation is the requesting user is not part of that conversation. + else getRemoteConversation zusr (toRemote cnv) getRemoteConversation :: UserId -> Remote ConvId -> Galley Public.Conversation getRemoteConversation zusr remoteConvId = do From 548499f0d1c60d15fc9156035b402169c7fcd8d0 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Thu, 19 Aug 2021 12:28:15 +0200 Subject: [PATCH 23/29] Extract helper for concurrent monadic partitioning --- services/galley/src/Galley/Data.hs | 33 ++++++++++++++++-------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/services/galley/src/Galley/Data.hs b/services/galley/src/Galley/Data.hs index ccd89bca8c..8e99343fae 100644 --- a/services/galley/src/Galley/Data.hs +++ b/services/galley/src/Galley/Data.hs @@ -586,31 +586,34 @@ conversationIdsOf usr cids = runIdentity <$$> retry x1 (query Cql.selectUserConv -- -- Returns @(foundConvs, notFoundConvs)@. localConversationIdsOf :: forall m. (MonadClient m, MonadUnliftIO m) => UserId -> [ConvId] -> m ([ConvId], [ConvId]) -localConversationIdsOf usr cids = do - partitionEithers <$> pooledMapConcurrentlyN 8 findConv cids +localConversationIdsOf usr = conncurrentPartitionM (pure . isJust <=< findConv) where - findConv :: ConvId -> m (Either ConvId ConvId) + findConv :: ConvId -> m (Maybe (Identity UserId)) findConv conv = do - mbMembership <- query1 Cql.selectUserConvMembership (params Quorum (usr, conv)) - pure $ case mbMembership of - Nothing -> Right conv - Just _ -> Left conv + query1 Cql.selectUserConvMembership (params Quorum (usr, conv)) -- | Takes a list of conversation ids and splits them by those found for the -- given user and those which are not found. -- -- Returns @(foundConvs, notFoundConvs)@. remoteConversationIdOf :: forall m. (MonadClient m, MonadLogger m, MonadUnliftIO m) => UserId -> [Remote ConvId] -> m ([Remote ConvId], [Remote ConvId]) -remoteConversationIdOf usr cids = - partitionEithers <$> pooledMapConcurrentlyN 8 findConv cids +remoteConversationIdOf usr = conncurrentPartitionM (pure . isJust <=< findRemoteConv) where - findConv :: Remote ConvId -> m (Either (Remote ConvId) (Remote ConvId)) - findConv remoteConvId = do + findRemoteConv :: Remote ConvId -> m (Maybe (Identity UserId)) + findRemoteConv remoteConvId = do let (Qualified conv domain) = unTagged remoteConvId - mbMembership <- query1 Cql.selectRemoteConvMembership (params Quorum (usr, domain, conv)) - pure $ case mbMembership of - Nothing -> Right remoteConvId - Just _ -> Left remoteConvId + query1 Cql.selectRemoteConvMembership (params Quorum (usr, domain, conv)) + +-- | Like 'Control.Monad.Extra.partitionM', but works concurrently in 8 threads. +conncurrentPartitionM :: forall m a. MonadUnliftIO m => (a -> m Bool) -> [a] -> m ([a], [a]) +conncurrentPartitionM predM xs = partitionEithers <$> pooledMapConcurrentlyN 8 bar xs + where + bar :: a -> m (Either a a) + bar x = do + matches <- predM x + pure $ if matches + then Left x + else Right x conversationsRemote :: (MonadClient m) => UserId -> m [Remote ConvId] conversationsRemote usr = do From ecb39e4d8ee4b914f26485eec458f64ac2623eb4 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Thu, 19 Aug 2021 12:36:21 +0200 Subject: [PATCH 24/29] Fix docs for the integration test --- services/galley/test/integration/API.hs | 34 ++++++++++--------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index a561e4d53d..4b9142902e 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -155,7 +155,7 @@ tests s = test s "get conversations/:domain/:cnv - local" testGetQualifiedLocalConv, test s "get conversations/:domain/:cnv - remote" testGetQualifiedRemoteConv, test s "post list-conversations" testListRemoteConvs, - test s "post conversations/list/v2 - success" testBulkGetQualifiedConvsSuccess, + test s "post conversations/list/v2" testBulkGetQualifiedConvs, test s "add non-existing remote members" testAddRemoteMemberFailure, test s "add deleted remote members" testAddDeletedRemoteUser, test s "add remote members on invalid domain" testAddRemoteMemberInvalidDomain, @@ -1933,31 +1933,27 @@ testListRemoteConvs = do -- | Tests getting many converations given their ids. -- --- In this test: +-- In this test, Alice is a local user, who will be asking for metadata of these +-- conversations: -- --- - Alice is a local user, who will be part of these convs and be asking for --- their metadata +-- - A local conversation which she is part of -- --- - Bob is on a.far-away.example.com, there is 1 conversation between Alice and --- Bob +-- - A remote conv on a.far-away.example.com (with Bob) -- --- - Carl is on b.far-away.example.com, there is 1 conversation between Alice --- and Carl +-- - A remote conv on b.far-away.example.com (with Carl) -- --- - Alice tries to get 1 conversation from a.far-away.example.com, but is not --- found in the local DB +-- - A remote conv on a.far-away.example.com, which is not found in the local DB -- --- - Alice tries to get 1 conversation from b.far-away.example.com, it is --- found in the local DB but remote does not return it +-- - A remote conv on b.far-away.example.com, it is found in the local DB but +-- the remote does not return it -- --- - Alice tries to get 1 conversation from c.far-away.example.com, but --- the federated call fails +-- - A remote conv on c.far-away.example.com, for which the federated call fails -- --- - Alice tries to get 1 local conversation which doesn't exist +-- - A local conversation which doesn't exist -- --- - Alice tries to get 1 local conersation which they're not part of -testBulkGetQualifiedConvsSuccess :: TestM () -testBulkGetQualifiedConvsSuccess = do +-- - A local conersation which they're not part of +testBulkGetQualifiedConvs :: TestM () +testBulkGetQualifiedConvs = do localDomain <- viewFederationDomain aliceQ <- randomQualifiedUser let alice = qUnqualified aliceQ @@ -1988,8 +1984,6 @@ testBulkGetQualifiedConvsSuccess = do registerRemoteConv remoteConvIdBNotFoundOnRemote carlQ Nothing (Set.fromList [aliceAsOtherMember]) registerRemoteConv remoteConvIdCFailure carlQ Nothing (Set.fromList [aliceAsOtherMember]) - -- FUTUREWORK: Do this test with more than one remote domains - -- test POST /list-conversations let aliceAsSelfMember = Member (qUnqualified aliceQ) Nothing False Nothing Nothing False Nothing False Nothing roleNameWireAdmin bobAsOtherMember = OtherMember bobQ Nothing roleNameWireAdmin carlAsOtherMember = OtherMember carlQ Nothing roleNameWireAdmin From 5184128519f95423afb7402cd83666a3480155ec Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Thu, 19 Aug 2021 14:37:41 +0200 Subject: [PATCH 25/29] Ormolu --- services/galley/src/Galley/Data.hs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/services/galley/src/Galley/Data.hs b/services/galley/src/Galley/Data.hs index 8e99343fae..4479b2f409 100644 --- a/services/galley/src/Galley/Data.hs +++ b/services/galley/src/Galley/Data.hs @@ -611,9 +611,10 @@ conncurrentPartitionM predM xs = partitionEithers <$> pooledMapConcurrentlyN 8 b bar :: a -> m (Either a a) bar x = do matches <- predM x - pure $ if matches - then Left x - else Right x + pure $ + if matches + then Left x + else Right x conversationsRemote :: (MonadClient m) => UserId -> m [Remote ConvId] conversationsRemote usr = do From 28e360bbcb16f0eac4bbc00e7c8b6222510a2007 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Mon, 30 Aug 2021 11:37:20 +0200 Subject: [PATCH 26/29] Simplify bulk verifying membership of a user in a list of convs --- services/galley/src/Galley/API/Query.hs | 9 +++- services/galley/src/Galley/Data.hs | 50 +++++++--------------- services/galley/src/Galley/Data/Queries.hs | 6 +-- 3 files changed, 26 insertions(+), 39 deletions(-) diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index 079f016f4c..d0cdf0bd8e 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -289,8 +289,8 @@ listConversationsV2 user (Public.ListConversationsV2 ids) = do localDomain <- viewFederationDomain let (remoteIds, localIds) = partitionRemoteOrLocalIds' localDomain (fromRange ids) - (foundLocalIds, notFoundLocalIds) <- Data.localConversationIdsOf user localIds - (foundRemoteIds, locallyNotFoundRemoteIds) <- Data.remoteConversationIdOf user remoteIds + (foundLocalIds, notFoundLocalIds) <- foundsAndNotFounds (Data.localConversationIdsOf user) localIds + (foundRemoteIds, locallyNotFoundRemoteIds) <- foundsAndNotFounds (Data.remoteConversationIdOf user) remoteIds localInternalConversations <- Data.conversations foundLocalIds @@ -324,6 +324,11 @@ listConversationsV2 user (Public.ListConversationsV2 ids) = do removeDeleted c | Data.isConvDeleted c = Data.deleteConversation (Data.convId c) >> pure False | otherwise = pure True + foundsAndNotFounds :: (Monad m, Eq a) => ([a] -> m [a]) -> [a] -> m ([a], [a]) + foundsAndNotFounds f xs = do + founds <- f xs + let notFounds = xs \\ founds + pure (founds, notFounds) iterateConversations :: forall a. UserId -> Range 1 500 Int32 -> ([Data.Conversation] -> Galley a) -> Galley [a] iterateConversations uid pageSize handleConvs = go Nothing diff --git a/services/galley/src/Galley/Data.hs b/services/galley/src/Galley/Data.hs index 4479b2f409..9e65672ea5 100644 --- a/services/galley/src/Galley/Data.hs +++ b/services/galley/src/Galley/Data.hs @@ -577,44 +577,26 @@ conversationIdRowsForPagination usr start (fromRange -> max) = Just c -> paginate Cql.selectUserConvsFrom (paramsP Quorum (usr, c) max) Nothing -> paginate Cql.selectUserConvs (paramsP Quorum (Identity usr) max) --- Deprecated +-- TODO: Delete conversationIdsOf :: (MonadClient m) => UserId -> [ConvId] -> m [ConvId] conversationIdsOf usr cids = runIdentity <$$> retry x1 (query Cql.selectUserConvsIn (params Quorum (usr, cids))) --- | Takes a list of conversation ids and splits them by those found for the --- given user and those which are not found. --- --- Returns @(foundConvs, notFoundConvs)@. -localConversationIdsOf :: forall m. (MonadClient m, MonadUnliftIO m) => UserId -> [ConvId] -> m ([ConvId], [ConvId]) -localConversationIdsOf usr = conncurrentPartitionM (pure . isJust <=< findConv) - where - findConv :: ConvId -> m (Maybe (Identity UserId)) - findConv conv = do - query1 Cql.selectUserConvMembership (params Quorum (usr, conv)) - --- | Takes a list of conversation ids and splits them by those found for the --- given user and those which are not found. --- --- Returns @(foundConvs, notFoundConvs)@. -remoteConversationIdOf :: forall m. (MonadClient m, MonadLogger m, MonadUnliftIO m) => UserId -> [Remote ConvId] -> m ([Remote ConvId], [Remote ConvId]) -remoteConversationIdOf usr = conncurrentPartitionM (pure . isJust <=< findRemoteConv) - where - findRemoteConv :: Remote ConvId -> m (Maybe (Identity UserId)) - findRemoteConv remoteConvId = do - let (Qualified conv domain) = unTagged remoteConvId - query1 Cql.selectRemoteConvMembership (params Quorum (usr, domain, conv)) - --- | Like 'Control.Monad.Extra.partitionM', but works concurrently in 8 threads. -conncurrentPartitionM :: forall m a. MonadUnliftIO m => (a -> m Bool) -> [a] -> m ([a], [a]) -conncurrentPartitionM predM xs = partitionEithers <$> pooledMapConcurrentlyN 8 bar xs +-- | Takes a list of conversation ids and returns those found for the given +-- user. +localConversationIdsOf :: forall m. (MonadClient m, MonadUnliftIO m) => UserId -> [ConvId] -> m [ConvId] +localConversationIdsOf usr cids = do + runIdentity <$$> retry x1 (query Cql.selectUserConvsIn (params Quorum (usr, cids))) + +-- | Takes a list of remote conversation ids and splits them by those found for +-- the given user +remoteConversationIdOf :: forall m. (MonadClient m, MonadLogger m, MonadUnliftIO m) => UserId -> [Remote ConvId] -> m [Remote ConvId] +remoteConversationIdOf usr cnvs = do + concat <$$> pooledMapConcurrentlyN 8 findRemoteConvs . Map.assocs . partitionQualified . map unTagged $ cnvs where - bar :: a -> m (Either a a) - bar x = do - matches <- predM x - pure $ - if matches - then Left x - else Right x + findRemoteConvs :: (Domain, [ConvId]) -> m [Remote ConvId] + findRemoteConvs (domain, remoteConvIds) = do + foundCnvs <- runIdentity <$$> query Cql.selectRemoteConvMembershipIn (params Quorum (usr, domain, remoteConvIds)) + pure $ toRemote . (`Qualified` domain) <$> foundCnvs conversationsRemote :: (MonadClient m) => UserId -> m [Remote ConvId] conversationsRemote usr = do diff --git a/services/galley/src/Galley/Data/Queries.hs b/services/galley/src/Galley/Data/Queries.hs index 106de5b697..7bf9ad8c92 100644 --- a/services/galley/src/Galley/Data/Queries.hs +++ b/services/galley/src/Galley/Data/Queries.hs @@ -240,9 +240,6 @@ deleteCode = "DELETE FROM conversation_codes WHERE key = ? AND scope = ?" selectUserConvs :: PrepQuery R (Identity UserId) (Identity ConvId) selectUserConvs = "select conv from user where user = ? order by conv" -selectUserConvMembership :: PrepQuery R (UserId, ConvId) (Identity UserId) -selectUserConvMembership = "select user from user where user = ? and conv = ?" - selectUserConvsIn :: PrepQuery R (UserId, [ConvId]) (Identity ConvId) selectUserConvsIn = "select conv from user where user = ? and conv in ? order by conv" @@ -313,6 +310,9 @@ selectUserRemoteConvs = "select conv_remote_domain, conv_remote_id from user_rem selectRemoteConvMembership :: PrepQuery R (UserId, Domain, ConvId) (Identity UserId) selectRemoteConvMembership = "select user from user_remote_conv where user = ? and conv_remote_domain = ? and conv_remote_id = ?" +selectRemoteConvMembershipIn :: PrepQuery R (UserId, Domain, [ConvId]) (Identity ConvId) +selectRemoteConvMembershipIn = "select conv_remote_id from user_remote_conv where user = ? and conv_remote_domain = ? and conv_remote_id in ?" + deleteUserRemoteConv :: PrepQuery W (UserId, Domain, ConvId) () deleteUserRemoteConv = "delete from user_remote_conv where user = ? and conv_remote_domain = ? and conv_remote_id = ?" From d39308ec87deebb25e168dfdaf5509e7cbcad6d0 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Mon, 30 Aug 2021 11:38:18 +0200 Subject: [PATCH 27/29] Replace conversationIdsOf with localConverstaionIdsOf --- services/galley/src/Galley/API/Query.hs | 4 ++-- services/galley/src/Galley/Data.hs | 5 ----- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index d0cdf0bd8e..8e841cc0d8 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -226,7 +226,7 @@ getConversationsInternal user mids mstart msize = do -- get ids and has_more flag getIds (Just ids) = (False,) - <$> Data.conversationIdsOf + <$> Data.localConversationIdsOf user (fromCommaSeparatedList (fromRange ids)) getIds Nothing = do @@ -271,7 +271,7 @@ listConversations user (Public.ListConversations mIds qstart msize) = do size = fromMaybe (toRange (Proxy @32)) msize getIdsAndMore :: [ConvId] -> Galley (Bool, [ConvId]) - getIdsAndMore ids = (False,) <$> Data.conversationIdsOf user ids + getIdsAndMore ids = (False,) <$> Data.localConversationIdsOf user ids getAll :: Maybe ConvId -> Galley (Bool, [ConvId]) getAll mstart = do diff --git a/services/galley/src/Galley/Data.hs b/services/galley/src/Galley/Data.hs index 9e65672ea5..217a6416d3 100644 --- a/services/galley/src/Galley/Data.hs +++ b/services/galley/src/Galley/Data.hs @@ -62,7 +62,6 @@ module Galley.Data remoteConversationIdOf, localConversationIdsPageFrom, conversationIdRowsForPagination, - conversationIdsOf, conversationMeta, conversations, conversationsRemote, @@ -577,10 +576,6 @@ conversationIdRowsForPagination usr start (fromRange -> max) = Just c -> paginate Cql.selectUserConvsFrom (paramsP Quorum (usr, c) max) Nothing -> paginate Cql.selectUserConvs (paramsP Quorum (Identity usr) max) --- TODO: Delete -conversationIdsOf :: (MonadClient m) => UserId -> [ConvId] -> m [ConvId] -conversationIdsOf usr cids = runIdentity <$$> retry x1 (query Cql.selectUserConvsIn (params Quorum (usr, cids))) - -- | Takes a list of conversation ids and returns those found for the given -- user. localConversationIdsOf :: forall m. (MonadClient m, MonadUnliftIO m) => UserId -> [ConvId] -> m [ConvId] From 8ef391d333eb35009161a643ddc70ff27f1e5ec6 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Mon, 30 Aug 2021 11:46:37 +0200 Subject: [PATCH 28/29] Apply suggestions from code review - Make log line a warn - Fix typo Co-authored-by: jschaul --- services/galley/src/Galley/API/Query.hs | 2 +- services/galley/test/integration/API.hs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index 8e841cc0d8..8d1e18a1d7 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -305,7 +305,7 @@ listConversationsV2 user (Public.ListConversationsV2 ids) = do -- FUTUREWORK: This implies that the backends are out of sync. Maybe the -- current user should be considered removed from this conversation at this -- point. - Logger.info $ + Logger.warn $ Logger.msg ("Some locally found conversation ids were not returned by remotes" :: ByteString) . Logger.field "convIds" (show remoteNotFoundRemoteIds) diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index 4b9142902e..f9502042fc 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -1951,7 +1951,7 @@ testListRemoteConvs = do -- -- - A local conversation which doesn't exist -- --- - A local conersation which they're not part of +-- - A local conversation which they're not part of testBulkGetQualifiedConvs :: TestM () testBulkGetQualifiedConvs = do localDomain <- viewFederationDomain From 4ebd9e2bfcaa2b9f1e7315c87c93223f8d109386 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Mon, 30 Aug 2021 11:52:48 +0200 Subject: [PATCH 29/29] Apply suggestions from code review - Fix typo - Better swagger Co-authored-by: jschaul --- libs/wire-api/src/Wire/API/Conversation.hs | 2 +- services/galley/src/Galley/API/Query.hs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/wire-api/src/Wire/API/Conversation.hs b/libs/wire-api/src/Wire/API/Conversation.hs index c90884cef2..9cc597a479 100644 --- a/libs/wire-api/src/Wire/API/Conversation.hs +++ b/libs/wire-api/src/Wire/API/Conversation.hs @@ -374,7 +374,7 @@ instance ToSchema ListConversationsV2 where schema = objectWithDocModifier "ListConversations" - (description ?~ "A request to list some of a user's conversations, including remote ones") + (description ?~ "A request to list some of a user's conversations, including remote ones. Maximum 1000 qualified conversation IDs") $ ListConversationsV2 <$> (fromRange . lcQualifiedIds) .= field "qualified_ids" (rangedSchema sing sing (array schema)) diff --git a/services/galley/src/Galley/API/Query.hs b/services/galley/src/Galley/API/Query.hs index 8d1e18a1d7..d084787854 100644 --- a/services/galley/src/Galley/API/Query.hs +++ b/services/galley/src/Galley/API/Query.hs @@ -139,7 +139,7 @@ getRemoteConversationsWithFailures zusr remoteConvs = do Right a -> pure $ Right a Left e -> do Logger.warn $ - Logger.msg ("Error occurred while fetch remote conversations" :: ByteString) + Logger.msg ("Error occurred while fetching remote conversations" :: ByteString) . Logger.field "error" (show e) pure . Left $ map (`Qualified` domain) convIds concatEithers :: (Monoid a, Monoid b) => [Either a b] -> (a, b)