mozilla-services / mozilla-services/syncstorage-rs
Refactor TokenserverError
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 1.8k
- Forks
- 86
- Avg merge
- 2d 17h
- Merged PRs (30d)
- 21
Description
Tokenserver’s top level TokenserverError has a few drawbacks:
- As of https://github.com/mozilla-services/syncstorage-rs/pull/1611 it exceeds clippy’s
result_large_errlint - Historically, it has “lost” (not included) the original Error source (such as for
DbErrors which have aimpl From<DbError> for TokenserverError. https://github.com/mozilla-services/syncstorage-rs/pull/1611 added support for referencing the original source but in a kludgey/quick fashion. - A minor nit of #2 is it unnecessarily includes multiple backtraces in the chain of errors, where we really only need one.
- Another minor nit is it can be a little cumbersome to construct the errors. However there was an attempt to implement a builder pattern which was deemed not that much of a win.
Following our typical pattern of thiserror w/ an ErrorKind enum would naturally split some of these fields up and likely avoid #1.
I imagine the ErrorKind would also solve #2 more cleanly as DbError would be its own variant likely using thiserror’s #[source]/#[from]. It seems like the various helper methods invalid_generation/unauthorized/etc could likely become variants? ErrorKind::Unauthorized, etc. However there are numerous usages of bespoke TokenserverErrors that do not use these methods: what to do with those? Many of them in the extractors module, e.g.
TokenserverError {
{{ description: "invalid query params".to_owned(),}}
{{ context: "invalid query params".to_owned(),}}
{{ http_status: StatusCode::BAD_REQUEST,}}
{{ location: ErrorLocation::Url,}}
{{ ..Default::default()}}
{{ }}}
I’m not sure if this complicates the ErrorKind strategy, per a comment by Ethan “I do think TokenserverErrors are a bit different than ApiErrors in that there isn't a small, enumerable set of reusable errors (the legacy Tokenserver has a number of errors that look similar to one another but aren't exactly the same)”
Once the large struct size issue is resolved we should revert the (clippy::result_large_err) allows.
┆Issue is synchronized with this Jira Task
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the TokenserverError definition and its bespoke constructions in the extractors module, then compare the ErrorKind pattern in syncserver/src/error.rs. Determine how to preserve existing error variants and sources while addressing result_large_err and redundant backtraces; done includes removing the related clippy allows.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend-api-design
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100