Skip to content
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

error propagation considered harmful #719

Closed
VladimirAlexiev opened this issue May 18, 2020 · 3 comments
Closed

error propagation considered harmful #719

VladimirAlexiev opened this issue May 18, 2020 · 3 comments

Comments

@VladimirAlexiev
Copy link

https://spec.graphql.org/draft/#sec-Errors-and-Non-Nullability describes that any error in a non-null field should be propagated to its parent field, destroying the parent. This applies recursively to further ancestor fields, and across arrays with non-null members (if one member is null, the whole array should be destroyed).

Thus all ancestors are destroyed for a failure of any one of their descendants, if there is a chain of non-null fields going up from such descendants.
This destructive behavior is reminiscent of first-order logic, where one contradiction (being able to infer false) renders the whole system useless (you can infer anything).

Such behavior is brittle and I consider it harmful for real-world data integration scenarios, which I think is a major use case for GraphQL. When building a Knowledge Graph, you may have a field that's non-null and is valid in 99.99% of cases, but due to data quality problems there's almost always a tiny percentage of missing or malformed values that will cause such destruction.

This destructive behavior is implemented in some key software, and that harms our development efforts in the Ontotext Platform (which implements GraphQL over RDF Knowledge Graphs), see https://www.apollographql.com/docs/react/data/error-handling/

  • Both Apollo Client and Apollo Federation examine every GraphQL endpoint response and perform such destruction (mangle the response)
  • Furthermore, they propagate the "error" report upward and report the same problem at each ancestor level
  • At least the Client has a flag/mode to not mangle the response; Federation doesn't have such flag

https://www.apollographql.com/blog/using-nullability-in-graphql-2254f84c4ed7 by @stubailo explains related issues in detail. In particular "For output fields, removing non-null from a field is a breaking change".
All this means that you can never use non-null, except for fields such as ID without which you can't even find the object. Which weakens the GraphQL schema, eg leaves fewer clues for data entry UIs.

(Note: #259 is a bit related)

@markokr
Copy link

markokr commented May 18, 2020

Why not allow null in schema then?

@mike-marcacci
Copy link
Contributor

mike-marcacci commented May 18, 2020

I think your observations are really just supporting the idiom and best practice that is fairly well established with GraphQL: default to nullable, and use non-null to explicitly opt in to error propagation behavior.

I think you may also be confusing what is enforcing non-null: it isn't the client, but the server itself. If a schema says "the field id on type User can never be null", it would be illegal for the server to return a user without its ID. If your resolver throws an error when fetching/generating/serializing a user's ID, or otherwise returns null, this "nullifies" the entire user object. If a field is null 0.01% of the time, it isn't non-null. If the GraphQL server was to return null in a place defined as non-null, it would be breaking the guarantees it's making to the client.

In real-world schemas, fields may be null due to nonexistence, ephemeral errors, permission limits, or any other business logic that the client may or may not care about. To support this reality, the GraphQL type system itself defaults types to null, quite unlike most languages' type systems.

This behavior is well covered by the graphql.org best-practices section.

@VladimirAlexiev
Copy link
Author

Thanks for the pointer. And "use non-null to explicitly opt in to error propagation" is very elucidating.

I wish the spec would also make this more clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants