Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor TokenserverError #1612

Open
data-sync-user opened this issue Oct 16, 2024 · 0 comments
Open

Refactor TokenserverError #1612

data-sync-user opened this issue Oct 16, 2024 · 0 comments

Comments

@data-sync-user
Copy link
Collaborator

data-sync-user commented Oct 16, 2024

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 feat: initial organization, some db code, and the full request path #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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant