Denormalize project permissions to merge request in Elasticsearch

As part of our efforts to avoid using joins in Elasticsearch queries
https://gitlab.com/groups/gitlab-org/-/epics/2054 we need to
"denormalize" (copy them into child docs) the permission related fields
needed for searching.

In order to allow us to search for merge requests without joining to the
project we need to store the `merge_requests_access_level` as well as
the `visibility_level` of the project on the merge request record.

This MR also introduces an extra field `project_id` for the merge
request in Elasticsearch which is redundant since we already have
`target_project_id` and `project_id` is just aliased to this value but
adding it to Elasticsearch will make the query logic simpler to share
across all document types. Previously they were all able to consistently
join to a parent "project" so it helps when changing the code they all
have a field called `project_id`.

As well as saving these new fields with the merge requests we need to
also update these fields when they change which we also do in this MR.
We need to track updates in a few places:

1. When a `ProjectFeature` record is changed (this is where
`merge_requests_access_level` lives)
2. When a `Project` is updated (this is where `visibility_level` lives)
3. When a project is moved to another group. This logic was already
implemented generically to delegate to `Project` but we update the spec
for this just to be safe.

The change to index these new fields also introduced an N+1 query which
required an update to `MergeRequestClassProxy` to preload these new fields
we're now setting in Elasticsearch.

Changelog: changed

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