Skip to content
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

[RFC] Default value coercion rules #793

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

benjie
Copy link
Member

@benjie benjie commented Nov 13, 2020

Coercing Field Arguments states:

5.c. Let defaultValue be the default value for argumentDefinition.
[...]
5.h. If hasValue is not true and defaultValue exists (including null):
5.h.i. Add an entry to coercedValues named argumentName with the value defaultValue.
[...]
5.j.iii.1. If value cannot be coerced according to the input coercion rules of argumentType, throw a field error.
5.j.iii.2. Let coercedValue be the result of coercing value according to the input coercion rules of argumentType.
5.j.iii.3. Add an entry to coercedValues named argumentName with the value coercedValue.

Here we note that there is no run-time coercion of defaultValue, which makes sense (why do at runtime that which can be done at build time?). However, there doesn't seem to be a rule that specifies that defaultValue must be coerced at all, which leads to consequences:

  1. you could use any value for defaultValue and it could break the type safety guarantees of GraphQL
  2. nested defaultValues are not applied

When building the following GraphQL schema programmatically (code below) with GraphQL.js:

type Query {
  example(inputObject: ExampleInputObject! = {}): Int
}

input ExampleInputObject {
  number: Int! = 3
}

And a resolver for Query.example:

resolve(source, args) {
  return args.inputObject.number;
}

You might expect the following queries to all gave the same result:

query A {
  example
}
query B {
  example(inputObject: {})
}
query C {
  example(inputObject: { number: 3 })
}
query D($inputObject: ExampleInputObject! = {}) {
  example(inputObject: $inputObject)
}

However, it turns out that query A's result differs:

{"example":null}
{"example":3}
{"example":3}
{"example":3}

This is because defaultValue for Query.example(inputObject:) was not coerced, so none of the defaultValues of ExampleInputObject were applied.

This is extremely unexpected, because looking at the GraphQL schema definition it looks like there's no circumstance under which ExampleInputObject may not have number as an integer; however when the defaultValue of Query.example(inputObject:) is used, the value of number is undefined.

Example runnable with GraphQL.js
const {
  graphqlSync,
  printSchema,
  GraphQLSchema,
  GraphQLObjectType,
  GraphQLInputObjectType,
  GraphQLInt,
  GraphQLNonNull,
} = require("graphql");

const ExampleInputObject = new GraphQLInputObjectType({
  name: "ExampleInputObject",
  fields: {
    number: {
      type: new GraphQLNonNull(GraphQLInt),
      defaultValue: 3,
    },
  },
});

const Query = new GraphQLObjectType({
  name: "Query",
  fields: {
    example: {
      args: {
        inputObject: {
          type: new GraphQLNonNull(ExampleInputObject),
          defaultValue: {},
        },
      },
      type: GraphQLInt,
      resolve(source, args) {
        return args.inputObject.number;
      },
    },
  },
});

const schema = new GraphQLSchema({
  query: Query,
});

console.log(printSchema(schema));

// All four of these should be equivalent?
const source = /* GraphQL */ `
  query A {
    example
  }
  query B {
    example(inputObject: {})
  }
  query C {
    example(inputObject: { number: 3 })
  }
  query D($inputObject: ExampleInputObject! = {}) {
    example(inputObject: $inputObject)
  }
`;
const result1 = graphqlSync({ schema, source, operationName: "A" });
const result2 = graphqlSync({ schema, source, operationName: "B" });
const result3 = graphqlSync({ schema, source, operationName: "C" });
const result4 = graphqlSync({ schema, source, operationName: "D" });

console.log(JSON.stringify(result1.data));
console.log(JSON.stringify(result2.data));
console.log(JSON.stringify(result3.data));
console.log(JSON.stringify(result4.data));

This was raised against GraphQL.js back in 2016, but @IvanGoncharov closed it early last year stating that GraphQL.js conforms to the GraphQL Spec in this regard.

My proposal is that when a defaultValue is specified, the GraphQL implementation should coerce it to conform to the relevant type just like it does for runtime values as specified in Coercing Variable Values and Coercing Field Arguments.


This is validated for query documents (and schema defined as SDL), because:

