In specs mark all migrations as run from the start

This mirrors what happens with any new index. All migrations should be
marked as having finished as soon as the new index is created.

ee/changelogs/unreleased/273262-dont-use-joins-in-issue-global-search-new.yml

+---
+title: Avoid the use of Elasticsearch joins when searching for issues
+merge_request: 48583
+author:
+type: performance

ee/lib/elastic/latest/application_class_proxy.rb

@@ -118,11 +118,20 @@ module Elastic
             options[:current_user],
             options[:project_ids],
             options[:public_and_internal_projects],
-            options[:features]
+            options[:features],
+            options[:no_join_project]
           )
 
           query_hash[:query][:bool][:filter] ||= []
-          query_hash[:query][:bool][:filter] << {
+
+          query_hash[:query][:bool][:filter] << if options[:no_join_project]
+                                                  # Some models have denormalized project permissions into the
+                                                  # document so that we do not need to use joins
+                                                  {
+                                                    bool: project_query
+                                                  }
+                                                else
+                                                  {
                                                     has_parent: {
                                                       _name: context.name,
                                                       parent_type: "project",
@@ -132,6 +141,7 @@ module Elastic
                                                     }
                                                   }
                                                 end
+        end
 
         query_hash
       end
@@ -163,14 +173,14 @@ module Elastic
       # If a project feature(s) is specified, it indicates interest in child
       # documents gated by that project feature - e.g., "issues". The feature's
       # visibility level must be taken into account.
-      def project_ids_query(user, project_ids, public_and_internal_projects, features = nil)
+      def project_ids_query(user, project_ids, public_and_internal_projects, features = nil, no_join_project = false)
         scoped_project_ids = scoped_project_ids(user, project_ids)
 
         # At least one condition must be present, so pick no projects for
         # anonymous users.
         # Pick private, internal and public projects the user is a member of.
         # Pick all private projects for admins & auditors.
-        conditions = pick_projects_by_membership(scoped_project_ids, user, features)
+        conditions = pick_projects_by_membership(scoped_project_ids, user, no_join_project, features)
 
         if public_and_internal_projects
           context.name(:visibility) do
@@ -198,12 +208,17 @@ module Elastic
       # Admins & auditors are given access to all private projects. Access to
       # internal or public projects where the project feature is private is not
       # granted here.
-      def pick_projects_by_membership(project_ids, user, features = nil)
+      def pick_projects_by_membership(project_ids, user, no_join_project, features = nil)
+        # This method is used to construct a query on the join as well as query
+        # on top level doc. When querying top level doc the project's ID is
+        # `project_id` . When joining it is just `id`.
+        id_field = no_join_project ? :project_id : :id
+
         if features.nil?
           if project_ids == :any
             return [{ term: { visibility_level: { _name: context.name(:any), value: Project::PRIVATE } } }]
           else
-            return [{ terms: { _name: context.name(:membership, :id), id: project_ids } }]
+            return [{ terms: { _name: context.name(:membership, :id), id_field => project_ids } }]
           end
         end
 
@@ -212,7 +227,7 @@ module Elastic
             if project_ids == :any
               { term: { visibility_level: { _name: context.name(:any), value: Project::PRIVATE } } }
             else
-              { terms: { _name: context.name(:membership, :id), id: filter_ids_by_feature(project_ids, user, feature) } }
+              { terms: { _name: context.name(:membership, :id), id_field => filter_ids_by_feature(project_ids, user, feature) } }
             end
 
           limit = {

ee/lib/elastic/latest/issue_class_proxy.rb

@@ -21,6 +21,7 @@ module Elastic
           end
 
         options[:features] = 'issues'
+        options[:no_join_project] = Elastic::DataMigrationService.migration_has_finished?(:add_new_data_to_issues_documents)
         context.name(:issue) do
           query_hash = context.name(:authorized) { project_ids_filter(query_hash, options) }
           query_hash = context.name(:confidentiality) { confidentiality_filter(query_hash, options) }

ee/lib/elastic/latest/note_class_proxy.rb

@@ -150,8 +150,11 @@ module Elastic
       # Query notes based on the various feature permission of the noteable_type.
       # Appends `noteable_type` (which will be removed in project_ids_filter)
       # for base model filtering.
+      # We do not implement `no_join_project` argument for notes class yet
+      # as this is not supported. This will need to be fixed when we move
+      # notes to a new index.
       override :pick_projects_by_membership
-      def pick_projects_by_membership(project_ids, user, _ = nil)
+      def pick_projects_by_membership(project_ids, user, _, _ = nil)
         noteable_type_to_feature.map do |noteable_type, feature|
           context.name(feature) do
             condition =

ee/spec/models/elastic/migration_record_spec.rb

@@ -52,6 +52,8 @@ RSpec.describe Elastic::MigrationRecord, :elastic do
     let(:in_progress_migration) { described_class.new(version: 10, name: 10, filename: nil) }
 
     before do
+      es_helper.delete_index(index_name: es_helper.migrations_index_name)
+      es_helper.create_migrations_index
       completed_versions.each { |migration| migration.save!(completed: true) }
       in_progress_migration.save!(completed: false)
 

ee/spec/models/project_spec.rb

@@ -2646,6 +2646,7 @@ RSpec.describe Project do
 
     context 'on update' do
       let(:project) { create(:project, :public) }
+      let!(:issue) { create(:issue, project: project) }
 
       context 'when updating the visibility_level' do
         it 'triggers ElasticAssociationIndexerWorker to update issues' do
@@ -2653,6 +2654,18 @@ RSpec.describe Project do
 
           project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
         end
+
+        it 'ensures all visibility_level updates are correctly applied in issue searches', :sidekiq_inline do
+          ensure_elasticsearch_index!
+          results = Issue.elastic_search('*', options: { public_and_internal_projects: true })
+          expect(results.count).to eq(1)
+
+          project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+          ensure_elasticsearch_index!
+
+          results = Issue.elastic_search('*', options: { public_and_internal_projects: true })
+          expect(results.count).to eq(0)
+        end
       end
 
       context 'when changing the title' do

ee/spec/services/elastic/data_migration_service_spec.rb

@@ -34,7 +34,7 @@ RSpec.describe Elastic::DataMigrationService, :elastic do
     let(:migration_name) { migration.name.underscore }
 
     it 'returns true if migration has finished' do
-      expect(subject.migration_has_finished_uncached?(migration_name)).to eq(false)
+      expect(subject.migration_has_finished_uncached?(migration_name)).to eq(true)
 
       migration.save!(completed: false)
       refresh_index!
@@ -51,7 +51,7 @@ RSpec.describe Elastic::DataMigrationService, :elastic do
   describe '.migration_has_finished?' do
     let(:migration) { subject.migrations.first }
     let(:migration_name) { migration.name.underscore }
-    let(:finished) { false }
+    let(:finished) { true }
 
     before do
       allow(Rails).to receive(:cache).and_return(ActiveSupport::Cache::MemoryStore.new)
@@ -67,6 +67,13 @@ RSpec.describe Elastic::DataMigrationService, :elastic do
   end
 
   describe '.mark_all_as_completed!' do
+    before do
+      # Clear out the migrations index since it is setup initially with
+      # everything finished migrating
+      es_helper.delete_index(index_name: es_helper.migrations_index_name)
+      es_helper.create_migrations_index
+    end
+
     it 'creates all migration versions' do
       expect(Elastic::MigrationRecord.persisted_versions(completed: true).count).to eq(0)
 

ee/spec/services/search/global_service_spec.rb

@@ -108,6 +108,35 @@ RSpec.describe Search::GlobalService do
         end
       end
 
+      # Since newly created indices automatically have all migrations as
+      # finished we need a test to verify the old style searches work for
+      # instances which haven't finished the migration yet
+      context 'when add_new_data_to_issues_documents migration is not finished' do
+        let!(:issue) { create :issue, project: project }
+
+        before do
+          allow(Elastic::DataMigrationService).to receive(:migration_has_finished?)
+            .with(:add_new_data_to_issues_documents)
+            .and_return(false)
+        end
+
+        where(:project_level, :feature_access_level, :membership, :admin_mode, :expected_count) do
+          permission_table_for_guest_feature_access
+        end
+
+        with_them do
+          it "respects visibility" do
+            enable_admin_mode!(user) if admin_mode
+            update_feature_access_level(project, feature_access_level)
+            ensure_elasticsearch_index!
+
+            expect_search_results(user, 'issues', expected_count: expected_count) do |user|
+              described_class.new(user, search: issue.title).execute
+            end
+          end
+        end
+      end
+
       context 'sort by created_at' do
         let!(:project) { create(:project, :public) }
         let!(:old_result) { create(:issue, project: project, title: 'sorted old', created_at: 1.month.ago) }
@@ -122,6 +151,52 @@ RSpec.describe Search::GlobalService do
           let(:results) { described_class.new(nil, search: 'sorted', sort: sort).execute }
         end
       end
+
+      context 'using joins for global permission checks' do
+        let(:results) { described_class.new(nil, search: '*').execute.objects('issues') }
+        let(:es_host) { Gitlab::CurrentSettings.elasticsearch_url[0] }
+        let(:search_url) { Addressable::Template.new("#{es_host}/{index}/doc/_search{?params*}") }
+
+        before do
+          ensure_elasticsearch_index!
+        end
+
+        context 'when add_new_data_to_issues_documents migration is finished' do
+          before do
+            allow(Elastic::DataMigrationService).to receive(:migration_has_finished?)
+              .with(:add_new_data_to_issues_documents)
+              .and_return(true)
+          end
+
+          it 'does not use joins to apply permissions' do
+            request = a_request(:get, search_url).with do |req|
+              expect(req.body).not_to include("has_parent")
+            end
+
+            results
+
+            expect(request).to have_been_made
+          end
+        end
+
+        context 'when add_new_data_to_issues_documents migration is not finished' do
+          before do
+            allow(Elastic::DataMigrationService).to receive(:migration_has_finished?)
+              .with(:add_new_data_to_issues_documents)
+              .and_return(false)
+          end
+
+          it 'uses joins to apply permissions' do
+            request = a_request(:get, search_url).with do |req|
+              expect(req.body).to include("has_parent")
+            end
+
+            results
+
+            expect(request).to have_been_made
+          end
+        end
+      end
     end
 
     context 'merge_request' do

ee/spec/support/elastic.rb

@@ -12,6 +12,8 @@ RSpec.configure do |config|
 
     helper.create_empty_index(options: { settings: { number_of_replicas: 0 } })
     helper.create_migrations_index
+    ::Elastic::DataMigrationService.mark_all_as_completed!
+    refresh_index!
 
     example.run
 

ee/spec/support/helpers/elasticsearch_helpers.rb

@@ -39,6 +39,12 @@ module ElasticsearchHelpers
     es_helper.refresh_index(index_name: es_helper.migrations_index_name)
   end
 
+  def warm_elasticsearch_migrations_cache!
+    ::Elastic::DataMigrationService.migrations.each do |migration|
+      ::Elastic::DataMigrationService.migration_has_finished?(migration.name.underscore.to_sym)
+    end
+  end
+
   def es_helper
     Gitlab::Elastic::Helper.default
   end

ee/spec/support/shared_examples/lib/gitlab/elastic/search_results_shared_examples.rb

@@ -4,6 +4,10 @@ RSpec.shared_examples 'does not hit Elasticsearch twice for objects and counts'
   scopes.each do |scope|
     context "for scope #{scope}", :elastic, :request_store do
       it 'makes 1 Elasticsearch query' do
+        # We want to warm the cache for checking migrations have run since we
+        # don't want to count these requests as searches
+        allow(Rails).to receive(:cache).and_return(ActiveSupport::Cache::MemoryStore.new)
+        warm_elasticsearch_migrations_cache!
         ::Gitlab::SafeRequestStore.clear!
 
         results.objects(scope)

ee/spec/tasks/gitlab/elastic_rake_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe 'gitlab:elastic namespace rake tasks', :elastic do
 
     before do
       es_helper.delete_index
+      es_helper.delete_index(index_name: es_helper.migrations_index_name)
     end
 
     it 'creates an index' do

ee/spec/workers/elastic_delete_project_worker_spec.rb

@@ -8,13 +8,18 @@ RSpec.describe ElasticDeleteProjectWorker, :elastic do
   # Create admin user and search globally to avoid dealing with permissions in
   # these tests
   let(:user) { create(:admin) }
-  let(:search_options) { { options: { current_user: user, project_ids: :any } } }
 
   before do
     stub_ee_application_setting(elasticsearch_indexing: true)
   end
 
-  it 'deletes a project with all nested objects', :aggregate_failures do
+  # Extracted to a method as the `#elastic_search` methods using it below will
+  # mutate the hash and mess up the following searches
+  def search_options
+    { options: { current_user: user, project_ids: :any } }
+  end
+
+  it 'deletes a project with all nested objects' do
     project = create :project, :repository
     issue = create :issue, project: project
     milestone = create :milestone, project: project