Args array is empty in GraphQLType fields() function

[nurges]

Versions:

• graphql-laravel Version: 2.0.1
• Laravel/Lumen Version: 5.8
• PHP Version: 7.3

Description:

Empty args array is passed into GraphQLType fields() function, although it's correct in query resolve().
The cause is line 99 in Rebing\GraphQL\Support\SelectFields.php where $requestedFields['args'] is already empty.
Previous graphql version used self::$args instead, that contains the correct data.

Steps To Reproduce:

Execute a query with some arguments and try to access these from GraphQLType fields().

[mfn]

Can you describe a bit more your use case why you would need (the now non-existent) self::$args (basically the "global" one from the resolver) deep down in a relation query?

Thanks

[nurges]

Here's an example

class TestType extends GraphQLType
{
    public function fields(): array
    {
        return [
            'test'     => [
                'type'  => Type::listOf(GraphQL::type('test')),
                'query' => function (array $args, $query) {
                    // I need to use values from $args here
                    return $query;
                },
            ],
        ];
    }
}

$requestedFields['args'] is correct at the start of getSelectableFieldsAndRelations function, self::handleFields(...) line strips it out.

[mfn]

Can you give an example of a GraphQL query (with args) where the type using the 'query' is used in?

[nurges]

A simplified example follows.

GraphQL query

{
  getDocuments(company_id: 77) {
    id
    comment
      text
  }
}

PaginationQuery class

class DocumentsQuery extends PaginationQuery
{

    public function type(): Type
    {
        return GraphQL::paginate('document');
    }

    public function args(): array
    {
        return array_merge(parent::args(), [
            'company_id' => [
                'type'  => Type::nonNull(Type::int()),
                'rules' => ['required'],
            ],
        ]);
    }

    public function resolve($root, $args, $context, ResolveInfo $info, Closure $getSelectFields)
    {
        $fields = $getSelectFields();
        $with = $fields->getRelations();
        $select = $fields->getSelect();

        return Repo::document()->companyDocs($args['company_id'], $search, $with, $select);
    }
}

Type

class DocumentType extends GraphQLType
{
    protected $attributes = [
        'name'        => 'Document',
        'model'       => Document::class,
    ];

    public function fields(): array
    {
        return [
            'id'         => [
                'type' => Type::nonNull(Type::int()),
            ],

            'comments'     => [
                'type'  => Type::listOf(GraphQL::type('comment')),
                'query' => function (array $args, $query) {
                    if (array_has($args, 'company_id')) {
                        return $query->where('company_id', '=', $args['company_id']);
                    }

                    return $query;
                },
            ],
        ];
    }
}

[rebing]

#460

[mfn]

Proposed solution

I think the correct solution is to add the 'args' to the 'comments' relation:

            'comments'     => [
                'type'  => Type::listOf(GraphQL::type('comment')),
                'args'  => [
                    'company_id' => [
                        'type'  => Type::nonNull(Type::int()),
                        'rules' => ['required'],
                    ],
                ],
                'query' => function (array $args, $query) {
                    if (array_has($args, 'company_id')) {
                        return $query->where('company_id', '=', $args['company_id']);
                    }

                    return $query;
                },
            ],

Then the query would become like this:

{
  getDocuments(company_id: 77) {
    id
    comment(company_id: 77) {
      text
  }
}

Usually the repetitiveness is handled by using variables within the query:

query getDocuments($company_id: int!){
  getDocuments(company_id: $company_id) {
    id
    comment(company_id: $company_id) {
      text
  }
}

Note: I think in this example, in practice, you would not have int! for the 'args' on 'commens', but rather int; I think in this case it also means that the query needs to define two variables because the types need to match (int vs. int!); YMMV

Background information

Here is the initial bug report for it: #314

Although I created the report, I did not come up with the problem, rather it was discovered as part of #310 when I investigating another issue reported.

Here is the PR which changed / fixed this: #327

The TL;DR

It's quite some ground to cover so I try to summarize it briefly:

• Previously, only the 'args' from the "root object" === the GraphQL query object were passed on to custom 'query'
• This also meant, no matter how deeply nested your fields and types were, you would also receive the top level 'args' 🔝
• But this made it impossible to have different kind of relation on different level of different types loaded with different 'args' requirements, because all custom 'query' received always the same 'args'; "local" 'args' were effectively ignored 🛑
• We added enough tests to make sure we don't break this; that's why the tests in Provide query arguments to custom query #460 are failing 🤞

IMO this change was correct to make: passing arbitrary top level data to a deeply nested query does not make sense and totally bypassing GraphQL type expectations; that's why those args have to be locally defined on the relation.

It seems it's a breaking change from 1.x to 2.0 which we missed to properly document in the upgrade guide :/

Please let me know what you think about this, thank you.

[nurges]

Thank you for the explanation, it makes sense.

I've currently fixed my use case and also made the tests pass by merging the query and field level arguments. Field arguments should overwrite the matching global ones, so this fix shouldn't break anything.

I have to evaluate the query refactoring needed to use the field level arguments.

[rebing]

@mfn You are right that the correct solution would be to modify the query to use sub-arguments. However, I think we should merge this PR for now, as it would solve the current problem and it should not break anything else. It's also like this in the documentation: https://github.com/rebing/graphql-laravel#type-relationship-query

[mfn]

The problem with the merge is that the 'query' does not know where it came from:

• you could have a GraphQL query 'args' named 'flag' of type bool
• and you could have by coincidence the same 'args' 'flags' on the relation where you define the 'query'

As long you pass the 'flag' to the GraphQL query, your relation 'query' also receives it and it can't decide whether it was meant to use it or not.

I was about to write a test for the PR when I realized this. This can lead to big confusions. We need another approach here.

Alternative proposal

Everything in GraphQL relies on parent/child relationships. You can't skip a level and you can only access your direct parents information.

Obviously via ResolveInfo you can provide some hacks but only "top down"; even as a child you can't access "higher up" nodes in the hierarchy.

However, the GraphQL spec created one thing which is shared between all resolvers within a query/mutation: the "context".

Therefore I believe a better solution would be to:

• Add the "context" as a 3rd argument to the 'query'
  That would actual be very ideal because on all resolvers the context is the 3rd arg already
• Implementors can decide to use a custom Context class by overriding \Rebing\GraphQL\GraphQLController::queryContext
• A GraphQL query can the decide to add whatever it wants to this context
• and then it would also be available on 'query' and it can decide whether to mutate the $query based on data from the context or the local 'args'

The signature would be:

                'query' => function (array $args, $query, $ctx) {
                    …
                },

And $ctx is whatever queryContext returned.

---

I think I understand what drives the desire for the fix: this is one of the breaking changes which affects consumers of GraphQL and are much harder to contain.

However I stand by my opinion that the internal behaviour of the passed 'args' itself should not be changed (un-intuitive, hard to debug problems) and I think my proposal allows to fix this so consumers are not affected.

I'm willing to take care of the implementation/test and docs if we consent on this.

[nurges]

@mfn I agree with your considerations.

We can forward arbitrary data via context to the query. It's also trivial to refactor our existing code to use this feature as opposed to changing the client queries.

[mfn]

Roger, will move forward here ASAP