Literal values must be compatible with the type expected in the position they are found as per the coercion rules defined in the Type System chapter.
-- http://spec.graphql.org/draft/#sel-FALXDFDDAACFAhuF

But there doesn't seem to be any such assertion for GraphQL schemas defined in code.

@benjie
Copy link
Member Author

benjie commented Nov 13, 2020

An alternative approach is to apply the coercion during execution in the same way it is done for user-supplied values (the spec mods for this would look like: cb29f6e) but this could of course be done once and cached at schema build time.

EDIT: the reason to do it this way would be so that the defaultValue could appear more naturally in print(schema) as inputObject: ExampleInputObject! = {} (i.e. as specified, and as the user would expect) rather then as inputObject: ExampleInputObject! = { number: 3 } which would be the result of the coercion.

@benjie
Copy link
Member Author

benjie commented Nov 13, 2020

NOTE: we'd also have to prevent infinite recursion in default values; for example the following should throw a schema validation error:

input O {
  o: O = {}
  a: Int
}

but the following should be fine:

input O {
  o: O = { o: null }
  a: Int
}

@eapache
Copy link
Member

eapache commented Nov 18, 2020

Whether a server does the coercion at execution time or build time is largely an implementation detail, but I do think we should specify the more natural behaviour that comes from doing it at execution time: expected output from print(schema), and the "free" infinite recursion guard that presumably already protects coercion in regular runtime inputs.

graphql-ruby at least already seems to try to coerce the default value, but if the coercion fails it just silently passes a null to the resolver even if the argument is required. It would be great to have this behaviour well-defined.

@benjie
Copy link
Member Author

benjie commented Nov 18, 2020

I do think we should specify the more natural behaviour that comes from doing it at execution time: expected output from print(schema), and the "free" infinite recursion guard that presumably already protects coercion in regular runtime inputs.

This is exactly what I'm thinking too; it's why I closed my graphql-js PR because I think the solution's going to be somewhat more involved.

Comment on lines 591 to 592
* Let {coercedDefaultValue} be the result of coercing {defaultValue} according to the
input coercion rules of {argumentType}.
Copy link
Member Author

@benjie benjie Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: this is memoizeable and could be computed at schema build time (without affecting the introspection results).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: add this as a non-normative note below.

@benjie
Copy link
Member Author

benjie commented Nov 25, 2020

Put a little more time into this spec edit, I think it's much more viable now.

@IvanGoncharov
Copy link
Member

@benjie I'm all for adding validation on default values but I'm against applying coercion rules on them, because:

  1. Input coercion rules are done to simplify passing input values for clients but the same simplicity makes it less obvious for readers of SDL (they need to figure out coercion rules).
  2. Coercion rules for scalars are server-specific (even for standard scalars) so we exposing this behavior through introspection. For example for graphql-js default values "5" and 5 is equal for ID scalar but you can't reliably know that from introspection and it can affect tooling (e.g. tool that generates changelog).
  3. Introduces recursion during coercion and non-trivial validation rule.
  4. So far we designed SDL in such a way that every type is self-descriptive (e.g. need to specify all fields from interfaces it implements).
  5. It's a breaking change for current SDL files and more importantly it would be triggered by a simple update of GraphQL lib so it would be the first SDL breaking change we added that is not opt-in.

@benjie
Copy link
Member Author

benjie commented Dec 4, 2020

The infinite recursion during coercion problem

The following schema is (I believe) valid GraphQL currently:

# SCHEMA 1

input A {
  b: B = {}
}

input B {
 a: A = {}
}

type Query {
  q(a: A = {}): Int
}

(Note: we could also do this with a self-reference input A { a: A = {} } but I felt spreading it across two types make it clearer there's some walking to do.)

In current implementations this will be fine because the default value is not coerced and thus there is no recursion.

However, if we implement this RFC and ignore the "must not cause an infinite loop" part (highlighted by @andimarek) then this could cause infinite recursion during coercion.

However; the following GraphQL schema would be fine as there'd be no infinite recursion:

# SCHEMA 2

input A {
  b: B = {}
}

input B {
 a: A = {b: null}
}

type Query {
  q(a: A = {}): Int
}

Once we reach a situation where B.a's default value will be used the chain will end because the default for B.a explicitly provides that the next A.b will be null.

I think we need a schema validation rule that invalidates "SCHEMA 1". This would be a breaking change to the GraphQL spec, but I think it's an acceptable one and can be easily resolved by GraphQL schema designers by explicitly changing the defaults from {} to {a: null} / {b: null} as appropriate - a very minor fix that will not break client queries.

I do not know how to write this change into the spec (hence the "must not cause an infinite loop" placeholder); @leebyron could you give some hints on the approach you'd take?

@benjie
Copy link
Member Author

benjie commented Dec 4, 2020

@IvanGoncharov mentioned that #701 might help with the recursive defaults problem, but alas I don't think it will as currently stated - the RFC seems to relate to making sure fields are nullable to prevent expressing a state that's not representable via a JSON input, this doesn't solve the infinite recursion issue with default values above (note the fields above are all nullable).

@benjie
Copy link
Member Author

benjie commented Dec 4, 2020

@IvanGoncharov in response to your feedback, we discussed this during the WG but I'll include a summary of my positions here:

Input coercion rules are done to simplify passing input values for clients but the same simplicity makes it less obvious for readers of SDL (they need to figure out coercion rules).

For me, seeing = {} means that I would read it as "this would be the same as if I were to pass {} explicitly" which is currently not the case so I'd actually argue the opposite of this: people reading the SDL have to know that the defaults will not be applied, which is entirely unexpected.

Ultimately, it is up to the people who develop the schema to decide if this is an issue for them or not - if it is they can consider not using defaultValue at all.

Coercion rules for scalars are server-specific (even for standard scalars) so we exposing this behavior through introspection. For example for graphql-js default values "5" and 5 is equal for ID scalar but you can't reliably know that from introspection and it can affect tooling (e.g. tool that generates changelog).

Can you expand on this, and perhaps suggest how the proposal can address it/minimise it? To my mind the defaultValue should be exactly an equivalent of the user passing a value explicitly, so anything a user can pass explicitly should be representable as defaultValue and vice versa.

Introduces recursion during coercion and non-trivial validation rule.

Indeed, I think this is an acceptable trade-off.

So far we designed SDL in such a way that every type is self-descriptive (e.g. need to specify all fields from interfaces it implements).

This was discussed in the WG, and I feel that default value / passed value equivalence (i.e. seeing = {} should be the same as passing {} explicitly) outweighs this concern by a significant degree.

It's a breaking change for current SDL files and more importantly it would be triggered by a simple update of GraphQL lib so it would be the first SDL breaking change we added that is not opt-in.

Agree, however the fix is straightforward and it would be determined at schema validation time (not runtime) so should be quick to address for implementors. Further, if implementors are already doing what you say they should be doing (i.e. supplying the full expected default value, applying whatever defaults are needed manually) then their schemas will not be broken by this change.

@Alan-Cha
Copy link

Alan-Cha commented Jan 7, 2021

During the January working group meeting, we discussed the idea of sharing example schemas so we have a better idea of how to move forward and I mentioned that I worked on a research paper where we analyzed a large corpus (8,399) of open source schemas that we collected from GitHub.

I mentioned that there may be complications with sharing this corpus but it turns out I was misremembering some of the details. We were not able to directly publish the raw schemas and "recovered" schemas (schemas formed by merging small sub-schemas), but we were able to publish links to the commit-stamped files!

See here.

Additionally, we also published the scripts used to collect, clean, and refactor all the schemas in the corpus. This could be used to generate a new up to date corpus from GitHub.

See here for the scripts.

@mmatsa
Copy link

mmatsa commented Jan 19, 2021

@benjie asked for real examples of use. Here’s an “anonymized” example from a real schema:

input Z {
  x: Int = 1
  y: Int = 2
}

input A {
  b: Z = {x: 1, y: 2}
  c: Z = {x: 1, y: 3}
}

type T {
    f(a: A = {b: {x: 1, y: 2}, c: {x: 1, y: 3}}): F

Note that there is really use of multiple levels of default values here. Given the spec today the f() definition is required to override them even though the values are identical. This schema uses default input objects for arguments hundreds of times. Obviously, it must overwrite the values, so I think that either of the changes proposed to the spec recently wouldn't break it. I'm sharing it as only one example, per @benjie 's request.

Base automatically changed from master to main February 3, 2021 04:50
@mcohen75
Copy link

mcohen75 commented Feb 4, 2021

Providing another example here from a real-world schema from Indeed's GraphQL API.

We have a number of query operations that define nested input types that specify default values.

The most prominent example is the core job search operation:

jobSearch(
  cursor: String
  filters: [JobSearchFilterInput!]! = []
  grouping: JobSearchGroupingInput! = {type: EMPLOYER_TITLE}
  includeSponsoredResults: JobSearchIncludeSponsoredResults! = NONE
  lastSearchTime: Timestamp
  limit: Int! = 10
  location: JobSearchLocationInput! = {radius: 0, radiusUnit: KILOMETERS, where: ""}
  numNextPages: Int! = 1
  numPrevPages: Int! = 1
  sort: JobSearchSortOrder! = RELEVANCE
  what: String! = ""
): JobSearchPayload

Note that the default values are specified inline for nested inputs.

@dwwoelfel
Copy link

GitHub uses objects as defaultValues a lot in their schema for their orderBy arguments.

For example, Query.securityAdvisories looks something like:

securityAdvisories(
   ...
  orderBy: GitHubSecurityVulnerabilityOrder = {field: UPDATED_AT, direction: DESC}
   ...
): GitHubSecurityVulnerabilityConnection!

@benjie
Copy link
Member Author

benjie commented Feb 19, 2021

A developer from a large company (household name) DM'd me on Twitter to share parts of their schema, and they have a similar pattern to GitHub for ordering; the relevant parts of their schema looks a bit like (manually anonymised):

enum SortDirection {
  ASC
  DESC
}

input FooOrder {
  sort: FooSort!
  direction: SortDirection! = ASC
}

type Query {
  foos(orderBy: FooOrder = {sort: DATE, direction: ASC}): FooConnection
}

They also use this for conditions:

enum OperatorEnum {
  OR
  AND
}

input BarSearchInput {
  queries: [BarQuery]
  queryOperator: OperatorEnum = OR
}

type Query {
  barSearch(search: BarSearchInput = {queryOperator: OR}): BarConnection
}

And have defaults used for input objects used in mutations:

enum BazType {
  CAT
  HEDGEHOG
  ZEBRA
}

input BazInput {
  type: BazType = CAT
  url: String!
  name: String
}

type Mutation {
  addBaz(quxId: ID!, baz: BazInput!): Baz
}

@netlify
Copy link

netlify bot commented Jan 6, 2022

❌ Deploy Preview for graphql-spec-draft failed.

🔨 Explore the source changes: 9b7bfd5

🔍 Inspect the deploy log: https://app.netlify.com/sites/graphql-spec-draft/deploys/61d6e26d6152a7000846e0fb

@benjie
Copy link
Member Author

benjie commented Jan 6, 2022

I've re-written the algorithm to detect cycles in input objects following @leebyron's implementation in graphql/graphql-js@fceda32

yaacovCR pushed a commit to yaacovCR/graphql-executor that referenced this pull request Jan 2, 2023
Implements graphql/graphql-spec#793

* Adds validation of default values during schema validation.
* Adds coercion of default values anywhere a default value is used at runtime

Potentially breaking:
* Remove `astFromValue`
* Changes type of `defaultValue` provided during type configuration from an "internal" to an "external" value.
yaacovCR pushed a commit to yaacovCR/graphql-executor that referenced this pull request Jan 2, 2023
Implements graphql/graphql-spec#793

* Adds validation of default values during schema validation.
* Adds coercion of default values anywhere a default value is used at runtime

Potentially breaking:
* Remove `astFromValue`
* Changes type of `defaultValue` provided during type configuration from an "internal" to an "external" value.
@netlify
Copy link

netlify bot commented Jan 31, 2023

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit bb6ab78
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/63d938f4059bf200083a6542
😎 Deploy Preview https://deploy-preview-793--graphql-spec-draft.netlify.app/draft
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

yaacovCR pushed a commit to yaacovCR/graphql-executor that referenced this pull request Jan 31, 2023
Implements graphql/graphql-spec#793

* Adds validation of default values during schema validation.
* Adds coercion of default values anywhere a default value is used at runtime

Potentially breaking:
* Remove `astFromValue`
* Changes type of `defaultValue` provided during type configuration from an "internal" to an "external" value.
yaacovCR pushed a commit to yaacovCR/graphql-executor that referenced this pull request Feb 1, 2023
Implements graphql/graphql-spec#793

* Adds validation of default values during schema validation.
* Adds coercion of default values anywhere a default value is used at runtime

Potentially breaking:
* Remove `astFromValue`
* Changes type of `defaultValue` provided during type configuration from an "internal" to an "external" value.
yaacovCR pushed a commit to yaacovCR/graphql-executor that referenced this pull request Feb 6, 2023
Implements graphql/graphql-spec#793

* Adds validation of default values during schema validation.
* Adds coercion of default values anywhere a default value is used at runtime

Potentially breaking:
* Remove `astFromValue`
* Changes type of `defaultValue` provided during type configuration from an "internal" to an "external" value.
yaacovCR pushed a commit to yaacovCR/graphql-executor that referenced this pull request Feb 6, 2023
Implements graphql/graphql-spec#793

* Adds validation of default values during schema validation.
* Adds coercion of default values anywhere a default value is used at runtime

Potentially breaking:
* Remove `astFromValue`
* Changes type of `defaultValue` provided during type configuration from an "internal" to an "external" value.
yaacovCR pushed a commit to yaacovCR/graphql-executor that referenced this pull request Feb 6, 2023
Implements graphql/graphql-spec#793

* Adds validation of default values during schema validation.
* Adds coercion of default values anywhere a default value is used at runtime

Potentially breaking:
* Remove `astFromValue`
* Changes type of `defaultValue` provided during type configuration from an "internal" to an "external" value.
yaacovCR pushed a commit to yaacovCR/graphql-executor that referenced this pull request Feb 6, 2023
Implements graphql/graphql-spec#793

* Adds validation of default values during schema validation.
* Adds coercion of default values anywhere a default value is used at runtime

Potentially breaking:
* Remove `astFromValue`
* Changes type of `defaultValue` provided during type configuration from an "internal" to an "external" value.
yaacovCR pushed a commit to yaacovCR/graphql-executor that referenced this pull request May 31, 2023
Implements graphql/graphql-spec#793

* Adds validation of default values during schema validation.
* Adds coercion of default values anywhere a default value is used at runtime

Potentially breaking:
* Remove `astFromValue`
* Changes type of `defaultValue` provided during type configuration from an "internal" to an "external" value.
yaacovCR pushed a commit to yaacovCR/graphql-executor that referenced this pull request May 31, 2023
Implements graphql/graphql-spec#793

* Adds validation of default values during schema validation.
* Adds coercion of default values anywhere a default value is used at runtime

Potentially breaking:
* Remove `astFromValue`
* Changes type of `defaultValue` provided during type configuration from an "internal" to an "external" value.
yaacovCR pushed a commit to yaacovCR/graphql-executor that referenced this pull request May 31, 2023
Implements graphql/graphql-spec#793

* Adds validation of default values during schema validation.
* Adds coercion of default values anywhere a default value is used at runtime

Potentially breaking:
* Remove `astFromValue`
* Changes type of `defaultValue` provided during type configuration from an "internal" to an "external" value.
yaacovCR pushed a commit to yaacovCR/graphql-executor that referenced this pull request May 31, 2023
Implements graphql/graphql-spec#793

* Adds validation of default values during schema validation.
* Adds coercion of default values anywhere a default value is used at runtime

Potentially breaking:
* Remove `astFromValue`
* Changes type of `defaultValue` provided during type configuration from an "internal" to an "external" value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 Draft (RFC 2) RFC Stage 2 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet