From cb5eb4d2ddbb14a5c462bd228477d0bf31188b55 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Wed, 22 Jan 2020 00:11:59 +1300 Subject: [PATCH 1/9] Bulk epic issues loader Loads epic tree into a data structure with epic id, iid, issue count and parent_id Add lazy resolver Connect CTE to a lazy resolver, which does a bulk query for all the epic aggregate (weight and count) rather than get a separate query for each. This also prevents us from having to get information we already have since one requested epic might be a child epic of another. Connect aggregate results with GraphQL type - Use a PORO to represent the data via method exposure Put together the recursive logic to sum the counts/weights while separating them by graphql facet (count or weight_sum). --- app/graphql/gitlab_schema.rb | 2 + app/models/issue.rb | 2 - doc/api/graphql/reference/gitlab_schema.json | 55 +++++ ee/app/graphql/epics/aggregate.rb | 24 ++ ee/app/graphql/epics/aggregate_constants.rb | 17 ++ ee/app/graphql/epics/count_aggregate.rb | 20 ++ ee/app/graphql/epics/epic_node.rb | 91 ++++++++ ee/app/graphql/epics/lazy_epic_aggregate.rb | 104 +++++++++ ee/app/graphql/epics/sum_total.rb | 23 ++ ee/app/graphql/epics/weight_sum_aggregate.rb | 18 ++ ee/app/graphql/types/epic_type.rb | 33 +-- ee/app/models/ee/epic.rb | 2 - .../epics/bulk_epic_aggregate_loader.rb | 48 ++++ ee/spec/graphql/epics/aggregate_spec.rb | 81 +++++++ ee/spec/graphql/epics/epic_node_spec.rb | 211 ++++++++++++++++++ .../graphql/epics/lazy_epic_aggregate_spec.rb | 143 ++++++++++++ .../group/epic/epic_aggregate_query_spec.rb | 109 ++++++--- .../epics/bulk_epic_aggregate_loader_spec.rb | 137 ++++++++++++ .../matchers/ee/epic_aggregate_matchers.rb | 51 +++++ spec/spec_helper.rb | 4 + 20 files changed, 1122 insertions(+), 53 deletions(-) create mode 100644 ee/app/graphql/epics/aggregate.rb create mode 100644 ee/app/graphql/epics/aggregate_constants.rb create mode 100644 ee/app/graphql/epics/count_aggregate.rb create mode 100644 ee/app/graphql/epics/epic_node.rb create mode 100644 ee/app/graphql/epics/lazy_epic_aggregate.rb create mode 100644 ee/app/graphql/epics/sum_total.rb create mode 100644 ee/app/graphql/epics/weight_sum_aggregate.rb create mode 100644 ee/app/services/epics/bulk_epic_aggregate_loader.rb create mode 100644 ee/spec/graphql/epics/aggregate_spec.rb create mode 100644 ee/spec/graphql/epics/epic_node_spec.rb create mode 100644 ee/spec/graphql/epics/lazy_epic_aggregate_spec.rb create mode 100644 ee/spec/services/epics/bulk_epic_aggregate_loader_spec.rb create mode 100644 ee/spec/support/matchers/ee/epic_aggregate_matchers.rb diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index ea5776534d5c5b..b1cd4fd748c616 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -28,6 +28,8 @@ class GitlabSchema < GraphQL::Schema default_max_page_size 100 + lazy_resolve Epics::LazyEpicAggregate, :epic_aggregate + class << self def multiplex(queries, **kwargs) kwargs[:max_complexity] ||= max_query_complexity(kwargs[:context]) diff --git a/app/models/issue.rb b/app/models/issue.rb index d3f597c0bda0b6..aab5c832df9db2 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -74,8 +74,6 @@ class Issue < ApplicationRecord scope :public_only, -> { where(confidential: false) } scope :confidential_only, -> { where(confidential: true) } - scope :counts_by_state, -> { reorder(nil).group(:state_id).count } - ignore_column :state, remove_with: '12.7', remove_after: '2019-12-22' after_commit :expire_etag_cache, unless: :importing? diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index 4d52f45d3e4beb..54491b8720c884 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -4987,6 +4987,20 @@ "isDeprecated": false, "deprecationReason": null }, + { + "name": "descendantWeightSum", + "description": "Total weight of open and closed descendant epic's issues", + "args": [ + + ], + "type": { + "kind": "OBJECT", + "name": "EpicDescendantWeights", + "ofType": null + }, + "isDeprecated": false, + "deprecationReason": null + }, { "name": "description", "description": "Description of the epic", @@ -13084,6 +13098,47 @@ "enumValues": null, "possibleTypes": null }, + { + "kind": "OBJECT", + "name": "EpicDescendantWeights", + "description": "Total weight of open and closed descendant issues", + "fields": [ + { + "name": "closedIssues", + "description": "Total weight of completed (closed) issues in this epic, including epic descendants", + "args": [ + + ], + "type": { + "kind": "SCALAR", + "name": "Int", + "ofType": null + }, + "isDeprecated": false, + "deprecationReason": null + }, + { + "name": "openedIssues", + "description": "Total weight of opened issues in this epic, including epic descendants", + "args": [ + + ], + "type": { + "kind": "SCALAR", + "name": "Int", + "ofType": null + }, + "isDeprecated": false, + "deprecationReason": null + } + ], + "inputFields": null, + "interfaces": [ + + ], + "enumValues": null, + "possibleTypes": null + }, { "kind": "OBJECT", "name": "TimelogConnection", diff --git a/ee/app/graphql/epics/aggregate.rb b/ee/app/graphql/epics/aggregate.rb new file mode 100644 index 00000000000000..6c2af81845af37 --- /dev/null +++ b/ee/app/graphql/epics/aggregate.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Epics + class Aggregate + include AggregateConstants + + def initialize(values) + raise NotImplementedError.new('Use either CountAggregate or WeightSumAggregate') + end + + private + + def sum_objects(state, type) + matching = @sums.select { |sum| sum.state == state && sum.type == type && sum.facet == facet} + return 0 if @sums.empty? + + matching.map(&:value).reduce(:+) || 0 + end + + def facet + raise NotImplementedError.new('Use either CountAggregate or WeightSumAggregate') + end + end +end diff --git a/ee/app/graphql/epics/aggregate_constants.rb b/ee/app/graphql/epics/aggregate_constants.rb new file mode 100644 index 00000000000000..739caadf5017db --- /dev/null +++ b/ee/app/graphql/epics/aggregate_constants.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Epics + module AggregateConstants + ISSUE_TYPE = :issue + EPIC_TYPE = :epic + + CLOSED_ISSUE_STATE = Issue.available_states[:closed].freeze + OPENED_ISSUE_STATE = Issue.available_states[:opened].freeze + + CLOSED_EPIC_STATE = Epic.available_states[:closed].freeze + OPENED_EPIC_STATE = Epic.available_states[:opened].freeze + + COUNT = :count + WEIGHT_SUM = :weight_sum + end +end diff --git a/ee/app/graphql/epics/count_aggregate.rb b/ee/app/graphql/epics/count_aggregate.rb new file mode 100644 index 00000000000000..1bec84f04cea60 --- /dev/null +++ b/ee/app/graphql/epics/count_aggregate.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module Epics + class CountAggregate < Aggregate + attr_accessor :opened_issues, :closed_issues, :opened_epics, :closed_epics + + def initialize(sums) + @sums = sums + + @opened_issues = sum_objects(OPENED_ISSUE_STATE, ISSUE_TYPE) + @closed_issues = sum_objects(CLOSED_ISSUE_STATE, ISSUE_TYPE) + @opened_epics = sum_objects(OPENED_EPIC_STATE, EPIC_TYPE) + @closed_epics = sum_objects(CLOSED_EPIC_STATE, EPIC_TYPE) + end + + def facet + COUNT + end + end +end diff --git a/ee/app/graphql/epics/epic_node.rb b/ee/app/graphql/epics/epic_node.rb new file mode 100644 index 00000000000000..ffd3316d2c8fae --- /dev/null +++ b/ee/app/graphql/epics/epic_node.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +# This class represents an Epic's aggregate information (added up counts) about its child epics and direct issues + +module Epics + class EpicNode + include AggregateConstants + + attr_reader :epic_id, :epic_state_id, :epic_info_flat_list, :parent_id, + :direct_sums, # we calculate these and recursively add them + :sum_total + + attr_accessor :child_ids + + def initialize(epic_id, flat_info_list) + # epic aggregate records from the DB loader look like the following: + # { 1 => [{iid: 1, epic_state_id: 1, issues_count: 1, issues_weight_sum: 2, parent_id: nil, state_id: 2}] ... } + # They include the sum of each epic's direct issues, grouped by status, + # so in order to get a sum of the entire tree, we have to add that up recursively + @epic_id = epic_id + @epic_info_flat_list = flat_info_list + @child_ids = [] + @direct_sums = [] + + set_epic_attributes(flat_info_list.first) # there will always be one + end + + def assemble_issue_sums + # this is a representation of the epic's + # direct child issues and epics that have come from the DB + [OPENED_ISSUE_STATE, CLOSED_ISSUE_STATE].each do |issue_state| + matching_issue_state_entry = epic_info_flat_list.find do |epic_info_node| + epic_info_node[:issues_state_id] == issue_state + end || {} + + create_sum_if_needed(WEIGHT_SUM, issue_state, ISSUE_TYPE, matching_issue_state_entry.fetch(:issues_weight_sum, 0)) + create_sum_if_needed(COUNT, issue_state, ISSUE_TYPE, matching_issue_state_entry.fetch(:issues_count, 0)) + end + end + + def assemble_epic_sums(children) + [OPENED_EPIC_STATE, CLOSED_EPIC_STATE].each do |epic_state| + create_sum_if_needed(COUNT, epic_state, EPIC_TYPE, children.select { |node| node.epic_state_id == epic_state }.count) + end + end + + def calculate_recursive_sums(tree) + return sum_total if sum_total + + @sum_total = SumTotal.new + child_ids.each do |child_id| + child = tree[child_id] + # get the child's totals, add to your own + child_sums = child.calculate_recursive_sums(tree).sums + sum_total.add(child_sums) + end + sum_total.add(direct_sums) + sum_total + end + + def aggregate_object_by(facet) + sum_total.by_facet(facet) + end + + def to_s + { epic_id: @epic_id, parent_id: @parent_id, direct_sums: direct_sums, child_ids: child_ids }.to_json + end + + alias_method :inspect, :to_s + alias_method :id, :epic_id + + private + + def create_sum_if_needed(facet, state, type, value) + return if value.nil? || value < 1 + + direct_sums << Sum.new(facet, state, type, value) + end + + def set_epic_attributes(record) + @epic_state_id = record[:epic_state_id] + @parent_id = record[:parent_id] + end + + Sum = Struct.new(:facet, :state, :type, :value) do + def inspect + "" + end + end + end +end diff --git a/ee/app/graphql/epics/lazy_epic_aggregate.rb b/ee/app/graphql/epics/lazy_epic_aggregate.rb new file mode 100644 index 00000000000000..088ba8b8c9843f --- /dev/null +++ b/ee/app/graphql/epics/lazy_epic_aggregate.rb @@ -0,0 +1,104 @@ +# frozen_string_literal: true + +module Epics + class LazyEpicAggregate + include AggregateConstants + + attr_reader :facet, :epic_id, :lazy_state + + # Because facets "count" and "weight_sum" share the same db query, but have a different graphql type object, + # we can separate them and serve only the fields which are requested by the GraphQL query + def initialize(query_ctx, epic_id, aggregate_facet) + @epic_id = epic_id + + raise ArgumentError.new("No aggregate facet provided. Please specify either #{COUNT} or #{WEIGHT_SUM}") unless aggregate_facet.present? + raise ArgumentError.new("Invalid aggregate facet #{aggregate_facet} provided. Please specify either #{COUNT} or #{WEIGHT_SUM}") unless [COUNT, WEIGHT_SUM].include?(aggregate_facet.to_sym) + + @facet = aggregate_facet.to_sym + + # Initialize the loading state for this query, + # or get the previously-initiated state + @lazy_state = query_ctx[:lazy_epic_aggregate] ||= { + pending_ids: Set.new, + tree: {} + } + # Register this ID to be loaded later: + @lazy_state[:pending_ids] << epic_id + end + + # Return the loaded record, hitting the database if needed + def epic_aggregate + # Check if the record was already loaded: + # load from tree by epic + loaded_epic_info_node = @lazy_state[:tree][@epic_id] + + if loaded_epic_info_node + # The pending IDs were already loaded, + # so return the result of that previous load + loaded_epic_info_node.aggregate_object_by(@facet) + else + load_records_into_tree + end + end + + private + + def tree + @lazy_state[:tree] + end + + def load_records_into_tree + # The record hasn't been loaded yet, so + # hit the database with all pending IDs + pending_ids = @lazy_state[:pending_ids].to_a + + # Fire off the db query and get the results (grouped by epic_id and facet) + raw_epic_aggregates = Epics::BulkEpicAggregateLoader.new(epic_ids: pending_ids).execute + + # Assemble the tree and sum everything + create_structure_from(raw_epic_aggregates) + + @lazy_state[:pending_ids].clear + + # Now, get the matching node and return its aggregate depending on the facet: + epic_node = @lazy_state[:tree][@epic_id] + epic_node.aggregate_object_by(@facet) + end + + def create_structure_from(aggregate_records) + # create EpicNode object for each epic id + aggregate_records.each do |epic_id, aggregates| + next if aggregates.nil? || aggregates.empty? + + new_node = EpicNode.new(epic_id, aggregates) + tree[epic_id] = new_node + end + + associate_parents_and_children + assemble_direct_sums + calculate_recursive_sums + end + + # each of the methods below are done one after the other + def associate_parents_and_children + tree.each do |epic_id, node| + node.child_ids = tree.select { |_, child_node| epic_id == child_node.parent_id }.keys + end + end + + def assemble_direct_sums + tree.each do |_, node| + node.assemble_issue_sums + + node_children = tree.select { |_, child_node| node.epic_id == child_node.parent_id }.values + node.assemble_epic_sums(node_children) + end + end + + def calculate_recursive_sums + tree.each do |_, node| + node.calculate_recursive_sums(tree) + end + end + end +end diff --git a/ee/app/graphql/epics/sum_total.rb b/ee/app/graphql/epics/sum_total.rb new file mode 100644 index 00000000000000..a682b94722b765 --- /dev/null +++ b/ee/app/graphql/epics/sum_total.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Epics + class SumTotal + include AggregateConstants + + attr_accessor :sums + + def initialize + @sums = [] + end + + def add(other_sums) + sums.concat(other_sums) + end + + def by_facet(facet) + return ::Epics::CountAggregate.new(sums) if facet == COUNT + + ::Epics::WeightSumAggregate.new(sums) + end + end +end diff --git a/ee/app/graphql/epics/weight_sum_aggregate.rb b/ee/app/graphql/epics/weight_sum_aggregate.rb new file mode 100644 index 00000000000000..24a55509c54584 --- /dev/null +++ b/ee/app/graphql/epics/weight_sum_aggregate.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Epics + class WeightSumAggregate < Aggregate + attr_accessor :opened_issues, :closed_issues + + def initialize(sums) + @sums = sums + + @opened_issues = sum_objects(OPENED_ISSUE_STATE, :issue) + @closed_issues = sum_objects(CLOSED_ISSUE_STATE, :issue) + end + + def facet + WEIGHT_SUM + end + end +end diff --git a/ee/app/graphql/types/epic_type.rb b/ee/app/graphql/types/epic_type.rb index 7003836feaab9c..45b0d61920d8d5 100644 --- a/ee/app/graphql/types/epic_type.rb +++ b/ee/app/graphql/types/epic_type.rb @@ -121,21 +121,26 @@ class EpicType < BaseObject resolver: Resolvers::EpicIssuesResolver field :descendant_counts, Types::EpicDescendantCountType, null: true, complexity: 10, - description: 'Number of open and closed descendant epics and issues', - resolve: -> (epic, args, ctx) do - Epics::DescendantCountService.new(epic, ctx[:current_user]) - end + description: 'Number of open and closed descendant epics and issues', + resolve: -> (epic, args, ctx) do + if Feature.enabled?(:unfiltered_epic_aggregates) + Epics::LazyEpicAggregate.new(ctx, epic.id, Epics::LazyEpicAggregate::COUNT) + else + Epics::DescendantCountService.new(epic, ctx[:current_user]) + end + end field :descendant_weight_sum, Types::EpicDescendantWeightSumType, null: true, complexity: 10, - description: "Total weight of open and closed descendant epic's issues", - feature_flag: :unfiltered_epic_aggregates - - def descendant_weight_sum - OpenStruct.new( - # We shouldn't stop the whole query, so returning -1 for a semi-noisy error - opened_issues: -1, - closed_issues: -1 - ) - end + description: "Total weight of open and closed descendant epic's issues", + feature_flag: :unfiltered_epic_aggregates, + resolve: -> (epic, args, ctx) do + Epics::LazyEpicAggregate.new(ctx, epic.id, Epics::LazyEpicAggregate::WEIGHT_SUM) + end + + field :health_status, + ::Types::HealthStatusEnum, + null: true, + description: 'Current health status', + feature_flag: :save_issuable_health_status end end diff --git a/ee/app/models/ee/epic.rb b/ee/app/models/ee/epic.rb index a924d3a53b61b2..640303c6aa62e1 100644 --- a/ee/app/models/ee/epic.rb +++ b/ee/app/models/ee/epic.rb @@ -102,8 +102,6 @@ def close scope :start_date_inherited, -> { where(start_date_is_fixed: [nil, false]) } scope :due_date_inherited, -> { where(due_date_is_fixed: [nil, false]) } - scope :counts_by_state, -> { group(:state_id).count } - MAX_HIERARCHY_DEPTH = 5 def etag_caching_enabled? diff --git a/ee/app/services/epics/bulk_epic_aggregate_loader.rb b/ee/app/services/epics/bulk_epic_aggregate_loader.rb new file mode 100644 index 00000000000000..7671efcf8c0748 --- /dev/null +++ b/ee/app/services/epics/bulk_epic_aggregate_loader.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Epics + class BulkEpicAggregateLoader + include AggregateConstants + + # This class retrieves each epic and its child epics recursively + # It allows us to recreate the epic tree structure in POROs + def initialize(epic_ids:) + @results = {} + @target_epic_ids = epic_ids.present? ? [epic_ids].flatten.compact : [] + end + + # rubocop: disable CodeReuse/ActiveRecord + def execute + return {} unless target_epic_ids.any? + + # We do a left outer join in order to capture epics with no issues + # This is so we can aggregate the epic counts for every epic + raw_results = ::Gitlab::ObjectHierarchy.new(Epic.where(id: target_epic_ids)).base_and_descendants + .left_joins(epic_issues: :issue) + .group("issues.state_id", "epics.id", "epics.iid", "epics.parent_id", "epics.state_id") + .select("epics.id, epics.iid, epics.parent_id, epics.state_id AS epic_state_id, issues.state_id AS issues_state_id, COUNT(issues) AS issues_count, SUM(COALESCE(issues.weight, 0)) AS issues_weight_sum") + + raw_results = raw_results.map(&:attributes).map(&:with_indifferent_access) + group_by_epic_id(raw_results) + @results + end + # rubocop: enable CodeReuse/ActiveRecord + + private + + attr_reader :target_epic_ids, :results + + def group_by_epic_id(raw_records) + # for each id, populate with matching records + # change from a series of { id: x ... } to { x => [{...}, {...}] } + raw_records.map { |r| r[:id] }.uniq.each do |epic_id| + records = [] + matching_records = raw_records.select { |record| record[:id] == epic_id } + matching_records.each do |record| + records << record.except(:id).to_h.with_indifferent_access + end + @results[epic_id] = records + end + end + end +end diff --git a/ee/spec/graphql/epics/aggregate_spec.rb b/ee/spec/graphql/epics/aggregate_spec.rb new file mode 100644 index 00000000000000..c940487771f190 --- /dev/null +++ b/ee/spec/graphql/epics/aggregate_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +describe Epics::Aggregate do + + let(:epic_type) { described_class::EPIC_TYPE } + let(:issue_type) { described_class::ISSUE_TYPE } + + let(:opened_epic_state) { described_class::OPENED_EPIC_STATE } + let(:closed_epic_state) { described_class::CLOSED_EPIC_STATE } + let(:opened_issue_state) { described_class::OPENED_ISSUE_STATE } + let(:closed_issue_state) { described_class::CLOSED_ISSUE_STATE } + + let(:weight_sum) { Epics::EpicNode::WEIGHT_SUM } + let(:count) { Epics::EpicNode::COUNT } + + context 'when CountAggregate' do + subject { Epics::CountAggregate.new(sums) } + + describe 'summation' do + let(:sums) { [] } + + context 'when there are no sum objects' do + it 'returns 0 for all values', :aggregate_failures do + expect(subject.opened_issues).to eq 0 + expect(subject.closed_issues).to eq 0 + expect(subject.opened_epics).to eq 0 + expect(subject.closed_epics).to eq 0 + end + end + + context 'when some sums exist' do + let(:sums) do + [ + Epics::Sum.new(facet: :count, type: epic_type, value: 1, state: opened_epic_state), + Epics::Sum.new(facet: :count, type: epic_type, value: 1, state: closed_epic_state), + Epics::Sum.new(facet: :count, type: issue_type, value: 1, state: opened_issue_state), + Epics::Sum.new(facet: :count, type: issue_type, value: 1, state: opened_issue_state), + Epics::Sum.new(facet: :weight_sum, type: issue_type, value: 22, state: closed_issue_state) + ] + end + + it 'returns sums of appropriate values', :aggregate_failures do + expect(subject.opened_issues).to eq 2 + expect(subject.closed_issues).to eq 0 + expect(subject.opened_epics).to eq 1 + expect(subject.closed_epics).to eq 1 + end + end + end + end + + context 'when WeightSumAggregate' do + subject { Epics::WeightSumAggregate.new(sums) } + + describe 'summation' do + let(:sums) { [] } + + context 'when there are no sum objects' do + it 'returns 0 for all values', :aggregate_failures do + expect(subject.opened_issues).to eq 0 + expect(subject.closed_issues).to eq 0 + end + end + + context 'when some sums exist' do + let(:sums) do + [ + Epics::Sum.new(facet: :weight_sum, type: issue_type, value: 1, state: opened_issue_state), + Epics::Sum.new(facet: :weight_sum, type: issue_type, value: 1, state: opened_issue_state), + Epics::Sum.new(facet: :count, type: issue_type, value: 22, state: closed_issue_state) + ] + end + + it 'returns sums of appropriate values', :aggregate_failures do + expect(subject.opened_issues).to eq 2 + expect(subject.closed_issues).to eq 0 + end + end + end + end +end diff --git a/ee/spec/graphql/epics/epic_node_spec.rb b/ee/spec/graphql/epics/epic_node_spec.rb new file mode 100644 index 00000000000000..d4903f5b89ae36 --- /dev/null +++ b/ee/spec/graphql/epics/epic_node_spec.rb @@ -0,0 +1,211 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Epics::EpicNode do + let(:epic_id) { 34 } + let(:epic_iid) { 5 } + + describe '#initialize' do + let(:fake_data) do + [ + { iid: epic_iid, epic_state_id: epic_state_id, issues_count: 1, issues_weight_sum: 2, parent_id: parent_id, issues_state_id: Constants::OPENED_ISSUE_STATE }, + { iid: epic_iid, epic_state_id: epic_state_id, issues_count: 2, issues_weight_sum: 2, parent_id: parent_id, issues_state_id: Constants::CLOSED_ISSUE_STATE } + ] + end + + shared_examples 'setting attributes based on the first record' do |attributes| + let(:parent_id) { attributes[:parent_id] } + let(:epic_state_id) { attributes[:epic_state_id] } + + it 'sets epic attributes based on the first record' do + new_node = described_class.new(epic_id, fake_data) + + expect(new_node.parent_id).to eq parent_id + end + end + + it_behaves_like 'setting attributes based on the first record', { epic_state_id: Constants::OPENED_EPIC_STATE, parent_id: nil } + it_behaves_like 'setting attributes based on the first record', { epic_state_id: Constants::CLOSED_EPIC_STATE, parent_id: 2 } + end + + describe '#assemble_issue_sums' do + subject { described_class.new(epic_id, fake_data) } + + context 'an epic with no issues' do + let(:fake_data) do + [ + { iid: epic_iid, epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 0, issues_weight_sum: 0, parent_id: nil, issues_state_id: nil } + ] + end + + it 'does not create any sums' do + subject.assemble_issue_sums + + expect(subject.direct_sums.count).to eq 0 + end + end + + context 'an epic with issues' do + context 'with a nonzero count but a zero weight' do + let(:fake_data) do + [ + { iid: epic_iid, epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 1, issues_weight_sum: 0, parent_id: nil, issues_state_id: Constants::OPENED_ISSUE_STATE } + ] + end + + it 'creates no sums for the weight if the issues have 0 weight' do + subject.assemble_issue_sums + + expect(subject.direct_sums.count).to eq 1 + expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::COUNT, Constants::OPENED_ISSUE_STATE, 1) + end + end + + context 'with a nonzero count and nonzero weight for a single state' do + let(:fake_data) do + [ + { iid: epic_iid, epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 1, issues_weight_sum: 2, parent_id: nil, issues_state_id: Constants::OPENED_ISSUE_STATE } + ] + end + + it 'creates two sums' do + subject.assemble_issue_sums + + expect(subject.direct_sums.count).to eq 2 + expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::COUNT, Constants::OPENED_ISSUE_STATE, 1) + expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::OPENED_ISSUE_STATE, 2) + end + end + + context 'with a nonzero count and nonzero weight for multiple states' do + let(:fake_data) do + [ + { iid: epic_iid, epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 1, issues_weight_sum: 2, parent_id: nil, issues_state_id: Constants::OPENED_ISSUE_STATE }, + { iid: epic_iid, epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 3, issues_weight_sum: 5, parent_id: nil, issues_state_id: Constants::CLOSED_ISSUE_STATE } + ] + end + + it 'creates two sums' do + subject.assemble_issue_sums + + expect(subject.direct_sums.count).to eq 4 + expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::COUNT, Constants::OPENED_ISSUE_STATE, 1) + expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::OPENED_ISSUE_STATE, 2) + expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::COUNT, Constants::CLOSED_ISSUE_STATE, 3) + expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::CLOSED_ISSUE_STATE, 5) + end + end + end + end + + describe '#assemble_epic_sums' do + subject { described_class.new(epic_id, [{ parent_id: nil, epic_state_id: Constants::CLOSED_EPIC_STATE }]) } + + context 'with a child epic' do + let(:child_epic_id) { 45 } + let!(:child_epic_node) { described_class.new(child_epic_id, [{ parent_id: epic_id, epic_state_id: Constants::CLOSED_EPIC_STATE }]) } + + before do + subject.child_ids << child_epic_id + end + + it 'adds up the number of the child epics' do + subject.assemble_epic_sums([child_epic_node]) + + expect(subject.direct_sums.count).to eq 1 + expect(subject).to have_direct_sum(Constants::EPIC_TYPE, Constants::COUNT, Constants::CLOSED_EPIC_STATE, 1) + end + end + end + + describe '#calculate_recursive_sums' do + subject { described_class.new(epic_id, [{ parent_id: nil, epic_state_id: Constants::CLOSED_EPIC_STATE }]) } + + before do + allow(subject).to receive(:direct_sums).and_return(direct_sums) + end + + context 'an epic with no child epics' do + let(:tree) do + { epic_id => subject } + end + + context 'with no child issues' do + let(:direct_sums) do + [] + end + + it 'returns a SumTotal with no sums' do + result = subject.calculate_recursive_sums(tree) + + expect(result.sums).not_to be_nil + expect(result.sums.count).to eq 0 + end + end + + context 'with an issue with 0 weight' do + let(:direct_sums) do + [Epics::EpicNode::Sum.new(Constants::COUNT, Constants::CLOSED_EPIC_STATE, Constants::ISSUE_TYPE, 1)] + end + + it 'returns a SumTotal with only a weight sum' do + result = subject.calculate_recursive_sums(tree) + + expect(result.sums).not_to be_nil + expect(result.sums.count).to eq 1 + end + end + + context 'with an issue with nonzero weight' do + let(:direct_sums) do + [ + Epics::EpicNode::Sum.new(Constants::COUNT, Constants::CLOSED_EPIC_STATE, Constants::ISSUE_TYPE, 1), + Epics::EpicNode::Sum.new(Constants::WEIGHT_SUM, Constants::CLOSED_EPIC_STATE, Constants::ISSUE_TYPE, 2) + ] + end + + it 'returns a SumTotal with only a weight sum' do + result = subject.calculate_recursive_sums(tree) + + expect(result.sums).not_to be_nil + expect(result.sums.count).to eq 2 + end + end + end + + context 'an epic with child epics' do + let(:child_epic_id) { 45 } + let(:tree) do + { epic_id => subject, child_epic_id => child_epic_node } + end + let(:child_epic_node) { described_class.new(child_epic_id, [{ parent_id: epic_id, epic_state_id: Constants::CLOSED_EPIC_STATE }]) } + let(:direct_sums) do + [ # only one opened epic, the child + Epics::EpicNode::Sum.new(Constants::COUNT, Constants::OPENED_EPIC_STATE, Constants::EPIC_TYPE, 1) + ] + end + + before do + subject.child_ids << child_epic_id + allow(child_epic_node).to receive(:direct_sums).and_return(child_sums) + end + + context 'with a child that has issues of nonzero weight' do + let(:child_sums) do + [ # 1 issue of weight 2 + Epics::EpicNode::Sum.new(Constants::COUNT, Constants::OPENED_ISSUE_STATE, Constants::ISSUE_TYPE, 1), + Epics::EpicNode::Sum.new(Constants::WEIGHT_SUM, Constants::OPENED_ISSUE_STATE, Constants::ISSUE_TYPE, 2) + ] + end + + it 'returns the correct sum total' do + result = subject.calculate_recursive_sums(tree) + + expect(result.sums).not_to be_nil + expect(result.sums.count).to eq 3 + end + end + end + end +end diff --git a/ee/spec/graphql/epics/lazy_epic_aggregate_spec.rb b/ee/spec/graphql/epics/lazy_epic_aggregate_spec.rb new file mode 100644 index 00000000000000..955493f957c811 --- /dev/null +++ b/ee/spec/graphql/epics/lazy_epic_aggregate_spec.rb @@ -0,0 +1,143 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Epics::LazyEpicAggregate do + let(:query_ctx) do + {} + end + let(:epic_id) { 37 } + let(:epic_iid) { 18 } + let(:child_epic_id) { 38 } + + describe '#initialize' do + it 'requires either :weight_sum or :count as a facet', :aggregate_failures do + expect { described_class.new(query_ctx, epic_id, :nonsense) }.to raise_error(ArgumentError, /Invalid aggregate facet/) + expect { described_class.new(query_ctx, epic_id, nil) }.to raise_error(ArgumentError, /No aggregate facet/) + expect { described_class.new(query_ctx, epic_id, "") }.to raise_error(ArgumentError, /No aggregate facet/) + end + + context 'with valid facets :weight_sum or :count' do + specify 'as a symbol', :aggregate_failures do + [:weight_sum, :count].each do |valid_facet| + described_class.new(query_ctx, epic_id, valid_facet) + end + end + + specify 'as a string', :aggregate_failures do + %w(weight_sum count).each do |valid_facet| + described_class.new(query_ctx, epic_id, valid_facet) + end + end + end + + it 'adds the epic_id to lazy state' do + described_class.new(query_ctx, epic_id, :count) + + expect(query_ctx[:lazy_epic_aggregate][:pending_ids]).to match [epic_id] + end + end + + describe '#epic_aggregate' do + let(:single_record) do + { iid: 6, issues_count: 4, issues_weight_sum: 9, parent_id: nil, issues_state_id: Constants::OPENED_ISSUE_STATE, epic_state_id: Constants::OPENED_EPIC_STATE } + end + let(:epic_info_node) { Epics::EpicNode.new(epic_id, [single_record] ) } + + subject { described_class.new(query_ctx, epic_id, :count) } + + before do + subject.instance_variable_set(:@lazy_state, fake_state) + end + + context 'if the record has already been loaded' do + let(:fake_state) do + { pending_ids: Set.new, tree: { epic_id => epic_info_node } } + end + + it 'does not make the query again' do + expect(epic_info_node).to receive(:aggregate_object_by).with(subject.facet) + expect(Epics::BulkEpicAggregateLoader).not_to receive(:new) + + subject.epic_aggregate + end + end + + context 'if the record has not been loaded' do + let(:other_epic_id) { 39 } + let(:fake_state) do + { pending_ids: Set.new([epic_id, child_epic_id]), tree: {} } + end + let(:fake_data) do + { + epic_id => [{ epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 2, issues_weight_sum: 5, parent_id: nil, issues_state_id: Constants::OPENED_ISSUE_STATE }], + child_epic_id => [{ epic_state_id: Constants::CLOSED_EPIC_STATE, issues_count: 4, issues_weight_sum: 17, parent_id: epic_id, issues_state_id: Constants::CLOSED_ISSUE_STATE }], + other_epic_id => [{ epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 0, issues_weight_sum: 0, parent_id: nil, issues_state_id: nil }] # represents an epic with no parent and no issues + } + end + + before do + allow(Epics::EpicNode).to receive(:aggregate_object_by).and_call_original + expect_any_instance_of(Epics::BulkEpicAggregateLoader).to receive(:execute).and_return(fake_data) + end + + it 'clears the pending IDs' do + subject.epic_aggregate + + lazy_state = subject.instance_variable_get(:@lazy_state) + + expect(lazy_state[:pending_ids]).to be_empty + end + + it 'creates the parent-child associations', :aggregate_failures do + subject.epic_aggregate + + lazy_state = subject.instance_variable_get(:@lazy_state) + tree = lazy_state[:tree] + + expect(tree[child_epic_id].parent_id).to eq epic_id + expect(tree[epic_id].child_ids).to match_array([child_epic_id]) + end + + context 'for a parent-child relationship' do + it 'assembles direct sums', :aggregate_failures do + subject.epic_aggregate + + lazy_state = subject.instance_variable_get(:@lazy_state) + tree = lazy_state[:tree] + + expect(tree[epic_id]).to have_direct_sum(Constants::EPIC_TYPE, Constants::COUNT, Constants::CLOSED_EPIC_STATE, 1) + expect(tree[epic_id]).to have_direct_sum(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::OPENED_ISSUE_STATE, 5) + expect(tree[epic_id]).to have_direct_sum(Constants::EPIC_TYPE, Constants::COUNT, Constants::CLOSED_EPIC_STATE, 1) + + expect(tree[child_epic_id]).to have_direct_sum(Constants::ISSUE_TYPE, Constants::COUNT, Constants::CLOSED_ISSUE_STATE, 4) + expect(tree[child_epic_id]).to have_direct_sum(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::CLOSED_ISSUE_STATE, 17) + end + + it 'assembles recursive sums for the parent', :aggregate_failures do + subject.epic_aggregate + + lazy_state = subject.instance_variable_get(:@lazy_state) + tree = lazy_state[:tree] + + expect(tree[epic_id]).to have_aggregate(Constants::ISSUE_TYPE, Constants::COUNT, Constants::OPENED_ISSUE_STATE, 2) + expect(tree[epic_id]).to have_aggregate(Constants::ISSUE_TYPE, Constants::COUNT, Constants::CLOSED_ISSUE_STATE, 4) + expect(tree[epic_id]).to have_aggregate(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::OPENED_ISSUE_STATE, 5) + expect(tree[epic_id]).to have_aggregate(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::CLOSED_ISSUE_STATE, 17) + expect(tree[epic_id]).to have_aggregate(Constants::EPIC_TYPE, Constants::COUNT, Constants::CLOSED_EPIC_STATE, 1) + end + end + + context 'for a standalone epic with no issues' do + it 'assembles direct sums', :aggregate_failures do + subject.epic_aggregate + + lazy_state = subject.instance_variable_get(:@lazy_state) + tree = lazy_state[:tree] + + expect(tree[other_epic_id].direct_sums).to be_empty + end + end + end + end +end diff --git a/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb b/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb index de94aea20790a9..1b7428dbce7f04 100644 --- a/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb +++ b/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb @@ -7,50 +7,89 @@ let_it_be(:current_user) { create(:user) } let_it_be(:group) { create(:group, :public) } + let_it_be(:subgroup) { create(:group, :private, parent: group)} + let_it_be(:subsubgroup) { create(:group, :private, parent: subgroup)} + + let_it_be(:project) { create(:project, namespace: group) } + let_it_be(:parent_epic) { create(:epic, id: 1, group: group, title: 'parent epic') } + let_it_be(:epic_with_issues) { create(:epic, id: 2, group: subgroup, parent: parent_epic, title: 'epic with issues') } + let_it_be(:epic_without_issues) { create(:epic, :closed, id: 3, group: subgroup, parent: parent_epic, title: 'epic without issues') } + let_it_be(:closed_epic) { create(:epic, :closed, id: 4, group: subgroup, parent: parent_epic, title: 'closed epic') } - let(:query) do - graphql_query_for('group', { fullPath: group.full_path }, query_graphql_field('epics', { iid: parent_epic.iid }, epic_aggregates_query)) - end + let_it_be(:issue1) { create(:issue, project: project, weight: 5, state: :opened) } + let_it_be(:issue2) { create(:issue, project: project, weight: 7, state: :closed) } + + let_it_be(:epic_issue1) { create(:epic_issue, epic: epic_with_issues, issue: issue1) } + let_it_be(:epic_issue2) { create(:epic_issue, epic: epic_with_issues, issue: issue2) } let(:epic_aggregates_query) do <<~QUERY - nodes { - descendantWeightSum { - openedIssues - closedIssues - } - descendantCounts { - openedEpics - closedEpics - openedIssues - closedIssues - } + nodes { + descendantWeightSum { + openedIssues + closedIssues + } + descendantCounts { + openedEpics + closedEpics + openedIssues + closedIssues } + } QUERY end + let(:query) do + graphql_query_for('group', { fullPath: group.full_path }, query_graphql_field('epics', { iid: parent_epic.iid }, epic_aggregates_query)) + end + + subject { graphql_data.dig('group', 'epics', 'nodes') } + before do + group.add_developer(current_user) stub_licensed_features(epics: true) + post_graphql(query, current_user: current_user) end + it_behaves_like 'a working graphql query' + context 'with feature flag enabled' do before do stub_feature_flags(unfiltered_epic_aggregates: true) end - it 'returns a placeholder with -1 weights and does not error' do - post_graphql(query, current_user: current_user) + it 'returns the epic counts' do + epic_count_result = { + "openedEpics" => 1, + "closedEpics" => 2 + } - actual_result = graphql_data.dig('group', 'epics', 'nodes').first - expected_result = { - "descendantWeightSum" => { - "openedIssues" => -1, - "closedIssues" => -1 - } + is_expected.to include( + a_hash_including('descendantCounts' => a_hash_including(epic_count_result)) + ) + end + + it 'returns the issue counts' do + issue_count_result = { + "openedIssues" => 1, + "closedIssues" => 1 } - expect(actual_result).to include expected_result + is_expected.to include( + a_hash_including('descendantCounts' => a_hash_including(issue_count_result)) + ) + end + + it 'returns the weights' do + descendant_weight_result = { + "openedIssues" => 5, + "closedIssues" => 7 + } + + is_expected.to include( + a_hash_including('descendantWeightSum' => a_hash_including(descendant_weight_result)) + ) end end @@ -62,14 +101,14 @@ context 'when requesting counts' do let(:epic_aggregates_query) do <<~QUERY - nodes { - descendantCounts { - openedEpics - closedEpics - openedIssues - closedIssues + nodes { + descendantCounts { + openedEpics + closedEpics + openedIssues + closedIssues + } } - } QUERY end @@ -83,12 +122,12 @@ context 'when requesting weights' do let(:epic_aggregates_query) do <<~QUERY - nodes { - descendantWeightSum { - openedIssues - closedIssues + nodes { + descendantWeightSum { + openedIssues + closedIssues + } } - } QUERY end diff --git a/ee/spec/services/epics/bulk_epic_aggregate_loader_spec.rb b/ee/spec/services/epics/bulk_epic_aggregate_loader_spec.rb new file mode 100644 index 00000000000000..312b3e75b955d3 --- /dev/null +++ b/ee/spec/services/epics/bulk_epic_aggregate_loader_spec.rb @@ -0,0 +1,137 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Epics::BulkEpicAggregateLoader do + let_it_be(:closed_issue_state_id) { Issue.available_states[:closed] } + let_it_be(:opened_issue_state_id) { Issue.available_states[:opened] } + + let_it_be(:closed_epic_state_id) { Epic.available_states[:closed] } + let_it_be(:opened_epic_state_id) { Epic.available_states[:opened] } + + let_it_be(:group) { create(:group, :public) } + let_it_be(:subgroup) { create(:group, :private, parent: group)} + + let_it_be(:project) { create(:project, namespace: group) } + let_it_be(:subproject) { create(:project, namespace: subgroup) } + + let_it_be(:parent_epic) { create(:epic, group: group, title: 'parent epic') } + let_it_be(:epic_with_issues) { create(:epic, group: subgroup, parent: parent_epic, state: :opened, title: 'epic with issues') } + + # closed, no issues + let_it_be(:epic_without_issues) { create(:epic, group: subgroup, parent: parent_epic, state: :closed, title: 'epic without issues') } + + # open, public + let_it_be(:issue1) { create(:issue, project: project, weight: 1, state: :opened) } + let_it_be(:issue2) { create(:issue, project: project, weight: 1, state: :opened) } + # closed + let_it_be(:issue3) { create(:issue, project: project, weight: 1, state: :closed) } + let_it_be(:issue4) { create(:issue, project: project, weight: 1, state: :closed) } + # confidential + let_it_be(:issue5) { create(:issue, project: project, weight: 1, confidential: true, state: :opened) } + let_it_be(:issue6) { create(:issue, project: project, weight: 1, confidential: true, state: :opened) } + # in private project, private subgroup + let_it_be(:issue7) { create(:issue, project: subproject, weight: 1, state: :opened) } + let_it_be(:issue8) { create(:issue, project: subproject, weight: 1, state: :opened) } + # private project, confidential, private subgroup + let_it_be(:issue9) { create(:issue, project: subproject, weight: 1, confidential: true, state: :opened) } + let_it_be(:issue10) { create(:issue, project: subproject, weight: 1, confidential: true, state: :opened) } + # nil weight doesn't break it + let_it_be(:issue11) { create(:issue, project: project, weight: 0, state: :opened) } + let_it_be(:issue12) { create(:issue, project: project, weight: nil, state: :opened) } + + let_it_be(:epic_issue1) { create(:epic_issue, epic: parent_epic, issue: issue1) } + let_it_be(:epic_issue2) { create(:epic_issue, epic: epic_with_issues, issue: issue2) } + let_it_be(:epic_issue3) { create(:epic_issue, epic: parent_epic, issue: issue3) } + let_it_be(:epic_issue4) { create(:epic_issue, epic: epic_with_issues, issue: issue4) } + let_it_be(:epic_issue5) { create(:epic_issue, epic: parent_epic, issue: issue5) } + let_it_be(:epic_issue6) { create(:epic_issue, epic: epic_with_issues, issue: issue6) } + let_it_be(:epic_issue7) { create(:epic_issue, epic: parent_epic, issue: issue7) } + let_it_be(:epic_issue8) { create(:epic_issue, epic: epic_with_issues, issue: issue8) } + let_it_be(:epic_issue9) { create(:epic_issue, epic: parent_epic, issue: issue9) } + let_it_be(:epic_issue10) { create(:epic_issue, epic: epic_with_issues, issue: issue10) } + let_it_be(:epic_issue11) { create(:epic_issue, epic: parent_epic, issue: issue11) } + let_it_be(:epic_issue12) { create(:epic_issue, epic: epic_with_issues, issue: issue12) } + + subject { described_class.new(epic_ids: target_ids) } + + before do + stub_licensed_features(epics: true) + end + + context 'when epic ids with issues is provided' do + let(:target_ids) { parent_epic.id } + + it 'sums all the weights, even confidential, or in private groups' do + expected_result = { + parent_epic.id => [ + result_for(parent_epic, issues_state: opened_issue_state_id, issues_count: 5, issues_weight_sum: 4), + result_for(parent_epic, issues_state: closed_issue_state_id, issues_count: 1, issues_weight_sum: 1) + ], + epic_with_issues.id => [ + result_for(epic_with_issues, issues_state: opened_issue_state_id, issues_count: 5, issues_weight_sum: 4), + result_for(epic_with_issues, issues_state: closed_issue_state_id, issues_count: 1, issues_weight_sum: 1) + ], + epic_without_issues.id => [ + result_for(epic_without_issues, issues_state: nil, issues_count: 0, issues_weight_sum: 0) + ] + } + + result = subject.execute + + expected_result.each do |epic_id, records| + expect(result[epic_id]).to match_array records + end + end + + it 'contains results for all epics, even if they do not have issues' do + result = subject.execute + + # epic_without_issues is included, even if it has none + expect(result.keys).to match_array([parent_epic.id, epic_with_issues.id, epic_without_issues.id]) + end + + context 'testing for a single database query' do + it 'does not repeat database queries for subepics' do + recorder = ActiveRecord::QueryRecorder.new { described_class.new(epic_ids: epic_with_issues.id).execute } + + # this one has sub-epics, but there should still only be one query + expect { described_class.new(epic_ids: [parent_epic.id, epic_with_issues.id]).execute }.not_to exceed_query_limit(recorder) + end + + it 'avoids N+1' do + recorder = ActiveRecord::QueryRecorder.new { described_class.new(epic_ids: epic_with_issues.id).execute } + + expect { described_class.new(epic_ids: [epic_with_issues.id, parent_epic.id]).execute }.not_to exceed_query_limit(recorder) + end + end + end + + context 'when an epic without issues is provided' do + let(:target_ids) { epic_without_issues.id } + + it 'returns a placeholder' do + expected_result = [ + result_for(epic_without_issues, issues_state: nil, issues_count: 0, issues_weight_sum: 0) + ] + + actual_result = subject.execute + + expect(actual_result[epic_without_issues.id]).to match_array(expected_result) + end + end + + context 'when no epic ids are provided' do + [nil, [], ""].each do |empty_arg| + let(:target_ids) { empty_arg } + + it 'returns an empty set' do + expect(subject.execute).to eq({}) + end + end + end + + def result_for(epic, issues_state:, issues_count:, issues_weight_sum:) + { iid: epic.iid, issues_count: issues_count, issues_weight_sum: issues_weight_sum, parent_id: epic.parent_id, issues_state_id: issues_state, epic_state_id: Epic.available_states[epic.state_id] }.stringify_keys + end +end diff --git a/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb b/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb new file mode 100644 index 00000000000000..0a8f7180b2295f --- /dev/null +++ b/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +RSpec::Matchers.define :have_direct_sum do |type, facet, state, value| + # supports_block_expectations + + match do |epic_node_result| + expect(epic_node_result).not_to be_nil + expect(epic_node_result.direct_sums).not_to be_empty + + matching = epic_node_result.direct_sums.select { |sum| sum.type == type && sum.facet == facet && sum.state == state && sum.value == value } + expect(matching).not_to be_empty + end + + failure_message do |epic_node_result| + if epic_node_result.nil? + "expected for there to be an epic node, but it is nil" + else + <<~FAILURE_MSG + expected epic node with id #{epic_node_result.epic_id} to have a sum with facet '#{facet}', state '#{state}', type '#{type}' and value '#{value}'. Has #{epic_node_result.direct_sums.count} sum objects#{", none of which match" if epic_node_result.direct_sums.count > 0}. + Sums: #{epic_node_result.direct_sums.inspect} + FAILURE_MSG + end + end +end + +RSpec::Matchers.define :have_aggregate do |type, facet, state, value| + supports_block_expectations + + match do |epic_node_result| + aggregate_object = epic_node_result.aggregate_object_by(facet) + expect(aggregate_object.send(method_name(type, state))).to eq value + end + + failure_message do |epic_node_result| + aggregate_object = epic_node_result.aggregate_object_by(facet) + aggregate_method = method_name(type, state) + "Epic node with id #{epic_node_result.epic_id} called #{aggregate_method} on aggregate object of type #{aggregate_object.class.name}. Value was expected to be #{value} but was #{aggregate_object.send(aggregate_method)}." + end + + def method_name(type, state) + if type == Constants::ISSUE_TYPE + return :opened_issues if state == Constants::OPENED_ISSUE_STATE + + :closed_issues + elsif type == Constants::EPIC_TYPE + return :opened_epics if state == Constants::OPENED_EPIC_STATE + + :closed_epics + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b862fc4bbd906b..9de04c6400fa8f 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -276,3 +276,7 @@ # Disable timestamp checks for invisible_captcha InvisibleCaptcha.timestamp_enabled = false + +class Constants + include Epics::AggregateConstants +end -- GitLab From c0749f85923da9538a1bd52ce70aa1ac027e5522 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Fri, 21 Feb 2020 23:26:21 +1300 Subject: [PATCH 2/9] Add feature flag Without the feature flag, the DescendantCountService functionality is used instead --- app/graphql/gitlab_schema.rb | 2 +- app/models/issue.rb | 2 + ee/app/models/ee/epic.rb | 2 + .../group/epic/epic_aggregate_query_spec.rb | 128 ++++++------------ 4 files changed, 43 insertions(+), 91 deletions(-) diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index b1cd4fd748c616..a327ad228d7ab3 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -28,7 +28,7 @@ class GitlabSchema < GraphQL::Schema default_max_page_size 100 - lazy_resolve Epics::LazyEpicAggregate, :epic_aggregate + lazy_resolve ::Epics::LazyEpicAggregate, :epic_aggregate class << self def multiplex(queries, **kwargs) diff --git a/app/models/issue.rb b/app/models/issue.rb index aab5c832df9db2..d3f597c0bda0b6 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -74,6 +74,8 @@ class Issue < ApplicationRecord scope :public_only, -> { where(confidential: false) } scope :confidential_only, -> { where(confidential: true) } + scope :counts_by_state, -> { reorder(nil).group(:state_id).count } + ignore_column :state, remove_with: '12.7', remove_after: '2019-12-22' after_commit :expire_etag_cache, unless: :importing? diff --git a/ee/app/models/ee/epic.rb b/ee/app/models/ee/epic.rb index 640303c6aa62e1..a924d3a53b61b2 100644 --- a/ee/app/models/ee/epic.rb +++ b/ee/app/models/ee/epic.rb @@ -102,6 +102,8 @@ def close scope :start_date_inherited, -> { where(start_date_is_fixed: [nil, false]) } scope :due_date_inherited, -> { where(due_date_is_fixed: [nil, false]) } + scope :counts_by_state, -> { group(:state_id).count } + MAX_HIERARCHY_DEPTH = 5 def etag_caching_enabled? diff --git a/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb b/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb index 1b7428dbce7f04..5846e9076d7904 100644 --- a/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb +++ b/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe 'Epic aggregates (count and weight)' do +describe 'Query epic aggregates (count and weight)' do include GraphqlHelpers let_it_be(:current_user) { create(:user) } @@ -25,18 +25,18 @@ let(:epic_aggregates_query) do <<~QUERY - nodes { - descendantWeightSum { - openedIssues - closedIssues - } - descendantCounts { - openedEpics - closedEpics - openedIssues - closedIssues - } + nodes { + descendantWeightSum { + openedIssues + closedIssues } + descendantCounts { + openedEpics + closedEpics + openedIssues + closedIssues + } + } QUERY end @@ -54,88 +54,36 @@ it_behaves_like 'a working graphql query' - context 'with feature flag enabled' do - before do - stub_feature_flags(unfiltered_epic_aggregates: true) - end - - it 'returns the epic counts' do - epic_count_result = { - "openedEpics" => 1, - "closedEpics" => 2 - } + it 'returns the epic counts' do + epic_count_result = { + "openedEpics" => 1, + "closedEpics" => 2 + } - is_expected.to include( - a_hash_including('descendantCounts' => a_hash_including(epic_count_result)) - ) - end - - it 'returns the issue counts' do - issue_count_result = { - "openedIssues" => 1, - "closedIssues" => 1 - } - - is_expected.to include( - a_hash_including('descendantCounts' => a_hash_including(issue_count_result)) - ) - end + is_expected.to include( + a_hash_including('descendantCounts' => a_hash_including(epic_count_result)) + ) + end - it 'returns the weights' do - descendant_weight_result = { - "openedIssues" => 5, - "closedIssues" => 7 - } + it 'returns the issue counts' do + issue_count_result = { + "openedIssues" => 1, + "closedIssues" => 1 + } - is_expected.to include( - a_hash_including('descendantWeightSum' => a_hash_including(descendant_weight_result)) - ) - end + is_expected.to include( + a_hash_including('descendantCounts' => a_hash_including(issue_count_result)) + ) end - context 'with feature flag disabled' do - before do - stub_feature_flags(unfiltered_epic_aggregates: false) - end - - context 'when requesting counts' do - let(:epic_aggregates_query) do - <<~QUERY - nodes { - descendantCounts { - openedEpics - closedEpics - openedIssues - closedIssues - } - } - QUERY - end - - it 'uses the DescendantCountService' do - expect(Epics::DescendantCountService).to receive(:new) - - post_graphql(query, current_user: current_user) - end - end - - context 'when requesting weights' do - let(:epic_aggregates_query) do - <<~QUERY - nodes { - descendantWeightSum { - openedIssues - closedIssues - } - } - QUERY - end - - it 'returns an error' do - post_graphql(query, current_user: current_user) - - expect_graphql_errors_to_include /Field 'descendantWeightSum' doesn't exist on type 'Epic/ - end - end + it 'returns the weights' do + descendant_weight_result = { + "openedIssues" => 5, + "closedIssues" => 7 + } + + is_expected.to include( + a_hash_including('descendantWeightSum' => a_hash_including(descendant_weight_result)) + ) end end -- GitLab From 70f5a5b0e6f4d9833b44ef74edcd61c9fb66d3c6 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Sat, 22 Feb 2020 08:40:39 +1300 Subject: [PATCH 3/9] Add GraphQL schema EE concern Remove constants class from spec_helper --- app/graphql/gitlab_schema.rb | 4 ++-- ee/app/graphql/ee/gitlab_schema.rb | 11 +++++++++++ ee/spec/graphql/epics/aggregate_spec.rb | 5 ++++- spec/spec_helper.rb | 4 ---- 4 files changed, 17 insertions(+), 7 deletions(-) create mode 100644 ee/app/graphql/ee/gitlab_schema.rb diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index a327ad228d7ab3..5a4a223093ee26 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -28,8 +28,6 @@ class GitlabSchema < GraphQL::Schema default_max_page_size 100 - lazy_resolve ::Epics::LazyEpicAggregate, :epic_aggregate - class << self def multiplex(queries, **kwargs) kwargs[:max_complexity] ||= max_query_complexity(kwargs[:context]) @@ -143,3 +141,5 @@ def max_query_depth(ctx) end end end + +GitlabSchema.prepend_if_ee('EE::GitlabSchema') diff --git a/ee/app/graphql/ee/gitlab_schema.rb b/ee/app/graphql/ee/gitlab_schema.rb new file mode 100644 index 00000000000000..391c966dff4f6a --- /dev/null +++ b/ee/app/graphql/ee/gitlab_schema.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module EE + module GitlabSchema + extend ActiveSupport::Concern + + prepended do + lazy_resolve ::Epics::LazyEpicAggregate, :epic_aggregate + end + end +end diff --git a/ee/spec/graphql/epics/aggregate_spec.rb b/ee/spec/graphql/epics/aggregate_spec.rb index c940487771f190..f58aeab3f1f1c3 100644 --- a/ee/spec/graphql/epics/aggregate_spec.rb +++ b/ee/spec/graphql/epics/aggregate_spec.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true describe Epics::Aggregate do - let(:epic_type) { described_class::EPIC_TYPE } let(:issue_type) { described_class::ISSUE_TYPE } @@ -13,6 +12,10 @@ let(:weight_sum) { Epics::EpicNode::WEIGHT_SUM } let(:count) { Epics::EpicNode::COUNT } + class Constants + include ::Epics::AggregateConstants + end + context 'when CountAggregate' do subject { Epics::CountAggregate.new(sums) } diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 9de04c6400fa8f..b862fc4bbd906b 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -276,7 +276,3 @@ # Disable timestamp checks for invisible_captcha InvisibleCaptcha.timestamp_enabled = false - -class Constants - include Epics::AggregateConstants -end -- GitLab From 64ae89da5f9534e626191add8547c13d2a47a6d7 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Mon, 24 Feb 2020 11:32:55 +1300 Subject: [PATCH 4/9] Make test constants consistent - Use shared context --- doc/api/graphql/reference/gitlab_schema.json | 55 ------------------ ee/spec/graphql/epics/aggregate_spec.rb | 33 ++++------- ee/spec/graphql/epics/epic_node_spec.rb | 56 ++++++++++--------- .../graphql/epics/lazy_epic_aggregate_spec.rb | 30 +++++----- .../matchers/ee/epic_aggregate_matchers.rb | 12 ++-- .../epic_aggregate_constants.rb | 14 +++++ 6 files changed, 74 insertions(+), 126 deletions(-) create mode 100644 ee/spec/support/shared_contexts/epic_aggregate_constants.rb diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index 54491b8720c884..4d52f45d3e4beb 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -4987,20 +4987,6 @@ "isDeprecated": false, "deprecationReason": null }, - { - "name": "descendantWeightSum", - "description": "Total weight of open and closed descendant epic's issues", - "args": [ - - ], - "type": { - "kind": "OBJECT", - "name": "EpicDescendantWeights", - "ofType": null - }, - "isDeprecated": false, - "deprecationReason": null - }, { "name": "description", "description": "Description of the epic", @@ -13098,47 +13084,6 @@ "enumValues": null, "possibleTypes": null }, - { - "kind": "OBJECT", - "name": "EpicDescendantWeights", - "description": "Total weight of open and closed descendant issues", - "fields": [ - { - "name": "closedIssues", - "description": "Total weight of completed (closed) issues in this epic, including epic descendants", - "args": [ - - ], - "type": { - "kind": "SCALAR", - "name": "Int", - "ofType": null - }, - "isDeprecated": false, - "deprecationReason": null - }, - { - "name": "openedIssues", - "description": "Total weight of opened issues in this epic, including epic descendants", - "args": [ - - ], - "type": { - "kind": "SCALAR", - "name": "Int", - "ofType": null - }, - "isDeprecated": false, - "deprecationReason": null - } - ], - "inputFields": null, - "interfaces": [ - - ], - "enumValues": null, - "possibleTypes": null - }, { "kind": "OBJECT", "name": "TimelogConnection", diff --git a/ee/spec/graphql/epics/aggregate_spec.rb b/ee/spec/graphql/epics/aggregate_spec.rb index f58aeab3f1f1c3..b0f1f854d3516f 100644 --- a/ee/spec/graphql/epics/aggregate_spec.rb +++ b/ee/spec/graphql/epics/aggregate_spec.rb @@ -1,20 +1,9 @@ # frozen_string_literal: true -describe Epics::Aggregate do - let(:epic_type) { described_class::EPIC_TYPE } - let(:issue_type) { described_class::ISSUE_TYPE } - - let(:opened_epic_state) { described_class::OPENED_EPIC_STATE } - let(:closed_epic_state) { described_class::CLOSED_EPIC_STATE } - let(:opened_issue_state) { described_class::OPENED_ISSUE_STATE } - let(:closed_issue_state) { described_class::CLOSED_ISSUE_STATE } - - let(:weight_sum) { Epics::EpicNode::WEIGHT_SUM } - let(:count) { Epics::EpicNode::COUNT } +require 'spec_helper' - class Constants - include ::Epics::AggregateConstants - end +describe Epics::Aggregate do + include_context 'includes EpicAggregate constants' context 'when CountAggregate' do subject { Epics::CountAggregate.new(sums) } @@ -34,11 +23,11 @@ class Constants context 'when some sums exist' do let(:sums) do [ - Epics::Sum.new(facet: :count, type: epic_type, value: 1, state: opened_epic_state), - Epics::Sum.new(facet: :count, type: epic_type, value: 1, state: closed_epic_state), - Epics::Sum.new(facet: :count, type: issue_type, value: 1, state: opened_issue_state), - Epics::Sum.new(facet: :count, type: issue_type, value: 1, state: opened_issue_state), - Epics::Sum.new(facet: :weight_sum, type: issue_type, value: 22, state: closed_issue_state) + double(:sum, facet: COUNT_FACET, type: EPIC_TYPE, value: 1, state: OPENED_EPIC_STATE), + double(:sum, facet: COUNT_FACET, type: EPIC_TYPE, value: 1, state: CLOSED_EPIC_STATE), + double(:sum, facet: COUNT_FACET, type: ISSUE_TYPE, value: 1, state: OPENED_ISSUE_STATE), + double(:sum, facet: COUNT_FACET, type: ISSUE_TYPE, value: 1, state: OPENED_ISSUE_STATE), + double(:sum, facet: WEIGHT_SUM_FACET, type: ISSUE_TYPE, value: 22, state: CLOSED_ISSUE_STATE) ] end @@ -68,9 +57,9 @@ class Constants context 'when some sums exist' do let(:sums) do [ - Epics::Sum.new(facet: :weight_sum, type: issue_type, value: 1, state: opened_issue_state), - Epics::Sum.new(facet: :weight_sum, type: issue_type, value: 1, state: opened_issue_state), - Epics::Sum.new(facet: :count, type: issue_type, value: 22, state: closed_issue_state) + double(:sum, facet: WEIGHT_SUM_FACET, type: ISSUE_TYPE, value: 1, state: OPENED_ISSUE_STATE), + double(:sum, facet: WEIGHT_SUM_FACET, type: ISSUE_TYPE, value: 1, state: OPENED_ISSUE_STATE), + double(:sum, facet: COUNT_FACET, type: ISSUE_TYPE, value: 22, state: CLOSED_ISSUE_STATE) ] end diff --git a/ee/spec/graphql/epics/epic_node_spec.rb b/ee/spec/graphql/epics/epic_node_spec.rb index d4903f5b89ae36..245861846894b9 100644 --- a/ee/spec/graphql/epics/epic_node_spec.rb +++ b/ee/spec/graphql/epics/epic_node_spec.rb @@ -3,14 +3,16 @@ require 'spec_helper' describe Epics::EpicNode do + include_context 'includes EpicAggregate constants' + let(:epic_id) { 34 } let(:epic_iid) { 5 } describe '#initialize' do let(:fake_data) do [ - { iid: epic_iid, epic_state_id: epic_state_id, issues_count: 1, issues_weight_sum: 2, parent_id: parent_id, issues_state_id: Constants::OPENED_ISSUE_STATE }, - { iid: epic_iid, epic_state_id: epic_state_id, issues_count: 2, issues_weight_sum: 2, parent_id: parent_id, issues_state_id: Constants::CLOSED_ISSUE_STATE } + { iid: epic_iid, epic_state_id: epic_state_id, issues_count: 1, issues_weight_sum: 2, parent_id: parent_id, issues_state_id: OPENED_ISSUE_STATE }, + { iid: epic_iid, epic_state_id: epic_state_id, issues_count: 2, issues_weight_sum: 2, parent_id: parent_id, issues_state_id: CLOSED_ISSUE_STATE } ] end @@ -25,8 +27,8 @@ end end - it_behaves_like 'setting attributes based on the first record', { epic_state_id: Constants::OPENED_EPIC_STATE, parent_id: nil } - it_behaves_like 'setting attributes based on the first record', { epic_state_id: Constants::CLOSED_EPIC_STATE, parent_id: 2 } + it_behaves_like 'setting attributes based on the first record', { epic_state_id: OPENED_EPIC_STATE, parent_id: nil } + it_behaves_like 'setting attributes based on the first record', { epic_state_id: CLOSED_EPIC_STATE, parent_id: 2 } end describe '#assemble_issue_sums' do @@ -35,7 +37,7 @@ context 'an epic with no issues' do let(:fake_data) do [ - { iid: epic_iid, epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 0, issues_weight_sum: 0, parent_id: nil, issues_state_id: nil } + { iid: epic_iid, epic_state_id: OPENED_EPIC_STATE, issues_count: 0, issues_weight_sum: 0, parent_id: nil, issues_state_id: nil } ] end @@ -50,7 +52,7 @@ context 'with a nonzero count but a zero weight' do let(:fake_data) do [ - { iid: epic_iid, epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 1, issues_weight_sum: 0, parent_id: nil, issues_state_id: Constants::OPENED_ISSUE_STATE } + { iid: epic_iid, epic_state_id: OPENED_EPIC_STATE, issues_count: 1, issues_weight_sum: 0, parent_id: nil, issues_state_id: OPENED_ISSUE_STATE } ] end @@ -58,14 +60,14 @@ subject.assemble_issue_sums expect(subject.direct_sums.count).to eq 1 - expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::COUNT, Constants::OPENED_ISSUE_STATE, 1) + expect(subject).to have_direct_sum(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) end end context 'with a nonzero count and nonzero weight for a single state' do let(:fake_data) do [ - { iid: epic_iid, epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 1, issues_weight_sum: 2, parent_id: nil, issues_state_id: Constants::OPENED_ISSUE_STATE } + { iid: epic_iid, epic_state_id: OPENED_EPIC_STATE, issues_count: 1, issues_weight_sum: 2, parent_id: nil, issues_state_id: OPENED_ISSUE_STATE } ] end @@ -73,16 +75,16 @@ subject.assemble_issue_sums expect(subject.direct_sums.count).to eq 2 - expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::COUNT, Constants::OPENED_ISSUE_STATE, 1) - expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::OPENED_ISSUE_STATE, 2) + expect(subject).to have_direct_sum(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) + expect(subject).to have_direct_sum(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) end end context 'with a nonzero count and nonzero weight for multiple states' do let(:fake_data) do [ - { iid: epic_iid, epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 1, issues_weight_sum: 2, parent_id: nil, issues_state_id: Constants::OPENED_ISSUE_STATE }, - { iid: epic_iid, epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 3, issues_weight_sum: 5, parent_id: nil, issues_state_id: Constants::CLOSED_ISSUE_STATE } + { iid: epic_iid, epic_state_id: OPENED_EPIC_STATE, issues_count: 1, issues_weight_sum: 2, parent_id: nil, issues_state_id: OPENED_ISSUE_STATE }, + { iid: epic_iid, epic_state_id: OPENED_EPIC_STATE, issues_count: 3, issues_weight_sum: 5, parent_id: nil, issues_state_id: CLOSED_ISSUE_STATE } ] end @@ -90,21 +92,21 @@ subject.assemble_issue_sums expect(subject.direct_sums.count).to eq 4 - expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::COUNT, Constants::OPENED_ISSUE_STATE, 1) - expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::OPENED_ISSUE_STATE, 2) - expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::COUNT, Constants::CLOSED_ISSUE_STATE, 3) - expect(subject).to have_direct_sum(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::CLOSED_ISSUE_STATE, 5) + expect(subject).to have_direct_sum(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) + expect(subject).to have_direct_sum(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) + expect(subject).to have_direct_sum(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 3) + expect(subject).to have_direct_sum(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 5) end end end end describe '#assemble_epic_sums' do - subject { described_class.new(epic_id, [{ parent_id: nil, epic_state_id: Constants::CLOSED_EPIC_STATE }]) } + subject { described_class.new(epic_id, [{ parent_id: nil, epic_state_id: CLOSED_EPIC_STATE }]) } context 'with a child epic' do let(:child_epic_id) { 45 } - let!(:child_epic_node) { described_class.new(child_epic_id, [{ parent_id: epic_id, epic_state_id: Constants::CLOSED_EPIC_STATE }]) } + let!(:child_epic_node) { described_class.new(child_epic_id, [{ parent_id: epic_id, epic_state_id: CLOSED_EPIC_STATE }]) } before do subject.child_ids << child_epic_id @@ -114,13 +116,13 @@ subject.assemble_epic_sums([child_epic_node]) expect(subject.direct_sums.count).to eq 1 - expect(subject).to have_direct_sum(Constants::EPIC_TYPE, Constants::COUNT, Constants::CLOSED_EPIC_STATE, 1) + expect(subject).to have_direct_sum(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) end end end describe '#calculate_recursive_sums' do - subject { described_class.new(epic_id, [{ parent_id: nil, epic_state_id: Constants::CLOSED_EPIC_STATE }]) } + subject { described_class.new(epic_id, [{ parent_id: nil, epic_state_id: CLOSED_EPIC_STATE }]) } before do allow(subject).to receive(:direct_sums).and_return(direct_sums) @@ -146,7 +148,7 @@ context 'with an issue with 0 weight' do let(:direct_sums) do - [Epics::EpicNode::Sum.new(Constants::COUNT, Constants::CLOSED_EPIC_STATE, Constants::ISSUE_TYPE, 1)] + [Epics::EpicNode::Sum.new(COUNT_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 1)] end it 'returns a SumTotal with only a weight sum' do @@ -160,8 +162,8 @@ context 'with an issue with nonzero weight' do let(:direct_sums) do [ - Epics::EpicNode::Sum.new(Constants::COUNT, Constants::CLOSED_EPIC_STATE, Constants::ISSUE_TYPE, 1), - Epics::EpicNode::Sum.new(Constants::WEIGHT_SUM, Constants::CLOSED_EPIC_STATE, Constants::ISSUE_TYPE, 2) + Epics::EpicNode::Sum.new(COUNT_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 1), + Epics::EpicNode::Sum.new(WEIGHT_SUM_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 2) ] end @@ -179,10 +181,10 @@ let(:tree) do { epic_id => subject, child_epic_id => child_epic_node } end - let(:child_epic_node) { described_class.new(child_epic_id, [{ parent_id: epic_id, epic_state_id: Constants::CLOSED_EPIC_STATE }]) } + let(:child_epic_node) { described_class.new(child_epic_id, [{ parent_id: epic_id, epic_state_id: CLOSED_EPIC_STATE }]) } let(:direct_sums) do [ # only one opened epic, the child - Epics::EpicNode::Sum.new(Constants::COUNT, Constants::OPENED_EPIC_STATE, Constants::EPIC_TYPE, 1) + Epics::EpicNode::Sum.new(COUNT_FACET, OPENED_EPIC_STATE, EPIC_TYPE, 1) ] end @@ -194,8 +196,8 @@ context 'with a child that has issues of nonzero weight' do let(:child_sums) do [ # 1 issue of weight 2 - Epics::EpicNode::Sum.new(Constants::COUNT, Constants::OPENED_ISSUE_STATE, Constants::ISSUE_TYPE, 1), - Epics::EpicNode::Sum.new(Constants::WEIGHT_SUM, Constants::OPENED_ISSUE_STATE, Constants::ISSUE_TYPE, 2) + Epics::EpicNode::Sum.new(COUNT_FACET, OPENED_ISSUE_STATE, ISSUE_TYPE, 1), + Epics::EpicNode::Sum.new(WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, ISSUE_TYPE, 2) ] end diff --git a/ee/spec/graphql/epics/lazy_epic_aggregate_spec.rb b/ee/spec/graphql/epics/lazy_epic_aggregate_spec.rb index 955493f957c811..8752a4f73cba9d 100644 --- a/ee/spec/graphql/epics/lazy_epic_aggregate_spec.rb +++ b/ee/spec/graphql/epics/lazy_epic_aggregate_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe Epics::LazyEpicAggregate do + include_context 'includes EpicAggregate constants' + let(:query_ctx) do {} end @@ -40,7 +42,7 @@ describe '#epic_aggregate' do let(:single_record) do - { iid: 6, issues_count: 4, issues_weight_sum: 9, parent_id: nil, issues_state_id: Constants::OPENED_ISSUE_STATE, epic_state_id: Constants::OPENED_EPIC_STATE } + { iid: 6, issues_count: 4, issues_weight_sum: 9, parent_id: nil, issues_state_id: OPENED_ISSUE_STATE, epic_state_id: OPENED_EPIC_STATE } end let(:epic_info_node) { Epics::EpicNode.new(epic_id, [single_record] ) } @@ -70,9 +72,9 @@ end let(:fake_data) do { - epic_id => [{ epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 2, issues_weight_sum: 5, parent_id: nil, issues_state_id: Constants::OPENED_ISSUE_STATE }], - child_epic_id => [{ epic_state_id: Constants::CLOSED_EPIC_STATE, issues_count: 4, issues_weight_sum: 17, parent_id: epic_id, issues_state_id: Constants::CLOSED_ISSUE_STATE }], - other_epic_id => [{ epic_state_id: Constants::OPENED_EPIC_STATE, issues_count: 0, issues_weight_sum: 0, parent_id: nil, issues_state_id: nil }] # represents an epic with no parent and no issues + epic_id => [{ epic_state_id: OPENED_EPIC_STATE, issues_count: 2, issues_weight_sum: 5, parent_id: nil, issues_state_id: OPENED_ISSUE_STATE }], + child_epic_id => [{ epic_state_id: CLOSED_EPIC_STATE, issues_count: 4, issues_weight_sum: 17, parent_id: epic_id, issues_state_id: CLOSED_ISSUE_STATE }], + other_epic_id => [{ epic_state_id: OPENED_EPIC_STATE, issues_count: 0, issues_weight_sum: 0, parent_id: nil, issues_state_id: nil }] # represents an epic with no parent and no issues } end @@ -106,12 +108,12 @@ lazy_state = subject.instance_variable_get(:@lazy_state) tree = lazy_state[:tree] - expect(tree[epic_id]).to have_direct_sum(Constants::EPIC_TYPE, Constants::COUNT, Constants::CLOSED_EPIC_STATE, 1) - expect(tree[epic_id]).to have_direct_sum(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::OPENED_ISSUE_STATE, 5) - expect(tree[epic_id]).to have_direct_sum(Constants::EPIC_TYPE, Constants::COUNT, Constants::CLOSED_EPIC_STATE, 1) + expect(tree[epic_id]).to have_direct_sum(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) + expect(tree[epic_id]).to have_direct_sum(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 5) + expect(tree[epic_id]).to have_direct_sum(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) - expect(tree[child_epic_id]).to have_direct_sum(Constants::ISSUE_TYPE, Constants::COUNT, Constants::CLOSED_ISSUE_STATE, 4) - expect(tree[child_epic_id]).to have_direct_sum(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::CLOSED_ISSUE_STATE, 17) + expect(tree[child_epic_id]).to have_direct_sum(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 4) + expect(tree[child_epic_id]).to have_direct_sum(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 17) end it 'assembles recursive sums for the parent', :aggregate_failures do @@ -120,11 +122,11 @@ lazy_state = subject.instance_variable_get(:@lazy_state) tree = lazy_state[:tree] - expect(tree[epic_id]).to have_aggregate(Constants::ISSUE_TYPE, Constants::COUNT, Constants::OPENED_ISSUE_STATE, 2) - expect(tree[epic_id]).to have_aggregate(Constants::ISSUE_TYPE, Constants::COUNT, Constants::CLOSED_ISSUE_STATE, 4) - expect(tree[epic_id]).to have_aggregate(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::OPENED_ISSUE_STATE, 5) - expect(tree[epic_id]).to have_aggregate(Constants::ISSUE_TYPE, Constants::WEIGHT_SUM, Constants::CLOSED_ISSUE_STATE, 17) - expect(tree[epic_id]).to have_aggregate(Constants::EPIC_TYPE, Constants::COUNT, Constants::CLOSED_EPIC_STATE, 1) + expect(tree[epic_id]).to have_aggregate(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 2) + expect(tree[epic_id]).to have_aggregate(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 4) + expect(tree[epic_id]).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 5) + expect(tree[epic_id]).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 17) + expect(tree[epic_id]).to have_aggregate(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) end end diff --git a/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb b/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb index 0a8f7180b2295f..1ba369511bf6e4 100644 --- a/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb +++ b/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true RSpec::Matchers.define :have_direct_sum do |type, facet, state, value| - # supports_block_expectations - match do |epic_node_result| expect(epic_node_result).not_to be_nil expect(epic_node_result.direct_sums).not_to be_empty @@ -24,8 +22,6 @@ end RSpec::Matchers.define :have_aggregate do |type, facet, state, value| - supports_block_expectations - match do |epic_node_result| aggregate_object = epic_node_result.aggregate_object_by(facet) expect(aggregate_object.send(method_name(type, state))).to eq value @@ -38,12 +34,12 @@ end def method_name(type, state) - if type == Constants::ISSUE_TYPE - return :opened_issues if state == Constants::OPENED_ISSUE_STATE + if type == ISSUE_TYPE + return :opened_issues if state == OPENED_ISSUE_STATE :closed_issues - elsif type == Constants::EPIC_TYPE - return :opened_epics if state == Constants::OPENED_EPIC_STATE + elsif type == EPIC_TYPE + return :opened_epics if state == OPENED_EPIC_STATE :closed_epics end diff --git a/ee/spec/support/shared_contexts/epic_aggregate_constants.rb b/ee/spec/support/shared_contexts/epic_aggregate_constants.rb new file mode 100644 index 00000000000000..4a661689e11f67 --- /dev/null +++ b/ee/spec/support/shared_contexts/epic_aggregate_constants.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +shared_context 'includes EpicAggregate constants' do + EPIC_TYPE = Epics::AggregateConstants::EPIC_TYPE + ISSUE_TYPE = Epics::AggregateConstants::ISSUE_TYPE + + OPENED_EPIC_STATE = Epics::AggregateConstants::OPENED_EPIC_STATE + CLOSED_EPIC_STATE = Epics::AggregateConstants::CLOSED_EPIC_STATE + OPENED_ISSUE_STATE = Epics::AggregateConstants::OPENED_ISSUE_STATE + CLOSED_ISSUE_STATE = Epics::AggregateConstants::CLOSED_ISSUE_STATE + + WEIGHT_SUM_FACET = Epics::AggregateConstants::WEIGHT_SUM + COUNT_FACET = Epics::AggregateConstants::COUNT +end -- GitLab From 920c4e708c6f8f18dbba475bc33e27e9055d247e Mon Sep 17 00:00:00 2001 From: charlieablett Date: Wed, 26 Feb 2020 22:28:48 +1300 Subject: [PATCH 5/9] Move aggregate logic Move from `app/graphql` to `lib/gitlab/graphql/aggregations/epics` --- ee/app/graphql/ee/gitlab_schema.rb | 2 +- ee/app/graphql/epics/aggregate.rb | 24 --- ee/app/graphql/epics/aggregate_constants.rb | 17 -- ee/app/graphql/epics/count_aggregate.rb | 20 --- ee/app/graphql/epics/epic_node.rb | 91 ---------- ee/app/graphql/epics/lazy_epic_aggregate.rb | 104 ----------- ee/app/graphql/epics/sum_total.rb | 23 --- ee/app/graphql/epics/weight_sum_aggregate.rb | 18 -- .../epics/bulk_epic_aggregate_loader.rb | 48 ----- .../graphql/aggregations/epics/aggregate.rb | 30 ++++ .../graphql/aggregations/epics/constants.rb | 23 +++ .../aggregations/epics/count_aggregate.rb | 26 +++ .../graphql/aggregations/epics/epic_node.rb | 97 ++++++++++ .../aggregations/epics/lazy_epic_aggregate.rb | 110 ++++++++++++ .../graphql/aggregations/epics/sum_total.rb | 29 +++ .../epics/weight_sum_aggregate.rb | 24 +++ .../group/epic/epic_aggregate_query_spec.rb | 166 +++++++++++++----- .../epic_aggregate_constants.rb | 16 +- .../epics/bulk_epic_aggregate_loader.rb | 54 ++++++ .../epics/bulk_epic_aggregate_loader_spec.rb | 2 +- .../aggregations}/epics/epic_node_spec.rb | 14 +- .../epics/lazy_epic_aggregate_spec.rb | 10 +- 22 files changed, 533 insertions(+), 415 deletions(-) delete mode 100644 ee/app/graphql/epics/aggregate.rb delete mode 100644 ee/app/graphql/epics/aggregate_constants.rb delete mode 100644 ee/app/graphql/epics/count_aggregate.rb delete mode 100644 ee/app/graphql/epics/epic_node.rb delete mode 100644 ee/app/graphql/epics/lazy_epic_aggregate.rb delete mode 100644 ee/app/graphql/epics/sum_total.rb delete mode 100644 ee/app/graphql/epics/weight_sum_aggregate.rb delete mode 100644 ee/app/services/epics/bulk_epic_aggregate_loader.rb create mode 100644 ee/lib/gitlab/graphql/aggregations/epics/aggregate.rb create mode 100644 ee/lib/gitlab/graphql/aggregations/epics/constants.rb create mode 100644 ee/lib/gitlab/graphql/aggregations/epics/count_aggregate.rb create mode 100644 ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb create mode 100644 ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb create mode 100644 ee/lib/gitlab/graphql/aggregations/epics/sum_total.rb create mode 100644 ee/lib/gitlab/graphql/aggregations/epics/weight_sum_aggregate.rb create mode 100644 lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader.rb rename {ee/spec/services => lib/gitlab/graphql/aggregations}/epics/bulk_epic_aggregate_loader_spec.rb (98%) rename {ee/spec/graphql => spec/lib/gitlab/graphql/aggregations}/epics/epic_node_spec.rb (89%) rename {ee/spec/graphql => spec/lib/gitlab/graphql/aggregations}/epics/lazy_epic_aggregate_spec.rb (90%) diff --git a/ee/app/graphql/ee/gitlab_schema.rb b/ee/app/graphql/ee/gitlab_schema.rb index 391c966dff4f6a..796c8edd8b94a7 100644 --- a/ee/app/graphql/ee/gitlab_schema.rb +++ b/ee/app/graphql/ee/gitlab_schema.rb @@ -5,7 +5,7 @@ module GitlabSchema extend ActiveSupport::Concern prepended do - lazy_resolve ::Epics::LazyEpicAggregate, :epic_aggregate + lazy_resolve ::Gitlab::Graphql::Aggregations::Epics::LazyEpicAggregate, :epic_aggregate end end end diff --git a/ee/app/graphql/epics/aggregate.rb b/ee/app/graphql/epics/aggregate.rb deleted file mode 100644 index 6c2af81845af37..00000000000000 --- a/ee/app/graphql/epics/aggregate.rb +++ /dev/null @@ -1,24 +0,0 @@ -# frozen_string_literal: true - -module Epics - class Aggregate - include AggregateConstants - - def initialize(values) - raise NotImplementedError.new('Use either CountAggregate or WeightSumAggregate') - end - - private - - def sum_objects(state, type) - matching = @sums.select { |sum| sum.state == state && sum.type == type && sum.facet == facet} - return 0 if @sums.empty? - - matching.map(&:value).reduce(:+) || 0 - end - - def facet - raise NotImplementedError.new('Use either CountAggregate or WeightSumAggregate') - end - end -end diff --git a/ee/app/graphql/epics/aggregate_constants.rb b/ee/app/graphql/epics/aggregate_constants.rb deleted file mode 100644 index 739caadf5017db..00000000000000 --- a/ee/app/graphql/epics/aggregate_constants.rb +++ /dev/null @@ -1,17 +0,0 @@ -# frozen_string_literal: true - -module Epics - module AggregateConstants - ISSUE_TYPE = :issue - EPIC_TYPE = :epic - - CLOSED_ISSUE_STATE = Issue.available_states[:closed].freeze - OPENED_ISSUE_STATE = Issue.available_states[:opened].freeze - - CLOSED_EPIC_STATE = Epic.available_states[:closed].freeze - OPENED_EPIC_STATE = Epic.available_states[:opened].freeze - - COUNT = :count - WEIGHT_SUM = :weight_sum - end -end diff --git a/ee/app/graphql/epics/count_aggregate.rb b/ee/app/graphql/epics/count_aggregate.rb deleted file mode 100644 index 1bec84f04cea60..00000000000000 --- a/ee/app/graphql/epics/count_aggregate.rb +++ /dev/null @@ -1,20 +0,0 @@ -# frozen_string_literal: true - -module Epics - class CountAggregate < Aggregate - attr_accessor :opened_issues, :closed_issues, :opened_epics, :closed_epics - - def initialize(sums) - @sums = sums - - @opened_issues = sum_objects(OPENED_ISSUE_STATE, ISSUE_TYPE) - @closed_issues = sum_objects(CLOSED_ISSUE_STATE, ISSUE_TYPE) - @opened_epics = sum_objects(OPENED_EPIC_STATE, EPIC_TYPE) - @closed_epics = sum_objects(CLOSED_EPIC_STATE, EPIC_TYPE) - end - - def facet - COUNT - end - end -end diff --git a/ee/app/graphql/epics/epic_node.rb b/ee/app/graphql/epics/epic_node.rb deleted file mode 100644 index ffd3316d2c8fae..00000000000000 --- a/ee/app/graphql/epics/epic_node.rb +++ /dev/null @@ -1,91 +0,0 @@ -# frozen_string_literal: true - -# This class represents an Epic's aggregate information (added up counts) about its child epics and direct issues - -module Epics - class EpicNode - include AggregateConstants - - attr_reader :epic_id, :epic_state_id, :epic_info_flat_list, :parent_id, - :direct_sums, # we calculate these and recursively add them - :sum_total - - attr_accessor :child_ids - - def initialize(epic_id, flat_info_list) - # epic aggregate records from the DB loader look like the following: - # { 1 => [{iid: 1, epic_state_id: 1, issues_count: 1, issues_weight_sum: 2, parent_id: nil, state_id: 2}] ... } - # They include the sum of each epic's direct issues, grouped by status, - # so in order to get a sum of the entire tree, we have to add that up recursively - @epic_id = epic_id - @epic_info_flat_list = flat_info_list - @child_ids = [] - @direct_sums = [] - - set_epic_attributes(flat_info_list.first) # there will always be one - end - - def assemble_issue_sums - # this is a representation of the epic's - # direct child issues and epics that have come from the DB - [OPENED_ISSUE_STATE, CLOSED_ISSUE_STATE].each do |issue_state| - matching_issue_state_entry = epic_info_flat_list.find do |epic_info_node| - epic_info_node[:issues_state_id] == issue_state - end || {} - - create_sum_if_needed(WEIGHT_SUM, issue_state, ISSUE_TYPE, matching_issue_state_entry.fetch(:issues_weight_sum, 0)) - create_sum_if_needed(COUNT, issue_state, ISSUE_TYPE, matching_issue_state_entry.fetch(:issues_count, 0)) - end - end - - def assemble_epic_sums(children) - [OPENED_EPIC_STATE, CLOSED_EPIC_STATE].each do |epic_state| - create_sum_if_needed(COUNT, epic_state, EPIC_TYPE, children.select { |node| node.epic_state_id == epic_state }.count) - end - end - - def calculate_recursive_sums(tree) - return sum_total if sum_total - - @sum_total = SumTotal.new - child_ids.each do |child_id| - child = tree[child_id] - # get the child's totals, add to your own - child_sums = child.calculate_recursive_sums(tree).sums - sum_total.add(child_sums) - end - sum_total.add(direct_sums) - sum_total - end - - def aggregate_object_by(facet) - sum_total.by_facet(facet) - end - - def to_s - { epic_id: @epic_id, parent_id: @parent_id, direct_sums: direct_sums, child_ids: child_ids }.to_json - end - - alias_method :inspect, :to_s - alias_method :id, :epic_id - - private - - def create_sum_if_needed(facet, state, type, value) - return if value.nil? || value < 1 - - direct_sums << Sum.new(facet, state, type, value) - end - - def set_epic_attributes(record) - @epic_state_id = record[:epic_state_id] - @parent_id = record[:parent_id] - end - - Sum = Struct.new(:facet, :state, :type, :value) do - def inspect - "" - end - end - end -end diff --git a/ee/app/graphql/epics/lazy_epic_aggregate.rb b/ee/app/graphql/epics/lazy_epic_aggregate.rb deleted file mode 100644 index 088ba8b8c9843f..00000000000000 --- a/ee/app/graphql/epics/lazy_epic_aggregate.rb +++ /dev/null @@ -1,104 +0,0 @@ -# frozen_string_literal: true - -module Epics - class LazyEpicAggregate - include AggregateConstants - - attr_reader :facet, :epic_id, :lazy_state - - # Because facets "count" and "weight_sum" share the same db query, but have a different graphql type object, - # we can separate them and serve only the fields which are requested by the GraphQL query - def initialize(query_ctx, epic_id, aggregate_facet) - @epic_id = epic_id - - raise ArgumentError.new("No aggregate facet provided. Please specify either #{COUNT} or #{WEIGHT_SUM}") unless aggregate_facet.present? - raise ArgumentError.new("Invalid aggregate facet #{aggregate_facet} provided. Please specify either #{COUNT} or #{WEIGHT_SUM}") unless [COUNT, WEIGHT_SUM].include?(aggregate_facet.to_sym) - - @facet = aggregate_facet.to_sym - - # Initialize the loading state for this query, - # or get the previously-initiated state - @lazy_state = query_ctx[:lazy_epic_aggregate] ||= { - pending_ids: Set.new, - tree: {} - } - # Register this ID to be loaded later: - @lazy_state[:pending_ids] << epic_id - end - - # Return the loaded record, hitting the database if needed - def epic_aggregate - # Check if the record was already loaded: - # load from tree by epic - loaded_epic_info_node = @lazy_state[:tree][@epic_id] - - if loaded_epic_info_node - # The pending IDs were already loaded, - # so return the result of that previous load - loaded_epic_info_node.aggregate_object_by(@facet) - else - load_records_into_tree - end - end - - private - - def tree - @lazy_state[:tree] - end - - def load_records_into_tree - # The record hasn't been loaded yet, so - # hit the database with all pending IDs - pending_ids = @lazy_state[:pending_ids].to_a - - # Fire off the db query and get the results (grouped by epic_id and facet) - raw_epic_aggregates = Epics::BulkEpicAggregateLoader.new(epic_ids: pending_ids).execute - - # Assemble the tree and sum everything - create_structure_from(raw_epic_aggregates) - - @lazy_state[:pending_ids].clear - - # Now, get the matching node and return its aggregate depending on the facet: - epic_node = @lazy_state[:tree][@epic_id] - epic_node.aggregate_object_by(@facet) - end - - def create_structure_from(aggregate_records) - # create EpicNode object for each epic id - aggregate_records.each do |epic_id, aggregates| - next if aggregates.nil? || aggregates.empty? - - new_node = EpicNode.new(epic_id, aggregates) - tree[epic_id] = new_node - end - - associate_parents_and_children - assemble_direct_sums - calculate_recursive_sums - end - - # each of the methods below are done one after the other - def associate_parents_and_children - tree.each do |epic_id, node| - node.child_ids = tree.select { |_, child_node| epic_id == child_node.parent_id }.keys - end - end - - def assemble_direct_sums - tree.each do |_, node| - node.assemble_issue_sums - - node_children = tree.select { |_, child_node| node.epic_id == child_node.parent_id }.values - node.assemble_epic_sums(node_children) - end - end - - def calculate_recursive_sums - tree.each do |_, node| - node.calculate_recursive_sums(tree) - end - end - end -end diff --git a/ee/app/graphql/epics/sum_total.rb b/ee/app/graphql/epics/sum_total.rb deleted file mode 100644 index a682b94722b765..00000000000000 --- a/ee/app/graphql/epics/sum_total.rb +++ /dev/null @@ -1,23 +0,0 @@ -# frozen_string_literal: true - -module Epics - class SumTotal - include AggregateConstants - - attr_accessor :sums - - def initialize - @sums = [] - end - - def add(other_sums) - sums.concat(other_sums) - end - - def by_facet(facet) - return ::Epics::CountAggregate.new(sums) if facet == COUNT - - ::Epics::WeightSumAggregate.new(sums) - end - end -end diff --git a/ee/app/graphql/epics/weight_sum_aggregate.rb b/ee/app/graphql/epics/weight_sum_aggregate.rb deleted file mode 100644 index 24a55509c54584..00000000000000 --- a/ee/app/graphql/epics/weight_sum_aggregate.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -module Epics - class WeightSumAggregate < Aggregate - attr_accessor :opened_issues, :closed_issues - - def initialize(sums) - @sums = sums - - @opened_issues = sum_objects(OPENED_ISSUE_STATE, :issue) - @closed_issues = sum_objects(CLOSED_ISSUE_STATE, :issue) - end - - def facet - WEIGHT_SUM - end - end -end diff --git a/ee/app/services/epics/bulk_epic_aggregate_loader.rb b/ee/app/services/epics/bulk_epic_aggregate_loader.rb deleted file mode 100644 index 7671efcf8c0748..00000000000000 --- a/ee/app/services/epics/bulk_epic_aggregate_loader.rb +++ /dev/null @@ -1,48 +0,0 @@ -# frozen_string_literal: true - -module Epics - class BulkEpicAggregateLoader - include AggregateConstants - - # This class retrieves each epic and its child epics recursively - # It allows us to recreate the epic tree structure in POROs - def initialize(epic_ids:) - @results = {} - @target_epic_ids = epic_ids.present? ? [epic_ids].flatten.compact : [] - end - - # rubocop: disable CodeReuse/ActiveRecord - def execute - return {} unless target_epic_ids.any? - - # We do a left outer join in order to capture epics with no issues - # This is so we can aggregate the epic counts for every epic - raw_results = ::Gitlab::ObjectHierarchy.new(Epic.where(id: target_epic_ids)).base_and_descendants - .left_joins(epic_issues: :issue) - .group("issues.state_id", "epics.id", "epics.iid", "epics.parent_id", "epics.state_id") - .select("epics.id, epics.iid, epics.parent_id, epics.state_id AS epic_state_id, issues.state_id AS issues_state_id, COUNT(issues) AS issues_count, SUM(COALESCE(issues.weight, 0)) AS issues_weight_sum") - - raw_results = raw_results.map(&:attributes).map(&:with_indifferent_access) - group_by_epic_id(raw_results) - @results - end - # rubocop: enable CodeReuse/ActiveRecord - - private - - attr_reader :target_epic_ids, :results - - def group_by_epic_id(raw_records) - # for each id, populate with matching records - # change from a series of { id: x ... } to { x => [{...}, {...}] } - raw_records.map { |r| r[:id] }.uniq.each do |epic_id| - records = [] - matching_records = raw_records.select { |record| record[:id] == epic_id } - matching_records.each do |record| - records << record.except(:id).to_h.with_indifferent_access - end - @results[epic_id] = records - end - end - end -end diff --git a/ee/lib/gitlab/graphql/aggregations/epics/aggregate.rb b/ee/lib/gitlab/graphql/aggregations/epics/aggregate.rb new file mode 100644 index 00000000000000..63faff7c979248 --- /dev/null +++ b/ee/lib/gitlab/graphql/aggregations/epics/aggregate.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Aggregations + module Epics + class Aggregate + include Constants + + def initialize(values) + raise NotImplementedError.new('Use either CountAggregate or WeightSumAggregate') + end + + private + + def sum_objects(state, type) + matching = @sums.select { |sum| sum.state == state && sum.type == type && sum.facet == facet} + return 0 if @sums.empty? + + matching.map(&:value).reduce(:+) || 0 + end + + def facet + raise NotImplementedError.new('Use either CountAggregate or WeightSumAggregate') + end + end + end + end + end +end diff --git a/ee/lib/gitlab/graphql/aggregations/epics/constants.rb b/ee/lib/gitlab/graphql/aggregations/epics/constants.rb new file mode 100644 index 00000000000000..27d7e69c7c92d0 --- /dev/null +++ b/ee/lib/gitlab/graphql/aggregations/epics/constants.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Aggregations + module Epics + module Constants + ISSUE_TYPE = :issue + EPIC_TYPE = :epic + + CLOSED_ISSUE_STATE = Issue.available_states[:closed].freeze + OPENED_ISSUE_STATE = Issue.available_states[:opened].freeze + + CLOSED_EPIC_STATE = Epic.available_states[:closed].freeze + OPENED_EPIC_STATE = Epic.available_states[:opened].freeze + + COUNT = :count + WEIGHT_SUM = :weight_sum + end + end + end + end +end diff --git a/ee/lib/gitlab/graphql/aggregations/epics/count_aggregate.rb b/ee/lib/gitlab/graphql/aggregations/epics/count_aggregate.rb new file mode 100644 index 00000000000000..d0ad89da652094 --- /dev/null +++ b/ee/lib/gitlab/graphql/aggregations/epics/count_aggregate.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Aggregations + module Epics + class CountAggregate < Aggregate + attr_accessor :opened_issues, :closed_issues, :opened_epics, :closed_epics + + def initialize(sums) + @sums = sums + + @opened_issues = sum_objects(OPENED_ISSUE_STATE, ISSUE_TYPE) + @closed_issues = sum_objects(CLOSED_ISSUE_STATE, ISSUE_TYPE) + @opened_epics = sum_objects(OPENED_EPIC_STATE, EPIC_TYPE) + @closed_epics = sum_objects(CLOSED_EPIC_STATE, EPIC_TYPE) + end + + def facet + COUNT + end + end + end + end + end +end diff --git a/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb b/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb new file mode 100644 index 00000000000000..14e061cca53d99 --- /dev/null +++ b/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +# This class represents an Epic's aggregate information (added up counts) about its child epics and direct issues + +module Gitlab + module Graphql + module Aggregations + module Epics + class EpicNode + include Constants + + attr_reader :epic_id, :epic_state_id, :epic_info_flat_list, :parent_id, + :direct_sums, # we calculate these and recursively add them + :sum_total + + attr_accessor :child_ids + + def initialize(epic_id, flat_info_list) + # epic aggregate records from the DB loader look like the following: + # { 1 => [{iid: 1, epic_state_id: 1, issues_count: 1, issues_weight_sum: 2, parent_id: nil, state_id: 2}] ... } + # They include the sum of each epic's direct issues, grouped by status, + # so in order to get a sum of the entire tree, we have to add that up recursively + @epic_id = epic_id + @epic_info_flat_list = flat_info_list + @child_ids = [] + @direct_sums = [] + + set_epic_attributes(flat_info_list.first) # there will always be one + end + + def assemble_issue_sums + # this is a representation of the epic's + # direct child issues and epics that have come from the DB + [OPENED_ISSUE_STATE, CLOSED_ISSUE_STATE].each do |issue_state| + matching_issue_state_entry = epic_info_flat_list.find do |epic_info_node| + epic_info_node[:issues_state_id] == issue_state + end || {} + + create_sum_if_needed(WEIGHT_SUM, issue_state, ISSUE_TYPE, matching_issue_state_entry.fetch(:issues_weight_sum, 0)) + create_sum_if_needed(COUNT, issue_state, ISSUE_TYPE, matching_issue_state_entry.fetch(:issues_count, 0)) + end + end + + def assemble_epic_sums(children) + [OPENED_EPIC_STATE, CLOSED_EPIC_STATE].each do |epic_state| + create_sum_if_needed(COUNT, epic_state, EPIC_TYPE, children.select { |node| node.epic_state_id == epic_state }.count) + end + end + + def calculate_recursive_sums(tree) + return sum_total if sum_total + + @sum_total = SumTotal.new + child_ids.each do |child_id| + child = tree[child_id] + # get the child's totals, add to your own + child_sums = child.calculate_recursive_sums(tree).sums + sum_total.add(child_sums) + end + sum_total.add(direct_sums) + sum_total + end + + def aggregate_object_by(facet) + sum_total.by_facet(facet) + end + + def to_s + { epic_id: @epic_id, parent_id: @parent_id, direct_sums: direct_sums, child_ids: child_ids }.to_json + end + + alias_method :inspect, :to_s + alias_method :id, :epic_id + + private + + def create_sum_if_needed(facet, state, type, value) + return if value.nil? || value < 1 + + direct_sums << Sum.new(facet, state, type, value) + end + + def set_epic_attributes(record) + @epic_state_id = record[:epic_state_id] + @parent_id = record[:parent_id] + end + + Sum = Struct.new(:facet, :state, :type, :value) do + def inspect + "" + end + end + end + end + end + end +end diff --git a/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb b/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb new file mode 100644 index 00000000000000..5196a24f852b3b --- /dev/null +++ b/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Aggregations + module Epics + class LazyEpicAggregate + include Constants + + attr_reader :facet, :epic_id, :lazy_state + + # Because facets "count" and "weight_sum" share the same db query, but have a different graphql type object, + # we can separate them and serve only the fields which are requested by the GraphQL query + def initialize(query_ctx, epic_id, aggregate_facet) + @epic_id = epic_id + + raise ArgumentError.new("No aggregate facet provided. Please specify either #{COUNT} or #{WEIGHT_SUM}") unless aggregate_facet.present? + raise ArgumentError.new("Invalid aggregate facet #{aggregate_facet} provided. Please specify either #{COUNT} or #{WEIGHT_SUM}") unless [COUNT, WEIGHT_SUM].include?(aggregate_facet.to_sym) + + @facet = aggregate_facet.to_sym + + # Initialize the loading state for this query, + # or get the previously-initiated state + @lazy_state = query_ctx[:lazy_epic_aggregate] ||= { + pending_ids: Set.new, + tree: {} + } + # Register this ID to be loaded later: + @lazy_state[:pending_ids] << epic_id + end + + # Return the loaded record, hitting the database if needed + def epic_aggregate + # Check if the record was already loaded: + # load from tree by epic + loaded_epic_info_node = @lazy_state[:tree][@epic_id] + + if loaded_epic_info_node + # The pending IDs were already loaded, + # so return the result of that previous load + loaded_epic_info_node.aggregate_object_by(@facet) + else + load_records_into_tree + end + end + + private + + def tree + @lazy_state[:tree] + end + + def load_records_into_tree + # The record hasn't been loaded yet, so + # hit the database with all pending IDs + pending_ids = @lazy_state[:pending_ids].to_a + + # Fire off the db query and get the results (grouped by epic_id and facet) + raw_epic_aggregates = Epics::BulkEpicAggregateLoader.new(epic_ids: pending_ids).execute + + # Assemble the tree and sum everything + create_structure_from(raw_epic_aggregates) + + @lazy_state[:pending_ids].clear + + # Now, get the matching node and return its aggregate depending on the facet: + epic_node = @lazy_state[:tree][@epic_id] + epic_node.aggregate_object_by(@facet) + end + + def create_structure_from(aggregate_records) + # create EpicNode object for each epic id + aggregate_records.each do |epic_id, aggregates| + next if aggregates.nil? || aggregates.empty? + + new_node = EpicNode.new(epic_id, aggregates) + tree[epic_id] = new_node + end + + associate_parents_and_children + assemble_direct_sums + calculate_recursive_sums + end + + # each of the methods below are done one after the other + def associate_parents_and_children + tree.each do |epic_id, node| + node.child_ids = tree.select { |_, child_node| epic_id == child_node.parent_id }.keys + end + end + + def assemble_direct_sums + tree.each do |_, node| + node.assemble_issue_sums + + node_children = tree.select { |_, child_node| node.epic_id == child_node.parent_id }.values + node.assemble_epic_sums(node_children) + end + end + + def calculate_recursive_sums + tree.each do |_, node| + node.calculate_recursive_sums(tree) + end + end + end + end + end + end +end diff --git a/ee/lib/gitlab/graphql/aggregations/epics/sum_total.rb b/ee/lib/gitlab/graphql/aggregations/epics/sum_total.rb new file mode 100644 index 00000000000000..7da1df1d7dec43 --- /dev/null +++ b/ee/lib/gitlab/graphql/aggregations/epics/sum_total.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Aggregations + module Epics + class SumTotal + include Constants + + attr_accessor :sums + + def initialize + @sums = [] + end + + def add(other_sums) + sums.concat(other_sums) + end + + def by_facet(facet) + return CountAggregate.new(sums) if facet == COUNT + + WeightSumAggregate.new(sums) + end + end + end + end + end +end diff --git a/ee/lib/gitlab/graphql/aggregations/epics/weight_sum_aggregate.rb b/ee/lib/gitlab/graphql/aggregations/epics/weight_sum_aggregate.rb new file mode 100644 index 00000000000000..0b4481e8b5ad8c --- /dev/null +++ b/ee/lib/gitlab/graphql/aggregations/epics/weight_sum_aggregate.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Aggregations + module Epics + class WeightSumAggregate < Aggregate + attr_accessor :opened_issues, :closed_issues + + def initialize(sums) + @sums = sums + + @opened_issues = sum_objects(OPENED_ISSUE_STATE, :issue) + @closed_issues = sum_objects(CLOSED_ISSUE_STATE, :issue) + end + + def facet + WEIGHT_SUM + end + end + end + end + end +end diff --git a/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb b/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb index 5846e9076d7904..562c4062669f8c 100644 --- a/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb +++ b/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb @@ -2,26 +2,16 @@ require 'spec_helper' -describe 'Query epic aggregates (count and weight)' do +describe 'Epic aggregates (count and weight)' do include GraphqlHelpers let_it_be(:current_user) { create(:user) } let_it_be(:group) { create(:group, :public) } - let_it_be(:subgroup) { create(:group, :private, parent: group)} - let_it_be(:subsubgroup) { create(:group, :private, parent: subgroup)} - - let_it_be(:project) { create(:project, namespace: group) } - let_it_be(:parent_epic) { create(:epic, id: 1, group: group, title: 'parent epic') } - let_it_be(:epic_with_issues) { create(:epic, id: 2, group: subgroup, parent: parent_epic, title: 'epic with issues') } - let_it_be(:epic_without_issues) { create(:epic, :closed, id: 3, group: subgroup, parent: parent_epic, title: 'epic without issues') } - let_it_be(:closed_epic) { create(:epic, :closed, id: 4, group: subgroup, parent: parent_epic, title: 'closed epic') } - let_it_be(:issue1) { create(:issue, project: project, weight: 5, state: :opened) } - let_it_be(:issue2) { create(:issue, project: project, weight: 7, state: :closed) } - - let_it_be(:epic_issue1) { create(:epic_issue, epic: epic_with_issues, issue: issue1) } - let_it_be(:epic_issue2) { create(:epic_issue, epic: epic_with_issues, issue: issue2) } + let(:query) do + graphql_query_for('group', { fullPath: group.full_path }, query_graphql_field('epics', { iid: parent_epic.iid }, epic_aggregates_query)) + end let(:epic_aggregates_query) do <<~QUERY @@ -40,50 +30,130 @@ QUERY end - let(:query) do - graphql_query_for('group', { fullPath: group.full_path }, query_graphql_field('epics', { iid: parent_epic.iid }, epic_aggregates_query)) - end - - subject { graphql_data.dig('group', 'epics', 'nodes') } - before do - group.add_developer(current_user) stub_licensed_features(epics: true) - post_graphql(query, current_user: current_user) end - it_behaves_like 'a working graphql query' + context 'count and weight totals' do + subject { graphql_data.dig('group', 'epics', 'nodes') } - it 'returns the epic counts' do - epic_count_result = { - "openedEpics" => 1, - "closedEpics" => 2 - } + let_it_be(:subgroup) { create(:group, :private, parent: group)} + let_it_be(:subsubgroup) { create(:group, :private, parent: subgroup)} - is_expected.to include( - a_hash_including('descendantCounts' => a_hash_including(epic_count_result)) - ) - end + let_it_be(:project) { create(:project, namespace: group) } - it 'returns the issue counts' do - issue_count_result = { - "openedIssues" => 1, - "closedIssues" => 1 - } + let_it_be(:epic_with_issues) { create(:epic, id: 2, group: subgroup, parent: parent_epic, title: 'epic with issues') } + let_it_be(:epic_without_issues) { create(:epic, :closed, id: 3, group: subgroup, parent: parent_epic, title: 'epic without issues') } + let_it_be(:closed_epic) { create(:epic, :closed, id: 4, group: subgroup, parent: parent_epic, title: 'closed epic') } + + let_it_be(:issue1) { create(:issue, project: project, weight: 5, state: :opened) } + let_it_be(:issue2) { create(:issue, project: project, weight: 7, state: :closed) } + + let_it_be(:epic_issue1) { create(:epic_issue, epic: epic_with_issues, issue: issue1) } + let_it_be(:epic_issue2) { create(:epic_issue, epic: epic_with_issues, issue: issue2) } - is_expected.to include( - a_hash_including('descendantCounts' => a_hash_including(issue_count_result)) - ) + before do + group.add_developer(current_user) + post_graphql(query, current_user: current_user) + end + + shared_examples 'counts properly' do + it_behaves_like 'a working graphql query' + + it 'returns the epic counts' do + epic_count_result = { + "openedEpics" => 1, + "closedEpics" => 2 + } + + is_expected.to include( + a_hash_including('descendantCounts' => a_hash_including(epic_count_result)) + ) + end + + it 'returns the issue counts' do + issue_count_result = { + "openedIssues" => 1, + "closedIssues" => 1 + } + + is_expected.to include( + a_hash_including('descendantCounts' => a_hash_including(issue_count_result)) + ) + end end - it 'returns the weights' do - descendant_weight_result = { - "openedIssues" => 5, - "closedIssues" => 7 - } + context 'with feature flag enabled' do + before do + stub_feature_flags(unfiltered_epic_aggregates: true) + end + + it 'uses the LazyEpicAggregate service' do + # one for count, one for weight_sum, even though the share the same tree state as part of the context + expect(Epics::LazyEpicAggregate).to receive(:new).twice + + post_graphql(query, current_user: current_user) + end + + it_behaves_like 'counts properly' + + it 'returns the weights' do + descendant_weight_result = { + "openedIssues" => 5, + "closedIssues" => 7 + } + + is_expected.to include( + a_hash_including('descendantWeightSum' => a_hash_including(descendant_weight_result)) + ) + end + end - is_expected.to include( - a_hash_including('descendantWeightSum' => a_hash_including(descendant_weight_result)) - ) + context 'with feature flag disabled' do + before do + stub_feature_flags(unfiltered_epic_aggregates: false) + end + + context 'when requesting counts' do + let(:epic_aggregates_query) do + <<~QUERY + nodes { + descendantCounts { + openedEpics + closedEpics + openedIssues + closedIssues + } + } + QUERY + end + + it 'uses the DescendantCountService' do + expect(Epics::DescendantCountService).to receive(:new) + + post_graphql(query, current_user: current_user) + end + + it_behaves_like 'counts properly' + end + + context 'when requesting weights' do + let(:epic_aggregates_query) do + <<~QUERY + nodes { + descendantWeightSum { + openedIssues + closedIssues + } + } + QUERY + end + + it 'returns an error' do + post_graphql(query, current_user: current_user) + + expect_graphql_errors_to_include /Field 'descendantWeightSum' doesn't exist on type 'Epic/ + end + end end end diff --git a/ee/spec/support/shared_contexts/epic_aggregate_constants.rb b/ee/spec/support/shared_contexts/epic_aggregate_constants.rb index 4a661689e11f67..6bacad0fff6d09 100644 --- a/ee/spec/support/shared_contexts/epic_aggregate_constants.rb +++ b/ee/spec/support/shared_contexts/epic_aggregate_constants.rb @@ -1,14 +1,14 @@ # frozen_string_literal: true shared_context 'includes EpicAggregate constants' do - EPIC_TYPE = Epics::AggregateConstants::EPIC_TYPE - ISSUE_TYPE = Epics::AggregateConstants::ISSUE_TYPE + EPIC_TYPE = Gitlab::Graphql::Aggregations::Epics::Constants::EPIC_TYPE + ISSUE_TYPE = Gitlab::Graphql::Aggregations::Epics::Constants::ISSUE_TYPE - OPENED_EPIC_STATE = Epics::AggregateConstants::OPENED_EPIC_STATE - CLOSED_EPIC_STATE = Epics::AggregateConstants::CLOSED_EPIC_STATE - OPENED_ISSUE_STATE = Epics::AggregateConstants::OPENED_ISSUE_STATE - CLOSED_ISSUE_STATE = Epics::AggregateConstants::CLOSED_ISSUE_STATE + OPENED_EPIC_STATE = Gitlab::Graphql::Aggregations::Epics::Constants::OPENED_EPIC_STATE + CLOSED_EPIC_STATE = Gitlab::Graphql::Aggregations::Epics::Constants::CLOSED_EPIC_STATE + OPENED_ISSUE_STATE = Gitlab::Graphql::Aggregations::Epics::Constants::OPENED_ISSUE_STATE + CLOSED_ISSUE_STATE = Gitlab::Graphql::Aggregations::Epics::Constants::CLOSED_ISSUE_STATE - WEIGHT_SUM_FACET = Epics::AggregateConstants::WEIGHT_SUM - COUNT_FACET = Epics::AggregateConstants::COUNT + WEIGHT_SUM_FACET = Gitlab::Graphql::Aggregations::Epics::Constants::WEIGHT_SUM + COUNT_FACET = Gitlab::Graphql::Aggregations::Epics::Constants::COUNT end diff --git a/lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader.rb b/lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader.rb new file mode 100644 index 00000000000000..60795d2a71048a --- /dev/null +++ b/lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Aggregations + module Epics + class BulkEpicAggregateLoader + include Constants + + # This class retrieves each epic and its child epics recursively + # It allows us to recreate the epic tree structure in POROs + def initialize(epic_ids:) + @results = {} + @target_epic_ids = epic_ids.present? ? [epic_ids].flatten.compact : [] + end + + # rubocop: disable CodeReuse/ActiveRecord + def execute + return {} unless target_epic_ids.any? + + # We do a left outer join in order to capture epics with no issues + # This is so we can aggregate the epic counts for every epic + raw_results = ::Gitlab::ObjectHierarchy.new(Epic.where(id: target_epic_ids)).base_and_descendants + .left_joins(epic_issues: :issue) + .group("issues.state_id", "epics.id", "epics.iid", "epics.parent_id", "epics.state_id") + .select("epics.id, epics.iid, epics.parent_id, epics.state_id AS epic_state_id, issues.state_id AS issues_state_id, COUNT(issues) AS issues_count, SUM(COALESCE(issues.weight, 0)) AS issues_weight_sum") + + raw_results = raw_results.map(&:attributes).map(&:with_indifferent_access) + group_by_epic_id(raw_results) + @results + end + # rubocop: enable CodeReuse/ActiveRecord + + private + + attr_reader :target_epic_ids, :results + + def group_by_epic_id(raw_records) + # for each id, populate with matching records + # change from a series of { id: x ... } to { x => [{...}, {...}] } + raw_records.map { |r| r[:id] }.uniq.each do |epic_id| + records = [] + matching_records = raw_records.select { |record| record[:id] == epic_id } + matching_records.each do |record| + records << record.except(:id).to_h.with_indifferent_access + end + @results[epic_id] = records + end + end + end + end + end + end +end diff --git a/ee/spec/services/epics/bulk_epic_aggregate_loader_spec.rb b/lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader_spec.rb similarity index 98% rename from ee/spec/services/epics/bulk_epic_aggregate_loader_spec.rb rename to lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader_spec.rb index 312b3e75b955d3..1e9eb7d2027132 100644 --- a/ee/spec/services/epics/bulk_epic_aggregate_loader_spec.rb +++ b/lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe Epics::BulkEpicAggregateLoader do +describe Gitlab::Graphql::Aggregations::Epics::BulkEpicAggregateLoader do let_it_be(:closed_issue_state_id) { Issue.available_states[:closed] } let_it_be(:opened_issue_state_id) { Issue.available_states[:opened] } diff --git a/ee/spec/graphql/epics/epic_node_spec.rb b/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb similarity index 89% rename from ee/spec/graphql/epics/epic_node_spec.rb rename to spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb index 245861846894b9..5e6e3e2b3e5e72 100644 --- a/ee/spec/graphql/epics/epic_node_spec.rb +++ b/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe Epics::EpicNode do +describe Gitlab::Graphql::Aggregations::Epics::EpicNode do include_context 'includes EpicAggregate constants' let(:epic_id) { 34 } @@ -148,7 +148,7 @@ context 'with an issue with 0 weight' do let(:direct_sums) do - [Epics::EpicNode::Sum.new(COUNT_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 1)] + [Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 1)] end it 'returns a SumTotal with only a weight sum' do @@ -162,8 +162,8 @@ context 'with an issue with nonzero weight' do let(:direct_sums) do [ - Epics::EpicNode::Sum.new(COUNT_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 1), - Epics::EpicNode::Sum.new(WEIGHT_SUM_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 2) + Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 1), + Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(WEIGHT_SUM_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 2) ] end @@ -184,7 +184,7 @@ let(:child_epic_node) { described_class.new(child_epic_id, [{ parent_id: epic_id, epic_state_id: CLOSED_EPIC_STATE }]) } let(:direct_sums) do [ # only one opened epic, the child - Epics::EpicNode::Sum.new(COUNT_FACET, OPENED_EPIC_STATE, EPIC_TYPE, 1) + Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, OPENED_EPIC_STATE, EPIC_TYPE, 1) ] end @@ -196,8 +196,8 @@ context 'with a child that has issues of nonzero weight' do let(:child_sums) do [ # 1 issue of weight 2 - Epics::EpicNode::Sum.new(COUNT_FACET, OPENED_ISSUE_STATE, ISSUE_TYPE, 1), - Epics::EpicNode::Sum.new(WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, ISSUE_TYPE, 2) + Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, OPENED_ISSUE_STATE, ISSUE_TYPE, 1), + Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, ISSUE_TYPE, 2) ] end diff --git a/ee/spec/graphql/epics/lazy_epic_aggregate_spec.rb b/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb similarity index 90% rename from ee/spec/graphql/epics/lazy_epic_aggregate_spec.rb rename to spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb index 8752a4f73cba9d..2a601195f32d85 100644 --- a/ee/spec/graphql/epics/lazy_epic_aggregate_spec.rb +++ b/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe Epics::LazyEpicAggregate do +describe Gitlab::Graphql::Aggregations::Epics::LazyEpicAggregate do include_context 'includes EpicAggregate constants' let(:query_ctx) do @@ -44,7 +44,7 @@ let(:single_record) do { iid: 6, issues_count: 4, issues_weight_sum: 9, parent_id: nil, issues_state_id: OPENED_ISSUE_STATE, epic_state_id: OPENED_EPIC_STATE } end - let(:epic_info_node) { Epics::EpicNode.new(epic_id, [single_record] ) } + let(:epic_info_node) { Gitlab::Graphql::Aggregations::Epics::EpicNode.new(epic_id, [single_record] ) } subject { described_class.new(query_ctx, epic_id, :count) } @@ -59,7 +59,7 @@ it 'does not make the query again' do expect(epic_info_node).to receive(:aggregate_object_by).with(subject.facet) - expect(Epics::BulkEpicAggregateLoader).not_to receive(:new) + expect(Gitlab::Graphql::Aggregations::Epics::BulkEpicAggregateLoader).not_to receive(:new) subject.epic_aggregate end @@ -79,8 +79,8 @@ end before do - allow(Epics::EpicNode).to receive(:aggregate_object_by).and_call_original - expect_any_instance_of(Epics::BulkEpicAggregateLoader).to receive(:execute).and_return(fake_data) + allow(Gitlab::Graphql::Aggregations::Epics::EpicNode).to receive(:aggregate_object_by).and_call_original + expect_any_instance_of(Gitlab::Graphql::Aggregations::Epics::BulkEpicAggregateLoader).to receive(:execute).and_return(fake_data) end it 'clears the pending IDs' do -- GitLab From a883852c3d06421257ebeda2141e9a24341b5012 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Thu, 27 Feb 2020 23:29:49 +1300 Subject: [PATCH 6/9] Simplify auxiliary classes - Remove utility classes - Store more state in EpicNode --- ee/app/graphql/types/epic_type.rb | 8 +- .../graphql/aggregations/epics/aggregate.rb | 30 ---- .../aggregations/epics/count_aggregate.rb | 26 --- .../graphql/aggregations/epics/epic_node.rb | 104 +++++++++--- .../aggregations/epics/lazy_epic_aggregate.rb | 27 ++-- .../graphql/aggregations/epics/sum_total.rb | 29 ---- .../epics/weight_sum_aggregate.rb | 24 --- .../loaders/bulk_epic_aggregate_loader.rb | 52 ++++++ ee/spec/graphql/epics/aggregate_spec.rb | 73 --------- .../aggregations/epics/epic_node_spec.rb | 121 +++++++------- .../epics/lazy_epic_aggregate_spec.rb | 33 ++-- .../bulk_epic_aggregate_loader_spec.rb | 2 +- .../group/epic/epic_aggregate_query_spec.rb | 149 +++++++++--------- .../matchers/ee/epic_aggregate_matchers.rb | 22 +-- .../epics/bulk_epic_aggregate_loader.rb | 54 ------- 15 files changed, 322 insertions(+), 432 deletions(-) delete mode 100644 ee/lib/gitlab/graphql/aggregations/epics/aggregate.rb delete mode 100644 ee/lib/gitlab/graphql/aggregations/epics/count_aggregate.rb delete mode 100644 ee/lib/gitlab/graphql/aggregations/epics/sum_total.rb delete mode 100644 ee/lib/gitlab/graphql/aggregations/epics/weight_sum_aggregate.rb create mode 100644 ee/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader.rb delete mode 100644 ee/spec/graphql/epics/aggregate_spec.rb rename {spec => ee/spec}/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb (58%) rename {spec => ee/spec}/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb (72%) rename {lib/gitlab/graphql/aggregations/epics => ee/spec/lib/gitlab/graphql/loaders}/bulk_epic_aggregate_loader_spec.rb (98%) delete mode 100644 lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader.rb diff --git a/ee/app/graphql/types/epic_type.rb b/ee/app/graphql/types/epic_type.rb index 45b0d61920d8d5..72860ca1c758df 100644 --- a/ee/app/graphql/types/epic_type.rb +++ b/ee/app/graphql/types/epic_type.rb @@ -2,6 +2,8 @@ module Types class EpicType < BaseObject + include ::Gitlab::Graphql::Aggregations::Epics::Constants + graphql_name 'Epic' description 'Represents an epic.' @@ -124,17 +126,17 @@ class EpicType < BaseObject description: 'Number of open and closed descendant epics and issues', resolve: -> (epic, args, ctx) do if Feature.enabled?(:unfiltered_epic_aggregates) - Epics::LazyEpicAggregate.new(ctx, epic.id, Epics::LazyEpicAggregate::COUNT) + Gitlab::Graphql::Aggregations::Epics::LazyEpicAggregate.new(ctx, epic.id, COUNT) else Epics::DescendantCountService.new(epic, ctx[:current_user]) end end field :descendant_weight_sum, Types::EpicDescendantWeightSumType, null: true, complexity: 10, - description: "Total weight of open and closed descendant epic's issues", + description: "Total weight of open and closed issues in the epic and its descendants", feature_flag: :unfiltered_epic_aggregates, resolve: -> (epic, args, ctx) do - Epics::LazyEpicAggregate.new(ctx, epic.id, Epics::LazyEpicAggregate::WEIGHT_SUM) + Gitlab::Graphql::Aggregations::Epics::LazyEpicAggregate.new(ctx, epic.id, WEIGHT_SUM) end field :health_status, diff --git a/ee/lib/gitlab/graphql/aggregations/epics/aggregate.rb b/ee/lib/gitlab/graphql/aggregations/epics/aggregate.rb deleted file mode 100644 index 63faff7c979248..00000000000000 --- a/ee/lib/gitlab/graphql/aggregations/epics/aggregate.rb +++ /dev/null @@ -1,30 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - module Aggregations - module Epics - class Aggregate - include Constants - - def initialize(values) - raise NotImplementedError.new('Use either CountAggregate or WeightSumAggregate') - end - - private - - def sum_objects(state, type) - matching = @sums.select { |sum| sum.state == state && sum.type == type && sum.facet == facet} - return 0 if @sums.empty? - - matching.map(&:value).reduce(:+) || 0 - end - - def facet - raise NotImplementedError.new('Use either CountAggregate or WeightSumAggregate') - end - end - end - end - end -end diff --git a/ee/lib/gitlab/graphql/aggregations/epics/count_aggregate.rb b/ee/lib/gitlab/graphql/aggregations/epics/count_aggregate.rb deleted file mode 100644 index d0ad89da652094..00000000000000 --- a/ee/lib/gitlab/graphql/aggregations/epics/count_aggregate.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - module Aggregations - module Epics - class CountAggregate < Aggregate - attr_accessor :opened_issues, :closed_issues, :opened_epics, :closed_epics - - def initialize(sums) - @sums = sums - - @opened_issues = sum_objects(OPENED_ISSUE_STATE, ISSUE_TYPE) - @closed_issues = sum_objects(CLOSED_ISSUE_STATE, ISSUE_TYPE) - @opened_epics = sum_objects(OPENED_EPIC_STATE, EPIC_TYPE) - @closed_epics = sum_objects(CLOSED_EPIC_STATE, EPIC_TYPE) - end - - def facet - COUNT - end - end - end - end - end -end diff --git a/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb b/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb index 14e061cca53d99..6f3bdb20048088 100644 --- a/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb +++ b/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb @@ -1,36 +1,38 @@ # frozen_string_literal: true -# This class represents an Epic's aggregate information (added up counts) about its child epics and direct issues +# This class represents an Epic's aggregate information (added up counts) about its child epics and immediate issues module Gitlab module Graphql module Aggregations module Epics class EpicNode - include Constants + include ::Gitlab::Graphql::Aggregations::Epics::Constants + include Gitlab::Utils::StrongMemoize attr_reader :epic_id, :epic_state_id, :epic_info_flat_list, :parent_id, - :direct_sums, # we calculate these and recursively add them - :sum_total + :immediate_count_totals, :immediate_weight_sum_totals, # only counts/weights of immediate issues and child epic counts + :count_aggregate, :weight_sum_aggregate - attr_accessor :child_ids + attr_accessor :child_ids, :calculated_count_totals, :calculated_weight_sum_totals def initialize(epic_id, flat_info_list) # epic aggregate records from the DB loader look like the following: # { 1 => [{iid: 1, epic_state_id: 1, issues_count: 1, issues_weight_sum: 2, parent_id: nil, state_id: 2}] ... } - # They include the sum of each epic's direct issues, grouped by status, + # They include the sum of each epic's immediate issues, grouped by status, # so in order to get a sum of the entire tree, we have to add that up recursively @epic_id = epic_id @epic_info_flat_list = flat_info_list @child_ids = [] - @direct_sums = [] + @immediate_count_totals = [] + @immediate_weight_sum_totals = [] set_epic_attributes(flat_info_list.first) # there will always be one end - def assemble_issue_sums + def assemble_issue_totals # this is a representation of the epic's - # direct child issues and epics that have come from the DB + # immediate child issues and epics that have come from the DB [OPENED_ISSUE_STATE, CLOSED_ISSUE_STATE].each do |issue_state| matching_issue_state_entry = epic_info_flat_list.find do |epic_info_node| epic_info_node[:issues_state_id] == issue_state @@ -41,43 +43,89 @@ def assemble_issue_sums end end - def assemble_epic_sums(children) + def assemble_epic_totals(children) [OPENED_EPIC_STATE, CLOSED_EPIC_STATE].each do |epic_state| create_sum_if_needed(COUNT, epic_state, EPIC_TYPE, children.select { |node| node.epic_state_id == epic_state }.count) end end - def calculate_recursive_sums(tree) - return sum_total if sum_total + def aggregate_count(tree) + strong_memoize(:count_aggregate) do + calculate_recursive_sums(COUNT, tree) + OpenStruct.new({ + opened_issues: sum_objects(COUNT, OPENED_ISSUE_STATE, ISSUE_TYPE), + closed_issues: sum_objects(COUNT, CLOSED_ISSUE_STATE, ISSUE_TYPE), + opened_epics: sum_objects(COUNT, OPENED_EPIC_STATE, EPIC_TYPE), + closed_epics: sum_objects(COUNT, CLOSED_EPIC_STATE, EPIC_TYPE) + }) + end + end - @sum_total = SumTotal.new - child_ids.each do |child_id| - child = tree[child_id] - # get the child's totals, add to your own - child_sums = child.calculate_recursive_sums(tree).sums - sum_total.add(child_sums) + def aggregate_weight_sum(tree) + strong_memoize(:weight_sum_aggregate) do + calculate_recursive_sums(WEIGHT_SUM, tree) + OpenStruct.new({ + opened_issues: sum_objects(WEIGHT_SUM, OPENED_ISSUE_STATE, ISSUE_TYPE), + closed_issues: sum_objects(WEIGHT_SUM, CLOSED_ISSUE_STATE, ISSUE_TYPE) + }) + end + end + + def immediate_totals(facet) + strong_memoize(:"immediate_#{facet}_totals") do + [] end - sum_total.add(direct_sums) - sum_total end - def aggregate_object_by(facet) - sum_total.by_facet(facet) + def calculated_totals(facet) + if facet == COUNT + return calculated_count_totals + end + + calculated_weight_sum_totals end def to_s - { epic_id: @epic_id, parent_id: @parent_id, direct_sums: direct_sums, child_ids: child_ids }.to_json + { + epic_id: @epic_id, + parent_id: @parent_id, + immediate_count_totals: immediate_count_totals, + immediate_weight_sum_totals: immediate_weight_sum_totals, + child_ids: child_ids + }.to_json end alias_method :inspect, :to_s alias_method :id, :epic_id + def calculate_recursive_sums(facet, tree) + return calculated_totals(facet) if calculated_totals(facet) + + sum_total = [] + child_ids.each do |child_id| + child = tree[child_id] + # get the child's totals, add to your own + child_sums = child.calculate_recursive_sums(facet, tree) + sum_total.concat(child_sums) + end + sum_total.concat(immediate_totals(facet)) + set_calculated_total(facet, sum_total) + end + private + def sum_objects(facet, state, type) + sums = calculated_totals(facet) || [] + matching = sums.select { |sum| sum.state == state && sum.type == type } + return 0 if sums.empty? + + matching.map(&:value).reduce(:+) || 0 + end + def create_sum_if_needed(facet, state, type, value) return if value.nil? || value < 1 - direct_sums << Sum.new(facet, state, type, value) + immediate_totals(facet) << Sum.new(facet, state, type, value) end def set_epic_attributes(record) @@ -85,6 +133,14 @@ def set_epic_attributes(record) @parent_id = record[:parent_id] end + def set_calculated_total(facet, calculated_sums) + if facet == COUNT + @calculated_count_totals = calculated_sums + else + @calculated_weight_sum_totals = calculated_sums + end + end + Sum = Struct.new(:facet, :state, :type, :value) do def inspect "" diff --git a/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb b/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb index 5196a24f852b3b..edf2cf7a1b88ef 100644 --- a/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb +++ b/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb @@ -5,7 +5,7 @@ module Graphql module Aggregations module Epics class LazyEpicAggregate - include Constants + include ::Gitlab::Graphql::Aggregations::Epics::Constants attr_reader :facet, :epic_id, :lazy_state @@ -38,7 +38,7 @@ def epic_aggregate if loaded_epic_info_node # The pending IDs were already loaded, # so return the result of that previous load - loaded_epic_info_node.aggregate_object_by(@facet) + aggregate_object(loaded_epic_info_node) else load_records_into_tree end @@ -56,16 +56,16 @@ def load_records_into_tree pending_ids = @lazy_state[:pending_ids].to_a # Fire off the db query and get the results (grouped by epic_id and facet) - raw_epic_aggregates = Epics::BulkEpicAggregateLoader.new(epic_ids: pending_ids).execute + raw_epic_aggregates = Gitlab::Graphql::Loaders::BulkEpicAggregateLoader.new(epic_ids: pending_ids).execute - # Assemble the tree and sum everything + # Assemble the tree and sum immediate child epic/issues create_structure_from(raw_epic_aggregates) @lazy_state[:pending_ids].clear # Now, get the matching node and return its aggregate depending on the facet: epic_node = @lazy_state[:tree][@epic_id] - epic_node.aggregate_object_by(@facet) + aggregate_object(epic_node) end def create_structure_from(aggregate_records) @@ -78,8 +78,7 @@ def create_structure_from(aggregate_records) end associate_parents_and_children - assemble_direct_sums - calculate_recursive_sums + assemble_immediate_totals end # each of the methods below are done one after the other @@ -89,18 +88,20 @@ def associate_parents_and_children end end - def assemble_direct_sums + def assemble_immediate_totals tree.each do |_, node| - node.assemble_issue_sums + node.assemble_issue_totals node_children = tree.select { |_, child_node| node.epic_id == child_node.parent_id }.values - node.assemble_epic_sums(node_children) + node.assemble_epic_totals(node_children) end end - def calculate_recursive_sums - tree.each do |_, node| - node.calculate_recursive_sums(tree) + def aggregate_object(node) + if @facet == COUNT + node.aggregate_count(tree) + else + node.aggregate_weight_sum(tree) end end end diff --git a/ee/lib/gitlab/graphql/aggregations/epics/sum_total.rb b/ee/lib/gitlab/graphql/aggregations/epics/sum_total.rb deleted file mode 100644 index 7da1df1d7dec43..00000000000000 --- a/ee/lib/gitlab/graphql/aggregations/epics/sum_total.rb +++ /dev/null @@ -1,29 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - module Aggregations - module Epics - class SumTotal - include Constants - - attr_accessor :sums - - def initialize - @sums = [] - end - - def add(other_sums) - sums.concat(other_sums) - end - - def by_facet(facet) - return CountAggregate.new(sums) if facet == COUNT - - WeightSumAggregate.new(sums) - end - end - end - end - end -end diff --git a/ee/lib/gitlab/graphql/aggregations/epics/weight_sum_aggregate.rb b/ee/lib/gitlab/graphql/aggregations/epics/weight_sum_aggregate.rb deleted file mode 100644 index 0b4481e8b5ad8c..00000000000000 --- a/ee/lib/gitlab/graphql/aggregations/epics/weight_sum_aggregate.rb +++ /dev/null @@ -1,24 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - module Aggregations - module Epics - class WeightSumAggregate < Aggregate - attr_accessor :opened_issues, :closed_issues - - def initialize(sums) - @sums = sums - - @opened_issues = sum_objects(OPENED_ISSUE_STATE, :issue) - @closed_issues = sum_objects(CLOSED_ISSUE_STATE, :issue) - end - - def facet - WEIGHT_SUM - end - end - end - end - end -end diff --git a/ee/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader.rb b/ee/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader.rb new file mode 100644 index 00000000000000..93ff1144ee81df --- /dev/null +++ b/ee/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Loaders + class BulkEpicAggregateLoader + include ::Gitlab::Graphql::Aggregations::Epics::Constants + + # This class retrieves each epic and its child epics recursively + # It allows us to recreate the epic tree structure in POROs + def initialize(epic_ids:) + @results = {} + @target_epic_ids = epic_ids.present? ? [epic_ids].flatten.compact : [] + end + + # rubocop: disable CodeReuse/ActiveRecord + def execute + return {} unless target_epic_ids.any? + + # We do a left outer join in order to capture epics with no issues + # This is so we can aggregate the epic counts for every epic + raw_results = ::Gitlab::ObjectHierarchy.new(Epic.where(id: target_epic_ids)).base_and_descendants + .left_joins(epic_issues: :issue) + .group("issues.state_id", "epics.id", "epics.iid", "epics.parent_id", "epics.state_id") + .select("epics.id, epics.iid, epics.parent_id, epics.state_id AS epic_state_id, issues.state_id AS issues_state_id, COUNT(issues) AS issues_count, SUM(COALESCE(issues.weight, 0)) AS issues_weight_sum") + + raw_results = raw_results.map(&:attributes).map(&:with_indifferent_access) + group_by_epic_id(raw_results) + @results + end + # rubocop: enable CodeReuse/ActiveRecord + + private + + attr_reader :target_epic_ids, :results + + def group_by_epic_id(raw_records) + # for each id, populate with matching records + # change from a series of { id: x ... } to { x => [{...}, {...}] } + raw_records.map { |r| r[:id] }.uniq.each do |epic_id| + records = [] + matching_records = raw_records.select { |record| record[:id] == epic_id } + matching_records.each do |record| + records << record.except(:id).to_h.with_indifferent_access + end + @results[epic_id] = records + end + end + end + end + end +end diff --git a/ee/spec/graphql/epics/aggregate_spec.rb b/ee/spec/graphql/epics/aggregate_spec.rb deleted file mode 100644 index b0f1f854d3516f..00000000000000 --- a/ee/spec/graphql/epics/aggregate_spec.rb +++ /dev/null @@ -1,73 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Epics::Aggregate do - include_context 'includes EpicAggregate constants' - - context 'when CountAggregate' do - subject { Epics::CountAggregate.new(sums) } - - describe 'summation' do - let(:sums) { [] } - - context 'when there are no sum objects' do - it 'returns 0 for all values', :aggregate_failures do - expect(subject.opened_issues).to eq 0 - expect(subject.closed_issues).to eq 0 - expect(subject.opened_epics).to eq 0 - expect(subject.closed_epics).to eq 0 - end - end - - context 'when some sums exist' do - let(:sums) do - [ - double(:sum, facet: COUNT_FACET, type: EPIC_TYPE, value: 1, state: OPENED_EPIC_STATE), - double(:sum, facet: COUNT_FACET, type: EPIC_TYPE, value: 1, state: CLOSED_EPIC_STATE), - double(:sum, facet: COUNT_FACET, type: ISSUE_TYPE, value: 1, state: OPENED_ISSUE_STATE), - double(:sum, facet: COUNT_FACET, type: ISSUE_TYPE, value: 1, state: OPENED_ISSUE_STATE), - double(:sum, facet: WEIGHT_SUM_FACET, type: ISSUE_TYPE, value: 22, state: CLOSED_ISSUE_STATE) - ] - end - - it 'returns sums of appropriate values', :aggregate_failures do - expect(subject.opened_issues).to eq 2 - expect(subject.closed_issues).to eq 0 - expect(subject.opened_epics).to eq 1 - expect(subject.closed_epics).to eq 1 - end - end - end - end - - context 'when WeightSumAggregate' do - subject { Epics::WeightSumAggregate.new(sums) } - - describe 'summation' do - let(:sums) { [] } - - context 'when there are no sum objects' do - it 'returns 0 for all values', :aggregate_failures do - expect(subject.opened_issues).to eq 0 - expect(subject.closed_issues).to eq 0 - end - end - - context 'when some sums exist' do - let(:sums) do - [ - double(:sum, facet: WEIGHT_SUM_FACET, type: ISSUE_TYPE, value: 1, state: OPENED_ISSUE_STATE), - double(:sum, facet: WEIGHT_SUM_FACET, type: ISSUE_TYPE, value: 1, state: OPENED_ISSUE_STATE), - double(:sum, facet: COUNT_FACET, type: ISSUE_TYPE, value: 22, state: CLOSED_ISSUE_STATE) - ] - end - - it 'returns sums of appropriate values', :aggregate_failures do - expect(subject.opened_issues).to eq 2 - expect(subject.closed_issues).to eq 0 - end - end - end - end -end diff --git a/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb b/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb similarity index 58% rename from spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb rename to ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb index 5e6e3e2b3e5e72..275f0fc0763c6a 100644 --- a/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb +++ b/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb @@ -31,7 +31,7 @@ it_behaves_like 'setting attributes based on the first record', { epic_state_id: CLOSED_EPIC_STATE, parent_id: 2 } end - describe '#assemble_issue_sums' do + describe '#assemble_issue_totals' do subject { described_class.new(epic_id, fake_data) } context 'an epic with no issues' do @@ -41,10 +41,11 @@ ] end - it 'does not create any sums' do - subject.assemble_issue_sums + it 'does not create any totals' do + subject.assemble_issue_totals - expect(subject.direct_sums.count).to eq 0 + expect(subject.immediate_totals(COUNT_FACET).count).to eq 0 + expect(subject.immediate_totals(WEIGHT_SUM_FACET).count).to eq 0 end end @@ -57,10 +58,11 @@ end it 'creates no sums for the weight if the issues have 0 weight' do - subject.assemble_issue_sums + subject.assemble_issue_totals - expect(subject.direct_sums.count).to eq 1 - expect(subject).to have_direct_sum(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) + expect(subject.immediate_totals(COUNT_FACET).count).to eq 1 + expect(subject.immediate_totals(WEIGHT_SUM_FACET).count).to eq 0 + expect(subject).to have_immediate_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) end end @@ -72,11 +74,10 @@ end it 'creates two sums' do - subject.assemble_issue_sums + subject.assemble_issue_totals - expect(subject.direct_sums.count).to eq 2 - expect(subject).to have_direct_sum(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) - expect(subject).to have_direct_sum(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) + expect(subject).to have_immediate_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) + expect(subject).to have_immediate_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) end end @@ -89,19 +90,18 @@ end it 'creates two sums' do - subject.assemble_issue_sums + subject.assemble_issue_totals - expect(subject.direct_sums.count).to eq 4 - expect(subject).to have_direct_sum(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) - expect(subject).to have_direct_sum(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) - expect(subject).to have_direct_sum(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 3) - expect(subject).to have_direct_sum(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 5) + expect(subject).to have_immediate_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) + expect(subject).to have_immediate_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) + expect(subject).to have_immediate_total(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 3) + expect(subject).to have_immediate_total(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 5) end end end end - describe '#assemble_epic_sums' do + describe '#assemble_epic_totals' do subject { described_class.new(epic_id, [{ parent_id: nil, epic_state_id: CLOSED_EPIC_STATE }]) } context 'with a child epic' do @@ -113,10 +113,9 @@ end it 'adds up the number of the child epics' do - subject.assemble_epic_sums([child_epic_node]) + subject.assemble_epic_totals([child_epic_node]) - expect(subject.direct_sums.count).to eq 1 - expect(subject).to have_direct_sum(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) + expect(subject).to have_immediate_total(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) end end end @@ -125,7 +124,16 @@ subject { described_class.new(epic_id, [{ parent_id: nil, epic_state_id: CLOSED_EPIC_STATE }]) } before do - allow(subject).to receive(:direct_sums).and_return(direct_sums) + allow(subject).to receive(:immediate_totals).with(COUNT_FACET).and_return(immediate_count_totals) + allow(subject).to receive(:immediate_totals).with(WEIGHT_SUM_FACET).and_return(immediate_weight_sum_totals) + end + + shared_examples 'returns calculated totals by facet' do |facet, count| + it 'returns a calculated_count_total' do + subject.calculate_recursive_sums(facet, tree) + + expect(subject.calculated_totals(facet).count).to eq count + end end context 'an epic with no child epics' do @@ -134,45 +142,37 @@ end context 'with no child issues' do - let(:direct_sums) do - [] - end - - it 'returns a SumTotal with no sums' do - result = subject.calculate_recursive_sums(tree) + let(:immediate_count_totals) { [] } + let(:immediate_weight_sum_totals) { [] } - expect(result.sums).not_to be_nil - expect(result.sums.count).to eq 0 - end + it_behaves_like 'returns calculated totals by facet', COUNT_FACET, 0 + it_behaves_like 'returns calculated totals by facet', WEIGHT_SUM_FACET, 0 end context 'with an issue with 0 weight' do - let(:direct_sums) do + let(:immediate_count_totals) do [Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 1)] end + let(:immediate_weight_sum_totals) { [] } - it 'returns a SumTotal with only a weight sum' do - result = subject.calculate_recursive_sums(tree) - - expect(result.sums).not_to be_nil - expect(result.sums.count).to eq 1 - end + it_behaves_like 'returns calculated totals by facet', COUNT_FACET, 1 + it_behaves_like 'returns calculated totals by facet', WEIGHT_SUM_FACET, 0 end context 'with an issue with nonzero weight' do - let(:direct_sums) do + let(:immediate_count_totals) do + [ + Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 1) + ] + end + let(:immediate_weight_sum_totals) do [ - Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 1), Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(WEIGHT_SUM_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 2) ] end - it 'returns a SumTotal with only a weight sum' do - result = subject.calculate_recursive_sums(tree) - - expect(result.sums).not_to be_nil - expect(result.sums.count).to eq 2 - end + it_behaves_like 'returns calculated totals by facet', COUNT_FACET, 1 + it_behaves_like 'returns calculated totals by facet', WEIGHT_SUM_FACET, 1 end end @@ -182,30 +182,41 @@ { epic_id => subject, child_epic_id => child_epic_node } end let(:child_epic_node) { described_class.new(child_epic_id, [{ parent_id: epic_id, epic_state_id: CLOSED_EPIC_STATE }]) } - let(:direct_sums) do + let(:immediate_count_totals) do [ # only one opened epic, the child Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, OPENED_EPIC_STATE, EPIC_TYPE, 1) ] end + let(:immediate_weight_sum_totals) { [] } before do subject.child_ids << child_epic_id - allow(child_epic_node).to receive(:direct_sums).and_return(child_sums) + allow(child_epic_node).to receive(:immediate_totals).with(COUNT_FACET).and_return(child_count_totals) + allow(child_epic_node).to receive(:immediate_totals).with(WEIGHT_SUM_FACET).and_return(child_weight_sum_totals) end context 'with a child that has issues of nonzero weight' do - let(:child_sums) do - [ # 1 issue of weight 2 - Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, OPENED_ISSUE_STATE, ISSUE_TYPE, 1), + let(:child_count_totals) do + [ + Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, OPENED_ISSUE_STATE, ISSUE_TYPE, 1) + ] + end + let(:child_weight_sum_totals) do + [ Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, ISSUE_TYPE, 2) ] end - it 'returns the correct sum total' do - result = subject.calculate_recursive_sums(tree) + it 'returns the correct count total' do + subject.calculate_recursive_sums(COUNT_FACET, tree) + + expect(subject.calculated_count_totals.count).to eq 2 + end + + it 'returns the correct weight sum total' do + subject.calculate_recursive_sums(WEIGHT_SUM_FACET, tree) - expect(result.sums).not_to be_nil - expect(result.sums.count).to eq 3 + expect(subject.calculated_weight_sum_totals.count).to eq 1 end end end diff --git a/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb b/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb similarity index 72% rename from spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb rename to ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb index 2a601195f32d85..9b9b20dcb22ef8 100644 --- a/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb +++ b/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb @@ -58,8 +58,8 @@ end it 'does not make the query again' do - expect(epic_info_node).to receive(:aggregate_object_by).with(subject.facet) - expect(Gitlab::Graphql::Aggregations::Epics::BulkEpicAggregateLoader).not_to receive(:new) + expect(epic_info_node).to receive(:aggregate_count) + expect(Gitlab::Graphql::Loaders::BulkEpicAggregateLoader).not_to receive(:new) subject.epic_aggregate end @@ -79,8 +79,8 @@ end before do - allow(Gitlab::Graphql::Aggregations::Epics::EpicNode).to receive(:aggregate_object_by).and_call_original - expect_any_instance_of(Gitlab::Graphql::Aggregations::Epics::BulkEpicAggregateLoader).to receive(:execute).and_return(fake_data) + allow(Gitlab::Graphql::Aggregations::Epics::EpicNode).to receive(:aggregate_count).and_call_original + expect_any_instance_of(Gitlab::Graphql::Loaders::BulkEpicAggregateLoader).to receive(:execute).and_return(fake_data) end it 'clears the pending IDs' do @@ -108,12 +108,12 @@ lazy_state = subject.instance_variable_get(:@lazy_state) tree = lazy_state[:tree] - expect(tree[epic_id]).to have_direct_sum(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) - expect(tree[epic_id]).to have_direct_sum(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 5) - expect(tree[epic_id]).to have_direct_sum(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) + expect(tree[epic_id]).to have_immediate_total(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) + expect(tree[epic_id]).to have_immediate_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 5) + expect(tree[epic_id]).to have_immediate_total(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) - expect(tree[child_epic_id]).to have_direct_sum(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 4) - expect(tree[child_epic_id]).to have_direct_sum(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 17) + expect(tree[child_epic_id]).to have_immediate_total(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 4) + expect(tree[child_epic_id]).to have_immediate_total(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 17) end it 'assembles recursive sums for the parent', :aggregate_failures do @@ -122,22 +122,23 @@ lazy_state = subject.instance_variable_get(:@lazy_state) tree = lazy_state[:tree] - expect(tree[epic_id]).to have_aggregate(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 2) - expect(tree[epic_id]).to have_aggregate(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 4) - expect(tree[epic_id]).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 5) - expect(tree[epic_id]).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 17) - expect(tree[epic_id]).to have_aggregate(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) + expect(tree[epic_id]).to have_aggregate(tree, ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 2) + expect(tree[epic_id]).to have_aggregate(tree, ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 4) + expect(tree[epic_id]).to have_aggregate(tree, ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 5) + expect(tree[epic_id]).to have_aggregate(tree, ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 17) + expect(tree[epic_id]).to have_aggregate(tree, EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) end end context 'for a standalone epic with no issues' do - it 'assembles direct sums', :aggregate_failures do + it 'assembles direct totals', :aggregate_failures do subject.epic_aggregate lazy_state = subject.instance_variable_get(:@lazy_state) tree = lazy_state[:tree] - expect(tree[other_epic_id].direct_sums).to be_empty + expect(tree[other_epic_id].immediate_count_totals).to be_empty + expect(tree[other_epic_id].immediate_weight_sum_totals).to be_empty end end end diff --git a/lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader_spec.rb b/ee/spec/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader_spec.rb similarity index 98% rename from lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader_spec.rb rename to ee/spec/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader_spec.rb index 1e9eb7d2027132..96c301290a394c 100644 --- a/lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader_spec.rb +++ b/ee/spec/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe Gitlab::Graphql::Aggregations::Epics::BulkEpicAggregateLoader do +describe Gitlab::Graphql::Loaders::BulkEpicAggregateLoader do let_it_be(:closed_issue_state_id) { Issue.available_states[:closed] } let_it_be(:opened_issue_state_id) { Issue.available_states[:opened] } diff --git a/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb b/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb index 562c4062669f8c..39f06f06845a92 100644 --- a/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb +++ b/ee/spec/requests/api/graphql/group/epic/epic_aggregate_query_spec.rb @@ -57,102 +57,103 @@ post_graphql(query, current_user: current_user) end - shared_examples 'counts properly' do - it_behaves_like 'a working graphql query' + shared_examples 'counts properly' do + it_behaves_like 'a working graphql query' - it 'returns the epic counts' do - epic_count_result = { - "openedEpics" => 1, - "closedEpics" => 2 - } + it 'returns the epic counts' do + epic_count_result = { + "openedEpics" => 1, + "closedEpics" => 2 + } - is_expected.to include( - a_hash_including('descendantCounts' => a_hash_including(epic_count_result)) - ) - end + is_expected.to include( + a_hash_including('descendantCounts' => a_hash_including(epic_count_result)) + ) + end - it 'returns the issue counts' do - issue_count_result = { - "openedIssues" => 1, - "closedIssues" => 1 - } + it 'returns the issue counts' do + issue_count_result = { + "openedIssues" => 1, + "closedIssues" => 1 + } - is_expected.to include( - a_hash_including('descendantCounts' => a_hash_including(issue_count_result)) - ) + is_expected.to include( + a_hash_including('descendantCounts' => a_hash_including(issue_count_result)) + ) + end end - end - context 'with feature flag enabled' do - before do - stub_feature_flags(unfiltered_epic_aggregates: true) - end + context 'with feature flag enabled' do + before do + stub_feature_flags(unfiltered_epic_aggregates: true) + end - it 'uses the LazyEpicAggregate service' do - # one for count, one for weight_sum, even though the share the same tree state as part of the context - expect(Epics::LazyEpicAggregate).to receive(:new).twice + it 'uses the LazyEpicAggregate service' do + # one for count, one for weight_sum, even though the share the same tree state as part of the context + expect(Gitlab::Graphql::Aggregations::Epics::LazyEpicAggregate).to receive(:new).twice - post_graphql(query, current_user: current_user) - end + post_graphql(query, current_user: current_user) + end - it_behaves_like 'counts properly' + it_behaves_like 'counts properly' - it 'returns the weights' do - descendant_weight_result = { - "openedIssues" => 5, - "closedIssues" => 7 - } + it 'returns the weights' do + descendant_weight_result = { + "openedIssues" => 5, + "closedIssues" => 7 + } - is_expected.to include( - a_hash_including('descendantWeightSum' => a_hash_including(descendant_weight_result)) - ) + is_expected.to include( + a_hash_including('descendantWeightSum' => a_hash_including(descendant_weight_result)) + ) + end end - end - context 'with feature flag disabled' do - before do - stub_feature_flags(unfiltered_epic_aggregates: false) - end + context 'with feature flag disabled' do + before do + stub_feature_flags(unfiltered_epic_aggregates: false) + end - context 'when requesting counts' do - let(:epic_aggregates_query) do - <<~QUERY - nodes { - descendantCounts { - openedEpics - closedEpics - openedIssues - closedIssues + context 'when requesting counts' do + let(:epic_aggregates_query) do + <<~QUERY + nodes { + descendantCounts { + openedEpics + closedEpics + openedIssues + closedIssues + } } - } - QUERY - end + QUERY + end - it 'uses the DescendantCountService' do - expect(Epics::DescendantCountService).to receive(:new) + it 'uses the DescendantCountService' do + expect(Epics::DescendantCountService).to receive(:new) - post_graphql(query, current_user: current_user) - end + post_graphql(query, current_user: current_user) + end - it_behaves_like 'counts properly' - end + it_behaves_like 'counts properly' + end - context 'when requesting weights' do - let(:epic_aggregates_query) do - <<~QUERY - nodes { - descendantWeightSum { - openedIssues - closedIssues + context 'when requesting weights' do + let(:epic_aggregates_query) do + <<~QUERY + nodes { + descendantWeightSum { + openedIssues + closedIssues + } } - } - QUERY - end + QUERY + end - it 'returns an error' do - post_graphql(query, current_user: current_user) + it 'returns an error' do + post_graphql(query, current_user: current_user) - expect_graphql_errors_to_include /Field 'descendantWeightSum' doesn't exist on type 'Epic/ + expect_graphql_errors_to_include /Field 'descendantWeightSum' doesn't exist on type 'Epic/ + end end end end diff --git a/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb b/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb index 1ba369511bf6e4..225dcc5eedc1b0 100644 --- a/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb +++ b/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb @@ -1,11 +1,12 @@ # frozen_string_literal: true -RSpec::Matchers.define :have_direct_sum do |type, facet, state, value| +RSpec::Matchers.define :have_immediate_total do |type, facet, state, expected_value| match do |epic_node_result| expect(epic_node_result).not_to be_nil - expect(epic_node_result.direct_sums).not_to be_empty + immediate_totals = epic_node_result.immediate_totals(facet) + expect(immediate_totals).not_to be_empty - matching = epic_node_result.direct_sums.select { |sum| sum.type == type && sum.facet == facet && sum.state == state && sum.value == value } + matching = immediate_totals.select { |sum| sum.type == type && sum.facet == facet && sum.state == state && sum.value == expected_value } expect(matching).not_to be_empty end @@ -13,24 +14,25 @@ if epic_node_result.nil? "expected for there to be an epic node, but it is nil" else + immediate_totals = epic_node_result.immediate_totals(facet) <<~FAILURE_MSG - expected epic node with id #{epic_node_result.epic_id} to have a sum with facet '#{facet}', state '#{state}', type '#{type}' and value '#{value}'. Has #{epic_node_result.direct_sums.count} sum objects#{", none of which match" if epic_node_result.direct_sums.count > 0}. - Sums: #{epic_node_result.direct_sums.inspect} + expected epic node with id #{epic_node_result.epic_id} to have a sum with facet '#{facet}', state '#{state}', type '#{type}' and value '#{expected_value}'. Has #{immediate_totals.count} immediate sum objects#{", none of which match" if immediate_totals.count > 0}. + Sums: #{immediate_totals.inspect} FAILURE_MSG end end end -RSpec::Matchers.define :have_aggregate do |type, facet, state, value| +RSpec::Matchers.define :have_aggregate do |tree, type, facet, state, expected_value| match do |epic_node_result| - aggregate_object = epic_node_result.aggregate_object_by(facet) - expect(aggregate_object.send(method_name(type, state))).to eq value + aggregate_object = epic_node_result.public_send(:"aggregate_#{facet}", tree) + expect(aggregate_object.public_send(method_name(type, state))).to eq expected_value end failure_message do |epic_node_result| - aggregate_object = epic_node_result.aggregate_object_by(facet) + aggregate_object = epic_node_result.public_send(:"aggregate_#{facet}", tree) aggregate_method = method_name(type, state) - "Epic node with id #{epic_node_result.epic_id} called #{aggregate_method} on aggregate object of type #{aggregate_object.class.name}. Value was expected to be #{value} but was #{aggregate_object.send(aggregate_method)}." + "Epic node with id #{epic_node_result.epic_id} called #{aggregate_method} on aggregate object. Value was expected to be #{expected_value} but was #{aggregate_object.send(aggregate_method)}." end def method_name(type, state) diff --git a/lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader.rb b/lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader.rb deleted file mode 100644 index 60795d2a71048a..00000000000000 --- a/lib/gitlab/graphql/aggregations/epics/bulk_epic_aggregate_loader.rb +++ /dev/null @@ -1,54 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - module Aggregations - module Epics - class BulkEpicAggregateLoader - include Constants - - # This class retrieves each epic and its child epics recursively - # It allows us to recreate the epic tree structure in POROs - def initialize(epic_ids:) - @results = {} - @target_epic_ids = epic_ids.present? ? [epic_ids].flatten.compact : [] - end - - # rubocop: disable CodeReuse/ActiveRecord - def execute - return {} unless target_epic_ids.any? - - # We do a left outer join in order to capture epics with no issues - # This is so we can aggregate the epic counts for every epic - raw_results = ::Gitlab::ObjectHierarchy.new(Epic.where(id: target_epic_ids)).base_and_descendants - .left_joins(epic_issues: :issue) - .group("issues.state_id", "epics.id", "epics.iid", "epics.parent_id", "epics.state_id") - .select("epics.id, epics.iid, epics.parent_id, epics.state_id AS epic_state_id, issues.state_id AS issues_state_id, COUNT(issues) AS issues_count, SUM(COALESCE(issues.weight, 0)) AS issues_weight_sum") - - raw_results = raw_results.map(&:attributes).map(&:with_indifferent_access) - group_by_epic_id(raw_results) - @results - end - # rubocop: enable CodeReuse/ActiveRecord - - private - - attr_reader :target_epic_ids, :results - - def group_by_epic_id(raw_records) - # for each id, populate with matching records - # change from a series of { id: x ... } to { x => [{...}, {...}] } - raw_records.map { |r| r[:id] }.uniq.each do |epic_id| - records = [] - matching_records = raw_records.select { |record| record[:id] == epic_id } - matching_records.each do |record| - records << record.except(:id).to_h.with_indifferent_access - end - @results[epic_id] = records - end - end - end - end - end - end -end -- GitLab From 9cd771580728f1cba1889ff8c9b0501234354cad Mon Sep 17 00:00:00 2001 From: charlieablett Date: Mon, 2 Mar 2020 22:22:43 +1300 Subject: [PATCH 7/9] Apply reviewer comments - Rename immediate to direct - Another matcher for calculated as well as direct sums - Remove unneeded constant methods - Test epic node state - Simplify epic tree load methods - Combine epic children and direct total assembly --- .../graphql/reference/gitlab_schema.graphql | 9 ++- doc/api/graphql/reference/index.md | 3 +- .../graphql/aggregations/epics/epic_node.rb | 41 ++++++-------- .../aggregations/epics/lazy_epic_aggregate.rb | 56 +++++++++---------- .../loaders/bulk_epic_aggregate_loader.rb | 26 ++------- ee/spec/graphql/types/epic_type_spec.rb | 2 +- .../aggregations/epics/epic_node_spec.rb | 37 ++++++------ .../epics/lazy_epic_aggregate_spec.rb | 14 ++--- .../bulk_epic_aggregate_loader_spec.rb | 16 ++---- .../matchers/ee/epic_aggregate_matchers.rb | 38 +++++++------ 10 files changed, 110 insertions(+), 132 deletions(-) diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index 336c21ef6a1504..8114982909ed76 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -1948,8 +1948,8 @@ type Epic implements Noteable { descendantCounts: EpicDescendantCount """ - Total weight of open and closed descendant epic's issues. Available only when - feature flag `unfiltered_epic_aggregates` is enabled. + Total weight of open and closed issues in the epic and its descendants. + Available only when feature flag `unfiltered_epic_aggregates` is enabled. """ descendantWeightSum: EpicDescendantWeights @@ -2023,6 +2023,11 @@ type Epic implements Noteable { """ hasIssues: Boolean! + """ + Current health status. Available only when feature flag `save_issuable_health_status` is enabled. + """ + healthStatus: HealthStatus + """ ID of the epic """ diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 923cc7dff3d08a..ecb68543321044 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -317,7 +317,7 @@ Represents an epic. | `closedAt` | Time | Timestamp of the epic's closure | | `createdAt` | Time | Timestamp of the epic's creation | | `descendantCounts` | EpicDescendantCount | Number of open and closed descendant epics and issues | -| `descendantWeightSum` | EpicDescendantWeights | Total weight of open and closed descendant epic's issues. Available only when feature flag `unfiltered_epic_aggregates` is enabled. | +| `descendantWeightSum` | EpicDescendantWeights | Total weight of open and closed issues in the epic and its descendants. Available only when feature flag `unfiltered_epic_aggregates` is enabled. | | `description` | String | Description of the epic | | `downvotes` | Int! | Number of downvotes the epic has received | | `dueDate` | Time | Due date of the epic | @@ -327,6 +327,7 @@ Represents an epic. | `group` | Group! | Group to which the epic belongs | | `hasChildren` | Boolean! | Indicates if the epic has children | | `hasIssues` | Boolean! | Indicates if the epic has direct issues | +| `healthStatus` | HealthStatus | Current health status. Available only when feature flag `save_issuable_health_status` is enabled. | | `id` | ID! | ID of the epic | | `iid` | ID! | Internal ID of the epic | | `parent` | Epic | Parent epic of the epic | diff --git a/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb b/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb index 6f3bdb20048088..5dcf9e9b44562f 100644 --- a/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb +++ b/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# This class represents an Epic's aggregate information (added up counts) about its child epics and immediate issues +# This class represents an Epic's aggregate information (added up counts) about its child epics and direct issues module Gitlab module Graphql @@ -11,7 +11,7 @@ class EpicNode include Gitlab::Utils::StrongMemoize attr_reader :epic_id, :epic_state_id, :epic_info_flat_list, :parent_id, - :immediate_count_totals, :immediate_weight_sum_totals, # only counts/weights of immediate issues and child epic counts + :direct_count_totals, :direct_weight_sum_totals, # only counts/weights of direct issues and child epic counts :count_aggregate, :weight_sum_aggregate attr_accessor :child_ids, :calculated_count_totals, :calculated_weight_sum_totals @@ -19,20 +19,20 @@ class EpicNode def initialize(epic_id, flat_info_list) # epic aggregate records from the DB loader look like the following: # { 1 => [{iid: 1, epic_state_id: 1, issues_count: 1, issues_weight_sum: 2, parent_id: nil, state_id: 2}] ... } - # They include the sum of each epic's immediate issues, grouped by status, + # They include the sum of each epic's direct issues, grouped by status, # so in order to get a sum of the entire tree, we have to add that up recursively @epic_id = epic_id @epic_info_flat_list = flat_info_list @child_ids = [] - @immediate_count_totals = [] - @immediate_weight_sum_totals = [] + @direct_count_totals = [] + @direct_weight_sum_totals = [] set_epic_attributes(flat_info_list.first) # there will always be one end def assemble_issue_totals # this is a representation of the epic's - # immediate child issues and epics that have come from the DB + # direct child issues and epics that have come from the DB [OPENED_ISSUE_STATE, CLOSED_ISSUE_STATE].each do |issue_state| matching_issue_state_entry = epic_info_flat_list.find do |epic_info_node| epic_info_node[:issues_state_id] == issue_state @@ -71,8 +71,10 @@ def aggregate_weight_sum(tree) end end - def immediate_totals(facet) - strong_memoize(:"immediate_#{facet}_totals") do + def direct_totals(facet) + # Sums of only child issues and immediate child epics (but not their issues + # ) + strong_memoize(:"direct_#{facet}_totals") do [] end end @@ -85,19 +87,6 @@ def calculated_totals(facet) calculated_weight_sum_totals end - def to_s - { - epic_id: @epic_id, - parent_id: @parent_id, - immediate_count_totals: immediate_count_totals, - immediate_weight_sum_totals: immediate_weight_sum_totals, - child_ids: child_ids - }.to_json - end - - alias_method :inspect, :to_s - alias_method :id, :epic_id - def calculate_recursive_sums(facet, tree) return calculated_totals(facet) if calculated_totals(facet) @@ -108,7 +97,7 @@ def calculate_recursive_sums(facet, tree) child_sums = child.calculate_recursive_sums(facet, tree) sum_total.concat(child_sums) end - sum_total.concat(immediate_totals(facet)) + sum_total.concat(direct_totals(facet)) set_calculated_total(facet, sum_total) end @@ -116,16 +105,18 @@ def calculate_recursive_sums(facet, tree) def sum_objects(facet, state, type) sums = calculated_totals(facet) || [] - matching = sums.select { |sum| sum.state == state && sum.type == type } return 0 if sums.empty? - matching.map(&:value).reduce(:+) || 0 + sums.inject(0) do |result, sum| + result += sum.value if sum.state == state && sum.type == type + result + end end def create_sum_if_needed(facet, state, type, value) return if value.nil? || value < 1 - immediate_totals(facet) << Sum.new(facet, state, type, value) + direct_totals(facet) << Sum.new(facet, state, type, value) end def set_epic_attributes(record) diff --git a/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb b/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb index edf2cf7a1b88ef..d8268af43e1dcc 100644 --- a/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb +++ b/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb @@ -9,13 +9,17 @@ class LazyEpicAggregate attr_reader :facet, :epic_id, :lazy_state + PERMITTED_FACETS = [COUNT, WEIGHT_SUM].freeze + # Because facets "count" and "weight_sum" share the same db query, but have a different graphql type object, # we can separate them and serve only the fields which are requested by the GraphQL query def initialize(query_ctx, epic_id, aggregate_facet) @epic_id = epic_id - raise ArgumentError.new("No aggregate facet provided. Please specify either #{COUNT} or #{WEIGHT_SUM}") unless aggregate_facet.present? - raise ArgumentError.new("Invalid aggregate facet #{aggregate_facet} provided. Please specify either #{COUNT} or #{WEIGHT_SUM}") unless [COUNT, WEIGHT_SUM].include?(aggregate_facet.to_sym) + error = validate_facet(aggregate_facet) + if error + raise ArgumentError.new("#{error}. Please specify either #{COUNT} or #{WEIGHT_SUM}") + end @facet = aggregate_facet.to_sym @@ -33,19 +37,25 @@ def initialize(query_ctx, epic_id, aggregate_facet) def epic_aggregate # Check if the record was already loaded: # load from tree by epic - loaded_epic_info_node = @lazy_state[:tree][@epic_id] - - if loaded_epic_info_node - # The pending IDs were already loaded, - # so return the result of that previous load - aggregate_object(loaded_epic_info_node) - else + unless tree[@epic_id] load_records_into_tree end + + aggregate_object(tree[@epic_id]) end private + def validate_facet(aggregate_facet) + unless aggregate_facet.present? + return "No aggregate facet provided." + end + + unless PERMITTED_FACETS.include?(aggregate_facet.to_sym) + return "Invalid aggregate facet #{aggregate_facet} provided." + end + end + def tree @lazy_state[:tree] end @@ -62,38 +72,26 @@ def load_records_into_tree create_structure_from(raw_epic_aggregates) @lazy_state[:pending_ids].clear - - # Now, get the matching node and return its aggregate depending on the facet: - epic_node = @lazy_state[:tree][@epic_id] - aggregate_object(epic_node) end def create_structure_from(aggregate_records) # create EpicNode object for each epic id aggregate_records.each do |epic_id, aggregates| - next if aggregates.nil? || aggregates.empty? + next if aggregates.blank? - new_node = EpicNode.new(epic_id, aggregates) - tree[epic_id] = new_node + tree[epic_id] = EpicNode.new(epic_id, aggregates) end - associate_parents_and_children - assemble_immediate_totals - end - - # each of the methods below are done one after the other - def associate_parents_and_children - tree.each do |epic_id, node| - node.child_ids = tree.select { |_, child_node| epic_id == child_node.parent_id }.keys - end + assemble_direct_child_totals end - def assemble_immediate_totals + def assemble_direct_child_totals tree.each do |_, node| - node.assemble_issue_totals + node_children = tree.select { |_, child_node| node.epic_id == child_node.parent_id } + node.child_ids = node_children.keys + node.assemble_epic_totals(node_children.values) - node_children = tree.select { |_, child_node| node.epic_id == child_node.parent_id }.values - node.assemble_epic_totals(node_children) + node.assemble_issue_totals end end diff --git a/ee/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader.rb b/ee/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader.rb index 93ff1144ee81df..7b261594ac183c 100644 --- a/ee/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader.rb +++ b/ee/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader.rb @@ -6,16 +6,18 @@ module Loaders class BulkEpicAggregateLoader include ::Gitlab::Graphql::Aggregations::Epics::Constants + attr_reader :target_epic_ids, :results + # This class retrieves each epic and its child epics recursively # It allows us to recreate the epic tree structure in POROs def initialize(epic_ids:) @results = {} - @target_epic_ids = epic_ids.present? ? [epic_ids].flatten.compact : [] + @target_epic_ids = epic_ids end # rubocop: disable CodeReuse/ActiveRecord def execute - return {} unless target_epic_ids.any? + return {} unless target_epic_ids # We do a left outer join in order to capture epics with no issues # This is so we can aggregate the epic counts for every epic @@ -25,27 +27,9 @@ def execute .select("epics.id, epics.iid, epics.parent_id, epics.state_id AS epic_state_id, issues.state_id AS issues_state_id, COUNT(issues) AS issues_count, SUM(COALESCE(issues.weight, 0)) AS issues_weight_sum") raw_results = raw_results.map(&:attributes).map(&:with_indifferent_access) - group_by_epic_id(raw_results) - @results + @results = raw_results.group_by { |record| record[:id] } end # rubocop: enable CodeReuse/ActiveRecord - - private - - attr_reader :target_epic_ids, :results - - def group_by_epic_id(raw_records) - # for each id, populate with matching records - # change from a series of { id: x ... } to { x => [{...}, {...}] } - raw_records.map { |r| r[:id] }.uniq.each do |epic_id| - records = [] - matching_records = raw_records.select { |record| record[:id] == epic_id } - matching_records.each do |record| - records << record.except(:id).to_h.with_indifferent_access - end - @results[epic_id] = records - end - end end end end diff --git a/ee/spec/graphql/types/epic_type_spec.rb b/ee/spec/graphql/types/epic_type_spec.rb index 33552cc080c436..e90230b732a869 100644 --- a/ee/spec/graphql/types/epic_type_spec.rb +++ b/ee/spec/graphql/types/epic_type_spec.rb @@ -11,7 +11,7 @@ closed_at created_at updated_at children has_children has_issues web_path web_url relation_path reference issues user_permissions notes discussions relative_position subscribed participants - descendant_counts descendant_weight_sum upvotes downvotes + descendant_counts descendant_weight_sum upvotes downvotes health_status ] end diff --git a/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb b/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb index 275f0fc0763c6a..871568aa64f809 100644 --- a/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb +++ b/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb @@ -24,6 +24,7 @@ new_node = described_class.new(epic_id, fake_data) expect(new_node.parent_id).to eq parent_id + expect(new_node.epic_state_id).to eq epic_state_id end end @@ -44,8 +45,8 @@ it 'does not create any totals' do subject.assemble_issue_totals - expect(subject.immediate_totals(COUNT_FACET).count).to eq 0 - expect(subject.immediate_totals(WEIGHT_SUM_FACET).count).to eq 0 + expect(subject.direct_totals(COUNT_FACET).count).to eq 0 + expect(subject.direct_totals(WEIGHT_SUM_FACET).count).to eq 0 end end @@ -60,9 +61,9 @@ it 'creates no sums for the weight if the issues have 0 weight' do subject.assemble_issue_totals - expect(subject.immediate_totals(COUNT_FACET).count).to eq 1 - expect(subject.immediate_totals(WEIGHT_SUM_FACET).count).to eq 0 - expect(subject).to have_immediate_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) + expect(subject.direct_totals(COUNT_FACET).count).to eq 1 + expect(subject.direct_totals(WEIGHT_SUM_FACET).count).to eq 0 + expect(subject).to have_direct_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) end end @@ -76,8 +77,8 @@ it 'creates two sums' do subject.assemble_issue_totals - expect(subject).to have_immediate_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) - expect(subject).to have_immediate_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) + expect(subject).to have_direct_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) + expect(subject).to have_direct_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) end end @@ -92,10 +93,10 @@ it 'creates two sums' do subject.assemble_issue_totals - expect(subject).to have_immediate_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) - expect(subject).to have_immediate_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) - expect(subject).to have_immediate_total(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 3) - expect(subject).to have_immediate_total(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 5) + expect(subject).to have_direct_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) + expect(subject).to have_direct_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) + expect(subject).to have_direct_total(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 3) + expect(subject).to have_direct_total(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 5) end end end @@ -115,7 +116,7 @@ it 'adds up the number of the child epics' do subject.assemble_epic_totals([child_epic_node]) - expect(subject).to have_immediate_total(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) + expect(subject).to have_direct_total(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) end end end @@ -124,8 +125,8 @@ subject { described_class.new(epic_id, [{ parent_id: nil, epic_state_id: CLOSED_EPIC_STATE }]) } before do - allow(subject).to receive(:immediate_totals).with(COUNT_FACET).and_return(immediate_count_totals) - allow(subject).to receive(:immediate_totals).with(WEIGHT_SUM_FACET).and_return(immediate_weight_sum_totals) + allow(subject).to receive(:direct_totals).with(COUNT_FACET).and_return(immediate_count_totals) + allow(subject).to receive(:direct_totals).with(WEIGHT_SUM_FACET).and_return(immediate_weight_sum_totals) end shared_examples 'returns calculated totals by facet' do |facet, count| @@ -191,8 +192,8 @@ before do subject.child_ids << child_epic_id - allow(child_epic_node).to receive(:immediate_totals).with(COUNT_FACET).and_return(child_count_totals) - allow(child_epic_node).to receive(:immediate_totals).with(WEIGHT_SUM_FACET).and_return(child_weight_sum_totals) + allow(child_epic_node).to receive(:direct_totals).with(COUNT_FACET).and_return(child_count_totals) + allow(child_epic_node).to receive(:direct_totals).with(WEIGHT_SUM_FACET).and_return(child_weight_sum_totals) end context 'with a child that has issues of nonzero weight' do @@ -210,13 +211,13 @@ it 'returns the correct count total' do subject.calculate_recursive_sums(COUNT_FACET, tree) - expect(subject.calculated_count_totals.count).to eq 2 + expect(subject).to have_calculated_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) end it 'returns the correct weight sum total' do subject.calculate_recursive_sums(WEIGHT_SUM_FACET, tree) - expect(subject.calculated_weight_sum_totals.count).to eq 1 + expect(subject).to have_calculated_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) end end end diff --git a/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb b/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb index 9b9b20dcb22ef8..d71bd004796f8c 100644 --- a/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb +++ b/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb @@ -108,12 +108,12 @@ lazy_state = subject.instance_variable_get(:@lazy_state) tree = lazy_state[:tree] - expect(tree[epic_id]).to have_immediate_total(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) - expect(tree[epic_id]).to have_immediate_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 5) - expect(tree[epic_id]).to have_immediate_total(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) + expect(tree[epic_id]).to have_direct_total(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) + expect(tree[epic_id]).to have_direct_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 5) + expect(tree[epic_id]).to have_direct_total(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) - expect(tree[child_epic_id]).to have_immediate_total(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 4) - expect(tree[child_epic_id]).to have_immediate_total(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 17) + expect(tree[child_epic_id]).to have_direct_total(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 4) + expect(tree[child_epic_id]).to have_direct_total(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 17) end it 'assembles recursive sums for the parent', :aggregate_failures do @@ -137,8 +137,8 @@ lazy_state = subject.instance_variable_get(:@lazy_state) tree = lazy_state[:tree] - expect(tree[other_epic_id].immediate_count_totals).to be_empty - expect(tree[other_epic_id].immediate_weight_sum_totals).to be_empty + expect(tree[other_epic_id].direct_count_totals).to be_empty + expect(tree[other_epic_id].direct_weight_sum_totals).to be_empty end end end diff --git a/ee/spec/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader_spec.rb b/ee/spec/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader_spec.rb index 96c301290a394c..9510ce2529f109 100644 --- a/ee/spec/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader_spec.rb +++ b/ee/spec/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader_spec.rb @@ -3,11 +3,7 @@ require 'spec_helper' describe Gitlab::Graphql::Loaders::BulkEpicAggregateLoader do - let_it_be(:closed_issue_state_id) { Issue.available_states[:closed] } - let_it_be(:opened_issue_state_id) { Issue.available_states[:opened] } - - let_it_be(:closed_epic_state_id) { Epic.available_states[:closed] } - let_it_be(:opened_epic_state_id) { Epic.available_states[:opened] } + include_context 'includes EpicAggregate constants' let_it_be(:group) { create(:group, :public) } let_it_be(:subgroup) { create(:group, :private, parent: group)} @@ -65,12 +61,12 @@ it 'sums all the weights, even confidential, or in private groups' do expected_result = { parent_epic.id => [ - result_for(parent_epic, issues_state: opened_issue_state_id, issues_count: 5, issues_weight_sum: 4), - result_for(parent_epic, issues_state: closed_issue_state_id, issues_count: 1, issues_weight_sum: 1) + result_for(parent_epic, issues_state: OPENED_ISSUE_STATE, issues_count: 5, issues_weight_sum: 4), + result_for(parent_epic, issues_state: CLOSED_ISSUE_STATE, issues_count: 1, issues_weight_sum: 1) ], epic_with_issues.id => [ - result_for(epic_with_issues, issues_state: opened_issue_state_id, issues_count: 5, issues_weight_sum: 4), - result_for(epic_with_issues, issues_state: closed_issue_state_id, issues_count: 1, issues_weight_sum: 1) + result_for(epic_with_issues, issues_state: OPENED_ISSUE_STATE, issues_count: 5, issues_weight_sum: 4), + result_for(epic_with_issues, issues_state: CLOSED_ISSUE_STATE, issues_count: 1, issues_weight_sum: 1) ], epic_without_issues.id => [ result_for(epic_without_issues, issues_state: nil, issues_count: 0, issues_weight_sum: 0) @@ -132,6 +128,6 @@ end def result_for(epic, issues_state:, issues_count:, issues_weight_sum:) - { iid: epic.iid, issues_count: issues_count, issues_weight_sum: issues_weight_sum, parent_id: epic.parent_id, issues_state_id: issues_state, epic_state_id: Epic.available_states[epic.state_id] }.stringify_keys + { id: epic.id, iid: epic.iid, issues_count: issues_count, issues_weight_sum: issues_weight_sum, parent_id: epic.parent_id, issues_state_id: issues_state, epic_state_id: Epic.available_states[epic.state_id] }.stringify_keys end end diff --git a/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb b/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb index 225dcc5eedc1b0..33d435580493fa 100644 --- a/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb +++ b/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb @@ -1,24 +1,26 @@ # frozen_string_literal: true -RSpec::Matchers.define :have_immediate_total do |type, facet, state, expected_value| - match do |epic_node_result| - expect(epic_node_result).not_to be_nil - immediate_totals = epic_node_result.immediate_totals(facet) - expect(immediate_totals).not_to be_empty - - matching = immediate_totals.select { |sum| sum.type == type && sum.facet == facet && sum.state == state && sum.value == expected_value } - expect(matching).not_to be_empty - end +%w[direct calculated].each do |total_type| + RSpec::Matchers.define :"have_#{total_type}_total" do |type, facet, state, expected_value| + match do |epic_node_result| + expect(epic_node_result).not_to be_nil + totals = epic_node_result.public_send("#{total_type}_totals", facet) + expect(totals).not_to be_empty + + matching = totals.select { |sum| sum.type == type && sum.facet == facet && sum.state == state && sum.value == expected_value } + expect(matching).not_to be_empty + end - failure_message do |epic_node_result| - if epic_node_result.nil? - "expected for there to be an epic node, but it is nil" - else - immediate_totals = epic_node_result.immediate_totals(facet) - <<~FAILURE_MSG - expected epic node with id #{epic_node_result.epic_id} to have a sum with facet '#{facet}', state '#{state}', type '#{type}' and value '#{expected_value}'. Has #{immediate_totals.count} immediate sum objects#{", none of which match" if immediate_totals.count > 0}. - Sums: #{immediate_totals.inspect} - FAILURE_MSG + failure_message do |epic_node_result| + if epic_node_result.nil? + "expected for there to be an epic node, but it is nil" + else + totals = epic_node_result.public_send("#{total_type}_totals", facet) + <<~FAILURE_MSG + expected epic node with id #{epic_node_result.epic_id} to have a sum with facet '#{facet}', state '#{state}', type '#{type}' and value '#{expected_value}'. Has #{totals.count} #{total_type} sum objects#{", none of which match" if totals.count > 0}. + Sums: #{totals.inspect} + FAILURE_MSG + end end end end -- GitLab From 3752f266dcac5ed99edd70665792a7b7c870b51c Mon Sep 17 00:00:00 2001 From: charlieablett Date: Wed, 4 Mar 2020 21:55:43 +1300 Subject: [PATCH 8/9] Use children rather than child_ids - Direct children --- ee/app/graphql/types/epic_type.rb | 6 ----- .../graphql/aggregations/epics/epic_node.rb | 23 ++++++++++++++----- .../aggregations/epics/lazy_epic_aggregate.rb | 10 +++++--- ee/spec/graphql/types/epic_type_spec.rb | 2 +- .../aggregations/epics/epic_node_spec.rb | 6 ++--- .../epics/lazy_epic_aggregate_spec.rb | 2 +- 6 files changed, 29 insertions(+), 20 deletions(-) diff --git a/ee/app/graphql/types/epic_type.rb b/ee/app/graphql/types/epic_type.rb index 72860ca1c758df..61a46052c76593 100644 --- a/ee/app/graphql/types/epic_type.rb +++ b/ee/app/graphql/types/epic_type.rb @@ -138,11 +138,5 @@ class EpicType < BaseObject resolve: -> (epic, args, ctx) do Gitlab::Graphql::Aggregations::Epics::LazyEpicAggregate.new(ctx, epic.id, WEIGHT_SUM) end - - field :health_status, - ::Types::HealthStatusEnum, - null: true, - description: 'Current health status', - feature_flag: :save_issuable_health_status end end diff --git a/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb b/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb index 5dcf9e9b44562f..c433be318b087a 100644 --- a/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb +++ b/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb @@ -14,7 +14,7 @@ class EpicNode :direct_count_totals, :direct_weight_sum_totals, # only counts/weights of direct issues and child epic counts :count_aggregate, :weight_sum_aggregate - attr_accessor :child_ids, :calculated_count_totals, :calculated_weight_sum_totals + attr_accessor :children, :calculated_count_totals, :calculated_weight_sum_totals def initialize(epic_id, flat_info_list) # epic aggregate records from the DB loader look like the following: @@ -23,7 +23,7 @@ def initialize(epic_id, flat_info_list) # so in order to get a sum of the entire tree, we have to add that up recursively @epic_id = epic_id @epic_info_flat_list = flat_info_list - @child_ids = [] + @children = [] @direct_count_totals = [] @direct_weight_sum_totals = [] @@ -43,7 +43,7 @@ def assemble_issue_totals end end - def assemble_epic_totals(children) + def assemble_epic_totals [OPENED_EPIC_STATE, CLOSED_EPIC_STATE].each do |epic_state| create_sum_if_needed(COUNT, epic_state, EPIC_TYPE, children.select { |node| node.epic_state_id == epic_state }.count) end @@ -91,9 +91,7 @@ def calculate_recursive_sums(facet, tree) return calculated_totals(facet) if calculated_totals(facet) sum_total = [] - child_ids.each do |child_id| - child = tree[child_id] - # get the child's totals, add to your own + children.each do |child| child_sums = child.calculate_recursive_sums(facet, tree) sum_total.concat(child_sums) end @@ -101,6 +99,19 @@ def calculate_recursive_sums(facet, tree) set_calculated_total(facet, sum_total) end + def inspect + { + epic_id: @epic_id, + parent_id: @parent_id, + direct_count_totals: direct_count_totals, + direct_weight_sum_totals: direct_weight_sum_totals, + children: children, + object_id: object_id + }.to_json + end + + alias_method :to_s, :inspect + private def sum_objects(facet, state, type) diff --git a/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb b/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb index d8268af43e1dcc..50a11856a8fd41 100644 --- a/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb +++ b/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb @@ -87,10 +87,14 @@ def create_structure_from(aggregate_records) def assemble_direct_child_totals tree.each do |_, node| - node_children = tree.select { |_, child_node| node.epic_id == child_node.parent_id } - node.child_ids = node_children.keys - node.assemble_epic_totals(node_children.values) + parent = tree[node.parent_id] + next if parent.nil? + parent.children << node + end + + tree.each do |_, node| + node.assemble_epic_totals node.assemble_issue_totals end end diff --git a/ee/spec/graphql/types/epic_type_spec.rb b/ee/spec/graphql/types/epic_type_spec.rb index e90230b732a869..33552cc080c436 100644 --- a/ee/spec/graphql/types/epic_type_spec.rb +++ b/ee/spec/graphql/types/epic_type_spec.rb @@ -11,7 +11,7 @@ closed_at created_at updated_at children has_children has_issues web_path web_url relation_path reference issues user_permissions notes discussions relative_position subscribed participants - descendant_counts descendant_weight_sum upvotes downvotes health_status + descendant_counts descendant_weight_sum upvotes downvotes ] end diff --git a/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb b/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb index 871568aa64f809..5391dc98d542f6 100644 --- a/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb +++ b/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb @@ -110,11 +110,11 @@ let!(:child_epic_node) { described_class.new(child_epic_id, [{ parent_id: epic_id, epic_state_id: CLOSED_EPIC_STATE }]) } before do - subject.child_ids << child_epic_id + subject.children << child_epic_node end it 'adds up the number of the child epics' do - subject.assemble_epic_totals([child_epic_node]) + subject.assemble_epic_totals expect(subject).to have_direct_total(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) end @@ -191,7 +191,7 @@ let(:immediate_weight_sum_totals) { [] } before do - subject.child_ids << child_epic_id + subject.children << child_epic_node allow(child_epic_node).to receive(:direct_totals).with(COUNT_FACET).and_return(child_count_totals) allow(child_epic_node).to receive(:direct_totals).with(WEIGHT_SUM_FACET).and_return(child_weight_sum_totals) end diff --git a/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb b/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb index d71bd004796f8c..f8e7118f15e59e 100644 --- a/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb +++ b/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb @@ -98,7 +98,7 @@ tree = lazy_state[:tree] expect(tree[child_epic_id].parent_id).to eq epic_id - expect(tree[epic_id].child_ids).to match_array([child_epic_id]) + expect(tree[epic_id].children.map(&:epic_id)).to match_array([child_epic_id]) end context 'for a parent-child relationship' do -- GitLab From aa7cdbaa5ad42157be5a2c271f263336b72bee56 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Wed, 4 Mar 2020 22:10:15 +1300 Subject: [PATCH 9/9] Extra lazy calculation -Don't even add up the direct sums until we're ready to calculate everything - Modify tests - Make constants consistent COUNT_FACET is just COUNT - Add limit to db call --- .../graphql/reference/gitlab_schema.graphql | 5 - doc/api/graphql/reference/index.md | 1 - .../graphql/aggregations/epics/epic_node.rb | 111 +++------- .../aggregations/epics/lazy_epic_aggregate.rb | 21 +- .../loaders/bulk_epic_aggregate_loader.rb | 8 +- .../aggregations/epics/epic_node_spec.rb | 205 +++++------------- .../epics/lazy_epic_aggregate_spec.rb | 62 ++---- .../bulk_epic_aggregate_loader_spec.rb | 16 +- .../matchers/ee/epic_aggregate_matchers.rb | 31 +-- .../epic_aggregate_constants.rb | 4 +- 10 files changed, 142 insertions(+), 322 deletions(-) diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index 8114982909ed76..fc26af40c696ef 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -2023,11 +2023,6 @@ type Epic implements Noteable { """ hasIssues: Boolean! - """ - Current health status. Available only when feature flag `save_issuable_health_status` is enabled. - """ - healthStatus: HealthStatus - """ ID of the epic """ diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index ecb68543321044..7230be484e610c 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -327,7 +327,6 @@ Represents an epic. | `group` | Group! | Group to which the epic belongs | | `hasChildren` | Boolean! | Indicates if the epic has children | | `hasIssues` | Boolean! | Indicates if the epic has direct issues | -| `healthStatus` | HealthStatus | Current health status. Available only when feature flag `save_issuable_health_status` is enabled. | | `id` | ID! | ID of the epic | | `iid` | ID! | Internal ID of the epic | | `parent` | Epic | Parent epic of the epic | diff --git a/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb b/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb index c433be318b087a..27900cf8c9e940 100644 --- a/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb +++ b/ee/lib/gitlab/graphql/aggregations/epics/epic_node.rb @@ -11,141 +11,84 @@ class EpicNode include Gitlab::Utils::StrongMemoize attr_reader :epic_id, :epic_state_id, :epic_info_flat_list, :parent_id, - :direct_count_totals, :direct_weight_sum_totals, # only counts/weights of direct issues and child epic counts :count_aggregate, :weight_sum_aggregate attr_accessor :children, :calculated_count_totals, :calculated_weight_sum_totals def initialize(epic_id, flat_info_list) # epic aggregate records from the DB loader look like the following: - # { 1 => [{iid: 1, epic_state_id: 1, issues_count: 1, issues_weight_sum: 2, parent_id: nil, state_id: 2}] ... } + # { 1 => [{iid: 1, parent_id: nil, epic_state_id: 1, issues_count: 1, issues_weight_sum: 2, issues_state_id: 2}] ... } # They include the sum of each epic's direct issues, grouped by status, # so in order to get a sum of the entire tree, we have to add that up recursively @epic_id = epic_id @epic_info_flat_list = flat_info_list @children = [] - @direct_count_totals = [] - @direct_weight_sum_totals = [] + @sums = {} set_epic_attributes(flat_info_list.first) # there will always be one end - def assemble_issue_totals - # this is a representation of the epic's - # direct child issues and epics that have come from the DB - [OPENED_ISSUE_STATE, CLOSED_ISSUE_STATE].each do |issue_state| - matching_issue_state_entry = epic_info_flat_list.find do |epic_info_node| - epic_info_node[:issues_state_id] == issue_state - end || {} - - create_sum_if_needed(WEIGHT_SUM, issue_state, ISSUE_TYPE, matching_issue_state_entry.fetch(:issues_weight_sum, 0)) - create_sum_if_needed(COUNT, issue_state, ISSUE_TYPE, matching_issue_state_entry.fetch(:issues_count, 0)) - end - end - - def assemble_epic_totals - [OPENED_EPIC_STATE, CLOSED_EPIC_STATE].each do |epic_state| - create_sum_if_needed(COUNT, epic_state, EPIC_TYPE, children.select { |node| node.epic_state_id == epic_state }.count) - end - end - - def aggregate_count(tree) + def aggregate_count strong_memoize(:count_aggregate) do - calculate_recursive_sums(COUNT, tree) OpenStruct.new({ opened_issues: sum_objects(COUNT, OPENED_ISSUE_STATE, ISSUE_TYPE), closed_issues: sum_objects(COUNT, CLOSED_ISSUE_STATE, ISSUE_TYPE), opened_epics: sum_objects(COUNT, OPENED_EPIC_STATE, EPIC_TYPE), closed_epics: sum_objects(COUNT, CLOSED_EPIC_STATE, EPIC_TYPE) - }) + }) end end - def aggregate_weight_sum(tree) + def aggregate_weight_sum strong_memoize(:weight_sum_aggregate) do - calculate_recursive_sums(WEIGHT_SUM, tree) OpenStruct.new({ opened_issues: sum_objects(WEIGHT_SUM, OPENED_ISSUE_STATE, ISSUE_TYPE), closed_issues: sum_objects(WEIGHT_SUM, CLOSED_ISSUE_STATE, ISSUE_TYPE) - }) + }) end end - def direct_totals(facet) - # Sums of only child issues and immediate child epics (but not their issues - # ) - strong_memoize(:"direct_#{facet}_totals") do - [] - end - end - - def calculated_totals(facet) - if facet == COUNT - return calculated_count_totals - end - - calculated_weight_sum_totals - end - - def calculate_recursive_sums(facet, tree) - return calculated_totals(facet) if calculated_totals(facet) - - sum_total = [] - children.each do |child| - child_sums = child.calculate_recursive_sums(facet, tree) - sum_total.concat(child_sums) - end - sum_total.concat(direct_totals(facet)) - set_calculated_total(facet, sum_total) - end - - def inspect + def to_s { epic_id: @epic_id, parent_id: @parent_id, - direct_count_totals: direct_count_totals, - direct_weight_sum_totals: direct_weight_sum_totals, children: children, object_id: object_id - }.to_json + }.to_s end - alias_method :to_s, :inspect - - private - def sum_objects(facet, state, type) - sums = calculated_totals(facet) || [] - return 0 if sums.empty? + key = [facet, state, type] + return @sums[key] if @sums[key] - sums.inject(0) do |result, sum| - result += sum.value if sum.state == state && sum.type == type - result + direct_sum = value_from_records(*key) + sum_from_children = children.inject(0) do |total, child| + total += child.sum_objects(*key) + total end - end - - def create_sum_if_needed(facet, state, type, value) - return if value.nil? || value < 1 - direct_totals(facet) << Sum.new(facet, state, type, value) + @sums[key] = direct_sum + sum_from_children end + private + def set_epic_attributes(record) @epic_state_id = record[:epic_state_id] @parent_id = record[:parent_id] end - def set_calculated_total(facet, calculated_sums) - if facet == COUNT - @calculated_count_totals = calculated_sums + def value_from_records(facet, state, type) + # DB records look like: + # {iid: 1, epic_state_id: 1, issues_count: 1, issues_weight_sum: 2, parent_id: nil, issues_state_id: 2} + if type == EPIC_TYPE + # can only be COUNT + children.select { |node| node.epic_state_id == state }.count else - @calculated_weight_sum_totals = calculated_sums - end - end + matching_record = epic_info_flat_list.find do |record| + record[:issues_state_id] == state + end || {} - Sum = Struct.new(:facet, :state, :type, :value) do - def inspect - "" + matching_record.fetch("issues_#{facet}".to_sym, 0) end end end diff --git a/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb b/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb index 50a11856a8fd41..803f3f52b2bbcc 100644 --- a/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb +++ b/ee/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate.rb @@ -67,43 +67,34 @@ def load_records_into_tree # Fire off the db query and get the results (grouped by epic_id and facet) raw_epic_aggregates = Gitlab::Graphql::Loaders::BulkEpicAggregateLoader.new(epic_ids: pending_ids).execute - - # Assemble the tree and sum immediate child epic/issues - create_structure_from(raw_epic_aggregates) - + create_epic_nodes(raw_epic_aggregates) @lazy_state[:pending_ids].clear end - def create_structure_from(aggregate_records) - # create EpicNode object for each epic id + def create_epic_nodes(aggregate_records) aggregate_records.each do |epic_id, aggregates| next if aggregates.blank? tree[epic_id] = EpicNode.new(epic_id, aggregates) end - assemble_direct_child_totals + relate_parents_and_children end - def assemble_direct_child_totals + def relate_parents_and_children tree.each do |_, node| parent = tree[node.parent_id] next if parent.nil? parent.children << node end - - tree.each do |_, node| - node.assemble_epic_totals - node.assemble_issue_totals - end end def aggregate_object(node) if @facet == COUNT - node.aggregate_count(tree) + node.aggregate_count else - node.aggregate_weight_sum(tree) + node.aggregate_weight_sum end end end diff --git a/ee/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader.rb b/ee/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader.rb index 7b261594ac183c..b95a3a54b9260d 100644 --- a/ee/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader.rb +++ b/ee/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader.rb @@ -6,6 +6,8 @@ module Loaders class BulkEpicAggregateLoader include ::Gitlab::Graphql::Aggregations::Epics::Constants + MAXIMUM_LOADABLE = 100_001 + attr_reader :target_epic_ids, :results # This class retrieves each epic and its child epics recursively @@ -25,8 +27,12 @@ def execute .left_joins(epic_issues: :issue) .group("issues.state_id", "epics.id", "epics.iid", "epics.parent_id", "epics.state_id") .select("epics.id, epics.iid, epics.parent_id, epics.state_id AS epic_state_id, issues.state_id AS issues_state_id, COUNT(issues) AS issues_count, SUM(COALESCE(issues.weight, 0)) AS issues_weight_sum") + .limit(MAXIMUM_LOADABLE) + + raw_results = raw_results.map { |record| record.attributes.with_indifferent_access } + + raise ArgumentError.new("There are too many records to load. Please select fewer epics or contact your administrator.") if raw_results.count == MAXIMUM_LOADABLE - raw_results = raw_results.map(&:attributes).map(&:with_indifferent_access) @results = raw_results.group_by { |record| record[:id] } end # rubocop: enable CodeReuse/ActiveRecord diff --git a/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb b/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb index 5391dc98d542f6..dec636dcff0603 100644 --- a/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb +++ b/ee/spec/lib/gitlab/graphql/aggregations/epics/epic_node_spec.rb @@ -32,194 +32,105 @@ it_behaves_like 'setting attributes based on the first record', { epic_state_id: CLOSED_EPIC_STATE, parent_id: 2 } end - describe '#assemble_issue_totals' do - subject { described_class.new(epic_id, fake_data) } - - context 'an epic with no issues' do - let(:fake_data) do - [ - { iid: epic_iid, epic_state_id: OPENED_EPIC_STATE, issues_count: 0, issues_weight_sum: 0, parent_id: nil, issues_state_id: nil } - ] - end - - it 'does not create any totals' do - subject.assemble_issue_totals + describe 'recursive totals' do + subject { described_class.new(epic_id, [{ parent_id: nil, epic_state_id: CLOSED_EPIC_STATE }]) } - expect(subject.direct_totals(COUNT_FACET).count).to eq 0 - expect(subject.direct_totals(WEIGHT_SUM_FACET).count).to eq 0 - end + before do + allow(subject).to receive(:epic_info_flat_list).and_return(flat_info) end - context 'an epic with issues' do - context 'with a nonzero count but a zero weight' do - let(:fake_data) do - [ - { iid: epic_iid, epic_state_id: OPENED_EPIC_STATE, issues_count: 1, issues_weight_sum: 0, parent_id: nil, issues_state_id: OPENED_ISSUE_STATE } - ] - end + context 'an epic with no child epics' do + context 'with no child issues', :aggregate_results do + let(:flat_info) { [] } - it 'creates no sums for the weight if the issues have 0 weight' do - subject.assemble_issue_totals + it 'has the correct aggregates', :aggregate_failures do + expect(subject).to have_aggregate(ISSUE_TYPE, COUNT, OPENED_ISSUE_STATE, 0) + expect(subject).to have_aggregate(ISSUE_TYPE, COUNT, CLOSED_ISSUE_STATE, 0) + expect(subject).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM, OPENED_ISSUE_STATE, 0) + expect(subject).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM, CLOSED_ISSUE_STATE, 0) - expect(subject.direct_totals(COUNT_FACET).count).to eq 1 - expect(subject.direct_totals(WEIGHT_SUM_FACET).count).to eq 0 - expect(subject).to have_direct_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) + expect(subject).to have_aggregate(EPIC_TYPE, COUNT, OPENED_EPIC_STATE, 0) + expect(subject).to have_aggregate(EPIC_TYPE, COUNT, CLOSED_EPIC_STATE, 0) end end - context 'with a nonzero count and nonzero weight for a single state' do - let(:fake_data) do + context 'with an issue with 0 weight', :aggregate_results do + let(:flat_info) do [ - { iid: epic_iid, epic_state_id: OPENED_EPIC_STATE, issues_count: 1, issues_weight_sum: 2, parent_id: nil, issues_state_id: OPENED_ISSUE_STATE } + record_for(epic_id: epic_id, parent_id: nil, epic_state_id: CLOSED_EPIC_STATE, issues_state_id: OPENED_ISSUE_STATE, issues_count: 1, issues_weight_sum: 0) ] end - it 'creates two sums' do - subject.assemble_issue_totals + it 'has the correct aggregates', :aggregate_failures do + expect(subject).to have_aggregate(ISSUE_TYPE, COUNT, OPENED_ISSUE_STATE, 1) + expect(subject).to have_aggregate(ISSUE_TYPE, COUNT, CLOSED_ISSUE_STATE, 0) + expect(subject).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM, OPENED_ISSUE_STATE, 0) + expect(subject).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM, CLOSED_ISSUE_STATE, 0) - expect(subject).to have_direct_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) - expect(subject).to have_direct_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) + expect(subject).to have_aggregate(EPIC_TYPE, COUNT, OPENED_EPIC_STATE, 0) + expect(subject).to have_aggregate(EPIC_TYPE, COUNT, CLOSED_EPIC_STATE, 0) end end - context 'with a nonzero count and nonzero weight for multiple states' do - let(:fake_data) do + context 'with an issue with nonzero weight' do + let(:flat_info) do [ - { iid: epic_iid, epic_state_id: OPENED_EPIC_STATE, issues_count: 1, issues_weight_sum: 2, parent_id: nil, issues_state_id: OPENED_ISSUE_STATE }, - { iid: epic_iid, epic_state_id: OPENED_EPIC_STATE, issues_count: 3, issues_weight_sum: 5, parent_id: nil, issues_state_id: CLOSED_ISSUE_STATE } + record_for(epic_id: epic_id, parent_id: nil, epic_state_id: CLOSED_EPIC_STATE, issues_state_id: OPENED_ISSUE_STATE, issues_count: 1, issues_weight_sum: 2) ] end - it 'creates two sums' do - subject.assemble_issue_totals - - expect(subject).to have_direct_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) - expect(subject).to have_direct_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) - expect(subject).to have_direct_total(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 3) - expect(subject).to have_direct_total(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 5) - end - end - end - end - - describe '#assemble_epic_totals' do - subject { described_class.new(epic_id, [{ parent_id: nil, epic_state_id: CLOSED_EPIC_STATE }]) } - - context 'with a child epic' do - let(:child_epic_id) { 45 } - let!(:child_epic_node) { described_class.new(child_epic_id, [{ parent_id: epic_id, epic_state_id: CLOSED_EPIC_STATE }]) } - - before do - subject.children << child_epic_node - end - - it 'adds up the number of the child epics' do - subject.assemble_epic_totals - - expect(subject).to have_direct_total(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) - end - end - end - - describe '#calculate_recursive_sums' do - subject { described_class.new(epic_id, [{ parent_id: nil, epic_state_id: CLOSED_EPIC_STATE }]) } - - before do - allow(subject).to receive(:direct_totals).with(COUNT_FACET).and_return(immediate_count_totals) - allow(subject).to receive(:direct_totals).with(WEIGHT_SUM_FACET).and_return(immediate_weight_sum_totals) - end - - shared_examples 'returns calculated totals by facet' do |facet, count| - it 'returns a calculated_count_total' do - subject.calculate_recursive_sums(facet, tree) - - expect(subject.calculated_totals(facet).count).to eq count - end - end - - context 'an epic with no child epics' do - let(:tree) do - { epic_id => subject } - end - - context 'with no child issues' do - let(:immediate_count_totals) { [] } - let(:immediate_weight_sum_totals) { [] } - - it_behaves_like 'returns calculated totals by facet', COUNT_FACET, 0 - it_behaves_like 'returns calculated totals by facet', WEIGHT_SUM_FACET, 0 - end - - context 'with an issue with 0 weight' do - let(:immediate_count_totals) do - [Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 1)] - end - let(:immediate_weight_sum_totals) { [] } - - it_behaves_like 'returns calculated totals by facet', COUNT_FACET, 1 - it_behaves_like 'returns calculated totals by facet', WEIGHT_SUM_FACET, 0 - end + it 'has the correct aggregates', :aggregate_failures do + expect(subject).to have_aggregate(ISSUE_TYPE, COUNT, OPENED_ISSUE_STATE, 1) + expect(subject).to have_aggregate(ISSUE_TYPE, COUNT, CLOSED_ISSUE_STATE, 0) + expect(subject).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM, OPENED_ISSUE_STATE, 2) + expect(subject).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM, CLOSED_ISSUE_STATE, 0) - context 'with an issue with nonzero weight' do - let(:immediate_count_totals) do - [ - Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 1) - ] - end - let(:immediate_weight_sum_totals) do - [ - Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(WEIGHT_SUM_FACET, CLOSED_EPIC_STATE, ISSUE_TYPE, 2) - ] + expect(subject).to have_aggregate(EPIC_TYPE, COUNT, OPENED_EPIC_STATE, 0) + expect(subject).to have_aggregate(EPIC_TYPE, COUNT, CLOSED_EPIC_STATE, 0) end - - it_behaves_like 'returns calculated totals by facet', COUNT_FACET, 1 - it_behaves_like 'returns calculated totals by facet', WEIGHT_SUM_FACET, 1 end end context 'an epic with child epics' do let(:child_epic_id) { 45 } - let(:tree) do - { epic_id => subject, child_epic_id => child_epic_node } - end - let(:child_epic_node) { described_class.new(child_epic_id, [{ parent_id: epic_id, epic_state_id: CLOSED_EPIC_STATE }]) } - let(:immediate_count_totals) do - [ # only one opened epic, the child - Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, OPENED_EPIC_STATE, EPIC_TYPE, 1) + let(:child_epic_node) { described_class.new(child_epic_id, child_flat_info) } + let(:flat_info) do + [ + record_for(epic_id: epic_id, parent_id: nil, epic_state_id: OPENED_EPIC_STATE, issues_state_id: OPENED_ISSUE_STATE, issues_count: 0, issues_weight_sum: 0) ] end - let(:immediate_weight_sum_totals) { [] } before do subject.children << child_epic_node - allow(child_epic_node).to receive(:direct_totals).with(COUNT_FACET).and_return(child_count_totals) - allow(child_epic_node).to receive(:direct_totals).with(WEIGHT_SUM_FACET).and_return(child_weight_sum_totals) end context 'with a child that has issues of nonzero weight' do - let(:child_count_totals) do + let(:child_flat_info) do [ - Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(COUNT_FACET, OPENED_ISSUE_STATE, ISSUE_TYPE, 1) + record_for(epic_id: epic_id, parent_id: nil, epic_state_id: OPENED_EPIC_STATE, issues_state_id: OPENED_ISSUE_STATE, issues_count: 1, issues_weight_sum: 2) ] end - let(:child_weight_sum_totals) do - [ - Gitlab::Graphql::Aggregations::Epics::EpicNode::Sum.new(WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, ISSUE_TYPE, 2) - ] - end - - it 'returns the correct count total' do - subject.calculate_recursive_sums(COUNT_FACET, tree) - - expect(subject).to have_calculated_total(ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 1) - end - - it 'returns the correct weight sum total' do - subject.calculate_recursive_sums(WEIGHT_SUM_FACET, tree) - expect(subject).to have_calculated_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 2) + it 'has the correct aggregates', :aggregate_failures do + expect(subject).to have_aggregate(ISSUE_TYPE, COUNT, OPENED_ISSUE_STATE, 1) + expect(subject).to have_aggregate(ISSUE_TYPE, COUNT, CLOSED_ISSUE_STATE, 0) + expect(subject).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM, OPENED_ISSUE_STATE, 2) + expect(subject).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM, CLOSED_ISSUE_STATE, 0) + expect(subject).to have_aggregate(EPIC_TYPE, COUNT, OPENED_EPIC_STATE, 1) + expect(subject).to have_aggregate(EPIC_TYPE, COUNT, CLOSED_EPIC_STATE, 0) end end end end + + def record_for(epic_id:, parent_id:, epic_state_id:, issues_state_id:, issues_count:, issues_weight_sum:) + { + epic_id: epic_id, + issues_count: issues_count, + issues_weight_sum: issues_weight_sum, + parent_id: parent_id, + issues_state_id: issues_state_id, + epic_state_id: epic_state_id + } + end end diff --git a/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb b/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb index f8e7118f15e59e..a19a875fd7eb8b 100644 --- a/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb +++ b/ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb @@ -21,20 +21,20 @@ context 'with valid facets :weight_sum or :count' do specify 'as a symbol', :aggregate_failures do - [:weight_sum, :count].each do |valid_facet| - described_class.new(query_ctx, epic_id, valid_facet) + [WEIGHT_SUM, COUNT].each do |valid_facet| + expect { described_class.new(query_ctx, epic_id, valid_facet) }.not_to raise_error end end specify 'as a string', :aggregate_failures do %w(weight_sum count).each do |valid_facet| - described_class.new(query_ctx, epic_id, valid_facet) + expect { described_class.new(query_ctx, epic_id, valid_facet) }.not_to raise_error end end end it 'adds the epic_id to lazy state' do - described_class.new(query_ctx, epic_id, :count) + described_class.new(query_ctx, epic_id, COUNT) expect(query_ctx[:lazy_epic_aggregate][:pending_ids]).to match [epic_id] end @@ -46,7 +46,7 @@ end let(:epic_info_node) { Gitlab::Graphql::Aggregations::Epics::EpicNode.new(epic_id, [single_record] ) } - subject { described_class.new(query_ctx, epic_id, :count) } + subject { described_class.new(query_ctx, epic_id, COUNT) } before do subject.instance_variable_set(:@lazy_state, fake_state) @@ -79,8 +79,9 @@ end before do - allow(Gitlab::Graphql::Aggregations::Epics::EpicNode).to receive(:aggregate_count).and_call_original - expect_any_instance_of(Gitlab::Graphql::Loaders::BulkEpicAggregateLoader).to receive(:execute).and_return(fake_data) + expect_next_instance_of(Gitlab::Graphql::Loaders::BulkEpicAggregateLoader) do |loader| + expect(loader).to receive(:execute).and_return(fake_data) + end end it 'clears the pending IDs' do @@ -94,53 +95,38 @@ it 'creates the parent-child associations', :aggregate_failures do subject.epic_aggregate - lazy_state = subject.instance_variable_get(:@lazy_state) - tree = lazy_state[:tree] - expect(tree[child_epic_id].parent_id).to eq epic_id expect(tree[epic_id].children.map(&:epic_id)).to match_array([child_epic_id]) end context 'for a parent-child relationship' do - it 'assembles direct sums', :aggregate_failures do - subject.epic_aggregate - - lazy_state = subject.instance_variable_get(:@lazy_state) - tree = lazy_state[:tree] - - expect(tree[epic_id]).to have_direct_total(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) - expect(tree[epic_id]).to have_direct_total(ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 5) - expect(tree[epic_id]).to have_direct_total(EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) - - expect(tree[child_epic_id]).to have_direct_total(ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 4) - expect(tree[child_epic_id]).to have_direct_total(ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 17) - end - it 'assembles recursive sums for the parent', :aggregate_failures do subject.epic_aggregate - lazy_state = subject.instance_variable_get(:@lazy_state) - tree = lazy_state[:tree] - - expect(tree[epic_id]).to have_aggregate(tree, ISSUE_TYPE, COUNT_FACET, OPENED_ISSUE_STATE, 2) - expect(tree[epic_id]).to have_aggregate(tree, ISSUE_TYPE, COUNT_FACET, CLOSED_ISSUE_STATE, 4) - expect(tree[epic_id]).to have_aggregate(tree, ISSUE_TYPE, WEIGHT_SUM_FACET, OPENED_ISSUE_STATE, 5) - expect(tree[epic_id]).to have_aggregate(tree, ISSUE_TYPE, WEIGHT_SUM_FACET, CLOSED_ISSUE_STATE, 17) - expect(tree[epic_id]).to have_aggregate(tree, EPIC_TYPE, COUNT_FACET, CLOSED_EPIC_STATE, 1) + expect(tree[epic_id]).to have_aggregate(ISSUE_TYPE, COUNT, OPENED_ISSUE_STATE, 2) + expect(tree[epic_id]).to have_aggregate(ISSUE_TYPE, COUNT, CLOSED_ISSUE_STATE, 4) + expect(tree[epic_id]).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM, OPENED_ISSUE_STATE, 5) + expect(tree[epic_id]).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM, CLOSED_ISSUE_STATE, 17) + expect(tree[epic_id]).to have_aggregate(EPIC_TYPE, COUNT, CLOSED_EPIC_STATE, 1) end end context 'for a standalone epic with no issues' do - it 'assembles direct totals', :aggregate_failures do + it 'assembles recursive sums', :aggregate_failures do subject.epic_aggregate - lazy_state = subject.instance_variable_get(:@lazy_state) - tree = lazy_state[:tree] - - expect(tree[other_epic_id].direct_count_totals).to be_empty - expect(tree[other_epic_id].direct_weight_sum_totals).to be_empty + expect(tree[other_epic_id]).to have_aggregate(ISSUE_TYPE, COUNT, OPENED_ISSUE_STATE, 0) + expect(tree[other_epic_id]).to have_aggregate(ISSUE_TYPE, COUNT, CLOSED_ISSUE_STATE, 0) + expect(tree[other_epic_id]).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM, OPENED_ISSUE_STATE, 0) + expect(tree[other_epic_id]).to have_aggregate(ISSUE_TYPE, WEIGHT_SUM, CLOSED_ISSUE_STATE, 0) + expect(tree[other_epic_id]).to have_aggregate(EPIC_TYPE, COUNT, CLOSED_EPIC_STATE, 0) end end end end + + def tree + lazy_state = subject.instance_variable_get(:@lazy_state) + lazy_state[:tree] + end end diff --git a/ee/spec/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader_spec.rb b/ee/spec/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader_spec.rb index 9510ce2529f109..88f90178116a8b 100644 --- a/ee/spec/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader_spec.rb +++ b/ee/spec/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader_spec.rb @@ -87,6 +87,12 @@ expect(result.keys).to match_array([parent_epic.id, epic_with_issues.id, epic_without_issues.id]) end + it 'errors when the number of retrieved records exceeds the maximum' do + stub_const("Gitlab::Graphql::Loaders::BulkEpicAggregateLoader::MAXIMUM_LOADABLE", 1) + + expect { subject.execute }.to raise_error(ArgumentError, /too many records/) + end + context 'testing for a single database query' do it 'does not repeat database queries for subepics' do recorder = ActiveRecord::QueryRecorder.new { described_class.new(epic_ids: epic_with_issues.id).execute } @@ -128,6 +134,14 @@ end def result_for(epic, issues_state:, issues_count:, issues_weight_sum:) - { id: epic.id, iid: epic.iid, issues_count: issues_count, issues_weight_sum: issues_weight_sum, parent_id: epic.parent_id, issues_state_id: issues_state, epic_state_id: Epic.available_states[epic.state_id] }.stringify_keys + { + id: epic.id, + iid: epic.iid, + issues_count: issues_count, + issues_weight_sum: issues_weight_sum, + parent_id: epic.parent_id, + issues_state_id: issues_state, + epic_state_id: Epic.available_states[epic.state_id] + }.stringify_keys end end diff --git a/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb b/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb index 33d435580493fa..9d9cc610bc3279 100644 --- a/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb +++ b/ee/spec/support/matchers/ee/epic_aggregate_matchers.rb @@ -1,38 +1,13 @@ # frozen_string_literal: true -%w[direct calculated].each do |total_type| - RSpec::Matchers.define :"have_#{total_type}_total" do |type, facet, state, expected_value| - match do |epic_node_result| - expect(epic_node_result).not_to be_nil - totals = epic_node_result.public_send("#{total_type}_totals", facet) - expect(totals).not_to be_empty - - matching = totals.select { |sum| sum.type == type && sum.facet == facet && sum.state == state && sum.value == expected_value } - expect(matching).not_to be_empty - end - - failure_message do |epic_node_result| - if epic_node_result.nil? - "expected for there to be an epic node, but it is nil" - else - totals = epic_node_result.public_send("#{total_type}_totals", facet) - <<~FAILURE_MSG - expected epic node with id #{epic_node_result.epic_id} to have a sum with facet '#{facet}', state '#{state}', type '#{type}' and value '#{expected_value}'. Has #{totals.count} #{total_type} sum objects#{", none of which match" if totals.count > 0}. - Sums: #{totals.inspect} - FAILURE_MSG - end - end - end -end - -RSpec::Matchers.define :have_aggregate do |tree, type, facet, state, expected_value| +RSpec::Matchers.define :have_aggregate do |type, facet, state, expected_value| match do |epic_node_result| - aggregate_object = epic_node_result.public_send(:"aggregate_#{facet}", tree) + aggregate_object = epic_node_result.public_send(:"aggregate_#{facet}") expect(aggregate_object.public_send(method_name(type, state))).to eq expected_value end failure_message do |epic_node_result| - aggregate_object = epic_node_result.public_send(:"aggregate_#{facet}", tree) + aggregate_object = epic_node_result.public_send(:"aggregate_#{facet}") aggregate_method = method_name(type, state) "Epic node with id #{epic_node_result.epic_id} called #{aggregate_method} on aggregate object. Value was expected to be #{expected_value} but was #{aggregate_object.send(aggregate_method)}." end diff --git a/ee/spec/support/shared_contexts/epic_aggregate_constants.rb b/ee/spec/support/shared_contexts/epic_aggregate_constants.rb index 6bacad0fff6d09..aea8333fb18d37 100644 --- a/ee/spec/support/shared_contexts/epic_aggregate_constants.rb +++ b/ee/spec/support/shared_contexts/epic_aggregate_constants.rb @@ -9,6 +9,6 @@ OPENED_ISSUE_STATE = Gitlab::Graphql::Aggregations::Epics::Constants::OPENED_ISSUE_STATE CLOSED_ISSUE_STATE = Gitlab::Graphql::Aggregations::Epics::Constants::CLOSED_ISSUE_STATE - WEIGHT_SUM_FACET = Gitlab::Graphql::Aggregations::Epics::Constants::WEIGHT_SUM - COUNT_FACET = Gitlab::Graphql::Aggregations::Epics::Constants::COUNT + WEIGHT_SUM = Gitlab::Graphql::Aggregations::Epics::Constants::WEIGHT_SUM + COUNT = Gitlab::Graphql::Aggregations::Epics::Constants::COUNT end -- GitLab