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

Use non-200 HTTP response status codes for API errors #7881

Closed
Turbo87 opened this issue Jan 5, 2024 · 6 comments
Closed

Use non-200 HTTP response status codes for API errors #7881

Turbo87 opened this issue Jan 5, 2024 · 6 comments
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works

Comments

@Turbo87
Copy link
Member

Turbo87 commented Jan 5, 2024

When cargo and crates.io were first implemented, the HTTP response status codes were ignored. cargo assumed that crates.io would always respond with a "200 OK" and any error messages are detected from the response JSON body. When crates.io responded with a different status code the cargo was showing the raw JSON body instead of a regular error message.

This was changed in rust-lang/cargo#6771, which was released in cargo 1.34 (mid-2019). Since that release, cargo now supports 4xx and 5xx status codes too and extracts the error message from the JSON response, if available.

The crates.io team has decided to phase out these "200 OK" response status codes for error responses to make our API a little more intuitive. Since this can be seen as a breaking change to our API we will need to announce this change publicly, including a cut-off date. On this date we want to switch the API from the previous "200 OK" behavior to the new 4xx/5xx behavior.

Inside the crates.io source code we have been using a cargo_err() fn which returns a "200 OK" JSON response with a custom error message. These calls will have to be replaced with other errors.

To allow us to incrementally migrate our codebase while maintaining the previous behavior, a middleware was introduced in #7715, which ensures that all cargo-relevant endpoints always return "200 OK". #7776 then implemented a feature flag on top of the middleware, which will allow us to flip the switch on the cut-off date and test it out ahead of time on our staging environment.

The remaining work now is to replace all calls of cargo_err() with 4xx/5xx error responses and adjust the corresponding tests, or write new tests if none exist for these error scenarios yet.

@Eh2406
Copy link
Contributor

Eh2406 commented Jan 6, 2024

cc: @rust-lang/cargo, for a heads up about the breaking change.

@weihanglo
Copy link
Member

Cargo might not need to change I guess? Just old cargos (<1.35) would get obscure error messages, which the versions are so old and should be fine.

@ehuss
Copy link
Contributor

ehuss commented Jan 6, 2024

How much of a breaking change is this? It seems like it is mostly an issue of the quality of error messages, correct?

For example, the old behaviors are:

  • 403 — received 403 unauthorized response code, which doesn't explain why, but should be sufficient for a user to figure out what was wrong (their token was bad, or for the wrong account/crate, etc.).
  • 404 — received 404 not found response code, which doesn't explain why, but should be sufficient for a user to figure out what was wrong (cargo owner for a package that doesn't exist).
  • all other non-200 codes — shows the raw headers and body, which although ugly, contains the text of the error.

This issue doesn't seem to clarify if it also encompasses rust-lang/cargo#13158, which is a real breaking change, and is only in 1.76.

Is there anything I'm missing?

@Turbo87
Copy link
Member Author

Turbo87 commented Jan 6, 2024

Cargo might not need to change I guess? Just old cargos (<1.35) would get obscure error messages

exactly. from what I've seen older cargo versions would show the raw JSON response instead of the embedded error message.

How much of a breaking change is this? It seems like it is mostly an issue of the quality of error messages, correct?

yes, exactly.

This issue doesn't seem to clarify if it also encompasses rust-lang/cargo#13158, which is a real breaking change, and is only in 1.76.

the feature flag and middleware have three states: full cargo compat, 2xx-to-200 compat and no status code adjustments. we are only planning to go from the first to the second. in other words: we're acting as if rust-lang/cargo#13158 didn't exist. in a couple of years we might move to "no status code adjustments", but we're far away from that.

Is there anything I'm missing?

nope, I don't think this will be a big deal since cargo has been compatible with the planned change for 4ish years now.

@Turbo87
Copy link
Member Author

Turbo87 commented Jan 31, 2024

announcement blog post is up for review now: rust-lang/blog.rust-lang.org#1236

@Turbo87
Copy link
Member Author

Turbo87 commented Mar 31, 2024

as mentioned in the blog post, on 2024-03-04 we flipped the feature flag, so now we are returning proper HTTP status codes for all of our error cases.

in other words: done! ✅

@Turbo87 Turbo87 closed this as completed Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

No branches or pull requests

4 participants