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

feat: Add autoendpoint crate #150

Merged
merged 15 commits into from
Jun 8, 2020
Merged

feat: Add autoendpoint crate #150

merged 15 commits into from
Jun 8, 2020

Conversation

AzureMarker
Copy link
Contributor

@AzureMarker AzureMarker commented Jun 3, 2020

Based off of the https://github.com/mozilla-services/autoendpoint-rs template, but with some changes:

  • Removed unused dependencies
  • Replaced failure (deprecated) with thiserror
  • Some small refactoring

Closes #102

@AzureMarker AzureMarker requested review from fzzzy, jrconlin and pjenvey June 4, 2020 19:01
Copy link
Member

@jrconlin jrconlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I'll approve after you deal with the merge conflict.

@AzureMarker AzureMarker requested a review from jrconlin June 5, 2020 17:11
jrconlin
jrconlin previously approved these changes Jun 5, 2020
pjenvey
pjenvey previously approved these changes Jun 5, 2020
Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just make sure to following the commit message format in CONTRIBUTING when squashing (it's missing a final "Closes #102" line)

Internal(String),
}

impl ApiError {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note about our experience w/ a top level ApiError in syncstorage: I'm beginning to wonder if it's really worth having -- it was originally created w/ an older version of actix-web w/ a slightly different Error type.

Being the centralized error, it's nice in that it provides a centralized place for implementing the actix-web ResponseError (that renders the error), and a couple other misc bits.

On the downside it's a little annoying having to convert from say e.g. DbError .into() an ApiError, then .into() again a ResponseError (in some cases we have 3 into()s, the first for the DbErrorKind. granted, async/await might allow more usages of ? which probably avoid these into()s entirely). Identifying certain types of errors at this level is also a bit cumbersome.

Metrics and io Error likely only happen during startup (main)?, so they wouldn't necessarily benefit from ApiError's features.

Could we avoid the ApiError and just have multiple types of errors specific to modules handle rendering themselves (without lots of code duplication everywhere)? Just something to keep in mind going forward: there's possibly room for improvement here.

Copy link
Contributor Author

@AzureMarker AzureMarker Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've addressed the issues you brought up in 18b1063:

  • The ApiError enum is renamed to ApiErrorKind and a new ApiError struct holds both a kind and a backtrace.
  • The number of From/Into conversions can be reduced by using error kind enums for underlying errors (DbErrorKind) and wrapping them in a variant on ApiErrorKind, ex. ApiErrorKind(DbErrorKind). The functionality brought about by DbError is mostly backtrace support, which is already present in ApiError. Additionally, I've added a From impl to ApiError which forwards the From impls from ApiErrorKind:
    // Forward From impls to ApiError from ApiErrorKind. Because From is reflexive,
    // this impl also takes care of From<ApiErrorKind>.
    impl<T> From<T> for ApiError
    where
    ApiErrorKind: From<T>,
    {
    fn from(item: T) -> Self {
    ApiError {
    kind: ApiErrorKind::from(item),
    backtrace: Backtrace::new(),
    }
    }
    }
  • Related to the previous point, in the case where we want to further isolate a database module such that it does not know about ApiError but still needs to act as an HTTP response, we can fallback to using DbError and add a "short-cut" impl: impl From<DbError> for HttpResponse which does the chain of conversion to ApiError and then HttpResponse. This would hide the repeated conversions during normal use.
  • Identifying the type of error is easier now, as you can do a direct match:
    match error.kind {
        ApiErrorKind::Db(DbErrorKind::BatchNotFound) => {
            // ...
        },
        // some other type of error...
    }
  • While the metrics and IO errors may only happen during startup, I don't see how they are disadvantaged by using the ApiError type. We could remove them from the list of errors and handle them explicitly, but that does not affect the decision to use a top level error or not. Additionally, using ApiError allows us to handle a function which could return either error type, such as Server::with_settings (and we have backtrace support, which Box<dyn Error> does not necessarily have).

If we do use multiple root-level error types instead of one, I would be concerned in two ways:

  • Their rendering may not be the same, which would be bad for API consumers. For example, if DB errors had a different schema compared to logic errors.
  • At some point we would likely have to handle two different error types in the same function. How should we return the error? As a Box<dyn Error>? This is worse than using a single root-level error type.

Let me know what you think of this, and if I missed anything.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a "short-cut" impl: impl From for HttpResponse which does the chain of conversion to ApiError and then HttpResponse. This would hide the repeated conversions during normal use.

👍👍👍

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

match error.kind {
ApiErrorKind::Db(DbErrorKind::BatchNotFound) => {

This helped me to understand why people use the ApiError/ApiErrorKind division. Thanks.

use serde::ser::SerializeMap;
use serde::{Serialize, Serializer};
use std::convert::From;
use thiserror::Error;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

ApiError has a display impl which prints out the kind, backtrace, and
source error chain.
@AzureMarker AzureMarker dismissed stale reviews from pjenvey and jrconlin via 18b1063 June 8, 2020 14:57
@AzureMarker AzureMarker merged commit e31ea1d into master Jun 8, 2020
@AzureMarker AzureMarker deleted the feat/autoendpoint branch June 8, 2020 20:47
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

Successfully merging this pull request may close these issues.

Setup endpoint crate structure and required dependencies
4 participants