Skip to content

Discuss rules for api deprecation and our stability guarantees #1105

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

Closed
jtgeibel opened this issue Oct 4, 2017 · 2 comments
Closed

Discuss rules for api deprecation and our stability guarantees #1105

jtgeibel opened this issue Oct 4, 2017 · 2 comments

Comments

@jtgeibel
Copy link
Member

jtgeibel commented Oct 4, 2017

In #1102 we recently touched on the concept of deprecation of unused api endpoints. I'm not necessarily advocating for the removal of these routes, but I think they help provide concrete examples for discussing our stability guarantees when it comes to the api.

First, a few questions to consider:

  • Should we require an RFC style process for removing old endpoints?
    • If so, should we also require the process for reviewing the addition of new endpoints?
  • Where would we communicate these changes so that external users are aware of proposed changes?
  • Should we instrument endpoints to confirm that that are no unexpected users before removal? Would reviewing logs be sufficient?
  • What exactly constitutes a breaking change?
    • For cargo endpoints, we should always return a 200 result status. Are we effectively committed to the same for other routes? (See HTTP error codes are barely used #712)
    • Is it okay to remove Option<T> fields from a json return type, or must we retain the field with a None value? In other words, is it okay to break clients that handle a nil value for a field but error when the field is absent?

Okay, so on to some specific examples!

GET /versions/:version_id and GET /crates/:crate_id/:version

These 2 routes are implemented in version::show(). There isn't really much reason for these to be combined as the endpoint has different semantics depending on if the :crate_id is present or not. I think this should be properly split into two endpoints.

For GET /versions/:version_id I don't expect there to be any external users at all as in order to use this api it is necessary to know the database id number for the version you seek. The primary way I see to obtain this is through GET /crates/:crate_id, but this endpoint already returns an array of versions containing all of this data. If there are any external users of this endpoint then we're probably doing duplicate effort to serve them.

GET /crates/:crate_id/:version has a much better interface, as crate_id and version are both human readable names and it is possible that some endpoints are constructing this URL on their own. This could be an argument for keeping this version of the endpoint around for longer, or at least doing more due-diligence to ensure there are no external uses.

GET /versions

This is very similar to the GET /versions/:version_id endpoint, except it accepts multiple id[] query parameters and returns a list of versions. Again, in order to make use of this endpoint the user has to already know the database id of the version.

JSON result type for GET /crates/:crate_id/:version/download

This endpoint is used by cargo and redirects to a URL on S3. However, if the request's Accept header contains json then we construct a 200 response with JSON data. The frontend code that used this code path was removed in #916 and had already been commented out for 3 years at the time.

While this code path is quite trivial, are we committed to keeping this until some far distant /api/v2 is released? (This is a slightly strange example because this is primarily a cargo route so I don't think this route would ever be removed even if other v1 endpoints are removed. At best we would remove this special case.)

Summary

In summary, I'm not suggesting that we remove any of these endpoints at this time. If we come up with a policy that allows for removal I would rank them as:

  • The endpoints that use raw database ids are low risk for removal.
  • The endpoint that uses human readable path segments is medium risk for removal.
  • The JSON response type for the download endpoint is medium risk for removal. We should probably add a comment describing the situation.

With these examples in mind, where do you fall on the scale between: "all endpoints must remain stable until /v2" and "cargo is our only official stability guarantee but we do our best to keep everyone happy"? There are of course mid-points along this scale. For instance, we could work with users of our api to triage the endpoints they are using and potentially expose them through an official client crate to encourage best practices.

@kureuil
Copy link
Contributor

kureuil commented Oct 8, 2017

  • Should we require an RFC style process for removing old endpoints?

I find this to be a very good idea, though I'd advocate that is should be used to deprecate an endpoint and not remove it.

    • If so, should we also require the process for reviewing the addition of new endpoints?

Probably, at least for the sake of consistency. Moreover, this would probably allow for a more cohesive and flexible API.

  • Where would we communicate these changes so that external users are aware of proposed changes?

The @cratesiostatus twitter account would be available for that, though it may not be popular enough atm. Maybe blog.rust-lang.org could be used for important announcements instead ?
I was thinking of maybe having a Jekyll blog deployed on Github Pages to allow for more in-depth explanations and general updates about the crates.io project than the Twitter account. Thoughts ?

  • Should we instrument endpoints to confirm that that are no unexpected users before removal?
    Would reviewing logs be sufficient?

I think instrumenting endpoints and being able to make choices based on the actual usage of the API would be a big step for crates.io. Reviewing logs might be possible now but it is too much error-prone to be relied on IMO.

  • What exactly constitutes a breaking change?

Do you mean are we committed to always return a 200 HTTP status code for other routes ? I think no, the only thing blocking us from having "classic" status code is cargo. We should be committed to use the existing status codes in future endpoints though.

    • Is it okay to remove Option fields from a json return type, or must we retain the field with a None value? In other words, is it okay to break clients that handle a nil value for a field but error when the field is absent?

I would say that it is not OK to break those clients by removing a field, and that forcing a None value isn't a lot more work that removing it.

My stance on this issue is "all endpoints must remain stable until /v2, at which point we will be able to slowly deprecate the v1 API, disabling the POST/PUT/DELETE enpoints first, and the read-only endpoints then.", especially since crates.io's API is advertised on the front-page since at least 3 years it seems strange to me to not handle it as a public API and as such we should not break the tools/applications people have built on top of it.

@Turbo87
Copy link
Member

Turbo87 commented Feb 11, 2021

I'm closing this issue for now as there hasn't been a lot of activity here recently.

@Turbo87 Turbo87 closed this as completed Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants