Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC3266: Room summary API #3266
base: old_master
Are you sure you want to change the base?
MSC3266: Room summary API #3266
Changes from 26 commits
642f4e1
188b6e5
dc5b372
975ece5
d148acf
6776863
df376a3
43eecf0
66fee23
469b77b
04f807b
f1233c4
5fc2f5b
9e41b45
cab37e5
a93190f
8186b72
1a8ecff
82d8f3b
208a58c
33f3733
a5bc9ef
ac3d5da
dba6705
9719119
2ad832c
81fd904
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We appear to be missing error conditions from the MSC: what if the room doesn't exist? is invalid? unroutable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear if this includes invites. It probably should, but that should be explicitly mentioned in the MSC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, so this means being
knockable
is not sufficient to make the contents visible ( since a user who has not yet received an invite to a knockable room lacks the necessary permissions to join it).So this seems to be in conflict with the current Synapse implementation.
We may need to reach a decision on matrix-org/matrix-spec#1186 if we are to avoid the two endpoints having subtly different behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention was always to allow access to the same rooms the hierarchy API allows access to. I don't think we need to wait for a decision on those spec issues, since the behaviour should be the same in any case. I added the explicit mention of that intention back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This MSC is still unclear in what it wants, imo. If it just wants to use the same conditions as
/hierarchy
then it can just say that (independent of us fixing matrix-org/matrix-spec#1184). Currently, it forbids knockable rooms which appears to be an anti-goal of this MSC.Suggested phrasing:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, but can we call it
room_summary
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really care about the name, but all the other endpoints are called /join/ and such as well, so I thought /summary/ would be more consistent with that, since it is followed by a room id/alias.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that for
/join
and/knock
, it's a bit more obvious that we're talking about a room./summary
could be anything, really.I'd be interested what other people think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on it, so we can change to to whatever, but I would appreciate some more opinions since otherwise I might need to flip flop between names again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's call it
/room_summary
- there's a good chance we'll want other entities to be summarized in the future and it'd be annoying to fix/replace the API endpoint for them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vias feel insufficiently specified, despite I think everyone "knowing" what you mean:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec text for this same parameter (but with a different name) on the /join/ endpoint is this:
Why does an MSC need to be more specific then the spec? It is needed when otherwise the room can't be routed to. If that is the case, depends on what room you are previewing. If you know you are joined already, you don't need it. If you pass an alias, you don't need it. If you got a random room and you know some vias, there is no harm in just passing them. Some examples where those vias can be from is already listed in the line you are commenting on, but if a client provides more ways to preview a room it might have client side fields for that, like a text field a user can enter servers in, and I don't think that needs to be specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AIUI, linking to
appendices/#routing
effectively answers the questions regarding "locally" and the choice of servers?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of taking this approach, why not just do what
/hierarchy
did and use aPublicRoomsChunk
explicitly, extending the schema of that? (the benefit being that all 3 related endpoints get aroom_type
,membership
,room_version
, andencryption
instead of having to subschema the nightmare a second time)Maintaining several similar objects is not ideal from a spec perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this implicitly specifies to reuse the "PublicRoomsChunk", but most people probably would not know what that means. I think it is clearer to name the endpoint, that already returns this value than using the name of the value, that most people probably don't remember what it is. At least I mostly ignore those names unless I work with the OpenAPI JSON directly.
I also copied this wording originally from the space summary MSCs. It only uses the PublicRoomsChunk name in one example, it generally talks about the return value of
publicRooms
otherwise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we tend to change sub-structure names in OpenAPI occasionally (that doesn't alter the shape of the OpenAPI, generated code notwithstanding), I agree with @deepbluev7 that the current text is good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the client meant to use the
membership
field? Even with the table below, the client should already have several other ways of knowing its current membership - is this really the best endpoint to reveal such information?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some clients don't call /sync. Having to use an extra endpoint to fetch this endpoint is opposite to the goals of this endpoint. In general the reason to call a summary endpoint is to do less calls than generating all that info yourself.
For example, if you want to join a room, you might want to generate a preview first. Then you want to name the button differently depending on if you are joined to the room or not. If you have all joined rooms cached locally, you can fetch that info from there, but some clients do not have that info without doing another request. And the server needs to check the join state anyway to generate some of the results, so it should not have an additional cost to just put that in the summary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the introduction of
knock_restricted
, knowing the join rule isn't sufficient to know which method the room can be joined via. As such the response should also include the allowedRoomIds, that the user is joined to or so.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear: does this mean the endpoint can be rate limited regardless of authentication? It might be safer to just list it like the spec does:
though I'm also not entirely convinced this should be allowed to be unauthenticated - the MSC needs to describe the usecase so it can be rationalized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is already an implementation of this, that can't use authentication, is that not enough? It is also the second example in the introduction...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which implementation can't use authentication? 2 years later, I'm less convinced of my 2022 opinion here, but curious if there's workarounds that could be explored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matrix.to can't use authentication. On the other hand, we already broke room avatars in matrix.to, so maybe workarounds have to be explored anyway.
(matrix.to can't use guest access, because not all servers allow guests, and previewing should not be centralized to go through one hardcoded server)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, I'd suggest trying guest access and if that fails, well, sorry - add context around the link rather than rely on the link :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that if we block this MSC behind requiring changes to
matrix.to
then it's going to get stuck for another 3.5 years while someone finds some tuits to do that work.Prove me wrong ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/badges/shields - as it takes the HS url in the badge request, so unless you require that all readme badges also bake in an access token it won't be able to auth or it'll only be able to connect to a handful of servers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead make authentication 'optional' — in the sense of recommended if you are an authenticated client, but fine to leave off if you're anonymous — and allow the homeserver to define the access rules for unauthenticated access here? This seems like maybe the most pragmatic option.
Creating a guest user for every matrix.to page load or shields.io badge load (etc etc) seems like an antipattern already anyway — it causes a proliferation of dummy users in the database and adds latency to the request, all without providing any meaningful benefit to, well, anyone really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also guest access is unavailable on servers with "Next Gen Auth" #3861 which is rolling out to major homeservers near you soon. (e.g. matrix.org)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please just use
PublicRoomsChunk
😭There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean
ChildRoomsChunk
. And again, I think those names are confusing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(FWIW - the name
ChildRoomsChunk
was introduced (by me) in a clarification PR (not MSC) solely for the purpose of describing, well, child rooms in/hierarchy
. Refactoring those sub-schema names is certainly not something for this MSC (we rarely, if ever, did it in the previous MSC practice).PublicRoomsChunk
is used in a couple of places across the API; I agree the name could be better but it actually reflects the current usage of this group of fields.)I think it's a good question for SCT on the status of those names, how much we have to preserve them etc.; in the meantime, I would suggest keeping the text as it is to avoid accidental confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this hardly seems like an unsurmountable problem... The argument for it dealing mostly with spaces is quite strong, but saying it's missing fields is just asking for an MSC to add the fields :p
The key with the alternatives section is it needs to describe why a solution isn't valid, so, why is writing an MSC for adding the fields a bad idea? (answer: it's because of the focus on spaces - it would be confusing to client developers to call a Spaces endpoint to get a non-Space summary, though there is an unargued point about shifting the complexity onto the server instead, as this MSC does).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This MSC actually adds most of those fields to the /hierarchy endpoint as well. But the EXISTING endpoint is not an alternative to this MSC, since it lacks those fields. Of course those could be added, but it is not the only reason why it currently can't be used for this. This sentence is just an additional complication that people would notice, when they try to use the /hierarchy endpoint as is without extending it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is all made invalid by the summary endpoint having to reduce itself down to a room ID before going out over federation? How is it any more routable if the server does the resolution instead of the client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the client resolves the alias, only the client knows the server that resolved the alias.
If the server resolves the alias, it receives a list of servers that it can join via, which it can then use to query for the summary. This is the same behavior as with every other endpoint that takes an alias (apart from the room directory endpoints themselves, I guess). Usually asking the server about the room the alias is hosted on is entirely sufficient though. Apart from the directory, there is no endpoint which passes an alias over federation, so I don't see how that would be an issue in this case now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true with any endpoint - the leak for this endpoint would more be that people could be surprised by the amount of metadata returned, such as in the case of matrix-org/matrix-spec#1186
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should I just delete that line? I do think it is a genuine concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also define a new
unstable_features
flag forGET /_matrix/client/versions
.For example, adapting from MSC3026:
Other MSCs that define an
unstable_features
are MSC2432, MSC2666, MSC2285, MSC3827, and MSC3440.