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

Conflicting use of PublicRoomsChunk for space hierarchy over federation #1729

Closed
Kladki opened this issue Feb 24, 2024 · 3 comments · Fixed by #1740
Closed

Conflicting use of PublicRoomsChunk for space hierarchy over federation #1729

Kladki opened this issue Feb 24, 2024 · 3 comments · Fixed by #1740
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit

Comments

@Kladki
Copy link
Contributor

Kladki commented Feb 24, 2024

Link to problem area:
https://spec.matrix.org/v1.9/server-server-api/#get_matrixfederationv1hierarchyroomid

Issue
There are two different types with the name PublicRoomsChunk, with the only difference being that the latter has a children_state field which the other doesn't. In ruma, it is assumed that the former is for the children field, and the latter is for the room field. However, in the example request in the spec, both the children and room fields use the children_state field, so it is unclear whether the example is correct and they should both have the children_field, ruma is correct and only the room field should have that, or even maybe the children field should have that and the room field shouldn't.
Either way, they really should either be merged (for the former solution), or have different names (for the others).

@Kladki Kladki added the clarification An area where the expected behaviour is understood, but the spec could do with being more explicit label Feb 24, 2024
@deepbluev7
Copy link
Contributor

@zecakeh
Copy link
Contributor

zecakeh commented Feb 24, 2024

It seems like the types are indeed supposed to be different:

  1. MSC2946 says:

    children_state: As per Client-Server API version, though only on the room and not children.

  2. In the Synapse implementation, there is a comment that says explicitely that the children_state is not set on children items: https://github.com/element-hq/synapse/blob/274f289a52a0097784d7179c1d912dbc3cfda3ee/synapse/handlers/room_summary.py#L402-L403

@clokep
Copy link
Contributor

clokep commented Feb 27, 2024

There are two different types with the name PublicRoomsChunk

There's also a third use under the public rooms API, I don't know if this causes issues or not but is certainly confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants