Unexpected promise handling by the authorized? type method

[ykonovets]

Describe the bug

self.authorized(object, context) method defined on the Type class does not seem to properly batch queries promised by the GraphqlBatch, firing N+1 queries instead.

Versions

graphql: 1.13.12 rails: 7.0.0 graphql-batch: 0.5.1

GraphQL schema

class Types::InvoiceType < GraphQL::Schema::Object
  field :contract, Types::ContractType, null: false
  
  def contract
      ::Loaders::AssociationRecord.for(Contract, :id).load(object.contract_id)
  end
end

class Types::ContractType < GraphQL::Schema::Object
  field :buyer, Types::BuyerType, null: false
  
  def buyer
    ::Loaders::AssociationRecord.for(Buyer, :id).load(object.buyer_id)
  end
end

class Types::BuyerType < GraphQL::Schema::Object
 def self.authorized?(object, context)
    super && ::Loaders::ShowPolicyLoader.for(BuyerPolicy, context[:authentication_token]).load(object)
  end
end

class Loaders::ShowPolicyLoader < GraphQL::Batch::Loader
  def initialize(policy_class, authentication_token)
    @policy_class = policy_class
    @authentication_token = authentication_token
  end

  def perform(objects)
    @policy_class.new(@authentication_token).objects_to_show(objects).each do |object|
      fulfill(object, true)
    end

    objects.each do |object|
      fulfill(object, false) unless fulfilled?(object)
    end
  end
end

GraphQL query

Example GraphQL query and response (if query execution is involved)

query invoices {
  invoices(perPage: 20) {
    data {
      contract {
        buyer {
          name
        }
      }
    }
  }
}

Expected behavior

I expect graphql-ruby to batch authorization calls for the Types::BuyerType , so that I load permissions for the whole array once and use the result on individual level

Actual behavior

graphql-ruby executes the Loaders::ShowPolicyLoader N times per each buyer, passing just a single ID.

NOTE I know there was already a similar issue posted here and there were some changes made to the library. I double-checked and I am using the right version with the corresponding fixes merged, however, the issue is still here.

Therefore, I am trying to understand whether it is by design and this is exactly how it's supposed to work? If so, what is a recommended way to execute complex authorization logic in efficient way, meaning executing SQL query on the list of objects being authorized rather than authorizing them 1 by 1 firing 1000 DB queries.

Thanks!

[rmosolgo]

Hi, thanks for the detailed report! Yes, I agree it should work this way. I bet it's an implementation mistake in the runtime that authorization isn't batched properly across list items.

[ykonovets]

@rmosolgo, huge thanks for the quick response and great library!

Are you planning to fix this issue soon or maybe I can try fixing it myself, since we rely a lot on graphql-ruby and definitely need this fixed asap. Maybe you have some ideas or wild guesses on what exactly might go wrong in authorization so that I can start there?

Thank you!

[ykonovets]

By the way, another interesting finding: if I execute the following query

query invoices {
  invoices(perPage: 20) {
    data {
      id
    }
}

, everything seems to be working just fine. So the issue seems to exist for the deep nested objects only.

[rmosolgo]

I don't have plans to fix it immediately. Here's where list values are executed:

https://github.com/rmosolgo/graphql-ruby/blob/01cb5b41db169df602d3ee2d1b1e0b20a23868f5/lib/graphql/execution/interpreter/runtime.rb#L760-L779

So that's certainly where I'd start looking for how promises (wrapped with GraphQL::Execution::Lazy in graphql-ruby) are handled for lists.

Another big help would be writing an isolated failure case for this, for example:

require "bundler/inline"

gemfile do 
  gem "graphql", "~>2.0"
  gem "graphql-batch"
end 

class ListAuthSchema < GraphQL::Schema 
  # example schema here 
end 

# example query that demonstrates bad batching here 

Getting a reliable reproduction will be half the battle!

[ykonovets]

Amazing, thanks! Let me start with a good example.

[ykonovets]

@rmosolgo, here are the examples, let me know if we are good with them, please!

Here is the first example where InvoiceType#contract is a plain object (not promise):

require "bundler/inline"

gemfile do
  gem "graphql", "1.13.12"
  gem "graphql-batch", "0.5.1"
end

Contract = Struct.new(:id, :buyer_id)
Invoice = Struct.new(:id, :contract_id)

$contracts = [Contract.new(1, 1), Contract.new(2, 1), Contract.new(3, 2), Contract.new(4, 2)]
$invoices = [Invoice.new(1, 1), Invoice.new(2, 2), Invoice.new(3, 3), Invoice.new(4, 4)]

class ShowPolicyLoader < GraphQL::Batch::Loader
  def initialize(model)
    puts "Loaders::ShowPolicyLoader init"
    @model = model
  end

  def perform(objects)
    puts "Loaders::ShowPolicyLoader#{self.object_id} perform with #{objects.map(&:id)}, #{@model}"

    objects.each { |o| fulfill(o, true) }
  end
end

class ContractType < GraphQL::Schema::Object
  field :id, ID, null: false

  def self.authorized?(object, context)
    super && ShowPolicyLoader.for(Contract).load(object)
  end
end

class InvoiceType < GraphQL::Schema::Object
  field :id, ID, null: false
  field :contract, ContractType, null: false

  def contract
    $contracts.find { |c| c.id == object.contract_id }
  end

  def self.authorized?(object, context)
    super && ShowPolicyLoader.for(Invoice).load(object)
  end
end

class QueryType < GraphQL::Schema::Object
  field :invoices, [InvoiceType], null: false

  def invoices
    Promise.resolve($invoices)
  end
end

class ListAuthSchema < GraphQL::Schema
  query(QueryType)

  use GraphQL::Batch
end

gql = <<-GQL
query InvoicesList {
  invoices {
    contract {
      id
    }
  }
}
GQL

p ListAuthSchema.execute(gql)

=>

Loaders::ShowPolicyLoader init
Loaders::ShowPolicyLoader2340 perform with [1, 2, 3, 4], Invoice
Loaders::ShowPolicyLoader init
Loaders::ShowPolicyLoader2380 perform with [1, 2, 3, 4], Contract

Everything seems to be working just fine

[ykonovets]

Now, let's try using promise in the InvoiceType#contract field: let's replace

class InvoiceType < GraphQL::Schema::Object
  ...
  def contract
    $contracts.find { |c| c.id == object.contract_id }
  end
  ...
end

with

class InvoiceType < GraphQL::Schema::Object
  ...
  def contract
    Promise.resolve($contracts.find { |c| c.id == object.contract_id })
  end
  ...
end

=>

Loaders::ShowPolicyLoader init
Loaders::ShowPolicyLoader2340 perform with [1, 2, 3, 4], Invoice
Loaders::ShowPolicyLoader init
Loaders::ShowPolicyLoader2380 perform with [1], Contract
Loaders::ShowPolicyLoader2380 perform with [2], Contract
Loaders::ShowPolicyLoader2380 perform with [3], Contract
Loaders::ShowPolicyLoader2380 perform with [4], Contract

Other words, looks like it does not properly handle .authorized? for the nested promise

[ykonovets]

@rmosolgo, I just tried the same approach using standard dataloader instead of graphql-ruby and seems to be working.

require "bundler/inline"

gemfile do
  gem "graphql", "1.13.12"
end

Contract = Struct.new(:id, :buyer_id)
Invoice = Struct.new(:id, :contract_id)

$contracts = [Contract.new(1, 1), Contract.new(2, 1), Contract.new(3, 2), Contract.new(4, 2)]
$invoices = [Invoice.new(1, 1), Invoice.new(2, 2), Invoice.new(3, 3), Invoice.new(4, 4)]

class FakeLoader < GraphQL::Dataloader::Source
  def initialize(data)
    @data = data
  end

  def fetch(objects)
    objects
  end
end

class ShowPolicyLoader < GraphQL::Dataloader::Source
  def initialize(model)
    puts "Loaders::ShowPolicyLoader init"
    @model = model
  end

  def fetch(objects)
    puts "Loaders::ShowPolicyLoader#{self.object_id} perform with #{objects.map(&:id)}, #{@model}"

    objects.map { |o| true}
  end
end

class ContractType < GraphQL::Schema::Object
  field :id, ID, null: false

  def self.authorized?(object, context)
    super && context.dataloader.with(ShowPolicyLoader, Contract).load(object)
  end
end

class InvoiceType < GraphQL::Schema::Object
  field :id, ID, null: false
  field :contract, ContractType, null: false

  def contract
    dataloader.with(FakeLoader, Contract).load($contracts.find { |c| c.id == object.contract_id })
  end

  def self.authorized?(object, context)
    super && context.dataloader.with(ShowPolicyLoader, Invoice).load(object)
  end
end

class QueryType < GraphQL::Schema::Object
  field :invoices, [InvoiceType], null: false

  def invoices
    dataloader.with(FakeLoader, Invoice).load_all($invoices)
  end
end

class ListAuthSchema < GraphQL::Schema
  query(QueryType)

  use GraphQL::Dataloader
end

gql = <<-GQL
query InvoicesList {
  invoices {
    contract {
      id
    }
  }
}
GQL

p ListAuthSchema.execute(gql)

=>

Loaders::ShowPolicyLoader init
Loaders::ShowPolicyLoader2240 perform with [1, 2, 3, 4], Invoice
Loaders::ShowPolicyLoader init
Loaders::ShowPolicyLoader2280 perform with [1, 2, 3, 4], Contract

Looks like the issue might not in the graphql-ruby itself, but in the way it works with graphql-batch. As of now, I think on moving to standard dataloaders to overcome the issue. What do you think?

UPD: confirmed, after removing GraphQL::Batch from the Schema and moving to dataloaders the issue seem to be gone. I think this is not a graphql-ruby issue, therefore I will move it to the graphql-batch repository.

[rmosolgo]

Thanks for sharing what you found here. I don't have plans to work further on this, so I'm going to close the issue. If anyone runs into similar trouble, please open a new issue!