New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Recommended strategy for handling 404 errors? #119
Comments
That's a great question. -- I would love this pattern to be added as a suggestion to the documentation. I think it's a class of problems that multiple apollo-links suffer from (apollo-link-state comes to mind). I don't know that we are sure this is the right answer for all users of ApolloLink, however? |
This is currently how all http errors are handled in apollo-link-rest
perhaps we add a case matching function which lets users dictate how the error is processed? |
While I share @fbartho's concern that a solution we introduce might not be right for all users of this link, I thought of one case where a 404 error is particularly harmful. If we take this example from the tests: query postAndTags {
post @rest(type: "Post", path: "/post/1") {
id
title
}
tags @rest(type: "[Tag]", path: "/tags") {
name
}
} and the const data = {
post: null,
tags: [{ name: 'apollo' }, { name: 'graphql' }],
} If we want to cater for this, adding additional error handling as @paulpdaniels suggests will be pretty easy. Looking over the 4xx errors I can only really see 404 and 400/412/422 (on mutations) possibly being non-fatal errors requiring special treatment, either as GraphQL errors or setting the result to The question remains whether it is reasonable to provide some fixed handler for these errors or if it should be left to the There is also the option of adding an error handler to the configuration, but that seems like it could be stealing functionality away from an |
@marnusw That's a great response. I would support a fix that implements your behavior by default! |
#142 adds 404 error handling. I think that is the most important thing from this discussion. |
I have put up a PR to reinstate 404's as normal network errors, to coincide with good REST API practices: #283 |
@christrude while I can't disagree with you as far as REST APIs are concerned, the purpose of this library is to make REST APIs work like GraphQL. Therefore, if the GraphQL approach to missing resources is to return |
Then how does GraphQL propose you handle a 404 response in the UI? Do nothing? So requests just die quietly? Thats incredibly bad/poor UX/api management. |
If you query an item by ID and get null back, handle that accordingly in the UI. If it contains data, then display it. |
I agree with @marnusw here, we can't really control what the user's rest API does so the library shouldn't be overly restrictive in what it allows. The 404 => null mapping is a relatively intuitive semantic which lets you move from the single resource per request model of REST to the multi-resource per request (from the client perspective) of GraphQL while not blowing up on missing data. Perhaps there could be a better way to opt-out of this behavior, like a pre-packaged version of the response transformer or an abstraction of the request handling logic that lets the user fully control the http status handling. But I don't think reverting the to the previous behavior is the right approach. |
Part of the problem of the approach is that down in the result subscription you have no knowledge if its a null result because its a 404, or if it’s a no content return 201 or something similar. I have this fixed by in the afterware, checking the status and forcing it to throw an error that assumes the issue because I can’t access the actual server error message, especially in my case because we use the custom fetch to inject an auth token, and can’t use it to get the response before apollo deletes it. My solution is a bad blanket for other 404 response across the app, aside from the one case. |
It is quite common for a REST API to use a 404 response to indicate that a record does not exist. Right now when this happens the result is a
networkError
which seems, by default, to be treated as a more fatal error than a GraphQL error by the Apollo stack.For example, with the query:
If the endpoint returns 404 it rejects with a network error that must be handled where the intent is likely to merely use the
error
render prop to display the non-fatal error message to a user. i.e. if this was a GraphQL query instead of REST this would not be classified as a network error and it would be handled in this way.What I did to fix this was to add a check to my custom
error-link
which transforms the 404 network error to a GraphQL error:There is probably room for improvement on this still, but it does solve the problem.
I was wondering if this might/should somehow be addressed in
apollo-link-rest
, so I figured I would start the conversation.The text was updated successfully, but these errors were encountered: