Automatic merge of gitlab-org/gitlab master

.rubocop_manual_todo.yml

@@ -620,15 +620,6 @@ RSpec/EmptyLineAfterFinalLetItBe:
     - ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb
     - ee/spec/services/alert_management/extract_alert_payload_fields_service_spec.rb
     - ee/spec/services/alert_management/process_prometheus_alert_service_spec.rb
-    - ee/spec/services/analytics/cycle_analytics/stages/delete_service_spec.rb
-    - ee/spec/services/analytics/cycle_analytics/stages/list_service_spec.rb
-    - ee/spec/services/analytics/cycle_analytics/stages/update_service_spec.rb
-    - ee/spec/services/analytics/cycle_analytics/value_streams/create_service_spec.rb
-    - ee/spec/services/analytics/deployments/frequency/aggregate_service_spec.rb
-    - ee/spec/services/analytics/devops_adoption/segments/bulk_delete_service_spec.rb
-    - ee/spec/services/analytics/devops_adoption/segments/create_service_spec.rb
-    - ee/spec/services/analytics/devops_adoption/segments/delete_service_spec.rb
-    - ee/spec/services/analytics/devops_adoption/segments/find_or_create_service_spec.rb
     - ee/spec/services/approval_rules/create_service_spec.rb
     - ee/spec/services/audit_event_service_spec.rb
     - ee/spec/services/auto_merge/add_to_merge_train_when_pipeline_succeeds_service_spec.rb

ee/changelogs/unreleased/324745-remove-more-n-plus-1-queries-for-indexing-notes-in-elasticsearc.yml

+---
+title: Fix more N+1 issues when indexing notes in Elasticsearch
+merge_request: 58751
+author:
+type: performance

ee/changelogs/unreleased/rspec-empty-lines-after-letitbe-ee-spec-analytics.yml

+---
+title: Fix RSpec/EmptyLineAfterFinalLetItBe rubocop offenses in ee/spec/services/analytics
+merge_request: 58359
+author: Abdul Wadood @abdulwd
+type: fixed

ee/lib/elastic/latest/note_class_proxy.rb

@@ -26,7 +26,7 @@ module Elastic
 
       # rubocop: disable CodeReuse/ActiveRecord
       def preload_indexing_data(relation)
-        relation.includes(noteable: :assignees)
+        relation.includes(noteable: :assignees, project: [:project_feature, :route])
       end
       # rubocop: enable CodeReuse/ActiveRecord
 

ee/spec/services/analytics/cycle_analytics/stages/delete_service_spec.rb

@@ -7,6 +7,7 @@ RSpec.describe Analytics::CycleAnalytics::Stages::DeleteService do
   let_it_be(:value_stream, refind: true) { create(:cycle_analytics_group_value_stream, group: group) }
   let_it_be(:user, refind: true) { create(:user) }
   let_it_be(:stage, refind: true) { create(:cycle_analytics_group_stage, group: group, value_stream: value_stream) }
+
   let(:params) { { id: stage.id } }
 
   subject { described_class.new(parent: group, params: params, current_user: user).execute }

ee/spec/services/analytics/cycle_analytics/stages/list_service_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe Analytics::CycleAnalytics::Stages::ListService do
   let_it_be(:group, refind: true) { create(:group) }
   let_it_be(:value_stream, refind: true) { create(:cycle_analytics_group_value_stream, group: group) }
   let_it_be(:user) { create(:user) }
+
   let(:stages) { subject.payload[:stages] }
 
   subject { described_class.new(parent: group, current_user: user).execute }

ee/spec/services/analytics/cycle_analytics/stages/update_service_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe Analytics::CycleAnalytics::Stages::UpdateService do
   let_it_be(:group, refind: true) { create(:group) }
   let_it_be(:value_stream, refind: true) { create(:cycle_analytics_group_value_stream, group: group) }
   let_it_be(:user, refind: true) { create(:user) }
+
   let(:default_stages) { Gitlab::Analytics::CycleAnalytics::DefaultStages.all }
   let(:params) { { value_stream: value_stream } }
   let(:persisted_stages) { group.reload.cycle_analytics_stages.ordered }
@@ -98,6 +99,7 @@ RSpec.describe Analytics::CycleAnalytics::Stages::UpdateService do
 
   context 'when updating a custom stage' do
     let_it_be(:stage) { create(:cycle_analytics_group_stage, group: group, value_stream: value_stream) }
+
     let(:params) { { id: stage.id, name: 'my new stage name', value_stream: value_stream } }
 
     it { expect(subject).to be_success }

ee/spec/services/analytics/cycle_analytics/value_streams/create_service_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 RSpec.describe Analytics::CycleAnalytics::ValueStreams::CreateService do
   let_it_be(:user) { create(:user) }
   let_it_be(:group, refind: true) { create(:group) }
+
   let(:params) { {} }
 
   subject { described_class.new(group: group, params: params, current_user: user).execute }

ee/spec/services/analytics/deployments/frequency/aggregate_service_spec.rb

@@ -9,6 +9,7 @@ RSpec.describe Analytics::Deployments::Frequency::AggregateService do
   let_it_be(:subgroup_project, refind: true) { create(:project, :repository, group: subgroup) }
   let_it_be(:developer) { create(:user) }
   let_it_be(:guest) { create(:user) }
+
   let(:container) { group_project }
   let(:actor) { developer }
   let(:service) { described_class.new(container: container, current_user: actor, params: params) }

ee/spec/services/analytics/devops_adoption/segments/bulk_delete_service_spec.rb

@@ -7,6 +7,7 @@ RSpec.describe Analytics::DevopsAdoption::Segments::BulkDeleteService do
 
   let_it_be(:group) { create(:group) }
   let_it_be(:admin) { create(:user, :admin) }
+
   let(:segment) { create(:devops_adoption_segment, namespace: group) }
   let(:segment2) { create(:devops_adoption_segment) }
   let(:current_user) { admin }

ee/spec/services/analytics/devops_adoption/segments/create_service_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 RSpec.describe Analytics::DevopsAdoption::Segments::CreateService do
   let_it_be(:group) { create(:group) }
   let_it_be(:reporter) { create(:user).tap { |u| group.add_reporter(u) } }
+
   let(:current_user) { reporter }
 
   let(:params) { { namespace: group } }

ee/spec/services/analytics/devops_adoption/segments/delete_service_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 RSpec.describe Analytics::DevopsAdoption::Segments::DeleteService do
   let_it_be(:group) { create(:group) }
   let_it_be(:reporter) { create(:user).tap { |u| group.add_reporter(u) } }
+
   let(:segment) { create(:devops_adoption_segment, namespace: group) }
   let(:current_user) { reporter }
 

ee/spec/services/analytics/devops_adoption/segments/find_or_create_service_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 RSpec.describe Analytics::DevopsAdoption::Segments::FindOrCreateService do
   let_it_be(:group) { create(:group) }
   let_it_be(:reporter) { create(:user).tap { |u| group.add_reporter(u) } }
+
   let(:current_user) { reporter }
 
   let(:params) { { namespace: group } }

ee/spec/services/elastic/process_bookkeeping_service_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_state do
+RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_state, :elastic do
   let(:ref_class) { ::Gitlab::Elastic::DocumentReference }
 
   let(:fake_refs) { Array.new(10) { |i| ref_class.new(Issue, i, "issue_#{i}", 'project_1') } }
@@ -125,17 +125,11 @@ RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_st
   end
 
   describe '#execute' do
-    let(:shard_limit) { 5 }
-    let(:shard_number) { 2 }
-    let(:limit) { shard_limit * shard_number }
-
-    before do
-      stub_const('Elastic::ProcessBookkeepingService::SHARD_LIMIT', shard_limit)
-      stub_const('Elastic::ProcessBookkeepingService::SHARDS_NUMBER', shard_number)
-    end
-
     context 'limit is less than refs count' do
-      let(:shard_limit) { 2 }
+      before do
+        stub_const('Elastic::ProcessBookkeepingService::SHARD_LIMIT', 2)
+        stub_const('Elastic::ProcessBookkeepingService::SHARDS_NUMBER', 2)
+      end
 
       it 'processes only up to limit' do
         described_class.track!(*fake_refs)
@@ -143,7 +137,7 @@ RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_st
         expect(described_class.queue_size).to eq(fake_refs.size)
         allow_processing(*fake_refs)
 
-        expect { described_class.new.execute }.to change(described_class, :queue_size).by(-limit)
+        expect { described_class.new.execute }.to change(described_class, :queue_size).by(-4)
       end
     end
 
@@ -151,17 +145,17 @@ RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_st
       described_class.track!(*fake_refs)
 
       expect(described_class.queue_size).to eq(fake_refs.size)
-      expect_processing(*fake_refs[0...limit])
+      expect_processing(*fake_refs)
 
-      expect { described_class.new.execute }.to change(described_class, :queue_size).by(-limit)
+      expect { described_class.new.execute }.to change(described_class, :queue_size).by(-fake_refs.count)
     end
 
     it 'returns the number of documents processed' do
       described_class.track!(*fake_refs)
 
-      expect_processing(*fake_refs[0...limit])
+      expect_processing(*fake_refs)
 
-      expect(described_class.new.execute).to eq(limit)
+      expect(described_class.new.execute).to eq(fake_refs.count)
     end
 
     it 'returns 0 without writing to the index when there are no documents' do
@@ -175,9 +169,9 @@ RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_st
       failed = fake_refs[0]
 
       expect(described_class.queue_size).to eq(10)
-      expect_processing(*fake_refs[0...limit], failures: [failed])
+      expect_processing(*fake_refs, failures: [failed])
 
-      expect { described_class.new.execute }.to change(described_class, :queue_size).by(-limit + 1)
+      expect { described_class.new.execute }.to change(described_class, :queue_size).by(-fake_refs.count + 1)
 
       shard = described_class.shard_number(failed.serialize)
       serialized = described_class.queued_items[shard].first[0]
@@ -224,29 +218,37 @@ RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_st
       end
 
       it 'does not have N+1 queries for notes' do
-        notes = []
-
-        2.times do
-          notes << create(:note)
-          notes << create(:discussion_note_on_merge_request)
-          notes << create(:note_on_merge_request)
-          notes << create(:note_on_commit)
+        # Gitaly N+1 calls when processing notes on commits
+        # https://gitlab.com/gitlab-org/gitlab/-/issues/327086 . Even though
+        # this block is in the spec there is still an N+1 to fix in the actual
+        # code.
+        Gitlab::GitalyClient.allow_n_plus_1_calls do
+          notes = []
+
+          2.times do
+            notes << create(:note)
+            notes << create(:discussion_note_on_merge_request)
+            notes << create(:note_on_merge_request)
+            notes << create(:note_on_commit)
+            notes << create(:diff_note_on_merge_request)
+          end
+
+          described_class.track!(*notes)
+
+          control = ActiveRecord::QueryRecorder.new(skip_cached: false) { described_class.new.execute }
+
+          3.times do
+            notes << create(:note)
+            notes << create(:discussion_note_on_merge_request)
+            notes << create(:note_on_merge_request)
+            notes << create(:note_on_commit)
+            notes << create(:diff_note_on_merge_request)
+          end
+
+          described_class.track!(*notes)
+
+          expect { described_class.new.execute }.not_to exceed_all_query_limit(control)
         end
-
-        described_class.track!(*notes)
-
-        control = ActiveRecord::QueryRecorder.new(skip_cached: false) { described_class.new.execute }
-
-        3.times do
-          notes << create(:note)
-          notes << create(:discussion_note_on_merge_request)
-          notes << create(:note_on_merge_request)
-          notes << create(:note_on_commit)
-        end
-
-        described_class.track!(*notes)
-
-        expect { described_class.new.execute }.not_to exceed_all_query_limit(control)
       end
 
       it 'does not have N+1 queries for issues' do

spec/workers/merge_requests/assignees_change_worker_spec.rb

@@ -51,7 +51,7 @@ RSpec.describe MergeRequests::AssigneesChangeWorker do
 
     it 'gets MergeRequests::UpdateAssigneesService to handle the changes' do
       expect_next(::MergeRequests::UpdateAssigneesService)
-        .to receive(:handle_assignee_changes).with(merge_request, old_assignees)
+        .to receive(:handle_assignee_changes).with(merge_request, match_array(old_assignees))
 
       worker.perform(merge_request.id, user.id, user_ids)
     end