Refactor legacy finder to find registries pending verification

These changes use the new finder when FDW is enabled without selective
sync to avoid code duplication.

ee/app/finders/geo/legacy_project_registry_pending_verification_finder.rb

@@ -2,7 +2,7 @@
 
 # Finder for retrieving project registries that need a repository or
 # wiki verification where projects belong to the specific shard
-# using cross-database joins for selective sync.
+# using cross-database joins.
 #
 # Basic usage:
 #
@@ -18,11 +18,18 @@ module Geo
     end
 
     def execute
-      if use_legacy_queries?
-        registries_pending_verification_for_selective_sync
-      else
-        registries_pending_verification
-      end
+      registries = find_registries_pending_verification_on_secondary
+      return Geo::ProjectRegistry.none if registries.empty?
+
+      registries_to_verify = filter_registries_verified_in_primary(registries)
+      return registries_to_verify unless selective_sync?
+
+      legacy_inner_join_registry_ids(
+        registries_to_verify,
+        current_node.projects.pluck_primary_key,
+        Geo::ProjectRegistry,
+        foreign_key: :project_id
+      )
     end
 
     private
@@ -30,55 +37,58 @@ module Geo
     attr_reader :batch_size, :shard_name
 
     # rubocop:disable CodeReuse/ActiveRecord
-    def registries_pending_verification
-      Geo::ProjectRegistry.all
-        .merge(Geo::Fdw::ProjectRegistry.registries_pending_verification)
-        .merge(Geo::Fdw::ProjectRegistry.within_shards(shard_name))
-        .limit(batch_size)
+    def find_registries_pending_verification_on_secondary
+      Geo::ProjectRegistry
+        .where(Geo::ProjectRegistry.registries_pending_verification)
+        .pluck(
+          :project_id,
+          Geo::ProjectRegistry.repositories_pending_verification.to_sql,
+          Geo::ProjectRegistry.wikis_pending_verification.to_sql
+        )
     end
     # rubocop:enable CodeReuse/ActiveRecord
 
-    # rubocop:disable CodeReuse/ActiveRecord
-    def registries_pending_verification_for_selective_sync
-      registries = Geo::ProjectRegistry
-        .where(Geo::ProjectRegistry.registries_pending_verification)
-        .pluck(:project_id, Geo::ProjectRegistry.repositories_pending_verification.to_sql, Geo::ProjectRegistry.wikis_pending_verification.to_sql)
-
-      return Geo::ProjectRegistry.none if registries.empty?
-
-      id_and_want_to_sync = registries.map do |project_id, want_to_sync_repo, want_to_sync_wiki|
-        "(#{project_id}, #{quote_value(want_to_sync_repo)}, #{quote_value(want_to_sync_wiki)})"
+    def filter_registries_verified_in_primary(registries)
+      filtered_project_ids = filter_projects_verified_on_primary(registries)
+      Geo::ProjectRegistry.project_id_in(filtered_project_ids)
     end
 
-      legacy_repository_state_table = ::ProjectRepositoryState.arel_table
-      project_registry_sync_table = Arel::Table.new(:project_registry_sync_table)
-
-      joined_relation =
-        ProjectRepositoryState.joins(<<~SQL_REPO)
-          INNER JOIN
-          (VALUES #{id_and_want_to_sync.join(',')})
-          project_registry_sync_table(project_id, want_to_sync_repo, want_to_sync_wiki)
-          ON #{legacy_repository_state_table.name}.project_id = project_registry_sync_table.project_id
-        SQL_REPO
-
-      project_ids = joined_relation
+    # rubocop:disable CodeReuse/ActiveRecord
+    def filter_projects_verified_on_primary(registries)
+      inner_join_project_repository_state(registries)
         .joins(:project)
-        .where(projects: { repository_storage: shard_name })
+        .merge(Project.within_shards(shard_name))
         .where(
           legacy_repository_state_table[:repository_verification_checksum].not_eq(nil)
-            .and(project_registry_sync_table[:want_to_sync_repo].eq(true))
+            .and(project_registry_verify_table[:want_to_verify_repo].eq(true))
           .or(legacy_repository_state_table[:wiki_verification_checksum].not_eq(nil)
-            .and(project_registry_sync_table[:want_to_sync_wiki].eq(true))))
+            .and(project_registry_verify_table[:want_to_verify_wiki].eq(true))))
         .limit(batch_size)
         .pluck_project_key
+    end
+    # rubocop:enable CodeReuse/ActiveRecord
 
