Error response refactoring step 2 #1929

jtgeibel · 2019-11-28T01:12:07Z

This is a continuation of #1920. This series captures several changes:

The controller prelude is split to provide helpers for either cargo or frontend endpoints. Eventually this will be unnecessary, but I see it as a temporary guard rail to encourage the correct usage.
Several user and session frontend endpoints are updated to return a response with the correct HTTP status. Tests were updated as necessary.
A util::errors::http module is added for concrete error types associated with HTTP status codes. There are more types to be moved here, but for review purposes that can be done in another PR.
Once cargo_err was refactored to use http::Ok, all of the description and fallback logic in the AppError trait and ConcreteAppError type was no longer necessary.

Here is what I have in mind for the next steps in this refactor:

Add a not_found helper that allows a 404 status with an error message for the client. (TBD if this can be merged with the existing NotFound type or if that functionality should remain independent.)
Finish moving types to http as applicable.
Review remaing existing usage of helpers within controllers.

After that groundwork, my final step is to investigate the ability to sanitize error response for cargo at the router level so that cargo_err can be removed. It isn't really practical currently to use the correct helper type from within models, since model functionality can be used from either type of endpoint (or both).

The router seems like a strange place for that behavior, but it's where the error to response conversion already happens, and the middleware layer only sees a generic dyn Error, not our AppError.

With a central place to sanitize errors for old cargo versions, we could even enable it only for old (and empty) user agents.

r? @smarnach

jtgeibel · 2019-11-28T01:50:56Z

For a bit further context, the tests for the generic error to response behavior are here. Basically, the error handling behavior is:

If AppError::response returns Some(_) then that response is sent to the user. When the error is converted to a response like this, it is no longer available for logging in the middleware layer (beyond seeing the HTTP status code, or extracting details from the JSON).
If AppError::response returns None then the error is converted to a Box<dyn Error>. This allows for more detailed logging, and is eventually converted to a generic status 500 response by the EnsureWellFormed500 middleware. (This applies to things like internal, and the generic AppError impl for Errors.)

jtgeibel · 2019-11-28T03:58:20Z

I've pushed an additional commit to resolve an issue in logging chained errors that was uncovered by this branch.

Returning the following from a controller

None.chain_error(|| internal("inner error")).chain_error(|| internal("middle error")).chain_error(|| internal("outer error"))

results in logging only the outer error on master:

[...] error="outer error"

With the changes in this PR to go through fmt::Display, now two implementations became active:

[...] error="outer error caused by middle error caused by inner error
Caused by: middle error caused by inner error
Caused by: inner error"

With the final commit here, we now see the expected log output:

[...] error="outer error caused by middle error caused by inner error"

I've added it to this PR since it doesn't really fix anything as a standalone fix on master.

bors · 2019-12-02T18:50:42Z

