[RFC] Ignore undefined input object fields

[leebyron]

Currently http://facebook.github.io/graphql/#sec-Input-Objects reads:

This unordered map should not contain any entries with names not defined by a field of this input object type, otherwise an error should be thrown.

Which is designed to help detect runtime errors which may be the result of typos. However this unnecessarily introduces a vector for breaking changes. If a field of an input object is removed in the schema, then existing queries may all the sudden result in execution errors even though they're providing strictly more inputs to the server than it requires.

Variations of this have been requested in graphql-js before (https://github.com/graphql/graphql-js/pull/343)

Notably, the same restriction is not placed on field arguments, additional field arguments on a field does not result in an execution error (however it does result in a validation error).

Validation should still insist on not allowing unknown fields to an input object.

[rmosolgo]

Hmm, let me see if I understand correctly:

Validation should still insist on not allowing unknown fields to an input object.

This means, if the value of an input object is provided as a literal, it should only contain input fields defined for that input object, right?

This suggestion is to change the behavior when the value is provided as a variable, previously extra fields (should have) resulted in an error, but now they'd just be ignored?

If I've got it right, :+1: from me. I don't remember putting that validation in graphql-ruby in the first place 😬

[leebyron]

I think that's right, though it's all up for debate.

http://facebook.github.io/graphql/#sec-Argument-Values-Type-Correctness and http://facebook.github.io/graphql/#sec-Variable-Default-Values-Are-Correctly-Typed encapsulate this behavior right now. Values provided must be of the right type.

Combine that with http://facebook.github.io/graphql/#sec-Input-Objects input coercion rules which says "... should not contain any entries with names not defined by a field of this input object type, otherwise an error should be thrown."

It's maybe a little weird for literal values and runtime variable values to diverge in behavior, so that deserves fair criticism. It seems to me that having the validation to enforce only allowing the fields of an input object for literals may still be valuable, but I think we could make a similar argument above about what should and shouldn't be considered a breaking change.

What was most motivating to me along the lines of breaking changes was that variable values are checked at runtime before executing, which breaks the execution of queries if additional variable data is provided. We already have precedent for schema changes that cause previously-valid queries to appear invalid though still have totally reasonable execution behavior, and this is a nice property to have. We could apply the same judgement to this case - removing an input object field from the schema would render queries invalid but at least still executable with expected behavior.

[benwilson512]

Just to be clear, suppose we have an input object Contact that has a value and type field.

query UserByContact {
  user(contact: {value: "foo@bar.com", type: EMAIL, extraField: false}) { name}
}

is invalid but

query UserByContact($contact: ContactInput) {
  user(contact: $contact) { name }
}
variables: {"contact":{"value": "foo@bar.com", "type": "EMAIL", "extraField": false}}

is not invalid?

[mssodhi]

Reviving this again because not sure what happened to this, but why not just do a strict input validation on "non-null" object types, and allow extra unspecified fields? I know the reasoning behind typos, but I don't see why we can't have a property like "allow-unknows" = true on a specific object input type.

[JeffRMoore]

Accepting unknown fields allows clients to create constraints on the evolution of the schema. For example, if a client sends an extra field and the server ignores it, then the server later adds that field, it can retroactively break the clients that over-sent. If the client is important enough, then that field name is burned and cannot be added. This type of break is hard to detect ahead of time and the names most logical for over-sending (artifacts of client implementation) are the ones most logical to be added to the schema later. This happened to me maintaining a public api over the course of many years.

[tolg]

Why not allows server side to make decision whether ignores the undefined fields? If your server doesn't worry about the potential safe problem, just change a setting to ignore these fields. The developers should have the right to weight between security and flexibility.

[vsg24]

5 years on and still missing some basic functionality. Utterly disappointed.

[acao]

It‘s a strawman with no champion or spec RFC, it’s up to the community to advance this. Who are you waiting for @VSG24 ?

[Pastromhaug]

Just chiming in here in support of ignoring undefined input object fields, or at least having an option to. I'll explain my company's situation to shed some light on how this would be useful for us.

We have a GraphQL server supporting a mobile app and a web dashboard. On the frontend we rely on apollo client, graphql-codegen, and typescript. The issue we are encountering is that we semi-frequently ship PRs that accidentally are passing extra fields to a graphql input. This is really easy to do accidentally because TypeScript allows for extra fields. So this is effectively a runtime error that we are unable to prevent using TypeScript. I just shipped this kind of bug yesterday even after doing a lot of manual QA.

Let's say I have the following GraphQL types

type Mutation {
  updatePerson(personId: String!, input: PersonInput!): Person
}

type Person {
   id: String! 
   name: String!
   age: Int!
}

input PersonInput {
  name: String!
  age: Int!
}

We'll often query for the Person, then when a user hits "save", we fire the mutation. The classic error is the __typename and id are not defined in PersonInput, because somewhere in the frontend code we used a spread operator, and typescript accepts it as a PersonInput. It's easy enough in this case to strip these fields out manually in this simplified example, but it becomes a lot more complicated with nested input types, and it's a huge headache to keep this in mind everywhere in a complex web app.

Hope this is helpful!