apollo-client: Handle partial responses when the server returns an error

XXX in QueryManager#makeRequest.

About this issue

  • Original URL
  • State: closed
  • Created 8 years ago
  • Reactions: 2
  • Comments: 50 (21 by maintainers)

Commits related to this issue

Most upvoted comments

This issue has been automatically closed because it has not had recent activity after being marked as stale. If you belive this issue is still a problem or should be reopened, please reopen it! Thank you for your contributions to Apollo Client!

Can we re-open this? The problem still exists—I just hit it today.

Querying event data means that for some languages e.g. a town might not exist and returns null.

query eventQuery {
  transaction(id: 111111) {
    performance(siteId: 1) {
      venue {
        name
        town
      }
    }
  }
}

We get this response:

{
  "data": {
    "transaction": {
      "performance": {
        "venue": {
          "name": "Kobetamendi",
          "town": null,
        }
      }
    }
  },
  "errors": [
    {
      "message": "Error trying to resolve town.",
      "locations": [
        {
          "line": 9,
          "column": 11
        }
      ]
    }
  ]
}

Apollo doesn’t pass any of the partial data it gets right now, which is frustrating.

At a high level, what would be ideal for our use case would be an option passed to ApolloClient:

const client = new ApolloClient({
  partialResultsOnError: true
})

When this option is set, and a query contains errors, the client would resolve successfully with what information it did receive and attach an errors field:

{
  data: [{
    team: {
      members: [{
        id: 'b',
        name: 'Foo Bar'
      }, null, null]
  }],
  errors: [ Error(could not find id 'a'), Error(could not find other id 'c') ]
}

If partialResultsOnError is false, the client would act exactly as it does now.

How well does this fit into the existing system? I’m happy to make a PR if you can give me some pointers on where to start.

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

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 to Apollo Client!

@sorenbs this is currently blocked on the store refactor which we’re working on at the moment. Currently the plan for the new API is to handle errors the same way as we handle data, and write them into the store as well if they have a path (which means you might retrieve cached errors from the store). I’m not yet 100% clear on what we should do with errors that don’t have a path, but my intuition is to give people a lot of control around how they want to handle those, and decide on a per-query basis whether something with errors should be written to the store or returned from the store. This will make it very easy to configure Apollo Client in just the way people need, but it comes with the drawback that it will be more complicated to tell what’s really going on, which is why choosing sensible defaults will be very important.

What you could help us with is letting us know exactly how you would want your client to behave in different circumstances, and helping us choose sensible defaults so that Apollo Client does what people expect right out of the box (while keeping options open to modify its behavior).

Here are some questions to discuss:

  1. Should errors (with/without path) be written to the store?
  2. Should errors be retrieved from the cache if an earlier query errored on the same path?
  3. What should happen with queries that have errors without path?
  4. How should GraphQL errors be delivered to components? How about non-GraphQL errors?
  5. If we don’t cache errors, how should we deal with them during SSR?

There are probably a few more missing here, but

@stubailo Thanks for the context-- I’ll take a look tonight and give you my thoughts tomorrow.

Thanks Jonas - that’s awesome news!

I very much agree with allowing developers to configure the behaviour, and agree the sensible defaults are super important.

I’ll take some time soon to write up my perspective on this.

So here’s the long and short of the problem.

  1. Apollo uses a global cache to store all of the data you have ever fetched, which is important if you want your UI to be consistent, or you need to handle mutation results and have them show up properly.
  2. GraphQL.js simply returns null on any field where there is an error, and doesn’t have any indication of where in the response the error happened.

So, basically this unfortunate situation can happen:

  1. You fetch query A, the result is cached
  2. You fetch query A again, but this time there’s an error that caused a null, the result is cached
  3. Now, you have a null in your cache, even thought you had some good data from (1)

Given that there’s no way to tell the difference between a real null result and an error, we decided it’s best to just keep the original data and not let the new result “poison” it.

What’s a different approach that would be better?

For example, we could just always put the data in the cache and deal with the fact that it might be wrong; we could have a configuration option that would let you look at the error and say whether the result should be used or not; maybe something else?

@bl42 I am finding that all results in the error being omitted from the props method on the graphql callback object.

graphql(query, {
    options: ownProps => ({
      errorPolicy: 'all',
      variables: {
        personId: ownProps.match.params.id,
        startDateTime: ownProps.startDateTime,
        endDateTime: ownProps.endDateTime,
      }
    }),
    props: ({ data: { error, loading, user } }) => {
      return {
        error,
        loading,
        ...user,
      };
    }

If I omit the error policy, thus defaulting to none, the error is passed in the props callback.

If I use ignore, it is not passed, which is expected.

If I use all, the error is still not passed.

Am I missing something? 😕

Using the all policy is the best way to notify your users of potential issues while still showing as much data as possible from your server. It saves both data and errors into the Apollo Cache so your UI can use them. (Source)

@helfer Is there any traction on this?

This would be quite important for a project I’m working on too. Use case (dummy schema):

getCustomers {
  name
  email
  favouriteBook {
    id
    name
  }
}

favouriteBook needs to be fetched with a resolver for each Customer in the list, via http, as it’s in a separate micro-service. If one of the resolvers fails, meaning that we don’t get a favouriteBook for that customer, apollo client doesn’t return the remaining data, even though it comes in the network response. This means that the entire list fails to display, even though only one of the elements had an error.

Now, maybe we can handle this in the resolver by catching the error and returning empty data, however it would still be helpful to see the errors, as taking this approach would completely hide potential problems.

Error handling is high on our list of priorities, but we decided not to do it for 0.5 yet.

We could take advantage of this PR we added to graphql-js, to know exactly where the error happened: https://github.com/graphql/graphql-js/pull/396

But yeah, I’d be OK to add an option to ignore the errors and just put everything into the cache until reporting the error path is part of the GraphQL spec.

Do we want it to be per-query, or global?