Begin refactoring error handling/fallback logic #1920
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
smarnach
approved these changes
Nov 23, 2019
There was a problem hiding this comment.
This looks all good to me. The new names are definitely less idiosyncratic than what we had before. I'd also be fine with naming the error and result times Error and Result, which is a common pattern, but this would create quite a few conflicts we would have to fix manually, which doesn't seem worth it.
This PR has the potential to conflict with several other open PRs. My guess is that the quickest way forward is to first merge this PR and then resolve the conflicts in the other PRs, but that's just a gut feeling. Feel free to merge, and consider fixing the resulting conflicts yourself, since you will have more context about what's needed.
|
☔ The latest upstream changes (presumably #1909) made this pull request unmergeable. Please resolve the merge conflicts. |
These types represent generic application errors, and not errors specific to cargo requests/endpoints.
Errors that implement their own `response` method do not need to impl any of the fallback methods. This change should help avoid adding new errors that rely on this fallback behavior.
jtgeibel
force-pushed
the
rename-error-types-and-fns
branch
from
fcfb456 to
53e5ef2
Compare
My thoughts as well. @bors r=smarnach |
|
📌 Commit 53e5ef2 has been approved by |
bors
added a commit
that referenced
this pull request
Nov 26, 2019
Begin refactoring error handling/fallback logic This PR is best reviewed as individual commits. The first commits rename some types and functions without changing anything. The final commits remove some unused logic and lay some groundwork for future refactoring. The overall intent of this PR is to more clearly document the existing behavior. In particular, the top level error types no longer include `Cargo` in the name as not all error types are intended for cargo endpoints. r? @smarnach
|
☀️ Test successful - checks-travis |
bors
added a commit
that referenced
this pull request
Dec 19, 2019
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
added a commit
that referenced
this pull request
Jan 6, 2020
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is best reviewed as individual commits. The first commits rename some types and functions without changing anything. The final commits remove some unused logic and lay some groundwork for future refactoring.
The overall intent of this PR is to more clearly document the existing behavior. In particular, the top level error types no longer include
Cargoin the name as not all error types are intended for cargo endpoints.r? @smarnach