Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
b96e168
[WIP] Create new endpoint to get paginated conv ids
akshaymankar Jul 28, 2021
b059cfa
Fix old test so it actaully asserts
akshaymankar Aug 2, 2021
157045e
Add failing test
akshaymankar Aug 2, 2021
4b096a1
Implement paginated get remote conversations
akshaymankar Aug 3, 2021
febb54b
Rename /convs/ids/v2 -> /convs/list-ids
akshaymankar Aug 3, 2021
076cacd
Ormolu
akshaymankar Aug 3, 2021
6cda742
Add haddocks for how the conversations are ordered
akshaymankar Aug 3, 2021
082f2e8
Rename functions for consistency
akshaymankar Aug 3, 2021
5888386
Fix comments
akshaymankar Aug 3, 2021
93d84fb
CHANGELOG
akshaymankar Aug 3, 2021
02a8ca3
Mark `GET /conversations/ids` as deprecated
akshaymankar Aug 3, 2021
1967334
Fix typo
akshaymankar Aug 4, 2021
3d91255
Remove redundant brackets
akshaymankar Aug 4, 2021
db741e7
Reduce duplication
akshaymankar Aug 4, 2021
064a3fc
Fix typo
akshaymankar Aug 4, 2021
351919f
Optimize connections
akshaymankar Aug 4, 2021
18ff99c
Optimize connections
akshaymankar Aug 4, 2021
5b769e0
Remove forgotten print
akshaymankar Aug 4, 2021
a0dd768
Use pagingState to page through qualified conv ids
akshaymankar Aug 9, 2021
1285218
Move code for paginateWithState to cassandra-util
akshaymankar Aug 9, 2021
3a70ff2
Ormolu
akshaymankar Aug 9, 2021
b5495a5
Add some docs and rename some vars
akshaymankar Aug 9, 2021
da349b6
Add haddocks for 'getChuckedConvs'
akshaymankar Aug 9, 2021
aef1401
Galley/int: Use new type to decode qualified conv id list
akshaymankar Aug 10, 2021
c138c4a
Fix typos, grammar mistakes, etc
akshaymankar Aug 10, 2021
4cb8cca
Make ConversationPagingState Opaque
akshaymankar Aug 10, 2021
007baff
Add golden tests
akshaymankar Aug 10, 2021
c003902
Merge branch 'develop' into akshaymankar/paginate-remote-convs
akshaymankar Aug 10, 2021
0810f89
Ormolu
akshaymankar Aug 10, 2021
8ca1d9b
Better function names
akshaymankar Aug 10, 2021
b66a276
Optimize number of DB calls
akshaymankar Aug 10, 2021
a5f79bd
Optimize type class constraints
akshaymankar Aug 10, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,17 @@

## API Changes

