Merge branch '273234-update-index-issues-when-project-permissions-change' into 'master'

Update issues in ES when project visibility settings change

See merge request gitlab-org/gitlab!47007

config/sidekiq_queues.yml

@@ -92,6 +92,8 @@
   - 1
 - - disallow_two_factor_for_subgroups
   - 1
+- - elastic_association_indexer
+  - 1
 - - elastic_commit_indexer
   - 1
 - - elastic_delete_project

ee/app/models/concerns/elastic/projects_search.rb

@@ -7,14 +7,18 @@ module Elastic
     include ApplicationVersionedSearch
 
     included do
+      extend ::Gitlab::Utils::Override
+
       def use_elasticsearch?
         ::Gitlab::CurrentSettings.elasticsearch_indexes_project?(self)
       end
 
+      override :maintain_elasticsearch_create
       def maintain_elasticsearch_create
         ::Elastic::ProcessInitialBookkeepingService.track!(self)
       end
 
+      override :maintain_elasticsearch_destroy
       def maintain_elasticsearch_destroy
         ElasticDeleteProjectWorker.perform_async(self.id, self.es_id)
       end

ee/app/models/ee/project_feature.rb

@@ -9,12 +9,22 @@ module EE
     prepended do
       set_available_features(EE_FEATURES)
 
-      # Ensure changes to project visibility settings go to elasticsearch
+      # Ensure changes to project visibility settings go to elasticsearch if the tracked field(s) change
       after_commit on: :update do
-        project.maintain_elasticsearch_update if project.maintaining_elasticsearch?
+        if project.maintaining_elasticsearch?
+          project.maintain_elasticsearch_update
+
+          ElasticAssociationIndexerWorker.perform_async(self.project.class.name, project_id, ['issues']) if elasticsearch_project_associations_need_updating?
+        end
       end
 
       default_value_for :requirements_access_level, value: Featurable::ENABLED, allows_nil: false
+
+      private
+
+      def elasticsearch_project_associations_need_updating?
+        self.previous_changes.key?(:issues_access_level)
+      end
     end
   end
 end

ee/app/services/elastic/process_bookkeeping_service.rb

@@ -53,6 +53,30 @@ module Elastic
       def with_redis(&blk)
         Gitlab::Redis::SharedState.with(&blk) # rubocop:disable CodeReuse/ActiveRecord
       end
+
+      def maintain_indexed_associations(object, associations)
+        each_indexed_association(object, associations) do |_, association|
+          association.find_in_batches do |group|
+            track!(*group)
+          end
+        end
+      end
+
+      private
+
+      def each_indexed_association(object, associations)
+        associations.each do |association_name|
+          association = object.association(association_name)
+          scope = association.scope
+          klass = association.klass
+
+          if klass == Note
+            scope = scope.searchable
+          end
+
+          yield klass, scope
+        end
+      end
     end
 
     def execute

ee/app/services/elastic/process_initial_bookkeeping_service.rb

@@ -4,8 +4,7 @@ module Elastic
   class ProcessInitialBookkeepingService < Elastic::ProcessBookkeepingService
     REDIS_SET_KEY = 'elastic:bulk:initial:0:zset'
     REDIS_SCORE_KEY = 'elastic:bulk:initial:0:score'
-
-    INDEXED_ASSOCIATIONS = [
+    INDEXED_PROJECT_ASSOCIATIONS = [
       :issues,
       :merge_requests,
       :snippets,
@@ -20,36 +19,12 @@ module Elastic
         projects.each do |project|
           raise ArgumentError, 'This method only accepts Projects' unless project.is_a?(Project)
 
-          maintain_indexed_associations(project)
+          maintain_indexed_associations(project, INDEXED_PROJECT_ASSOCIATIONS)
 
           ElasticCommitIndexerWorker.perform_async(project.id)
           ElasticCommitIndexerWorker.perform_async(project.id, nil, nil, true)
         end
       end
-
-      def each_indexed_association(project)
-        INDEXED_ASSOCIATIONS.each do |association_name|
-          association = project.association(association_name)
-          scope = association.scope
-          klass = association.klass
-
-          if klass == Note
-            scope = scope.searchable
-          end
-
-          yield klass, scope
-        end
-      end
-
-      private
-
-      def maintain_indexed_associations(project)
-        each_indexed_association(project) do |_, association|
-          association.find_in_batches do |group|
-            track!(*group)
-          end
-        end
-      end
     end
   end
 end

ee/app/workers/all_queues.yml

@@ -605,6 +605,14 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: elastic_association_indexer
+  :feature_category: :global_search
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent:
+  :tags: []
 - :name: elastic_commit_indexer
   :feature_category: :global_search
   :has_external_dependencies:

ee/app/workers/elastic_association_indexer_worker.rb

+# frozen_string_literal: true
+
+class ElasticAssociationIndexerWorker # rubocop:disable Scalability/IdempotentWorker
+  include ApplicationWorker
+
+  feature_category :global_search
+  loggable_arguments 0, 2
+
+  def perform(class_name, id, indexed_associations)
+    return unless Gitlab::CurrentSettings.elasticsearch_indexing?
+
+    klass = class_name.constantize
+    object = klass.find(id)
+    return unless object.use_elasticsearch?
+
+    Elastic::ProcessBookkeepingService.maintain_indexed_associations(object, indexed_associations)
+  end
+end

ee/spec/models/project_feature_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe ProjectFeature do
-  let(:project) { create(:project) }
+  let(:project) { create(:project, :public) }
   let(:user) { create(:user) }
 
   describe '#feature_available?' do
@@ -20,4 +20,35 @@ RSpec.describe ProjectFeature do
       end
     end
   end
+
+  describe 'project visibility changes' do
+    using RSpec::Parameterized::TableSyntax
+
+    before do
+      allow(project).to receive(:maintaining_elasticsearch?).and_return(true)
+    end
+
+    where(:feature, :worker_expected) do
+      'issues'          | true
+      'wiki'            | false
+      'builds'          | false
+      'merge_requests'  | false
+      'repository'      | false
+      'pages'           | false
+    end
+
+    with_them do
+      it 're-indexes project and project associations on update' do
+        expect(project).to receive(:maintain_elasticsearch_update)
+
+        if worker_expected
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, ['issues'])
+        else
+          expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async)
+        end
+
+        project.project_feature.update_attribute("#{feature}_access_level".to_sym, ProjectFeature::PRIVATE)
+      end
+    end
+  end
 end