-      legacy_inner_join_registry_ids(
-        Geo::ProjectRegistry.project_id_in(project_ids),
-        current_node.projects.pluck_primary_key,
-        Geo::ProjectRegistry,
-        foreign_key: :project_id
-      )
+    # rubocop:disable CodeReuse/ActiveRecord
+    def inner_join_project_repository_state(registries)
+      id_and_want_to_verify = registries.map do |project_id, want_to_verify_repo, want_to_verify_wiki|
+        "(#{project_id}, #{quote_value(want_to_verify_repo)}, #{quote_value(want_to_verify_wiki)})"
+      end
+
+      ProjectRepositoryState.joins(<<~SQL_REPO)
+        INNER JOIN
+        (VALUES #{id_and_want_to_verify.join(',')})
+        #{project_registry_verify_table.name}(project_id, want_to_verify_repo, want_to_verify_wiki)
+        ON #{legacy_repository_state_table.name}.project_id = #{project_registry_verify_table.name}.project_id
+      SQL_REPO
     end
     # rubocop:enable CodeReuse/ActiveRecord
+
+    def legacy_repository_state_table
+      @legacy_repository_state_table ||= ProjectRepositoryState.arel_table
+    end
+
+    def project_registry_verify_table
+      @project_registry_verify_table ||= Arel::Table.new(:project_registry_verify_table)
+    end
   end
 end

ee/app/finders/geo/project_registry_finder.rb

@@ -67,7 +67,9 @@ module Geo
     end
 
     def find_registries_to_verify(shard_name:, batch_size:)
-      registries_pending_verification(shard_name, batch_size)
+      finder_klass_for_registries_pending_verification
+        .new(current_node: current_node, shard_name: shard_name, batch_size: batch_size)
+        .execute
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
@@ -161,6 +163,10 @@ module Geo
 
     private
 
+    def use_legacy_queries_for_selective_sync?
+      selective_sync? && !Gitlab::Geo::Fdw.enabled_for_selective_sync?
+    end
+
     def finder_klass_for_synced_registries
       if Gitlab::Geo::Fdw.enabled_for_selective_sync?
         Geo::ProjectRegistrySyncedFinder
@@ -246,17 +252,11 @@ module Geo
     end
 
     def finder_klass_for_registries_pending_verification
-      if Gitlab::Geo::Fdw.enabled_for_selective_sync?
-        Geo::ProjectRegistryPendingVerificationFinder
-      else
+      if !Gitlab::Geo::Fdw.enabled? || use_legacy_queries_for_selective_sync?
         Geo::LegacyProjectRegistryPendingVerificationFinder
+      else
+        Geo::ProjectRegistryPendingVerificationFinder
       end
     end
-
-    def registries_pending_verification(shard_name, batch_size)
-      finder_klass_for_registries_pending_verification
-        .new(current_node: current_node, shard_name: shard_name, batch_size: batch_size)
-        .execute
-    end
   end
 end

ee/app/models/ee/project.rb

@@ -87,7 +87,7 @@ module EE
       end
 
       scope :with_wiki_enabled, -> { with_feature_enabled(:wiki) }
-
+      scope :within_shards, -> (shard_names) { where(repository_storage: Array(shard_names)) }
       scope :verification_failed_repos, -> { joins(:repository_state).merge(ProjectRepositoryState.verification_failed_repos) }
       scope :verification_failed_wikis, -> { joins(:repository_state).merge(ProjectRepositoryState.verification_failed_wikis) }
       scope :for_plan_name, -> (name) { joins(namespace: :plan).where(plans: { name: name }) }

ee/spec/finders/geo/legacy_project_registry_pending_verification_finder_spec.rb

@@ -8,7 +8,6 @@ describe Geo::LegacyProjectRegistryPendingVerificationFinder, :geo do
   describe '#execute' do
     let(:node) { create(:geo_node) }
 
-    shared_examples 'finds registries to verify' do
     subject { described_class.new(current_node: node, shard_name: 'default', batch_size: 100) }
 
     it 'does not return registries that are verified on primary and secondary' do
@@ -182,23 +181,4 @@ describe Geo::LegacyProjectRegistryPendingVerificationFinder, :geo do
       end
     end
   end
-
-    # Disable transactions via :delete method because a foreign table
-    # can't see changes inside a transaction of a different connection.
-    context 'FDW', :delete do
-      before do
-        skip('FDW is not configured') unless Gitlab::Geo::Fdw.enabled?
-      end
-
-      include_examples 'finds registries to verify'
-    end
-
-    context 'Legacy' do
-      before do
-        stub_fdw_disabled
-      end
-
-      include_examples 'finds registries to verify'
-    end
-  end
 end

ee/spec/finders/geo/project_registry_finder_spec.rb

 require 'spec_helper'
 
 describe Geo::ProjectRegistryFinder, :geo do
+  using RSpec::Parameterized::TableSyntax
+
   include ::EE::GeoHelpers
 
   # Using let() instead of set() because set() does not work properly
@@ -466,16 +468,6 @@ describe Geo::ProjectRegistryFinder, :geo do
 
       include_examples 'counts all the things', 'fdw'
       include_examples 'finds all the things', 'fdw'
-
-      describe '#find_registries_to_verify' do
-        it 'delegates to Geo::LegacyProjectRegistryPendingVerificationFinder' do
-          expect_next_instance_of(Geo::LegacyProjectRegistryPendingVerificationFinder, current_node: secondary, shard_name: 'default', batch_size: 100) do |finder|
-            expect(finder).to receive(:execute).once
-          end
-
-          subject.find_registries_to_verify(shard_name: 'default', batch_size: 100)
-        end
-      end
     end
 
     context 'with use_fdw_queries_for_selective_sync enabled' do
@@ -485,16 +477,6 @@ describe Geo::ProjectRegistryFinder, :geo do
 
       include_examples 'counts all the things', 'fdw'
       include_examples 'finds all the things', 'fdw'
-
-      describe '#find_registries_to_verify' do
-        it 'delegates to Geo::ProjectRegistryPendingVerificationFinder' do
-          expect_next_instance_of(Geo::ProjectRegistryPendingVerificationFinder, current_node: secondary, shard_name: 'default', batch_size: 100) do |finder|
-            expect(finder).to receive(:execute).once
-          end
-
-          subject.find_registries_to_verify(shard_name: 'default', batch_size: 100)
-        end
-      end
     end
   end
 
@@ -505,10 +487,29 @@ describe Geo::ProjectRegistryFinder, :geo do
 
     include_examples 'counts all the things', 'legacy'
     include_examples 'finds all the things', 'legacy'
+  end
 
-    describe '#find_registries_to_verify' do
-      it 'delegates to Geo::LegacyProjectRegistryPendingVerificationFinder' do
-        expect_next_instance_of(Geo::LegacyProjectRegistryPendingVerificationFinder, current_node: secondary, shard_name: 'default', batch_size: 100) do |finder|
+  describe '#find_registries_to_verify', :delete do
+    where(:selective_sync, :fdw_enabled, :use_fdw_queries_for_selective_sync, :finder) do
+      false | false | false | Geo::LegacyProjectRegistryPendingVerificationFinder
+      false | false | true  | Geo::LegacyProjectRegistryPendingVerificationFinder
+      false | true  | true  | Geo::ProjectRegistryPendingVerificationFinder
+      false | true  | false | Geo::ProjectRegistryPendingVerificationFinder
+      true  | false | false | Geo::LegacyProjectRegistryPendingVerificationFinder
+      true  | false | true  | Geo::LegacyProjectRegistryPendingVerificationFinder
+      true  | true  | true  | Geo::ProjectRegistryPendingVerificationFinder
+      true  | true  | false | Geo::LegacyProjectRegistryPendingVerificationFinder
+    end
+
+    with_them do
+      before do
+        stub_fdw(fdw_enabled)
+        stub_feature_flags(use_fdw_queries_for_selective_sync: use_fdw_queries_for_selective_sync)
+        stub_selective_sync(secondary, selective_sync)
+      end
+
+      it 'delegates to Geo::ProjectRegistryPendingVerificationFinder' do
+        expect_next_instance_of(finder, current_node: secondary, shard_name: 'default', batch_size: 100) do |finder|
           expect(finder).to receive(:execute).once
         end
 

ee/spec/support/helpers/ee/geo_helpers.rb

@@ -14,8 +14,16 @@ module EE
       allow(::Gitlab::Geo).to receive(:secondary?).and_return(true)
     end
 
+    def stub_fdw(value)
+      allow(::Gitlab::Geo::Fdw).to receive(:enabled?).and_return(value)
+    end
+
     def stub_fdw_disabled
-      allow(::Gitlab::Geo::Fdw).to receive(:enabled?).and_return(false)
+      stub_fdw(false)
+    end
+
+    def stub_selective_sync(node, value)
+      allow(node).to receive(:selective_sync?).and_return(value)
     end
   end
 end