Complex Errors about juniper HOT 11 CLOSED

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, 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.

from juniper.

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.

from juniper.

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.

from juniper.

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. More specifically the best practice appears to be include an error field within your response object. For example, in this issue here it was mentioned that Facebook has something like:

mutation {
   signUp(email: "[email protected]", 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" 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. 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.

from juniper.

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 (facebook/relay#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?

from juniper.

I see... now I am getting why @mhallin said this was tricky! It appears that both ways are encouraged for now as indicated here. 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.

from juniper.

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.

from juniper.

This comment on graphql-js, and this one 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:

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.

from juniper.

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.

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.

from juniper.

This should be figured out with / before 0.9, since it might require a breaking change.

from juniper.

One possible solution to this and #69 would be to make FieldError a struct like:

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:

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 :)

from juniper.