Merge branch 'mk/add-tests-for-sync-object-storage-attachments' into 'master'

Geo: Add tests for sync object storage attachments

Closes #229070

See merge request gitlab-org/gitlab!37239

ee/app/finders/geo/attachment_registry_finder.rb

@@ -43,7 +43,7 @@ module Geo
     end
 
     def replicables
-      current_node(fdw: false).attachments
+      ::Upload.replicables_for_geo_node
     end
 
     def syncable

ee/app/models/concerns/geo/selective_sync.rb

@@ -3,11 +3,6 @@
 module Geo::SelectiveSync
   extend ActiveSupport::Concern
 
-  # @return [ActiveRecord::Relation<Upload>] scope of everything that should be synced
-  def attachments
-    attachments_selective_sync_scope.merge(attachments_object_storage_scope)
-  end
-
   def projects_outside_selected_namespaces
     return project_model.none unless selective_sync_by_namespaces?
 
@@ -44,24 +39,35 @@ module Geo::SelectiveSync
     selective_sync_type == 'shards'
   end
 
-  private
-
-  # @return [ActiveRecord::Relation<Upload>] scope observing object storage settings
-  def attachments_object_storage_scope
-    return uploads_model.all if sync_object_storage?
-
-    uploads_model.with_files_stored_locally
-  end
-
-  # @return [ActiveRecord::Relation<Upload>] scope observing selective sync settings
-  def attachments_selective_sync_scope
-    if selective_sync?
-      uploads_model.where(group_attachments.or(project_attachments).or(other_attachments))
+  # This method should only be used when:
+  #
+  # - Selective sync is enabled
+  # - A replicable model is associated to Namespace but not to any Project
+  #
+  # When selectively syncing by namespace: We must sync every replicable of
+  # every selected namespace and descendent namespaces.
+  #
+  # When selectively syncing by shard: We must sync every replicable of every
+  # namespace of every project in those shards. We must also sync every ancestor
+  # of those namespaces.
+  #
+  # When selective sync is disabled: This method raises, instead of returning
+  # the technically correct `Namespace.all`, because it is easy for it to become
+  # part of an unnecessarily complex and inefficient query.
+  #
+  # @return [ActiveRecord::Relation<Namespace>] returns namespaces based on selective sync settings
+  def namespaces_for_group_owned_replicables
+    if selective_sync_by_namespaces?
+      selected_namespaces_and_descendants
+    elsif selective_sync_by_shards?
+      selected_leaf_namespaces_and_ancestors
     else
-      uploads_model.all
+      raise 'This scope should not be needed without selective sync'
     end
   end
 
+  private
+
   def selected_namespaces_and_descendants
     relation = selected_namespaces_and_descendants_cte.apply_to(namespaces_model.all)
     read_only(relation)
@@ -119,35 +125,6 @@ module Geo::SelectiveSync
     relation
   end
 
-  def group_attachments
-    namespaces =
-      if selective_sync_by_namespaces?
-        selected_namespaces_and_descendants
-      elsif selective_sync_by_shards?
-        selected_leaf_namespaces_and_ancestors
-      else
-        namespaces_model.none
-      end
-
-    attachments_for_model_type_with_id_in('Namespace', namespaces.select(:id))
-  end
-
-  def project_attachments
-    attachments_for_model_type_with_id_in('Project', projects.select(:id))
-  end
-
-  def other_attachments
-    uploads_table[:model_type].not_in(%w[Namespace Project])
-  end
-
-  def attachments_for_model_type_with_id_in(model_type, model_ids)
-    uploads_table[:model_type]
-        .eq(model_type)
-        .and(
-          uploads_table[:model_id].in(model_ids.arel)
-        )
-  end
-
   # This concern doesn't define a geo_node_namespace_links relation. That's
   # done in ::GeoNode or ::Geo::Fdw::GeoNode respectively. So when we use the
   # same code from the two places, they act differently - the first doesn't
@@ -178,13 +155,4 @@ module Geo::SelectiveSync
   def projects_table
     project_model.arel_table
   end
-
-  def uploads_model
-    raise NotImplementedError,
-      "#{self.class} does not implement #{__method__}"
-  end
-
-  def uploads_table
-    uploads_model.arel_table
-  end
 end

ee/app/models/ee/upload.rb

@@ -15,6 +15,46 @@ module EE
       scope :syncable, -> { with_files_stored_locally }
     end
 
+    class_methods do
+      # @return [ActiveRecord::Relation<Upload>] scope of everything that should be synced to this node
+      def replicables_for_geo_node(node = ::Gitlab::Geo.current_node)
+        selective_sync_scope(node).merge(object_storage_scope(node))
+      end
+
+      private
+
+      # @return [ActiveRecord::Relation<Upload>] scope observing object storage settings of the given node
+      def object_storage_scope(node)
+        return all if node.sync_object_storage?
+
+        with_files_stored_locally
+      end
+
+      # @return [ActiveRecord::Relation<Upload>] scope observing selective sync settings of the given node
+      def selective_sync_scope(node)
+        if node.selective_sync?
+          group_attachments(node).or(project_attachments(node)).or(other_attachments)
+        else
+          all
+        end
+      end
+
+      # @return [ActiveRecord::Relation<Upload>] scope of Namespace-associated uploads observing selective sync settings of the given node
+      def group_attachments(node)
+        where(model_type: 'Namespace', model_id: node.namespaces_for_group_owned_replicables.select(:id))
+      end
+
+      # @return [ActiveRecord::Relation<Upload>] scope of Project-associated uploads observing selective sync settings of the given node
+      def project_attachments(node)
+        where(model_type: 'Project', model_id: node.projects.select(:id))
+      end
+
+      # @return [ActiveRecord::Relation<Upload>] scope of uploads which are not associated with Namespace or Project
+      def other_attachments
+        where.not(model_type: %w[Namespace Project])
+      end
+    end
+
     def log_geo_deleted_event
       ::Geo::UploadDeletedEventStore.new(self).create!
     end

