Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add GET endpoint for subconversations #2869

Merged
merged 12 commits into from
Dec 14, 2022
Merged

Add GET endpoint for subconversations #2869

merged 12 commits into from
Dec 14, 2022

Conversation

smatting
Copy link
Contributor

@smatting smatting commented Nov 24, 2022 •
edited by mdimjasevic
Loading

This PR:

  • refactors MLS functions: use ConvOrSub instead of plain conversations
  • introduces a GET endpoint for subconversations

Tracked by https://wearezeta.atlassian.net/browse/FS-901.

Checklist

  • Add a test for initialGroupId
  • Add Stefan M. as a co-author in the merge commit
  • Address the TODO in Galley.API.Federation ("TODO: remove this before the rebase")
  • Write golden tests for the SubConvId and PublicSubConversation types
  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@smatting smatting temporarily deployed to cachix November 24, 2022 18:10 Inactive
@smatting smatting temporarily deployed to cachix November 24, 2022 18:10 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Nov 24, 2022
@smatting smatting force-pushed the fs-901/subconversation-endpoints branch from 60403da to 5b55ed1 Compare November 25, 2022 15:08
@smatting smatting temporarily deployed to cachix November 25, 2022 15:08 Inactive
@smatting smatting temporarily deployed to cachix November 25, 2022 15:08 Inactive
@mdimjasevic mdimjasevic force-pushed the fs-901/subconversation-endpoints branch from 6362cbd to 63f1f3c Compare November 30, 2022 12:15
@smatting smatting force-pushed the fs-901/subconversation-endpoints branch from 63f1f3c to 0ced46d Compare November 30, 2022 14:02
@mdimjasevic mdimjasevic force-pushed the fs-901/subconversation-endpoints branch from dab9fb5 to 5561966 Compare December 5, 2022 13:12
@mdimjasevic mdimjasevic temporarily deployed to cachix December 5, 2022 13:12 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 5, 2022 13:12 Inactive
@smatting smatting temporarily deployed to cachix December 5, 2022 17:22 Inactive
@smatting smatting temporarily deployed to cachix December 5, 2022 17:22 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 6, 2022 10:27 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 6, 2022 10:27 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 6, 2022 10:57 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 6, 2022 10:57 Inactive
@smatting smatting temporarily deployed to cachix December 6, 2022 11:55 Inactive
@smatting smatting temporarily deployed to cachix December 6, 2022 11:55 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 6, 2022 13:32 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 6, 2022 13:32 Inactive
@smatting smatting temporarily deployed to cachix December 6, 2022 16:12 Inactive
@smatting smatting temporarily deployed to cachix December 6, 2022 16:12 Inactive
@mdimjasevic mdimjasevic force-pushed the fs-901/subconversation-endpoints branch from d3273b1 to 035d5af Compare December 7, 2022 06:47
@mdimjasevic mdimjasevic temporarily deployed to cachix December 7, 2022 06:47 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 7, 2022 06:47 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 7, 2022 09:44 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 7, 2022 09:44 — with GitHub Actions Inactive
@smatting smatting force-pushed the fs-901/subconversation-endpoints branch from dd563a5 to 05db618 Compare December 12, 2022 11:55
@smatting smatting temporarily deployed to cachix December 12, 2022 11:55 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 13, 2022 21:24 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 13, 2022 21:24 — with GitHub Actions Inactive
mdimjasevic
mdimjasevic reviewed Dec 14, 2022
View reviewed changes
services/galley/src/Galley/API/MLS/Types.hs Outdated
GroupId
. convert
. Crypto.hash @ByteString @Crypto.SHA256
$ toByteString' (tUnqualified lcnv) <> toByteString' (unSubConvId sconv)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that two subconversations with the same ConvId and SubConvId, but that reside on different domains, have the same group ID. This could lead to various problems so let's make this depend on the domain too.

@mdimjasevic mdimjasevic temporarily deployed to cachix December 14, 2022 12:54 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 14, 2022 12:54 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 14, 2022 13:01 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 14, 2022 13:01 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 14, 2022 13:07 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 14, 2022 13:07 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic marked this pull request as ready for review December 14, 2022 13:07
@mdimjasevic mdimjasevic temporarily deployed to cachix December 14, 2022 13:09 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 14, 2022 13:09 — with GitHub Actions Inactive
elland
elland reviewed Dec 14, 2022
View reviewed changes
libs/wire-api/test/unit/Test/Wire/API/MLS/SubConversation.hs
injectiveInitialGroupId (cnv, scnv1, scnv2) = do
let domain = Domain "group.example.com"
lcnv = toLocalUnsafe domain cnv
initialGroupId lcnv scnv1 =/= initialGroupId lcnv scnv2
Copy link
Contributor

@elland elland Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests that they'll (probably) never overlap, but we could also test that the deterministic nature of the function also holds true.

initialGroupId lcnv scnv1 === initialGroupId lcnv scnv1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the way we have it now, i.e., without running in any monad that could leave room for non-determinism, we don't have to test that as all "pure" functions are deterministic. If we were to run it in some monad m we'd have to change the test anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough 👍

@mdimjasevic mdimjasevic temporarily deployed to cachix December 14, 2022 13:18 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 14, 2022 13:18 — with GitHub Actions Inactive
mdimjasevic
mdimjasevic reviewed Dec 14, 2022
View reviewed changes
services/galley/src/Galley/Cassandra/Queries.hs
Comment on lines +331 to +335
updateSubConvPublicGroupState :: PrepQuery W (ConvId, SubConvId, Maybe OpaquePublicGroupState) ()
updateSubConvPublicGroupState = "INSERT INTO subconversation (conv_id, subconv_id, public_group_state) VALUES (?, ?, ?)"

selectSubConvPublicGroupState :: PrepQuery R (ConvId, SubConvId) (Identity (Maybe OpaquePublicGroupState))
selectSubConvPublicGroupState = "SELECT public_group_state FROM subconversation WHERE conv_id = ? AND subconv_id = ?"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These don't really belong to this PR, but I won't insist on removing them from the PR as they'll be needed in the next PR anyway.

@mdimjasevic mdimjasevic merged commit 72baa93 into mls Dec 14, 2022
@mdimjasevic mdimjasevic deleted the fs-901/subconversation-endpoints branch December 14, 2022 14:59
@stefanwire stefanwire mentioned this pull request Jan 17, 2023
Merged
9 tasks
This was referenced Oct 19, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdimjasevic mdimjasevic mdimjasevic approved these changes

@elland elland elland left review comments

Assignees
No one assigned
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@smatting @mdimjasevic @elland @zebot