Fix setting share_with_group_lockfix-share-with-group-lock-update

Prior to this commit running
Namespace#force_share_with_group_lock_on_descendants would result in
updating _all_ namespaces in the namespaces table, not just the
descendants. This is the result of ActiveRecord::Relation#update_all not
taking into account the CTE. To work around this we use the CTE query as
a sub-query instead of directly calling #update_all.

To prevent this from happening the relations returned by
Gitlab::GroupHierarchy are now marked as read-only, resulting in an
error being raised when methods such as #update_all are used.

Fortunately on GitLab.com our statement timeouts appear to have
prevented this query from actually doing any damage other than causing
a very large amount of dead tuples.

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/37916

4 files changed, 51 insertions, 4 deletions

diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 4a9a23fea1f..e279d8dd8c5 100644
--- a/app/models/namespace.rb
+++ b/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
diff --git a/lib/gitlab/database/read_only_relation.rb b/lib/gitlab/database/read_only_relation.rb
new file mode 100644
index 00000000000..4571ad122ce
--- /dev/null
+++ b/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
diff --git a/lib/gitlab/group_hierarchy.rb b/lib/gitlab/group_hierarchy.rb
index 5a31e56cb30..635f52131f9 100644
--- a/lib/gitlab/group_hierarchy.rb
+++ b/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
diff --git a/spec/lib/gitlab/group_hierarchy_spec.rb b/spec/lib/gitlab/group_hierarchy_spec.rb
index 08010c2d0e2..8dc83a6db7f 100644
--- a/spec/lib/gitlab/group_hierarchy_spec.rb
+++ b/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