apollo-link-http@1.3.3 error-mapping introduces potentially undocumented breaking change

[luhmann]

Probably related to: #475

In version 1.3.3 apollo-link-http remaps a whole range of errors from networkErrors to graphQLErrors under the assumption that whenever any errors-field is present in the response that those errors are graphQL-Errors. ref

This is a problem when any API that is behind the GraphQL-Server uses an error-response-schema that also includes an errors-field (which is quite common in API-Design) and the GraphQL-API just proxies the error-response to the frontend so it has the information what went wrong (eg. for authentication):

     ,-------------.                              ,-------------.                                   ,---.
     |Apollo Client|                              |Apollo Server|                                   |API|
     `------+------'                              `------+------'                                   `-+-'
            |                 FetchUser                  |                                            |  
            |------------------------------------------->|                                            |  
            |                                            |                                            |  
            |                                            |                 FetchUser                  |  
            |                                            |------------------------------------------->|  
            |                                            |                                            |  
            |                                            |**HTTP 401**: { errors: ['invalid_token'] } |  
            |                                            |<- - - - - - - - - - - - - - - - - - - - - -|  
            |                                            |                                            |  
            |**HTTP 401**: { errors: ['invalid_token'] } |                                            |  
            |<- - - - - - - - - - - - - - - - - - - - - -|                                            |  
     ,------+------.                              ,------+------.                                   ,-+-.
     |Apollo Client|                              |Apollo Server|                                   |API|
     `-------------'                              `-------------'                                   `---'

Intended outcome:

This setup would have worked prior to 1.3.3 because apollo-link-http would have turned the 401 into a networkError which could then be handled in apollo-link-error or apollo-link-retry (if you wanted to eg. set some different headers force a token-refresh, a pretty common pattern in OAuth-Authentication-Flows).

Actual outcome:

• in apollo-link-error this is now turned into graphQLErrors: invalid_token and I lose the 401 status code on the response-object that is also handed to apollo-link-error. I now have to parse the graphql-error-objects for potential errors that warrant a user-logout and can not rely on information within the http-status-code, which to me would have been more expressive.
• apollo-link-retry is not even triggered because the error is no longer a networkError

How to reproduce the issue:

Recreate the setup above

I completely realize the reasoning behind the change and that for a GraphQL-Client like Apollo it might be a reasonable assumption that the errors it gets passed are solely GraphQL-Errors.

I still think this caveat should be pointed out somewhere, as it might break an existing setup. To me personally this would not have qualified as a patch-level-release in semantic-versioning. See below, realized that that my approach rather worked accidentally in the first place.

I would also be interested in your opinion on how such a scenario should be handled after the introduced change. To me it feels like if I want to keep the information about why some authentication might have failed in the frontend, I now either have to map the reason of the authentication-error to a GraphQL-Error in the Apollo-Server and handle that, or I might need to map these errors to a different response-field, both of which does not feel very good.

---

Edit: After reading and thinking more about this I guess this is more of a problem of my current GraphQL-Endpoint-Design in that it applies a too REST-oriented thinking. It is still unfortunate that this is a rather subtle breaking change, but then the docs clearly state:

networkError: Any error during the link execution or server response, that wasn’t delivered as part of the errors field in the GraphQL result

which is another way of saying "if there is an errors-field on the response, we will treat it as graphQLErrors. And sadly I now cannot easily use apollo-link-retry to refresh a session token as it does not support retrying GraphQL-errors (yet). It might still be beneficial to mention this caveat in CHANGELOG, even though my approach kind of relied on something that you never guaranteed in the first place.

Feel free to close.

[evans]

@luhmann Thank you for the detailed issue!

Could you look into adding support for retrying GraphQL-errors to apollo-link-retry? It's definitely a worthwhile feature!

[tdeekens]

Thanks for the response @evans!

I think adding support to apollo-link-retry to be triggered also on GraphQL errors would be worthwhile. I wonder however if apollo-link-retry goes into its onError if errors beforehand have been tampered with. Which also means that the configurable attempts-fn (becoming the retryIf) will actually never be invoked. How generic would you want to see the retry logic to be configurable? Something like failOnGraphQLError: boolean or another callback giving more flexibility?

However, there is other underlying issue that the latest release of apollo-link-http has been marked as a fix version but it can actually also equally be considered breaking. We fear that also other run into the same problems with out expectations of apollo-link-http not mapping so many networkErrors onto graphQLErrors.