Merge branch 'improve-query-analyzers' into 'master'

Improve cross-DB discovery for edge conditions

See merge request gitlab-org/gitlab!76862

lib/gitlab/database/query_analyzer.rb

@@ -58,10 +58,10 @@ module Gitlab
         return unless parsed
 
         analyzers.each do |analyzer|
-          next if analyzer.suppressed?
+          next if analyzer.suppressed? && !analyzer.requires_tracking?(parsed)
 
           analyzer.analyze(parsed)
-        rescue StandardError => e
+        rescue StandardError, QueryAnalyzers::Base::QueryAnalyzerError => e
           # We catch all standard errors to prevent validation errors to introduce fatal errors in production
           Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e)
         end
@@ -75,7 +75,7 @@ module Gitlab
 
             true
           end
-        rescue StandardError => e
+        rescue StandardError, QueryAnalyzers::Base::QueryAnalyzerError => e
           Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e)
 
           false
@@ -88,7 +88,7 @@ module Gitlab
       def end!
         enabled_analyzers.select do |analyzer|
           analyzer.end!
-        rescue StandardError => e
+        rescue StandardError, QueryAnalyzers::Base::QueryAnalyzerError => e
           Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e)
         end
 

lib/gitlab/database/query_analyzers/base.rb

@@ -4,10 +4,17 @@ module Gitlab
   module Database
     module QueryAnalyzers
       class Base
+        # `Exception` to ensure that is not easily rescued when running in test env
+        QueryAnalyzerError = Class.new(Exception) # rubocop:disable Lint/InheritException
+
         def self.suppressed?
           Thread.current[self.suppress_key]
         end
 
+        def self.requires_tracking?(parsed)
+          false
+        end
+
         def self.suppress=(value)
           Thread.current[self.suppress_key] = value
         end

lib/gitlab/database/query_analyzers/prevent_cross_database_modification.rb

@@ -4,7 +4,7 @@ module Gitlab
   module Database
     module QueryAnalyzers
       class PreventCrossDatabaseModification < Database::QueryAnalyzers::Base
-        CrossDatabaseModificationAcrossUnsupportedTablesError = Class.new(StandardError)
+        CrossDatabaseModificationAcrossUnsupportedTablesError = Class.new(QueryAnalyzerError)
 
         # This method will allow cross database modifications within the block
         # Example:
@@ -36,10 +36,13 @@ module Gitlab
             Feature.enabled?(:detect_cross_database_modification, default_enabled: :yaml)
         end
 
+        def self.requires_tracking?(parsed)
+          # The transaction boundaries always needs to be tracked regardless of suppress behavior
+          self.transaction_begin?(parsed) || self.transaction_end?(parsed)
+        end
+
         # rubocop:disable Metrics/AbcSize
         def self.analyze(parsed)
-          return if in_factory_bot_create?
-
           database = ::Gitlab::Database.db_config_name(parsed.connection)
           sql = parsed.sql
 
@@ -51,14 +54,18 @@ module Gitlab
             return
           elsif self.transaction_end?(parsed)
             context[:transaction_depth_by_db][database] -= 1
-            if context[:transaction_depth_by_db][database] <= 0
+            if context[:transaction_depth_by_db][database] == 0
               context[:modified_tables_by_db][database].clear
+            elsif context[:transaction_depth_by_db][database] < 0
+              context[:transaction_depth_by_db][database] = 0
+              raise CrossDatabaseModificationAcrossUnsupportedTablesError, "Misaligned cross-DB transactions discovered at query #{sql}. This could be a bug in #{self.class} or a valid issue to investigate. Read more at https://docs.gitlab.com/ee/development/database/multiple_databases.html#removing-cross-database-transactions ."
             end
 
             return
           end
 
-          return if context[:transaction_depth_by_db].values.all?(&:zero?)
+          return unless self.in_transaction?
+          return if in_factory_bot_create?
 
           # PgQuery might fail in some cases due to limited nesting:
           # https://github.com/pganalyze/pg_query/issues/209
@@ -141,13 +148,21 @@ module Gitlab
           Rails.env.test?
         end
 
+        def self.in_transaction?
+          context[:transaction_depth_by_db].values.any?(&:positive?)
+        end
+
         # We ignore execution in the #create method from FactoryBot
         # because it is not representative of real code we run in
         # production. There are far too many false positives caused
         # by instantiating objects in different `gitlab_schema` in a
         # FactoryBot `create`.
         def self.in_factory_bot_create?
