Merge branch 'fix-share-with-group-lock-update' into 'master'

Fix setting share_with_group_lock

Closes #37916

See merge request gitlab-org/gitlab-ce!14300

app/models/namespace.rb

@@ -231,6 +231,13 @@ class Namespace < ActiveRecord::Base
   end
 
   def force_share_with_group_lock_on_descendants
-    descendants.update_all(share_with_group_lock: true)
+    return unless Group.supports_nested_groups?
+
+    # We can't use `descendants.update_all` since Rails will throw away the WITH
+    # RECURSIVE statement. We also can't use WHERE EXISTS since we can't use
+    # different table aliases, hence we're just using WHERE IN. Since we have a
+    # maximum of 20 nested groups this should be fine.
+    Namespace.where(id: descendants.select(:id))
+      .update_all(share_with_group_lock: true)
   end
 end

lib/gitlab/database/read_only_relation.rb

@@ -0,0 +1,16 @@
+module Gitlab
+  module Database
+    # Module that can be injected into a ActiveRecord::Relation to make it
+    # read-only.
+    module ReadOnlyRelation
+      [:delete, :delete_all, :update, :update_all].each do |method|
+        define_method(method) do |*args|
+          raise(
+            ActiveRecord::ReadOnlyRecord,
+            "This relation is marked as read-only"
+          )
+        end
+      end
+    end
+  end
+end

lib/gitlab/group_hierarchy.rb

@@ -22,7 +22,7 @@ module Gitlab
     def base_and_ancestors
       return ancestors_base unless Group.supports_nested_groups?
 
-      base_and_ancestors_cte.apply_to(model.all)
+      read_only(base_and_ancestors_cte.apply_to(model.all))
     end
 
     # Returns a relation that includes the descendants_base set of groups
@@ -30,7 +30,7 @@ module Gitlab
     def base_and_descendants
       return descendants_base unless Group.supports_nested_groups?
 
-      base_and_descendants_cte.apply_to(model.all)
+      read_only(base_and_descendants_cte.apply_to(model.all))
     end
 
     # Returns a relation that includes the base groups, their ancestors,
@@ -67,11 +67,13 @@ module Gitlab
       union = SQL::Union.new([model.unscoped.from(ancestors_table),
                               model.unscoped.from(descendants_table)])
 
-      model
+      relation = model
         .unscoped
         .with
         .recursive(ancestors.to_arel, descendants.to_arel)
         .from("(#{union.to_sql}) #{model.table_name}")
+
+      read_only(relation)
     end
 
     private
@@ -107,5 +109,12 @@ module Gitlab
     def groups_table
       model.arel_table
     end
+
+    def read_only(relation)
+      # relations using a CTE are not safe to use with update_all as it will
+      # throw away the CTE, hence we mark them as read-only.
+      relation.extend(Gitlab::Database::ReadOnlyRelation)
+      relation
+    end
   end
 end

spec/lib/gitlab/group_hierarchy_spec.rb

@@ -23,6 +23,11 @@ describe Gitlab::GroupHierarchy, :postgresql do
 
       expect(relation).to include(parent, child1, child2)
     end
+
+    it 'does not allow the use of #update_all' do
+      expect { relation.update_all(share_with_group_lock: false) }
+        .to raise_error(ActiveRecord::ReadOnlyRecord)
+    end
   end
 
   describe '#base_and_descendants' do
@@ -43,6 +48,11 @@ describe Gitlab::GroupHierarchy, :postgresql do
 
       expect(relation).to include(parent, child1, child2)
     end
+
+    it 'does not allow the use of #update_all' do
+      expect { relation.update_all(share_with_group_lock: false) }
+        .to raise_error(ActiveRecord::ReadOnlyRecord)
+    end
   end
 
   describe '#all_groups' do
@@ -73,5 +83,10 @@ describe Gitlab::GroupHierarchy, :postgresql do
 
       expect(relation).to include(child2)
     end
+
+    it 'does not allow the use of #update_all' do
+      expect { relation.update_all(share_with_group_lock: false) }
+        .to raise_error(ActiveRecord::ReadOnlyRecord)
+    end
   end
 end