From 693aff4b8d5c92e4888d52cc196867ae10cd07bb Mon Sep 17 00:00:00 2001 From: Markus Kahl Date: Thu, 8 Sep 2016 23:07:03 +0100 Subject: [PATCH] prevent n+1 cost queries --- app/models/cost_entry.rb | 67 ++++------ app/models/work_package/abstract_costs.rb | 124 ++++++++++++++++++ app/models/work_package/labor_costs.rb | 11 ++ app/models/work_package/material_costs.rb | 11 ++ lib/open_project/costs/engine.rb | 11 ++ lib/open_project/costs/patches/query_patch.rb | 18 ++- .../costs/patches/time_entry_patch.rb | 70 ++++------ .../costs/patches/work_package_patch.rb | 12 +- 8 files changed, 235 insertions(+), 89 deletions(-) create mode 100644 app/models/work_package/abstract_costs.rb create mode 100644 app/models/work_package/labor_costs.rb create mode 100644 app/models/work_package/material_costs.rb diff --git a/app/models/cost_entry.rb b/app/models/cost_entry.rb index ca6d0d3c51..9e325469c0 100644 --- a/app/models/cost_entry.rb +++ b/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 diff --git a/app/models/work_package/abstract_costs.rb b/app/models/work_package/abstract_costs.rb new file mode 100644 index 0000000000..9817684acf --- /dev/null +++ b/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 diff --git a/app/models/work_package/labor_costs.rb b/app/models/work_package/labor_costs.rb new file mode 100644 index 0000000000..82d91d95bf --- /dev/null +++ b/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 diff --git a/app/models/work_package/material_costs.rb b/app/models/work_package/material_costs.rb new file mode 100644 index 0000000000..3cadbb9a43 --- /dev/null +++ b/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 diff --git a/lib/open_project/costs/engine.rb b/lib/open_project/costs/engine.rb index ddd7d7b1f2..893885eb4b 100644 --- a/lib/open_project/costs/engine.rb +++ b/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 diff --git a/lib/open_project/costs/patches/query_patch.rb b/lib/open_project/costs/patches/query_patch.rb index ab9f5b3c10..718eef2615 100644 --- a/lib/open_project/costs/patches/query_patch.rb +++ b/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 })) diff --git a/lib/open_project/costs/patches/time_entry_patch.rb b/lib/open_project/costs/patches/time_entry_patch.rb index 21526ff3ce..2269ba7a47 100644 --- a/lib/open_project/costs/patches/time_entry_patch.rb +++ b/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 diff --git a/lib/open_project/costs/patches/work_package_patch.rb b/lib/open_project/costs/patches/work_package_patch.rb index 621ba3efa6..0a8f8fd920 100644 --- a/lib/open_project/costs/patches/work_package_patch.rb +++ b/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