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

The transaction_id within events is not serialised in many endpoints #3000

Open
matrixbot opened this issue Nov 1, 2024 · 3 comments
Open
Labels
good first issue Good for newcomers O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience spec-compliance Fix something that doesn't comply with the specs T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@matrixbot
Copy link
Collaborator

This issue was originally created by @sandhose at matrix-org/dendrite#3000.

Similar to matrix-org/synapse#15173 in Synapse

There are many endpoints that return events, and in those representations they should include the transaction_id in the unsigned part of the event.

I wrote a Complement test to highlight that Synapse had this issue by testing using the /rooms/{roomId}/event/{eventId} endpoint, and Dendrite also fails on this test. I haven't tested the other endpoints, nor covered them in the Complement test. matrix-org/complement#621

@matrixbot matrixbot added good first issue Good for newcomers O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience spec-compliance Fix something that doesn't comply with the specs T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. labels Nov 1, 2024
@matrixbot
Copy link
Collaborator Author

This comment was originally posted by @abheekda1 at matrix-org/dendrite#3000 (comment).

I'd be interested in taking a shot at this, would it be possible for someone to point me in the right direction in terms of the serialization code and behavior?

@matrixbot
Copy link
Collaborator Author

This comment was originally posted by @S7evinK at matrix-org/dendrite#3000 (comment).

For a starting point: This method gets the events for e.g. /search or the mentioned /rooms/{roomId}/event/{eventId} but doesn't have a device parameter we could pass to StreamEventsToEvents which does the "magic" with transaction_id.

Almost the same is happening for /messages in handleNonEmptyEventsSlice, where we also don't pass a device to StreamEventsToEvents

/context seems to be a little bit trickier, as the database queries don't return StreamEvents.

@matrixbot
Copy link
Collaborator Author

This comment was originally posted by @tmills80 at matrix-org/dendrite#3000 (comment).

This doesn't seem to have any activity since March so I'm going to have a stab at it.
I've got the Complement test passing, but haven't checked any other endpoints yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience spec-compliance Fix something that doesn't comply with the specs T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

No branches or pull requests

1 participant