FS-1467: Make conversation metadata APIs fault tolerant to federation errors by lepsa · Pull Request #3229 · wireapp/wire-server

lepsa · 2023-04-13T08:23:38Z

Checklist

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

Changes

Making conversation metadata APIs fault tolerant of unavailable federation servers.
Adding new tests to check that we get the expected results when federation servers are unavailable.

lepsa · 2023-04-13T08:24:18Z

@mdimjasevic Can you have a look at this for me.

elland · 2023-04-13T09:11:14Z

services/galley/src/Galley/API/Action.hs

  notifyConversationAction
-    -- Removing members should be fault tolerant.
    ( case tag of
+        -- Removing members should be fault tolerant.


Should've brought this up before, but I'm feeling a case of boolean blindness here. Could we use a more descriptive type? 🤞

Fair enough, I've swapped it for a new type with better names.

elland · 2023-04-17T06:51:23Z

services/galley/src/Galley/API/Action.hs

  let action = ConversationJoin neUsers role
  pure (bmFromMembers lmems rmems, action)

+data FederationFailEarly


mdimjasevic

Looks good! I believe you forgot to invoke those tests in Federation.hs so please double-check that before merging.

mdimjasevic · 2023-04-19T09:05:31Z

services/galley/test/integration/API.hs

+        <!! const 200 === statusCode
+  liftIO $ map omQualifiedId (cmOthers (cnvMembers conv2)) @?= [bob]
+
+-- @END


Please remove this comment tag as it will otherwise mess up a test report the QA team generates. Conversely, you can add its counterpart before the test to make it a whole, but I'm not sure what should go in there.

mdimjasevic · 2023-04-19T09:10:46Z

services/galley/test/integration/API/Federation.hs

+      mkMember quid = OtherMember quid Nothing roleNameWireMember
+  fedGalleyClient <- view tsFedGalleyClient
+
+  mapM_ (`connectWithRemoteUser` qbob) [alice]


Isn't this a strange way of saying connectWithRemoteUser alice qbob?

mdimjasevic · 2023-04-19T09:16:02Z

services/galley/test/integration/API/Federation.hs

+notifyConvRenameUnavailable :: TestM ()
+notifyConvRenameUnavailable = do
+  let d = ConversationRename "gossip++"
+  notifyUpdateUnavailable [] (SomeConversationAction (sing @'ConversationRenameTag) d) ConvRename (EdConvRename d)
+
+notifyMessageTimerUnavailable :: TestM ()
+notifyMessageTimerUnavailable = do
+  let d = ConversationMessageTimerUpdate (Just 5000)
+  notifyUpdateUnavailable
+    []
+    (SomeConversationAction (sing @'ConversationMessageTimerUpdateTag) d)
+    ConvMessageTimerUpdate
+    (EdConvMessageTimerUpdate d)
+
+notifyReceiptModeUnavailable :: TestM ()
+notifyReceiptModeUnavailable = do
+  let d = ConversationReceiptModeUpdate (ReceiptMode 42)
+  notifyUpdateUnavailable
+    []
+    (SomeConversationAction (sing @'ConversationReceiptModeUpdateTag) d)
+    ConvReceiptModeUpdate
+    (EdConvReceiptModeUpdate d)
+
+notifyAccessUnavailable :: TestM ()
+notifyAccessUnavailable = do
+  let d = ConversationAccessData (Set.fromList [InviteAccess, LinkAccess]) (Set.fromList [TeamMemberAccessRole])
+  notifyUpdateUnavailable
+    []
+    (SomeConversationAction (sing @'ConversationAccessDataTag) d)
+    ConvAccessUpdate
+    (EdConvAccessUpdate d)


You forgot to list and therefore actually call these tests in the tests function on line 67, didn't you?

- Maybe it works now. Integration test SSL certificates expired and a merged PR from less than an hour ago fixed that

mdimjasevic · 2023-04-24T12:10:07Z

@lepsa , this still hasn't run in the CI so please don't merge just yet.

mdimjasevic · 2023-05-08T10:05:36Z

services/galley/test/integration/API/Federation.hs

+    ((), _fedRequests) <-
+      withTempMockFederator' (throw $ MockErrorResponse Http.status500 "Down for maintenance") $
+        runFedClient @"on-conversation-updated" fedGalleyClient bdom cu
+    putStrLn $ "on-conversation-updated: " <> show _fedRequests


Can you remove this debugging line?

mdimjasevic · 2023-05-09T07:11:43Z

@lepsa , before merging there are conflicts to be resolved in these files:

services/galley/src/Galley/API/Action.hs
services/galley/src/Galley/API/Federation.hs
services/galley/src/Galley/API/Update.hs

mdimjasevic · 2023-05-09T09:32:47Z

@lepsa , before merging there are conflicts to be resolved in these files:
services/galley/src/Galley/API/Action.hs
services/galley/src/Galley/API/Federation.hs
services/galley/src/Galley/API/Update.hs 

I started resolving these.

lepsa added 7 commits March 10, 2023 14:19

FS-1467: Updates to allow partial success and tests

3a4940c

Merge remote-tracking branch 'origin/develop' into FS-1467

79f8ebb

Merge remote-tracking branch 'origin/develop' into FS-1467

fde2821

FS-1467 Setting which conversation actions should be fault tolerant

70df213

FS-1467: Fixing tests for unavailable federator

aafcba0

FS-1467: Formatting and linting

1796f66

FS-1467: Adding changelog

4e09baf

elland reviewed Apr 13, 2023

View reviewed changes

FS-1467: Swapping out a bool for a more descriptive type

7b6ac1d

elland reviewed Apr 17, 2023

View reviewed changes

mdimjasevic approved these changes Apr 19, 2023

View reviewed changes

FS-1467: Updates from PR comments

1d13864

mdimjasevic approved these changes Apr 24, 2023

View reviewed changes

lepsa and others added 2 commits April 24, 2023 19:00

Empty commit, rerun CI

0739e55

Another Hi, CI

b2233d9

- Maybe it works now. Integration test SSL certificates expired and a merged PR from less than an hour ago fixed that

HI CI

ea5fbd5

elland added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label May 8, 2023

Hi CI now that ok-to-test is in place

0985a71

mdimjasevic reviewed May 8, 2023

View reviewed changes

Remove a debugging statement

5300181

mdimjasevic approved these changes May 9, 2023

View reviewed changes

Marko Dimjašević added 2 commits May 9, 2023 11:35

Merge commit '3836eb0' into FS-1467

56a08c5

Hi CI

7ed327a

mdimjasevic merged commit 2ad4204 into wireapp:develop May 9, 2023

This was referenced May 9, 2023

FS-1467: Make conversation metadata APIs fault tolerant to federation errors #3279

Closed

FS-1467: Make conversation metadata APIs fault tolerant to federation errors #3280

Merged

smatting mentioned this pull request May 25, 2023

Merge develop into mls-olaf #3317

Merged

zebot mentioned this pull request Aug 11, 2023

Release 2023-08-11 - (expected chart version 4.36.0) #3493

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

FS-1467: Make conversation metadata APIs fault tolerant to federation errors#3229

FS-1467: Make conversation metadata APIs fault tolerant to federation errors#3229
mdimjasevic merged 16 commits intowireapp:developfrom
lepsa:FS-1467

lepsa commented Apr 13, 2023

Uh oh!

lepsa commented Apr 13, 2023

Uh oh!

elland Apr 13, 2023

Uh oh!

lepsa Apr 14, 2023

Uh oh!

elland Apr 17, 2023

Uh oh!

mdimjasevic left a comment

Uh oh!

mdimjasevic Apr 19, 2023

Uh oh!

mdimjasevic Apr 19, 2023

Uh oh!

mdimjasevic Apr 19, 2023

Uh oh!

mdimjasevic commented Apr 24, 2023

Uh oh!

mdimjasevic May 8, 2023

Uh oh!

mdimjasevic commented May 9, 2023 •

edited

Loading

Uh oh!

mdimjasevic commented May 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

lepsa commented Apr 13, 2023

Checklist

Changes

Uh oh!

lepsa commented Apr 13, 2023

Uh oh!

elland Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

lepsa Apr 14, 2023

Choose a reason for hiding this comment

Uh oh!

elland Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

mdimjasevic left a comment

Choose a reason for hiding this comment

Uh oh!

mdimjasevic Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

mdimjasevic Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

mdimjasevic Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

mdimjasevic commented Apr 24, 2023

Uh oh!

mdimjasevic May 8, 2023

Choose a reason for hiding this comment

Uh oh!

mdimjasevic commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdimjasevic commented May 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mdimjasevic commented May 9, 2023 •

edited

Loading