Conversation
flipcash2's chat_service.proto defines GetGroupChatFeed, StartChat, JoinChat, and LeaveChat, plus a RosterUpdate stream message for join/leave notifications, but the Kotlin service layer only had the DM chat path. Add the Api/Service/Repository/Controller methods for all four RPCs, following the existing getChat/getDmChatFeed pattern: request validation via protovalidate, Result mapping through foldWithSuppression, and owner resolution from UserManager.accountCluster. StartChat returns Result<ChatModel.Metadata> rather than a dedicated result wrapper, since that already matches getChat's single-object shape, and folds the TITLE_MODERATED case into a new StartChatError.TitleModerated carrying the flagged category. Add four sealed error types (GetGroupChatFeedError, StartChatError, JoinChatError, LeaveChatError), a RosterUpdate domain model (MemberJoined/MemberLeft) with mapping in ProtobufToLocal, and the ChatRules/BlobId-to-proto extensions StartChat's request needs in LocalToProtobuf. ChatUpdate gains a rosterUpdates list, populated from the proto's new roster_updates field; the event-stream consumer for it lands in a follow-up commit. ChatControllerTest's FakeChatRepository now implements the four new ChatRepository methods, with a test region per RPC mirroring the existing getChat/getDmChatFeed coverage (owner-resolution failure, argument forwarding, success, and failure passthrough).
ChatUpdate.rosterUpdates (added in the prior commit) reaches EventStreamDelegate.applyUpdate but nothing consumed it, so a member joining or leaving a group chat never touched Room: the roster changed server-side, but a client already displaying that chat wouldn't see membership change, and a client not yet in it wouldn't see the chat appear. Track each chat's last-applied RosterSummary.version in an in-memory map on the delegate rather than a Room column: the field's doc comment lays out why — chat_metadata has no version today, and treating this as the same kind of durable-but-not-authoritative signal as the event-sequence cursor would need a migration for a value that's cheap to reconstruct (a missed update self-heals via refetch, per the proto's own doc comment on RosterSummary). Applying an update whose version isn't greater than the tracked one is a no-op, so replays and reordered delivery converge. MemberJoined upserts the joining member, and additionally upserts chat metadata and its full member list when the update carries metadata — that only happens when the recipient is the one who joined, so this is how a newly-joined chat gets inserted into the local chat list without a separate feed refetch. MemberLeft deletes the chat's metadata and member rows when the leaving user is the caller (self-removal), otherwise just removes that one member row. Add roster coverage to ChatCoordinatorEventsTest: the version drop rule, recipient-join insert, non-recipient-join member-only upsert, recipient- leave chat removal, and non-recipient-leave member removal. Promote memberDataSource from a local mock in setUp() to a test-class field so these tests can coVerify against it, matching metadataDataSource and messageDataSource. ChatMemberDao/ChatMemberDataSource and ChatMetadataDao/ChatMetadataDataSource gain deleteMember/removeMember and deleteById/delete respectively, needed by the recipient-leave and non-recipient-leave paths above; neither existed before this since nothing previously deleted a single member row or a whole chat.
…mberLeft An unset RosterUpdate.kind is how a future oneof arm looks to an already-shipped client. Mapping it to a fabricated MemberLeft(userId = emptyList()) let EventStreamDelegate record that update's version as applied while doing nothing, permanently blocking the refetch the proto relies on to reconcile a version a client can't understand. toRosterUpdateOrNull now returns null for that case, and toChatUpdate drops it via mapNotNull before it reaches the delegate, so the version stays unadvanced until a reconcilable update or a roster refetch heals it.
Group chat RPCs and roster updates need the 0.7.0 contract. The version is not on Maven Central yet, so resolution fails until it publishes.
github-actions
Bot
added
type: feature
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the client side of the flipcash2 group chat contract, and pins
flipcash2-client-protocolto0.7.0.Blocked on code-payments/flipcash2-client-protocol#13.
0.7.0is not on Maven Central yet, so dependency resolution fails here until that PR merges andpublishes — expect
com.flipcash:flipcash2-client-protocol:0.7.0 FAILED. Draft until then. The workitself was written and built against the contract checkout through
protoLocalRoot, which CI neversees.
What this adds
Four RPCs through the existing Api → Service → Repository → Controller layering:
getGroupChatFeed,startChat,joinChat,leaveChat, each with a sealed error type mapping theresult enum case by case.
startChatsurfaces the server's moderation category asStartChatError.TitleModerated(flaggedCategory), reusingModerationResult.FlaggedCategoryratherthan introducing a second enum for the same values.
Roster updates — a domain
RosterUpdate(MemberJoined/MemberLeft),ChatUpdate.rosterUpdates,and the mapping for both.
EventStreamDelegateapplies them per chat gated onRosterSummary.version, dropping anything not strictly greater than the version it holds. A memberjoining or leaving updates the member rows; the recipient joining inserts the chat from the metadata
snapshot the update carries, and the recipient leaving removes it. That last case needed a per-chat
delete on
ChatMetadataDaoand a per-member delete onChatMemberDao, which did not exist.ChatUpdate.new_messagesmoving toreservedneeds nothing here — the last reference went in #1441.Two things worth a reviewer's attention
Roster versions are tracked in memory, not in Room.
EventStreamDelegateholds aMutableMap<ChatId, Long>.chat_metadatahas no roster-version column, so aChatMetadatarebuiltfrom Room already defaults to
RosterSummary(0, 0); tracking in memory matches that and avoids aschema migration. The cost is that a process restart re-applies the first roster update it sees per
chat, which is safe for a convergent overlay — real versions are always ≥ 1 — and the proto's own
contract is that an unreconcilable version heals via a roster refetch. A migration is the alternative
if durable versioning turns out to matter.
An unrecognized
RosterUpdate.kindis dropped, not defaulted.toRosterUpdateOrNullreturnsnull for an oneof arm this client does not know about, and the call site filters it with
mapNotNull,so it cannot reach the delegate and cannot advance the version tracker. Mapping it to a default would
record a version as applied while applying nothing, which would defeat the refetch reconciliation.
Tested
:services:flipcash:test(24) and:apps:flipcash:shared:chat:test(14, including five new rostertests) pass, and
:apps:flipcash:app:assembleDebugbuilds against the local contract checkout.