fix: handle instance dependent associations (#567)

Author: FraDim

app/services/forest_liana/base_getter.rb

@@ -29,5 +29,27 @@ def includes_for_serialization
     def compute_includes
       @includes = ForestLiana::QueryHelper.get_one_association_names_symbol(@resource)
     end
+
+    def optimize_record_loading(resource, records)
+      instance_dependent_associations = instance_dependent_associations(resource)
+      eager_loads = @includes - instance_dependent_associations
+
+      result = records.eager_load(eager_loads)
+
+      # Rails 7 can mix `eager_load` and `preload` in the same scope
+      # Rails 6 cannot mix `eager_load` and `preload` in the same scope
+      # Rails 6 and 7 cannot mix `eager_load` and `includes` in the same scope
+      if Rails::VERSION::MAJOR >= 7
+        result = result.preload(instance_dependent_associations)
+      end
+
+      result
+    end
+
+    def instance_dependent_associations(resource)
+      @includes.select do |association_name|
+        resource.reflect_on_association(association_name)&.scope&.arity&.positive?
+      end
+    end
   end
 end

app/services/forest_liana/has_many_getter.rb

@@ -64,12 +64,8 @@ def model_association
     end
 
     def prepare_query
-      @records = @search_query_builder.perform(
-        get_resource()
-          .find(@params[:id])
-          .send(@params[:association_name])
-          .eager_load(@includes)
-      )
+      association = get_resource().find(@params[:id]).send(@params[:association_name])
+      @records = optimize_record_loading(association, @search_query_builder.perform(association))
     end
 
     def offset

app/services/forest_liana/pie_stat_getter.rb

@@ -5,7 +5,7 @@ class PieStatGetter < StatGetter
     def perform
       if @params[:group_by_field]
         timezone_offset = @params[:timezone].to_i
-        resource = get_resource().eager_load(@includes)
+        resource = optimize_record_loading(@resource, get_resource)
 
         filters = ForestLiana::ScopeManager.append_scope_for_user(@params[:filters], @user, @resource.name)
 

app/services/forest_liana/resource_getter.rb

@@ -12,7 +12,7 @@ def initialize(resource, params, forest_user)
     end
 
     def perform
-      records = get_resource().eager_load(@includes)
+      records = optimize_record_loading(@resource, get_resource())
       scoped_records = ForestLiana::ScopeManager.apply_scopes_on_records(records, @user, @collection_name, @params[:timezone])
       @record = scoped_records.find(@params[:id])
     end

app/services/forest_liana/resources_getter.rb

@@ -66,13 +66,13 @@ def self.get_ids_from_request(params, user)
     end
 
     def perform
-      @records = @records.eager_load(@includes)
+      @records = optimize_record_loading(@resource, @records)
     end
 
     def count
-      # NOTICE: For performance reasons, do not eager load the data if there is  no search or
+      # NOTICE: For performance reasons, do not optimize loading the data if there is  no search or
       #         filters on associations.
-      @records_count = @count_needs_includes ? @records.eager_load(@includes).count : @records.count
+      @records_count = @count_needs_includes ? optimize_record_loading(@resource, @records).count : @records.count
     end
 
     def query_for_batch

app/services/forest_liana/value_stat_getter.rb

@@ -4,7 +4,7 @@ class ValueStatGetter < StatGetter
 
     def perform
       return if @params[:aggregate].blank?
-      resource = get_resource().eager_load(@includes)
+      resource = optimize_record_loading(@resource, get_resource)
 
       filters = ForestLiana::ScopeManager.append_scope_for_user(@params[:filters], @user, @resource.name)
 

spec/dummy/app/models/island.rb

@@ -3,4 +3,5 @@ class Island < ActiveRecord::Base
 
   has_many :trees
   has_one :location
+  has_one :eponymous_tree, ->(record) { where(name: record.name) }, class_name: 'Tree'
 end

spec/dummy/app/models/tree.rb

@@ -2,4 +2,9 @@ class Tree < ActiveRecord::Base
   belongs_to :owner, class_name: 'User', inverse_of: :trees_owned
   belongs_to :cutter, class_name: 'User', inverse_of: :trees_cut
   belongs_to :island
+  belongs_to :eponymous_island,
+    ->(record) { where(name: record.name) },
+    class_name: 'Island',
+    inverse_of: :eponymous_tree,
+    optional: true
 end

spec/helpers/forest_liana/query_helper_spec.rb

@@ -14,29 +14,35 @@ module ForestLiana
         end
       end
 
-      context 'on a model having 3 belongsTo associations' do
+      context 'on a model having some belongsTo associations' do
+        let(:expected_association_attributes) do
+          [
+            { name: :owner, klass: User },
+            { name: :cutter, klass: User },
+            { name: :island, klass: Island },
+            { name: :eponymous_island, klass: Island },
+          ]
+        end
+
         it 'should return the one-one associations' do
           associations = QueryHelper.get_one_associations(Tree)
-          expect(associations.length).to eq(3)
-          expect(associations.first.name).to eq(:owner)
-          expect(associations.first.klass).to eq(User)
-          expect(associations.second.name).to eq(:cutter)
-          expect(associations.second.klass).to eq(User)
-          expect(associations.third.name).to eq(:island)
-          expect(associations.third.klass).to eq(Island)
+          expect(associations.length).to eq(expected_association_attributes.length)
+          associations.zip(expected_association_attributes).each do |association, expected_attributes|
+            expect(association).to have_attributes(expected_attributes)
+          end
         end
       end
     end
 
     describe 'get_one_association_names_symbol' do
       it 'should return the one-one associations names as symbols' do
-        expect(QueryHelper.get_one_association_names_symbol(Tree)).to eq([:owner, :cutter, :island])
+        expect(QueryHelper.get_one_association_names_symbol(Tree)).to eq([:owner, :cutter, :island, :eponymous_island])
       end
     end
 
     describe 'get_one_association_names_string' do
       it 'should return the one-one associations names as strings' do
-        expect(QueryHelper.get_one_association_names_string(Tree)).to eq(['owner', 'cutter', 'island'])
+        expect(QueryHelper.get_one_association_names_string(Tree)).to eq(['owner', 'cutter', 'island', 'eponymous_island'])
       end
     end
 
@@ -64,8 +70,9 @@ module ForestLiana
         end
 
         it 'should return relationships on models having a custom table name' do
-          expect(tables_associated_to_relations_name['isle'].length).to eq(1)
+          expect(tables_associated_to_relations_name['isle'].length).to eq(2)
           expect(tables_associated_to_relations_name['isle'].first).to eq(:island)
+          expect(tables_associated_to_relations_name['isle'].second).to eq(:eponymous_island)
         end
       end
     end

spec/services/forest_liana/resources_getter_spec.rb

@@ -155,6 +155,21 @@ def init_scopes
       end
     end
 
+    describe 'when getting instance dependent associations' do
+      let(:resource) { Island }
+      let(:fields) { { 'Island' => 'id,eponymous_tree', 'eponymous_tree' => 'id,name'} }
+
+      it 'should get only the expected records' do
+        getter.perform
+        records = getter.records
+        count = getter.count
+
+        expect(records.count).to eq Island.count
+        expect(count).to eq Island.count
+        expect(records.map(&:name)).to match_array(Island.pluck(:name))
+      end
+    end
+
     describe 'when filtering on an ambiguous field' do
       let(:resource) { Tree }
       let(:pageSize) { 5 }