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.

Sign up for GitHub

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
Open

[RFC] Default value coercion rules #793

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8939e20
Default value coercion rules
benjie Nov 13, 2020
25da056
Fix 'must not cause an infinite loop' wording.
benjie Feb 19, 2021
f73af73
Reorder assertions to prevent infinite loop during coercion
benjie Feb 26, 2021
5e793fb
Reduce diff
benjie Feb 26, 2021
084b2d5
Clarify the default value cycle detection logic
benjie May 3, 2021
475d697
Rewrite default value cycle algorithm in the style of DetectFragmentC…
benjie Jan 6, 2022
dde1f7b
Rewrite input object cycle algorithm to match GraphQL.js implementation
benjie Jan 6, 2022
12e26d8
Asterisks
benjie Jan 6, 2022
2838c4f
Remove unnecessary step
benjie Jan 6, 2022
9b7bfd5
Add non-normative note about memoizing default value coercion
benjie Jan 6, 2022
bb6ab78
Merge branch 'main' into input-object-default-value
benjie Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion spec/Section 3 -- Type System.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -818,6 +818,8 @@ of rules must be adhered to by every Object type in a GraphQL schema.
characters {"__"} (two underscores).
2. The argument must accept a type where {IsInputType(argumentType)}
returns {true}.
3. If the argument has a default value it must be compatible with
{argumentType} as per the coercion rules for that type.
3. An object type may declare that it implements one or more unique interfaces.
4. An object type must be a super-set of all interfaces it implements:
1. Let this object type be {objectType}.
Expand Down Expand Up @@ -1520,7 +1522,8 @@ defined by the input object type and for which a value exists. The resulting map
is constructed with the following rules:

* If no value is provided for a defined input object field and that field
definition provides a default value, the default value should be used. If no
definition provides a default value, the result of coercing the default value
according to the coercion rules of the input field type should be used. If no
default value is provided and the input object field's type is non-null, an
error should be thrown. Otherwise, if the field is not required, then no entry
is added to the coerced unordered map.
Expand Down Expand Up @@ -1580,6 +1583,17 @@ Literal Value | Variables | Coerced Value
characters {"__"} (two underscores).
3. The input field must accept a type where {IsInputType(inputFieldType)}
returns {true}.
4. If the input field has a non-null default value:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this just be:

Suggested change
4. If the input field has a non-null default value:
4. If the input field has a default value:

Since a null default value might fail the inner step 2, "must be compatible with {inputFieldType}" if that type might be a Non-Null

Copy link
Member Author

@benjie benjie Apr 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The non-null is here because if the default for the field is null (assuming this is compatible with inputFieldType), then no further validation is required since no recursion through the default value can occur. Consider the A vs B types in the below scenarios; both are fine, but the validation of A1 can stop at the A1.a2 field immediately, whereas the validation of B1 has to traverse through B2 and B3 to get back to finding that the nested reference to B1 via B3.b1 defaults to null.

input A1 {
  a2: A2 = null
}

input A2 {
  a3: A3 = {}
}

input A3 {
  a1: A1 = null
}
input B1 {
  b2: B2 = {}
}

input B2 {
  b3: B3 = {}
}

input B3 {
  b1: B1 = null
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm less concerned about the recursion (nested step 1) and more about the type correctness (nested step 2)

4. If the input field has a non-null default value:
  ...
  2. {defaultValue} must be compatible with {inputFieldType} as per the
     coercion rules for that type.

This implies that for the example:

input Test {
  a: String! = null
}

when validating input field a, it would see that the default value is null and not progress to determine if it is compatible with String! (which it would and should fail)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is no longer relevant with the rewriting of the text I think.

1. If the input field references this Input Object either directly or
through referenced Input Objects, all input fields in the chain of
references which reference this Input Object must either:
1. have no default value; or
2. have a {null} default value; or
3. have a default value, {nestedDefaultValue}, such that the value for
this field within {nestedDefaultValue} is either {null} or an empty
list.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this possibly be incorporated into the existing step 3 below?

Perhaps:

at least one of the fields in the chain of references must be either a nullable or a List type.

adds something along the lines of "and have either no default value, or a default value of either {null} or an empty list"

Copy link
Member Author

@benjie benjie Apr 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so, these constraints live at different levels. Step 3 below relates to the entire "default value" either being not present, null, or an empty list; however the text here (above) relates to the fields of the object within the default value. E.g. the following example:

input A {
  b: B = {}
}

input B {
  c: C = { a: null }
}

input C {
  a: A = {}
}

is fine since no matter whether we reference type A, B or C initially the chain will terminate at C.a since the default for B.c includes that C.a is null. Note: at no point is the default value for a field null or an empty list.

The wording could definitely do with some work though 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Scratch that; I think you're right but the wording you've suggested doesn't quite work (see above example). Maybe:

  1. If an Input Object references itself either directly or through referenced
    Input Objects:
    1. at least one of the fields in the chain of references must be
      either a nullable or a List type.
    2. at least one of the fields in the chain must have no default value, have a default value of either {null} or an empty list, or have a default value that results in all instances of one of the fields in the chain defaulting to {null} or an empty list)

Bit awkward wording, but hopefully you catch my drift. i.e. allow the following:

input A {
  b: B! = {}
}

input B {
  c: C = {}
}

input C {
  a: A! = { b: { c: null} }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

These are very helpful examples! I was definitely too restrictive in my rule set. I think what I currently have implemented in PR would reject all cycles, but would also incorrectly reject this example.

I agree that wording isn't quite right, but it's much closer. I like that this has just one rule about circular references but it makes it clear that there are multiple ways to have illegal circular references - and the above rule can specifically be about type checking the default value

Copy link
Contributor

Choose a reason for hiding this comment

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

just to be clear here:
the coerced default value for A = {b: {c: {a: {b: {c: null}}}}}.
the coerced default value for B = {c: {a: {b: {c: null}}}}.
the coerced default value for C = {a: {b: {c: null}}}

That correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

also: we would have to apply this validation rule also for default variable values, right?

Copy link
Collaborator

@leebyron leebyron Apr 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we also need it for variable values or argument defaults since those can only be the entry to a circular ref but not actually contained within a circular ref.

input A {
  a: A = {}
}

query Test ($x: A = {}) {}

This is problematic because A.a creates a circular reference, but $x only references it, it isn't contained in it.

I think more generally you can say that given a set of Input Objects that have no circular reference, there exists no default value for arguments or variable values that would create a circular reference.

Copy link
Collaborator

@leebyron leebyron Apr 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

just to be clear here:
the coerced default value for A = {b: {c: {a: {b: {c: null}}}}}.
the coerced default value for B = {c: {a: {b: {c: null}}}}.
the coerced default value for C = {a: {b: {c: null}}}

That correct?

Coerced default values apply to fields, not types - so:

the coerced default value for A.b = {c: {a: {b: {c: null}}}}.
the coerced default value for B.c = {a: {b: {c: null}}}.
the coerced default value for C.a = {b: {c: null}}

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I've had a go at writing an algorithm for this; I think the key learning whilst attempting this was that the act of referencing the default value a second time is what triggers a cycle; so the algoritm only tracks when the default value of a field is referenced.

2. {defaultValue} must be compatible with {inputFieldType} as per the
coercion rules for that type.
3. If an Input Object references itself either directly or through referenced
Input Objects, at least one of the fields in the chain of references must be
either a nullable or a List type.
Expand Down
8 changes: 6 additions & 2 deletions spec/Section 6 -- Execution.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,10 @@ CoerceVariableValues(schema, operation, variableValues):
* Let {value} be the value provided in {variableValues} for the
name {variableName}.
* If {hasValue} is not {true} and {defaultValue} exists (including {null}):
* Let {coercedDefaultValue} be the result of coercing {defaultValue} according to the
input coercion rules of {variableType}.
* Add an entry to {coercedValues} named {variableName} with the
value {defaultValue}.
value {coercedDefaultValue}.
* Otherwise if {variableType} is a Non-Nullable type, and either {hasValue}
is not {true} or {value} is {null}, throw a query error.
* Otherwise if {hasValue} is true:
Expand Down Expand Up @@ -586,8 +588,10 @@ CoerceArgumentValues(objectType, field, variableValues):
name {variableName}.
* Otherwise, let {value} be {argumentValue}.
* If {hasValue} is not {true} and {defaultValue} exists (including {null}):
* 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 •
edited
Loading

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.

* Add an entry to {coercedValues} named {argumentName} with the
value {defaultValue}.
value {coercedDefaultValue}.
* Otherwise if {argumentType} is a Non-Nullable type, and either {hasValue}
is not {true} or {value} is {null}, throw a field error.
* Otherwise if {hasValue} is true:
Expand Down