mozilla-services / mozilla-services/syncstorage-rs

Refactor TokenserverError

Open
#1,612 0 comments 0 reactions 0 assignees View on GitHub

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:

  1. As of https://github.com/mozilla-services/syncstorage-rs/pull/1611 it exceeds clippy’s result_large_err lint
  2. 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.
  3. A minor nit of #2 is it unnecessarily includes multiple backtraces in the chain of errors, where we really only need one.
  4. 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.