ee/app/models/geo/fdw/geo_node.rb

@@ -80,10 +80,6 @@ module Geo
       def project_model
         Geo::Fdw::Project
       end
-
-      def uploads_model
-        Geo::Fdw::Upload
-      end
     end
   end
 end

ee/spec/models/upload_spec.rb

@@ -4,6 +4,117 @@ require 'spec_helper'
 
 RSpec.describe Upload do
   include EE::GeoHelpers
+  using RSpec::Parameterized::TableSyntax
+
+  describe '.replicables_for_geo_node' do
+    # Selective sync is configured relative to the upload's model. Take care not
+    # to specify a model_factory that contradicts factory.
+    #
+    # Permutations of sync_object_storage combined with object-stored-uploads
+    # are tested in code, because the logic is simple, and to do it in the table
+    # would quadruple its size and have too much duplication.
+    where(:selective_sync_namespaces, :selective_sync_shards, :factory, :model_factory, :is_upload_included) do
+      nil                  | nil            | [:upload]                    | [:project]               | true
+      nil                  | nil            | [:upload, :issuable_upload]  | [:project]               | true
+      nil                  | nil            | [:upload, :namespace_upload] | [:group]                 | true
+      nil                  | nil            | [:upload, :favicon_upload]   | [:appearance]            | true
+      # selective sync by shard
+      nil                  | :model         | [:upload]                    | [:project]               | true
+      nil                  | :other         | [:upload]                    | [:project]               | false
+      nil                  | :model_project | [:upload, :namespace_upload] | [:group]                 | true
+      nil                  | :other         | [:upload, :namespace_upload] | [:group]                 | false
+      nil                  | :other         | [:upload, :favicon_upload]   | [:appearance]            | true
+      # selective sync by namespace
+      :model_parent        | nil            | [:upload]                    | [:project]               | true
+      :model_parent_parent | nil            | [:upload]                    | [:project, :in_subgroup] | true
+      :model               | nil            | [:upload, :namespace_upload] | [:group]                 | true
+      :model_parent        | nil            | [:upload, :namespace_upload] | [:group, :nested]        | true
+      :other               | nil            | [:upload]                    | [:project]               | false
+      :other               | nil            | [:upload]                    | [:project, :in_subgroup] | false
+      :other               | nil            | [:upload, :namespace_upload] | [:group]                 | false
+      :other               | nil            | [:upload, :namespace_upload] | [:group, :nested]        | false
+      :other               | nil            | [:upload, :favicon_upload]   | [:appearance]            | true
+    end
+
+    with_them do
+      subject(:upload_included) { described_class.replicables_for_geo_node.include?(upload) }
+
+      let(:model) { create(*model_factory) }
+      let(:node) { create_geo_node(model, sync_object_storage: sync_object_storage) }
+
+      before do
+        stub_current_geo_node(node)
+      end
+
+      context 'when sync object storage is enabled' do
+        let(:sync_object_storage) { true }
+
+        context 'when the upload is locally stored' do
+          let(:upload) { create(*factory, model: model) }
+
+          it { is_expected.to eq(is_upload_included) }
+        end
+
+        context 'when the upload is object stored' do
+          let(:upload) { create(*factory, :object_storage, model: model) }
+
+          it { is_expected.to eq(is_upload_included) }
+        end
+      end
+
+      context 'when sync object storage is disabled' do
+        let(:sync_object_storage) { false }
+
+        context 'when the upload is locally stored' do
+          let(:upload) { create(*factory, model: model) }
+
+          it { is_expected.to eq(is_upload_included) }
+        end
+
+        context 'when the upload is object stored' do
+          let(:upload) { create(*factory, :object_storage, model: model) }
+
+          it { is_expected.to be_falsey }
+        end
+      end
+    end
+
+    def create_geo_node(model, sync_object_storage:)
+      node = build(:geo_node)
+
+      if selective_sync_namespaces
+        node.selective_sync_type = 'namespaces'
+      elsif selective_sync_shards
+        node.selective_sync_type = 'shards'
+      end
+
+      case selective_sync_namespaces
+      when :model
+        node.namespaces = [model]
+      when :model_parent
+        node.namespaces = [model.parent]
+      when :model_parent_parent
+        node.namespaces = [model.parent.parent]
+      when :other
+        node.namespaces = [create(:group)]
+      end
+
+      case selective_sync_shards
+      when :model
+        node.selective_sync_shards = [model.repository_storage]
+      when :model_project
+        project = create(:project, namespace: model)
+        node.selective_sync_shards = [project.repository_storage]
+      when :other
+        node.selective_sync_shards = ['other_shard_name']
+      end
+
+      node.sync_object_storage = sync_object_storage
+
+      node.save!
+      node
+    end
+  end
 
   describe '#destroy' do
     subject { create(:upload, checksum: '8710d2c16809c79fee211a9693b64038a8aae99561bc86ce98a9b46b45677fe4') }

spec/factories/projects.rb

@@ -389,4 +389,8 @@ FactoryBot.define do
       create(:design, project: project, issue: issue)
     end
   end
+
+  trait :in_subgroup do
+    namespace factory: [:group, :nested]
+  end
 end