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

Fields with lazy results aren't resolved inside directive context #5004

Open
qiffp opened this issue Jun 28, 2024 · 1 comment
Open

Fields with lazy results aren't resolved inside directive context #5004

qiffp opened this issue Jun 28, 2024 · 1 comment

Comments

@qiffp
Copy link

qiffp commented Jun 28, 2024

Describe the bug

If you have a field that is resolved lazily, e.g. using a GraphQL::Batch::Loader, and that field is selected in a query using a directive which applies to the field, the field will be resolved outside of the directive's scope (outside of the yield of the directive's resolve).

Say you're using a gem with a class method that's given a block and that method sets/reverts a class variable before/after the block is called. Then you write a directive that passes resolve's block into that gem's class method as its block. Given a lazy field that uses the class variable, that field will be resolved using the "after" value instead of the "during" value.

Versions

graphql version: 2.3.7
rails (or other framework): N/A
other applicable versions (graphql-batch, etc): graphql-batch 0.6.0

GraphQL schema

class Attributed
  class << self
    attr_reader :attr

    def with_attribute(attr)
      previous_attr = @attr
      @attr = attr

      yield
    ensure
      @attr = previous_attr
    end
  end
end

class TestLoader < GraphQL::Batch::Loader
  def perform(keys)
    keys.each { fulfill(_1, Attributed.attr) }
  end
end

class TestSchema < GraphQL::Schema
  class TestQuery < GraphQL::Schema::Object
    field :lazy_value, String
    def lazy_value
      TestLoader.for.load(nil)
    end

    field :value, String
    def value
      Attributed.attr
    end
  end

  class WithAttribute < GraphQL::Schema::Directive
    locations(GraphQL::Schema::Directive::QUERY)

    def self.resolve(*, &block)
      Attributed.with_attribute("red", &block)
    end
  end

  query(TestQuery)
  directives(WithAttribute)
  use(GraphQL::Batch)
end

GraphQL query

query @withAttribute {
  value
  lazyValue
}
{
  "data": {
    "value": "red",
    "lazyValue": nil
  }
}

Steps to reproduce

Run the query above using the provided schema and classes

Expected behavior

Both value and lazyValue resolve to red

Actual behavior

lazyValue resolves to nil because it's outside of the withAttribute directive scope when it gets resolved and so Attributed.with_attributed has reverted to its previous attr value.

@rmosolgo
Copy link
Owner

rmosolgo commented Jul 1, 2024

Hey, thanks for the detailed report. I agree this is how this should work. Unfortunately it doesn't, and it's not going to be easy.

The problem is that directives are .resolve'd inside run_eager, which is executing query fields that aren't lazy:

call_method_on_directives(:resolve, runtime_object, root_operation.directives) do # execute query level directives

(At first, I thought putting GraphQL::Batch.batch { ... } inside def self.resolve would fix this problem, but it doesn't, because of that problem.)

And it won't be as easy as "just moving it up" to a higher spot in the call stack because of how GraphQL-Ruby does multiplexing. Given two queries like this:

q1 = "query Query1 @withAttribute { lazyValue}"
q2 = "query Query2 { lazyValue }"
r1, r2 = MySchema.multiplex([q1, q2])

We'd expect the first lazyValue to be run inside @withAttribute, but the second lazyValue not to have @withAttribute. But GraphQL-Ruby can't make that guarantee because it runs all lazy values inside the same pool.

It might be possible to split them out -- detect queries which have directives and run them separately. That's the technique GraphQL-Ruby uses for fields and fragments, but there isn't already a place to do it for queries.)

I'll keep this open while I'm thinking it over and follow up here when I have anything more to report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants