Merge branch '10546-fix-epic-depth-validation' into 'master'

Fix Epic depth validation

Closes #10546

See merge request gitlab-org/gitlab-ee!10331

ee/app/models/ee/epic.rb

@@ -157,30 +157,13 @@ module EE
       # epic2 - parent: epic1
       # Returns: 2
       def deepest_relationship_level
-        return unless ::Group.supports_nested_objects?
-
-        result =
-          ApplicationRecord.connection.execute(
-            <<-SQL
-              WITH RECURSIVE descendants AS (
-                  SELECT id, 1 depth
-                  FROM epics
-                  WHERE parent_id IS NOT NULL
-              UNION
-                  SELECT e.id, d.depth + 1
-                  FROM epics e
-                  INNER JOIN descendants d
-                  ON e.parent_id = d.id
-              )
-              SELECT MAX(depth) as deepest_level FROM descendants
-            SQL
-          )
+        return unless supports_nested_objects?
 
-        deepest_level = result.first["deepest_level"] || 0
+        hierarchy = ::Gitlab::ObjectHierarchy.new(self.where.not(parent_id: nil))
+        deepest_level = hierarchy.max_descendants_depth || 0
 
-        # For performance reason epics without a parent_id are being
-        # ignored in the query.
-        # So we sum 1 to the result to take into account first parent.
+        # For performance reasons, epics without a parent_id are being ignored in the query.
+        # So we add 1 to the result to take into account the first parent.
         deepest_level + 1
       end
 

ee/app/services/epic_links/create_service.rb

@@ -61,11 +61,7 @@ module EpicLinks
     end
 
     def level_depth_exceeded?(epic)
-      depth_level(epic) + parent_ancestors_count >= 5
-    end
-
-    def depth_level(epic)
-      epic.descendants.count + 1 # level including epic -> therefore +1
+      epic.hierarchy.max_descendants_depth + parent_ancestors_count >= 5
     end
 
     def parent_ancestors_count

ee/changelogs/unreleased/10546-fix-epic-depth-validation.yml

+---
+title: Fixed bug preventing users from adding child epics with multiple children
+merge_request: 10331
+author:
+type: fixed

ee/db/migrate/20190327085945_remove_cyclic_hierarchies_in_epics.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveCyclicHierarchiesInEpics < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    return unless Gitlab::Database.postgresql?
+
+    epics_in_loops_sql = <<~SQL
+      WITH RECURSIVE base_and_descendants (id, path, cycle) AS (
+        SELECT epics.id, ARRAY[epics.id], false FROM epics WHERE epics.parent_id IS NOT NULL
+      UNION
+          SELECT epics.id, path || epics.id, epics.id = ANY(path)
+          FROM epics, base_and_descendants
+          WHERE epics.parent_id = base_and_descendants.id AND NOT cycle
+      )
+      SELECT id, array_to_string(path, ',') AS path FROM base_and_descendants WHERE cycle ORDER BY id
+    SQL
+
+    # Group by sorted path so we can group epics that belong to the same loop
+    epics_grouped_by_loop = select_all(epics_in_loops_sql).group_by { |r| r['path'].split(',').uniq.sort.join(',') }
+
+    # We only need to update the first epic of the loop to break the cycle
+    epic_ids_to_update = epics_grouped_by_loop.map { |path, epics| epics.first['id'] }
+
+    # rubocop:disable Lint/UnneededCopDisableDirective
+    # rubocop:disable Migration/UpdateColumnInBatches
+    update_column_in_batches(:epics, :parent_id, nil) do |table, query|
+      query.where(
+        table[:id].in(epic_ids_to_update)
+      )
+    end
+  end
+
+  def down
+  end
+end

ee/spec/migrations/remove_cyclic_hierarchies_in_epics_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('ee', 'db', 'migrate', '20190327085945_remove_cyclic_hierarchies_in_epics.rb')
+
+describe RemoveCyclicHierarchiesInEpics, :migration, :postgresql do
+  let(:epics) { table(:epics) }
+  let(:group) { table(:namespaces).create(name: 'Group 1', type: 'Group', path: 'group_1') }
+  let(:user) { table(:users).create!(email: 'email@email.com', name: 'foo', username: 'foo', projects_limit: 0) }
+
+  def create_epic_with_defaults!(attributes = {})
+    @last_iid += 1
+
+    epics.create!(
+      attributes.reverse_merge(
+        iid: @last_iid,
+        group_id: group.id,
+        author_id: user.id,
+        title: "Epic",
+        title_html: "Epic"
+      )
+    )
+  end
+
+  let!(:epic_self_loop) { create_epic_with_defaults! }
+  let!(:epic_loop_1) { create_epic_with_defaults! }
+  let!(:epic_loop_2) { create_epic_with_defaults! }
+
+  let!(:epic_not_in_loop) { create_epic_with_defaults! }
+
+  before(:all) do
+    @last_iid = 0
+  end
+
+  before do
+    epic_self_loop.update(parent_id: epic_self_loop.id)
+
+    epic_loop_1_1 = create_epic_with_defaults!(parent_id: epic_loop_1.id)
+    epic_loop_1.update(parent_id: epic_loop_1_1.id)
+
+    epic_loop_2_1 = create_epic_with_defaults!(parent_id: epic_loop_2.id)
+    epic_loop_2_2 = create_epic_with_defaults!(parent_id: epic_loop_2_1.id)
+    epic_loop_2_3 = create_epic_with_defaults!(parent_id: epic_loop_2_2.id)
+    epic_loop_2.update(parent_id: epic_loop_2_3.id)
+
+    epic_parent = create_epic_with_defaults!
+    epic_not_in_loop.update(parent_id: epic_parent.id)
+  end
+
+  it 'clears parent_id of epic in loop' do
+    migrate!
+
+    expect(epic_self_loop.reload.parent_id).to be_nil
+    expect(epic_loop_1.reload.parent_id).to be_nil
+    expect(epic_loop_2.reload.parent_id).to be_nil
+  end
+
+  it 'does not clear parent_id for other epics' do
+    migrate!
+
+    expect(epic_not_in_loop.reload.parent_id).not_to be_nil
+  end
+end

ee/spec/services/epic_links/create_service_spec.rb

@@ -190,45 +190,53 @@ describe EpicLinks::CreateService, :postgresql do
           end
         end
 
-        context 'when adding an epic would would exceed level 5 in hierarchy' do
-          context 'when adding to already deep structure' do
-            before do
-              epic1 = create(:epic, group: group)
-              epic2 = create(:epic, group: group, parent: epic1)
-              epic3 = create(:epic, group: group, parent: epic2)
-              epic4 = create(:epic, group: group, parent: epic3)
+        context 'when adding to an Epic that is already at maximum depth' do
+          before do
+            epic1 = create(:epic, group: group)
+            epic2 = create(:epic, group: group, parent: epic1)
+            epic3 = create(:epic, group: group, parent: epic2)
+            epic4 = create(:epic, group: group, parent: epic3)
 
-              epic.update(parent: epic4)
-            end
+            epic.update(parent: epic4)
+          end
 
-            subject { add_epic([valid_reference]) }
+          subject { add_epic([valid_reference]) }
 
-            it 'returns an error' do
-              expect(subject).to eq(message: 'Epic hierarchy level too deep', status: :error, http_status: 409)
-            end
+          it 'returns an error' do
+            expect(subject).to eq(message: 'Epic hierarchy level too deep', status: :error, http_status: 409)
+          end
 
-            it 'no relationship is created' do
-              expect { subject }.not_to change { epic.children.count }
-            end
+          it 'no relationship is created' do
+            expect { subject }.not_to change { epic.children.count }
           end
+        end
 
-          context 'when adding an epic already having some epics as children' do
+        context 'when adding an Epic that has existing children' do
+          subject { add_epic([valid_reference]) }
+
+          context 'when total depth after adding would exceed limit' do
             before do
               epic1 = create(:epic, group: group)
 
               epic.update(parent: epic1) # epic is on level 2
 
-              # epic_to_add has 3 children (level 4 inlcuding epic_to_add)
+              # epic_to_add has 3 children (level 4 including epic_to_add)
               # that would mean level 6 after relating epic_to_add on epic
               epic2 = create(:epic, group: group, parent: epic_to_add)
               epic3 = create(:epic, group: group, parent: epic2)
               create(:epic, group: group, parent: epic3)
             end
 
-            subject { add_epic([valid_reference]) }
-
             include_examples 'returns not found error'
           end
+
+          context 'when Epic to add has more than 5 children' do
+            before do
+              create_list(:epic, 8, group: group, parent: epic_to_add)
+            end
+
+            include_examples 'returns success'
+          end
         end
 
         context 'when an epic is already assigned to another epic' do

lib/gitlab/object_hierarchy.rb

@@ -5,6 +5,8 @@ module Gitlab
   #
   # This class uses recursive CTEs and as a result will only work on PostgreSQL.
   class ObjectHierarchy
+    DEPTH_COLUMN = :depth
+
     attr_reader :ancestors_base, :descendants_base, :model
 
     # ancestors_base - An instance of ActiveRecord::Relation for which to
@@ -27,6 +29,17 @@ module Gitlab
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
+    # Returns the maximum depth starting from the base
+    # A base object with no children has a maximum depth of `1`
+    def max_descendants_depth
+      unless hierarchy_supported?
+        # This makes the return value consistent with the case where hierarchy is supported
+        return descendants_base.exists? ? 1 : nil
+      end
+
+      base_and_descendants(with_depth: true).maximum(DEPTH_COLUMN)
+    end
+
     # Returns the set of ancestors of a given relation, but excluding the given
     # relation
     #
@@ -64,10 +77,15 @@ module Gitlab
 
     # Returns a relation that includes the descendants_base set of objects
     # and all their descendants (recursively).
-    def base_and_descendants
-      return descendants_base unless hierarchy_supported?
-
-      read_only(base_and_descendants_cte.apply_to(model.all))
+    #
+    # When `with_depth` is `true`, a `depth` column is included where it starts with `1` for the base objects
+    # and incremented as we go down the descendant tree
+    def base_and_descendants(with_depth: false)
+      unless hierarchy_supported?
+        return with_depth ? descendants_base.select("1 as #{DEPTH_COLUMN}", objects_table[Arel.star]) : descendants_base
+      end
+
+      read_only(base_and_descendants_cte(with_depth: with_depth).apply_to(model.all))
     end
 
     # Returns a relation that includes the base objects, their ancestors,
@@ -124,10 +142,9 @@ module Gitlab
     # rubocop: disable CodeReuse/ActiveRecord
     def base_and_ancestors_cte(stop_id = nil, hierarchy_order = nil)
       cte = SQL::RecursiveCTE.new(:base_and_ancestors)
-      depth_column = :depth
 
       base_query = ancestors_base.except(:order)
-      base_query = base_query.select("1 as #{depth_column}", objects_table[Arel.star]) if hierarchy_order
+      base_query = base_query.select("1 as #{DEPTH_COLUMN}", "ARRAY[id] AS tree_path", "false AS tree_cycle", objects_table[Arel.star]) if hierarchy_order
 
       cte << base_query
 
@@ -137,7 +154,17 @@ module Gitlab
         .where(objects_table[:id].eq(cte.table[:parent_id]))
         .except(:order)
 
-      parent_query = parent_query.select(cte.table[depth_column] + 1, objects_table[Arel.star]) if hierarchy_order
+      if hierarchy_order
+        quoted_objects_table_name = model.connection.quote_table_name(objects_table.name)
+
+        parent_query = parent_query.select(
+          cte.table[DEPTH_COLUMN] + 1,
+          "tree_path || #{quoted_objects_table_name}.id",
+          "#{quoted_objects_table_name}.id = ANY(tree_path)",
+          objects_table[Arel.star]
+        ).where(cte.table[:tree_cycle].eq(false))
+      end
+
       parent_query = parent_query.where(cte.table[:parent_id].not_eq(stop_id)) if stop_id
 
       cte << parent_query
@@ -146,17 +173,32 @@ module Gitlab
     # rubocop: enable CodeReuse/ActiveRecord
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def base_and_descendants_cte
+    def base_and_descendants_cte(with_depth: false)
       cte = SQL::RecursiveCTE.new(:base_and_descendants)
 
-      cte << descendants_base.except(:order)
+      base_query = descendants_base.except(:order)
+      base_query = base_query.select("1 AS #{DEPTH_COLUMN}", "ARRAY[id] AS tree_path", "false AS tree_cycle", objects_table[Arel.star]) if with_depth
+
+      cte << base_query
 
       # Recursively get all the descendants of the base set.
-      cte << model
+      descendants_query = model
         .from([objects_table, cte.table])
         .where(objects_table[:parent_id].eq(cte.table[:id]))
         .except(:order)
 
+      if with_depth
+        quoted_objects_table_name = model.connection.quote_table_name(objects_table.name)
+
+        descendants_query = descendants_query.select(
+          cte.table[DEPTH_COLUMN] + 1,
+          "tree_path || #{quoted_objects_table_name}.id",
+          "#{quoted_objects_table_name}.id = ANY(tree_path)",
+          objects_table[Arel.star]
+        ).where(cte.table[:tree_cycle].eq(false))
+      end
+
+      cte << descendants_query
       cte
     end
     # rubocop: enable CodeReuse/ActiveRecord

spec/lib/gitlab/object_hierarchy_spec.rb

@@ -81,6 +81,24 @@ describe Gitlab::ObjectHierarchy, :postgresql do
       expect { relation.update_all(share_with_group_lock: false) }
         .to raise_error(ActiveRecord::ReadOnlyRecord)
     end
+
+    context 'when with_depth is true' do
+      let(:relation) do
+        described_class.new(Group.where(id: parent.id)).base_and_descendants(with_depth: true)
+      end
+
+      it 'includes depth in the results' do
+        object_depths = {
+          parent.id => 1,
+          child1.id => 2,
+          child2.id => 3
+        }
+
+        relation.each do |object|
+          expect(object.depth).to eq(object_depths[object.id])
+        end
+      end
+    end
   end
 
   describe '#descendants' do
@@ -91,6 +109,28 @@ describe Gitlab::ObjectHierarchy, :postgresql do
     end
   end
 
+  describe '#max_descendants_depth' do
+    subject { described_class.new(base_relation).max_descendants_depth }
+
+    context 'when base relation is empty' do
+      let(:base_relation) { Group.where(id: nil) }
+
+      it { expect(subject).to be_nil }
+    end
+
+    context 'when base has no children' do
+      let(:base_relation) { Group.where(id: child2) }
+
+      it { expect(subject).to eq(1) }
+    end
+
+    context 'when base has grandchildren' do
+      let(:base_relation) { Group.where(id: parent) }
+
+      it { expect(subject).to eq(3) }
+    end
+  end
+
   describe '#ancestors' do
     it 'includes only the ancestors' do
       relation = described_class.new(Group.where(id: child2)).ancestors