Merge branch '334291-rubocop-rule-for-referencing-active-record-base-connection' into 'master'

Rubocop rule to prevent AR::Base.connection [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!64937

.rubocop.yml

@@ -280,6 +280,22 @@ GitlabSecurity/PublicSend:
     - 'ee/lib/**/*.rake'
     - 'ee/spec/**/*'
 
+Database/MultipleDatabases:
+  Enabled: true
+  Include:
+    - 'app/**/*.rb'
+    - 'ee/app/**/*.rb'
+    - 'lib/**/*.rb'
+    - 'ee/lib/**/*.rb'
+    - 'spec/**/*.rb'
+    - 'ee/spec/**/*.rb'
+  Exclude:
+    - 'ee/db/**/*.rb'
+    - 'spec/migrations/**/*.rb'
+    - 'lib/gitlab/background_migration/**/*.rb'
+    - 'spec/lib/gitlab/background_migration/**/*.rb'
+    - 'spec/lib/gitlab/database/**/*.rb'
+
 Gitlab/DuplicateSpecLocation:
   Enabled: true
 

.rubocop_manual_todo.yml

@@ -2465,3 +2465,170 @@ Style/RegexpLiteralMixedPreserve:
     - 'lib/gitlab/regex.rb'
     - 'lib/gitlab/utils.rb'
     - 'lib/product_analytics/tracker.rb'
+    - 'qa/qa/page/project/settings/advanced.rb'
+    - 'qa/spec/service/docker_run/gitlab_runner_spec.rb'
+    - 'rubocop/cop/gitlab/duplicate_spec_location.rb'
+    - 'spec/features/clusters/cluster_health_dashboard_spec.rb'
+    - 'spec/features/markdown/metrics_spec.rb'
+    - 'spec/features/search/user_searches_for_code_spec.rb'
+    - 'spec/features/snippets/embedded_snippet_spec.rb'
+    - 'spec/helpers/diff_helper_spec.rb'
+    - 'spec/helpers/releases_helper_spec.rb'
+    - 'spec/lib/gitlab/ci/reports/test_case_spec.rb'
+    - 'spec/lib/gitlab/consul/internal_spec.rb'
+    - 'spec/lib/gitlab/import_export/shared_spec.rb'
+    - 'spec/lib/gitlab/utils/usage_data_spec.rb'
+    - 'spec/presenters/ci/build_runner_presenter_spec.rb'
+    - 'spec/requests/api/projects_spec.rb'
+    - 'spec/services/jira/requests/projects/list_service_spec.rb'
+    - 'spec/support/capybara.rb'
+    - 'spec/support/helpers/grafana_api_helpers.rb'
+    - 'spec/support/helpers/query_recorder.rb'
+    - 'spec/support/helpers/require_migration.rb'
+    - 'spec/views/layouts/_head.html.haml_spec.rb'
+
+Database/MultipleDatabases:
+  Exclude:
+    - 'app/mailers/previews/notify_preview.rb'
+    - 'app/models/application_setting.rb'
+    - 'app/models/internal_id.rb'
+    - 'app/services/auto_merge/base_service.rb'
+    - 'app/services/ci/delete_unit_tests_service.rb'
+    - 'app/services/ci/unlock_artifacts_service.rb'
+    - 'app/services/deployments/update_environment_service.rb'
+    - 'app/services/design_management/copy_design_collection/copy_service.rb'
+    - 'app/services/feature_flags/create_service.rb'
+    - 'app/services/feature_flags/destroy_service.rb'
+    - 'app/services/feature_flags/update_service.rb'
+    - 'app/services/issuable/clone/base_service.rb'
+    - 'app/services/issuable/common_system_notes_service.rb'
+    - 'app/services/issuable/destroy_label_links_service.rb'
+    - 'app/services/packages/create_dependency_service.rb'
+    - 'app/services/packages/go/create_package_service.rb'
+    - 'app/services/packages/npm/create_package_service.rb'
+    - 'app/services/packages/terraform_module/create_package_service.rb'
+    - 'app/services/projects/cleanup_service.rb'
+    - 'app/services/projects/fetch_statistics_increment_service.rb'
+    - 'app/services/releases/update_service.rb'
+    - 'app/services/todos/destroy/destroyed_issuable_service.rb'
+    - 'ee/app/models/dora/daily_metrics.rb'
+    - 'ee/app/services/analytics/devops_adoption/enabled_namespaces/bulk_delete_service.rb'
+    - 'ee/app/services/approval_rules/finalize_service.rb'
+    - 'ee/app/services/approval_rules/project_rule_destroy_service.rb'
+    - 'ee/app/services/app_sec/dast/site_profiles/create_service.rb'
+    - 'ee/app/services/app_sec/dast/site_profiles/update_service.rb'
+    - 'ee/app/services/ci/minutes/update_build_minutes_service.rb'
+    - 'ee/app/services/ee/issuable/common_system_notes_service.rb'
+    - 'ee/app/services/group_saml/group_managed_accounts/transfer_membership_service.rb'
+    - 'ee/app/services/group_saml/sign_up_service.rb'
+    - 'ee/app/services/iterations/roll_over_issues_service.rb'
+    - 'ee/app/services/security/store_scan_service.rb'
+    - 'ee/app/services/timebox_report_service.rb'
+    - 'ee/app/services/vulnerability_feedback/create_service.rb'
+    - 'ee/lib/ee/gitlab/checks/push_rule_check.rb'
+    - 'ee/lib/ee/gitlab/database.rb'
+    - 'ee/lib/gitlab/geo/database_tasks.rb'
+    - 'ee/lib/gitlab/geo/geo_tasks.rb'
+    - 'ee/lib/gitlab/geo/health_check.rb'
+    - 'ee/lib/gitlab/geo/log_cursor/daemon.rb'
+    - 'ee/lib/pseudonymizer/dumper.rb'
+    - 'ee/lib/pseudonymizer/pager.rb'
+    - 'ee/lib/system_check/geo/geo_database_configured_check.rb'
+    - 'ee/spec/lib/pseudonymizer/dumper_spec.rb'
+    - 'ee/spec/models/pg_replication_slot_spec.rb'
+    - 'ee/spec/services/ee/merge_requests/update_service_spec.rb'
+    - 'lib/backup/database.rb'
+    - 'lib/after_commit_queue.rb'
+    - 'lib/api/rubygem_packages.rb'
+    - 'lib/backup/manager.rb'
+    - 'lib/gitlab/analytics/cycle_analytics/stage_query_helpers.rb'
+    - 'lib/gitlab/chaos.rb'
+    - 'lib/gitlab/current_settings.rb'
+    - 'lib/gitlab/database/batch_count.rb'
+    - 'lib/gitlab/database/batch_counter.rb'
+    - 'lib/gitlab/database/count/reltuples_count_strategy.rb'
+    - 'lib/gitlab/database/count/tablesample_count_strategy.rb'
+    - 'lib/gitlab/database/grant.rb'
+    - 'lib/gitlab/database/load_balancing/load_balancer.rb'
+    - 'lib/gitlab/database/load_balancing.rb'
+    - 'lib/gitlab/database/load_balancing/sticking.rb'
+    - 'lib/gitlab/database/migrations/observers/migration_observer.rb'
+    - 'lib/gitlab/database/migrations/observers/query_log.rb'
+    - 'lib/gitlab/database/multi_threaded_migration.rb'
+    - 'lib/gitlab/database/partitioning_migration_helpers/backfill_partitioned_table.rb'
+    - 'lib/gitlab/database/partitioning/monthly_strategy.rb'
+    - 'lib/gitlab/database/partitioning/partition_manager.rb'
+    - 'lib/gitlab/database/partitioning/partition_creator.rb'
+    - 'lib/gitlab/database/partitioning/replace_table.rb'
+    - 'lib/gitlab/database/partitioning/time_partition.rb'
+    - 'lib/gitlab/database/postgres_hll/batch_distinct_counter.rb'
+    - 'lib/gitlab/database/postgresql_adapter/dump_schema_versions_mixin.rb'
+    - 'lib/gitlab/database/postgresql_database_tasks/load_schema_versions_mixin.rb'
+    - 'lib/gitlab/database.rb'
+    - 'lib/gitlab/database/reindexing/concurrent_reindex.rb'
+    - 'lib/gitlab/database/reindexing/reindex_concurrently.rb'
+    - 'lib/gitlab/database/schema_cache_with_renamed_table.rb'
+    - 'lib/gitlab/database/schema_migrations/context.rb'
+    - 'lib/gitlab/database/schema_version_files.rb'
+    - 'lib/gitlab/database/similarity_score.rb'
+    - 'lib/gitlab/database/unidirectional_copy_trigger.rb'
+    - 'lib/gitlab/database/with_lock_retries.rb'
+    - 'lib/gitlab/gitlab_import/importer.rb'
+    - 'lib/gitlab/health_checks/db_check.rb'
+    - 'lib/gitlab/import_export/base/relation_factory.rb'
+    - 'lib/gitlab/import_export/relation_tree_restorer.rb'
+    - 'lib/gitlab/legacy_github_import/importer.rb'
+    - 'lib/gitlab/metrics/samplers/database_sampler.rb'
+    - 'lib/gitlab/optimistic_locking.rb'
+    - 'lib/gitlab/otp_key_rotator.rb'
+    - 'lib/gitlab/profiler.rb'
+    - 'lib/gitlab/seeder.rb'
+    - 'lib/gitlab/sherlock/query.rb'
+    - 'lib/gitlab/sql/glob.rb'
+    - 'lib/gitlab/sql/set_operator.rb'
+    - 'lib/system_check/orphans/repository_check.rb'
+    - 'spec/db/schema_spec.rb'
+    - 'spec/features/admin/dashboard_spec.rb'
+    - 'spec/initializers/database_config_spec.rb'
+    - 'spec/initializers/lograge_spec.rb'
+    - 'spec/lib/backup/manager_spec.rb'
+    - 'spec/lib/gitlab/current_settings_spec.rb'
+    - 'spec/lib/gitlab/database_spec.rb'
+    - 'spec/lib/gitlab/import_export/fast_hash_serializer_spec.rb'
+    - 'spec/lib/gitlab/import_export/project/tree_saver_spec.rb'
+    - 'spec/lib/gitlab/metrics/subscribers/active_record_spec.rb'
+    - 'spec/lib/gitlab/pagination/keyset/order_spec.rb'
+    - 'spec/lib/gitlab/profiler_spec.rb'
+    - 'spec/lib/gitlab/query_limiting/active_support_subscriber_spec.rb'
+    - 'spec/lib/gitlab/sidekiq_logging/structured_logger_spec.rb'
+    - 'spec/lib/gitlab/sql/cte_spec.rb'
+    - 'spec/lib/gitlab/sql/glob_spec.rb'
+    - 'spec/lib/gitlab/sql/recursive_cte_spec.rb'
+    - 'spec/lib/gitlab/usage_data_metrics_spec.rb'
+    - 'spec/lib/gitlab/usage_data_queries_spec.rb'
+    - 'spec/lib/gitlab/usage/metrics/names_suggestions/relation_parsers/constraints_spec.rb'
+    - 'spec/lib/gitlab/usage/metrics/names_suggestions/relation_parsers/joins_spec.rb'
+    - 'spec/lib/gitlab/usage/metrics/instrumentations/database_metric_spec.rb'
+    - 'spec/lib/gitlab/utils/usage_data_spec.rb'
+    - 'spec/models/application_setting_spec.rb'
+    - 'spec/models/concerns/case_sensitivity_spec.rb'
+    - 'spec/models/concerns/sortable_spec.rb'
+    - 'spec/models/concerns/where_composite_spec.rb'
+    - 'spec/models/experiment_spec.rb'
+    - 'spec/models/internal_id_spec.rb'
+    - 'spec/models/project_feature_usage_spec.rb'
+    - 'spec/models/users_statistics_spec.rb'
+    - 'spec/requests/api/statistics_spec.rb'
+    - 'spec/services/users/activity_service_spec.rb'
+    - 'spec/support/caching.rb'
+    - 'spec/support/gitlab/usage/metrics_instrumentation_shared_examples.rb'
+    - 'spec/support/helpers/database_connection_helpers.rb'
+    - 'spec/support/helpers/database/database_helpers.rb'
+    - 'spec/support/helpers/database/table_schema_helpers.rb'
+    - 'spec/support/helpers/migrations_helpers.rb'
+    - 'spec/support/helpers/query_recorder.rb'
+    - 'spec/support/helpers/usage_data_helpers.rb'
+    - 'spec/tasks/gitlab/backup_rake_spec.rb'
+    - 'spec/tasks/gitlab/db_rake_spec.rb'
+    - 'spec/workers/analytics/usage_trends/counter_job_worker_spec.rb'
+    - 'spec/workers/users/create_statistics_worker_spec.rb'

doc/development/database/transaction_guidelines.md

+---
+stage: Enablement
+group: Database
+info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments
+---
+
+# Transaction guidelines
+
+This document gives a few examples of the usage of database transactions in application code.
+
+For further reference please check PostgreSQL documentation about [transactions](https://www.postgresql.org/docs/current/tutorial-transactions.html).
+
+## Database decomposition and sharding
+
+The [sharding group](https://about.gitlab.com/handbook/engineering/development/enablement/sharding) plans to split the main GitLab database and move some of the database tables to other database servers.
+
+The group will start decomposing the `ci_*` related database tables first. To maintain the current application development experience, tooling and static analyzers will be added to the codebase to ensure correct data access and data modification methods. By using the correct form for defining database transactions, we can save significant refactoring work in the future.
+
+## The transaction block
+
+The `ActiveRecord` library provides a convenient way to group database statements into a transaction.
+
+```ruby
+issue = Issue.find(10)
+project = issue.project
+
+ApplicationRecord.transaction do
+  issue.update!(title: 'updated title')
+  project.update!(last_update_at: Time.now)
+end
+```
+
+This transaction involves two database tables, in case of an error, each `UPDATE` statement will be rolled back to the previous, consistent state.
+
+NOTE:
+Avoid referencing the `ActiveRecord::Base` class and use `ApplicationRecord` instead.
+
+## Transaction and database locks
+
+When a transaction block is opened, the database will try to acquire the necessary locks on the resources. The type of locks will depend on the actual database statements.
+
+Consider a concurrent update scenario where the following code is executed at the same time from two different processes:
+
+```ruby
+issue = Issue.find(10)
+project = issue.project
+
+ApplicationRecord.transaction do
+  issue.update!(title: 'updated title')
+  project.update!(last_update_at: Time.now)
+end
+```
+
+The database will try to acquire the `FOR UPDATE` lock for the referenced `issue` and `project` records. In our case, we have two competing transactions for these locks, one of them will successfully acquire them. The other transaction will have to wait in the lock queue until the first transaction finishes. The execution of the second transaction is blocked at this point.
+
+## Transaction speed
+
+To prevent lock contention and maintain stable application performance, the transaction block should finish as fast as possible. When a transaction acquires locks, it will hold on to them until the transaction finishes.
+
+Apart from application performance, long-running transactions can also affect the application upgrade processes by blocking database migrations.
+
+### Dangerous example: 3rd party API calls
+
+Consider the following example:
+
+```ruby
+member = Member.find(5)
+
+Member.transaction do
+  member.update!(notification_email_sent: true)
+
+  member.send_notification_email
+end
+```
+
+Here, we ensure that the `notification_email_sent` column is updated only when the `send_notification_email` method succeeds. The `send_notification_email` method executes a network request to an email sending service. If the underlying infrastructure does not specify timeouts or the network call takes too long time, the database transaction will stay open.
+
+Ideally, a transaction should only contain database statements.
+
+Avoid doing in a `transaction` block:
+
+- External network requests such as: triggering Sidekiq jobs, sending emails, HTTP API calls and running database statements using a different connection.
+- File system operations.
+- Long, CPU intensive computation.
+- Calling `sleep(n)`.
+
+## Explicit model referencing
+
+If a transaction modifies records from the same database table, it's advised to use the `Model.transaction` block:
+
+```ruby
+build_1 = Ci::Build.find(1)
+build_2 = Ci::Build.find(2)
+
+Ci::Build.transaction do
+  build_1.touch
+  build_2.touch
+end
+```
+
+The transaction above will use the same database connection for the transaction as the models in the `transaction` block. In a multi-database environment the following example would be dangerous:
+
+```ruby
+# `ci_builds` table is located on another database
+class Ci::Build < CiDatabase
+end
+
+build_1 = Ci::Build.find(1)
+build_2 = Ci::Build.find(2)
+
+ActiveRecord::Base.transaction do
+  build_1.touch
+  build_2.touch
+end
+```
+
+The `ActiveRecord::Base` class uses a different database connection than the `Ci::Build` records. The two statements in the transaction block will not be part of the transaction and will not be rolled back in case something goes wrong. They act as 3rd part calls.

rubocop/cop/database/multiple_databases.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Database
+      # @example
+      #   # bad
+      #   ActiveRecord::Base.connection
+      #
+      #   # good
+      #   ApplicationRecord.connection
+      #
+      class MultipleDatabases < RuboCop::Cop::Cop
+        AR_BASE_MESSAGE = <<~EOF
+          Do not use methods from ActiveRecord::Base, use the ApplicationRecord class instead
+          For fixing offenses related to the ActiveRecord::Base.transaction method, see our guidelines:
+          https://docs.gitlab.com/ee/development/database/transaction_guidelines.html
+        EOF
+
+        def_node_matcher :active_record_base_method_is_used?, <<~PATTERN
+        (send (const (const nil? :ActiveRecord) :Base) $_)
+        PATTERN
+
+        def on_send(node)
+          return unless active_record_base_method_is_used?(node)
+
+          add_offense(node, location: :expression, message: AR_BASE_MESSAGE)
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/database/multiple_databases_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require_relative '../../../../rubocop/cop/database/multiple_databases'
+
+RSpec.describe RuboCop::Cop::Database::MultipleDatabases do
+  subject(:cop) { described_class.new }
+
+  it 'flags the use of ActiveRecord::Base.connection' do
+    expect_offense(<<~SOURCE)
+    ActiveRecord::Base.connection.inspect
+    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use methods from ActiveRecord::Base, [...]
+    SOURCE
+  end
+end