HTTP error codes are barely used #712
Comments
|
Hm. Sounds tricky if crates.io needs to support older versions of cargo. We could use /api/v2/ routes I suppose! I wonder if there's a more clever way to do this though... |
|
One thought I had was to do user-agent detection, looks like cargo 0.17 at least sends a useful user-agent ( |
carols10cents
added
the
C-enhancement ✨
1068: Begin refactoring module layout r=carols10cents This PR begins some work towards refactoring the module layout of our rust code. This PR includes several initial changes: * Group route definitions in `lib.rs` and document where those api endpoints are currently used. * Move the `krate` and `version` api modules into subdirectories. This will be explained further below. * Move `categories.rs` and `categories.toml` under a new `boot` module as these are only used during boot of the server process. * Pull github related functionality out of `http` into a separate `github` module, leaving the http security middleware behind. The second bullet point sets the groundwork for changes similar to what I prototyped in [this branch](3f67a0e...jtgeibel:refactor-api-into-submodules). I'm hoping that by moving and renaming these to `mod.rs` now, without actually changing the files yet, will cut down on merge conflicts with any future PRs. # Proposed further changes As proposed in the previous branch, I believe the next step would be to split the `krate` and `version` modules into submodules which support `cargo` and `frontend` functionality. At the moment, the `api/v1/crates` route is the only one used by both. My main motivation for this is that for `cargo` routes we need to strictly maintain backwards compatibility. For instance, as noted in #712, we always return a `200` response code with an `errors` array because this is expected by `cargo` (with the exception of `krate::download` where it does not). If we ever decide to change this then we will need to take care that we don't impact routes used by cargo. (Of course we need to be careful in general, as we've broken unknown external users in the past, but hopefully this will provide an extra speed bump when authoring and reviewing changes.) While experimenting on that branch I also got a better feel for how we could split logic between the api/controller level and the model/view level. My notes on this may be a bit dated, but in some modules we mix api functionality with diesel and serde structs. For instance, the following files contain logic for all these concerns: * `keyword.rs` * `krate/mod.rs` * `token.rs` * `user/mod.rs` * `version/mod.rs` In contrast, the following files contain only diesel and serde functionality, in support of api calls handled by the files above: * `category.rs` * `dependency.rs` * `download.rs` * `owner.rs` It would be nice if we could pull out the database and json related functionality in the first list, and then group all of those files in a new submodule. At this time, I do not think it is necessary to try to split apart the model and view functionality as this logic is very straightforward. For now, I hope this is a small enough PR to review independently and provide a place to discuss these suggestions for further refactoring. /cc @vignesh-sankaran
|
this situation is unlikely to change since we need to support old cargo versions in a backwards compatible way. for new endpoints we are trying to use the correct HTTP response codes, but for the existing ones we unfortunately can't change anything without risking to break older cargo versions. if we ever get to a API v2 we could add a compatibility layer that emulated API v1 on top of v2, just for cargo, but this does not seem likely to happen in the near future, so I'll close this issue for now until we actually have the capacity to revisit this. |
The only HTTP status codes other than 200 that are currently used are 404 and 403 in the places you'd expect. Any other errors (e.g. validation failures while publishing a crate) are returned as a 200 response with a json object describing the error.
I took a look at all the current uses of
util::errors::humanand they all seem to pretty nicely correspond to errors that should be a 400 Bad Request. I attempted this change but unfortunatelycargodoes not handle getting responses with status code 400 nicely (rust-lang/cargo#3995).Obviously this is something that may take a little finessing to not break existing cargo. Or the work required for that may make this not worth changing at all.
As a reason to do this other than just for consistency with HTTP APIs; I noticed this while attempting to get errors back to the UI in #697. When saving a data model Ember.js expects to either get back a response with status code 200 containing the saved model, or a response with an error status code containing the error messages. I can special case the validation errors, but this would introduce some inconsistency between different request handlers intended for the UI and request handlers intended for cargo.
The text was updated successfully, but these errors were encountered: