Merge branch 'add-prevent-cross-joins-spec-helper-and-fix-with-project-and-metadata' into 'master'

Allow prevent_cross_joins to be used directly in tests

See merge request gitlab-org/gitlab!67655

app/models/ci/build.rb

@@ -151,7 +151,7 @@ module Ci
 
     scope :with_project_and_metadata, -> do
       if Feature.enabled?(:non_public_artifacts, type: :development)
-        joins(:metadata).includes(:project, :metadata)
+        joins(:metadata).includes(:metadata).preload(:project)
       end
     end
 

doc/development/database/multiple_databases.md

@@ -113,7 +113,7 @@ patterns may apply to future cases.
 
 The simplest solution we've seen several times now has been an existing scope
 that is unused. This is the easiest example to fix. So the first step is to
-investigate if the code is unused and then simply remove it. These are some
+investigate if the code is unused and then remove it. These are some
 real examples:
 
 - <https://gitlab.com/gitlab-org/gitlab/-/merge_requests/67162>
@@ -131,6 +131,20 @@ to evaluate, because `UsageData` is not critical to users and it may be possible
 to get a similarly useful metric with a simpler approach. Alternatively we may
 find that nobody is using these metrics, so we can remove them.
 
+#### Use `preload` instead of `includes`
+
+The `includes` and `preload` methods in Rails are both ways to avoid an N+1
+query. The `includes` method in Rails uses a heuristic approach to determine
+if it needs to join to the table, or if it can load all of the
+records in a separate query. This method assumes it needs to join if it thinks
+you need to query the columns from the other table, but sometimes
+this method gets it wrong and executes a join even when not needed. In
+this case using `preload` to explicitly load the data in a separate query
+allows you to avoid the join, while still avoiding the N+1 query.
+
+You can see a real example of this solution being used in
+<https://gitlab.com/gitlab-org/gitlab/-/merge_requests/67655>.
+
 #### De-normalize some foreign key to the table
 
 De-normalization refers to adding redundant precomputed (duplicated) data to
@@ -243,3 +257,37 @@ A quick checklist for fixing a specific join query would be:
    adding a new column
 1. Can we remove the join by adding a new table in the correct database that
    replicates the minimum data needed to do the join
+
+#### How to validate you have correctly removed a cross-join
+
+Using RSpec tests, you can validate all SQL queries within a code block to
+ensure that none of them are joining across the two databases. This is a useful
+tool to confirm you have correctly fixed an existing cross-join.
+
+At some point in the future we will have fixed all cross-joins and this tool
+will run by default in all tests. For now, the tool needs to be explicitly enabled
+for your test.
+
+You can use this method like so:
+
+```ruby
+it 'does not join across databases' do
+  with_cross_joins_prevented do
+    ::Ci::Build.joins(:project).to_a
+  end
+end
+```
+
+This will raise an exception if the query joins across the two databases. The
+previous example is fixed by removing the join, like so:
+
+```ruby
+it 'does not join across databases' do
+  with_cross_joins_prevented do
+    ::Ci::Build.preload(:project).to_a
+  end
+end
+```
+
+You can see a real example of using this method for fixing a cross-join in
+<https://gitlab.com/gitlab-org/gitlab/-/merge_requests/67655>.

spec/models/ci/build_spec.rb

@@ -5188,6 +5188,14 @@ RSpec.describe Ci::Build do
     end
   end
 
+  describe '.with_project_and_metadata' do
+    it 'does not join across databases' do
+      with_cross_joins_prevented do
+        ::Ci::Build.with_project_and_metadata.to_a
+      end
+    end
+  end
+
   describe '.without_coverage' do
     let!(:build_with_coverage) { create(:ci_build, pipeline: pipeline, coverage: 100.0) }
 

spec/support/database/prevent_cross_joins.rb

@@ -56,6 +56,20 @@ module Database
       end
     end
 
+    module SpecHelpers
+      def with_cross_joins_prevented
+        subscriber = ActiveSupport::Notifications.subscribe('sql.active_record') do |event|
+          ::Database::PreventCrossJoins.validate_cross_joins!(event.payload[:sql])
+        end
+
+        Thread.current[:allow_cross_joins_across_databases] = false
+
+        yield
+      ensure
+        ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber
+      end
+    end
+
     module GitlabDatabaseMixin
       def allow_cross_joins_across_databases(url:)
         Thread.current[:allow_cross_joins_across_databases] = true
@@ -69,16 +83,10 @@ Gitlab::Database.singleton_class.prepend(
   Database::PreventCrossJoins::GitlabDatabaseMixin)
 
 RSpec.configure do |config|
+  config.include(::Database::PreventCrossJoins::SpecHelpers)
+
   # TODO: remove `:prevent_cross_joins` to enable the check by default
   config.around(:each, :prevent_cross_joins) do |example|
-    subscriber = ActiveSupport::Notifications.subscribe('sql.active_record') do |event|
-      ::Database::PreventCrossJoins.validate_cross_joins!(event.payload[:sql])
-    end
-
-    Thread.current[:allow_cross_joins_across_databases] = false
-
-    example.run
-  ensure
-    ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber
+    with_cross_joins_prevented { example.run }
   end
 end