Refactor Gitlab::Geo::Fdw class

These changes rename the method names to something more meaningful.
It also changes the spec to test all the internal details without
stubbing the private methods.

ee/app/models/geo/fdw/ci/job_artifact.rb

@@ -8,7 +8,7 @@ module Geo
 
         STORE_COLUMN = :file_store
 
-        self.table_name = Gitlab::Geo::Fdw.table('ci_job_artifacts')
+        self.table_name = Gitlab::Geo::Fdw.foreign_table_name('ci_job_artifacts')
 
         scope :not_expired, -> { where('expire_at IS NULL OR expire_at > ?', Time.current) }
         scope :geo_syncable, -> { with_files_stored_locally.not_expired }

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

@@ -7,7 +7,7 @@ module Geo
 
       STORE_COLUMN = :file_store
 
-      self.table_name = Gitlab::Geo::Fdw.table('lfs_objects')
+      self.table_name = Gitlab::Geo::Fdw.foreign_table_name('lfs_objects')
 
       scope :geo_syncable, -> { with_files_stored_locally }
     end

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

@@ -5,7 +5,7 @@ module Geo
     class Project < ::Geo::BaseFdw
       include Gitlab::SQL::Pattern
 
-      self.table_name = Gitlab::Geo::Fdw.table('projects')
+      self.table_name = Gitlab::Geo::Fdw.foreign_table_name('projects')
 
       class << self
         # Searches for a list of projects based on the query given in `query`.

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

@@ -3,7 +3,7 @@
 module Geo
   module Fdw
     class ProjectFeature < ::Geo::BaseFdw
-      self.table_name = Gitlab::Geo::Fdw.table('project_features')
+      self.table_name = Gitlab::Geo::Fdw.foreign_table_name('project_features')
     end
   end
 end

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

@@ -3,7 +3,7 @@
 module Geo
   module Fdw
     class ProjectRepositoryState < ::Geo::BaseFdw
-      self.table_name = Gitlab::Geo::Fdw.table('project_repository_states')
+      self.table_name = Gitlab::Geo::Fdw.foreign_table_name('project_repository_states')
     end
   end
 end

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

@@ -7,7 +7,7 @@ module Geo
 
       STORE_COLUMN = :store
 
-      self.table_name = Gitlab::Geo::Fdw.table('uploads')
+      self.table_name = Gitlab::Geo::Fdw.foreign_table_name('uploads')
 
       scope :geo_syncable, -> { with_files_stored_locally }
     end

ee/lib/gitlab/geo/fdw.rb

@@ -3,17 +3,11 @@
 module Gitlab
   module Geo
     class Fdw
-      DEFAULT_SCHEMA = 'public'.freeze
-      FDW_SCHEMA = 'gitlab_secondary'.freeze
+      DEFAULT_SCHEMA = 'public'
+      FOREIGN_SERVER = 'gitlab_secondary'
+      FOREIGN_SCHEMA = 'gitlab_secondary'
 
       class << self
-        # Return full table name with FDW schema
-        #
-        # @param [String] table_name
-        def table(table_name)
-          FDW_SCHEMA + ".#{table_name}"
-        end
-
         # Return if FDW is enabled for this instance
         #
         # @return [Boolean] whether FDW is enabled
@@ -25,19 +19,26 @@ module Gitlab
           value.nil? ? true : value
         end
 
-        def fdw_up_to_date?
+        # Return full table name with foreign schema
+        #
+        # @param [String] table_name
+        def foreign_table_name(table_name)
+          FOREIGN_SCHEMA + ".#{table_name}"
+        end
+
+        def foreign_tables_up_to_date?
           has_foreign_schema? && foreign_schema_tables_match?
         end
 
         # Number of existing tables
         #
         # @return [Integer] number of tables
-        def count_tables
+        def foreign_schema_tables_count
           Gitlab::Geo.cache_value(:geo_fdw_count_tables) do
             sql = <<~SQL
               SELECT COUNT(*)
                 FROM information_schema.tables
-               WHERE table_schema = '#{FDW_SCHEMA}'
+               WHERE table_schema = '#{FOREIGN_SCHEMA}'
                  AND table_type = 'FOREIGN TABLE'
                  AND table_name NOT LIKE 'pg_%'
             SQL
@@ -46,58 +47,55 @@ module Gitlab
           end
         end
 
-        def count_tables_match?
-          gitlab_tables.count == count_tables
-        end
-
-        def gitlab_tables
-          ActiveRecord::Schema.tables.reject { |table| table.start_with?('pg_') }
+        def gitlab_schema_tables_count
+          ActiveRecord::Schema.tables.reject { |table| table.start_with?('pg_') }.count
         end
 
         private
 
         def fdw_capable?
-          has_foreign_schema? && connection_exist? && count_tables.positive?
+          has_foreign_server? && has_foreign_schema? && foreign_schema_tables_count.positive?
+        end
+
+        # Check if there is at least one foreign server configured
+        #
+        # @return [Boolean] whether any foreign server exists
+        def has_foreign_server?
+          ::Geo::TrackingBase.connection.execute(
+            "SELECT 1 FROM pg_foreign_server"
+          ).count.positive?
         end
 
         def has_foreign_schema?
-          Gitlab::Geo.cache_value(:geo_fdw_schema_exist) do
+          Gitlab::Geo.cache_value(:geo_FOREIGN_SCHEMA_exist) do
             sql = <<~SQL
               SELECT 1
                 FROM information_schema.schemata
-               WHERE schema_name='#{FDW_SCHEMA}'
+               WHERE schema_name='#{FOREIGN_SCHEMA}'
             SQL
 
             ::Geo::TrackingBase.connection.execute(sql).count.positive?
           end
         end
 
-        # Check if there is at least one FDW connection configured
-        #
-        # @return [Boolean] whether any FDW connection exists
-        def connection_exist?
-          ::Geo::TrackingBase.connection.execute(
-            "SELECT 1 FROM pg_foreign_server"
-          ).count.positive?
-        end
-
         # Check if foreign schema has exact the same tables and fields defined on secondary database
         #
         # @return [Boolean] whether schemas match and are not empty
         def foreign_schema_tables_match?
-          Gitlab::Geo.cache_value(:geo_fdw_schema_tables_match) do
-            schema = gitlab_schema
+          Gitlab::Geo.cache_value(:geo_foreign_schema_tables_match) do
+            gitlab_schema_tables = retrieve_gitlab_schema_tables.to_set
+            foreign_schema_tables = retrieve_foreign_schema_tables.to_set
 
-            schema.present? && (schema.to_set == fdw_schema.to_set)
+            gitlab_schema_tables.present? && (gitlab_schema_tables == foreign_schema_tables)
           end
         end
 
-        def gitlab_schema
-          retrieve_schema_tables(ActiveRecord::Base, ActiveRecord::Base.connection_config[:database], DEFAULT_SCHEMA).to_a
+        def retrieve_foreign_schema_tables
+          retrieve_schema_tables(::Geo::TrackingBase, Rails.configuration.geo_database['database'], FOREIGN_SCHEMA).to_a
         end
 
-        def fdw_schema
-          retrieve_schema_tables(::Geo::TrackingBase, Rails.configuration.geo_database['database'], FDW_SCHEMA).to_a
+        def retrieve_gitlab_schema_tables
+          retrieve_schema_tables(ActiveRecord::Base, ActiveRecord::Base.connection_config[:database], DEFAULT_SCHEMA).to_a
         end
 
         def retrieve_schema_tables(adapter, database, schema)
@@ -113,7 +111,6 @@ module Gitlab
           adapter.connection.select_all(sql)
         end
       end
-
     end
   end
 end

ee/lib/gitlab/geo/geo_tasks.rb

@@ -55,7 +55,7 @@ module Gitlab
         sql = <<~SQL
           SELECT count(1)
             FROM pg_foreign_server
-           WHERE srvname = '#{Gitlab::Geo::Fdw::FDW_SCHEMA}';
+           WHERE srvname = '#{Gitlab::Geo::Fdw::FOREIGN_SERVER}';
         SQL
 
         Gitlab::Geo::DatabaseTasks.with_geo_db do

ee/lib/gitlab/geo/health_check.rb

@@ -21,11 +21,14 @@ module Gitlab
 
         return 'The Geo database is not configured to use Foreign Data Wrapper.' unless Gitlab::Geo::Fdw.enabled?
 
-        unless Gitlab::Geo::Fdw.fdw_up_to_date?
+        unless Gitlab::Geo::Fdw.foreign_tables_up_to_date?
           output = "The Geo database has an outdated FDW remote schema."
 
-          unless Gitlab::Geo::Fdw.count_tables_match?
-            output = "#{output} It contains #{Gitlab::Geo::Fdw.count_tables} of #{Gitlab::Geo::Fdw.gitlab_tables.count} expected tables."
+          foreign_schema_tables_count = Gitlab::Geo::Fdw.foreign_schema_tables_count
+          gitlab_schema_tables_count = Gitlab::Geo::Fdw.gitlab_schema_tables_count
+
+          unless gitlab_schema_tables_count == foreign_schema_tables_count
+            output = "#{output} It contains #{foreign_schema_tables_count} of #{gitlab_schema_tables_count} expected tables."
           end
 
           return output

ee/lib/system_check/geo/fdw_schema_up_to_date_check.rb

@@ -25,7 +25,7 @@ module SystemCheck
       end
 
       def check?
-        Gitlab::Geo::Fdw.fdw_up_to_date?
+        Gitlab::Geo::Fdw.foreign_tables_up_to_date?
       end
 
       def show_error

ee/lib/tasks/geo.rake

@@ -58,7 +58,7 @@ namespace :geo do
     desc 'Refresh Foreign Tables definition in Geo Secondary node'
     task refresh_foreign_tables: [:environment] do
       if Gitlab::Geo::GeoTasks.foreign_server_configured?
-        print "\nRefreshing foreign tables for FDW: #{Gitlab::Geo::Fdw::FDW_SCHEMA} ... "
+        print "\nRefreshing foreign tables for FDW: #{Gitlab::Geo::Fdw::FOREIGN_SCHEMA} ... "
         Gitlab::Geo::GeoTasks.refresh_foreign_tables!
         puts 'Done!'
       else

ee/spec/lib/gitlab/geo/fdw_spec.rb

 require 'spec_helper'
 
 describe Gitlab::Geo::Fdw, :geo do
-  include ::EE::GeoHelpers
+  describe '.enabled?' do
+    it 'returns false when foreign server does not exist' do
+      drop_foreign_server
 
-  describe 'enabled?' do
-    it 'returns false when PostgreSQL FDW is not enabled' do
-      expect(described_class).to receive(:count_tables).and_return(0)
-      allow(Rails.configuration).to receive(:geo_database).and_return('fdw' => true)
+      expect(described_class.enabled?).to eq false
+    end
 
-      expect(described_class.enabled?).to be_falsey
+    it 'returns false when foreign server exists but foreign schema does not exist' do
+      drop_foreign_schema
+
+      expect(described_class.enabled?).to eq false
     end
 
-    context 'with fdw capable' do
-      before do
-        allow(described_class).to receive(:fdw_capable?).and_return(true)
-      end
+    it 'returns false when foreign server and schema exists but foreign tables are empty' do
+      drop_foreign_schema
+      create_foreign_schema
 
-      it 'returns true by default' do
-        allow(Rails.configuration).to receive(:geo_database).and_return('fdw' => nil)
+      expect(described_class.enabled?).to eq false
+    end
+
+    it 'returns false when fdw is disabled in `config/database_geo.yml`' do
+      allow(Rails.configuration).to receive(:geo_database).and_return('fdw' => false)
+
+      expect(described_class.enabled?).to be_falsey
+    end
 
-        expect(described_class.enabled?).to be_truthy
-      end
+    it 'returns true when fdw is set in `config/database_geo.yml`' do
+      allow(Rails.configuration).to receive(:geo_database).and_return('fdw' => true)
 
-      it 'returns false if disabled in `config/database_geo.yml`' do
-        allow(Rails.configuration).to receive(:geo_database).and_return('fdw' => false)
+      expect(described_class.enabled?).to be_truthy
+    end
 
-        expect(described_class.enabled?).to be_falsey
-      end
+    it 'returns true when fdw is nil in `config/database_geo.yml`' do
+      allow(Rails.configuration).to receive(:geo_database).and_return('fdw' => nil)
 
-      it 'returns true if configured in `config/database_geo.yml`' do
-        allow(Rails.configuration).to receive(:geo_database).and_return('fdw' => true)
+      expect(described_class.enabled?).to be_truthy
+    end
 
-        expect(described_class.enabled?).to be_truthy
-      end
+    it 'returns true with a functional fdw environment' do
+      expect(described_class.enabled?).to be_truthy
     end
   end
 
-  describe '.gitlab_tables' do
-    it 'excludes pg_ tables' do
-      ActiveRecord::Base.connection.create_table(:pg_gitlab_test)
+  describe '.foreign_tables_up_to_date?' do
+    it 'returns false when foreign schema does not exist' do
+      drop_foreign_schema
 
-      expect(described_class.gitlab_tables).not_to include('pg_gitlab_test')
+      expect(described_class.foreign_tables_up_to_date?).to eq false
+    end
 
-      ActiveRecord::Base.connection.drop_table(:pg_gitlab_test)
+    it 'returns false when foreign schema exists but tables in schema doesnt match' do
+      create_foreign_table(:gitlab_test)
+
+      expect(described_class.foreign_tables_up_to_date?).to eq false
+    end
+
+    it 'returns true when foreign schema exists and foreign schema has same tables as secondary database' do
+      expect(described_class.foreign_tables_up_to_date?).to eq true
     end
   end
 
-  describe 'fdw_up_to_date?' do
-    context 'with mocked FDW environment' do
-      it 'returns true when FDW is enabled and foreign schema has same tables as secondary database' do
-        expect(described_class).to receive(:has_foreign_schema?).and_return(true)
-        expect(described_class).to receive(:foreign_schema_tables_match?).and_return(true)
+  describe '.foreign_schema_tables_count' do
+    before do
+      drop_foreign_schema
+      create_foreign_schema
+    end
 
-        expect(described_class.fdw_up_to_date?).to be_truthy
-      end
+    it 'returns the number of tables in the foreign schema' do
+      create_foreign_table(:gitlab_test)
 
-      it 'returns false when FDW is enabled but tables in schema doesnt match' do
-        expect(described_class).to receive(:has_foreign_schema?).and_return(true)
-        expect(described_class).to receive(:foreign_schema_tables_match?).and_return(false)
+      expect(described_class.foreign_schema_tables_count).to eq(1)
+    end
 
-        expect(described_class.fdw_up_to_date?).to be_falsey
-      end
+    it 'excludes tables that start with `pg_`' do
+      create_foreign_table(:pg_gitlab_test)
 
-      it 'returns false when FDW is disabled' do
-        expect(described_class).to receive(:has_foreign_schema?).and_return(false)
+      expect(described_class.foreign_schema_tables_count).to eq(0)
+    end
+  end
 
-        expect(described_class.fdw_up_to_date?).to be_falsey
-      end
+  describe '.gitlab_schema_tables_count' do
+    it 'returns the same number of tables as defined in the database' do
+      expect(described_class.gitlab_schema_tables_count).to eq(ActiveRecord::Schema.tables.count)
     end
 
-    context 'with functional FDW environment' do
-      it 'returns true' do
-        expect(described_class.fdw_up_to_date?).to be_truthy
-      end
+    it 'excludes tables that start with `pg_`' do
+      ActiveRecord::Base.connection.create_table(:pg_gitlab_test)
+
+      expect(described_class.gitlab_schema_tables_count).to eq(ActiveRecord::Schema.tables.count - 1)
+
+      ActiveRecord::Base.connection.drop_table(:pg_gitlab_test)
     end
   end
 
-  describe 'count_tables' do
-    context 'with functional FDW environment' do
-      it 'returns same amount as defined in schema migration' do
-        # When testing it locally, you may need to refresh FDW with:
-        #
-        # rake geo:db:test:refresh_foreign_tables
-        expect(described_class.count_tables).to eq(ActiveRecord::Schema.tables.count)
-      end
-    end
+  def with_foreign_connection
+    Geo::TrackingBase.connection
+  end
+
+  def drop_foreign_server
+    with_foreign_connection.execute <<-SQL
+      DROP SERVER IF EXISTS #{described_class::FOREIGN_SERVER} CASCADE
+    SQL
+  end
+
+  def drop_foreign_schema
+    with_foreign_connection.execute <<-SQL
+      DROP SCHEMA IF EXISTS #{described_class::FOREIGN_SCHEMA} CASCADE
+    SQL
+  end
+
+  def create_foreign_schema
+    with_foreign_connection.execute <<-SQL
+      CREATE SCHEMA IF NOT EXISTS #{described_class::FOREIGN_SCHEMA}
+    SQL
+
+    with_foreign_connection.execute <<-SQL
+      GRANT USAGE ON FOREIGN SERVER #{described_class::FOREIGN_SERVER} TO current_user
+    SQL
+  end
+
+  def create_foreign_table(table_name)
+    with_foreign_connection.execute <<-SQL
+      CREATE FOREIGN TABLE IF NOT EXISTS #{described_class::FOREIGN_SCHEMA}.#{table_name} (
+        id int
+      ) SERVER #{described_class::FOREIGN_SERVER}
+    SQL
   end
 end

ee/spec/lib/gitlab/geo/health_check_spec.rb

@@ -96,8 +96,9 @@ describe Gitlab::Geo::HealthCheck, :geo do
       allow(described_class).to receive(:database_secondary?) { true }
       allow(described_class).to receive(:streaming_active?) { true }
 
-      allow(Gitlab::Geo::Fdw).to receive(:fdw_up_to_date?) { false }
-      allow(Gitlab::Geo::Fdw).to receive(:count_tables_match?) { true }
+      allow(Gitlab::Geo::Fdw).to receive(:foreign_tables_up_to_date?) { false }
+      allow(Gitlab::Geo::Fdw).to receive(:foreign_schema_tables_count) { 1 }
+      allow(Gitlab::Geo::Fdw).to receive(:gitlab_schema_tables_count) { 1 }
 
       expect(subject.perform_checks).to match(/The Geo database has an outdated FDW remote schema\./)
     end
@@ -106,8 +107,9 @@ describe Gitlab::Geo::HealthCheck, :geo do
       allow(described_class).to receive(:database_secondary?) { true }
       allow(described_class).to receive(:streaming_active?) { true }
 
-      allow(Gitlab::Geo::Fdw).to receive(:fdw_up_to_date?) { false }
-      allow(Gitlab::Geo::Fdw).to receive(:count_tables_match?) { false }
+      allow(Gitlab::Geo::Fdw).to receive(:foreign_tables_up_to_date?) { false }
+      allow(Gitlab::Geo::Fdw).to receive(:foreign_schema_tables_count) { 1 }
+      allow(Gitlab::Geo::Fdw).to receive(:gitlab_schema_tables_count) { 2 }
 
       expect(subject.perform_checks).to match(/The Geo database has an outdated FDW remote schema\. It contains [0-9]+ of [0-9]+ expected tables/)
     end