graphql-ruby icon indicating copy to clipboard operation
graphql-ruby copied to clipboard

Lookahead#selects? not matching fragment/type fields

Open RobinDaugherty opened this issue 5 years ago • 7 comments

There seems to be an issue with fields that are selected through fragments or types of a field. While the selections method returns the expected list of field names, calling selects? does not return true.

For example (extracted from a spec) I need know if the name field was requested on either the BirdSpecies or BirdGenus type.

query = GraphQL::Query.new(LookaheadTest::Schema, <<-GRAPHQL)
  query {
    node(id: "Cardinal") {
      ... on BirdSpecies {
        name
      }
      ... on BirdGenus {
        name
      }
      id
    }
  }
GRAPHQL

node_lookahead = query.lookahead.selection("node")

node_lookahead.selections.map(&:name)
=> [:id, :name, :name]

node_lookahead.selects?('id')
=> true

node_lookahead.selects?('name')
=> false

node_lookahead.selects?('BirdSpecies.name')
=> false

node_lookahead.selects?('birdSpecies.name')
=> false

I've added specs for Lookahead#selects? to demonstrate this. Am I correct that this is a bug, or am I misusing the selects? method?

RobinDaugherty avatar Jun 28 '20 16:06 RobinDaugherty

Hey, yes, you've understood correctly, it's a bug! Do you want to take a crack at fixing it in this PR?

rmosolgo avatar Jun 29 '20 19:06 rmosolgo

I believe this is because the way selection is implemented (which is called by selects?): def selection(field_name, selected_type: @selected_type, arguments: nil)

selected_type is used to find the corresponding field selections. In the case of queries above the selected_type will be "Node". You can get the behavior desired above via:

node_lookahead.selection(:name, selected_type: schema.find_type("BirdGenus")).selected?
node_lookahead.selection(:name, selected_type: schema.find_type("BirdSpecies")).selected?

timward60 avatar Jul 09 '20 03:07 timward60

Coming from here: https://github.com/DmitryTsepelev/graphql-ruby-fragment_cache#limitations

Is there a proposed solution? We'd be willing to work on a PR, but not exactly sure where to start

gsdean avatar Oct 01 '20 18:10 gsdean

Hey @gsdean , this branch contains example tests but not a fix. Yes, if you want to open a PR with a fix, that'd be great!

rmosolgo avatar Oct 01 '20 20:10 rmosolgo

I blew the dust off these tests and got them passing, but I'm not sure it's the right implementation.

After my changes, a Lookahead has a map of { type => ast_nodes, ... }, to handle things like name in the example above. As a result, there's not just one field for a lookahead, but n fields, since, in the example of name above, there are two different name fields involved. But for compatibility, it still pretends to be a single-typed object, with field and owner_type methods. However, these methods misbehave for multi-typed Lookaheads. I could:

  • Leave it the way it is, and it would silently fail for code that encounters a multi-type situation;
  • Remove the single-type methods (#field, #owner_type) and force callers to update to the new methods
  • In between: raise when a lookahead has multiple types but a single-type method is called. In that case, the schema's code should be updated to use plural methods instead.

But they all seem complicated 😖

rmosolgo avatar Feb 23 '21 22:02 rmosolgo

I'm also coming from https://github.com/DmitryTsepelev/graphql-ruby-fragment_cache#limitations. I am using that gem to facilitate caching, but because my schema has unions, some parts of my app can't be cached at the moment.

  • Leave it the way it is, and it would silently fail for code that encounters a multi-type situation;
  • Remove the single-type methods (#field, #owner_type) and force callers to update to the new methods
  • In between: raise when a lookahead has multiple types but a single-type method is called. In that case, the schema's code should be updated to use plural methods instead.

I'm guessing point 2 or 3 would be better than 1, because with solution 1 the graphql-ruby-fragment_cache gem would still not work with unions.

jeromedalbert avatar Aug 19 '21 21:08 jeromedalbert

@gsdean FYI I worked around the issue in my app by not using union types, and using interfaces instead. It seemed to mostly work.

Unions and interfaces in GraphQL are very similar, so migrating required minimal effort. No changes were needed for the frontend / API consumer, since the ... on syntax in queries remained the same.

jeromedalbert avatar Aug 10 '23 20:08 jeromedalbert