[RFC] Add flag to tolerate additional properties on input objects.

[glan]

Adding tolerateAdditionalProps property to a GraphQLInputObjectType schema will allow that type to tolerate additional properties which are not specified in the schema.

Example:

new GraphQLInputObjectType({
  name: 'TestFlexInputObject',
  tolerateAdditionalProps: true,
  fields: {
    a: { type: GraphQLString },
    b: { type: new GraphQLList(GraphQLString) },
    c: { type: new GraphQLNonNull(GraphQLString) },
    d: { type: TestComplexScalar },
  }
});

Will accept:

{a: "foo", b: ["bar"], c: "baz", dog: "dog"}

Note that the additional properties will still get filtered out but we will not get an error.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

[coveralls]

Coverage increased (+0.0008%) to 99.444% when pulling 2eb0e2d on jackmobile:tolerate into 3974438 on graphql:master.

[leebyron]

Thanks for testing out this idea. I'm still a bit worried about the pitfalls this produces, but I'm happy to leave this open for more comments. I'm sure @dschafer also would like to take a look.

My opinion is that we should probably pick a behavior and stick with it rather than making it a per-object opt-in or opt-out. If it is customized per object, it should also be exposed via introspection and this behavior would need to be reflected in the specification as well.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[rwe]

I'm also worried about this feature, but since I don't see much more discussion toward resolving this PR, here are some thoughts to get it rolling.

If a variation of this feature were to make sense, it may be better applied as an option to schema validation as a whole. I can see an argument for the following use-cases which would desire different schema behaviour:

• Servers gracefully accepting inputs with extra fields, in line with the Robustness Principle, might want this enabled.
• Client validators (linters, etc.) strictly throwing errors on extra input fields when validating against a schema would want this disabled.

However, there are big arguments against allowing servers to accept unexpected properties:

• Unless the parser scrubs the input object for expected fields before passing to the resolver, there are potential security issues; for example when a developer uses the input object in a spread. And if those fields are scrubbed, then that's probably violating the intention of the client for those fields to be passed through or preserved.
• A client sending extra fields usually indicates a semantic error. What the server does with extra fields is undefined, but if the fields weren't important, they shouldn't have been included. If the fields were deprecated, there are other mechanisms for that. If the input is actually free-form, there are less complicated ways of handling that case too, for example custom scalars or JSON encoded strings.

Just my two cents!

[leebyron]

Thanks for the thoughts!

Another case I've been thinking about is evolving input objects over time. It's non breaking to add new (optional) fields to an existing input object, however given the current behavior we could not deprecate and remove input fields. As far as input goes, if the server doesn't care about the input anymore, there's no reason to include the field for the client's benefit.

I think probably a version of this that makes the most sense is too not throw an error for additional fields, but also ignore those additional fields as they're not described in the server schema.

The downside here is that runtime validation becomes a little weaker, and misspellings may be harder to debug.

[glan]

As an alternative approach to having this flag it is possible to craft a schema to not introspect an input, see: #303 (comment) as an example of an Any input type.

It's kind of ugly though and completely lacks any API.

[rwe]

@leebyron - It seems that applying deprecation directives to specific fields would solve input type evolution more intuitively. (I think this is what graphql/graphql-spec#197 is suggesting). To deprecate a field one would add the directive to that field in the schema and make sure it's nullable. Am I missing something?

The benefit is that deprecation is explicitly documented, emits warnings, and isn't quite at the threshold of "unexpected" inputs from the perspective of a developer implementing resolvers. Implicitly accepting undocumented input fields seems to just open up a huge bug surface area for both client and server developers.

[lisbakke]

What's the latest on this? I'm looking forward to using this option so that my service isn't as fragile.

[wasauce]

I would love to use this with my service. Any update? Is there something I can do to help?

[lisbakke]

Ping. Is this issue still alive?

[racerxdl]

Ping. This will be merged?

[glan]

It's been a while since I proposed this PR. I somewhat satisfied my own use case by defining Any input types. I'd comment on @leebyron [RFC] Ignore undefined input object fields to continue the discussion.

[leebyron]

I'm going to close this PR since we should avoid changing behavior based on configuration flags in favor of consistent correct behavior. There's an ongoing conversation about this relative to the spec text with good arguments on both sides and no clear outcome yet.