* Add `POST /conversations/list-ids` (#1686)
* Deprecate `GET /converstations/ids` (#1686)

## Features

## Bug fixes and other updates

## Federation changes (alpha feature, do not use yet)

* Add new API to list paginated qualified conversation ids (#1686)

## Documentation

* fix swagger: mark name in UserUpdate as optional (#1691)
Expand Down
27 changes: 26 additions & 1 deletion libs/wire-api/src/Wire/API/Conversation.hs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ module Wire.API.Conversation
ConversationCoverView (..),
ConversationList (..),
ListConversations (..),
GetPaginatedConversationIds (..),

-- * Conversation properties
Access (..),
Expand Down Expand Up @@ -79,7 +80,7 @@ import Data.List1
import Data.Misc
import Data.Proxy (Proxy (Proxy))
import Data.Qualified (Qualified (qUnqualified), deprecatedSchema)
import Data.Range (Range)
import Data.Range (Range, toRange)
import Data.Schema
import qualified Data.Set as Set
import Data.String.Conversions (cs)
Expand Down Expand Up @@ -225,6 +226,9 @@ instance ConversationListItem ConvId where
instance ConversationListItem Conversation where
convListItemName _ = "conversations"

instance ConversationListItem (Qualified ConvId) where
convListItemName _ = "qualified Conversation IDs"

instance (ConversationListItem a, S.ToSchema a) => S.ToSchema (ConversationList a) where
declareNamedSchema _ = do
listSchema <- S.declareSchemaRef (Proxy @[a])
Expand Down Expand Up @@ -252,6 +256,27 @@ instance FromJSON a => FromJSON (ConversationList a) where
<$> o A..: "conversations"
<*> o A..: "has_more"

data GetPaginatedConversationIds = GetPaginatedConversationIds
{ gpciStartingPoint :: Maybe (Qualified ConvId),
gpciSize :: Range 1 1000 Int32
}
deriving stock (Eq, Show, Generic)
deriving (FromJSON, ToJSON, S.ToSchema) via Schema GetPaginatedConversationIds

instance ToSchema GetPaginatedConversationIds where
schema =
let addStartingPointDoc =
description
?~ "starting conversation id, this conversation id will not be included in the response. \
\The conversations are ordered lexicographically by domain and then by id using UUID ordering."
addSizeDoc = description ?~ "optional, must be <= 1000, defaults to 1000."
in objectWithDocModifier
"GetPaginatedConversationIds"
(description ?~ "A request to list some or all of a user's conversation ids, including remote ones")
$ GetPaginatedConversationIds
<$> gpciStartingPoint .= optFieldWithDocModifier "starting_point" Nothing addStartingPointDoc schema
<*> 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
Expand Down
12 changes: 10 additions & 2 deletions libs/wire-api/src/Wire/API/Routes/Public/Galley.hs
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,9 @@ data Api routes = Api
:> Capture "cnv" ConvId
:> "roles"
:> Get '[Servant.JSON] Public.ConversationRolesList,
getConversationIds ::
listConversationIdsUnqualified ::
routes
:- Summary "Get all conversation IDs."
:- Summary "[deprecated] Get all local conversation IDs."
-- FUTUREWORK: add bounds to swagger schema for Range
:> ZUser
:> "conversations"
Expand All @@ -162,6 +162,14 @@ data Api routes = Api
"size"
(Range 1 1000 Int32)
:> Get '[Servant.JSON] (Public.ConversationList ConvId),
listConversationIds ::
routes
:- Summary "Get all conversation IDs."
:> ZUser
:> "conversations"
:> "list-ids"
:> ReqBody '[Servant.JSON] Public.GetPaginatedConversationIds
:> Post '[Servant.JSON] (Public.ConversationList (Qualified ConvId)),
getConversations ::
routes
:- Summary "Get all *local* conversations."
Expand Down
3 changes: 2 additions & 1 deletion services/galley/src/Galley/API/Public.hs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ servantSitemap =
{ GalleyAPI.getUnqualifiedConversation = Query.getUnqualifiedConversation,
GalleyAPI.getConversation = Query.getConversation,
GalleyAPI.getConversationRoles = Query.getConversationRoles,
GalleyAPI.getConversationIds = Query.getConversationIds,
GalleyAPI.listConversationIdsUnqualified = Query.listConversationIdsUnqualified,
GalleyAPI.listConversationIds = Query.listConversationIds,
GalleyAPI.getConversations = Query.getConversations,
GalleyAPI.getConversationByReusableCode = Query.getConversationByReusableCode,
GalleyAPI.listConversations = Query.listConversations,
Expand Down
44 changes: 41 additions & 3 deletions services/galley/src/Galley/API/Query.hs
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@
--
-- You should have received a copy of the GNU Affero General Public License along
-- with this program. If not, see <https://www.gnu.org/licenses/>.
{-# LANGUAGE RecordWildCards #-}

module Galley.API.Query
( getBotConversationH,
getUnqualifiedConversation,
getConversation,
getConversationRoles,
getConversationIds,
listConversationIdsUnqualified,
listConversationIds,
getConversations,
listConversations,
iterateConversations,
Expand Down Expand Up @@ -118,15 +120,51 @@ getConversationRoles zusr cnv = do
-- be merged with the team roles (if they exist)
pure $ Public.ConversationRolesList wireConvRoles

getConversationIds :: UserId -> Maybe ConvId -> Maybe (Range 1 1000 Int32) -> Galley (Public.ConversationList ConvId)
getConversationIds zusr start msize = do
listConversationIdsUnqualified :: UserId -> Maybe ConvId -> Maybe (Range 1 1000 Int32) -> Galley (Public.ConversationList ConvId)
listConversationIdsUnqualified zusr start msize = do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about converting this to use conversationIdsPageFrom, so we can remove the legacy functions and even the ResultSet type?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I like the new name, I don't know if we can easily remove the ResultSet type. I will look into it, but let's do it in the next PR?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

let size = fromMaybe (toRange (Proxy @1000)) msize
ids <- Data.conversationIdsFrom zusr start size
pure $
Public.ConversationList
(Data.resultSetResult ids)
(Data.resultSetType ids == Data.ResultSetTruncated)

-- | Lists conversation ids for the logged in user. The request can optionally
-- have a 'startingPoint', this can be used to list conversation ids in a
-- paginated way.
--
-- Pagination requires an order, in this case the order is defined as:
--
-- - First all the local conversations are listed ordered by their id
--
-- - After local conversations, remote conversations are listed ordered
-- - lexicographically by their domain and then by their id.
listConversationIds :: UserId -> Public.GetPaginatedConversationIds -> Galley (Public.ConversationList (Qualified ConvId))
listConversationIds zusr Public.GetPaginatedConversationIds {..} = do
localDomain <- viewFederationDomain
let mStartDomain = qDomain <$> gpciStartingPoint
case mStartDomain of
Nothing -> localsAndRemotes localDomain Nothing gpciSize
Just x | x == localDomain -> localsAndRemotes localDomain (qUnqualified <$> gpciStartingPoint) gpciSize
Just _ -> remotesOnly gpciStartingPoint $ fromRange gpciSize
Comment thread
akshaymankar marked this conversation as resolved.
Outdated
where
localsAndRemotes :: Domain -> Maybe ConvId -> Range 1 1000 Int32 -> Galley (ConversationList (Qualified ConvId))
localsAndRemotes localDomain conv size = do
localPage <- resultSetToConvList . fmap (`Qualified` localDomain) <$> Data.conversationIdsFrom zusr conv size
let remainingSize = fromRange size - fromIntegral (length (Public.convList localPage))
if Public.convHasMore localPage
then pure localPage
else do
remotePage <- remotesOnly Nothing remainingSize
pure $ remotePage {convList = Public.convList localPage <> Public.convList remotePage}

remotesOnly :: Maybe (Qualified ConvId) -> Int32 -> Galley (ConversationList (Qualified ConvId))
remotesOnly start size =
resultSetToConvList <$> Data.remoteConversationIdsFrom zusr start size

resultSetToConvList :: Data.ResultSet a -> ConversationList a
resultSetToConvList res = Public.ConversationList (Data.resultSetResult res) (Data.resultSetType res == Data.ResultSetTruncated)

getConversations :: UserId -> Maybe (Range 1 32 (CommaSeparatedList ConvId)) -> Maybe ConvId -> Maybe (Range 1 500 Int32) -> Galley (Public.ConversationList Public.Conversation)
getConversations user mids mstart msize = do
ConversationList cs more <- getConversationsInternal user mids mstart msize
Expand Down
25 changes: 25 additions & 0 deletions services/galley/src/Galley/Data.hs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ module Galley.Data
updateConversationMessageTimer,
deleteConversation,
lookupReceiptMode,
remoteConversationIdsFrom,

-- * Conversation Members
addMember,
Expand Down Expand Up @@ -551,6 +552,30 @@ conversationIdsFrom usr start (fromRange -> max) =
where
strip p = p {result = take (fromIntegral max) (result p)}

-- | When the 'start' parameter is set to 'Nothing', reads first 'max' records
-- from 'user_remote_converstaions' table.
--
-- Otherwise, reads 'max' records starting from the 'start' parameter. Doing
-- this is unfortunately not trivial, so this function first gets all the
-- conversations which match the domain and then if there is still space, gets
-- the conversations which have domain > domain of start.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I imagine this is necessary because there is no way in cassandra to say (domain, id) > (start_domain, start_id)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not exactly this, but there is a token function. But its behaviour depends on some Cassandra settings, I am going to try to use it and if it works, maybe explore how Cassandra will behave under different settings.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

From my reading so far token can only be used on "partitiion keys". Partition keys are keys which are listed in the first column of the primary key. Right now our primary key is (user, remote_conv_domain, remote_conv_id), so our primary key is just user. If we want this to work, I guess we'll have to change this to something more complex like ((user, remote_conv_domain, remote_conv_id), user). And then set the cluster ordering by user. This may allow us to write queries like WHERE token(user, remote_conv_domain, remote_conv_id) > (?, ?, ?) AND user = ?. Not sure what other problems this might cause, I am already thinking this might cause data for one user to be scattered across different nodes, which might not be ideal. I am gonna read more.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually, the partition key doesn't need to be unique, so we can even get rid of user from it, making our primary key to be ((remote_conv_domain, remote_conv_id), user)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But why not use pagingState instead and let the driver handle it? Or am I missing something?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But why not use pagingState instead and let the driver handle it? Or am I missing something?

I will try this next and see if it works.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This actually works 🎉

remoteConversationIdsFrom :: (MonadClient m, MonadLogger m) => UserId -> Maybe (Qualified ConvId) -> Int32 -> m (ResultSet (Qualified ConvId))
remoteConversationIdsFrom usr start max =
case start of
Just (Qualified c d) -> do
domainPage <- toResultSet max <$> paginate Cql.selectUserRemoteConvsForDomainFrom (paramsP Quorum (usr, d, c) (max + 1))
let remainingMax = max - fromIntegral (length (resultSetResult domainPage))
if resultSetType domainPage == ResultSetTruncated
then pure domainPage
else do
nextPage <- toResultSet remainingMax <$> paginate Cql.selectUserRemoteConvsFromDomain (paramsP Quorum (usr, d) (remainingMax + 1))
pure $ nextPage {resultSetResult = resultSetResult domainPage <> resultSetResult nextPage}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This idiom of using max + 1 then discarding the last element is used in multiple places, so it would be good to extract it in a convenience function.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is no longer required, so maybe we can just retire this thing soon.

Nothing ->
toResultSet max <$> paginate Cql.selectUserRemoteConvs (paramsP Quorum (Identity usr) (max + 1))
where
toResultSet max' = mkResultSet . strip max' . fmap (uncurry (flip Qualified))
strip max' p = p {result = take (fromIntegral max') (result p)}

conversationIdRowsForPagination :: MonadClient m => UserId -> Maybe ConvId -> Range 1 1000 Int32 -> m (Page ConvId)
conversationIdRowsForPagination usr start (fromRange -> max) =
runIdentity
Expand Down
6 changes: 6 additions & 0 deletions services/galley/src/Galley/Data/Queries.hs
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,12 @@ insertUserRemoteConv = "insert into user_remote_conv (user, conv_remote_domain,
selectUserRemoteConvs :: PrepQuery R (Identity UserId) (Domain, ConvId)
selectUserRemoteConvs = "select conv_remote_domain, conv_remote_id from user_remote_conv where user = ? order by conv_remote_domain"
Comment thread
mdimjasevic marked this conversation as resolved.

selectUserRemoteConvsForDomainFrom :: PrepQuery R (UserId, Domain, ConvId) (Domain, ConvId)
selectUserRemoteConvsForDomainFrom = "select conv_remote_domain, conv_remote_id from user_remote_conv where user = ? and conv_remote_domain = ? and conv_remote_id > ? order by conv_remote_domain"

selectUserRemoteConvsFromDomain :: PrepQuery R (UserId, Domain) (Domain, ConvId)
selectUserRemoteConvsFromDomain = "select conv_remote_domain, conv_remote_id from user_remote_conv where user = ? and conv_remote_domain > ? order by conv_remote_domain"

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 = ?"

Expand Down
Loading