Move search operations to UserSubsystem by akshaymankar · Pull Request #4188 · wireapp/wire-server

akshaymankar · 2024-08-06T09:48:18Z

https://wearezeta.atlassian.net/browse/WPB-8888

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

akshaymankar · 2024-08-20T08:13:02Z

libs/wire-subsystems/src/Wire/IndexedUserStore/ElasticSearch.hs

Not updating versions can cause issues while reindexing. This seems like an oversight, we should try to break it and create tickets accordingly.

This doesn't block this PR.

libs/wire-subsystems/src/Wire/UserSearchSubsystem/Interpreter.hs

libs/wire-subsystems/src/Wire/UserStore/Cassandra.hs

libs/wire-subsystems/src/Wire/UserSearch/Types.hs

services/brig/src/Brig/IO/Intra.hs

services/brig/src/Brig/User/Search/Index.hs

Pending: - Index Management - Search - Metrics - Update brig to use the new code (currently, brig is just broken)

Some metrics have been deleted as they are for bulk operations and there is no way for us to get those metrics because these operations don't run in an http request.

akshaymankar · 2024-09-17T14:50:37Z

libs/wire-api/src/Wire/API/Error/Brig.hs

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


What was the error before the refactoring?

"TODO: searcher doesn't exist" :)

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

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

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

akshaymankar · 2024-09-17T14:52:39Z

libs/wire-subsystems/src/Wire/FederationConfigStore.hs

-- TODO: This store effect is more than just a store, we should break it up in
-- business logic and store
+-- FUTUREWORK: This store effect is more than just a store,
+-- we should break it up in business logic and store


Is this still work for this ticket? We moved the code from brig to wire-subsystems in this PR so it might be confusing for someone to see that it wasn't made to fit the nomenclature here.

Is this still work for this ticket? We moved the code from brig to wire-subsystems in this PR so it might be confusing for someone to see that it wasn't made to fit the nomenclature here.

Is this a blocker for you or is a follow-up ticket OK?

Follow up is also ok 👍

mdimjasevic

This is just a partial review in case anyone has capacity to address my comments while I review the rest of the PR.

mdimjasevic · 2024-09-18T07:40:11Z

libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs

+  UpdateTeamSearchVisibilityInbound status ->
+    updateTeamSearchVisibilityInboundImpl status


Shouldn't this one be in a team effect?

This isn't a blocker for me.

mdimjasevic · 2024-09-18T07:40:31Z

libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs

+  BrowseTeam uid browseTeamFilters mMaxResults mPagingState ->
+    browseTeamImpl uid browseTeamFilters mMaxResults mPagingState


This one too sounds like it belongs to a team effect.

This isn't a blocker for me.

Makes sense, but I think this would make a very weird first thing to implement in a TeamSubsystem. Should we add a note here and move it later? Same for UpdateTeamSearchVisibilityInbound.

mdimjasevic · 2024-09-18T07:42:29Z

libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs

  guardLockedFields user updateOrigin update
  mapError (\StoredUserUpdateHandleExists -> UserSubsystemHandleExists) $
    updateUser uid (storedUserUpdate update)
+  let interestingToUpdateIndex = isJust update.name || isJust update.accentId


Why only these two are relevant here, and not others? Somewhere else we have a helper function that computes for an update if it is interesting. How do these two relate?

libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs

libs/wire-subsystems/test/unit/Wire/MockInterpreters/FederationConfigStore.hs

libs/wire-subsystems/test/unit/Wire/MockInterpreters/UserStore.hs

mdimjasevic · 2024-09-18T09:15:56Z

services/brig/src/Brig/API/Internal.hs

+  liftSem $
+    if success
+      then do
+        UserSubsystem.internalUpdateSearchIndex uid


Was something like this done in the version before this PR?

I think Intra.onUserEvent did this.

mdimjasevic · 2024-09-18T09:30:08Z

services/brig/src/Brig/API/User.hs

    wrapClient $ Data.insertAccount account Nothing pw False
    liftSem $ GalleyAPIAccess.createSelfConv uid
-    liftSem $ Intra.onUserEvent uid Nothing (UserCreated (accountUser account))
+    liftSem $ Events.generateUserEvent uid Nothing (UserCreated (accountUser account))


No need to update the search index here?

If it needed, why did we pull out this updating? It just opens room for errors without any clear benefits to me.

I think activateUser does that part.

The reason for pulling this out was that we were processing notifications meant for users as if they were intra backend communication which didn't seem very intuitive. Yes, this opens a room for bugs, but we have to trust our testing otherwise refactorings like this one would never happen.

services/brig/src/Brig/API/User.hs

mdimjasevic

There are still a few things left to address, and I've pointed to them in my earlier reviews. Overall, this looks good, but please don't merge before addressing the remaining issues.

services/brig/src/Brig/Team/API.hs

…bsystem

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 6, 2024

akshaymankar force-pushed the wpb-8888/search-subsystem branch 5 times, most recently from f8ceeb9 to d765d9a Compare August 12, 2024 14:36

akshaymankar force-pushed the wpb-8888/search-subsystem branch 7 times, most recently from 51287eb to 1273d16 Compare August 20, 2024 07:25