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

MSC1753: client-server capabilities API #1753

Merged
merged 7 commits into from Jan 9, 2019

Conversation

@richvdh
Copy link
Member

commented Dec 12, 2018

@turt2live

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Simple and straightforward - lgtm!

Show resolved Hide resolved proposals/1753-capabilities.md Outdated
Show resolved Hide resolved proposals/1753-capabilities.md Outdated
Show resolved Hide resolved proposals/1753-capabilities.md Outdated
query, where a server would return all possible supported operations. The
problem with this is that it may add load to the server, and increase network
traffic, by returning a large number of features which the client may have no
interest in.

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Dec 13, 2018

Member

This sounds slightly spurious to me, tbh. You'd need quite a large set of capabilities before it'd be noticeable I'd have thought (especially when using compression). I also wouldn't expect it be queried very often by clients, so compared to /sync and co I think the traffic/load would be negligible?

OTOH, I'm not against using a POST, though I kinda expect a lot of folks to ask why they can't simply do a GET (I could certainly see myself wanting to query everything to debug what's going on)

This comment has been minimized.

Copy link
@richvdh

richvdh Dec 13, 2018

Author Member

My concern is less about network traffic and more about a world where a server is divided up into microservices so has to go and ask lots of other bits of the system for answers on the capabilities - or even just do database queries or whatever.

@erikjohnston

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

Broadly LGTM, other than I'm wondering slightly about having GET, and perhaps we might (in future) want to consider adding a capabilities section to sync so clients can see changes?

@ara4n

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

it generally lgtm. i think it'd also be nice to support GET to discover what caps are around as an fyi (even if we don't know what to do with them), even though clients would in practice typically POST to do discover specific caps.

A nagging part of my brain is wondering whether should put these as state in a server room (same as the server msgs room?) to avoid creating yet another custom API and to get realtime sync for free, etc.

@richvdh

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

it generally lgtm. i think it'd also be nice to support GET to discover what caps are around as an fyi (even if we don't know what to do with them), even though clients would in practice typically POST to do discover specific caps.

Surely clients are going to use the GET if it's available, thus defeating the point of the POST. I don't feel that strongly about whether it should be GET or POST, but having both seems to be the worst of all worlds.

@richvdh

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

A nagging part of my brain is wondering whether should put these as state in a server room (same as the server msgs room?) to avoid creating yet another custom API and to get realtime sync for free, etc.

that sounds much more complicated to implement. Why is a custom API so much worse than custom event types?

@ara4n

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

that sounds much more complicated to implement. Why is a custom API so much worse than custom event types?

because it’s another set of endpoints to be specced, impl’d and maintained - and a new section to /sync if we want it pushed to the client in realtime. i’m surprised it seems more complicated, but i may be missing something?

To be clear: i am just trying to do the thought experiment here to ensure we evaluate both options sufficiently.

@richvdh

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2018

All I want for Christmas is threading.

because it’s another set of endpoints to be specced, impl’d and maintained

one endpoint, which seems to compare very favourably to one custom event type which also has to be specced, impl’d and maintained.

  • and a new section to /sync if we want it pushed to the client in realtime. i’m surprised it seems more complicated, but i may be missing something?

Let's suppose we change the configuration on the server so that users can no longer change their passwords. That means that the server needs to (a) spot that config change has been made, which may be non-trivial, and then (b) create a state event for every single user who has ever used the server so that it's ready for the client next time it syncs. That process may take a long time, so we need to be able to resume it halfway through.

As an alternative to (b), maybe we wait until the client syncs before creating a new event, but then that feels very similar to adding a new section to /sync.

Compare that with a simple endpoint that the client can go and check when it cares about something?

@richvdh

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2018

Based on the feedback here, I'm going to update the proposal to suggest a simple GET /_matrix/client/r0/capabilities API instead of the POST API. I don't really see the value in having both - it just feels like needless complication in the server and API.

@richvdh

This comment has been minimized.

Copy link
Member Author

commented Dec 24, 2018

It seems a bit unfair to call a FCP on this over Christmas, but if folks could let me know if they have anything else to add that would be helpful, so we can FCP it in January.

Show resolved Hide resolved proposals/1753-capabilities.md Outdated
@richvdh

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2019

ok folks I'd like to propose an FCP for this.

@richvdh

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2019

@mscbot fcp merge

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Jan 2, 2019

Team member @richvdh has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

Show resolved Hide resolved proposals/1753-capabilities.md Outdated
Show resolved Hide resolved proposals/1753-capabilities.md Outdated
Show resolved Hide resolved proposals/1753-capabilities.md Outdated

turt2live and others added some commits Jan 3, 2019

Update proposals/1753-capabilities.md
r0/versions isn't a thing

Co-Authored-By: richvdh <1389908+richvdh@users.noreply.github.com>
Update proposals/1753-capabilities.md
r0/versions isn't a thing

Co-Authored-By: richvdh <1389908+richvdh@users.noreply.github.com>
Update proposals/1753-capabilities.md
r0/versions isn't a thing

Co-Authored-By: richvdh <1389908+richvdh@users.noreply.github.com>

@anoadragon453 anoadragon453 removed the T-Core label Jan 4, 2019

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

lgtm.

@mscbot reviewed

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Jan 4, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Jan 9, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@anoadragon453 anoadragon453 merged commit ec97e1e into master Jan 9, 2019

7 checks passed

ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details

Superceded by https://github.com/orgs/matrix-org/projects/8 automation moved this from Review to Done - Operations Jan 9, 2019

turt2live added a commit that referenced this pull request Jan 31, 2019

Specify how capabilities work in the c2s API
Original proposals:
* #1753
* #1804

Implementation proof:
* matrix-org/synapse#4472
* matrix-org/matrix-js-sdk#830

There is one change to MSC1753 which is included in this commit. MSC1804 remains unchanged. In the original proposal, the change password capability being present was an indication that password changes were possible. It was found that this doesn't really communicate the state very well to clients in that lack of a capability (or a 404, etc) would mean that users would erroneously not be able to change their passwords. A simple boolean flag was added to assist clients in detecting this capability.
@turt2live

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

Merged via #1829 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.