feat: Return detailed autoendpoint errors (#170)

* Return an option in get_user to better handle the missing user case

* Add errno values for error kinds

* Add ApiErrorKind::errno

* Use the original error schema and return detailed error responses

Closes #159

autoendpoint/src/error.rs

@@ -18,8 +18,8 @@ use thiserror::Error;
 /// Common `Result` type.
 pub type ApiResult<T> = Result<T, ApiError>;
 
-/// How long the client should wait before retrying a conflicting write.
-pub const RETRY_AFTER: u8 = 10;
+/// A link for more info on the returned error
+const ERROR_URL: &str = "http://autopush.readthedocs.io/en/latest/http.html#error-codes";
 
 /// The main error type.
 #[derive(Debug)]
@@ -74,6 +74,9 @@ pub enum ApiErrorKind {
     #[error("Invalid token")]
     InvalidToken,
 
+    #[error("UAID not found")]
+    NoUser,
+
     #[error("No such subscription")]
     NoSubscription,
 
@@ -103,7 +106,7 @@ impl ApiErrorKind {
             | ApiErrorKind::TokenHashValidation(_)
             | ApiErrorKind::Uuid(_) => StatusCode::BAD_REQUEST,
 
-            ApiErrorKind::NoSubscription => StatusCode::GONE,
+            ApiErrorKind::NoUser | ApiErrorKind::NoSubscription => StatusCode::GONE,
 
             ApiErrorKind::VapidError(_) | ApiErrorKind::Jwt(_) => StatusCode::UNAUTHORIZED,
 
@@ -117,6 +120,23 @@ impl ApiErrorKind {
             | ApiErrorKind::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR,
         }
     }
+
+    /// Get the associated error number
+    pub fn errno(&self) -> Option<usize> {
+        match self {
+            ApiErrorKind::InvalidEncryption(_) => Some(110),
+            ApiErrorKind::VapidError(_)
+            | ApiErrorKind::TokenHashValidation(_)
+            | ApiErrorKind::Jwt(_) => Some(109),
+            ApiErrorKind::InvalidToken => Some(102),
+            ApiErrorKind::NoUser => Some(103),
+            ApiErrorKind::NoSubscription => Some(106),
+            ApiErrorKind::PayloadError(PayloadError::Overflow)
+            | ApiErrorKind::PayloadTooLarge(_) => Some(104),
+            ApiErrorKind::Internal(_) => Some(999),
+            _ => None,
+        }
+    }
 }
 
 // Print out the error and backtrace, including source errors
@@ -174,40 +194,23 @@ impl From<ApiError> for HttpResponse {
 
 impl ResponseError for ApiError {
     fn error_response(&self) -> HttpResponse {
-        // To return a descriptive error response, this would work. We do not
-        // unfortunately do that so that we can retain Sync 1.1 backwards compatibility
-        // as the Python one does.
-        // HttpResponse::build(self.status).json(self)
-        //
-        // So instead we translate our error to a backwards compatible one
-        HttpResponse::build(self.kind.status())
-            .header("Retry-After", RETRY_AFTER.to_string())
-            .finish()
+        HttpResponse::build(self.kind.status()).json(self)
     }
 }
 
-// TODO: Use the same schema as documented here?
-//       https://autopush.readthedocs.io/en/latest/http.html#response
 impl Serialize for ApiError {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
     where
         S: Serializer,
     {
         let status = self.kind.status();
-        let size = if status == StatusCode::UNAUTHORIZED {
-            2
-        } else {
-            3
-        };
-
-        let mut map = serializer.serialize_map(Some(size))?;
-        map.serialize_entry("status", &status.as_u16())?;
-        map.serialize_entry("reason", status.canonical_reason().unwrap_or(""))?;
-
-        if status != StatusCode::UNAUTHORIZED {
-            map.serialize_entry("errors", &self.kind.to_string())?;
-        }
+        let mut map = serializer.serialize_map(Some(5))?;
 
+        map.serialize_entry("code", &status.as_u16())?;
+        map.serialize_entry("errno", &self.kind.errno())?;
+        map.serialize_entry("error", &status.canonical_reason())?;
+        map.serialize_entry("message", &self.kind.to_string())?;
+        map.serialize_entry("more_info", ERROR_URL)?;
         map.end()
     }
 }

autoendpoint/src/server/extractors/subscription.rs

@@ -63,7 +63,8 @@ impl FromRequest for Subscription {
                 .get_user(&uaid)
                 .compat()
                 .await
-                .map_err(ApiErrorKind::Database)?;
+                .map_err(ApiErrorKind::Database)?
+                .ok_or(ApiErrorKind::NoUser)?;
             validate_user(&user, &channel_id, &state).await?;
 
             // Validate the VAPID JWT token and record the version

autopush-common/src/db/mod.rs

@@ -443,17 +443,17 @@ impl DynamoStorage {
         })
     }
 
-    pub fn get_user(&self, uaid: &Uuid) -> impl Future<Item = DynamoDbUser, Error = Error> {
+    pub fn get_user(&self, uaid: &Uuid) -> impl Future<Item = Option<DynamoDbUser>, Error = Error> {
         let ddb = self.ddb.clone();
         let result = commands::get_uaid(ddb, uaid, &self.router_table_name).and_then(|result| {
             future::result(
                 result
                     .item
-                    .ok_or_else(|| "No user record found".into())
-                    .and_then(|item| {
+                    .map(|item| {
                         let user = serde_dynamodb::from_hashmap(item);
                         user.chain_err(|| "Error deserializing")
-                    }),
+                    })
+                    .transpose(),
             )
         });
         Box::new(result)

autopush/src/client.rs

@@ -693,6 +693,11 @@ fn save_and_notify_undelivered_messages(
                 srv2.ddb.get_user(&uaid)
             })
             .and_then(move |user| {
+                let user = match user.ok_or_else(|| "No user record found".into()) {
+                    Ok(user) => user,
+                    Err(e) => return future::err(e),
+                };
+
                 // Return an err to stop processing if the user hasn't reconnected yet, otherwise
                 // attempt to construct a client to make the request
                 if user.connected_at == connected_at {