QueryComplexity analyzer may have IO side-effect

[thlacroix]

Describe the bug

Discussed in #4800 (comment)

The GraphQL::Analysis::QueryComplexity may have IO side-effects (eg making a DB query via active record), while it would be expected to be fully static (as mentioned in the doc "None of GraphQL-Ruby’s validators make IO calls").
This happens because the field complexity calculation for connections (in https://github.com/rmosolgo/graphql-ruby/blob/v2.4.10/lib/graphql/schema/field.rb#L504) calls the argument_cache to read the arguments (to read static arguments like :first), which fully resolves the arguments. This is a problem for example when an argument uses loads: to get an ActiveRecord object from id. This makes using the validate_timeout feature for analysis unsafe.

Versions

graphql version: 2.4.10

Script to reproduce

(took inspiration from the script in #5036 (comment))

require 'bundler/inline'

gemfile do
  gem 'graphql'
  gem 'sqlite3'
  gem 'activerecord', require: 'active_record'
end

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Schema.define do
  create_table :mentions do |t|
    t.string(:content)
    t.belongs_to :ticket
  end
  create_table :tickets do |t|
    t.string(:subject)
  end
end

class Mention < ActiveRecord::Base; end

class Ticket < ActiveRecord::Base
  has_many :mentions
end

ticket = Ticket.create!(subject: "Ticket 1")

Mention.create!(content: "Mention 1", ticket_id: ticket.id)

class MySchema < GraphQL::Schema

  class Mention < GraphQL::Schema::Object
    field :content, String
  end

  class Ticket < GraphQL::Schema::Object
    field :subject, String
    field :mentions, Mention.connection_type

    def mentions(obj)
      obj.mentions
    end
  end

  class Query < GraphQL::Schema::Object
    field :mentions, Mention.connection_type do
      argument :ticket_id, ID, loads: Ticket
    end

    def mentions(ticket:)
      ticket.mentions
    end
  end

  default_page_size 50
  max_complexity 1000
  query(Query)

  def self.object_from_id(id, ctx)
    ::Ticket.find(id)
  end

  def self.resolve_type(abs_t, obj, ctx)
    Ticket
  end
end

query_str = <<~GRAPHQL
  query queryOne($ticketId: ID!, $first: Int!) {
    mentions(ticketId: $ticketId, first: $first) {
      nodes { content }
    }
  }
GRAPHQL

ActiveRecord::Base.logger = Logger.new(STDOUT) # Logs ActiveRecord SQL queries

query = GraphQL::Query.new(MySchema, query_str, variables: { ticketId: '1', first: 10 })

pp GraphQL::Analysis.analyze_query(query, [GraphQL::Analysis::QueryComplexity])
# D, [2025-02-28T16:55:50.009374 #66030] DEBUG -- :   Ticket Load (0.2ms)  SELECT "tickets".* FROM "tickets" WHERE "tickets"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
# [12]
pp query.arguments_cache.instance_variable_get(:@storage).values[0].values[0].values[0]
# #<GraphQL::Execution::Interpreter::Arguments @keyword_arguments={:first=>10, :ticket=>#<Ticket id: 1, subject: "Ticket 1">}>

Expected behavior

No DB call should be made, analyzer should be fully static. Arguments that are needed for the complexity calculation should be read statically.

Actual behavior

We can see a DB call made through ActiveRecord

D, [2025-02-28T16:55:50.009374 #66030] DEBUG -- :   Ticket Load (0.2ms)  SELECT "tickets".* FROM "tickets" WHERE "tickets"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]

And can see the argument resolved in the arguments_cache

 #<GraphQL::Execution::Interpreter::Arguments @keyword_arguments={:first=>10, :ticket=>#<Ticket id: 1, subject: "Ticket 1">}>

[rmosolgo]

Hey, thanks for opening this issue to discuss this further. In my eyes, there are a few things to consider:

• GraphQL-Ruby should move from Timeout.timeout to a manually-decremented timeout value to avoid I/O disruptions, making I/O safe within this timeout.
• If possible, GraphQL-Ruby should use more minimal technique for getting pagination arguments during default connection analysis. Can we get just first and last without loading other arguments?
• I don't intend to remove all I/O from the analysis step. Loading arguments is essential for some kinds of analysis so I intend to keep it there. But maybe this consideration can be better documented (or otherwise less surprising somehow).

[gmac]

Is this not completely avoidable doing something like...?

  class Query < GraphQL::Schema::Object
    field :mentions, Mention.connection_type do
      argument :ticket_id, ID
    end

    def mentions(ticket_id:)
      Ticket.find_by(id: ticket_id)&.mentions
    end
  end

Call me old fashioned but there's some bliss in simplicity.

[rmosolgo]

completely avoidable

Yes, you could also do it in the resolver. But using loads: also supports Relay-style UUIDs (since it goes through Schema.object_from_id) and it also does GraphQL-level authorization and type checking (via resolve_type).

All of that could also be implemented in the application layer itself, but sometimes ... it isn't 😅