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.

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

PDU Sender split #3100

Merged
merged 30 commits into from
Jun 6, 2023
Merged

PDU Sender split #3100

merged 30 commits into from
Jun 6, 2023

Conversation

devonh
Copy link
Collaborator

@devonh devonh commented Jun 3, 2023

Initial cut of splitting PDU Sender into SenderID & UserID

@devonh devonh requested a review from a team as a code owner June 3, 2023 02:12
@codecov
Copy link

codecov bot commented Jun 3, 2023

Codecov Report

Patch coverage: 45.34% and project coverage change: -0.22 ⚠️

Comparison is base (725ff55) 66.23% compared to head (051aa22) 66.01%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3100      +/-   ##
==========================================
- Coverage   66.23%   66.01%   -0.22%     
==========================================
  Files         500      500              
  Lines       53862    54127     +265     
==========================================
+ Hits        35677    35734      +57     
- Misses      14591    14795     +204     
- Partials     3594     3598       +4     
Flag Coverage Δ
unittests 50.07% <45.34%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
clientapi/routing/directory.go 48.05% <0.00%> (-6.18%) ⬇️
clientapi/routing/redaction.go 49.52% <0.00%> (-0.96%) ⬇️
clientapi/routing/sendevent.go 69.45% <0.00%> (-1.01%) ⬇️
federationapi/routing/invite.go 43.40% <0.00%> (-1.48%) ⬇️
federationapi/routing/join.go 48.82% <0.00%> (-4.78%) ⬇️
federationapi/routing/leave.go 42.90% <0.00%> (-1.21%) ⬇️
roomserver/api/alias.go 100.00% <ø> (ø)
roomserver/api/api.go 100.00% <ø> (ø)
roomserver/internal/input/input_missing.go 66.47% <0.00%> (-1.59%) ⬇️
roomserver/internal/perform/perform_admin.go 40.24% <0.00%> (-0.67%) ⬇️
... and 40 more

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

We need to be more rigorous to distinguish between event.UserID() and QueryUserIDForSender(event.SenderID()).

QueryUserIDForSender should do a DB hit to answer:

  • is this sender key mapped to a user ID at all? AND
  • is this user ID local? (aka do I have a private key for this sender key)

I am struggling to identify a legit use case for event.UserID() as I don't know how this field is actually getting set in GMSL (currently it just assumes sender ID is a spec.UserID).

My intuition here would be to:

  • remove event.UserID() entirely in GMSL.
  • Expect Dendrite to bounce via QueryUserIDForSender when it needs to map a SenderID to a UserID.

This keeps GMSL "pure" in that it dosn't need to care about "hey, I need to find the m.room.member event for this sender of this message event to serve queries for event.UserID()".

In the rare cases where GMSL needs the user ID for an event, we should interface up a function which Dendrite implements which ultimately calls QueryUserIDForSender.

userapi/consumers/roomserver.go Outdated Show resolved Hide resolved
userapi/consumers/roomserver.go Outdated Show resolved Hide resolved
userapi/consumers/roomserver_test.go Outdated Show resolved Hide resolved
syncapi/synctypes/clientevent.go Outdated Show resolved Hide resolved
syncapi/streams/stream_invite.go Outdated Show resolved Hide resolved
}
}

deviceSenderID, err := rsAPI.QuerySenderIDForUser(req.Context(), alias, *userID)
Copy link
Member

Choose a reason for hiding this comment

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

Do we expect this to create the sender ID if one does not exist? I guess in this case no, but in some cases yes?

roomserver/internal/input/input_events.go Outdated Show resolved Hide resolved
roomserver/internal/input/input_events.go Outdated Show resolved Hide resolved
roomserver/internal/input/input_events.go Outdated Show resolved Hide resolved
roomserver/internal/input/input_events.go Outdated Show resolved Hide resolved
@devonh devonh merged commit 7a1fd7f into main Jun 6, 2023
@devonh devonh deleted the sender-split branch June 6, 2023 20:55
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.

2 participants