Merge branch...

Merge branch '322786-denormalize-project-permissions-to-merge-request-in-elasticsearch' into 'master'

Denormalize project permissions to merge request in Elasticsearch

See merge request gitlab-org/gitlab!59731

ee/app/models/ee/project.rb

@@ -109,6 +109,7 @@ module EE
       has_one :security_orchestration_policy_configuration, class_name: 'Security::OrchestrationPolicyConfiguration', foreign_key: :project_id, inverse_of: :project
 
       elastic_index_dependant_association :issues, on_change: :visibility_level
+      elastic_index_dependant_association :merge_requests, on_change: :visibility_level
       elastic_index_dependant_association :notes, on_change: :visibility_level
 
       scope :with_shared_runners_limit_enabled, -> do

ee/app/models/ee/project_feature.rb

@@ -23,6 +23,7 @@ module EE
 
           associations_to_update = []
           associations_to_update << 'issues' if elasticsearch_project_issues_need_updating?
+          associations_to_update << 'merge_requests' if elasticsearch_project_merge_requests_need_updating?
           associations_to_update << 'notes' if elasticsearch_project_notes_need_updating?
 
           ElasticAssociationIndexerWorker.perform_async(self.project.class.name, project_id, associations_to_update) if associations_to_update.any?
@@ -40,6 +41,10 @@ module EE
       def elasticsearch_project_issues_need_updating?
         self.previous_changes.key?(:issues_access_level)
       end
+
+      def elasticsearch_project_merge_requests_need_updating?
+        self.previous_changes.key?(:merge_requests_access_level)
+      end
     end
   end
 end

ee/app/workers/elastic_delete_project_worker.rb

@@ -59,7 +59,7 @@ class ElasticDeleteProjectWorker
               },
               {
                 term: {
-                  target_project_id: project_id # handle merge_request which aliases project_id to target_project_id
+                  target_project_id: project_id # handle merge_request which previously did not store project_id and only stored target_project_id
                 }
               }
             ]

ee/changelogs/unreleased/322786-denormalize-project-permissions-to-merge-request-in-elasticsearch.yml

+---
+title: Denormalize project permissions to merge request in Elasticsearch
+merge_request: 59731
+author:
+type: changed

ee/lib/elastic/latest/merge_request_class_proxy.rb

@@ -26,6 +26,12 @@ module Elastic
 
         search(query_hash, options)
       end
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      def preload_indexing_data(relation)
+        relation.includes(target_project: [:project_feature])
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
     end
   end
 end

ee/lib/elastic/latest/merge_request_instance_proxy.rb

@@ -21,11 +21,15 @@ module Elastic
           :merge_status,
           :source_project_id,
           :target_project_id,
+          :project_id, # Redundant field aliased to target_project_id makes it easier to share searching code
           :author_id
         ].each do |attr|
           data[attr.to_s] = safely_read_attribute_for_elasticsearch(attr)
         end
 
+        data['visibility_level'] = target.project.visibility_level
+        data['merge_requests_access_level'] = safely_read_project_feature_for_elasticsearch(:merge_requests)
+
         data.merge(generic_attributes)
       end
     end

ee/spec/models/concerns/elastic/merge_request_spec.rb

@@ -71,6 +71,7 @@ RSpec.describe MergeRequest, :elastic do
 
   it "returns json with all needed elements" do
     merge_request = create :merge_request
+    merge_request.project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
 
     expected_hash = merge_request.attributes.extract!(
       'id',
@@ -92,12 +93,23 @@ RSpec.describe MergeRequest, :elastic do
       'join_field' => {
         'name' => merge_request.es_type,
         'parent' => merge_request.es_parent
-      }
+      },
+      'merge_requests_access_level' => ProjectFeature::ENABLED,
+      'visibility_level' => Gitlab::VisibilityLevel::INTERNAL,
+      'project_id' => merge_request.target_project.id
     })
 
     expect(merge_request.__elasticsearch__.as_indexed_json).to eq(expected_hash)
   end
 
+  it 'handles when a project is missing project_feature' do
+    merge_request = create :merge_request
+    allow(merge_request.project).to receive(:project_feature).and_return(nil)
+
+    expect { merge_request.__elasticsearch__.as_indexed_json }.not_to raise_error
+    expect(merge_request.__elasticsearch__.as_indexed_json['merge_requests_access_level']).to eq(ProjectFeature::PRIVATE)
+  end
+
   it_behaves_like 'no results when the user cannot read cross project' do
     let(:record1) { create(:merge_request, source_project: project, title: 'test-mr') }
     let(:record2) { create(:merge_request, source_project: project2, title: 'test-mr') }

ee/spec/models/project_feature_spec.rb

@@ -32,7 +32,7 @@ RSpec.describe ProjectFeature do
       'issues'                      | true        | %w[issues notes]
       'wiki'                        | false       | nil
       'builds'                      | false       | nil
-      'merge_requests'              | true        | %w[notes]
+      'merge_requests'              | true        | %w[merge_requests notes]
       'repository'                  | true        | %w[notes]
       'snippets'                    | true        | %w[notes]
       'operations'                  | false       | nil

ee/spec/models/project_spec.rb

@@ -2863,10 +2863,11 @@ RSpec.describe Project do
     context 'on update' do
       let(:project) { create(:project, :public) }
       let!(:issue) { create(:issue, project: project) }
+      let!(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
 
       context 'when updating the visibility_level' do
-        it 'triggers ElasticAssociationIndexerWorker to update issues and notes' do
-          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, %w[issues notes])
+        it 'triggers ElasticAssociationIndexerWorker to update issues, merge_requests and notes' do
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, %w[issues merge_requests notes])
 
           project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
         end
@@ -2882,6 +2883,18 @@ RSpec.describe Project do
           results = Issue.elastic_search('*', options: { public_and_internal_projects: true })
           expect(results.count).to eq(0)
         end
+
+        it 'ensures all visibility_level updates are correctly applied in merge_request searches', :sidekiq_inline do
+          ensure_elasticsearch_index!
+          results = MergeRequest.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 = MergeRequest.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/process_bookkeeping_service_spec.rb

@@ -108,10 +108,12 @@ RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_st
     it 'calls track! for each associated object' do
       issue_1 = create(:issue, project: project)
       issue_2 = create(:issue, project: project)
+      merge_request1 = create(:merge_request, source_project: project, target_project: project)
 
-      expect(described_class).to receive(:track!).with(issue_1, issue_2)
+      expect(described_class).to receive(:track!).with(issue_1, issue_2).ordered
+      expect(described_class).to receive(:track!).with(merge_request1).ordered
 
-      described_class.maintain_indexed_associations(project, ['issues'])
+      described_class.maintain_indexed_associations(project, %w[issues merge_requests])
     end
 
     it 'correctly scopes associated note objects to not include system notes' do
@@ -264,6 +266,20 @@ RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_st
 
         expect { described_class.new.execute }.not_to exceed_all_query_limit(control)
       end
+
+      it 'does not have N+1 queries for merge_requests' do
+        merge_requests = create_list(:merge_request, 2)
+
+        described_class.track!(*merge_requests)
+
+        control = ActiveRecord::QueryRecorder.new(skip_cached: false) { described_class.new.execute }
+
+        merge_requests += create_list(:merge_request, 3)
+
+        described_class.track!(*merge_requests)
+
+        expect { described_class.new.execute }.not_to exceed_all_query_limit(control)
+      end
     end
 
     def expect_processing(*refs, failures: [])

ee/spec/services/groups/transfer_service_spec.rb

@@ -60,18 +60,18 @@ RSpec.describe Groups::TransferService, '#execute' do
       context 'when visibility changes' do
         let(:new_group) { create(:group, :private) }
 
-        it 'does not invalidate the cache and reindexes projects and associated issues' do
+        it 'does not invalidate the cache and reindexes projects and associated issues, merge_requests and notes' do
           project1 = create(:project, :repository, :public, namespace: group)
           project2 = create(:project, :repository, :public, namespace: group)
           project3 = create(:project, :repository, :private, namespace: group)
 
           expect(::Gitlab::CurrentSettings).not_to receive(:invalidate_elasticsearch_indexes_cache_for_project!)
           expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project1)
-          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project1.id, %w[issues notes])
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project1.id, %w[issues merge_requests notes])
           expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project2)
-          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project2.id, %w[issues notes])
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project2.id, %w[issues merge_requests notes])
           expect(Elastic::ProcessBookkeepingService).not_to receive(:track!).with(project3)
-          expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async).with('Project', project3.id, %w[issues notes])
+          expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async).with('Project', project3.id, %w[issues merge_requests notes])
 
           transfer_service.execute(new_group)
 

ee/spec/services/projects/transfer_service_spec.rb

@@ -74,7 +74,7 @@ RSpec.describe Projects::TransferService do
 
       it 'reindexes the project and associated issues and notes' do
         expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project)
-        expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, %w[issues notes])
+        expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, %w[issues merge_requests notes])
 
         subject.execute(group)
       end