`Query#lookahead` with no selected operation

[ravangen]

Describe the bug

https://github.com/rmosolgo/graphql-ruby/blob/c66362145bd120a22fdbc14aac612b486c32b3d8/lib/graphql/query.rb#L211-L212

When there is no selected_operation, we get an exception related to nil:

Error:
NoMethodError: undefined method `operation_type' for nil
    /Users/robvangennip/.gem/ruby/3.3.1/gems/graphql-2.3.18/lib/graphql/query.rb:206:in `lookahead'

GraphQL schema

class QueryRoot < GraphQL::Schema::Object
  field :echo, String, null: true do |field|
    field.argument(:value, String, required: true)
  end

  def echo(value:)
    value
  end
end

class Inspire < GraphQL::Schema::Mutation
  field :value, String

  def resolve
    "Do. Or do not. There is no try."
  end
end

class MutationRoot < GraphQL::Schema::Object
  field :inspire, mutation: Inspire
end

class Schema < GraphQL::Schema
  query QueryRoot
  mutation MutationRoot
end

class Query < GraphQL::Query
  def root_fields
    return @root_fields if defined?(@root_fields)

    @root_fields = lookahead.selections.filter_map { |selection| selection.field&.graphql_name }.uniq
  end
end

Steps to reproduce

query_string = <<~DESC
  query Greeting {
    echo(value: "Hello there")
  }
  mutation Motivation {
    inspire { value }
  }
DESC
query = Query.new(Schema, query_string, operation_name: "Invalid")

assert_equal [], query.root_fields

Expected behavior

I'm not sure what is best:

• Raise no operation selected (interrupts flow)
• Default to query_root (unexpected)
• Return nil (changes interface but matches selected_operation_name)
• Leave as is

Actual behavior

NoMethodError: undefined method `operation_type' for nil

Additional context

I've added a check for the query being valid to work around this, but should lookahead be more defensive?

query.valid?
=> false

[rmosolgo]

Thanks for the detailed report of this bug! I agree some other behavior would be an improvement. Another option is NULL_LOOKAHEAD which is what Lookahead returns when there are no selections:

https://github.com/rmosolgo/graphql-ruby/blob/c66362145bd120a22fdbc14aac612b486c32b3d8/lib/graphql/execution/lookahead.rb#L213-L241

What do you think of returning that in this case? It has the upside of keeping the API the same (no nil) and not raising any errors -- and probably, it would do the "right thing" in case of invalid queries like the one in your example.

[ravangen]

TIL, sounds reasonable