Replace ExecuteSelectionSet with ExecuteCollectedFields

[benjie]

Essentially this PR replaces the ExecuteSelectionSet method with ExecuteCollectedFields (which essentially just drops the first line of ExecuteSelectionSet, which was responsible for collecting fields to be executed). It then refactors the rest of the spec to accommodate this change, reducing the repetition in ExecuteQuery, ExecuteMutation and ExecuteSubscriptionEvent; and removing MergeSelectionSets (which generated a "virtual" selection set to accomodate the ExecuteSelectionSet method), instead adding a CollectSubfields algorithm which produces a grouped field set directly, ready for execution.

I extracted this common refactoring from a number of my attempts to write spec changes for the @defer and @stream directives - it turns out that this refactoring of the spec was always needed as a base for my changes. Similarly, @yaacovCR found similar in his attempts to address this same problem, and raised #999 extracted from his solution. This PR was introduced independently of #999 (other than using the CollectSubfields algorithm name) however there is significant alignment, so @yaacovCR suggested that I raise it as an alternative PR.

It may be easier to review this PR in "split" view rather than "unified" view.

[netlify]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	5459f16
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/681351f5d7e7f300080ececc
😎 Deploy Preview	https://deploy-preview-1039--graphql-spec-draft.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[benjie]

(Rebased on main)

[benjie]

(Rebased on main)

[Keweiqu]

@benjie we need to formally define "grouped field set" in Section 2 "Language". You might have done so in a prior PR that I missed. Here is the link to us defining "section sets" in section 2 https://spec.graphql.org/draft/#sec-Selection-Sets

[mjmahone]

Nits because we agreed to name things responseName instead of responseKey

[leebyron]

@benjie this change looks good but has some conflicts with the other changes pulled out and recently merged. Want to rebase and fix? Then I'll get this one merged

[benjie]

These are the only lines that have actually been changed in Field Collection, everything else was just moved.

(To prove this, take the current Field Collection text and paste it over this PR, the diff should render as:

diff --git i/spec/Section 6 -- Execution.md w/spec/Section 6 -- Execution.md
index f81e38d..549930f 100644
--- i/spec/Section 6 -- Execution.md	
+++ w/spec/Section 6 -- Execution.md	
@@ -374,12 +374,11 @@ serial):
 
 ### Field Collection
 
-:: A _grouped field set_ is a map where each entry is a list of field selections
-that share a _response name_ (the alias if defined, otherwise the field name).
-
-Before execution, the _selection set_ is converted to a _grouped field set_ by
-calling {CollectFields()}. This ensures all fields with the same response name
-(including those in referenced fragments) are executed at the same time.
+Before execution, the _selection set_ is converted to a grouped field set by
+calling {CollectFields()}. Each entry in the grouped field set is a list of
+fields that share a _response name_ (the alias if defined, otherwise the field
+name). This ensures all fields with the same response name (including those in
+referenced fragments) are executed at the same time.
 
 As an example, collecting the fields of this selection set would collect two
 instances of the field `a` and one of field `b`:

)

[benjie]

@leebyron I opted for a merge instead as it was less work, this is now good to review again.

