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

jtgeibel · 2019-12-21T02:38:24Z

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, 😓).

r? @smarnach

smarnach · 2019-12-23T13:29:08Z

I plan to review this on 27 December, but this plan is subject to change…

bors · 2019-12-23T15:34:24Z

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

smarnach · 2019-12-27T15:56:34Z

I'm sorry I did not get to this today, and I won't have time until 2020-01-05.

bors · 2020-01-04T01:58:18Z

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

jtgeibel · 2020-01-04T17:43:06Z

I'm sorry I did not get to this today, and I won't have time until 2020-01-05.

No problem at all. When you have a chance, would you start with PR #2071. It's a small fix and I'd like to get it deployed, just want a 2nd set of eyes on it. Thanks!

bors · 2020-01-05T21:42:03Z

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

smarnach

Phew, this one took me a while to get through. :)

I'm really glad you untangled this mess. The old code more or less threw away concrete error information in most cases and converted everything into a CargoError, essentially introducing all the downsides of exceptions in languages like Python. The new version makes much better use of Rust's great error handling capabilities and has fewer explicit error conversions, dynamic calls and allocations.

And I couldn't find anything to complain about. :) On a high level, I completely agree with these changes, and in the details the compiler already did most of the review work for me.

It would be great if you could introduce some high-level documentation in a doc comment at the beginning of error.rs, explaining the use cases for the main error or result types – basically a summary of what you put in the PR description. This could help keeping things nice and clean in the future (though the types in the preludes of the different subcomponents also help a lot ensuring future consistency).

I added a question or two, so I'm not going to r+ right now. However, feel free to merge without changes.

src/background_jobs.rs

src/bin/enqueue-job.rs

Background jobs do not need the JSON response functionality of `AppError`. Git operations, and most upload functions now use a generic boxed error.

jtgeibel · 2020-01-06T02:30:34Z

It would be great if you could introduce some high-level documentation in a doc comment at the beginning of error.rs, explaining the use cases for the main error or result types – basically a summary of what you put in the PR description.

I had been holding off on adding high-level documentation until things settled down, but I think this is now a good time to add these recommendations as documentation. (Especially since "I'll add some docs in a future PR" always takes longer than expected to get back around to.) I've rebased the PR, and added a commit adapting part of the PR description into module docs.

Thanks for the review and feedback!

@bors r+

bors · 2020-01-06T02:30:35Z

📌 Commit 541f710 has been approved by jtgeibel

bors · 2020-01-06T02:30:44Z

⌛ Testing commit 541f710 with merge a251368...

@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

bors · 2020-01-06T02:37:39Z

☀️ Test successful - checks-travis
Approved by: jtgeibel
Pushing a251368 to master...

rust-highfive assigned smarnach Dec 21, 2019

rust-highfive added the S-waiting-on-review label Dec 21, 2019

jtgeibel force-pushed the error-refactor-step3 branch from bfc1631 to ba52dff Compare December 26, 2019 23:07

jtgeibel mentioned this pull request Jan 4, 2020

Remove some usage of unwrap() #2072

Merged

jtgeibel force-pushed the error-refactor-step3 branch from ba52dff to 6f33bbe Compare January 4, 2020 17:46

smarnach approved these changes Jan 5, 2020

View reviewed changes

src/background_jobs.rs Outdated Show resolved Hide resolved

src/bin/enqueue-job.rs Show resolved Hide resolved

jtgeibel added 17 commits January 5, 2020 20:38

Move BadRequest to http

1d0d321

Add json_error to simplify error type impls

bdd0114

Remove an allocation when generating error responses

87435d0

Make AppErrToStdErr private

10e7dea

Remove usage of std_error_no_send

660a6c4

Background jobs do not need the JSON response functionality of `AppError`. Git operations, and most upload functions now use a generic boxed error.

Return concrete error from db_conn

616938e

Convert enqueue-job bin to a new concrete error type

6920afe

Convert pagerduty bins to the concrete error type

4d48156

Convert categories boot sync to concrete error type

9ae544c

Convert email errors to server_error

49cc2ca

Clean up error re-exports

b7c8c8c

Downgrade more return types from AppResult to QueryResult

795bf15

Simplify uploader error handling

7a079a0

Add middleware Result type

14c7d0c

Add conversion from s3::Error into util::Error

08f2853

Prefer swirl::PerformError over swirl::errors::PerformError

8800959

Add module level documentation describing typical usage of error types

541f710

jtgeibel force-pushed the error-refactor-step3 branch from c621f0d to 541f710 Compare January 6, 2020 02:21

bors merged commit 541f710 into rust-lang:master Jan 6, 2020

This was referenced Jan 6, 2020

Add Crate Owner Auditing #2025

Closed

Convert pagination ISE into a Bad Request response #2065

Merged

Use the read-only replica for some queries in production #2073

Merged

Defer user authentication until requested by endpoint #2077

Merged

jtgeibel deleted the error-refactor-step3 branch January 6, 2020 02:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jtgeibel commented Dec 21, 2019 •

edited

Loading

smarnach commented Dec 23, 2019

bors commented Dec 23, 2019

smarnach commented Dec 27, 2019

bors commented Jan 4, 2020

jtgeibel commented Jan 4, 2020

bors commented Jan 5, 2020

smarnach left a comment •

edited

Loading

jtgeibel commented Jan 6, 2020

bors commented Jan 6, 2020

bors commented Jan 6, 2020

bors commented Jan 6, 2020

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

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

Conversation

jtgeibel commented Dec 21, 2019 • edited Loading

smarnach commented Dec 23, 2019

bors commented Dec 23, 2019

smarnach commented Dec 27, 2019

bors commented Jan 4, 2020

jtgeibel commented Jan 4, 2020

bors commented Jan 5, 2020

smarnach left a comment • edited Loading

Choose a reason for hiding this comment

jtgeibel commented Jan 6, 2020

bors commented Jan 6, 2020

bors commented Jan 6, 2020

bors commented Jan 6, 2020

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

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

jtgeibel commented Dec 21, 2019 •

edited

Loading

smarnach left a comment •

edited

Loading