Repository: graphql-rust/juniper (language: rust)

Issue:
Complex Errors
As I can see, right now, only a simple String error for the `message` field is supported.

I often need more complex errors with additional data, like this for example:

```json
"errors": [
  {
    "message": "Validation failed", 
    "code": "validation_err",
    "data": {
      "username": {"code": "min_length", "message": "must be at least 5 characters long", "meta": {"min": 5, "max": 15}}
    }
  }
]
```

Do you see a way to implement this?

It would require changing the type of `FieldResult`.

As an idea:

```rust
pub enum FieldError {
  Simple(String),
  Custom(String, json_value::Value),
}

type FieldResult<T> = Result<T, FieldError>;
```

The problem is the type of the extra data.
It would require either having FieldError be generic over a type that implements `Serialize` (for serde) or tying it to a value type like [serde_value](https://github.com/arcnmx/serde-value) or `json_value::Value`.


Discussion / hints:
Hmm, I think I have an idea on how to implement this if @mhallin doesn't disapprove I'll try to write a PR for it in the next couple of days.
Errors in GraphQL are tricky! I think a clean way of handling failing mutations is one of the less thought-out areas of the whole standard. On the other hand - it might be hard to standardize this on the GraphQL level.

What I've been doing - both in Juniper and when using other systems - is to have the mutation return two fields: a nullable result object and a nullable list of error objects which usually contain a field enum and a message.

This way circumvents the "standard" GraphQL error system for better or worse. An upside is that the errors are actually documented using the same introspective schema like the rest of the system, so you can actually model the error handling pretty rigidly.

That being said, all of these points only cover failing mutations so I'm interested in hearing your proposed solution.
I've been doing some research to ensure my initial idea was a good one. However my idea doesn't agree with the current best practice which is further discussed [here](https://github.com/graphql/graphql-js/issues/560). More specifically the best practice appears to be include an error field within your response object. For example, in this [issue](https://github.com/facebook/graphql/issues/117#issuecomment-170180628) here it was mentioned that Facebook has something like: 

```
mutation {
   signUp(email: "hello@atfakeemail.com", password: "thisbetterbeencrypted") {
       # bool
       didSucceed, 
       errors {
         # fields 
      }
   }
}
```
In regards to say failing incorrect user input in a query I would suggest to apply the same thing but the  expected data is a nullable object and a nullable error object as well. However, the fact is I haven't found any direct confirmation that this is the case, maybe I should get a key for the Facebook api and have look around, but I digress. Ultimately, by using this approach one can still generally apply the "rules" of presenting an error. These "[rules](http://facebook.github.io/graphql/#sec-Errors)" being: 

1) If no errors were encountered during the requested operation, the errors entry should not be present in the result.
2) If the data entry in the response is null or not present, the errors entry in the response must not be empty. It must contain at least one error. The errors it contains should indicate why no data was able to be returned.
3) If the data entry in the response is not null, the errors entry in the response may contain any errors that occurred during execution. If errors occurred during execution, it should contain those errors.

