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

Remove 'room_id' field from m.typing, m.receipt and m.fully_read examples and schema #3679

Merged
merged 5 commits into from Mar 1, 2022

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Jan 26, 2022

It was discovered here that the spec had an erroneous room_id field in a m.typing EDU entry of /sync. After some further research, room_id fields also appear in m.read receipts in /sync, and m.fully_read room account data objects in the spec. None of these are necessary nor used in practice.

Checking part of the ecosystem for whether clients look for, or homeservers include, these room_id fields, I found that:

  • Element does not require them, nor does Synapse include them.
  • Ruma does not include them.
  • Dendrite does not include them.
  • nheko/mtxclient does not look for them.

This change removes room_id from the example and OpenAPI schema in each case mentioned above. It only affects the Client-Server spec - the Server-Server spec text remains unchanged.

The field was initially introduced in 0f28f83#diff-8d917764f05b0823bc6aee1f9a74895d38df32fb1eeb10c18a55d9120cf23cb2R3.

Preview: https://pr3679--matrix-org-previews.netlify.app

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

This change feels wrong - a room_edu is in a room, so needs a room ID. Not all EDUs use a top-level room_id though, so possibly just a badly written example somewhere specific?

@richvdh
Copy link
Member

richvdh commented Jan 27, 2022

This change feels wrong - a room_edu is in a room, so needs a room ID.

I think the confusion here stems from the fact that typing events (and, for that matter, EDUs in general) have a different format when sent over the federation API (where they do have a room_id) than when sent in a /sync response (where the room is implied by the position in the /sync response).

I haven't checked, but I think this file is only used when rendering the /sync response, so is probably correct?

@anoadragon453
Copy link
Member Author

@richvdh I believe so, though CI seems to be unhappy.

We should perhaps rename this in some way so that it's explicitly client-specific?

@turt2live
Copy link
Member

The CI is failing because the schema and the example don't match - this PR only touches the example, not the schema.

@anoadragon453 anoadragon453 changed the title Remove 'room_id' field from EDU example Remove 'room_id' field from m.typing, m.receipt and m.fully_read examples and schema Feb 8, 2022
@anoadragon453 anoadragon453 requested review from turt2live and removed request for turt2live February 8, 2022 20:21
@anoadragon453
Copy link
Member Author

I have updated the schema, and CI now passes. I also discovered that m.fully_read is affected by this. So room_id has been removed in the appropriate places as well.

I've only been verifying against Synapse/Element here. I need to check other implementations to see if these fields are being relied upon.

@aaronraimist
Copy link
Contributor

This should fix #3183

@anoadragon453 anoadragon453 linked an issue Feb 9, 2022 that may be closed by this pull request
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

lgtm - just need a changelog to denote the bugfix in the spec prose

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm

@richvdh richvdh merged commit b26aa7f into main Mar 1, 2022
@anoadragon453 anoadragon453 deleted the anoa/edu_room_id branch March 2, 2022 16:51
RiotTranslateBot pushed a commit to RiotTranslateBot/matrix-doc that referenced this pull request Aug 22, 2023
…` examples and schema (matrix-org#3679)

The spec had an erroneous `room_id` field in a m.typing EDU entry of /sync, `m.read` receipts in `/sync`, and `m.fully_read` room account data objects in the spec. None of these are necessary nor used in practice.

Checking part of the ecosystem for whether clients look for, or homeservers include, these room_id fields, I found that:

    Element does not require them, nor does Synapse include them.
    Ruma does not include them.
    Dendrite does not include them.
    nheko/mtxclient does not look for them.

This change removes room_id from the example and OpenAPI schema in each case mentioned above. It only affects the Client-Server spec - the Server-Server spec text remains unchanged.

The field was initially introduced in 0f28f83.
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.

m.fully_read example is misleading
4 participants