-          Rails.env.test? && caller_locations.any? { |l| l.path.end_with?('lib/factory_bot/evaluation.rb') && l.label == 'create' }
+          Rails.env.test? && caller_locations.any? do |l|
+            l.path.end_with?('lib/factory_bot/evaluation.rb') && l.label == 'create' ||
+            l.path.end_with?('lib/factory_bot/strategy/create.rb') ||
+            l.path.end_with?('shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb') && l.label == 'create_existing_record'
+          end
         end
       end
     end

spec/lib/gitlab/database/query_analyzer_spec.rb

@@ -128,11 +128,20 @@ RSpec.describe Gitlab::Database::QueryAnalyzer, query_analyzers: false do
 
     it 'does not call analyze on suppressed analyzers' do
       expect(analyzer).to receive(:suppressed?).and_return(true)
+      expect(analyzer).to receive(:requires_tracking?).and_return(false)
       expect(analyzer).not_to receive(:analyze)
 
       expect { process_sql("SELECT 1 FROM projects") }.not_to raise_error
     end
 
+    it 'does call analyze on suppressed analyzers if some queries require tracking' do
+      expect(analyzer).to receive(:suppressed?).and_return(true)
+      expect(analyzer).to receive(:requires_tracking?).and_return(true)
+      expect(analyzer).to receive(:analyze)
+
+      expect { process_sql("SELECT 1 FROM projects") }.not_to raise_error
+    end
+
     def process_sql(sql)
       described_class.instance.within do
         ApplicationRecord.load_balancer.read_write do |connection|

spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb

@@ -181,4 +181,49 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModificatio
       end.to raise_error /Cross-database data modification.*The gitlab_schema was undefined/
     end
   end
+
+  context 'when execution is rescued with StandardError' do
+    it 'raises cross-database data modification exception' do
+      expect do
+        Project.transaction do
+          project.touch
+          project.connection.execute('UPDATE foo_bars_undefined_table SET a=1 WHERE id = -1')
+        end
+      rescue StandardError
+        # Ensures that standard rescue does not silence errors
+      end.to raise_error /Cross-database data modification.*The gitlab_schema was undefined/
+    end
+  end
+
+  context 'when uniquiness validation is tested', type: :model do
+    subject { build(:ci_variable) }
+
+    it 'does not raise exceptions' do
+      expect do
+        is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope).with_message(/\(\w+\) has already been taken/)
+      end.not_to raise_error
+    end
+  end
+
+  context 'when doing rollback in a suppressed block' do
+    it 'does not raise misaligned transactions exception' do
+      expect do
+        # This is non-materialised transaction:
+        # 1. the transaction will be open on a write (project.touch) (in a suppressed block)
+        # 2. the rescue will be handled outside of suppressed block
+        #
+        # This will create misaligned boundaries since BEGIN
+        # of transaction will be executed within a suppressed block
+        Project.transaction do
+          described_class.with_suppressed do
+            project.touch
+
+            raise 'force rollback'
+          end
+
+          # the ensure of `.transaction` executes `ROLLBACK TO SAVEPOINT`
+        end
+      end.to raise_error /force rollback/
+    end
+  end
 end

spec/support/database/cross-database-modification-allowlist.yml

-- "./ee/spec/controllers/projects/settings/access_tokens_controller_spec.rb"
-- "./ee/spec/lib/gitlab/ci/templates/Jobs/dast_default_branch_gitlab_ci_yaml_spec.rb"
 - "./ee/spec/mailers/notify_spec.rb"
-- "./ee/spec/models/ci/bridge_spec.rb"
-- "./ee/spec/models/ci/build_spec.rb"
-- "./ee/spec/models/ee/ci/job_artifact_spec.rb"
 - "./ee/spec/models/group_member_spec.rb"
-- "./ee/spec/replicators/geo/pipeline_artifact_replicator_spec.rb"
 - "./ee/spec/replicators/geo/terraform_state_version_replicator_spec.rb"
-- "./ee/spec/services/ci/destroy_pipeline_service_spec.rb"
 - "./ee/spec/services/ci/retry_build_service_spec.rb"
-- "./ee/spec/services/ci/subscribe_bridge_service_spec.rb"
-- "./ee/spec/services/deployments/auto_rollback_service_spec.rb"
-- "./ee/spec/services/ee/ci/job_artifacts/destroy_all_expired_service_spec.rb"
-- "./ee/spec/services/ee/users/destroy_service_spec.rb"
-- "./ee/spec/services/security/security_orchestration_policies/rule_schedule_service_spec.rb"
 - "./spec/controllers/abuse_reports_controller_spec.rb"
-- "./spec/controllers/admin/spam_logs_controller_spec.rb"
-- "./spec/controllers/admin/users_controller_spec.rb"
 - "./spec/controllers/omniauth_callbacks_controller_spec.rb"
 - "./spec/controllers/projects/issues_controller_spec.rb"
