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

Clarify execution section. #221

Merged
merged 5 commits into from
Oct 24, 2016
Merged

Clarify execution section. #221

merged 5 commits into from
Oct 24, 2016

Conversation

leebyron
Copy link
Collaborator

@leebyron leebyron commented Oct 21, 2016

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.
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

be thrown.

The result of coercion is an environment-specific unordered map defining slots
for each field of the input object type.
Copy link
Contributor

Choose a reason for hiding this comment

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

"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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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.
Copy link
Contributor

@jjergus jjergus Oct 21, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.


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.
Copy link
Contributor

Choose a reason for hiding this comment

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

missing s in Reponse

* 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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


## Evaluating operations
CoerceVariableValues(schema, operation, variableValues)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing algorithm here?

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

ExecuteQuery(query, schema, variableValues, initialValue):

* Let {queryType} be the root Query type in {schema}.
* Assert: {queryType} is an Object type.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

ExecuteMutation(mutation, schema, variableValues, initialValue):

* Let {variableValues} be the set of variable values to be used by any
field argument value coercion.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

goodcatch - this was a mistake :)

* 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above (schema validation).

`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
Copy link
Contributor

Choose a reason for hiding this comment

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

missing "ed" in "explained"

{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)}.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.


* 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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}.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{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}.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

* 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@leebyron
Copy link
Collaborator Author

Added some additional changes.

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

* 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

* Return {coercedValues}.

Note: This algorithm is very similar to {CoerceArgumentValues()}, however is
less forgiving of non-coerceable values.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

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

* 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}:
Copy link
Contributor

Choose a reason for hiding this comment

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

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}:

* 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}.
Copy link
Contributor

Choose a reason for hiding this comment

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

I love the new version, thanks!

* 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}.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

* 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}.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

* 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}.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@leebyron leebyron merged commit 13194df into master Oct 24, 2016
@leebyron leebyron deleted the exe-clarify branch October 24, 2016 18:58
leebyron added a commit to graphql/graphql-js that referenced this pull request Nov 1, 2016
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.
leebyron added a commit to graphql/graphql-js that referenced this pull request Nov 3, 2016
* 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
IvanGoncharov pushed a commit to IvanGoncharov/graphql that referenced this pull request Jun 17, 2017
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants