Skip to content
This repository has been archived by the owner on Nov 25, 2024. It is now read-only.

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

Open
sandhose opened this issue Mar 6, 2023 · 3 comments
Open
Labels
good first issue Want to help with Dendrite? These are the issues to start with! 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

@sandhose
Copy link
Member

sandhose commented Mar 6, 2023

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

@S7evinK S7evinK added good first issue Want to help with Dendrite? These are the issues to start with! spec-compliance Fix something that doesn't comply with the specs T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience labels Mar 7, 2023
@abheekda1
Copy link

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?

@S7evinK
Copy link
Contributor

S7evinK commented Mar 29, 2023

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.

@tmills80
Copy link

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Want to help with Dendrite? These are the issues to start with! 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

4 participants