-- "./spec/controllers/projects/settings/access_tokens_controller_spec.rb"
 - "./spec/features/issues/issue_detail_spec.rb"
 - "./spec/features/projects/pipelines/pipeline_spec.rb"
 - "./spec/features/signed_commits_spec.rb"
 - "./spec/helpers/issuables_helper_spec.rb"
 - "./spec/lib/gitlab/auth_spec.rb"
 - "./spec/lib/gitlab/ci/pipeline/chain/create_spec.rb"
-- "./spec/lib/gitlab/ci/pipeline/chain/seed_block_spec.rb"
-- "./spec/lib/gitlab/ci/pipeline/seed/build_spec.rb"
-- "./spec/lib/gitlab/ci/tags/bulk_insert_spec.rb"
-- "./spec/lib/gitlab/ci/templates/5_minute_production_app_ci_yaml_spec.rb"
-- "./spec/lib/gitlab/ci/templates/AWS/deploy_ecs_gitlab_ci_yaml_spec.rb"
-- "./spec/lib/gitlab/ci/templates/Jobs/deploy_gitlab_ci_yaml_spec.rb"
-- "./spec/lib/gitlab/ci/templates/auto_devops_gitlab_ci_yaml_spec.rb"
-- "./spec/lib/gitlab/ci/templates/managed_cluster_applications_gitlab_ci_yaml_spec.rb"
 - "./spec/lib/gitlab/email/handler/create_issue_handler_spec.rb"
 - "./spec/lib/gitlab/email/handler/create_merge_request_handler_spec.rb"
 - "./spec/lib/gitlab/email/handler/create_note_handler_spec.rb"
 - "./spec/lib/gitlab/email/handler/create_note_on_issuable_handler_spec.rb"
-- "./spec/lib/peek/views/active_record_spec.rb"
-- "./spec/models/ci/build_need_spec.rb"
 - "./spec/models/ci/build_trace_chunk_spec.rb"
-- "./spec/models/ci/group_variable_spec.rb"
 - "./spec/models/ci/job_artifact_spec.rb"
-- "./spec/models/ci/job_variable_spec.rb"
 - "./spec/models/ci/runner_spec.rb"
-- "./spec/models/ci/variable_spec.rb"
 - "./spec/models/clusters/applications/runner_spec.rb"
-- "./spec/models/commit_status_spec.rb"
-- "./spec/models/concerns/batch_destroy_dependent_associations_spec.rb"
-- "./spec/models/concerns/bulk_insertable_associations_spec.rb"
-- "./spec/models/concerns/has_environment_scope_spec.rb"
-- "./spec/models/concerns/token_authenticatable_spec.rb"
 - "./spec/models/design_management/version_spec.rb"
 - "./spec/models/hooks/system_hook_spec.rb"
 - "./spec/models/members/project_member_spec.rb"
-- "./spec/models/spam_log_spec.rb"
 - "./spec/models/user_spec.rb"
 - "./spec/models/user_status_spec.rb"
-- "./spec/requests/api/ci/pipeline_schedules_spec.rb"
-- "./spec/requests/api/ci/pipelines_spec.rb"
 - "./spec/requests/api/commits_spec.rb"
-- "./spec/requests/api/graphql/mutations/ci/pipeline_destroy_spec.rb"
-- "./spec/requests/api/resource_access_tokens_spec.rb"
-- "./spec/requests/api/users_spec.rb"
-- "./spec/services/ci/create_pipeline_service/environment_spec.rb"
-- "./spec/services/ci/create_pipeline_service_spec.rb"
-- "./spec/services/ci/destroy_pipeline_service_spec.rb"
-- "./spec/services/ci/ensure_stage_service_spec.rb"
-- "./spec/services/ci/expire_pipeline_cache_service_spec.rb"
-- "./spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb"
-- "./spec/services/ci/job_artifacts/destroy_associations_service_spec.rb"
-- "./spec/services/ci/pipeline_bridge_status_service_spec.rb"
-- "./spec/services/ci/pipelines/add_job_service_spec.rb"
 - "./spec/services/ci/retry_build_service_spec.rb"
-- "./spec/services/projects/destroy_service_spec.rb"
 - "./spec/services/projects/overwrite_project_service_spec.rb"
-- "./spec/services/resource_access_tokens/revoke_service_spec.rb"
-- "./spec/services/users/destroy_service_spec.rb"
-- "./spec/services/users/reject_service_spec.rb"
 - "./spec/workers/merge_requests/create_pipeline_worker_spec.rb"
-- "./spec/workers/remove_expired_members_worker_spec.rb"
 - "./spec/workers/repository_cleanup_worker_spec.rb"