facebook / relay

Relay is a JavaScript framework for building data-driven React applications.
https://relay.dev
MIT License
18.37k stars 1.82k forks source link

GraphQL error handling in Relay Modern #1913

Open taion opened 7 years ago

taion commented 7 years ago

I think error handling is a little bit weird.

Contra the example in https://facebook.github.io/relay/docs/network-layer.html, it looks like I need to actually throw an Error in the event that there are any query errors. I can't just pass through the graphql object with the errors property.

This is because in https://github.com/facebook/relay/blob/v1.1.0/packages/relay-runtime/network/RelayNetwork.js#L197-L208, normalizePayload only throws an error if data is nully.

But the forward throw branch there is the only code path that will trigger onError for the request, and correspondingly the only path that will hit onError in <QueryRenderer> per https://github.com/facebook/relay/blob/v1.1.0/packages/react-relay/modern/ReactRelayQueryRenderer.js#L205-L212.

Otherwise we hit onNext per https://github.com/facebook/relay/blob/v1.1.0/packages/react-relay/modern/ReactRelayQueryRenderer.js#L229, and will always set error: null on the readyState that gets passed to the render callback.

This is weird. Am I missing something, or are things supposed to work this way?

johntran commented 7 years ago

Looks like there is an error param for the onCompleted callback:

https://github.com/facebook/relay/pull/1938/files

Don't need to throw in the relay network layer anymore.

taion commented 7 years ago

Not quite the issue – the problem is actually in QueryRenderer.

maikelrobier commented 7 years ago

Yeah, I have found that QueryRenderer swallows errors. For instance, If I add throw new Error() inside the render method of a component rendered with QueryRenderer the exception won't be shown anywhere. I don't know if related.

taion commented 7 years ago

That's not what I'm talking about. Of course errors in render get swallowed in some sense – but that's just how React works.

cberkom commented 7 years ago

Agreed, it would be very helpful to have a way to view the GraphQL errors within the QueryRenderer render method.

eliperkins commented 7 years ago

Otherwise we hit onNext per /packages/react-relay/modern/ReactRelayQueryRenderer.js@v1.1.0#L229, and will always set error: null on the readyState that gets passed to the render callback.

This definitely seems like a bug, but I'm not sure what the best resolution is here, API-wise. Should the onNext be passed the error? Or should onError be called on the existence of any data in the errors?

I don't mind submitting a PR to fix it either way, but I'd like some advice on which direction to go in.

DennisKo commented 7 years ago

So can anyone clarify this? Whats the Relay way of handling this?

Is that a bug and Relay is supposed to listen on the 'errors' field of the mutation response? Or are we supposed to throw an Error serverside?

lukewlms commented 7 years ago

Still trying to figure out how to handle "network is unavailable" in Relay without it crashing in production - I have the error in fetchQuery but don't know what to do to get it back to the Component to display. Is everyone here stuck on the same thing or can someone point me to a resource?

Emsu commented 7 years ago

Seems like there's a lot of confusion on what @taion is talking about in this thread.

I think I'm hitting the same issue @taion is and wanted to illustrate with some examples. Basically, I'm passing GraphQL errors back to Relay and they're getting swallowed by QueryRenderer. Something like:

{
  data: {
    viewer: {
      me: null
    }
  },
  errors: [{message: 'Bad error', code: 'some error' ...}]
}

Then in the render function of QueryRenderer I get props like { viewer: {me: null }} and error is null. However, I am expecting to get back the GraphQL errors I passed back.

For the nully data returning a processed version of the error that @taion mentions, if I return:

{
  data: null,
  errors: [{message: 'Bad Error', code: 'some error' ...}]
}

from the server, then I get my error to come through but it gets formatted and aggregated into some Relay Modern error:

{
  name: "RelayNetwork", 
  type: "mustfix",
  framesToPop: 2,
  source: {
    errors: [ Original GraphQL Errors are here ],
    operations: {...},
    variables: {}
  },
  ...
}

So it seems like I can access my errors if I force my data to be nully as @taion mentioned but that definitely feels a bit weird. Also, the GraphQL spec says errors can be returned even when some data can be returned. In their example, one of the elements is missing a name property and an error is returned and name is set to null, but the rest of the graph is returned. http://facebook.github.io/graphql/#sec-Errors