ee/spec/models/project_spec.rb

@@ -2543,7 +2543,7 @@ RSpec.describe Project do
     end
   end
 
-  describe 'caculate template repositories' do
+  describe 'calculate template repositories' do
     let(:group1) { create(:group) }
     let(:group2) { create(:group) }
     let(:group2_sub1) { create(:group, parent: group2) }

ee/spec/services/elastic/process_bookkeeping_service_spec.rb

@@ -76,6 +76,28 @@ RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_st
     end
   end
 
+  describe '.maintain_indexed_associations' do
+    let(:project) { create(:project) }
+
+    it 'calls track! for each associated object' do
+      issue_1 = create(:issue, project: project)
+      issue_2 = create(:issue, project: project)
+
+      expect(described_class).to receive(:track!).with(issue_1, issue_2)
+
+      described_class.maintain_indexed_associations(project, ['issues'])
+    end
+
+    it 'correctly scopes associated note objects to not include system notes' do
+      note_searchable = create(:note, :on_issue, project: project)
+      create(:note, :on_issue, :system, project: project)
+
+      expect(described_class).to receive(:track!).with(note_searchable)
+
+      described_class.maintain_indexed_associations(project, ['notes'])
+    end
+  end
+
   describe '#execute' do
     let(:limit) { 5 }
 

ee/spec/services/elastic/process_initial_bookkeeping_service_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Elastic::ProcessInitialBookkeepingService do
 
   describe '.backfill_projects!' do
     it 'calls initial project indexing' do
-      expect(described_class).to receive(:maintain_indexed_associations)
+      expect(described_class).to receive(:maintain_indexed_associations).with(project, Elastic::ProcessInitialBookkeepingService::INDEXED_PROJECT_ASSOCIATIONS)
       expect(ElasticCommitIndexerWorker).to receive(:perform_async).with(project.id)
       expect(ElasticCommitIndexerWorker).to receive(:perform_async).with(project.id, nil, nil, true)
 

ee/spec/workers/elastic_association_indexer_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ElasticAssociationIndexerWorker do
+  subject { described_class.new }
+
+  let(:indexed_associations) { [:issues] }
+
+  before do
+    stub_ee_application_setting(elasticsearch_indexing: true)
+  end
+
+  context 'when elasticsearch_indexing is disabled' do
+    it 'does nothing' do
+      stub_ee_application_setting(elasticsearch_indexing: false)
+      expect(Elastic::ProcessBookkeepingService).not_to receive(:maintain_indexed_associations)
+
+      subject.perform('Project', 1, indexed_associations)
+    end
+  end
+
+  context 'when elasticsearch_indexing is enabled' do
+    let!(:project) { create(:project) }
+
+    context 'but object is not setup to use elasticsearch' do
+      it 'does nothing' do
+        expect_next_found_instance_of(Project) do |p|
+          expect(p).to receive(:use_elasticsearch?).and_return(false)
+        end
+        expect(Elastic::ProcessBookkeepingService).not_to receive(:maintain_indexed_associations)
+
+        subject.perform(project.class.name, project.id, indexed_associations)
+      end
+    end
+
+    it 'updates associations for the object' do
+      expect(Elastic::ProcessBookkeepingService).to receive(:maintain_indexed_associations).with(project, indexed_associations)
+
+      subject.perform(project.class.name, project.id, indexed_associations)
+    end
+  end
+end