-
Notifications
You must be signed in to change notification settings - Fork 334
WPB-21366: Add user type field to GET /search/contacts
#4913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WPB-21366: Add user type field to GET /search/contacts
#4913
Conversation
fbd0b50 to
c76d34c
Compare
5ec54b8 to
83fcf33
Compare
f5501ad to
38aa39c
Compare
b9b0d18 to
8fe483f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a type field to the Contact data type returned by GET /search/contacts, enabling clients to distinguish between regular users, bots, and apps in search results. The implementation extends Elasticsearch indexing to store and query user types, and updates all profile creation and search logic to propagate this information.
Changes:
- Added
typefield toContactandUserDocdata types with corresponding JSON serialization - Updated
mkUserProfileandmkUserProfileWithEmailfunctions to accept and useUserTypeparameter - Implemented
getUserTypehelper function to determine user type based on serviceId and apps table lookup - Updated all test fixtures (golden tests, unit tests, integration tests) to include the new type field
Reviewed changes
Copilot reviewed 61 out of 63 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/wire-api/src/Wire/API/User/Search.hs | Added required contactType field to Contact data type |
| libs/wire-api/src/Wire/API/User.hs | Modified mkUserProfile functions to accept UserType parameter |
| libs/wire-subsystems/src/Wire/UserSearch/Types.hs | Added udType field to UserDoc and userDocToContact helper function |
| libs/wire-subsystems/src/Wire/UserStore/IndexUser.hs | Updated indexUserToDoc to accept and store user type |
| libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs | Implemented getUserType helper and integrated it into profile creation and search |
| libs/wire-subsystems/src/Wire/IndexedUserStore/Bulk/ElasticSearch.hs | Added user type determination during bulk indexing operations |
| services/brig/src/Brig/User/Search/Index.hs | Added type field to Elasticsearch mapping |
| services/brig/src/Brig/User/Search/SearchIndex.hs | Refactored search to use centralized userDocToContact function |
| services/brig/src/Brig/User/API/Handle.hs | Updated contactFromProfile to include type field |
| services/brig/src/Brig/Provider/API.hs | Fixed bot profile creation to use UserTypeBot |
| services/brig/src/Brig/Index/Eval.hs | Added UsageError handling and Hasql.Pool import |
| libs/wire-subsystems/test/unit/* | Updated all unit tests to provide UserType when creating profiles |
| libs/wire-api/test/golden/* | Updated all golden test fixtures with type field set to "regular", "bot", or "app" |
| integration/test/Test/Apps.hs | Added integration test verifying app users have type "app" in search results |
| changelog.d/1-api-changes/* | Added changelog entries documenting the new field and migration requirements |
| postgres-schema.sql | Updated PostgreSQL dump version number (17.6 → 17.7) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
libs/wire-subsystems/src/Wire/IndexedUserStore/Bulk/ElasticSearch.hs
Outdated
Show resolved
Hide resolved
libs/wire-subsystems/src/Wire/IndexedUserStore/Bulk/ElasticSearch.hs
Outdated
Show resolved
Hide resolved
libs/wire-subsystems/src/Wire/IndexedUserStore/Bulk/ElasticSearch.hs
Outdated
Show resolved
Hide resolved
- fix comment - add `getUserType` helper, use it
It needs to be constructed at sync time: it's a derived field (i.e, not a field on the user table in Cassandra).
make sanitize-pr
There is prior art to this with email visibility and role, we're just following the pattern. There might be a nicer way, though.
The code isn't a lot shorter this way, but there aren't two places that make the same debatable decision to lie about user types of bots before ES re-index.
this may be a problem, but it's unrelated; deferring to #4944
This way, the type field is already returned in the /search/contacts endpoint, but it will contain `regular` instead of `app` everywhere. The change to brig-index will happen in an upcoming PR.
This reverts commit 738c459.
the only debatable one is about performance in ES re-indexing, but there we already do the expensive thing for roles and team visibilities, so the effect on performance should be tolerable.
f8b342e to
1c097b1
Compare
libs/wire-subsystems/src/Wire/IndexedUserStore/Bulk/ElasticSearch.hs
Outdated
Show resolved
Hide resolved
libs/wire-subsystems/src/Wire/IndexedUserStore/Bulk/ElasticSearch.hs
Outdated
Show resolved
Hide resolved
libs/wire-subsystems/src/Wire/IndexedUserStore/Bulk/ElasticSearch.hs
Outdated
Show resolved
Hide resolved
libs/wire-subsystems/src/Wire/IndexedUserStore/Bulk/ElasticSearch.hs
Outdated
Show resolved
Hide resolved
(don't know how the "add get-app endpoint got here, but it doesn't belong.)
WPB-21366: [Backend] Add type field to search contacts endpoint response
This PR adds the type field to contacts retrieved from
GET /search/contacts. To achieve this it needs to send that field to Elastic Search as that's where the search happens. The challenge (sometimes) is in how to determine whether the user is an app or not, as that depends on whether there is an according row in the apps table for the user (i.e, it's a derived field and not a field on the user row).Leaving this as draft, as there is still work outstanding:
XXX, all these should be resolved before mergingState [StoredApp]there to theUserStoreinterpreter, as consulting the apps determines a user's typeUPDATE(fisx): this PR is done with one exception: brig-index wrongly initiates
typefield withregularinstead of app. to do better, it needs to talk to postgres, which isn't trivial. PR coming up!Checklist
changelog.d