Skip to content

Federation: support read receipts #1801

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

Merged
merged 14 commits into from
Sep 28, 2021

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Sep 22, 2021

The initial purpose of this PR was to add federation behaviour for receipt mode update, which it does. However, in an effort to avoid copy-pasting code, it also unifies all the federation-aware conversation update code paths, and as a consequence the diff ended up being quite large.

The core of the unified interface is the updateLocalConversation function, which performs an update and notifies local and remote users.

Note: the remove member update is a bit special, because it was implemented using checked exceptions. To make it compatible with the other updates, checked exceptions are not currently used, although they are still present in some type signatures.

To make the unification possible, a number of other changes were necessary:

  • Removed checked add member wrapper. This wasn't either safe (because it didn't encompass all the necessary size checks) nor necessary (because the checks were already performed by ensureMemberLimit).
  • Aligned conversation update RPC to endpoint interface. The generalised RPC interface is not necessary for now (it can always be regeneralised later if needed), and making it the same as the public interface simplifies some things.
  • Separated self-member updates from other-member updates. These are really different anyway, and there was nothing gained by having a common interface.
  • Added some general code to deal with local and remote members uniformly. This simplifies many functions that can operate on both local and remote members. For example member removal, which is also used for leaving a remote conversation.
  • Introduced a UserList type, representing a list of users partitioned into locals and remotes.
  • Removed many of the convenience functions to add members to a conversation. The default admin role functionality is now implemented using a ToUserRole type class.
  • Event creation for adding and removing members or creating conversations is now done outside the Data module. This will make it possible to unify the event creation and propagation logic across many conversation endpoints.
  • Many functions operating on lists have been generalised to Foldable, so they can work uniformly with UserLists and normal lists.
  • The ConvSizeChecked newtype was broken, because the Functor instance would allow to break the invariant encoded by the type. This is now fixed, and the type can now wrap an arbitrary Foldable.
  • Support for creating managed conversations has been dropped

This is part of https://wearezeta.atlassian.net/browse/SQCORE-885 (federated conversation metadata updates).

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information:
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

@pcapriotti pcapriotti force-pushed the pcapriotti/propagate-read-receipts branch from 2497da0 to cf94042 Compare September 22, 2021 10:44
@pcapriotti pcapriotti changed the base branch from develop to pcapriotti/propagate-other-members September 22, 2021 10:44
Base automatically changed from pcapriotti/propagate-other-members to develop September 22, 2021 13:37
@pcapriotti pcapriotti force-pushed the pcapriotti/propagate-read-receipts branch 3 times, most recently from bfae6a1 to c4bb6c9 Compare September 24, 2021 08:52
@pcapriotti pcapriotti marked this pull request as ready for review September 24, 2021 10:43
This is a pretty invasive refactoring of code dealing with members of
conversations, especially in `Galley.Data`. Here is a summary of changes:

 - Introduced a `UserList` type, representing a list of users
 partitioned into locals and remotes.
 - Removed many of the convenience functions to add members to a
 conversation. The default admin role functionality is now implemented
 using a `ToUserRole` type class.
 - Event creation for adding members or creating conversations is now
 done outside the `Data` module. This will make it possible to unify the
 event creation and propagation logic across many conversation
 endpoints.
 - Many functions operating on lists have been generalised to
 `Foldable`, so they can work uniformly with `UserList`s and normal
 lists.
 - The `ConvSizeChecked` newtype was broken, because the `Functor`
 instance would allow to break the invariant encoded by the type. This
 is now fixed, and the type can now wrap an arbitrary `Foldable`.
 - Similar changes have been applied to `ConvMemberAddSizeChecked`.
 - Support for creating managed conversations has been dropped
This further separates the self member update code path from the other
member update. There was really no reason to have them together in the
first place.
The core of the unified interface is the `updateLocalConversation` function,
which performs an update and notifies local and remote users.

**Note**: the remove member update is a bit special, because it was implemented
using checked exceptions. To make it compatible with the other updates, checked
exceptions are not currently used, although they are still present in some type
signatures.

To make the unification possible, a number of other changes were necessary:

 - Removed checked add member wrapper. This wasn't either safe (because it
 didn't encompass all the necessary size checks) nor necessary (because the
 checks were already performed by `ensureMemberLimit`).
 - Aligned conversation update RPC to endpoint interface. The generalised RPC
 interface is not necessary for now (it can always be regeneralised later if
 needed), and making it the same as the public interface simplifies some
 things.
 - Added some general code to deal with local and remote members uniformly.
 This simplifies many functions that can operate on both local and remote
 members. For example member removal, which is also used for leaving a remote
 conversation.
A leave action is a special case of a member remove action, but it has a
different "tag", so now `conversationActionTag` takes the originating
user as an extra argument, and retuns a `LeaveConversation` tag for
`ConversationActionRemoveMember` actions that remove the originating
user.

This also fixes legalhold-related failures when non-consenting users are
removed after legal hold is enabled.
Creating managed conversations is not supported anymore
@pcapriotti pcapriotti force-pushed the pcapriotti/propagate-read-receipts branch from 8249bc8 to edb1772 Compare September 24, 2021 14:44
@fisx
Copy link
Contributor

fisx commented Sep 24, 2021

and as a consequence the diff ended up being quite large.

My first thought was "that should have been split up, then!". My second, 200ms later: "this keeps happening to me, too, why is this so hard?!" :)

@pcapriotti
Copy link
Contributor Author

and as a consequence the diff ended up being quite large.

My first thought was "that should have been split up, then!". My second, 200ms later: "this keeps happening to me, too, why is this so hard?!" :)

It requires a lot of discipline to keep changes separated when refactoring things on a larger scale, and I don't always have it. It wouldn't be impossible to carefully split the changes into meaningful commits after the fact, but it would take considerable time, and I thought it might be better spent for other things.

@pcapriotti pcapriotti merged commit 3aab0a4 into develop Sep 28, 2021
@pcapriotti pcapriotti deleted the pcapriotti/propagate-read-receipts branch September 28, 2021 09:55
pcapriotti added a commit that referenced this pull request Sep 29, 2021
Before, we were checking whether the number of people added to a
conversation was strictly less than the limit - 1, which was causing
conversation with maximum size to be rejected.

This was introduced by the refactoring in #1801.
@pcapriotti pcapriotti mentioned this pull request Sep 29, 2021
2 tasks
mdimjasevic pushed a commit that referenced this pull request Sep 29, 2021
* Fix check on conversation size

Before, we were checking whether the number of people added to a
conversation was strictly less than the limit - 1, which was causing
conversation with maximum size to be rejected.

This was introduced by the refactoring in #1801.

* Add test about conversation size limit

Added a test checking that creating conversations of exactly the size
limit is allowed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants