QueryPlan drops implementor fields from named fragments on abstract types when groupImplementorFields is enabled

[tim-hanssen]

Hi all!

While updating a flow in our implementation we encountered an issue where, in rare cases, fields were missing from our db query.

Summary

QueryPlan drops concrete implementor selections when:

1. groupImplementorFields is enabled;
2. a fragment spread has an abstract type condition (union or interface); and
3. that fragment contains inline fragments for concrete object types.

Equivalent inline fragments written directly in the parent selection work correctly.

Affected versions

Reproduced with webonyx/graphql-php 15.32.3.

The affected implementation is still present in v15.34.0 and current master:

Minimal reproduction

This can be added to tests/Type/QueryPlanTest.php:

public function testQueryPlanGroupsImplementorsInsideNamedAbstractFragment(): void
{
    $car = new ObjectType([
        'name' => 'Car',
        'fields' => [
            'mark' => Type::string(),
        ],
    ]);

    $building = new ObjectType([
        'name' => 'Building',
        'fields' => [
            'city' => Type::string(),
        ],
    ]);

    $item = new UnionType([
        'name' => 'Item',
        'types' => [$car, $building],
    ]);

    $queryPlan = null;

    $query = new ObjectType([
        'name' => 'Query',
        'fields' => [
            'item' => [
                'type' => $item,
                'resolve' => static function (
                    $value,
                    array $args,
                    $context,
                    ResolveInfo $info
                ) use (&$queryPlan) {
                    $queryPlan = $info->lookAhead([
                        'groupImplementorFields' => true,
                    ]);

                    return null;
                },
            ],
        ],
    ]);

    $schema = new Schema([
        'query' => $query,
        'types' => [$car, $building],
    ]);

    $result = GraphQL::executeQuery($schema, <<<'GRAPHQL'
        query {
            item {
                ...ItemFields
            }
        }

        fragment ItemFields on Item {
            ... on Car {
                mark
            }
            ... on Building {
                city
            }
        }
        GRAPHQL
    )->toArray();

    self::assertSame(['data' => ['item' => null]], $result);
    self::assertInstanceOf(QueryPlan::class, $queryPlan);
    self::assertSame(
        ['Car', 'Building'],
        array_keys($queryPlan->queryPlan()['implementors'] ?? [])
    );
}

Actual result

The plan contains no implementors:

[
    'fields' => [],
]

Both the Car.mark and Building.city selections disappear.

Expected result

The grouped plan should contain:

[
    'fields' => [],
    'implementors' => [
        'Car' => [
            'fields' => [
                'mark' => /* ... */,
            ],
        ],
        'Building' => [
            'fields' => [
                'city' => /* ... */,
            ],
        ],
    ],
]

Root cause

In QueryPlan::analyzeSelectionSet(), the FragmentSpreadNode branch calls:

$subfields = $this->analyzeSubFields(
    $type,
    $fragment->selectionSet
);

When $type is abstract, analyzeSubFields() collects concrete implementors internally. Because the third by-reference $implementors argument is not supplied, those implementors are discarded.

The InlineFragmentNode path has the same problem when its type condition is abstract and it contains further concrete fragments.

Direct inline fragments work because their fields are merged into the parent $implementors collection.

Suggested fix

Capture implementors while recursively analyzing fragment selections and merge them into the current implementor collection.

src/Type/Definition/QueryPlan.php

} elseif ($selection instanceof FragmentSpreadNode) {
    $spreadName = $selection->name->value;
    $fragment = $this->fragments[$spreadName] ?? null;
    if ($fragment === null) {
        continue;
    }

    $type = $this->schema->getType($fragment->typeCondition->name->value);
    assert($type instanceof Type, 'ensured by query validation');

-   $subfields = $this->analyzeSubFields($type, $fragment->selectionSet);
+   $subImplementors = [];
+   $subfields = $this->analyzeSubFields(
+       $type,
+       $fragment->selectionSet,
+       $subImplementors
+   );
    $fields = $this->mergeFields(
        $parentType,
        $type,
        $fields,
        $subfields,
        $implementors
    );
+
+   if ($this->groupImplementorFields && $subImplementors !== []) {
+       $implementors = $this->arrayMergeDeep(
+           $implementors,
+           $subImplementors
+       );
+   }
} elseif ($selection instanceof InlineFragmentNode) {
    $typeCondition = $selection->typeCondition;
    $type = $typeCondition === null
        ? $parentType
        : $this->schema->getType($typeCondition->name->value);
    assert($type instanceof Type, 'ensured by query validation');

-   $subfields = $this->analyzeSubFields($type, $selection->selectionSet);
+   $subImplementors = [];
+   $subfields = $this->analyzeSubFields(
+       $type,
+       $selection->selectionSet,
+       $subImplementors
+   );
    $fields = $this->mergeFields(
        $parentType,
        $type,
        $fields,
        $subfields,
        $implementors
    );
+
+   if ($this->groupImplementorFields && $subImplementors !== []) {
+       $implementors = $this->arrayMergeDeep(
+           $implementors,
+           $subImplementors
+       );
+   }
}

Effect

A consumer using QueryPlan to create type-aware data-loading plans sees an abstract reference field but no child selections. The parent object is loaded, while nested fields requested inside the named fragment are silently omitted from the loading plan.is