Clarify execution section. #221

leebyron · 2016-10-21T04:33:32Z

This adds algorithms to the section on execution and reorders content to better follow the flow of execution.

Additional semantics are being introduced in this PR. In addition to algorithmic clarification, this makes Variable value and Argument value coercion more strict for non-null typed values.

Also, the coercion of Variables and Argument values previously did not distinguish between null and not-provided for the application of default values. This describes that when explicitly providing null, a default value does not apply.

This adds algorithms to the section on execution and reorders content to better follow the flow of execution. Note that no additional semantics are being introduced in this PR. This is simply algorithmic clarification of the execution process.

jjergus · 2016-10-21T16:34:14Z

spec/Section 3 -- Type System.md

+If any non-nullable fields defined by the input object do not have corresponding
+entries in the original value, were provided a variable for which a value was
+not provided, or for which the value {null} was provided, an error should
+be thrown.


With the introduction of a null literal, do we need a separate concept of "non-nullable" and "required"? For example, should it be possible to declare an input object field which must be provided, but its value may be null?

{ required_field: 42 } # valid { required_field: null } # valid { } # invalid { required_field: $var } # valid if $var is required but $var may be nullable...I guess?

I'll make sure to specifically address this point in the null literal proposal.

I think it's actually not worth separating these notions. I think there probably isn't a ton of value beyond fairly pedantic specificity in the cases of required but nullable and optional but non-nullable.

jjergus · 2016-10-21T16:38:52Z

spec/Section 3 -- Type System.md

+be thrown.
+
+The result of coercion is an environment-specific unordered map defining slots
+for each field of the input object type.


"Each field of the input object type" or "each field included in the value before coercion"?

If the former, how do we distinguish between fields that weren't provided and fields that had explicit null value?

I'll clarify this for this section, but leave the null value handling to the null literal proposal.

jjergus · 2016-10-21T16:41:35Z

spec/Section 3 -- Type System.md

+For each field of the input object type, if the original value has an entry with
+the same name, and the value at that entry is a literal value or a variable
+which was provided a runtime value, an entry is added to the result with the
+name of the field.


If I understand it correctly

{ optional_field: $var }

coerces to

{ optional_field: 42 } # if $var == 42 { optional_field: null } # if $var is explicitly null { } # if $var was not provided

I guess that makes sense but we should probably have some examples here.

Although the case where a field that was explicitly included in the input object literal can "disappear" if its value is an optional variable feels non-intuitive to me, I'm not sure if that's the right call.

That's the current behavior at least :/ - I'll definitely include examples, that's a great idea.

If the behavior seems wrong we can discuss proposing to change it.

jjergus · 2016-10-21T16:46:00Z

spec/Section 6 -- Execution.md


-To evaluate a request, the executor must have a parsed `Document` (as defined
+Given this information, the result of {ExecuteRequest()} produces the response,
+to be formatted according to the Reponse section below.


missing s in Reponse

jjergus · 2016-10-21T16:58:42Z

spec/Section 6 -- Execution.md

+* A Document containing GraphQL Operations and Fragments to execute.
+* Optionally: The name of the Operation in the Document to execute.
+* Optionally: Values for Variables defined by the Operation.
+* Optionally: An initial value corresponding to the root type being executed.


I don't know about this -- so the same query with the same variables could return different results if executed with different "initial values"? What would that be useful for?

This matches current reality for the semantics. The root type and executing the top level selection set is almost exactly the same as executing any other selection set (with the exception of mutations being executed serially).

You can think of initialValue as the universe over which you're running the query. For pure functional environments this value is critical. It's also totally reasonable for a server to only ever have one of these.

jjergus · 2016-10-21T17:00:25Z

spec/Section 6 -- Execution.md

-operations” section.
+error. If the operation is found, then the result of executing the request
+should be the result of executing the operation according to the "Executing
+Operations” section.


This paragraph is redundant since the same algorithm is more accurately described below it.

jjergus · 2016-10-21T17:02:11Z

spec/Section 6 -- Execution.md


-## Evaluating operations
+CoerceVariableValues(schema, operation, variableValues)


Missing algorithm here?

Also if we provide an algorithm here, the paragraphs above can be shortened or removed probably.

Whoops. Good catch. I'll fill that in.

I think the prose is still helpful, even if redundant as long as presenting another way to think about why the algorithm is doing what it is doing.

jjergus · 2016-10-21T17:02:57Z

spec/Section 6 -- Execution.md

+ExecuteQuery(query, schema, variableValues, initialValue):
+
+  * Let {queryType} be the root Query type in {schema}.
+  * Assert: {queryType} is an Object type.


This should be asserted during schema validation, not during query execution.

jjergus · 2016-10-21T17:06:14Z

spec/Section 6 -- Execution.md

+ExecuteMutation(mutation, schema, variableValues, initialValue):
+
+  * Let {variableValues} be the set of variable values to be used by any
+    field argument value coercion.


This step seems redundant, doesn't say anything useful (and we don't have it in the "query" algorithm above).

goodcatch - this was a mistake :)

jjergus · 2016-10-21T17:06:24Z

spec/Section 6 -- Execution.md

+  * Let {variableValues} be the set of variable values to be used by any
+    field argument value coercion.
+  * Let {mutationType} be the root Mutation type in {schema}.
+  * Assert: {mutationType} is an Object type.


Same as above (schema validation).

jjergus · 2016-10-21T17:09:04Z

spec/Section 6 -- Execution.md

-`CollectFields`, initializing `visitedFragments` to an empty list.
+Note: Normally, each call to {GetFieldEntry()} in the algorithm above is
+performed in parallel. However there are conditions in which each call must be
+done in serial, such as for mutations. This is explain in more detail in the


missing "ed" in "explained"

jjergus · 2016-10-21T17:36:40Z

spec/Section 6 -- Execution.md

+    {CollectFields(objectType, selectionSet, visitedFragments, variableValues)}.
+  * Initialize {resultMap} to an empty ordered map.
+  * For each {groupedFields} in {groupedFieldSet}:
+    * Let {entryTuple} be {GetFieldEntry(objectType, objectValue, groupedFields, variableValues)}.


This tuple thing seems hacky and non-intuitive. The "responseKey" is just the key for the current entry in "groupedFieldSet", so there is no reason to have GetFieldEntry return it. GetFieldEntry doesn't, and shouldn't have the ability to affect the response key of the current field.

It's not very clear from just looking at the algorithm, but as far as I can tell the only reason we're returning a tuple is so that we can distinguish between the "field should be omitted" case and the "field should have null value" case. I think we could find a more elegant way to do that.

I would rewrite this part to be something like:

For each {responseKey: groupedFields} in {groupedFieldSet}:

Let {fieldName} be ...

Let {fieldType} be the result of calling {GetFieldTypeFromObjectType(objectType, fieldName)}

If {fieldType} is not null:

Let {responseValue} be {GetFieldValue(...)}

Set {responseValue} as the value for {responseKey} in {resultMap}.

Another possibility would be to at least change the first value in the tuple to be a bool flag "should include field" instead of making it the response key just so that we can check if it's null.

The tuple approach was decided when @dschafer and I were writing this section last year. Namely we need to distinguish between a field value of null vs a field not being included at all. Otherwise as you're pointing out, a larger part of the GetFieldEntry would end up in this algorithm instead. It aligns to the reference implementation (which in JS returns either null as a value or undefined as a skip).

I like this suggestion though - I'd like to explore this as a follow up since it might also suggest changing the reference implementation as well.

Can we have a little bit more explanation or example regarding {responseKey: groupedFields}, in what case a responseKey might have grouped fields? In such case how to determine the result of responseKey.

jjergus · 2016-10-21T17:41:18Z

spec/Section 6 -- Execution.md


  * Initialize {groupedFields} to an empty ordered list of lists.
-  * For each {selection} in {selectionSet};
+  * For each {selection} in {selectionSet}:
    * If {selection} provides the directive `@skip`, let {skipDirective} be that directive.


We should make it clear (if it isn't already) somewhere in the spec that each selection item can only have one @skip/@include directive. Probably add a validation rule?

e.g. some_field @skip(if: $var1) @skip(if: $var2) is not valid

This actually isn't already a validation rule and is valid, though it does weird things. Changing this is probably not appropriate in an editing PR, but we should definitely fix this.

Filed #223

jjergus · 2016-10-21T17:46:27Z

spec/Section 6 -- Execution.md

+entry in the result map for this item. Note that this is distinct from
+returning an entry with a string key and a {null} value, which indicates that
+an entry in the result should be added for that key, and its value should
+be {null}.


Like I commented above, I don't think this should return a tuple with the response key. It is the responsibility of ExecuteSelectionSet+CollectFields to determine response keys for each item, and GetFieldEntry doesn't and shouldn't have the ability to affect the response keys.

I would refactor this to be GetFieldValue instead of GetFieldEntry and make it the responsibility of ExecuteSelectionSet to determine which fields must be omitted like I described above, but there are other options too.

jjergus · 2016-10-21T17:58:12Z

spec/Section 6 -- Execution.md

+      {resultItem} is each item in {result}.
+  * If {fieldType} is a Scalar or Enum type:
+    * Return the result of "coercing" {result}, ensuring it is a legal value of
+      {fieldType}, otherwise {null}.


Should an error be added to the list of errors if coercion fails?

Either way -- this should have consistent behavior with what happens for list type if result is not a collection of values.

jjergus · 2016-10-21T18:01:44Z

spec/Section 6 -- Execution.md

+  * Let {coercedArgumentValues} be an empty Map.
+  * For each {argumentDefinitions} as {argumentName} and {argumentType}:
+    * If no value was provided in {argumentValues} for the name {argumentName}:
+      * Continue to the next argument definition.


...or throw a field exception if the argument is non-nullable?

That's not how things work today, but I agree that's problematic. Filed #224 to follow up with this.

jjergus · 2016-10-21T18:03:15Z

spec/Section 6 -- Execution.md


-If the selection set is being evaluated on the `null` object, then the result
-of evaluating the selection set is `null`.
+  * Initialize {visitedFragments} to be the empty set.


visitedFragments is internal state of the CollectFields function, it should probably be initialized inside it, not passed in.

@jjergus

…Included @jjergus's suggestion of getting rid of the tuple based response keying.

leebyron · 2016-10-22T02:17:15Z

Added some additional changes.

Here's what these changes add up to for easier review https://ns-dtlgzkzepm.now.sh/#sec-Execution

jjergus · 2016-10-24T17:28:36Z

spec/Section 6 -- Execution.md

+    * Let {variableType} be the expected type of {variableDefinition}.
+    * If no value was provided in {variableValues} for the name {variableName}:
+      * If {variableType} is a Non-Nullable type, throw a query error.
+      * Otherwise, continue to the next variable definition.


If the variable has a default value, shouldn't we use that here?

We should also explicitly mention whether the default value should or shouldn't be used if the variable is included in variableValues but its value is null.

jjergus · 2016-10-24T17:30:59Z

spec/Section 6 -- Execution.md

+  * Return {coercedValues}.
+
+Note: This algorithm is very similar to {CoerceArgumentValues()}, however is
+less forgiving of non-coerceable values.


Is it less forgiving by design, or should we change that?

nit: I honestly don't know which is the correct spelling, but we use "coercible" instead of "coerceable" in the rest of the spec.

jjergus · 2016-10-24T17:31:54Z

spec/Section 6 -- Execution.md


-An initial value can be optionally provided when executing a query.
+An initial value may be provided when executing a query.


You made the initial value non-optional above but this still describes it as optional.

jjergus · 2016-10-24T17:34:10Z

spec/Section 6 -- Execution.md

-    * If {entryTuple} is not {null}:
-      * Let {responseKey} and {responseValue} be the values of {entryTuple}.
-      * Set {responseValue} as the value for {responseKey} in {resultMap}.
+  * For each {groupedFieldSet} as {responseKey} and {fields}:


I'm not familiar with common pseudocode conventions so maybe this is OK, but is there maybe a clearer way to show that responseKey is the key in the map and fields is the value? My intuition would be something like

For each {groupedFieldSet} as {responseKey: fields}:

jjergus · 2016-10-24T17:34:59Z

spec/Section 6 -- Execution.md

+    * If {fieldType} is {null}:
+      * Continue to the next iteration of {groupedFieldSet}.
+    * Let {responseValue} be {ExecuteField(objectType, objectValue, fields, fieldType, variableValues)}.
+    * Set {responseValue} as the value for {responseKey} in {resultMap}.


I love the new version, thanks!

jjergus · 2016-10-24T17:55:46Z

spec/Section 6 -- Execution.md

+    * If {value} is a Variable:
+      * If a value exists in {variableValues} for the Variable {value}:
+        * Add an entry to {coercedValues} named {argName} with the
+          value of the Variable {value} found in {variableValues}.


Do we want

Else If {argumentType} is a Non-Nullable type, throw a field error.

here?

I know we don't want to duplicate checks that were done during validation, but it would be consistent with the branch above (If no value was provided in {argumentValues} for the name {argumentName}) -- which is also a duplicated validation-time check. So I'd either add it here too, or remove that one too.

jjergus · 2016-10-24T17:57:55Z

spec/Section 6 -- Execution.md

+      * Let {coercedValue} be the result of coercing {value} according to the
+        input coercion rules of {argType}.
+      * Add an entry to {coercedValues} named {argName} with the
+        value {coercedValue}.


This should also be consistent with whatever choice we make above -- either ignore errors (which is what it currently does) or throw a field error.

jjergus · 2016-10-24T18:07:01Z

spec/Section 6 -- Execution.md

-      * Let {responseKey} and {responseValue} be the values of {entryTuple}.
-      * Set {responseValue} as the value for {responseKey} in {resultMap}.
+  * For each {groupedFieldSet} as {responseKey} and {fields}:
+    * Let {fieldName} be the name of the first entry in {fields}.


I like the phrasing you used for a similar case in CollectFields better:

Let {fieldName} be the name shared by all fields in {fields}.

I think it makes it more intuitive what's going on.

Another option would be to add a note explaining that all {fields} have the same name.

Before this diff, bad input to arguments and variables was often ignored and replaced with `null` rather than rejected. Now that `null` has a semantic meaning, and thanks to some recent changes to the spec (graphql/graphql-spec#221) - changes are necessary in order to enforce the stricter coercion rules. This diff does the following: * Implements the CoerceArgumentValues as described in the spec. * Implements the CoerceVariablesValues as described in the spec. * Alters valueFromAST and coerceValue (dual functions) to strictly enforce coercion, returning `undefined` implicitly when they fail to do so. It also fixes issues where undefined returns were being ignored as items in a list or fields in an input object.

* Enforces input coercion rules. Before this diff, bad input to arguments and variables was often ignored and replaced with `null` rather than rejected. Now that `null` has a semantic meaning, and thanks to some recent changes to the spec (graphql/graphql-spec#221) - changes are necessary in order to enforce the stricter coercion rules. This diff does the following: * Implements the CoerceArgumentValues as described in the spec. * Implements the CoerceVariablesValues as described in the spec. * Alters valueFromAST and coerceValue (dual functions) to strictly enforce coercion, returning `undefined` implicitly when they fail to do so. It also fixes issues where undefined returns were being ignored as items in a list or fields in an input object. * Fix most of the failing tests * Fix missing data from errors * fix lint issues * Add full behavior test cases for valueFromAST * additional value coercion tests * Add appropriate list item behavior for missing vars

@jjergus

* Clarify execution section. This adds algorithms to the section on execution and reorders content to better follow the flow of execution. Note that no additional semantics are being introduced in this PR. This is simply algorithmic clarification of the execution process. * Follow up improvements thanks to @jjergus feedback * Another pass at further improvements to describing these operations. Included @jjergus's suggestion of getting rid of the tuple based response keying. * Note about the purpose of initial value * Add default value rules, further error throwing spots

leebyron force-pushed the exe-clarify branch from a73d85e to 748413f Compare October 21, 2016 04:35

Clarify execution section.

d193f58

This adds algorithms to the section on execution and reorders content to better follow the flow of execution. Note that no additional semantics are being introduced in this PR. This is simply algorithmic clarification of the execution process.

leebyron force-pushed the exe-clarify branch from 748413f to d193f58 Compare October 21, 2016 04:38

jjergus reviewed Oct 21, 2016

View reviewed changes

This was referenced Oct 22, 2016

@skip or @include should only be used once per selection #223

Closed

Ensure non-nullable arguments during execution #224

Closed

leebyron added 3 commits October 21, 2016 17:15

Follow up improvements thanks to @jjergus feedback

237a9cf

Another pass at further improvements to describing these operations. …

7352f04

…Included @jjergus's suggestion of getting rid of the tuple based response keying.

Note about the purpose of initial value

6147aef

jjergus approved these changes Oct 24, 2016

View reviewed changes

Add default value rules, further error throwing spots

a82434f

leebyron merged commit 13194df into master Oct 24, 2016

leebyron deleted the exe-clarify branch October 24, 2016 18:58

leebyron mentioned this pull request Oct 24, 2016

Update to latest spec details for execution argument values graphql/graphql-js#529

Closed

leebyron mentioned this pull request Nov 1, 2016

Enforces input coercion rules. graphql/graphql-js#553

Merged

mhallin mentioned this pull request Jan 27, 2017

Accept explicit null input values in queries graphql-rust/juniper#26

Merged


		## Evaluating operations
		CoerceVariableValues(schema, operation, variableValues)


		An initial value can be optionally provided when executing a query.
		An initial value may be provided when executing a query.

Clarify execution section. #221

Clarify execution section. #221

Conversation

leebyron commented Oct 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjergus Oct 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron commented Oct 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron commented Oct 21, 2016 •

edited

jjergus Oct 21, 2016 •

edited