akomm commented 6 years ago

My current workaround until fixed for this is to return explicit data: null in network:

if (result && result.errors) {
  return {data: null, errors: result.errors};
}
renatonmendes commented 6 years ago

I´m having the same problem. I need to query for a item that was deleted from another user at the server side database. I receive node: null at QueryRenderer and no error parameter. Here is the return from GraphQL:

{
  "errors": [
    {
      "message": "ERROR: Id does not contain an id: Admin.UserRecord",
      "locations": [
        {
          "line": 4,
          "column": 3
        }
      ],
      "stack": "Error: ERROR: Id does not contain an id: Admin.UserRecord\n    at Object.getNodeById (D:/myproject/graphql/loaders.js:31:9)\n    at resolve (D:/myproject/graphql/queries.js:72:20)\n    at resolveFieldValueOrError (D:\\9. DEV\\WORKSPACE\\momejected\\node_modules\\graphql\\execution\\execute.js:498:12)\n    at resolveField (D:\\9. DEV\\WORKSPACE\\momejected\\node_modules\\graphql\\execution\\execute.js:462:16)\n    at D:\\9. DEV\\WORKSPACE\\momejected\\node_modules\\graphql\\execution\\execute.js:311:18\n    at Array.reduce (native)\n    at executeFields (D:\\9. DEV\\WORKSPACE\\momejected\\node_modules\\graphql\\execution\\execute.js:308:42)\n    at executeOperation (D:\\9. DEV\\WORKSPACE\\momejected\\node_modules\\graphql\\execution\\execute.js:236:122)\n    at executeImpl (D:\\9. DEV\\WORKSPACE\\momejected\\node_modules\\graphql\\execution\\execute.js:133:26)\n    at execute (D:\\9. DEV\\WORKSPACE\\momejected\\node_modules\\graphql\\execution\\execute.js:102:150)\n    at D:\\9. DEV\\WORKSPACE\\momejected\\node_modules\\express-graphql\\dist\\index.js:166:37\n    at process._tickDomainCallback (internal/process/next_tick.js:129:7)",
      "path": [
        "node"
      ]
    }
  ],
  "data": {
    "node": null
  }
}

Although the client application behavior is OK (no error in client application), there must be a way to check for the GraphQL transaction response. Today QueryRenderer puts nothing on error variable.

How can I get this GraphQL transaction error detected ? Here is my environment code:

import {
  Environment,
  Network,
  RecordSource,
  Store,
} from 'relay-runtime';

const SERVER = 'http://localhost:3000/graphql';

const source = new RecordSource();
const store = new Store(source);

function fetchQuery(operation, variables, cacheConfig, uploadables) {

  return fetch(SERVER, {
    method: 'POST',
    headers: {
          'Accept': 'application/json',
        'Content-type': 'application/json'
    },
    body: JSON.stringify({
      query: operation.text, // GraphQL text from input
      variables,
    }),
  }).then(response => {
    return response.json();
  });
}

const network = Network.create(fetchQuery);

const handlerProvider = null;

const environment = new Environment({
  handlerProvider, // Can omit.
  network,
  store,
});

export default environment;
gigitux commented 6 years ago

@renatonmendes How do you resolve?

renatonmendes commented 6 years ago

I haven´t solved. What I did is to throw an error on server side based on database layer return and on client side I do check for data and errors field on a normal QueryRenderer return. If there is no data, then it´s an error. If the error field contains messages, then its a error. If there is data and error field is empty, then I have a normal operation and I proceed.

gigitux commented 6 years ago

@renatonmendes Thank you for your answer. This is my code:

  return fetch(
    '/api/graphql',
    {
      method: 'POST',
      body: JSON.stringify({
        query: operation.text,
        variables,
      }),
    },
  ).then(response => response.json()).then(responeJson => {
    if (responeJson.errors) {
      return {
        data: null,
        errors: responeJson.errors
      }
    }
    console.log(responeJson)
    return responeJson
  });
}  

But i have on error on TypeError: errors.map is not a function (like if errors would be an array and not object, but if i put that object in array is not a very clean solution)

jkettmann commented 6 years ago

For me setting data to null was not an option, because I needed the partial data and handle the errors somewhere in my components as well. I found a workaround to be able to get partial errors inside my query payload in Relay containers. I basically wanted to be able to create a query like following

query {
  errors {
    message
    path
  }
  someOtherField {
    ...
  }
}

This way the partial errors are accesible inside my components. To accomplish this I created a new error type in the GraphQL schema. I use graphql-js here.

import { GraphQLObjectType, GraphQLString, GraphQLInt, GraphQLList } from 'graphql';
import { globalIdField } from 'graphql-relay';

const errorLocationType = new GraphQLObjectType({
  name: 'ErrorLocation',
  fields: () => ({
    line: { type: GraphQLInt },
    column: { type: GraphQLInt },
  }),
});

const errorType = new GraphQLObjectType({
  name: 'Error',
  description: 'Error type. Not used on server but only on client as a workaround to get access to server errors in client components',
  fields: () => ({
    id: globalIdField('Error'),
    message: {
      type: GraphQLString,
      description: 'The errors message',
    },
    locations: {
      type: new GraphQLList(errorLocationType),
      description: 'The errors locations (probably meaning inside the query)',
    },
    path: {
      type: new GraphQLList(GraphQLString),
      description: 'The errors path (for an error in field "restrictedField" in { catalog: { unit: { restrictedField } } the path would be ["catalog", "unit", "restricedField"]',
    },
});

export default errorType;

I added an errors field on the root query type.

  ...
  fields: () => ({
    ...
    errors: {
      type: new GraphQLList(errorType),
      description: 'This is a list of server errors occurring during a query. It won\'t be set on the server response but inside the fetcher',
    },
    ...
  })
  ...

The errors field does not have a resolver. So errors is always null in the GraphQL response payload. The trick is to fill the errors inside the fetcher as follows.

async fetch(operation, variables) {
  const response = await fetch(this.url, {
    method: 'POST',
    credentials: 'same-origin',
    headers: {
      'Content-Type': 'application/json',
      ...this.headers,
    },
    body: JSON.stringify({ query: operation.text, variables }),
  });

  const json = await response.json();
  if (json && json.errors && json.data) {
    json.data = Object.assign({}, json.data, { errors: json.errors});
  }
  return json;
}

You can query the errors field now in any component. For testing you can add a type like following to your schema, where one field cannot be resolved due to some error.

import { GraphQLObjectType, GraphQLString } from 'graphql';
import { globalIdField } from 'graphql-relay';
import { UserError } from 'graphql-errors';

import errorType from './errorType';

const authorizedTestType = new GraphQLObjectType({
  name: 'AuthorizedTest',
  description: 'Type to test authorized fields',
  fields: () => ({
    id: globalIdField('AuthorizedTest'),
    public: {
      type: GraphQLString,
      resolve: () => 'This is public',
    },
    private: {
      type: GraphQLString,
      resolve: () => Promise.reject(new UserError('401')),
    },
    error: {
      type: errorType,
    },
  }),
});

export default authorizedTestType;

If you want to handle server errors directly in your component you can add an error field to the corresponding type as above. We can use the fetcher again to set the error on this type. Following function is not well tested, but does the job for now.

export const addErrorsToData = (originalData, errors) => {
  const data = { ...originalData, errors };
  errors.forEach((error) => {
    const pathUntilToErrorableObject = error.path.slice(0, -1);
    return pathUntilToErrorableObject.reduce(
      (dataChild, childKey, index) => {
        if (index < pathUntilToErrorableObject.length - 1) {
          return dataChild[childKey];
        }

        // eslint-disable-next-line no-param-reassign
        dataChild[childKey].error = {
          ...error,
          field: error.path[error.path.length - 1],
        };
        return dataChild;
      },
      data,
    );
  });
  return data;
};

You can call it in the fetchers fetch function like following

if (json && json.errors && json.data) {
  json.data = addErrorsToData(json.data, json.errors);
}
alloy commented 6 years ago

Leaving a link to @josephsavona’s explanation here for posterity.

carlosdp commented 6 years ago

I just ran into this and was very confused as a user. I understand the trickiness in regards to mapping the concept of partial data with onSuccess/onError for direct queries or mutations, but it seems to me that the solution is pretty straightforward in regards to QueryRenderer. I would have expected it to return both the error and the partial props data, allowing me to determine for each component whether to attempt to render partial data or not, knowing error is not null.

Is there something I'm missing here? It seems this behavior can be rectified independent of the discussion of callbacks.

orlowang commented 6 years ago

any update for this? for now I add errors field into data to make my project work just like what @jkettmann doing, but it's seems not the best practices, for me.

stebunovd commented 6 years ago

I have tried the workaround described above (passing null to data) but it didn't work for me with Relay versions 1.5.0 and 1.6.0. It turns out that error handling in QueryRender was completely broken since v1.5.0, see #2385. I rolled back to 1.4.1, and the workaround worked.

jamesone commented 6 years ago

I'm also receiving this error. I'm seeing this error popup when I use react-relay-network-layer & react-relay-network-modern. I wrote up a post explaining my issues on SO: https://stackoverflow.com/questions/52049098/how-can-i-catch-relayobervable-unhandled-error-when-graphql-errors-are-returned

I've tried all the above suggestions without any luck. My app still shows the redscreen in development and crashes in production.

nodkz commented 6 years ago

@jamesone please try noThrow option:

const network = new RelayNetworkLayer(middlewares, { noThrow: true });
jamesone commented 6 years ago

@nodkz I confirmed that this fixes the issue for react-relay-network-modern

However, the react-relay-network-layer` package still throws the error.

nodkz commented 6 years ago

@jamesone will be great if you take care of this problem. You may see this pull request and add analogous changes to react-relay-network-layer (do not worry, there are more lint edits in readme than code fixes). Sorry, but I do not have the bandwidth for solving this problem right now.

jamesone commented 6 years ago

On it 👍 @nodkz I'll keep you in the loop. I was able to fix the issue, I'll let you know once I'm done and create a PR on the repo.

jamesone commented 6 years ago

@nodkz https://github.com/relay-tools/react-relay-network-layer/pull/61 - Check it out and let me know what you think

srghma commented 5 years ago

I decided to go other way - I dont think commitMutation should reject at all (because "partial success" feature is one of the founding principles of GraphQL)

import { commitMutation } from 'react-relay'

// inspired by https://github.com/relay-tools/relay-commit-mutation-promise
// check https://github.com/facebook/relay/issues/1913 for more info

export default function commitMutationPromise(environment, config) {
  return new Promise(function(resolve, _reject) {
    commitMutation(
      environment,
      Object.assign(
        {},
        config,
        {
          // onCompleted is called when:
          // - data is not null (it doesn't matter if errors field is empty or not)
          onCompleted: function(data, errors) {
            // restore original response
            resolve({ data, errors })
          },

          // onError is called when:
          // - data is null
          // - errors array is not empty
          onError: function (errors) {
            // TODO: can errors be an js error object? reject then?

            // never reject
            // (reject is pointless because "partial success" feature is one of the founding principles of GraphQL),

            // restore original response
            resolve({ data: null, errors })
          },
        }
      )
    )
  })
}
Eightyplus commented 5 years ago

Please fix this, this is annoying!

sameeravhad119 commented 5 years ago

I have somewhat older version of Relay Modern (v.1.6.0). This is how I fixed the issue.

When graphql sends error message, it has a default format like following,

{
  data: null,
  errors: [{message: 'Bad Error', code: 'some error' ...}]
}

You can modify it to simple format by adding formatError image (for more info on how to add custom error message, go here: https://medium.com/@estrada9166/return-custom-errors-with-status-code-on-graphql-45fca360852)

After doing this, Now graphql will send error message in following format. Now errorsis array of string.

{
  data: null,
  errors: ['Bad Error']
}

Now go to your front-end side code, open environment.js (file in which relay environment is setup)
add these lines to returned fetch function of fetchQueryfunction.

 .then(json => {
      if (json && json.errors) {
        throw new Error('Server Error: ' + json.errors[0])
      }
      return json;
    });

Now, It will kind of look like this:

function fetchQuery(
  operation,
  variables,
) {
  return fetch('/graphql', {
    method: 'POST',
    headers: {
      'content-type': 'application/json'
    },
    body: JSON.stringify({
      query: operation.text, 
      variables,
    }),
  }).then(response => {
    return response.json();
  }).then(json => {
    if (json && json.errors) {
      throw new Error('Server Error: ' + json.errors[0])
    }
    return json;
  })
};

Note:- Don't catch this error. This thrown error will get transmitted to QueryRenderer where it will be available as error prop. handle it there (show error related UI if error is present).

<QueryRenderer
        environment={environment}
        query={query}
        render={({error, props}) => {
          if (error) {
            return <div>{error.message}</div>;  //render UI based on error
          } else if (props) {
            return <div>{props.page.name} is great!</div>;
          }
          return <div>Loading</div>;
        }}
/>
hayes commented 4 years ago

I have been wondering if there would be a way to add support for additional patterns around error handling by adding a few new APIs.

At a high level, the issue seems to be that relay does not have a way to represent non-critical errors, or errors which are not intended to bubble all the way to the root of the query without introducing/representing those errors at the schema level. While I understand the value and benefits of properly representing error states you want to handle in your schema, and treating them as data rather than exceptions, many backend frameworks and patterns have been built around the idea of partial responses, and allowing errors in nullable fields to automatically be captured in the root error array with a path, and letting the rest of the response to succeed. In most of the backend frameworks for graphql I've looked at, automatically creating intermediate objects or unions for representing various error states is a very manual process, and I have not seen good examples of patterns that make handing things like failed network requests to an external service easy to handle without a lot of manual error handling and custom types.

I think eventually there will be some new backend patterns that will make this easier, but the current state of the world seems to be that there are lots of real world examples of graphql schemas that use patterns with partial + errors in responses.

Here is my ideal solution:

The end result of this would be that you could create error boundaries around components that consume a fragment and handle errors for more granular parts of a query without having to break down your query into smaller requests to get more graceful degradation.

I am pretty new to relay, so my understanding of relays concepts are pretty rough, but I think adding something like this could be done in a way that adds minimal overhead to existing patterns and would create something that is completely opt-in, while still providing a solution to a large set of problems around error handling.

taion commented 4 years ago

@hayes

Have you seen @alloy's blog post in https://artsy.github.io/blog/2018/10/19/where-art-thou-my-error/ ? Not sure if this is what you're thinking about w/r/t "very manual process".

One way to think about this – the profusion of custom types reflects the lack of generics in GraphQL, but that lack of generics is reasonable if you think of generics as mostly to make life easier for programmers, and as something that can be implemented inside your (programmatic) GraphQL schema, and merely reified on the actual emitted schema itself.

But with all that, then it might not be terribly bad to manually unwrap fields with errors that you intend to handle.

hayes commented 4 years ago

Yup, I love that post. It has a great breakdown of these problems. "very manual process" definitely was meant to encompass the work involved around setting up all the types/interfaces/unions required to represent these errors, but even after you've implemented most of the pieces in that blog post, without a lot of additional work, in most backend frameworks I've seen, its still relatively easy to end up with errors in your root level errors object. If you follow the advice in that blog post and have confidence most errors will come through as typed data in your response, you can always use one of the strategies mentioned above to bubble any remaining errors up to the root and have it fail at the query level. Every framework I've worked with (but I haven't looked at too many yet) default to having errors in a resolver will automatically fall back to making the closest nullable parent field null and inserting the error into the root errors list.

For most applications I've worked in, developers try to handle the obvious error cases like a failed request, but there are almost always cases that are not explicitly handled, and explicitly handling every possible error case is often not practical.

I am not trying to suggest that using unstructured errors like this is a good pattern, just that is a pattern which is common, and even in backends that try to implement better error handling by baking it into the schema are still likely to have some errors fall through the cracks and fall back into the root errors array. In these cases, I would personally prefer to have a way to extract those errors when retrieving the data/fields which the errors correspond to, rather than being forced to handle them at the root level.

taion commented 4 years ago

Right, so there are a couple of things going on here.

The first is that GraphQL's default null semantics are... "interesting". If you mark a field as non-nullable, but it errors out, it's game over for all its parents up until you get to the first non-nullable one. There's nothing you can do about that if your schema is set up that way, hence the suggested best practice of making everything nullable.

In terms of implementing the "either"-based approach on a GraphQL API, if you're using the programmatic API, it's really not so bad, if you're so inclined. You could do something like:

fields: () => wrapWithOrError({
  myField: {
    type: MyType,
    resolve: (obj) => whatever,
  },
});

and in wrapWithOrError, do something like... for each foo, add a fooOrError field that wraps resolve in a try-catch and builds a memoized union type of MyType + the appropriate error types (either defined as additional metadata on MyType or on the field itself if need be), with the errors implementing some baseline error interface, &c.

Then on the client side, if you try to select on a fragment for just the non-error branch of any of the MyTypeOrError unions, then you'd just naturally fail on accessing any of those fields, and I think you'd most likely naturally end up hitting any error boundaries on null pointer references, without having to explicitly handle the error.

Otherwise, with Relay (but similarly with any other GraphQL client), for most of these error conditions, if you really want to handle them gracefully, then you can also effectively (mostly) ignore request-level errors, and just blow up on null dereference in the relevant components.

hayes commented 4 years ago

if you really want to handle them gracefully, then you can also effectively (mostly) ignore request-level errors, and just blow up on null dereference in the relevant components

I think the issue there is that it assumes that a null means there was an error, which is often not the case, and would effectively mean you your schema can either have valid nulls or errors, but not both.

It seems like you are suggesting that there is are patterns you can use in your schema/server to represent any error condition in you schema. I don't disagree, but I also don't think it is a cleaner solution. Having every field wrapped in a union makes your queries and you schema a lot more complicated, many schemas already exist, and use the simpler pattern of nulls + top level errors, and migrating these schemas to properly represent all their error cases in the schema might be a lot more complicated than exposing a way to access these errors in relay.

I was not trying to advocate for this pattern to be the preferred way to handle errors in relay, just hoping it would be possible to expose these errors for users who would like to be able to access them somewhere outside of the root level query or the network layer.

But, after thinking about the problem a little more, I think all this is actually pretty manageable without anything in relay itself. It should be pretty straight forward to basically map errors to object ids + fields at the network layer, then write some hooks that wrap the relay hooks to check for errors when unmasking a fragment.

taion commented 4 years ago

On valid nulls or errors – without checking into the errors sub-object, you wouldn't be able to have them on the same field, yeah. As a rule of thumb, we generally try to limit our use of valid nulls, except in cases where it wouldn't be too bad to display the "null" result if something went wrong (though we're actually just throwing request-level exceptions on any schema errors, so this doesn't do anything in practice).

Oh, right, the unions would only help in a "neat" way when used at fragment boundaries. They wouldn't help for individual scalar fields that throw... at the very least, you'd have to box the value, and it wouldn't be transparent to the client developer.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

wyattanderson commented 2 years ago

But, after thinking about the problem a little more, I think all this is actually pretty manageable without anything in relay itself. It should be pretty straight forward to basically map errors to object ids + fields at the network layer, then write some hooks that wrap the relay hooks to check for errors when unmasking a fragment.

Hey @hayes, were you able to make this solution work for you? Would you happen to have any more details or example code?

hayes commented 2 years ago

@wyattanderson no, I ended up going in a different direction, and finding a better way to add errors into the schema itself. I didn't end up trying what I suggested above.

I ended up just writing this https://giraphql.com/plugins/errors, but that isn't a good solution unless your API is written in typescript.

rafasanmartinez commented 2 years ago

There seems to me like Relay proposes a mechanism for reporting and handling field errors that is different than the reccomendation in the GraphQL specification.

This mechanism is incorporated to Relay by design: Relay expects that a GraphQL server will expose field errors through wrapper types around field result objects, where the enclosed result objects are unions that can supply either the actual value of the field or the error object.

This is stated in the Relay documentation in the section Accessing errors in GraphQL Responses of the Error States with ErrorBoundaries chapter.

Given that, I believe that there is not much that I can do about it than either leave it or take it unless Relay provides the functionality.

I am initating myself into this framwework and I am experimenting with the GitHub GraphQL service, which seems to adhere to the GraphQL specification versus the error handling mechanism expected by Relay.

In any event, I want to continue exploring this framework for the good or for the bad, so I have proposed myself a compromise solution to this question in this example .

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.