Improved error handling for thrift apiserver

Summary:
At From<Error>::from errors are automatically downcast into an InternalError. While we unwrap them for http this was not done for thrift, resulting in an InternalError being returned for everything. By unwrapping the error first thrift now returns more meaningful errors.

In addition I added a new errorkind for ls_v2, this is purely for internal structure as the client will still see an InvalidInput error.

Reviewed By: StanislavGlebik

Differential Revision: D13881374

fbshipit-source-id: bd2e43a0ac7e5abdbf41068c3b1ab37681f03009

apiserver/src/actor/repo.rs

@@ -220,7 +220,7 @@ impl MononokeRepo {
         api::get_content_by_path(ctx, repo, changesetid, mpath)
             .and_then(move |content| match content {
                 Content::Tree(tree) => Ok(tree),
-                _ => Err(ErrorKind::InvalidInput(path.to_string(), None).into()),
+                _ => Err(ErrorKind::NotADirectory(path.to_string()).into())
             })
             .map(|tree| {
                 tree.list()

apiserver/src/errors.rs

@@ -42,6 +42,7 @@ pub enum ErrorKind {
     InvalidInput(String, Option<Error>),
     InternalError(Error),
     LFSNotFound(String),
+    NotADirectory(String),
 }
 
 impl ErrorKind {
@@ -53,6 +54,7 @@ impl ErrorKind {
             InvalidInput(..) => StatusCode::BAD_REQUEST,
             InternalError(_) => StatusCode::INTERNAL_SERVER_ERROR,
             LFSNotFound(_) => StatusCode::NOT_FOUND,
+            NotADirectory(_) => StatusCode::BAD_REQUEST,
         }
     }
 
@@ -61,7 +63,7 @@ impl ErrorKind {
         use errors::ErrorKind::*;
 
         match &self {
-            NotFound(..) | InvalidInput(..) | InternalError(_) => {
+            NotFound(..) | InvalidInput(..) | InternalError(_) | NotADirectory(_) => {
                 ErrorResponse::APIErrorResponse(APIErrorResponse {
                     message: self.to_string(),
                     causes: self
@@ -76,6 +78,19 @@ impl ErrorKind {
             }),
         }
     }
+    // Since all non-ErrorKind error including `Context<ErrorKind>` is wrapped in `InternalError`
+    // automatically at `From<Error>::from`, we need to downcast the `Context` retrieve the
+    // `ErrorKind` in the `Context`.
+    fn unwrap_errorkind(&self) -> &Self {
+        match self {
+            ErrorKind::InternalError(err) => err_downcast_ref! {
+                err,
+                err: ErrorKind => err,
+            }
+            .unwrap_or(self),
+            _ => self,
+        }
+    }
 }
 
 impl Fail for ErrorKind {
@@ -85,7 +100,7 @@ impl Fail for ErrorKind {
         match self {
             NotFound(_, cause) | InvalidInput(_, cause) => cause.as_ref().map(|e| e.as_fail()),
             InternalError(err) => Some(err.as_fail()),
-            LFSNotFound(_) => None,
+            LFSNotFound(_) | NotADirectory(_) => None,
         }
     }
 }
@@ -99,26 +114,14 @@ impl fmt::Display for ErrorKind {
             InvalidInput(_0, _) => write!(f, "{} is invalid", _0),
             InternalError(_0) => write!(f, "internal server error: {}", _0),
             LFSNotFound(_0) => write!(f, "{} is not found on LFS request", _0),
+            NotADirectory(_0) => write!(f, "{} is not a directory", _0),
         }
     }
 }
 
 impl ResponseError for ErrorKind {
-    // Since all non-ErrorKind error including `Context<ErrorKind>` is wrapped in `InternalError`
-    // automatically at `From<Error>::from`, we need to downcast the `Context` retrieve the
-    // `ErrorKind` in the `Context`.
     fn error_response(&self) -> HttpResponse {
-        let err = {
-            match self {
-                ErrorKind::InternalError(err) => err_downcast_ref! {
-                    err,
-                    err: ErrorKind => err,
-                }
-                .unwrap_or(self),
-                _ => self,
-            }
-        };
-
+        let err = self.unwrap_errorkind();
         HttpResponse::build(err.status_code()).json(err.into_error_response())
     }
 }
@@ -199,7 +202,7 @@ impl From<ErrorKind> for MononokeAPIException {
     fn from(e: ErrorKind) -> MononokeAPIException {
         use errors::ErrorKind::*;
 
-        match e {
+        match e.unwrap_errorkind() {
             e @ NotFound(..) => MononokeAPIException {
                 kind: MononokeAPIExceptionKind::NotFound,
                 reason: e.to_string(),
@@ -216,6 +219,10 @@ impl From<ErrorKind> for MononokeAPIException {
                 kind: MononokeAPIExceptionKind::NotFound,
                 reason: e.to_string(),
             },
+            e @ NotADirectory(_) => MononokeAPIException {
+                kind: MononokeAPIExceptionKind::InvalidInput,
+                reason: e.to_string(),
+            },
         }
     }
 }

tests/integration/test-apiserver.t

@@ -205,7 +205,7 @@ test nonexist fold
 
 test list a file
   $ sslcurl -w "\n%{http_code}" $APISERVER/repo/list/$COMMIT2/test-rename | extract_json_error
-  test-rename is invalid
+  test-rename is not a directory
   400
 
 test get blob by hash