Now this being said it is within the specs to "extend" the error section with our own errors which was my initial idea or at least that is how I interpreted [7.2.2](https://facebook.github.io/graphql/#sec-Errors). However, it is noted that at any time the specs could change to include a error that we are using and therefore break any custom errors that were added.

Now that I have read a little bit more into the execution errors it makes sense to treat them as developer only errors and not error that need to be revealed to client. 

Anyways, I was hoping to get some input before coding anything up. 
My 2 cents...it seems weird to use errors in the response, especially as Relay appears to use the errors entry last I looked at it (https://github.com/facebook/relay/issues/696). Using errors in the response would mean never using `onMutationFailure` in that example, which means my client code would be duplicating a (small) part of Relay's error-handling logic for seemingly no gain.

But, as @mhallin points out the introspection benefits of errors in the response are hard to ignore.

Perhaps `juniper` can differentiate between `ServerError` and `ApplicationError`, the former using the error entry and the latter having some nice ergonomics for adding to the payload, optionally also setting the error entry to trigger things like Relay's error handler?
I see... now I am getting why @mhallin said this was tricky! It appears that both ways are encouraged for now as indicated [here](https://github.com/facebook/relay/issues/1141). So I think the only requirements for errors is that they must contain the message field, and an optional location field which is going to be tricky to enforce but should be possible to do. 
I don't really see the big benefit of embedding the errors in the data.

It would be useful if errors were typed, but they are polymorphic, so all I would end up with is 

```
type Error {
  code: String!
  message: String!
  data: JSON! // Custom scalar type...
}
```

Which doesn't really help much compared to just having them in err.

Errors in the data are also prone to break assumptions, for example: javascript client library that rejects a Promise if err is in the response.

So I'd definitely petition for the possibility of complex errors in `err`.
[This comment](https://github.com/graphql/graphql-js/issues/560#issuecomment-259508214) on graphql-js, and [this one](https://github.com/facebook/graphql/issues/117#issuecomment-170180628) on the GraphQL spec itself (that TheServerAsterisk linked above) summarize my view on error handling in GraphQL.

It all depends on how you imagine the error handling should work. For e.g. sign up/log in mutations, you want to include all errors, their fields, and a human-readable string that goes with each field. For these cases, I've been using a schema that looks kind of like:

```graphql
type MutationType {
  signUpUser(email: String!, username: String!, password: String!): SignUpResult
}

type SignUpResult {
  user: User
  errors: [AuthFieldError]
}

type AuthFieldError {
  field: AuthField!
  message: String!
}

enum AuthField {
  USERNAME
  PASSWORD
  EMAIL
}
```

Note that this schema does _not_ include data for e.g. database or other internal errors. For those kinds of errors, or for mutations that are expected to always work, I just use the regular GraphQL error handling with a simple message.

What's the benefit of having a schema for the errors? I would argue that it's the same benefit of having a schema for the "regular" data in the first place :) You can have your error views be driven by GraphQL queries just like your other views, reason about types and state just like you would with the "successful" data. I think handling of expected errors should be just as robust as the rest of the application.

---

All that being said, I'm definitely not _opposed_ to the idea of being able to attach more data to the `FieldError` type. I just want to know the intended use case before we start thinking how to implement it.
I dug into the relay code it appears that they are using hasOwnProperty("errors") as a means to check whether the query/mutation was successful or not see [here](https://github.com/facebook/relay/blob/2c096f75fe64dc6336872275f3c96907bf6a3975/packages/react-relay/classic/network-layer/default/RelayDefaultNetworkLayer.js#L46).

Furthermore, it does seem like some real world examples are not using the error field within a GraphQl Object. For example, Github's GraphQL will return a standard execution error with an added type field. Don't have facebook so I can't confirm exactly how it is. That being said I just found a publically facing GraphQl API converted a JSON object into a string and shoved it into the message field. So this isn't a consistent thing between implementations or the individuals developing with such said tool. 

However, I don't think the use case is in dispute here because ultimately whether we use the way @mhallin suggested or we allow custom errors through the FieldResult they are both attempting to create client friendly errors (as far I can tell), what is in my opinion at dispute is how errors of this nature are to be treated whether it be part of a "successful" payload or an "erroneous" payload. Even though I agree with the way @mhallin has suggested achieving this task I would still suggest that this should be the end-users choice so long GraphQl spec allow the behavior. 
This should be figured out with / before 0.9, since it might require a breaking change.
One possible solution to this and #69 would be to make `FieldError` a struct like:

```rust
struct FieldError {
  message: String,
  extra_data: Value,
}

impl<E> From<E> for FieldError where E: Display {
  fn from(err: E) -> FieldError {
    FieldError { message: format!("{}", err), extra_data: Value::null() }
  }
}

impl FieldError {
  pub fn new<E: Display>(err: E, extra_data: Value) {
    FieldError { message: format!("{}", err), extra_data: extra_data }
  }
```

Then we could append or include the extra data into the error object when serializing it. Now you could do either the following:

```rust
let txn = executor.context().db.transaction()?;
// Would result in { "message": "<internal db error>", "locations": [...] }

// or
let txn = executor.context().db.transaction().map_err(
  |e| FieldError::new(e, Value::string("Could not start transaction"))?;
// Would result in { "message": "<internal db error>", "locations": [...], "data": "Could not start transaction" }
```

(With reservations for not having checked that the code actually compiles :)
Being able to define custom error types for production GraphQL usage is critical. In other systems that I maintain, where GraphQL is used as the frontend to all of the backend gRPC microservices, there is a contract defining what our error types will look like for when errors are returned.

According to [the GraphQL spec's description of errors](http://facebook.github.io/graphql/October2016/#sec-Errors), having the `message` field present is baseline (and quite logical). Perhaps defining an interface which will allow a higher-level handler in Juniper to take and serialize custom errors according to that interface would be a nice pattern.

Fix patch:
diff --git a/changelog/master.md b/changelog/master.md
index 5dbb81382..fdaac4cdd 100644
--- a/changelog/master.md
+++ b/changelog/master.md
@@ -28,3 +28,9 @@
   `#[graphql(description = "my description")]`.
 
   [#194](https://github.com/graphql-rust/juniper/issues/194)
+
+* Introduced `IntoFieldError` trait to allow custom error handling
+  i.e. custom result type. The error type must implement this trait resolving
+  the errors into `FieldError`.
+
+  [#40](https://github.com/graphql-rust/juniper/issues/40)
diff --git a/juniper/src/executor/mod.rs b/juniper/src/executor/mod.rs
index eb879be92..92795fff2 100644
--- a/juniper/src/executor/mod.rs
+++ b/juniper/src/executor/mod.rs
@@ -193,6 +193,21 @@ pub type ExecutionResult = Result<Value, FieldError>;
 /// The map of variables used for substitution during query execution
 pub type Variables = HashMap<String, InputValue>;
 
+/// Custom error handling trait to enable Error types other than `FieldError` to be specified
+/// as return value.
+///
+/// Any custom error type should implement this trait to convert it to `FieldError`.
+pub trait IntoFieldError {
+    #[doc(hidden)]
+    fn into_field_error(self) -> FieldError;
+}
+
+impl IntoFieldError for FieldError {
+    fn into_field_error(self) -> FieldError {
+        self
+    }
+}
+
 #[doc(hidden)]
 pub trait IntoResolvable<'a, T: GraphQLType, C>: Sized {
     #[doc(hidden)]
@@ -208,12 +223,13 @@ where
     }
 }
 
-impl<'a, T: GraphQLType, C> IntoResolvable<'a, T, C> for FieldResult<T>
+impl<'a, T: GraphQLType, C, E: IntoFieldError> IntoResolvable<'a, T, C> for Result<T, E>
 where
     T::Context: FromContext<C>,
 {
     fn into(self, ctx: &'a C) -> FieldResult<Option<(&'a T::Context, T)>> {
         self.map(|v| Some((FromContext::from(ctx), v)))
+            .map_err(|e| e.into_field_error())
     }
 }
 
diff --git a/juniper/src/lib.rs b/juniper/src/lib.rs
index 356f02f33..f3a75fbbd 100644
--- a/juniper/src/lib.rs
+++ b/juniper/src/lib.rs
@@ -152,7 +152,7 @@ use validation::{validate_input_values, visit_all_rules, ValidatorContext};
 
 pub use ast::{FromInputValue, InputValue, Selection, ToInputValue, Type};
 pub use executor::{Context, ExecutionError, ExecutionResult, Executor, FieldError, FieldResult,
-                   FromContext, IntoResolvable, Registry, Variables};
+                   FromContext, IntoResolvable, Registry, Variables, IntoFieldError};
 pub use executor::{Applies, LookAheadArgument, LookAheadSelection, LookAheadValue, LookAheadMethods};
 pub use schema::model::RootNode;
 pub use types::base::{Arguments, GraphQLType, TypeKind};


Test patch:
diff --git a/juniper/src/executor_tests/executor.rs b/juniper/src/executor_tests/executor.rs
index 7da8fb0fa..95e03fe5a 100644
--- a/juniper/src/executor_tests/executor.rs
+++ b/juniper/src/executor_tests/executor.rs
@@ -676,7 +676,7 @@ mod dynamic_context_switching {
 }
 
 mod propagates_errors_to_nullable_fields {
-    use executor::{ExecutionError, FieldError, FieldResult};
+    use executor::{ExecutionError, FieldError, FieldResult, IntoFieldError};
     use parser::SourcePosition;
     use schema::model::RootNode;
     use types::scalars::EmptyMutation;
@@ -685,6 +685,23 @@ mod propagates_errors_to_nullable_fields {
     struct Schema;
     struct Inner;
 
+    enum CustomError {
+        NotFound
+    }
+
+    impl IntoFieldError for CustomError {
+        fn into_field_error(self) -> FieldError {
+            match self {
+                CustomError::NotFound => FieldError::new(
+                    "Not Found",
+                    graphql_value!({
+                        "type": "NOT_FOUND"
+                    })
+                )
+            }
+        }
+    }
+
     graphql_object!(Schema: () |&self| {
         field inner() -> Inner { Inner }
         field inners() -> Vec<Inner> { (0..5).map(|_| Inner).collect() }
@@ -696,6 +713,7 @@ mod propagates_errors_to_nullable_fields {
         field non_nullable_field() -> Inner { Inner }
         field nullable_error_field() -> FieldResult<Option<&str>> { Err("Error for nullableErrorField")? }
         field non_nullable_error_field() -> FieldResult<&str> { Err("Error for nonNullableErrorField")? }
+        field custom_error_field() -> Result<&str, CustomError> { Err(CustomError::NotFound) }
     });
 
     #[test]
@@ -747,6 +765,29 @@ mod propagates_errors_to_nullable_fields {
         );
     }
 
+    #[test]
+    fn custom_error_first_level() {
+        let schema = RootNode::new(Schema, EmptyMutation::<()>::new());
+        let doc = r"{ inner { customErrorField } }";
+
+        let vars = vec![].into_iter().collect();
+
+        let (result, errs) = ::execute(doc, None, &schema, &vars, &()).expect("Execution failed");
+
+        println!("Result: {:?}", result);
+
+        assert_eq!(result, graphql_value!(None));
+
+        assert_eq!(
+            errs,
+            vec![ExecutionError::new(
+                SourcePosition::new(10, 0, 10),
+                &["inner", "customErrorField"],
+                FieldError::new("Not Found", graphql_value!({ "type": "NOT_FOUND" })),
+            )]
+        );
+    }
+
     #[test]
     fn nullable_nested_level() {
         let schema = RootNode::new(Schema, EmptyMutation::<()>::new());