Skip to content

Support nullable & optional/required #542

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

Closed
raderio opened this issue Dec 11, 2018 · 14 comments
Closed

Support nullable & optional/required #542

raderio opened this issue Dec 11, 2018 · 14 comments

Comments

@raderio
Copy link

raderio commented Dec 11, 2018

In GraphQL we can mark a field that it is nullable or not by "!". But how to also mark that a field is required or optional?

Required means that this parameter must be passed, even if it is null.

Basically null has the meaning of "this property existed before, and I want to nullify its value". If there's no value, or the value needs not change (for requests, for example), it just shouldn't be sent.

For example an user have a birth day set in his profile, if he wants to update the birth day he must pass a value, but it is not allowed to pass null.

So, in this case a birth day type is not nullable, but it is optional, or user send a valida value or just not send this parameter.

This is supported by OpenAPI https://swagger.io/docs/specification/data-models/data-types/

@mjmahone
Copy link
Contributor

I would encourage creating a separate mutation root field for this case. Alternatively, use a boolean field to say whether the property is being deleted.

In general, each mutation should do exactly one kind of write: updates and deletes are usually different enough that they warrant their own top-level codepath, even if they end up calling into the same server code.

Back to your example, I'm also curious what the correct API call should be for "this property has never existed, and I still want it to not exist"? If you can pass in both a null value and a non-null value, how do you describe that situation? I'd argue this is exactly where an unset value makes sense (as much as the above API makes sense).

All that being said: if this is valuable to you, I'd encourage expanding your proposal to be a Strawman, with some actual examples on how things ought to work, what the language change might look like, and why a "best-effort" way of doing things in the current spec won't work.

@raderio
Copy link
Author

raderio commented Dec 12, 2018

Back to your example, I'm also curious what the correct API call should be for "this property has never existed, and I still want it to not exist"?

in REST we just not send the field
{ "foo": "bar", "baz": false} sending "foo" field
{ "baz": false} not sending "foo" field

We need this to implement a patch or partial update.
Something like https://medium.com/workflowgen/graphql-mutations-partial-updates-implementation-bff586bda989
But we want to mark in documentation that field can be undefined

@matthew-petrie
Copy link

Might be useful to reference here as we are needing the same functionality - creating specific keys to delete values would make our API very complex and would make it more complex for any developer working with it: https://stackoverflow.com/questions/53194116/optional-but-non-nullable-fields-in-graphql

@fluidsonic
Copy link
Contributor

fluidsonic commented May 30, 2020

I ran into a very similar problem of argument absence vs. null and am thinking of possible solutions.

Example use case:

  • In a CRM a user can update plenty of data points of an entity (e.g. a contact) at once.
  • One updateContact(…) mutation field allows updating all these data points at once.
  • In the operations, values will only be provided for arguments where the user has actually changed a data point.
scalar LocalDate

type Mutation {
   updateContact(
      id: ID!,
      firstName: String,
      lastName: String,
      birthday: LocalDate,
      children: Int,
      …
   ): Contact
}

mutation {
   updateContact(id: "123", firstName: "John", birthday: "1986-02-05", children: 2) { … }
}

In (valid) GraphQL that approach only works if null is used as default value to denote the absence of the argument. If the value is null the server will simply not update that data point.

However, if null is a legit value - for example to denote a data point as unknown/undecided/unset or simply means "none" - the approach no longer works.

  • How can I unset the birthday? Depending on the definition of the scalar there is no "empty value" like "0000-00-00" or "".
  • How can I unset children? 0 means no children and not that the number isn't known. -1 would be a magic value and only works for a few specific cases.

In my two GraphQL projects, I ran into that issue in plenty occasions.


Now there are a few alternative approaches with current GraphQL.

Every field is updated with every mutation

  • Client must include values for all data points, even if they haven't changed
  • Client must know the current value of all data points before performing an update
  • Client may overwrite values of another person if current values were stale, e.g. if a different user updates the same entity but different data points
  • Not forward-compatible (adding new arguments)

Create wrapper input types for every possible type

input IntChange {
   newValue: Int
}
input LocalDateChange {
   newValue: LocalDate
}
  • Example: birthday: null = no-op, birthday: LocalDateChange(newValue: null) = unset
  • Adds a lot of clutter to the schema
  • Adds an additional type for many scalars & input types.

A field that denotes whether to unset a specific data point

  • Example: unsetBirthday: true
  • Adds a lot of clutter to the schema
  • Isn't focused on data but instead indicates that some kind of "sub-operation" should happen
  • Allows conflicting input like unsetBirthday: true, birthday: "1986-02-05"

A field that denotes whether to update a specific data point

  • Example: updateBirthday: true, birthday: null
  • Adds a lot of clutter to the schema
  • Isn't focused on data but instead indicates that some kind of "sub-operation" should happen
  • Allows conflicting input like unsetBirthday: false, birthday: "1986-02-05"

Add a special empty/unset value for each type

  • Example: birthday: "unspecified"
  • Only works for scalars
  • Makes parsing more complicated
  • Adds lots of magic values

Split each one-per-entity mutation into one-per-entity-and-data-point mutation

  • Example: updateContactBirthday(id: "123", birthday: null)
  • Can lead to an enormous amount of mutation fields
  • Does not reflect the actual use case, i.e. of a user updating multiple data points of a Contact at once
  • Each field returns the Contact object but I only need the final one that includes all changes
  • Mutations are no longer performed atomically
    • Can lead to race conditions between multiple clients
    • Can fail partially (one data point updated but not the other)
    • Server cannot perform actions based on all changes (e.g. validate multiple related values, log the update in a single log entry, etc.)

Other approaches are not possible with current GraphQL without breaking the specification.

Use a special default value to denote absence

type Mutation {
   updateContact(
      id: ID!,
      firstName: String = ignore,
      lastName: String = ignore,
      birthday: LocalDate = ignore,
      children: Int = ignore,
      …
   ): Contact
}
  • If the server sees an ignore enum value it will treat the value as absent and thus not update the field
  • Specifying null explicitly will unset the value
  • Violates validation rule 5.6.1 Values of Correct Type
  • Default value abused for denoting absence, i.e. no value

Server treats explicit null vs. absence of value differently

  • Example: updateContact(id: "123", birthday: null) = unset, updateContact(id: "123") = no-op
  • Violates validation rule 5.4.2.1 Required Arguments if argument is non-null
  • Behavior diverges from the spec
  • Client does not know about that behavior as it isn't documented in the spec (could be alleviated by adding a directive like @maybe or @optional)

Add GraphQL language support for generics

input Change<T> {
   newValue: T
}

type Mutation {
   updateContact(
      id: ID!,
      firstName: Change<String>,
      lastName: Change<String>,
      birthday: Change<LocalDate>,
      children: Change<Int>,
      …
   ): Contact
}
  • Example: updateContact(id: "123", birthday: null) = no-op, updateContact(id: "123", birthday: Change(newValue: null) = unset
  • Complex extension to the type system for specific use-case

Add GraphQL language support for properly distinguishing between optional (unspecified) and null

  • Changes/extends the language
  • Provides proper specification
  • Gives proper documentation to users

@stephenh
Copy link

@fluidsonic fwiw not completely following b/c, at least using apollo / typescript / graphql-code-generator, in your birthday example, i.e. for an input type like:

input ContactInput {
  firstName: String
  birthday: LocalDate
}

The generated TS types look at:

interface ContactInput {
  firstName?: string | undefined | null;
  birthday?: string | undefined | null;
}

Which matches that valid (ignoring the TS types) incoming-JSON-from-the-wire can be any three of:

JSON: { firstName: "..." } --> firstName: "...", birthday is undefined, i.e. don't change it
JSON: { firstName: "...", birthday: "2020-01-01" } -> firstName: "...", birthday: "2020-01-01", i.e. set birthday to 1/1/20
JSON: { firstName: "...", birthday: null } -> firstName: "...", birthday: null, i.e. unset birthday

So, AFAIU GraphQL's types already support the tri-state of set/unset/noop that you're looking for? Am I missing something?

That said, I found this issue b/c I would like to be able to represent "this field is optional, but it cannot be null", i.e. a typescript type that looks like:

interface ContactInput {
  firstName?: string;
}

And get rid of the | null that GQL current forces me to handle/ignore everywhere.

However given that currently syntax wise:

firstName: String --> firstName?: string | null | undefined
firstName: String! -> firstName: string

And I generally assume these would have to remain that way for backwards compatibility, it's hard to think of a nice syntax that supports the/my desired firstName?: string output/semantics, i.e in a GraphQL input type something like:

firstName?: String! -> firstName?: String

So it's modifying the value with ! to be "cannot be null / must be string" but "adding back" the ? on the key itself, saying firstName as field presence is optional.

Maybe that's not too bad actually...

@fluidsonic
Copy link
Contributor

@stephenh looking at the spec birthday should default to null if unspecified.
That's the problem with nullable types - null and unspecified both end up as a null value.

I also have your problem though with non-nullable optional types. Same issue here - there's no "unspecified".

Also, JSON is just one possible transport format for variables. It should also work in plain GraphQL notation - i.e. without variables.

@stephenh
Copy link

@fluidsonic "birthday should default to null if unspecified." ... huh, here I express my naivety in that I have not read the spec, but had just observed the behavior of apollo-server, where when a field is not present, it left as undefined, and not turned into null.

I.e. I just confirmed with this test mutation:

mutation {
  saveAuthor(input:{}){
    id
  }
}

Where input.firstName and input.lastName are both Strings i.e. optional, and not present on the client's request, in the server-side resolver, they are also not present, i.e. input = {} and not input = { firstName: null, lastName: null }.

As a lazy ask, can you link to the section of the spec that asserts it should be null?

@fluidsonic
Copy link
Contributor

@stephenh you're right. I've just read the coercing algorithm again and I've indeed missed something. There's a difference in null and not present there after coercion.
Sorry for the confusion.

Therefor the issue is indeed only non-nullable optional types.

@stephenh
Copy link

Also fwiw I think this is a duplicate of #476

@jodinathan
Copy link

Server: "Hey, the ID input is required!"
User: "Are you sure?"
Server: "You bet I am!"
User: "{ ID: null }"
Server: "fuuuuuuuuuuuuuuuuuu"

@acao
Copy link
Member

acao commented Feb 14, 2021

@stephenh good call, this is a dupe of an existing strawman proposal #476 and should be closed so y’all can join the discussion over there

CC @IvanGoncharov @leebyron @benjie

@benjie
Copy link
Member

benjie commented Feb 15, 2021

I agree this seems like a duplicate of #476

@glen-84
Copy link

glen-84 commented May 14, 2022

@acao & @benjie – why not close it then? 🙂

@benjie
Copy link
Member

benjie commented May 16, 2022

Closing as duplicate 👍

@benjie benjie closed this as completed May 16, 2022
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

9 participants