Skip to content

Stop sending or processing the origin field in PDUs #18418

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

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented May 9, 2025

To align with this change in Matrix v1.12. Addresses part of #18118.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct
    (run the linters)

@github-actions github-actions bot deployed to PR Documentation Preview May 9, 2025 14:37 Active
@anoadragon453 anoadragon453 force-pushed the anoa/remove_origin_field branch from 0fd9fe7 to 2106d70 Compare May 9, 2025 14:40
@anoadragon453 anoadragon453 marked this pull request as ready for review May 9, 2025 14:40
@anoadragon453 anoadragon453 requested a review from a team as a code owner May 9, 2025 14:40
@@ -45,6 +45,8 @@ def make_graph(pdus: List[dict], filename_prefix: str) -> None:
colors = {"red", "green", "blue", "yellow", "purple"}

for pdu in pdus:
# TODO: The "origin" field have since been removed from events generated
# by Synapse.
origins.add(pdu.get("origin"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intention to actually eventually remove this or why is it retained? Is there a functional reason why it's still being retained?

Copy link
Member Author

Choose a reason for hiding this comment

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

The contrib/graph/graph.py is not a tool maintained by the core development team (as is anything else under contrib/), hence me not changing it.

But we'd accept a PR that updates the tool to remove that bit.

@3nprob
Copy link
Contributor

3nprob commented May 15, 2025

Bystander Q to understand intended scope and direction: Will the references to origin in PDU processing and storage f.e. in _process_incoming_pdus_in_room_inner also be intended legacy/deprecated/get removed or would that be unrelated to the changes here?

@anoadragon453
Copy link
Member Author

anoadragon453 commented May 16, 2025

Bystander Q to understand intended scope and direction: Will the references to origin in PDU processing and storage f.e. in _process_incoming_pdus_in_room_inner also be intended legacy/deprecated/get removed or would that be unrelated to the changes here?

That "origin" is referring to the origin field on PUT /_matrix/federation/v1/send/{txnId}, which is unrelated to this PR.

Also note the origin attribute of X-Matrix federation authentication headers, which too is separate.

@anoadragon453 anoadragon453 marked this pull request as draft May 16, 2025 10:49
@anoadragon453
Copy link
Member Author

Converting to draft while CI is still failing.

@3nprob
Copy link
Contributor

3nprob commented May 17, 2025

Bystander Q to understand intended scope and direction: Will the references to origin in PDU processing and storage f.e. in _process_incoming_pdus_in_room_inner also be intended legacy/deprecated/get removed or would that be unrelated to the changes here?

That "origin" is referring to the origin field on PUT /_matrix/federation/v1/send/{txnId}, which is unrelated to this PR.

Also note the origin attribute of X-Matrix federation authentication headers, which too is separate.

Thanks for clarifying!

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