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

Parameters passed to lookAheаd are not applied #818

Closed
zabavnikov opened this issue Aug 25, 2021 · 11 comments
Closed

Parameters passed to lookAheаd are not applied #818

zabavnikov opened this issue Aug 25, 2021 · 11 comments
Labels
Milestone

Comments

@zabavnikov
Copy link

Versions:

  • graphql-laravel Version: 8.0.0-rc4
  • Laravel Version: 8.56.0
  • PHP Version: 8.0

Description:

I can't figure it out yet, maybe the point is that in the Support / Field in the originalResolver method, lookAhead is called without arguments.

@zabavnikov zabavnikov added the bug label Aug 25, 2021
@crissi
Copy link
Contributor

crissi commented Aug 28, 2021

Do you have an example?

@zabavnikov
Copy link
Author

zabavnikov commented Aug 30, 2021

TimelineUnion

class TimelineUnion extends UnionType
{
    protected $attributes = [
        'name' => 'TimelineUnion',
    ];

    public function types(): array
    {
        return [
            GraphQL::type('Note'),
            GraphQL::type('Question'),
            GraphQL::type('Review'),
            GraphQL::type('Travel'),
            GraphQL::type('Album'),
        ];
    }

    public function resolveType($value): Type|null
    {
        if ($value instanceof Note) {
            return GraphQL::type('Note');
        } else if ($value instanceof Question) {
            return GraphQL::type('Question');
        } else if ($value instanceof Review) {
            return GraphQL::type('Review');
        } else if ($value instanceof Travel) {
            return GraphQL::type('Travel');
        } else if ($value instanceof Album) {
            return GraphQL::type('Album');
        }

        return null;
    }
}

TimelineQuery

class TimelineQuery extends Query
{
    protected $attributes = [
        'name' => 'Timeline',
    ];

    public function type(): Type
    {
        return Type::listOf(GraphQL::type('TimelineUnion'));
    }

    public function resolve($root, $args, $context, ResolveInfo $resolveInfo, Closure $getSelectFields)
    {
        dd($resolveInfo->lookAhead(['group-implementor-fields'])->queryPlan());
    }
}

Query:

{
  timeline {
    ... on Note {
      id
      published_at
    }
    ... on Travel {
      id
      published_at
    }
  }
}

An invalid result is returned. It is missing the implementors array

array:2 [
  "id" => array:3 [
    "type" => GraphQL\Type\Definition\NonNull {#1548
      -ofType: GraphQL\Type\Definition\IntType {#848
        +name: "Int"
        +description: """
          The `Int` scalar type represents non-fractional signed whole numeric
          values. Int can represent values between -(2^31) and 2^31 - 1. 
          """
        +astNode: null
        +extensionASTNodes: null
        +config: []
      }
      +name: null
      +description: null
      +astNode: null
      +config: null
      +extensionASTNodes: null
    }
    "fields" => []
    "args" => []
  ]
  "published_at" => array:3 [
    "type" => GraphQL\Type\Definition\StringType {#839
      +name: "String"
      +description: """
        The `String` scalar type represents textual data, represented as UTF-8
        character sequences. The String type is most often used by GraphQL to
        represent free-form human-readable text.
        """
      +astNode: null
      +extensionASTNodes: null
      +config: []
    }
    "fields" => []
    "args" => array:2 [
      "format" => "Y-m-d H:i"
      "relative" => false
    ]
  ]
]

And this result should have returned:

array:2 [
  "fields" => []
  "implementors" => array:2 [
    "Note" => array:2 [
      "type" => GraphQL\Type\Definition\ObjectType {#896}
      "fields" => array:2 [
        "id" => array:3 [
          "type" => GraphQL\Type\Definition\NonNull {#1548}
          "fields" => []
          "args" => []
        ]
        "published_at" => array:3 [
          "type" => GraphQL\Type\Definition\StringType {#839}
          "fields" => []
          "args" => array:2 [
            "format" => "Y-m-d H:i"
            "relative" => false
          ]
        ]
      ]
    ]
    "Travel" => array:2 [
      "type" => GraphQL\Type\Definition\ObjectType {#905}
      "fields" => array:2 [
        "id" => array:3 [
          "type" => GraphQL\Type\Definition\NonNull {#1430}
          "fields" => []
          "args" => []
        ]
        "published_at" => array:3 [
          "type" => GraphQL\Type\Definition\StringType {#839}
          "fields" => []
          "args" => array:2 [
            "format" => "Y-m-d H:i"
            "relative" => false
          ]
        ]
      ]
    ]
  ]
]

With the help of the group-implementor-fields, I would be able to solve this problem #817.

@crissi
Copy link
Contributor

crissi commented Aug 30, 2021

I think adding that would be fine, make a PR.

I was also working on a fix for SelectFields with union/interfaces where I am using group-implementor-fields. Just need to pick the task up again.

@michaelnabil230

This comment was marked as off-topic.

@mfn
Copy link
Collaborator

mfn commented Jan 13, 2023

Can't decide: is this really a bug or a missing feature?

@crissi
Copy link
Contributor

crissi commented Jan 13, 2023

i think we just need to add it to the lookAhead call in Field-class, dont know if it needs to be option or just always there!

@mfn
Copy link
Collaborator

mfn commented Jan 13, 2023

Ok.

First off, achtung: the option was renamed in the latest graphql-php release

Change expected QueryPlan options from ['group-implementor-fields'] to ['groupImplementorFields' => true] in ResolveInfo::lookAhead()

Second, my question: how we know it's a bug with graphql-laravel? 🤔

We do not instantiate / handle ResolveInfo in any way: we receive it from graphql-php and pass it on in the resolve() methods. We don't touch it in the process.

The implementation (in graphql-php; not our library) in ResolveInfo::lookAhead() just constructs a new QueryPlan based on the state of ResolveInfo itself.

AFAICS, graphql-laravel is not involved in this process anywhere and thus failure of the correct structure seems to be outside of our realm, no?

@zabavnikov can you please check/debug this?

@crissi
Copy link
Contributor

crissi commented Jan 13, 2023

Ahh yeah i see when I look again it this does nok look like anything to do with this package

@zabavnikov
Copy link
Author

zabavnikov commented Jan 24, 2023

Hi @mfn, @crissi!

It seems that after these edits, this code $resolveInfo->lookAhead(['group-implementor-fields']) became working.

After #953, $resolveInfo->lookAhead(['groupImplementorFields' => true]) should work as expected. 😉😉😉

@mfn
Copy link
Collaborator

mfn commented Jan 24, 2023

Oh, nice find 🤞🏼 I'll add it to the milestone then, too; thx for digging around 🙇🏼

@mfn mfn added this to the 9.0.0 milestone Jan 24, 2023
@mfn
Copy link
Collaborator

mfn commented Jun 25, 2023

As far as I understood this issue, the issue wasn't with this library but with graphql-php and we were waiting to update it to 15.x.

With the release of 9.0.0 this has now happened, and you should be able to pass the options workingly to it now 🤞🏼

@mfn mfn closed this as completed Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants