From 74bc9c41a13c78ce3f06d330f2b9a6d73cac52af Mon Sep 17 00:00:00 2001 From: DmitryTsepelev Date: Mon, 6 Jan 2025 17:37:23 +0300 Subject: [PATCH 1/2] Dataloader support --- CHANGELOG.md | 1 + README.md | 28 ++++++++++ lib/graphql/fragment_cache/object_helpers.rb | 7 +++ .../fragment_cache/object_helpers_spec.rb | 53 +++++++++++++++++++ spec/support/models/user.rb | 6 +++ spec/support/test_schema.rb | 13 +++++ 6 files changed, 108 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ddbd58f..ac0fdbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## master +- [PR#130](https://github.com/DmitryTsepelev/graphql-ruby-fragment_cache/pull/130) Dataloader support ([@DmitryTsepelev][]) - [PR#125](https://github.com/DmitryTsepelev/graphql-ruby-fragment_cache/pull/125) Introduce cache lookup instrumentation hook ([@danielhartnell][]) ## 1.20.5 (2024-11-02) diff --git a/README.md b/README.md index ecce0a4..9430db7 100644 --- a/README.md +++ b/README.md @@ -381,6 +381,34 @@ class QueryType < BaseObject end ``` +## Dataloader + +If you are using [Dataloader](https://graphql-ruby.org/dataloader/overview.html), you will need to let the gem know using `dataloader: true`: + +```ruby +class PostType < BaseObject + field :author, User, null: false + + def author + cache_fragment(dataloader: true) do + dataloader.with(AuthorDataloaderSource).load(object.id) + end + end +end + +# or + +class PostType < BaseObject + field :author, User, null: false, cache_fragment: {dataloader: true} + + def author + dataloader.with(AuthorDataloaderSource).load(object.id) + end +end +``` + +The problem is that I didn't find a way to detect that dataloader (and, therefore, Fiber) is used, and the block is forced to resolve, causing the N+1 inside the Dataloader Source class. + ## How to use `#cache_fragment` in extensions (and other places where context is not available) If you want to call `#cache_fragment` from places other that fields or resolvers, you'll need to pass `context` explicitly and turn on `raw_value` support. For instance, let's take a look at this extension: diff --git a/lib/graphql/fragment_cache/object_helpers.rb b/lib/graphql/fragment_cache/object_helpers.rb index 0dc41b2..f020619 100644 --- a/lib/graphql/fragment_cache/object_helpers.rb +++ b/lib/graphql/fragment_cache/object_helpers.rb @@ -48,6 +48,13 @@ def cache_fragment(object_to_cache = NO_OBJECT, **options, &block) fragment = Fragment.new(context_to_use, **options) + if options.delete(:dataloader) + # the following line will block the current fiber until Dataloader is resolved, and we will + # use the resolved value as the `object_to_cache` + object_to_cache = block.call + block = nil + end + GraphQL::FragmentCache::Schema::LazyCacheResolver.new(fragment, context_to_use, object_to_cache, &block) end end diff --git a/spec/graphql/fragment_cache/object_helpers_spec.rb b/spec/graphql/fragment_cache/object_helpers_spec.rb index c5b9161..c2f2e19 100644 --- a/spec/graphql/fragment_cache/object_helpers_spec.rb +++ b/spec/graphql/fragment_cache/object_helpers_spec.rb @@ -777,6 +777,59 @@ def post(id:, expires_in: nil) end end + describe "caching fields with dataloader" do + let(:query) do + <<~GQL + query GetPosts { + posts { + id + dataloaderCachedAuthor { + name + } + } + } + GQL + end + + let(:schema) do + build_schema do + use GraphQL::Dataloader + query(Types::Query) + end + end + + let(:user1) { User.new(id: 1, name: "User #1") } + let(:user2) { User.new(id: 2, name: "User #2") } + + let!(:post1) { Post.create(id: 1, title: "object test 1", author: user1) } + let!(:post2) { Post.create(id: 2, title: "object test 2", author: user2) } + + before do + allow(User).to receive(:find_by_post_ids).and_call_original + + # warmup cache + execute_query + expect(User).to have_received(:find_by_post_ids).with([post1.id, post2.id]) + + # make objects dirty + user1.name = "User #1 new" + user2.name = "User #2 new" + end + + it "returns cached results" do + expect(execute_query.dig("data", "posts")).to eq([ + { + "id" => "1", + "dataloaderCachedAuthor" => {"name" => "User #1"} + }, + { + "id" => "2", + "dataloaderCachedAuthor" => {"name" => "User #2"} + } + ]) + end + end + describe "conditional caching" do let(:schema) do field_resolver = resolver diff --git a/spec/support/models/user.rb b/spec/support/models/user.rb index 27b84d4..cafdd4b 100644 --- a/spec/support/models/user.rb +++ b/spec/support/models/user.rb @@ -4,6 +4,12 @@ class User attr_reader :id attr_accessor :name + class << self + def find_by_post_ids(post_ids) + post_ids.map { |id| Post.find(id).author } + end + end + def initialize(id:, name:) @id = id @name = name diff --git a/spec/support/test_schema.rb b/spec/support/test_schema.rb index cffd253..7b9c805 100644 --- a/spec/support/test_schema.rb +++ b/spec/support/test_schema.rb @@ -7,6 +7,12 @@ def perform(posts) end end +class AuthorDataloaderSource < GraphQL::Dataloader::Source + def fetch(post_ids) + User.find_by_post_ids(post_ids) + end +end + module Types class Base < GraphQL::Schema::Object include GraphQL::FragmentCache::Object @@ -41,6 +47,7 @@ class Post < Base field :cached_author, User, null: false field :batched_cached_author, User, null: false field :cached_author_inside_batch, User, null: false + field :dataloader_cached_author, User, null: false field :meta, String, null: true @@ -60,6 +67,12 @@ def cached_author_inside_batch cache_fragment(author, context: context) end end + + def dataloader_cached_author + cache_fragment(dataloader: true) do + dataloader.with(AuthorDataloaderSource).load(object.id) + end + end end class PostInput < GraphQL::Schema::InputObject From 56d59b3acb37b222dcc32d23c2f715e0fe6ce852 Mon Sep 17 00:00:00 2001 From: DmitryTsepelev Date: Tue, 7 Jan 2025 13:36:08 +0300 Subject: [PATCH 2/2] Ensure dataloader is called only once --- lib/graphql/fragment_cache/fragment.rb | 9 +++------ lib/graphql/fragment_cache/object_helpers.rb | 7 ------- .../schema/lazy_cache_resolver.rb | 11 ++++++++++ .../fragment_cache/object_helpers_spec.rb | 5 ++++- .../schema/lazy_cache_resolver_spec.rb | 20 +++++++++---------- 5 files changed, 28 insertions(+), 24 deletions(-) diff --git a/lib/graphql/fragment_cache/fragment.rb b/lib/graphql/fragment_cache/fragment.rb index fcae4a8..c45b093 100644 --- a/lib/graphql/fragment_cache/fragment.rb +++ b/lib/graphql/fragment_cache/fragment.rb @@ -16,12 +16,10 @@ def read_multi(fragments) return fragments.map { |f| [f, f.read] }.to_h end - fragments_to_cache_keys = fragments - .map { |f| [f, f.cache_key] }.to_h + fragments_to_cache_keys = fragments.map { |f| [f, f.cache_key] }.to_h # Filter out all the cache_keys for fragments with renew_cache: true in their context - cache_keys = fragments_to_cache_keys - .reject { |k, _v| k.context[:renew_cache] == true }.values + cache_keys = fragments_to_cache_keys.reject { |k, _v| k.context[:renew_cache] == true }.values # If there are cache_keys look up values with read_multi otherwise return an empty hash cache_keys_to_values = if cache_keys.empty? @@ -46,8 +44,7 @@ def read_multi(fragments) end # Fragmenst without values or with renew_cache: true in their context will have nil values like the read method - fragments_to_cache_keys - .map { |fragment, cache_key| [fragment, cache_keys_to_values[cache_key]] }.to_h + fragments_to_cache_keys.map { |fragment, cache_key| [fragment, cache_keys_to_values[cache_key]] }.to_h end end diff --git a/lib/graphql/fragment_cache/object_helpers.rb b/lib/graphql/fragment_cache/object_helpers.rb index f020619..0dc41b2 100644 --- a/lib/graphql/fragment_cache/object_helpers.rb +++ b/lib/graphql/fragment_cache/object_helpers.rb @@ -48,13 +48,6 @@ def cache_fragment(object_to_cache = NO_OBJECT, **options, &block) fragment = Fragment.new(context_to_use, **options) - if options.delete(:dataloader) - # the following line will block the current fiber until Dataloader is resolved, and we will - # use the resolved value as the `object_to_cache` - object_to_cache = block.call - block = nil - end - GraphQL::FragmentCache::Schema::LazyCacheResolver.new(fragment, context_to_use, object_to_cache, &block) end end diff --git a/lib/graphql/fragment_cache/schema/lazy_cache_resolver.rb b/lib/graphql/fragment_cache/schema/lazy_cache_resolver.rb index 046952f..8defbf2 100644 --- a/lib/graphql/fragment_cache/schema/lazy_cache_resolver.rb +++ b/lib/graphql/fragment_cache/schema/lazy_cache_resolver.rb @@ -16,6 +16,8 @@ def initialize(fragment, query_ctx, object_to_cache, &block) @block = block @lazy_state[:pending_fragments] << @fragment + + ensure_dataloader_resulution! if @fragment.options[:dataloader] end def resolve @@ -35,6 +37,15 @@ def resolve @query_ctx.fragments << @fragment end end + + private + + def ensure_dataloader_resulution! + return if FragmentCache.cache_store.exist?(@fragment.cache_key) + + @object_to_cache = @block.call + @block = nil + end end end end diff --git a/spec/graphql/fragment_cache/object_helpers_spec.rb b/spec/graphql/fragment_cache/object_helpers_spec.rb index c2f2e19..7fbc4f7 100644 --- a/spec/graphql/fragment_cache/object_helpers_spec.rb +++ b/spec/graphql/fragment_cache/object_helpers_spec.rb @@ -804,12 +804,13 @@ def post(id:, expires_in: nil) let!(:post1) { Post.create(id: 1, title: "object test 1", author: user1) } let!(:post2) { Post.create(id: 2, title: "object test 2", author: user2) } + let(:memory_store) { GraphQL::FragmentCache::MemoryStore.new } + before do allow(User).to receive(:find_by_post_ids).and_call_original # warmup cache execute_query - expect(User).to have_received(:find_by_post_ids).with([post1.id, post2.id]) # make objects dirty user1.name = "User #1 new" @@ -827,6 +828,8 @@ def post(id:, expires_in: nil) "dataloaderCachedAuthor" => {"name" => "User #2"} } ]) + + expect(User).to have_received(:find_by_post_ids).with([post1.id, post2.id]).once end end diff --git a/spec/graphql/fragment_cache/schema/lazy_cache_resolver_spec.rb b/spec/graphql/fragment_cache/schema/lazy_cache_resolver_spec.rb index c60ece5..fe53195 100644 --- a/spec/graphql/fragment_cache/schema/lazy_cache_resolver_spec.rb +++ b/spec/graphql/fragment_cache/schema/lazy_cache_resolver_spec.rb @@ -6,13 +6,19 @@ describe "#initialize" do context "lazy cache resolver state management" do let(:state_key) { :lazy_cache_resolver_statez } + let(:gql_context) { instance_double "Context" } + let(:fragment) { GraphQL::FragmentCache::Fragment.new(gql_context) } + + before do + allow(gql_context).to receive(:namespace).and_return({}) + end it "adds lazy state property to the query context" do context = {} expect(context).not_to have_key(state_key) - GraphQL::FragmentCache::Schema::LazyCacheResolver.new(nil, context, {}) + GraphQL::FragmentCache::Schema::LazyCacheResolver.new(fragment, context, {}) expect(context).to have_key(state_key) end @@ -20,7 +26,7 @@ it "has :pending_fragments Set in state" do context = {} - GraphQL::FragmentCache::Schema::LazyCacheResolver.new({}, context, {}) + GraphQL::FragmentCache::Schema::LazyCacheResolver.new(fragment, context, {}) expect(context[state_key]).to have_key(:pending_fragments) expect(context[state_key][:pending_fragments]).to be_instance_of(Set) @@ -29,7 +35,7 @@ it "has :resolved_fragments Hash in state" do context = {} - GraphQL::FragmentCache::Schema::LazyCacheResolver.new({}, context, {}) + GraphQL::FragmentCache::Schema::LazyCacheResolver.new(fragment, context, {}) expect(context[state_key]).to have_key(:resolved_fragments) expect(context[state_key][:resolved_fragments]).to be_instance_of(Hash) @@ -39,7 +45,7 @@ context = {} fragments = [] - 3.times { fragments.push(Object.new) } + 3.times { fragments.push(GraphQL::FragmentCache::Fragment.new(gql_context)) } fragments.each do |f| GraphQL::FragmentCache::Schema::LazyCacheResolver.new(f, context, {}) @@ -51,10 +57,4 @@ end end end - - it "has :resolve method" do - lazy_cache_resolver = GraphQL::FragmentCache::Schema::LazyCacheResolver.new({}, {}, {}) - - expect(lazy_cache_resolver).to respond_to(:resolve) - end end