refactor(errors): use failure crate to handle errors #85

brizental · 2018-06-19T15:39:32Z

Relates to #48

This is still a work in progress, but you guys can already have a look at how this would work out.

vladikoff · 2018-06-19T19:09:37Z

src/error.rs

+        let mut builder = Response::build_from(json.respond_to(request)?);
+        builder.status(status).ok()
+    }
+}


new line end of file

vladikoff · 2018-06-19T19:14:26Z

src/error.rs

+
+    /// 500 Internal Server Error
+    #[fail(display = "Internal Server Error")]
+    InternalServerError,


Is there a way to provide detailed logging in DEV mode for InternalServerErrors?

Yes, that's what I'm going for :)

vladikoff · 2018-06-19T19:15:10Z

src/send/test.rs

 use auth_db::DbClient;
 use bounces::Bounces;
 use message_data::MessageData;
+use error::{HandlerErrorKind};


do we need alphabetical order for this?

This is still WIP, so I haven't use cargo fmt -- --check yet, that will tell me about this kind of formatting errors.

vladikoff · 2018-06-19T19:16:06Z

src/error.rs

+    InternalError,
+
+    #[fail(display = "Unexpected rocket error: {:?}", _0)]
+    RocketError(rocket::Error), // rocket::Error isn't a std Error (so no #[cause])


~~a std Error -> an std Error~~

My mistake, a standard Error is right 👍

vladikoff · 2018-06-19T19:16:47Z

src/error.rs

+
+#[derive(Clone, Eq, PartialEq, Debug, Fail)]
+pub enum HandlerErrorKind {
+    /// 400 Bad Request


do we need to handle 401s ? (401 unauthorized)

Not yet, there's no authentication in the first cut of this service, so we never return a 401.

vladikoff · 2018-06-19T19:20:59Z

src/error.rs

+        let status = self.kind().http_status();
+        let log = MozlogLogger::with_request(request).map_err(|_| Status::InternalServerError)?;
+        let kind = self.kind();
+        slog_error!(log, "{}", &self; "status_code" => status.code,  "status_msg" => status.reason);


Based on

- status, - error: error.to_string(), - errno: None, - message: None, - data: None,

Can we provide errno and maybe other fields? I think that's the field format that we should into mozlog

...and if we do do that (which I agree with), does it make sense to do it via {:?} / implementing fmt:::Debug?

I also wonder if, for certain kinds of error, we should include inner.backtrace() in the debug output, as that could be one way to address https://github.com/mozilla/fxa-email-service/pull/85/files#r196545966 (although maybe not the optimal way).

philbooth · 2018-06-20T04:36:21Z

src/error.rs

+}
+
+#[derive(Clone, Eq, PartialEq, Debug, Fail)]
+pub enum HandlerErrorKind {


It's no biggie but fwiw I'm not a huge fan of the ...Kind suffix in this name. Is it a Rust convention or just something we opted for ourselves?

Hm, I followed this tutorial from the failure book -> https://boats.gitlab.io/failure/error-errorkind.html

It's the naming convention that they use there, but I guess we could name it something we think is more semantic. What did you have in mind?

No worries, if it's an idiomatic Rust thing I'm happy!

philbooth · 2018-06-20T04:38:27Z

src/error.rs

+
+use logging::MozlogLogger;
+
+pub type HandlerResult<T> = result::Result<T, HandlerError>;


Does Result need to be namespace-qualified here? Isn't it included in the standard prelude?

philbooth · 2018-06-20T04:43:03Z

src/send/mod.rs

+fn fail() -> data::Outcome<Email, HandlerError> {
+    Outcome::Failure((
+        Status::BadRequest, 
+        HandlerErrorKind::InvalidEmailAddress.into()


This probably shouldn't be hard-coded at this point because fail is also called if the provider param is invalid. And maybe it will be called for other params in the future, too. Let's either rework the outer logic so the HandlerErrorKind can be passed in as an argument, or keep that the same and change this to a more generic validation error that doesn't mention email addresses.

philbooth · 2018-06-20T04:47:11Z

src/app_errors/mod.rs

-impl ApplicationError {
-    pub fn new(status: u16, error: &str) -> ApplicationError {
-        // TODO: Set errno, message and data when rocket#596 is resolved
-        //       (https://github.com/SergioBenitez/Rocket/issues/596)


I'd like it if we could keep some form of this TODO comment in the code, as a visible reminder that we have outstanding work to do in order to make the errors from this service match our other errors (and what was specced out in the feature doc).

Presumably that just means moving it down to the HandlerError definition. If we do that maybe we should also keep the placeholders for errno, message and data there too, because we know we have to implement those.

philbooth · 2018-06-20T04:54:42Z

src/error.rs

+pub type HandlerResult<T> = result::Result<T, HandlerError>;
+
+#[derive(Debug)]
+pub struct HandlerError {


Fwiw, I also preferred the name ApplicationError to HandlerError, although I'm bike-shedding now I realise.

The idea behind the name ApplicationError was that they're errors at the API level, that the application presents to the outside world. As such, they're part of the documented interface every bit as much as the params to /send are.

Changing it to HandlerError makes that less clear, I think.

(and if we do go back to the name ApplicationError, this stuff should probably stay in app_errors/mod.rs)

I agree with going back to ApplicationError now that I think about it, It is is more semantic.

I called them HandlerError/Result because ultimately rocket handlers return them as HTTP responses (they impl. rocket's Responder). Though maybe one day rocket's request guards will also render them as Responders for us (Rocket#596) -- TBD what that will ultimately look like.

If you really want to change it I'd recommend a shorter "AppError". We'll probably just stick w/ "Handler" in megaphone/pushbox until Rocket#596 shakes out

brizental · 2018-06-23T20:52:18Z

Closing this in favor of #94

@pjenvey

@pjenvey Fixes #48 Fixes #77 I had to change basically everything since my last PR (#85) about this, so I thought it would be best to just create a new PR. To get the failure errors working I needed to get at least providers, bounces and db to also work with the failure errors, not just the Rocket and HTTP errors. That's what I'm doing in this PR. Since this is kind of a big change, I thought it would be best to send this in before adapting the tests to the new error type, so, right now, many tests are commented. Let me know what you guys think. If you think this is a good change I still would need to adapt all tests and also there are some error types specially for the queues bin that need to be changed as well. I personally think this is a good change. It is abstracting all the errors to one single place in the code. While working on this refactoring, I saw a bunch of repeated code for error handling. This ends that, which means it will be easier to maintain and create new error types with this way of doing things. Also, we are now getting much more information about each error, not just the HTTP status and message and it's very easy to customize this even further.

refactor(errors): use failure crate to handle errors

0117152

brizental added the WIP label Jun 19, 2018

vladikoff reviewed Jun 19, 2018

View reviewed changes

philbooth reviewed Jun 20, 2018

View reviewed changes

brizental mentioned this pull request Jun 23, 2018

refactor(errors): use failure crate to handle errors #94

Merged

brizental closed this Jun 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(errors): use failure crate to handle errors #85

refactor(errors): use failure crate to handle errors #85

brizental commented Jun 19, 2018

vladikoff Jun 19, 2018

vladikoff Jun 19, 2018

brizental Jun 20, 2018

vladikoff Jun 19, 2018

brizental Jun 20, 2018

vladikoff Jun 19, 2018 •

edited

Loading

vladikoff Jun 20, 2018

vladikoff Jun 19, 2018

philbooth Jun 20, 2018

vladikoff Jun 19, 2018

philbooth Jun 20, 2018

philbooth Jun 20, 2018

brizental Jun 20, 2018

philbooth Jun 20, 2018

philbooth Jun 20, 2018

philbooth Jun 20, 2018

philbooth Jun 20, 2018

philbooth Jun 20, 2018

philbooth Jun 20, 2018

philbooth Jun 20, 2018

brizental Jun 20, 2018

pjenvey Jun 22, 2018

brizental commented Jun 23, 2018 •

edited

Loading


		use logging::MozlogLogger;

		pub type HandlerResult<T> = result::Result<T, HandlerError>;

refactor(errors): use failure crate to handle errors #85

refactor(errors): use failure crate to handle errors #85

Conversation

brizental commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladikoff Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brizental commented Jun 23, 2018 • edited Loading

vladikoff Jun 19, 2018 •

edited

Loading

brizental commented Jun 23, 2018 •

edited

Loading