graphqlcrud / spec

Specification for GraphQLCRUD
https://graphqlcrud.org
Apache License 2.0
22 stars 4 forks source link

Non-null usage for return types #20

Closed danielrearden closed 4 years ago

danielrearden commented 4 years ago

The examples in the spec suggest that the return type for mutations should be non-nullable. There's two issues with that:

One, operations like updating or deleting a node by an id might not find a matching node, in which case they'll have nothing to return. In this case, we want to return null to indicate that a match wasn't found and the operation wasn't executed.

And two, even operations like creating a node can still fail. If they do, we don't want that to obfuscate any other mutation fields that did resolve successfully. For example, given an operation like

mutation CreateNotes ($a: CreateNoteInput!, $b: CreateNoteInput!){
  a: createNote(input: $a) {
    id
  }
  b: createNote(input: $a) {
    id
  }
}

under the current spec, if either a or b fails, the returned data will be null, even if the other mutation field resolved just fine. That happens because of the way GraphQL handles errors and non-null fields. A field that errors always resolves to null, but a non-null field can't be null, so the parent is nulled instead (or the next highest nullable field, if the parent is also non-null... all the way up to the data root).

I think it would be a good idea to at least make the return types for all mutations nullable.

It's generally good practice to make almost all fields nullable for the same reason -- you might not want one bad field causing your entire query to return null. Going from non-nullable to nullable is also a breaking change if (when) your business rules change and that description is now optional. However, there's something to be said for not having to a bunch of optional chaining on the client side too.

wtrocki commented 4 years ago

I think it would be a good idea to at least make the return types for all mutations nullable.

Fully agree. @craicoverflow do you have anything against this?

craicoverflow commented 4 years ago

Nope, I'm happy with this.

wtrocki commented 4 years ago

Addressed by @danielrearden