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

MSC2475: API versioning #2475

Closed
wants to merge 2 commits into from
Closed

MSC2475: API versioning #2475

wants to merge 2 commits into from

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Mar 27, 2020

@richvdh
Copy link
Member

richvdh commented Mar 27, 2020

I'm still a bit unclear about what you're proposing here.

I think you're saying that #2438 (comment) is wrong (which I agree with: we should be using v1 there)?

some examples would help.

@turt2live
Copy link
Member Author

I'm not saying anything is wrong per-say, just that we need to figure this out.


When using the `/v1` style of versioning in APIs, it is a per-endpoint version. The first
version of an endpoint should be `v1` in most cases, though for ease of understanding it is
accetable to use a higher version when making large/sweeping changes to the API. For example,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
accetable to use a higher version when making large/sweeping changes to the API. For example,
acceptable to use a higher version when making large/sweeping changes to the API. For example,

1. The spec currently has `/v1/profile`, `/v1/get_event`, and `/v2/send_message`.
2. An MSC is opened which adds authentication to all the endpoints. Some `/login` and
`/register` endpoints are added too. These endpoints can be `/v3/login` and `/v3/register`
for clarity and consistency with the bumps to the other endpoints.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for clarity and consistency with the bumps to the other endpoints.
for clarity and consistency with the bumps to the other endpoints (`/v3/profile/`, `/v3/get_event/`, and `/v3/send_message`).

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.

proposal as it stands sgtm. the numbering on the examples needs fixing.

@turt2live
Copy link
Member Author

gah, I put extra lines between the examples to avoid exactly that problem. Will give them titles I guess.

@anoadragon453
Copy link
Member

@turt2live You could also do:

  1. The spec currently has /v1/profile, /v1/get_event, and /v2/send_message.
    • An MSC to add a new endpoint, /delete_message is made. It would be /v1/delete_message because it's the first time we've seen it.

...perhaps.

Either way, this MSC looks pretty sane to me and @richvdh has agreed to it as well, so:

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Mar 30, 2020

This FCP proposal has been cancelled by #2475 (comment).

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

Concerns:

  • Needs more thinking time before we rush into things

Once at least 75% of reviewers approve (and there are no outstanding concerns), 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 information about what commands tagged team members can give me.

@mscbot mscbot added disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Mar 30, 2020
@turt2live
Copy link
Member Author

... I'm not yet convinced myself that the MSC is the way to go, and I see no reason to rush this through.

@mscbot concern Needs more thinking time before we rush into things


The version number is monotonically increasing integer. The `v1` style versioning has no
relation to the API's release version (`r0.1.0` has no meaning towards `v1`, unlike in the
`r0` style versioning).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think something like this is sorely needed, especially if something like login should ever move to interactive auth and hashed passwords. I'm not sure, if I understand correctly, how this should work though:

  • Does this mean an endpoint like /_matrix/client/r0/login becomes /_matrix/client/r0/v2/login?
  • What about nested endpoints? /_matrix/client/r0/v2/user/{userId}/rooms/{roomId}/tags or /_matrix/client/r0/user/{userId}/rooms/{roomId}/v2/tags?
  • what about conflicts, like you can't add a v2 here: /_matrix/client/r0/user/{userId}/rooms/{roomId}/tags/v2/{tag}, so maybe it only makes sense to add the v2 at the top level?

It seems like the federation API always adds the version below the top level, i.e. after /_matrix/federation/ or /_matrix/keys/, so maybe that was intended here? Would it be added after the r0 or before it though for the client server API?

Maybe tangentially related, is there some material for why the rX and vX versioning was chosen to be the way it currently is? I think I remember, that there was a longer discussion mentioned in a discussion about this topic.

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't touch the client-server API at all, and does not suggest moving the version number to anywhere except after the coarse scoping done by the path (/_matrix/:api/:version/:subpath).

Maybe tangentially related, is there some material for why the rX and vX versioning was chosen to be the way it currently is? I think I remember, that there was a longer discussion mentioned in a discussion about this topic.

It's old enough that any documentation has been lost to the wind. There might be some contained in this repo somewhere, but ultimately it's stuck in people's memories and not on paper. Some of those people no longer work on the project, unfortunately.

Copy link
Member

Choose a reason for hiding this comment

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

From memory, the rationale for the /rX.Y.Z prefix in the CS API was:

  • You don't have to guess what versions of APIs work together. There's one global prefix for the whole CS API, rather than the weird mix of /v1 and /v2_alpha prefixes we had before.
  • It's tied to the released version of the spec, with the compatibility guarantees given by the X.Y.Z numbering scheme.

The disadvantage is that if some random minor API makes a backwards-incompatible change, in theory the entire prefix should change to be /r2 or /r3 or whatever - which some might see as cumbersome. Others might argue that this is a good reason to avoid backwards-incompatible changes wherever possible (as indeed we have so far, hence being on /r0.y.z still).

The other disadvantage is that the SS API never moved to use the /rX.Y.Z format, so we're currently very inconsistent between the APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ara4n Thank you for the context. Those are just the problems with versioning in general. I think choosing the right tradeoff is really hard and there seems to be no perfect solution. I think it would make sense to accumulate /v2, /v3 changes and once they are stabilized, cut a new major revision, which removes old versions of those endpoints and puts them all under a global /r1, /r2 or whatever, so that a new server only needs to implement the /r1 API.

@turt2live So this proposal is only for the SS API? Was that not mentioned in the MSC or did I just miss it?

In any case, I think something like this is really needed for the CS API, but it needs a bit more work to fit there too and to specify the transition from /r0/v5 to /r1/v0 or /r1 (no suffix). There are a few endpoints in the CS API, that probably need a new version to improve them, but where you don't want to bump the full API version for now (like login for example). Similarly I'd say it would make sense to transition the federation API to /r0 at some point, when some of the /v1 endpoints should get dropped, to make implementing new homeservers easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

This affects all the APIs except the client-server API, as mentioned in the introduction. Making this affect client-server is best left for a different MSC.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I think that wasn't explicit enough for me :D

@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Apr 20, 2020
@matrix-org matrix-org deleted a comment from mscbot Apr 27, 2020
@turt2live
Copy link
Member Author

@mscbot fcp cancel

This needs to be reviewed for accuracy and relevance (by me).

@mscbot mscbot added proposal-in-review and removed disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Jan 26, 2021
@turt2live turt2live self-assigned this Jan 26, 2021
@turt2live
Copy link
Member Author

This MSC is no longer relevant.

@turt2live turt2live closed this Mar 22, 2021
@turt2live turt2live added obsolete A proposal which has been overtaken by other proposals and removed proposal-in-review labels Mar 22, 2021
@turt2live turt2live deleted the travis/msc/versioning branch March 22, 2021 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec obsolete A proposal which has been overtaken by other proposals proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants