graphql / graphql-spec

GraphQL is a query language and execution engine tied to any backend service.
https://spec.graphql.org
14.28k stars 1.12k forks source link

Mutations - Support being more specific about null and omission in input types #476

Open massimonewsuk opened 6 years ago

massimonewsuk commented 6 years ago

The issue

Here's a link to the specification for not-null: http://facebook.github.io/graphql/June2018/#sec-Input-Objects

It says:

Otherwise, if the field is not required, then no entry is added to the coerced unordered map.

This means that we have an opportunity to behave differently in our application code based on whether the user explicitly posts null for a field, or simply omits the field. However, we have no way of restricting the options available to a user.

Consider the following schema:

input PersonInput {
  id: ID!
  name: String # Does not support partial updates - i.e. null or omission results in this field being set to null in the database
  hobby: String # Supports partial updates - i.e. null means to unset this field, omission means to leave the existing value as-is
}

type Person {
  # ... omitted for brevity
}

type Mutation {
  updatePerson(input: PersonInput): Person
}

Consider the following scenarios:

Scenario 1 - User wants to blank out all values for a person

Option 1 - Setting all fields to null

The user posts the following object:

{
  "id": 1,
  "name": null,
  "hobby": null
}

I think this is the most idiomatic approach a user would use. The server does exactly what the user wanted.

Option 2 - Omitting all fields

The user posts the following object:

{
  "id": 1
}

This doesn't feel like a natural thing to do. The server nulls out the name, but the hobby is left as-is. The user didn't achieve what they wanted, but they will be left wondering why. They will probably write a more specific query and maybe file a bug against the API saying the hobby field doesn't work as expected.

Scenario 2 - User wants to blank out the hobby for a person

Option 1 - Providing the existing name, with the hobby set to null

The user posts the following object:

{
  "id": 1,
  "name": "John",
  "hobby": null
}

I think this is the most idiomatic approach a user would use. The server does exactly what the user wanted.

Option 2 - Providing the existing name, with the hobby omitted

The user posts the following object:

{
  "id": 1,
  "name": "John"
}

This doesn't feel like a natural thing to do. The server does not blank out the hobby. The user would probably retry the query using option 1 at this point.

Option 3 - Just setting the hobby to null

The user posts the following object:

{
  "id": 1,
  "hobby": null
}

This feels natural, especially in the "GraphQL" world where the API can constantly evolve and new fields can be added. The server nulls out the hobby as expected. However, it also nulls out the name, which the user did not expect.

Scenario 3 - User wants to blank out the name for a person

Option 1 - Providing the existing hobby, with the name set to null

The user posts the following object:

{
  "id": 1,
  "name": null,
  "hobby": "Skydiving"
}

I think this is the most idiomatic approach a user would use. The server does exactly what the user wanted.

Option 2 - Providing the existing hobby, with the name omitted

The user posts the following object:

{
  "id": 1,
  "hobby": "Skydiving"
}

This doesn't feel like a natural thing to do. However, the server does exactly what the user wanted.

Option 3 - Just setting the name to null

The user posts the following object:

{
  "id": 1,
  "name": null
}

This feels natural, especially in the "GraphQL" world where the API can constantly evolve. The server does exactly what the user wanted.

I have not illustrated examples of the scenarios regarding a field which is always mandatory but can be omitted from an update to retain its existing value.

Description

The following cases illustrate that there are several points of ambiguity in our API:

It's easy to turn around at this point based on these trivial examples and say "Well why the hell would you do that for? Why not be consistent?"

The reason we are facing this dilemma is because we have many mutations (none of which support partial updates, i.e. all fields are always required explicitly) and our consumers want some APIs to start supporting partial updates. At the moment, some fields are optional, so both null AND omission translate to "blank it out". However, we would need to start treating omission differently (i.e. "leave as-is"). Without excessive comments in our documentation and validation in our API, our API will be difficult to understand and seem "inconsistent" to end users. It would be great if we could define input types like the following:

input ExampleInput {
  id: ID!
  field1: String | null # Can be a string or null, but must always be provided (i.e. does not support partial updates). This would be a solution to the scenarios I listed above.
  field2?: String! # Can be a string (to update the value) or omitted (to leave as-is), but can't be null. This would be a solution to the scenario I briefly mentioned above in bold but did not provide examples for.
}

@trevorah @peterstarling @joaojeronimo @craigbilner @belema @jackharvey1 You guys might be interested in this

leebyron commented 6 years ago

This was an intentional decision to balance between expressiveness and complexity. While the subject of your proposal is absolutely true: we cannot communicate the difference between optionality and nullability in input types, our previous discussions have determined that this is a reasonable cost for the benefit of avoiding the additional syntax you've suggested and avoiding multiplication of possible states an input object can be in. In doing so we've left this sort of additional behavior of how to treat scenarios where "required null" or "optional nonnull" up to the much fewer services which require these.

In your particular scenario, I think you can add exactly what you're suggesting to your server, especially since you say that all mutations behave as full overwrite. I would suggest adding additional validation rules or throwing additional coercion errors relevant to the types where the ambiguity could result in unexpected results.

leebyron commented 5 years ago

This has been open for a while, but I'm marking it as straw man and needs champion to see if anyone has a compelling approach for this problem. If there's no championing solution in the near future then I'll move to reject

matthew-petrie commented 5 years ago

This is an area that we are also finding painful - https://github.com/facebook/graphql/issues/542#issuecomment-458918773

raderio commented 5 years ago

needs champion to see if anyone has a compelling approach for this problem

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

Hossein-s commented 5 years ago

Omission and nullability are primary things in designing APIs. GraphQL seems to be incomplete without one of them. Almost every app includes one of the schenarios described above and lack of an standard way for this situations leads to inconsistency between different parts. It's not too hard to deal with them. REST APIs have this feature for several years. I hope we see this next to graphql's beautiful concepts.

ChrisLahaye commented 5 years ago

Anyone has a solution for this?

leoloso commented 4 years ago

@ChrisLahaye I have implemented a solution to this problem, but it requires a new feature (#682) which I've been told has close to zero chance of being added to the GraphQL spec... but I hope it can still give you some ideas for alternative solutions.

The solution is composed of 2 parts:

First: introduce composable fields

GraphQL could accept fields within a directive parameter. For instance, let's say the type User has a field hasHobby of type Boolean:

{
  user(id:1) {
    id
    name
    hobby
    hasHobby
  }
}

with response:

{
  "id": 1,
  "name": "John",
  "hobby": null,
  "hasHobby": false
}

Then, you could @include field hobby only if hasHobby is true:

{
  user(id:1) {
    id
    name
    hobby @include(if:hasHobby)
  }
}

In order to not make it so verbose, having to add extra field hasHobby, I proposed in #682 to add composable fields and global fields. Composable fields is having a field be an input to another field, like field uppercase composing field name:

{
  user(id:1) {
    id
    name:uppercase(name)
  }
}

with response:

{
  "id": 1,
  "name": "JOHN"
}

Now, because uppercase is too generic, I call it a global field, and it can apply everywhere, not only on type User.

Having this feature, we could now do this:

{
  user(id:1) {
    id
    name
    hobby @skip(if:isNull(hobby))
  }
}

which would then omit hobby if it is null:

{
  "id": 1,
  "name": "John"
}

Second: Symbol ? as syntactic sugar

Because @skip(if:isNull(hobby)) is too verbose, we can introduce symbol ? at the end of the field, meaning: if this field resolves to null, then omit it:

{
  user(id:1) {
    id
    name
    hobby?
  }
}

This syntax is similar to the newly-added optional chaining in JavaScript (obj?.name?.first), so I think it could make sense.

Demonstration

I'm using a different syntax so I could already support this feature (and a few others).

In this query, featuredimage will sometimes return null (results here):

posts.
  title|
  featuredimage.
    src

In this query, featuredimage will be omitted whenever it is null (results here):

posts.
  title|
  featuredimage?.
    src

I hope this helps? Cheers

leoloso commented 4 years ago

I've now been able to find a GraphQL-compatible solution (the one in my previous comment was not).

If the GraphQL server has a good support for custom directives, then a directive may do. In my server (in PHP), directives are executed in a pipeline and get a lot of additional information, including the object containing all the resolved fields to that point in time, and any directive can be executed either before or after resolving the field.

Then, a directive @removeIfNull has to:

Here is the source code. Check out all the arguments that the directive resolver receives...

It can be tested in this GraphiQL, or in this blog post.

stephenh commented 4 years ago

Fwiw my strawman proposal would be to allow ? or ! on the key itself, i.e. (mapping GraphQL types to TypeScript types out of laziness/but it still express semantics well):

The existing input syntax mapped to the current semantics (note it is kept as-is for backwards compatibility):

firstName: String --> firstName?: string | null | undefined
(i.e. firstName might be null, and you can leave it out)

firstName: String! -> firstName: string
(i.e. firstName cannot be null, and you must include it)

Additional proposed syntax:

firstName!: String --> firstName: string | null
(i.e. firstName can be null, and you must include it)

firstName?: String! --> firstName?: string
(i.e. firstName cannot be null, and you can leave it out)

The result is:


Reformatted a bit:

eurostar-fennec-cooper commented 4 years ago

I'd like to back this up for the people using graphql-codegen with typescript: using the ! operator in graphql alone is not enough as without it, the typescript that is generated allows T | undefined | null because a graphql field without the ! operator can either be T, null, or not provided at all.

It would be nice to have a little more granularity, as at the moment we are restricted to "this field can be T or null" or "this field can be T or null or undefined".

stephenh commented 3 years ago

Following up on my strawman proposal, if making a breaking change was acceptable, it'd be simpler to change the semantics of firstName: String! to mean "key is optional, value is non-null" because then the key-required-ness or value-non-null-ness would be completely orthogonal and we would only need ! as the "key is required" / "value is non-null" specifier, and would not have to introduce ? as "wait key actually is optional even though value is non-null".

I have no idea how easy/hard making a breaking change is at this point, but just mentioning it out of completeness and admittedly idealistic simplicity. Like if we're going to be writing GQL 5-10 years from now, maybe just go for the semantic change to make our lives easier in the long run.

nicoknoll commented 1 year ago

Had the same issue and now went with the workaround of adding a updateFields?: string[]. This way I ignore fields that should not be updated and if I receive a null for a field name in updateFields I know this is on purpose.

Not sure if it helps anyone but might be at least one way of doing it without changing the spec.

aradalvand commented 11 months ago

GraphQL desperately needs this.

I think the "? on the key to denote omit-ability" solution (described in this comment) or its ! counterpart if we can afford a semantic breaking change (mentioned in this one), is clearly the way to go, it makes perfect sense; or even a native @optional directive, if we want to remain 100% backward-compatible.

This is among the top 10 most upvoted open issues in this repo, it deserves to be championed. And the solutions are also relatively simple and non-problematic.

benjie commented 11 months ago

Are you offering to champion this @aradalvand?

aradalvand commented 11 months ago

@benjie Is there a guide for that?

benjie commented 11 months ago

@aradalvand There is indeed! https://github.com/graphql/graphql-spec/blob/main/CONTRIBUTING.md

I’m also happy to offer guidance, feel free to ask any related questions via the GraphQL discord, either at https://discord.graphql.org in the #wg channel, or via DM on there (I’m @Benjie there too). Have a read of the guide and if you’re interested the next step would be to write up a basic proposal and bring it to the GraphQL working group. Don’t spend too long on it, and focus on the problem that needs to be solved rather than the solution you see - the specifics will come later.

benjie commented 9 months ago

Related comment: https://github.com/graphql/graphql-wg/discussions/1410#discussioncomment-7728813

AntonioLuisME commented 1 month ago

Incredible that after 6 years of people begging for this, nothing has been done.