From fda24161bccb7322492464f38ef90992813da544 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 18 Apr 2025 06:56:40 -0400 Subject: [PATCH 1/5] Support has_many in ActiveRecordAssociationSource --- .../active_record_association_source.rb | 17 ++++++++++------- .../active_record_association_source_spec.rb | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/lib/graphql/dataloader/active_record_association_source.rb b/lib/graphql/dataloader/active_record_association_source.rb index 9f87793e6fc..2c2f3d67733 100644 --- a/lib/graphql/dataloader/active_record_association_source.rb +++ b/lib/graphql/dataloader/active_record_association_source.rb @@ -23,9 +23,11 @@ def load(record) def fetch(records) record_classes = Set.new.compare_by_identity associated_classes = Set.new.compare_by_identity + all_singular_associations = true records.each do |record| if record_classes.add?(record.class) reflection = record.class.reflect_on_association(@association) + all_singular_associations &= !reflection.collection? if !reflection.polymorphic? && reflection.klass associated_classes.add(reflection.klass) end @@ -41,17 +43,18 @@ def fetch(records) ::ActiveRecord::Associations::Preloader.new(records: records, associations: @association, available_records: available_records, scope: @scope).call loaded_associated_records = records.map { |r| r.public_send(@association) } - records_by_model = {} - loaded_associated_records.each do |record| - if record - updates = records_by_model[record.class] ||= {} - updates[record.id] = record - end - end + if @scope.nil? # Don't cache records loaded via scope because they might have reduced `SELECT`s # Could check .select_values here? + records_by_model = {} + loaded_associated_records.flatten.each do |record| + if record + updates = records_by_model[record.class] ||= {} + updates[record.id] = record + end + end records_by_model.each do |model_class, updates| dataloader.with(RECORD_SOURCE_CLASS, model_class).merge(updates) end diff --git a/spec/graphql/dataloader/active_record_association_source_spec.rb b/spec/graphql/dataloader/active_record_association_source_spec.rb index 1ee764702a6..7718a3ace84 100644 --- a/spec/graphql/dataloader/active_record_association_source_spec.rb +++ b/spec/graphql/dataloader/active_record_association_source_spec.rb @@ -106,6 +106,25 @@ assert_equal ::Band.find(1), vulfpeck end + it_dataloads "works with collection associations" do |d| + wilco = ::Band.find(4) + chon = ::Band.find(3) + albums_by_band = nil + log = with_active_record_log(colorize: false) do + albums_by_band = d.with(GraphQL::Dataloader::ActiveRecordAssociationSource, :albums).load_all([wilco, chon]) + end + + assert_equal [[6], [4, 5]], albums_by_band.map { |al| al.map(&:id) } + + albums = nil + log = with_active_record_log(colorize: false) do + albums = d.with(GraphQL::Dataloader::ActiveRecordSource, Album).load_all([3,4,5,6]) + end + + assert_equal [3,4,5,6], albums.map(&:id) + assert_includes log, 'WHERE "albums"."id" = ? [["id", 3]]' + end + if Rails::VERSION::STRING > "7.1" # not supported in <7.1 it_dataloads "loads with composite primary keys and warms the cache" do |d| my_first_car = ::Album.find(2) From 22d22f813ea756132433680f64a4bb777fcd6932 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 18 Apr 2025 06:59:31 -0400 Subject: [PATCH 2/5] Remove unused variable --- lib/graphql/dataloader/active_record_association_source.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/graphql/dataloader/active_record_association_source.rb b/lib/graphql/dataloader/active_record_association_source.rb index 2c2f3d67733..1eac637fc02 100644 --- a/lib/graphql/dataloader/active_record_association_source.rb +++ b/lib/graphql/dataloader/active_record_association_source.rb @@ -23,11 +23,9 @@ def load(record) def fetch(records) record_classes = Set.new.compare_by_identity associated_classes = Set.new.compare_by_identity - all_singular_associations = true records.each do |record| if record_classes.add?(record.class) reflection = record.class.reflect_on_association(@association) - all_singular_associations &= !reflection.collection? if !reflection.polymorphic? && reflection.klass associated_classes.add(reflection.klass) end @@ -44,7 +42,6 @@ def fetch(records) loaded_associated_records = records.map { |r| r.public_send(@association) } - if @scope.nil? # Don't cache records loaded via scope because they might have reduced `SELECT`s # Could check .select_values here? From 5ade1a7c61e1a739119190e528460b030a26f5d2 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 18 Apr 2025 07:05:43 -0400 Subject: [PATCH 3/5] Add log assertion --- spec/graphql/dataloader/active_record_association_source_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/graphql/dataloader/active_record_association_source_spec.rb b/spec/graphql/dataloader/active_record_association_source_spec.rb index 7718a3ace84..cf31938e4c6 100644 --- a/spec/graphql/dataloader/active_record_association_source_spec.rb +++ b/spec/graphql/dataloader/active_record_association_source_spec.rb @@ -115,6 +115,7 @@ end assert_equal [[6], [4, 5]], albums_by_band.map { |al| al.map(&:id) } + assert_includes log, 'SELECT "albums".* FROM "albums" WHERE "albums"."band_id" IN (?, ?) [["band_id", 4], ["band_id", 3]]' albums = nil log = with_active_record_log(colorize: false) do From ec703d72fc1d107147dce464db589ab17420a42b Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 18 Apr 2025 08:33:55 -0400 Subject: [PATCH 4/5] Properly normalize scopes as keys --- .../active_record_association_source.rb | 8 +++++++ .../active_record_association_source_spec.rb | 23 +++++++++++++++++++ spec/support/active_record_setup.rb | 1 + 3 files changed, 32 insertions(+) diff --git a/lib/graphql/dataloader/active_record_association_source.rb b/lib/graphql/dataloader/active_record_association_source.rb index 1eac637fc02..6a3745b46e8 100644 --- a/lib/graphql/dataloader/active_record_association_source.rb +++ b/lib/graphql/dataloader/active_record_association_source.rb @@ -12,6 +12,14 @@ def initialize(association, scope = nil) @scope = scope end + def self.batch_key_for(association, scope = nil) + if scope + [association, scope.to_sql] + else + [association] + end + end + def load(record) if (assoc = record.association(@association)).loaded? assoc.target diff --git a/spec/graphql/dataloader/active_record_association_source_spec.rb b/spec/graphql/dataloader/active_record_association_source_spec.rb index cf31938e4c6..ca292c1f788 100644 --- a/spec/graphql/dataloader/active_record_association_source_spec.rb +++ b/spec/graphql/dataloader/active_record_association_source_spec.rb @@ -126,6 +126,29 @@ assert_includes log, 'WHERE "albums"."id" = ? [["id", 3]]' end + it_dataloads "works with collection associations with scope" do |d| + wilco = ::Band.find(4) + chon = ::Band.find(3) + albums_by_band = nil + log = with_active_record_log(colorize: false) do + one_month_ago = 1.month.ago.end_of_day + albums_by_band_1 = d.with(GraphQL::Dataloader::ActiveRecordAssociationSource, :albums, Album.where("created_at >= ?", one_month_ago)).request(wilco) + albums_by_band_2 = d.with(GraphQL::Dataloader::ActiveRecordAssociationSource, :albums, Album.where("created_at >= ?", one_month_ago)).request(chon) + albums_by_band = [albums_by_band_1.load, albums_by_band_2.load] + end + + assert_equal [[6], [4, 5]], albums_by_band.map { |al| al.map(&:id) } + assert_includes log, 'SELECT "albums".* FROM "albums" WHERE (created_at >= ?) AND "albums"."band_id" IN (?, ?)' + + albums = nil + log = with_active_record_log(colorize: false) do + albums = d.with(GraphQL::Dataloader::ActiveRecordSource, Album).load_all([3,4,5,6]) + end + + assert_equal [3,4,5,6], albums.map(&:id) + assert_includes log, 'WHERE "albums"."id" IN (?, ?, ?, ?) [["id", 3], ["id", 4], ["id", 5], ["id", 6]]' + end + if Rails::VERSION::STRING > "7.1" # not supported in <7.1 it_dataloads "loads with composite primary keys and warms the cache" do |d| my_first_car = ::Album.find(2) diff --git a/spec/support/active_record_setup.rb b/spec/support/active_record_setup.rb index 1ddd752f843..db8ff54d673 100644 --- a/spec/support/active_record_setup.rb +++ b/spec/support/active_record_setup.rb @@ -70,6 +70,7 @@ t.integer :band_id t.string :band_name t.integer :band_genre + t.timestamps end create_table :books do |t| From 30fa82ca5d84de053e7e1f59969e075bc61803b6 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 18 Apr 2025 08:41:27 -0400 Subject: [PATCH 5/5] Update assertions for Rails 7 --- .../dataloader/active_record_association_source_spec.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/spec/graphql/dataloader/active_record_association_source_spec.rb b/spec/graphql/dataloader/active_record_association_source_spec.rb index ca292c1f788..cdf761183ba 100644 --- a/spec/graphql/dataloader/active_record_association_source_spec.rb +++ b/spec/graphql/dataloader/active_record_association_source_spec.rb @@ -130,6 +130,7 @@ wilco = ::Band.find(4) chon = ::Band.find(3) albums_by_band = nil + one_month_ago = nil log = with_active_record_log(colorize: false) do one_month_ago = 1.month.ago.end_of_day albums_by_band_1 = d.with(GraphQL::Dataloader::ActiveRecordAssociationSource, :albums, Album.where("created_at >= ?", one_month_ago)).request(wilco) @@ -138,7 +139,13 @@ end assert_equal [[6], [4, 5]], albums_by_band.map { |al| al.map(&:id) } - assert_includes log, 'SELECT "albums".* FROM "albums" WHERE (created_at >= ?) AND "albums"."band_id" IN (?, ?)' + expected_log = if Rails::VERSION::STRING > "8" + 'SELECT "albums".* FROM "albums" WHERE (created_at >= ?) AND "albums"."band_id" IN (?, ?)' + else + 'SELECT "albums".* FROM "albums" WHERE (created_at >= ' + one_month_ago.utc.strftime("'%Y-%m-%d %H:%M:%S.%6N'") + ') AND "albums"."band_id" IN (?, ?)' + end + + assert_includes log, expected_log albums = nil log = with_active_record_log(colorize: false) do