prevent n+1 cost queries

app/models/cost_entry.rb

@@ -38,59 +38,46 @@ class CostEntry < ActiveRecord::Base
   validate :validate
 
   scope :visible, -> (*args) {
-    user = args.first || User.current
-    project = args[1]
+    with_visible_entries_on self, user: args.first, project: args[1]
+  }
+
+  scope :visible_costs, -> (*args) {
+    with_visible_costs_on self, user: args.first, project: args[1]
+  }
 
+  def self.with_visible_costs_on(scope, user: User.current, project: nil)
+    with_visible_entries = with_visible_entries_on(scope, user: user, project: project)
+    with_visible_rates_on with_visible_entries, user: user
+  end
+
+  def self.with_visible_entries_on(scope, user: User.current, project: nil)
     table = self.arel_table
 
     view_allowed = Project.allowed_to(user, :view_cost_entries).select(:id)
     view_own_allowed = Project.allowed_to(user, :view_own_cost_entries).select(:id)
-
-    view_or_view_own = table[:project_id]
-                       .in(view_allowed.arel)
-                       .or(table[:project_id]
-                           .in(view_own_allowed.arel)
-                           .and(table[:user_id].eq(user.id)))
-
-    scope = where(view_or_view_own)
+    visible_scope = scope.where view_or_view_own(table, view_allowed, view_own_allowed, user)
 
     if project
-      scope = scope.where(project_id: project.id)
+      visible_scope.where(project_id: project.id)
+    else
+      visible_scope
     end
+  end
 
-    scope
-  }
-
-  scope :visible_costs, lambda{|*args|
-    user = args.first || User.current
-    project = args[1]
+  def self.view_or_view_own(table, view_allowed, view_own_allowed, user)
+    table[:project_id]
+      .in(view_allowed.arel)
+      .or(
+        table[:project_id]
+          .in(view_own_allowed.arel)
+          .and(table[:user_id].eq(user.id)))
+  end
 
+  def self.with_visible_rates_on(scope, user: User.current)
     table = self.arel_table
-
     view_allowed = Project.allowed_to(user, :view_cost_rates).select(:id)
 
-    visible(user, project).where(table[:project_id].in(view_allowed.arel))
-  }
-
-  scope :on_work_packages, ->(work_packages) { where(work_package_id: work_packages) }
-
-  def self.costs_of(work_packages:)
-    # N.B. Because of an AR quirks the code below uses statements like
-    #   where(work_package_id: ids)
-    # You would expect to be able to simply write those as
-    #   where(work_package: work_packages)
-    # However, AR (Rails 4.2) will not expand :includes + :references inside a subquery,
-    # which will render the query invalid. Therefore we manually extract the IDs in a separate (pluck) query.
-    ids = if work_packages.respond_to?(:pluck)
-            work_packages.pluck(:id)
-          else
-            Array(work_packages).map { |wp| wp.id }
-          end
-    CostEntry.where(work_package_id: ids)
-      .joins(work_package: :project)
-      .visible_costs
-      .sum("COALESCE(#{CostEntry.table_name}.overridden_costs,
-                     #{CostEntry.table_name}.costs)").to_f
+    scope.where(table[:project_id].in(view_allowed.arel))
   end
 
   def after_initialize

app/models/work_package/abstract_costs.rb

@@ -0,0 +1,124 @@
+class WorkPackage
+  class AbstractCosts
+    attr_reader :user
+    attr_reader :project
+
+    def initialize(user: User.current, project: nil)
+      @user = user
+      @project = project
+    end
+
+    ##
+    # Adds to the given WorkPackage query's result an extra costs column.
+    #
+    # @param work_package_scope [WorkPackage::ActiveRecord_Relation]
+    # @return [WorkPackage::ActiveRecord_Relation] The query with the joined costs.
+    def add_to_work_packages(work_package_scope)
+      add_costs_to work_package_scope
+    end
+
+    ##
+    # For the given work packages calculates the sum of all costs.
+    #
+    # @param [WorkPackage::ActiveRecord_Relation | Array[WorkPackage]] List of work packages.
+    # @return [Float] The sum of the work packages' costs.
+    def costs_of(work_packages:)
+      # N.B. Because of an AR quirks the code below uses statements like
+      #   where(work_package_id: ids)
+      # You would expect to be able to simply write those as
+      #   where(work_package: work_packages)
+      # However, AR (Rails 4.2) will not expand :includes + :references inside a subquery,
+      # which will render the query invalid. Therefore we manually extract the IDs in a separate (pluck) query.
+      wp_ids = work_package_ids work_packages
+
+      filter_authorized(costs_model.where(work_package_id: wp_ids).joins(work_package: :project))
+        .sum(costs_value)
+        .to_f
+    end
+
+    ##
+    # The model on which the costs calculations are based.
+    # Can be any model which has the fields `overridden_costs` and `costs`
+    # and is related to work packages (i.e. has a `work_package_id` too).
+    #
+    # @return [Class] Class of the model the costs are based on, e.g. CostEntry or TimeEntry.
+    def costs_model
+      raise NotImplementedError, "subclass responsiblity"
+    end
+
+    private
+
+    def work_package_ids(work_packages)
+      if work_packages.respond_to?(:pluck)
+        work_packages.pluck(:id)
+      else
+        Array(work_packages).map { |wp| wp.id }
+      end
+    end
+
+    def costs_table_name
+      costs_model.table_name
+    end
+
+    def table_alias
+      "#{costs_table_name}_sum_per_wp"
+    end
+
+    def add_costs_to(scope)
+      query = costs.to_sql
+
+      scope
+        .joins(
+          "LEFT JOIN (#{query}) as #{table_alias} ON #{table_alias}.wp_id = #{wp_table.name}.id")
+        .select("#{table_alias}.#{costs_table_name}_sum")
+    end
+
+    def costs
+      filter_authorized all_costs
+    end
+
+    def all_costs
+      WorkPackage
+        .joins(sum_arel.join_sources)
+        .select("work_packages.id as wp_id, #{costs_sum} as #{costs_table_name}_sum")
+        .group(wp_table[:id])
+    end
+
+    def costs_sum
+      "SUM(#{costs_value})"
+    end
+
+    def costs_value
+      "COALESCE(#{costs_table_name}.overridden_costs, #{costs_table_name}.costs)"
+    end
+
+    ##
+    # Narrows down the query to only include costs visible to the user.
+    #
+    # @param [ActiveRecord::QueryMethods] Some query.
+    # @return [ActiveRecord::QueryMethods] The filtered query.
+    def filter_authorized(scope)
+      scope # allow all
+    end
+
+    def sum_arel
+      wp_table
+        .from(wp_table)
+        .join(ce_table, Arel::Nodes::OuterJoin).on(ce_table[:work_package_id].eq(wp_table[:id]))
+        .join(projects_table).on(projects_table[:id].eq(wp_table[:project_id]))
+        .group(wp_table[:id])
+    end
+
+    def wp_table
+      WorkPackage.arel_table
+    end
+
+    def ce_table
+      costs_model.arel_table
+    end
+
+    def projects_table
+      Project.arel_table
+    end
+  end
+end

app/models/work_package/labor_costs.rb

@@ -0,0 +1,11 @@
+class WorkPackage
+  class LaborCosts < AbstractCosts
+    def costs_model
+      TimeEntry
+    end
+
+    def filter_authorized(scope)
+      TimeEntry.with_visible_costs_on scope
+    end
+  end
+end

app/models/work_package/material_costs.rb

@@ -0,0 +1,11 @@
+class WorkPackage
+  class MaterialCosts < AbstractCosts
+    def costs_model
+      CostEntry
+    end
+
+    def filter_authorized(scope)
+      CostEntry.with_visible_costs_on scope
+    end
+  end
+end

lib/open_project/costs/engine.rb

@@ -351,6 +351,15 @@ module OpenProject::Costs
                                                attribute: :updated_on
     end
 
+    module EagerLoadedCosts
+      def eager_loaded_work_packages(ids)
+        material = WorkPackage::MaterialCosts.new
+        labor = WorkPackage::LaborCosts.new
+
+        material.add_to_work_packages(labor.add_to_work_packages(super))
+      end
+    end
+
     config.to_prepare do
       require 'open_project/costs/patches/members_patch'
       OpenProject::Costs::Members.mixin!
@@ -369,6 +378,8 @@ module OpenProject::Costs
                                                                       { cost_entries: [:project,
                                                                                        :user] },
                                                                       :cost_object]
+
+      API::V3::WorkPackages::WorkPackageCollectionRepresenter.prepend EagerLoadedCosts
     end
   end
 end

lib/open_project/costs/patches/query_patch.rb

@@ -62,20 +62,30 @@ module OpenProject::Costs::Patches::QueryPatch
       add_available_column(CurrencyQueryColumn.new(
                              :material_costs,
                              summable: -> (work_packages) {
-                               CostEntry.costs_of(work_packages: work_packages)
+                               WorkPackage::MaterialCosts
+                                 .new(user: User.current)
+                                 .costs_of(work_packages: work_packages)
                              }))
 
       add_available_column(CurrencyQueryColumn.new(
                              :labor_costs,
                              summable: -> (work_packages) {
-                               TimeEntry.costs_of(work_packages: work_packages)
+                               WorkPackage::LaborCosts
+                                 .new(user: User.current)
+                                 .costs_of(work_packages: work_packages)
                              }))
 
       add_available_column(CurrencyQueryColumn.new(
                              :overall_costs,
                              summable: -> (work_packages) {
-                               labor_costs = TimeEntry.costs_of(work_packages: work_packages)
-                               material_costs = CostEntry.costs_of(work_packages: work_packages)
+                               labor_costs = WorkPackage::LaborCosts
+                                 .new(user: User.current)
+                                 .costs_of(work_packages: work_packages)
+
+                               material_costs = WorkPackage::MaterialCosts
+                                 .new(user: User.current)
+                                 .costs_of(work_packages: work_packages)
+
                                labor_costs + material_costs
                              }))
 