(I diffed the diffs and only the expected changes have come through, I also searched for "response key" and /^key/ to ensure no older keys still exist. This also addresses Matt's feedback I think.)

[leebyron]

Really love this cleanup, much closer to GraphQL.js

[leebyron]

@benjie I took a pass at some re-ordering of the content here, I'd be curious for your feedback.

Also, I think "Grouped Field Set" has become a confusing name. Ideally this term "Collect" is what we use for this concept (instead of "group") and it's a bit weird this is called a "Set" when in fact its type is Map<string, List<Field>>. What do you think about renaming this?

"Collected Field Map"?

[benjie]

Scratch that. I'd be happy with "Collected Fields Map", but I think it's important "field" is plural, either via fields or via list or set.

:: A collected fields map is a map where each entry is a response name and a
list of selected fields that share that response name (the field alias if
defined, otherwise the field's name).

Executing a Collected Fields Map

To execute a collected fields map, the object value being evaluated and the object type need to be known, as well as whether it must be executed serially, or may be executed in parallel (see Normal and Serial Execution.

Each entry in the collected fields map represents a response name which produces
an entry into a result map.

ExecuteCollectedFieldsMap(collectedFieldsMap, objectType, objectValue,
variableValues):

• Initialize {resultMap} to an empty ordered map.
• For each {collectedFieldsMap} as {responseName} and {fields}:
  • Let {fieldName} be the name of the first entry in {fields}. Note: This value
    is unaffected if an alias is used.
  • Let {fieldType} be the return type defined for the field {fieldName} of
    {objectType}.
  • If {fieldType} is defined:
    • Let {responseValue} be {ExecuteField(objectType, objectValue, fieldType,
      fields, variableValues)}.
    • Set {responseValue} as the value for {responseName} in {resultMap}.
• Return {resultMap}.

[benjie]

Thanks for the edits @leebyron! I'm generally in favour, but have a few suggestions.

[benjie]

If we choose not to rename "grouped field set", we should at least define it in terms of a set:

Suggested change

	:: A _grouped field set_ is a map where each entry is a _response name_ and a
	list of selected fields that share that _response name_ (the field alias if
	defined, otherwise the field's name).
	:: A _grouped field set_ is a map where each entry is a _response name_ and an
	ordered set of selected fields that share that _response name_ (the field alias
	if defined, otherwise the field's name).

[benjie]

If we accept this, we should also make a few other changes such as:

-Initialize groupedFields to an empty ordered map of lists.
+Initialize groupedFields to an empty ordered map of ordered sets.

[robrichard]

There's no functional change between a list or a set in this case since the algorithm never attempts to add the same item to the set/list?

[benjie]

Yes indeed, list or ordered set is equivalent here since it will never include duplicates. The main reason to change it to set is because we call it a "grouped field set"; a "field set" being OrderedSet<Field>, and grouped being a map grouped by response key: Map<string, OrderedSet<Field>>. Technically we have Map<string, Field[]> currently, and calling that a "grouped field set" causes the confusion because no "set" is involved.

[benjie]

This might be a simpler solve actually:

Suggested change

	:: A _grouped field set_ is a map where each entry is a _response name_ and a
	list of selected fields that share that _response name_ (the field alias if
	defined, otherwise the field's name).
	:: A _grouped field set_ is a map where each entry is a _response name_ and a
	list of selected fields that share that _response name_ (the field alias if
	defined, otherwise the field's name).
	Note: No entry in the list of selected fields will be duplicated, hence we call
	it a "field set" even though in the algorithms we use a list structure for it.

[robrichard]

Worth mentioning here that the data structure returned by CollectFields will have to change to support @defer. It's currently modeled as:

type FieldDetails = { field: Field, deferUsage: DeferUsage }
type GroupedFieldSet = Map<string, List<FieldDetails>>

[benjie]

LGTM 👍

Having thought about it more, I'm in favour of keeping "grouped field set" - I think it's descriptive of a field set that has been grouped by response key, I think the only issue really is that we use a list instead of a set in the algorithms. Moreover, I think that an individual "field set" is in itself useful - we call it "fields" rather than "field set" in CompleteValue() but it's the same thing.

[benjie]

@leebyron To reflect your discussion above, I've renamed "ExecuteGroupedFieldSet" to "ExecuteCollectedFields" and that definitely flows better.

However, I think that "grouped field set" is still the right term to refer to the structure and actually defining a "field set" in general adds a lot of clarity to a number of algorithms (e.g. ExecuteField()) so I've both reworked the introduction to field collection in this PR and raised a separate PR that changes "grouped field set" to actually be Map<string, OrderedSet<FieldSelection>> rather than containing lists:

#1161

This makes the algorithms a lot clearer to me:

-CollectSubfields(objectType, fields, variableValues):
+CollectSubfields(objectType, fieldSet, variableValues):

^ Knowing that a "field set" is a set of field selections that share the same response key makes this algorithms purpose a lot clearer

-ExecuteField(objectType, objectValue, fieldType, fields, variableValues):
+ExecuteField(objectType, objectValue, fieldType, fieldSet, variableValues):

^ A lot more intuitive what fieldSet is (field selections representing the field to be executed) rather than fields (could be interpreted as the the fields requested on this field's selection set).

-CompleteValue(fieldType, fields, result, variableValues):
+CompleteValue(fieldType, fieldSet, result, variableValues):

^ Same

[benjie]

🆕

See also:

• Change "grouped field set" to be a map of ordered sets #1161

[martinbonnin]

Not directly related to this PR but a more precise wording would be to use selectionSet here as fragments are also valid, not only fields.

Suggested change

	Valid operations must supply a selection of fields for any field that returns an
	Valid operations must supply a valid sub _selection set_ for any field that returns an