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

Clarify that lazy_load_members in timeline isn't respected in /sync #2010

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bradtgmurray
Copy link
Contributor

@bradtgmurray bradtgmurray commented Nov 20, 2024

This one caught me out today, clarify that sync only uses lazy_load_members from state.

https://github.com/element-hq/synapse/blob/develop/synapse/api/filtering.py#L240-L241

Pull Request Checklist

Signed-off-by: Brad Murray [email protected]

Preview: https://pr2010--matrix-spec-previews.netlify.app

@bradtgmurray bradtgmurray requested a review from a team as a code owner November 20, 2024 20:31
@@ -28,15 +28,17 @@ allOf:
description: |-
If `true`, enables lazy-loading of membership events. See
[Lazy-loading room members](/client-server-api/#lazy-loading-room-members)
for more information. Defaults to `false`.
for more information. Defaults to `false`. Only applies to the `/messages`
Copy link
Contributor

Choose a reason for hiding this comment

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

It should also apply to the /context endpoint, as listed in the "lazy-loading room members" section. I believe it would be better to only list exceptions here.

@turt2live
Copy link
Member

Are we sure this isn't a synapse bug?

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.

Are we sure this isn't a synapse bug?

It seems to be part of the spec: https://spec.matrix.org/v1.12/client-server-api/#get_matrixclientv3sync says:

Lazy-loading members is only supported on a StateFilter for this endpoint

That said, I'm not convinced this is the clearest way to fix that confusion. For one thing, I don't know why we treat StateFilter and RoomEventFilter as different types, when they have exactly the same fields: I have opened #2015 to fix this.

I think probably we want words like:

Note that /sync only supports lazy-loading members on the state part of a RoomFilter.

Also, please use hyperlinks when referring to endpoints, other type definitions, etc.

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.

4 participants