lib/open_project/costs/patches/time_entry_patch.rb

@@ -31,64 +31,48 @@ module OpenProject::Costs::Patches::TimeEntryPatch
       before_save :update_costs
 
       scope :visible, -> (*args) {
-        user = args.first || User.current
-        project = args[1]
+        with_visible_entries_on self, user: args.first, project: args[1]
+      }
+
+      scope :visible_costs, -> (*args) {
+        with_visible_costs_on self, user: args.first, project: args[1]
+      }
 
+      def self.with_visible_costs_on(scope, user: User.current, project: nil)
+        with_visible_entries = with_visible_entries_on(scope, user: user, project: project)
+        with_visible_rates_on with_visible_entries, user: user
+      end
+
+      def self.with_visible_entries_on(scope, user: User.current, project: nil)
         table = self.arel_table
 
         view_allowed = Project.allowed_to(user, :view_time_entries).select(:id)
         view_own_allowed = Project.allowed_to(user, :view_own_time_entries).select(:id)
-
-        view_or_view_own = table[:project_id]
-                           .in(view_allowed.arel)
-                           .or(table[:project_id]
-                               .in(view_own_allowed.arel)
-                               .and(table[:user_id].eq(user.id)))
-
-        scope = where(view_or_view_own)
+        visible_scope = scope.where view_or_view_own(table, view_allowed, view_own_allowed, user)
 
         if project
-          scope = scope.where(project_id: project.id)
+          visible_scope.where(project_id: project.id)
+        else
+          visible_scope
         end
+      end
 
-        scope
-      }
-
-      scope :visible_costs, lambda{|*args|
-        user = args.first || User.current
-        project = args[1]
-
+      def self.with_visible_rates_on(scope, user: User.current)
         table = self.arel_table
 
         view_allowed = Project.allowed_to(user, :view_hourly_rates).select(:id)
         view_own_allowed = Project.allowed_to(user, :view_own_hourly_rates).select(:id)
 
-        view_or_view_own = table[:project_id]
-                           .in(view_allowed.arel)
-                           .or(table[:project_id]
-                               .in(view_own_allowed.arel)
-                               .and(table[:user_id].eq(user.id)))
-
-        visible(user, project).where(view_or_view_own)
-      }
+        scope.where view_or_view_own(table, view_allowed, view_own_allowed, user)
+      end
 
-      def self.costs_of(work_packages:)
-        # N.B. Because of an AR quirks the code below uses statements like
-        #   where(work_package_id: ids)
-        # You would expect to be able to simply write those as
-        #   where(work_package: work_packages)
-        # However, AR (Rails 4.2) will not expand :includes + :references inside a subquery,
-        # which will render the query invalid. Therefore we manually extract the IDs in a separate (pluck) query.
-        ids = if work_packages.respond_to?(:pluck)
-                work_packages.pluck(:id)
-              else
-                Array(work_packages).map { |wp| wp.id }
-              end
-        TimeEntry.where(work_package_id: ids)
-          .joins(work_package: :project)
-          .visible_costs
-          .sum("COALESCE(#{TimeEntry.table_name}.overridden_costs,
-                         #{TimeEntry.table_name}.costs)").to_f
+      def self.view_or_view_own(table, view_allowed, view_own_allowed, user)
+        table[:project_id]
+          .in(view_allowed.arel)
+          .or(
+            table[:project_id]
+              .in(view_own_allowed.arel)
+              .and(table[:user_id].eq(user.id)))
       end
     end
   end

lib/open_project/costs/patches/work_package_patch.rb

@@ -111,11 +111,19 @@ module OpenProject::Costs::Patches::WorkPackagePatch
     end
 
     def material_costs
-      CostEntry.costs_of(work_packages: self)
+      if respond_to?(:cost_entries_sum) # column has been eager loaded into result set
+        cost_entries_sum.to_f
+      else
+        WorkPackage::MaterialCosts.new(user: User.current).costs_of work_packages: self
+      end
     end
 
     def labor_costs
-      TimeEntry.costs_of(work_packages: self)
+      if respond_to?(:time_entries_sum) # column has been eager loaded into result set
+        time_entries_sum.to_f
+      else
+        WorkPackage::LaborCosts.new(user: User.current).costs_of work_packages: self
+      end
     end
 
     def overall_costs