From 56d59b3acb37b222dcc32d23c2f715e0fe6ce852 Mon Sep 17 00:00:00 2001 From: DmitryTsepelev Date: Tue, 7 Jan 2025 13:36:08 +0300 Subject: [PATCH] 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