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

Defer is not working on fragments #4953

Closed
alexus37 opened this issue May 15, 2024 · 10 comments · Fixed by #4959
Closed

Defer is not working on fragments #4953

alexus37 opened this issue May 15, 2024 · 10 comments · Fixed by #4959

Comments

@alexus37
Copy link

Describe the bug

If I put the @defer directive on fragment the fragment gets removed from the initial response but the context does not have a defer key to continue the execution.

Versions

graphql version: 2.2
graphql-pro version: 1.27

Steps to reproduce

Steps to reproduce the behavior

    query = <<~'GRAPHQL'
      query($id: ID!) {
        node(id: $id) {
          ... on Issue {
            title
            ...IssueBody @defer(label: "body")
          }
        }
      }
      fragment IssueBody on Issue {
        body
      }
    GRAPHQL

    response = execute_query(
      query,
      :$id => @issue.global_relay_id,
      :viewer => @author
    )
    debugger

    initial_data = response.data
    deferals = []
    # initial response only includes the title
    assert_equal initial_data, { "node" => { "title" => @issue.title } }
    assert response.query.context[:defer].present? # fails

Expected behavior

A clear and concise description of what you expected to happen.

response.query.context[:defer] is present and can compute the remaining chunks

Actual behavior

response.query.context[:defer] is nil

@rmosolgo
Copy link
Owner

Hey, thanks for reporting this. I definitely expect that example to work!

I wrote up a basic script here, but it worked fine: https://gist.github.com/rmosolgo/2d25ec6be3ad20476a978dcbc9eb46d1

To debug further:

  • Are there other cases that @defer does work in your system? For example, if you put it directly on body (eg body @defer), or on an inline fragment (... @defer { body }), do those work?
  • Could you share the code that hooks up GraphQL::Pro::Defer to your schema? (I assume it's there, or else the query would fail with a validation error, but maybe the setup code will give some clue.)
  • If you copy my script into your test suite (with the brand new schema definition), does it work for you? (If it doesn't, then maybe there are some Ruby-level moving parts in your environment to investigate.)

Let me know what you find!

@alexus37
Copy link
Author

alexus37 commented May 16, 2024

Are there other cases that @defer does work in your system? For example, if you put it directly on body (eg body @defer), or on an inline fragment (... @defer { body }), do those work?

Putting the directive directly on a field works as expected. However, inline fragments do not work.

Could you share the code that hooks up GraphQL::Pro::Defer to your schema? (I assume it's there, or else the query would fail with a validation error, but maybe the setup code will give some clue.)

Sure, the code that hooks it up is just calling directive(Directives::Defer). The directive itself is basically what is in the docs with the addition to run defer only if run_defer_directive is set in the context.

class Defer < GraphQL::Pro::Defer
      def self.resolve(obj, arguments, context, &block)
        # only use defer if the run_defer_directive flag is set on the context
        if context[:run_defer_directive]
          # While the query is running, store the batch executor to re-use later
          context[:graphql_batch_executor] ||= GraphQL::Batch::Executor.current
          super
        else
          yield
        end
      end

      class Deferral < GraphQL::Pro::Defer::Deferral
        def resolve
          # Before calling the deferred execution,
          # set GraphQL-Batch back up:
          prev_executor = GraphQL::Batch::Executor.current
          GraphQL::Batch::Executor.current ||= @context[:graphql_batch_executor]
          super
        ensure
          # Clean up afterward:
          GraphQL::Batch::Executor.current = prev_executor
        end
      end
    end

If you copy my script into your test suite (with the brand new schema definition), does it work for you? (If it doesn't, then maybe there are some Ruby-level moving parts in your environment to investigate.)

If I add your script into my test env it works, even when using my Defer directive. :/

If I adjust your script to have nested fragments it fails for me:

require "bundler/inline"

gemfile do
  gem "graphql", "~>1.13.22"
  gem "graphql-pro", "~>1.27.0"
end

class Schema < GraphQL::Schema
  class Issue < GraphQL::Schema::Object
    field :title, String
    field :body, String
  end

  class Repo < GraphQL::Schema::Object
    field :issue, Issue
    field :name, String
  end


  class Query < GraphQL::Schema::Object
    field :repo, Repo

    def repo
      {
        name: "my repo",
        issue: {
          title: "Failed Reticulated Splines",
          body: "It encountered an error while reticulating splines"
        }
      }
    end
  end

  query(Query)
  use GraphQL::Pro::Defer
end


query_str = <<-GRAPHQL
{
  repo {
    ...RepoFrag
  }
}

fragment RepoFrag on Repo {
  name
  issue {
    ... IssueFrag
  }
}

fragment IssueFrag on Issue {
  title
  ... IssueBody @defer
}

fragment IssueBody on Issue {
  body
}

GRAPHQL

res = Schema.execute(query_str)
if res.context[:defer].present?
  puts "Defer found"
else
  puts "Defer not found"
end

res.context[:defer].each do |deferral|
  pp [:deferred, deferral.to_h]
end

@rmosolgo
Copy link
Owner

Thanks for sharing those details! A couple of followup thoughts:

I noticed your original report uses GraphQL v2.2, but the second script in your latest comment uses GraphQL v1.13.22. Which one are you currently using in your app? (I wouldn't be surprised if some cases aren't handled well in v1.13.22... I'm open to addressing those bugs if you're currently using that version, but I want to make sure!)

I see your Defer directive only takes effect when context[:run_defer_directive] is present. I updated the gist with those modifications (https://gist.github.com/rmosolgo/2d25ec6be3ad20476a978dcbc9eb46d1) and it works alright for me (using v2.2 or v1.13.22), but only when I pass context: { run_defer_directive: true}. In your original report, I don't see run_defer_directive: true being added; does it work when you pass that flag?

@alexus37
Copy link
Author

Which one are you currently using in your app?

Sorry for any confusion! We are currently in the process of upgrading to the latest version. After testing both, it appears that there are issues with both 1.13 and 2.2.

I see your Defer directive only takes effect when context[:run_defer_directive] is present. I updated the gist with those modifications (https://gist.github.com/rmosolgo/2d25ec6be3ad20476a978dcbc9eb46d1) and it works alright for me (using v2.2 or v1.13.22), but only when I pass context: { run_defer_directive: true}. In your original report, I don't see run_defer_directive: true being added; does it work when you pass that flag?

That's a helpful tip! I took out that flag from the original post since it wasn't part of the default directive, but I included it during my testing. Additionally, the script I mentioned in my earlier comment simply utilizes use GraphQL::Pro::Defer and is still encountering the issue with both versions.

@rmosolgo
Copy link
Owner

Ok, thanks for clarifying! I also got your second script to fail on 2.2. I tracked it down to a bug in GraphQL-Ruby's runtime and replicated it in the test suite. I'll keep working up a fix in #4959

@rmosolgo
Copy link
Owner

Thanks again for the detailed report! I fixed this on master in #4959 and backported it to 2.2 in #4962. I just released 2.2.15 with the fix -- please let me know if you encounter more problems with it after updating to that version! (The next 2.3 release will also include the fix.)

@alexus37
Copy link
Author

Thank you so much for the fix! Any idea when we can expect the release of version 2.3.4?

@rmosolgo
Copy link
Owner

I just released it 🚢 !

@alexus37
Copy link
Author

alexus37 commented Jun 6, 2024

How much work would it be to back port it to version 2.0?

@rmosolgo
Copy link
Owner

rmosolgo commented Jun 8, 2024

I don't know, but I can give it a try if you're on that version. Would a 2.0.x backport be useful for you?

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

Successfully merging a pull request may close this issue.

2 participants