Grouping implementor fields for abstract types in QueryPlan

[yura3d]

I'm using inline fragments in my projects, so I need to know what fields are requested in these fragments.

Current ResolveInfo::getFieldSelection() implementation skips a level of fragments and returns their fields on the same level with another fields, that are not in fragment:

graphql-php/tests/Type/ResolveInfoTest.php

Lines 88 to 94 in 93ccd73

	pic {
	url
	width
	... on Image {
	height
	}
	}

graphql-php/tests/Type/ResolveInfoTest.php

Lines 131 to 135 in 93ccd73

	'pic' => [
	'url' => true,
	'width' => true,
	'height' => true,
	],

Also, we have a warning that was left in comment inside ResolveInfo.php, so the behavior described above is expected.

graphql-php/src/Type/Definition/ResolveInfo.php

Lines 165 to 166 in d1d4455

	* Warning: this method it is a naive implementation which does not take into account
	* conditional typed fragments. So use it with care for fields of interface and union types.

My small correction adds to the field of resolve array, that contains inline fragments, new array with a service key __inlineFragments. This array contains fragment resolutions as items, where key is an inline fragment type, and value is an array of fragment fields:

graphql-php/tests/Type/ResolveInfoTest.php

Lines 131 to 137 in c35379c

	'pic' => [
	'url' => true,
	'width' => true,
	'__inlineFragments' => [
	'Image' => ['height' => true],
	],
	],

So now there is an ability to get the fields that is requested in inline fragments.

[vladar]

getFieldSelection is intentionally a rather simple method. We have a more comprehensive solution for complex query analysis (unfortunately not documented yet). See #65 and PR #436 for insights. If some feature is missing in this QueryPlan class - feel free to add it there instead of getFieldSelection

[yura3d]

I've made the same changes for QueryPlan.

[vladar]

The idea of relying on __ seems a bit hacky for me. Besides it will cause a breaking change (since users of previous versions may loop through fields and not expect the __inlineFragments part).

I do think we need this feature, but need to think more about ergonomics of it. Anyone having ideas are welcome to share them!

[yura3d]

I've added some changes to this.

To prevent BC, method ResolveInfo::lookAhead() now have optional string ...$options parameter. If a user calls the method with no args, it works without any changes that introduced in this PR. If a user passes 'with-meta-fields' arg to this method, query plan will look like described below.

Every type resolution, in addition to fields array, now has optional fragments and inlineFragments arrays.
The fragments array is used for fragment spreads and has the following format:

graphql-php/tests/Type/QueryPlanTest.php

Lines 439 to 450 in c1795f9

	'fragments' => [
	'MyImage' => [
	'type' => $image,
	'fields' => [
	'url' => [
	'type' => Type::string(),
	'args' => [],
	'fields' => [],
	],
	],
	],
	],

The inlineFragments array is used for inline fragments and looks like that:

graphql-php/tests/Type/QueryPlanTest.php

Lines 490 to 500 in c1795f9

	'inlineFragments' => [
	'Image' => [
	'fields' => [
	'height' => [
	'type' => Type::int(),
	'args' => [],
	'fields' => [],
	],
	],
	],
	],

Since a root of query plan can also contain fragments, all the root fields are wrapped in arrays like it is done for the types:

graphql-php/tests/Type/QueryPlanTest.php

Lines 741 to 760 in c1795f9

	$expectedQueryPlan = [
	'fields' => [
	'name' => [
	'type' => Type::string(),
	'args' => [],
	'fields' => [],
	],
	],
	'inlineFragments' => [
	'Dog' => [
	'fields' => [
	'woofs' => [
	'type' => Type::boolean(),
	'fields' => [],
	'args' => [],
	],
	],
	],
	],
	];

[yura3d]

There are some changes comparing to my previous post.

Since now we have already no underscore fields (so-called "meta-fields") like __inlineFragments, the argument 'with-meta-fields' was renamed to 'with-type-conditions'. All methods and variable names in code and tests were changed according to it, too.

Fields of regular and inline fragments are merged together now. So, the arrays fragments and inlineFragments of type resolution output were replaced with only one array typeConditions, which presents conditional types of fragments and has the following format:

graphql-php/tests/Type/QueryPlanTest.php

Lines 748 to 758 in 6d9fff3

	'typeConditions' => [
	'Dog' => [
	'fields' => [
	'woofs' => [
	'type' => Type::boolean(),
	'fields' => [],
	'args' => [],
	],
	],
	],
	],

The tests are fully duplicated now to support both modes (with and without changes of this PR).

[vladar]

How will it work when there are several fragments (or inline fragments) on the same type especially with include/skip directives, i.e.

query Test {
        article {
            image {
                ...MyImage
                ...MyImage2
            }
            author {
                pic {
                    ... on Image {
                        height
                    }
                    ... on Image {
                        width
                    }
                    ...MyImage @include(if: false)
                }
            }
        }
      }
      fragment MyImage on Image {
        url
      }
      fragment MyImage2 on Image {
        width
        height
      }

[yura3d]

How will it work when there are several fragments (or inline fragments) on the same type

Fields of the same type will be merged together. Let's see example from test.
Spreads ...Replies01 (replies {body}) and ...Replies02 (replies {author}) in the query:

graphql-php/tests/Type/QueryPlanTest.php

Lines 1168 to 1216 in 6d9fff3

	$doc = '
	query Test {
	article {
	author {
	name
	pic(width: 100, height: 200) {
	url
	width
	}
	}
	image {
	width
	height
	...MyImage
	}
	...Replies01
	...Replies02
	}
	}
	fragment MyImage on Image {
	url
	}
	fragment Replies01 on Article {
	_replies012: replies {
	body
	}
	}
	fragment Replies02 on Article {
	_replies012: replies {
	author {
	id
	name
	pic {
	url
	width
	... on Image {
	height
	}
	}
	recentArticle {
	id
	title
	body
	}
	}
	}
	}
	';

are resolved to merged (replies {body, author}) fields list:

graphql-php/tests/Type/QueryPlanTest.php

Lines 1278 to 1303 in 6d9fff3

	'typeConditions' => [
	'Article' => [
	'fields' => [
	'replies' => [
	'type' => Type::listOf($reply),
	'args' => [],
	'fields' => [
	'body' => [
	'type' => Type::string(),
	'args' => [],
	'fields' => [],
	],
	'author' => [
	'type' => $author,
	'args' => [],
	'fields' => [
	'id' => [
	'type' => Type::string(),
	'args' => [],
	'fields' => [],
	],
	'name' => [
	'type' => Type::string(),
	'args' => [],
	'fields' => [],
	],

especially with include/skip directives

Directives are not supported as they are not supported in original QueryPlan (without changes of this PR), even for regular fields (that are not in fragments), so the resolution of fields with directives will be the same as of fields without them.
I can add support of directives in this PR or maybe it makes sense (as of directives support and fragments support are slightly different things) to create new PR.

[vladar]

My main concern is that it will be rather hard to cover all use-cases in the future with the current design of QueryPlan (which I had merged in the past a bit prematurely).

I have a proposal which may involve some rewrite %)

The idea is to have lookAhead return special class-collection of AST nodes which could be further filtered and chained. Similar to how jquery works on DOM nodes.

This should be way more extensible. Usage example:

$fields = $info->lookAhead()
    ->withinFragments()
    ->withIncludeDirective()
    ->filter(function (FieldNode $field, $parentCollection) { // custom convenience filter
        return $field->alias === 'myAlias' || $field->alias === 'myOtherAlias';
    });

$fieldNames = $fields->names(); // returns array of field names matching filters above
$fieldUnaliased = $field->aliases();
$firstFieldArguments = $fields->first()->arguments()->names();
$firstIncludeDirective = $fields->directives()->first()->arguments()->values($variables);

Technically we already have GraphQL\Language\AST\NodeList class which we could probably enhance with those chainable methods.

Then it will also benefit those using AST extensively.

@spawnia What do you think? I know you guys work with ASTs a lot.

I'd like to discuss method naming and general UX for this collection

[spawnia]

The most generally useful tool would be a data structure that contains the currently executing query with its given arguments together with the matching definitions.

As this PR also suggests, fragments should be inlined, as they don't really matter at that level - they are merely an implementation detail that this library takes care off, rather then something that an application needs to concern itself with.

[yura3d]

I don't think we need exactly AST nodes in QueryPlan. I agree that it should be objects that represents current query in more flexible way (with support of filtering, chaining, etc) than it's made now with arrays. But these objects should be on higher level (to describe what data is queried) than AST nodes (that describes how data is queried).

Simple example:

author {
    pic {
        ... on Image {
            height
        }
        ... on Image {
            width
        }
    }
    ...
}

Here we queried 2 fields (height, width) for Image type. But if we will think in terms of AST, QueryPlan will contain 2 Language\AST\InlineFragmentNode nodes with 1 field per node (that's how data is queried). But our resolver only should know what fields it needs to return, regardless of how we requested them (using 1 fragment or 2 fragments).

So, we need special objects that will describe what is requested. They need their own namespece.
For example, QueryPlan\FieldNode which will have getTypeConditions() method that will return an object-collection (with support of filtering, chaining. etc) of already merged QueryPlan\FieldNodes for every type condition inside current QueryPlan\FieldNode.

[vladar]

@yura3d Your notes make sense but AST is a quite flexible structure. It is easy to flatten it (unwrap fragments). It even doesn't have to pass formal GraphQL validation for this use-case.

One reason why I would prefer to re-use AST vs introducing new classes is Visitor - it is a very powerful tool for traversing / modifying AST.

Anyway, it seems like the scope of this new API is rather big (and the demand is questionable), so I will create a separate ticket for it and will merge your PR as is for now as it already provides value to users.

Changes we discuss will likely be breaking anyway (or require a separate chainable and filterable API).

[yura3d]

Several days ago I totally rethought and simplified this PR.

If we use fragments inside regular object types, there are no need to group fragment fields by type conditions in QueryPlan because object types and fragments type conditions will be the same.

Grouping makes sense only if parent type is abstract (interface or union) and helps to determine, which fields belong to concrete object types, that listed in fragments type conditions.

How QueryPlan works with interfaces now:

graphql-php/tests/Type/QueryPlanTest.php

Lines 326 to 346 in 62b4c38

	$query = 'query Test {
	pets {
	name
	... on Dog {
	woofs
	}
	}
	}';
	$expectedQueryPlan = [
	'woofs' => [
	'type' => Type::boolean(),
	'fields' => [],
	'args' => [],
	],
	'name' => [
	'type' => Type::string(),
	'args' => [],
	'fields' => [],
	],
	];

All the fields (woofs, name) are on the same level, so a resolver doesn't know which of them belong to concrete (Dog) type. As a result, it also doesn't know table name in DB (for example), from which the object data should be retrieved.

Introduced in the last commit new option group-implementor-fields will group all implementor fields by fragments condition types in additional implementors array:

graphql-php/tests/Type/QueryPlanTest.php

Lines 740 to 807 in 62b4c38

	$query = '{
	item {
	id
	owner
	... on Car {
	mark
	model
	}
	... on Building {
	city
	}
	...BuildingFragment
	}
	}
	fragment BuildingFragment on Building {
	address
	}';
	$expectedResult = [
	'data' => ['item' => null],
	];
	$expectedQueryPlan = [
	'fields' => [
	'id' => [
	'type' => Type::int(),
	'fields' => [],
	'args' => [],
	],
	'owner' => [
	'type' => Type::string(),
	'fields' => [],
	'args' => [],
	],
	],
	'implementors' => [
	'Car' => [
	'type' => $car,
	'fields' => [
	'mark' => [
	'type' => Type::string(),
	'fields' => [],
	'args' => [],
	],
	'model' => [
	'type' => Type::string(),
	'fields' => [],
	'args' => [],
	],
	],
	],
	'Building' => [
	'type' => $building,
	'fields' => [
	'city' => [
	'type' => Type::string(),
	'fields' => [],
	'args' => [],
	],
	'address' => [
	'type' => Type::string(),
	'fields' => [],
	'args' => [],
	],
	],
	],
	],
	];

So now the resolver knows that id and owner are common fields, mark and model can be found in cars table (for example), city and address - in buildings table.

[vladar]

Sounds reasonable. Ready for merge?

[yura3d]

@vladar Yes