Skip to content

Work around imprecisions in ES result ordering #1776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pcapriotti wants to merge 3 commits into from
Closed

Work around imprecisions in ES result ordering #1776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
67228ac
Work around imprecisions in ES result ordering
pcapriotti Sep 16, 2021
4daa80d
fixup! Work around imprecisions in ES result ordering
pcapriotti Sep 16, 2021
4d7ec3a
remove redundant import
jschaul Sep 16, 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
1 change: 1 addition & 0 deletions changelog.d/3-bug-fixes/search-order-fix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make searches more consistenty return exact name matches first
2 changes: 2 additions & 0 deletions libs/brig-types/src/Brig/Types/Search.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ module Brig.Types.Search
where

import Data.Id (TeamId)
import Imports
import Wire.API.User.Search

data TeamSearchInfo
Expand All @@ -38,3 +39,4 @@ data TeamSearchInfo
TeamOnly TeamId
| -- | When searching user is part of a team and 'Brig.Options.setSearchSameTeamOnly' is False
TeamAndNonMembers TeamId
deriving (Show)
51 changes: 17 additions & 34 deletions services/brig/src/Brig/User/API/Search.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,12 @@ import qualified Brig.IO.Intra as Intra
import qualified Brig.Options as Opts
import Brig.Team.Util (ensurePermissions)
import Brig.Types.Search as Search
import Brig.User.API.Handle (contactFromProfile)
import qualified Brig.User.API.Handle as HandleAPI
import Brig.User.Search.Index
import qualified Brig.User.Search.SearchIndex as Q
import Brig.User.Search.TeamUserSearch (RoleFilter (..), TeamUserSearchSortBy (..), TeamUserSearchSortOrder (..))
import qualified Brig.User.Search.TeamUserSearch as Q
import Control.Lens (view)
import Data.Domain (Domain)
import Data.Handle (parseHandle)
import Data.Id
import Data.Predicate
import Data.Range
Expand Down Expand Up @@ -150,23 +147,10 @@ searchLocally searcherId searchTerm maybeMaxResults = do
let maxResults = maybe 15 (fromIntegral . fromRange) maybeMaxResults
teamSearchInfo <- mkTeamSearchInfo

maybeExactHandleMatch <- exactHandleSearch teamSearchInfo
Copy link
Member

@jschaul jschaul Sep 16, 2021

Choose a reason for hiding this comment

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

There is a problem here: whether or not results are returned for users depends on the searchable flag (team users are by default not searchable by prefix search, but only by exact-handle-in-cassandra-search). The changeset here by no longer doing a request to cassandra omits those results entirely. That might at least partially explain the test failures.

The logic is a bit weird and grew as more requirements were tacked on to the product.

esResult <- Q.searchIndex searcherId teamSearchInfo searchTerm maxResults

let exactHandleMatchCount = length maybeExactHandleMatch
esMaxResults = maxResults - exactHandleMatchCount

esResult <-
if esMaxResults > 0
then Q.searchIndex searcherId teamSearchInfo searchTerm esMaxResults
else pure $ SearchResult 0 0 0 []

-- Prepend results matching exact handle and results from ES.
pure $
esResult
{ searchResults = maybeToList maybeExactHandleMatch <> searchResults esResult,
searchFound = exactHandleMatchCount + searchFound esResult,
searchReturned = exactHandleMatchCount + searchReturned esResult
}
-- Move exact matches to the beginning of the list
pure $ esResult {searchResults = sortResults teamSearchInfo (searchResults esResult)}
where
handleTeamVisibility :: TeamId -> TeamSearchVisibility -> Search.TeamSearchInfo
handleTeamVisibility t Team.SearchVisibilityStandard = Search.TeamAndNonMembers t
Expand All @@ -186,21 +170,20 @@ searchLocally searcherId searchTerm maybeMaxResults = do
-- For team users, we need to check the visibility flag
handleTeamVisibility t <$> Intra.getTeamSearchVisibility t

exactHandleSearch :: TeamSearchInfo -> Handler (Maybe Contact)
exactHandleSearch teamSearchInfo = do
let searchedHandleMaybe = parseHandle searchTerm
exactHandleResult <-
case searchedHandleMaybe of
Nothing -> pure Nothing
Just searchedHandle ->
contactFromProfile
<$$> HandleAPI.getLocalHandleInfo searcherId searchedHandle
pure $ case teamSearchInfo of
Search.TeamOnly t ->
if Just t == (contactTeam =<< exactHandleResult)
then exactHandleResult
else Nothing
_ -> exactHandleResult
sortResults :: TeamSearchInfo -> [Public.Contact] -> [Public.Contact]
sortResults tinfo = uncurry (<>) . partition (isExactMatch tinfo)

-- A match is considered "exact" if the name or handle are the same as the
-- search term.
-- However, for team searches, exact name matches for non-team members are
-- not considered exact
isExactMatch tinfo c =
isExactHandleMatch c || (isExactNameMatch c && includeExact tinfo c)
includeExact (Search.TeamOnly t) c = contactTeam c == Just t
includeExact (Search.TeamAndNonMembers t) c = contactTeam c == Just t
includeExact _ _ = True
isExactHandleMatch c = contactHandle c == Just searchTerm
isExactNameMatch c = contactName c == searchTerm

teamUserSearchH ::
( JSON
Expand Down
4 changes: 1 addition & 3 deletions services/brig/src/Brig/User/Search/SearchIndex.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,7 @@ defaultUserQuery u teamSearchInfo (normalized -> term') =
{ ES.boolQueryMustMatch =
[ ES.QueryBoolQuery
boolQuery
{ ES.boolQueryShouldMatch = [matchPhraseOrPrefix, legacyPrefixMatch],
-- This removes exact handle matches, as they are fetched from cassandra
ES.boolQueryMustNotMatch = [termQ "handle" term']
{ ES.boolQueryShouldMatch = [matchPhraseOrPrefix, legacyPrefixMatch]
}
],
ES.boolQueryShouldMatch = [ES.QueryExistsQuery (ES.FieldName "handle")]
Expand Down