fix: rails 8 compatibility for stat aggregators (#741)

Author: matthv

app/services/forest_liana/aggregation_helper.rb

@@ -0,0 +1,42 @@
+module ForestLiana
+  module AggregationHelper
+    def resolve_field_path(field_param, default_field = 'id')
+      if field_param.blank?
+        default_field ||= @resource.primary_key || 'id'
+        return "#{@resource.table_name}.#{default_field}"
+      end
+
+      if field_param.include?(':')
+        association, field = field_param.split ':'
+        associated_resource = @resource.reflect_on_association(association.to_sym)
+        "#{associated_resource.table_name}.#{field}"
+      else
+        "#{@resource.table_name}.#{field_param}"
+      end
+    end
+
+    def aggregation_sql(type, field)
+      field_path = resolve_field_path(field)
+
+      case type
+      when 'sum'
+        "SUM(#{field_path})"
+      when 'count'
+        "COUNT(DISTINCT #{field_path})"
+      else
+        raise "Unsupported aggregator : #{type}"
+      end
+    end
+
+    def aggregation_alias(type, field)
+      case type
+      when 'sum'
+        "sum_#{field.downcase}"
+      when 'count'
+        'count_id'
+      else
+        raise "Unsupported aggregator : #{type}"
+      end
+    end
+  end
+end

app/services/forest_liana/leaderboard_stat_getter.rb

@@ -1,6 +1,9 @@
 module ForestLiana
   class LeaderboardStatGetter < StatGetter
+    include AggregationHelper
+
     def initialize(parent_model, params, forest_user)
+      @resource = parent_model
       @scoped_parent_model = get_scoped_model(parent_model, forest_user, params[:timezone])
       child_model = @scoped_parent_model.reflect_on_association(params[:relationshipFieldName]).klass
       @scoped_child_model = get_scoped_model(child_model, forest_user, params[:timezone])
@@ -14,13 +17,15 @@ def initialize(parent_model, params, forest_user)
     def perform
       includes = ForestLiana::QueryHelper.get_one_association_names_symbol(@scoped_child_model)
 
+      alias_name = aggregation_alias(@aggregate, @aggregate_field)
+
       result = @scoped_child_model
         .joins(includes)
         .where({ @scoped_parent_model.name.downcase.to_sym => @scoped_parent_model })
         .group(@group_by)
-        .order(order)
+        .order(Arel.sql("#{alias_name} DESC"))
         .limit(@limit)
-        .send(@aggregate, @aggregate_field)
+        .pluck(@group_by, Arel.sql("#{aggregation_sql(@aggregate, @aggregate_field)} AS #{alias_name}"))
         .map { |key, value| { key: key, value: value } }
 
       @record = Model::Stat.new(value: result)
@@ -33,18 +38,5 @@ def get_scoped_model(model, forest_user, timezone)
 
       FiltersParser.new(scope_filters, model, timezone, @params).apply_filters
     end
-
-    def order
-      order = 'DESC'
-
-      # NOTICE: The generated alias for a count is "count_all", for a sum the
-      #         alias looks like "sum_#{aggregate_field}"
-      if @aggregate == 'sum'
-        field = @aggregate_field.downcase
-      else
-        field = 'all'
-      end
-      "#{@aggregate}_#{field} #{order}"
-    end
   end
 end

app/services/forest_liana/pie_stat_getter.rb

@@ -1,5 +1,6 @@
 module ForestLiana
   class PieStatGetter < StatGetter
+    include AggregationHelper
     attr_accessor :record
 
     def perform
@@ -13,11 +14,16 @@ def perform
           resource = FiltersParser.new(filters, resource, @params[:timezone], @params).apply_filters
         end
 
-        result = resource
-          .group(groupByFieldName)
-          .order(order)
-          .send(@params[:aggregator].downcase, @params[:aggregateFieldName])
-          .map do |key, value|
+        aggregation_type = @params[:aggregator].downcase
+        aggregation_field = @params[:aggregateFieldName]
+        alias_name = aggregation_alias(aggregation_type, aggregation_field)
+
+        resource = resource
+                     .group(groupByFieldName)
+                     .order(Arel.sql("#{alias_name} DESC"))
+                     .pluck(groupByFieldName, Arel.sql("#{aggregation_sql(aggregation_type, aggregation_field)} AS #{alias_name}"))
+
+        result = resource.map do |key, value|
             # NOTICE: Display the enum name instead of an integer if it is an
             #         "Enum" field type on old Rails version (before Rails
             #         5.1.3).
@@ -38,28 +44,7 @@ def perform
     end
 
     def groupByFieldName
-      if @params[:groupByFieldName].include? ':'
-        association, field = @params[:groupByFieldName].split ':'
-        resource = @resource.reflect_on_association(association.to_sym)
-        "#{resource.table_name}.#{field}"
-      else
-        "#{@resource.table_name}.#{@params[:groupByFieldName]}"
-      end
-    end
-
-    def order
-      order = 'DESC'
-
-      # NOTICE: The generated alias for a count is "count_all", for a sum the
-      #         alias looks like "sum_#{aggregateFieldName}"
-      if @params[:aggregator].downcase == 'sum'
-        field = @params[:aggregateFieldName].downcase
-      else
-        # `count_id` is required only for rails v5
-        field = Rails::VERSION::MAJOR == 5 || @includes.size > 0 ? 'id' : 'all'
-      end
-      "#{@params[:aggregator].downcase}_#{field} #{order}"
+      resolve_field_path(@params[:groupByFieldName])
     end
-
   end
 end

spec/services/forest_liana/pie_stat_getter_spec.rb

@@ -133,5 +133,187 @@ module ForestLiana
         end
       end
     end
+
+    describe 'aggregation methods behavior' do
+      let(:scopes) { {'scopes' => {}, 'team' => {'id' => '1', 'name' => 'Operations'}} }
+      let(:model) { Tree }
+      let(:collection) { 'trees' }
+      let(:groupByFieldName) { 'age' }
+
+      describe 'aggregation_sql method' do
+        subject { PieStatGetter.new(model, params, user) }
+
+        context 'with COUNT aggregator' do
+          let(:params) {
+            {
+              type: 'Pie',
+              sourceCollectionName: collection,
+              timezone: 'Europe/Paris',
+              aggregator: 'Count',
+              groupByFieldName: groupByFieldName
+            }
+          }
+
+          it 'should generate correct COUNT SQL' do
+            sql = subject.send(:aggregation_sql, 'count', nil)
+            expect(sql).to eq 'COUNT(DISTINCT trees.id)'
+          end
+
+          it 'should generate correct COUNT SQL with specific field' do
+            sql = subject.send(:aggregation_sql, 'count', 'age')
+            expect(sql).to eq 'COUNT(DISTINCT trees.age)'
+          end
+        end
+
+        context 'with SUM aggregator' do
+          let(:params) {
+            {
+              type: 'Pie',
+              sourceCollectionName: collection,
+              timezone: 'Europe/Paris',
+              aggregator: 'Sum',
+              aggregateFieldName: 'age',
+              groupByFieldName: groupByFieldName
+            }
+          }
+
+          it 'should generate correct SUM SQL' do
+            sql = subject.send(:aggregation_sql, 'sum', 'age')
+            expect(sql).to eq 'SUM(trees.age)'
+          end
+        end
+
+        context 'with association field' do
+          let(:params) {
+            {
+              type: 'Pie',
+              sourceCollectionName: collection,
+              timezone: 'Europe/Paris',
+              aggregator: 'Count',
+              groupByFieldName: 'owner:name'
+            }
+          }
+
+          it 'should handle association fields correctly' do
+            # Assuming Tree belongs_to :owner
+            allow(model).to receive(:reflect_on_association).with(:owner).and_return(
+              double(table_name: 'owners')
+            )
+
+            sql = subject.send(:aggregation_sql, 'count', 'owner:id')
+            expect(sql).to eq 'COUNT(DISTINCT owners.id)'
+          end
+        end
+
+        context 'with unsupported aggregator' do
+          let(:params) {
+            {
+              type: 'Pie',
+              sourceCollectionName: collection,
+              timezone: 'Europe/Paris',
+              aggregator: 'Invalid',
+              groupByFieldName: groupByFieldName
+            }
+          }
+
+          it 'should raise an error for unsupported aggregator' do
+            expect {
+              subject.send(:aggregation_sql, 'invalid', 'age')
+            }.to raise_error(ForestLiana::Errors::HTTP422Error)
+          end
+        end
+      end
+
+      describe 'aggregation_alias method' do
+        subject { PieStatGetter.new(model, params, user) }
+
+        context 'with COUNT aggregator' do
+          let(:params) {
+            {
+              type: 'Pie',
+              sourceCollectionName: collection,
+              timezone: 'Europe/Paris',
+              aggregator: 'Count',
+              groupByFieldName: groupByFieldName
+            }
+          }
+
+          it 'should return correct alias for count' do
+            alias_name = subject.send(:aggregation_alias, 'count', nil)
+            expect(alias_name).to eq 'count_id'
+          end
+        end
+
+        context 'with SUM aggregator' do
+          let(:params) {
+            {
+              type: 'Pie',
+              sourceCollectionName: collection,
+              timezone: 'Europe/Paris',
+              aggregator: 'Sum',
+              aggregateFieldName: 'age',
+              groupByFieldName: groupByFieldName
+            }
+          }
+
+          it 'should return correct alias for sum' do
+            alias_name = subject.send(:aggregation_alias, 'sum', 'age')
+            expect(alias_name).to eq 'sum_age'
+          end
+
+          it 'should handle field names with mixed case' do
+            alias_name = subject.send(:aggregation_alias, 'sum', 'TreeAge')
+            expect(alias_name).to eq 'sum_treeage'
+          end
+        end
+      end
+
+      describe 'results ordering' do
+        subject { PieStatGetter.new(model, params, user) }
+
+        context 'with COUNT aggregator' do
+          let(:params) {
+            {
+              type: 'Pie',
+              sourceCollectionName: collection,
+              timezone: 'Europe/Paris',
+              aggregator: 'Count',
+              groupByFieldName: groupByFieldName
+            }
+          }
+
+          it 'should return results ordered by count descending' do
+            subject.perform
+
+            expect(subject.record.value).to eq [
+                                                 { :key => 3, :value => 5},
+                                                 { :key => 15, :value => 4 }
+                                               ]
+          end
+        end
+
+        context 'with SUM aggregator' do
+          let(:params) {
+            {
+              type: 'Pie',
+              sourceCollectionName: collection,
+              timezone: 'Europe/Paris',
+              aggregator: 'Sum',
+              aggregateFieldName: 'age',
+              groupByFieldName: groupByFieldName
+            }
+          }
+
+          it 'should return results ordered by sum descending' do
+            subject.perform
+
+            expect(subject.record.value).to eq [
+                                                 { :key => 15, :value => 60 },
+                                                 { :key => 3, :value => 15 }
+                                               ]
+          end
+        end
+      end
+    end
   end
 end