syncstorage-rs icon indicating copy to clipboard operation
syncstorage-rs copied to clipboard

Refactor TokenserverError

Open data-sync-user opened this issue 1 year ago • 0 comments

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

data-sync-user avatar Oct 16 '24 20:10 data-sync-user