☔ The latest upstream changes (presumably #1891) made this pull request unmergeable. Please resolve the merge conflicts.

This serves as a guard rail to help avoid using the wrong error response method. Two files still reference the generic prelude, becuase those files contain mixed endpoints and there is no reason to do a larger refactor at this time.

…err`

The `impl<E: AppError> fmt::Display for ChainedError<E>` is sufficient.

jtgeibel · 2019-12-05T02:59:57Z

I've rebased to resolve merge conflicts.

I also have a series of commits building on this branch. I'll open a separate PR for that once this lands and I have a chance to add more documentation.

smarnach · 2019-12-12T09:38:00Z

@jtgeibel Apologies for the delay in reviewing this – I'll get to this in the weekend.

smarnach

Thanks for doing this refactoring! The idiosyncratic error reporting, and the wrong status codes in particular, have always irked me, and I'm glad to see these changes.

The endpoints you changed the status codes for look like they are not used by Cargo to me, but I don't know all the things that Cargo can do and have not double-checked in the Cargo code base. It would be great if we could add integration tests for the interaction with Cargo, so we can be more confident about changes that might cause regressions.

Would it be possible to add a test that error chains are logged correctly? Preferably in a fuzzy way, e.g. only making sure that the error message of each error in the change appears exactly once in the log.

src/util/errors/http.rs

jtgeibel · 2019-12-17T04:05:49Z

The endpoints you changed the status codes for look like they are not used by Cargo to me, but I don't know all the things that Cargo can do and have not double-checked in the Cargo code base.

My understanding is that with the exception of download, all interactions with crates.io are via the crates-io crate in the cargo source tree.

It would be great if we could add integration tests for the interaction with Cargo, so we can be more confident about changes that might cause regressions.

That would be awesome!

Would it be possible to add a test that error chains are logged correctly? Preferably in a fuzzy way, e.g. only making sure that the error message of each error in the change appears exactly once in the log.

I've pushed some commits that add tests for the chain_err behavior. I've added comments to identify the combinations where a user facing error wraps a more concrete error, causing no error to be logged (because a valid response was sent to the user).

I don't think its worth adding anything more complicated at the moment (such as integrating with the logging middleware to capture the actual log output). Right now most uses of chain_err in the codebase are to convert a None into a user facing error. Also, after my next set of commits, it may make sense to impl chain_err on the new concrete Error enum type. Thus eliminating any misuse of chain_err to wrap an error already intended for the user.

smarnach · 2019-12-19T10:34:33Z

Looks good to me.

@bors r+

bors · 2019-12-19T10:34:35Z

📌 Commit 2f1e697 has been approved by smarnach

bors · 2019-12-19T12:06:34Z

⌛ Testing commit 2f1e697 with merge d20160e...

@smarnach

Error response refactoring step 2 This is a continuation of #1920. This series captures several changes: * The controller prelude is split to provide helpers for either cargo or frontend endpoints. Eventually this will be unnecessary, but I see it as a temporary guard rail to encourage the correct usage. * Several user and session frontend endpoints are updated to return a response with the correct HTTP status. Tests were updated as necessary. * A `util::errors::http` module is added for concrete error types associated with HTTP status codes. There are more types to be moved here, but for review purposes that can be done in another PR. * Once `cargo_err` was refactored to use `http::Ok`, all of the description and fallback logic in the `AppError` trait and `ConcreteAppError` type was no longer necessary. Here is what I have in mind for the next steps in this refactor: * Add a `not_found` helper that allows a 404 status with an error message for the client. (TBD if this can be merged with the existing `NotFound` type or if that functionality should remain independent.) * Finish moving types to `http` as applicable. * Review remaing existing usage of helpers within controllers. After that groundwork, my final step is to investigate the ability to sanitize error response for cargo at the router level so that `cargo_err` can be removed. It isn't really practical currently to use the correct helper type from within models, since model functionality can be used from either type of endpoint (or both). The router seems like a strange place for that behavior, but it's where the [error to response conversion](https://github.com/rust-lang/crates.io/blob/4a1542cb17ae50c2bfd0f7e85876d82617e30415/src/router.rs#L139) already happens, and the middleware layer only sees a generic `dyn Error`, not our `AppError`. With a central place to sanitize errors for old cargo versions, we could even enable it only for old (and empty) user agents. r? @smarnach

bors · 2019-12-19T12:14:46Z

☀️ Test successful - checks-travis
Approved by: smarnach
Pushing d20160e to master...

@smarnach

Add a concrete `util::Error` type (step 3 of refactoring) This PR may be easier to review commit by commit. Overall this series continues the previous refactoring work in #1920 and #1929. In particular, a concrete `util::Error` error type is introduced. The code is updated to follow a style where: * The new concrete type `util::Error` is great for code that is not part of the request/response lifecycle. Binaries have been converted to `fn main() -> Result<(), Error>` where applicable. This avoids pulling in the unnecessary infrastructure to convert errors into a user facing JSON responses (relative to `AppError`). * There are now 13 results across 6 files for `, Error>`. Usage occurs in `src/bin/`, `src/boot/`, and `src/uploaders.rs`. * `diesel::QueryResult` - There is a lot of code that only deals with query errors. If only one type of error is possible in a function, using that specific error is preferable to the more general `util::Error`. This is especially common in model code. * There are now 49 results across 16 files for `QueryResult<`. Most usage is in `src/models/`, with some usage in `src/publish_rate_limit.rs`, `src/tasks/`, and `src/tests/`. * `util::errors::AppResult` - Some failures should be converted into user facing JSON responses. This error type is more dynamic and is box allocated. Low-level errors are typically not converted to user facing errors and most usage is within the models, controllers, and middleware layers (85 of 102 results below). * There are now 102 results across 36 files for `AppResult<`. Most usage is in `src/controllers/` and `src/models/`, with additional usage in `src/middleware/`, `src/{email,github,publish_rate_limit,router,uploaders}.rs`, and `src/{tests,util}/`. Finally, a `middleware::Result` type is introduced for use in middleware and router code. The behavior of `dyn AppError` and `chain_error` is a remaining area for improvement. There may be cases where we want to wrap an `internal` error in a user facing error (like `cargo_err` or `server_err`) and still have the internal error make it up to the logging middleware. However, I don't see an easy way to propagate this up the middleware stack (without encoding it in an internal response header that we later remove, :sweat:). r? @smarnach

rust-highfive assigned smarnach Nov 28, 2019

rust-highfive added the S-waiting-on-review label Nov 28, 2019

jtgeibel added 10 commits December 4, 2019 21:29

Remove some unused fields from ConcreteAppError

b961409

Fix HTTP status codes for 2 non-cargo user endpoints

5b0671d

Fix HTTP error status from me/crate_owner_invitations

49a568e

Fix HTTP error status for session endpoints

e2c9213

Simplify implementation of cargo_err

c84368a

Drop description and fallback logic that is no longer used by `cargo_…

f2009aa

…err`

Rename ConcreteAppError to InternalAppError

cb9e992

Switch logging to use to_string on errors

9d8a6bb

Remove duplicate error "caused by" logging

a5a451e

The `impl<E: AppError> fmt::Display for ChainedError<E>` is sufficient.

jtgeibel force-pushed the fix-some-error-responses branch from 2df534f to a5a451e Compare December 5, 2019 02:49

smarnach reviewed Dec 16, 2019

View reviewed changes

src/util/errors/http.rs Show resolved Hide resolved

Add test for chain_error used with internal

e86de9b

Add a few more tests for chain_err

2f1e697

jtgeibel force-pushed the fix-some-error-responses branch from 00deb91 to 2f1e697 Compare December 17, 2019 04:06

smarnach mentioned this pull request Dec 19, 2019

Limit the branches that travis builds on #1991

Merged

bors merged commit 2f1e697 into rust-lang:master Dec 19, 2019

jtgeibel mentioned this pull request Dec 21, 2019

Add a concrete util::Error type (step 3 of refactoring) #2032

Merged

jtgeibel deleted the fix-some-error-responses branch December 26, 2019 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error response refactoring step 2 #1929

Error response refactoring step 2 #1929

jtgeibel commented Nov 28, 2019

jtgeibel commented Nov 28, 2019

jtgeibel commented Nov 28, 2019

bors commented Dec 2, 2019

jtgeibel commented Dec 5, 2019

smarnach commented Dec 12, 2019 •

edited

Loading

smarnach left a comment

jtgeibel commented Dec 17, 2019

smarnach commented Dec 19, 2019

bors commented Dec 19, 2019

bors commented Dec 19, 2019

bors commented Dec 19, 2019

Error response refactoring step 2 #1929

Error response refactoring step 2 #1929

Conversation

jtgeibel commented Nov 28, 2019

jtgeibel commented Nov 28, 2019

jtgeibel commented Nov 28, 2019

bors commented Dec 2, 2019

jtgeibel commented Dec 5, 2019

smarnach commented Dec 12, 2019 • edited Loading

smarnach left a comment

Choose a reason for hiding this comment

jtgeibel commented Dec 17, 2019

smarnach commented Dec 19, 2019

bors commented Dec 19, 2019

bors commented Dec 19, 2019

bors commented Dec 19, 2019

smarnach commented Dec 12, 2019 •

edited

Loading