[FS-1148] Better resilience to unreachable backends#3282
[FS-1148] Better resilience to unreachable backends#3282mdimjasevic merged 10 commits intodevelopfrom
Conversation
mdimjasevic
force-pushed
the
fs-1148/only-for-develop
branch
from
aee7c2e to
0fa66a3
Compare
mdimjasevic
changed the title
… (1/2) (#3248) * Refactoring: use FailedToProcess * Refactoring: make UnreachableUsers a NonEmpty As agreed with client devs on Apr 4, 2023 in the Squad - Federation chat, the absence of the `failed_to_send` field in response to an MLS message send request has the same meaning as an empty list provided in the same field. * executeProposalAction: return failed-to-add users * MLS test utility: reuse code among utilities * Move and generalise mockUnreachableFor * Introduce failed to remove (via failed to fetch client info) * Propagate FailedToProcess across federation API arising from conversation updates * Fix/align an MLS integration test * Use a V4 add members endpoint in tests * Rethrow the invalid-domain exception * Rethrow federation-not-available error * Fix a golden test for LeaveConversationResponse * Golden tests for MLSMessageSendingStatus * Fix a test with an unreachable user * Test: clean up debugging leftovers * Test utility: fix wording of a haddoc * Clean up conv action federation failure handling * Move unreachability stuff into its own module
... as much as possible. Further dropping of the changes is not doable.
mdimjasevic
force-pushed
the
fs-1148/only-for-develop
branch
from
0fbc4ea to
05bb7e3
Compare
mdimjasevic
requested review from
elland and
smatting
and removed request for
smatting
zebot
added
the
ok-to-test
mdimjasevic
force-pushed
the
fs-1148/only-for-develop
branch
2 times, most recently
from
e7c3d27 to
3b7e0b7
Compare
| data ConversationUpdateResponse | ||
| = ConversationUpdateResponseError GalleyError | ||
| | ConversationUpdateResponseUpdate ConversationUpdate | ||
| | ConversationUpdateResponseUpdate (ConversationUpdate, FailedToProcess) |
There was a problem hiding this comment.
Are you asking why not a record? Sure, I can put a record instead, I was just lazy with coming up with a name for it.
There was a problem hiding this comment.
No, I'm asking why the constructor doesn't simply take the two arguments separately.
| import qualified Data.Swagger as S | ||
| import Imports | ||
|
|
||
| newtype UnreachableUsers = UnreachableUsers {unreachableUsers :: NonEmpty (Qualified UserId)} |
There was a problem hiding this comment.
It seems UnreachableUsers only appears within Maybe. What's the point of making it a non-empty list if it needs to be wrapped with Maybe anyway? Note that Maybe (NonEmpty a) is isomorphic to [a].
There was a problem hiding this comment.
It's true they're isomorphic.
For the FailedToProcess record type, which has multiple fields of type Maybe UnreachableUsers, there is a ToSchema instance and JSON instances derived from it. Each such list is present in the output only if it is a Just. If each such list was a plain list instead and I wanted to have the same JSON instances, I'd have to do dynamic checks if it is an empty list or not.
This is nothing major, but I saw it convenient to do it this way.
4 participants
This is a follow-up to #3275.
A few federation endpoints have been updated to better support resilience to unreachable backends. This PR involves minimal changes to MLS modules. Change specific to MLS modules will be done in a follow-up PR once this PR has been merged to
develop.Tracked by https://wearezeta.atlassian.net/browse/FS-1148.
Checklist
changelog.d