Merge branch '273234-denormalize-project-visibility-level' into 'master'

De-normalize project visibility_level into issues Elasticsearch documents

See merge request gitlab-org/gitlab!48436

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

@@ -48,18 +48,60 @@ module Elastic
       ::Elastic::ProcessBookkeepingService.track!(self)
     end
 
-    def maintain_elasticsearch_update
+    def maintain_elasticsearch_update(updated_attributes: previous_changes.keys)
+      updated_attributes = updated_attributes.map(&:to_s) # Allow caller to provide symbols but keep consistent to using strings
       ::Elastic::ProcessBookkeepingService.track!(self)
+
+      associations_to_update = associations_needing_elasticsearch_update(updated_attributes)
+      if associations_to_update.present?
+        ElasticAssociationIndexerWorker.perform_async(self.class.name, id, associations_to_update)
+      end
     end
 
     def maintain_elasticsearch_destroy
       ::Elastic::ProcessBookkeepingService.track!(self)
     end
 
+    # Override in child object if there are associations that need to be
+    # updated when specific fields are updated
+    def associations_needing_elasticsearch_update(updated_attributes)
+      self.class.elastic_index_dependants.map do |dependant|
+        association_name = dependant[:association_name]
+        on_change = dependant[:on_change]
+
+        next nil unless updated_attributes.include?(on_change.to_s)
+
+        association_name.to_s
+      end.compact.uniq
+    end
+
     class_methods do
       def __elasticsearch__
         @__elasticsearch__ ||= ::Elastic::MultiVersionClassProxy.new(self)
       end
+
+      # Mark a dependant association as needing to be updated when a specific
+      # field in this object changes. For example if you want to update
+      # project.issues in the index when project.visibility_level is changed
+      # then you can declare that as:
+      #
+      # elastic_index_dependant_association :issues, on_change: :visibility_level
+      #
+      def elastic_index_dependant_association(association_name, on_change:)
+        # This class is used for non ActiveRecord models but this method is not
+        # applicable for that so we raise.
+        raise "elastic_index_dependant_association is not applicable as this class is not an ActiveRecord model." unless self < ActiveRecord::Base
+
+        # Validate these are actually correct associations before sending to
+        # Sidekiq to avoid errors occuring when the job is picked up.
+        raise "Invalid association to index. \"#{association_name}\" is either not a collection or not an association. Hint: You must declare the has_many before declaring elastic_index_dependant_association." unless reflect_on_association(association_name)&.collection?
+
+        elastic_index_dependants << { association_name: association_name, on_change: on_change }
+      end
+
+      def elastic_index_dependants
+        @elastic_index_dependants ||= []
+      end
     end
   end
 end

ee/app/models/ee/project.rb

@@ -100,6 +100,8 @@ module EE
 
       has_many :incident_management_oncall_schedules, class_name: 'IncidentManagement::OncallSchedule', inverse_of: :project
 
+      elastic_index_dependant_association :issues, on_change: :visibility_level
+
       scope :with_shared_runners_limit_enabled, -> do
         if ::Ci::Runner.has_shared_runners_with_non_zero_public_cost?
           with_shared_runners

ee/app/services/ee/groups/transfer_service.rb

@@ -16,7 +16,7 @@ module EE
       override :post_update_hooks
       def post_update_hooks(updated_project_ids)
         ::Project.id_in(updated_project_ids).find_each do |project|
-          project.maintain_elasticsearch_update if project.maintaining_elasticsearch?
+          project.maintain_elasticsearch_update(updated_attributes: [:visibility_level]) if project.maintaining_elasticsearch?
         end
         super
       end

ee/lib/elastic/latest/issue_instance_proxy.rb

@@ -13,6 +13,7 @@ module Elastic
         end
 
         data['assignee_id'] = safely_read_attribute_for_elasticsearch(:assignee_ids)
+        data['visibility_level'] = target.project.visibility_level
 
         # protect against missing project_feature and set visibility to PRIVATE
         # if the project_feature is missing on a project

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Elastic::ApplicationVersionedSearch do
+  let(:klass) do
+    Class.new(ApplicationRecord) do
+      include Elastic::ApplicationVersionedSearch
+
+      has_many :widgets
+    end
+  end
+
+  describe '.elastic_index_dependant_association' do
+    it 'adds the associations to elastic_index_dependants' do
+      klass.elastic_index_dependant_association(:widgets, on_change: :title)
+
+      expect(klass.elastic_index_dependants).to include({
+        association_name: :widgets,
+        on_change: :title
+      })
+    end
+
+    context 'when the association does not exist' do
+      it 'raises an error' do
+        expect { klass.elastic_index_dependant_association(:foo_bars, on_change: :bar) }
+          .to raise_error("Invalid association to index. \"foo_bars\" is either not a collection or not an association. Hint: You must declare the has_many before declaring elastic_index_dependant_association.")
+      end
+    end
+
+    context 'when the class is not an ApplicationRecord' do
+      let(:not_application_record) do
+        Class.new do
+          include Elastic::ApplicationVersionedSearch
+        end
+      end
+
+      it 'raises an error' do
+        expect { not_application_record.elastic_index_dependant_association(:widgets, on_change: :title) }
+          .to raise_error("elastic_index_dependant_association is not applicable as this class is not an ActiveRecord model.")
+      end
+    end
+  end
+end

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

@@ -107,6 +107,7 @@ RSpec.describe Issue, :elastic do
 
   it "returns json with all needed elements" do
     assignee = create(:user)
+    project = create(:project, :internal)
     issue = create :issue, project: project, assignees: [assignee]
 
     expected_hash = issue.attributes.extract!(
@@ -129,7 +130,8 @@ RSpec.describe Issue, :elastic do
     })
 
     expected_hash['assignee_id'] = [assignee.id]
-    expected_hash['issues_access_level'] = issue.project.project_feature.issues_access_level
+    expected_hash['issues_access_level'] = ProjectFeature::ENABLED
+    expected_hash['visibility_level'] = Gitlab::VisibilityLevel::INTERNAL
 
     expect(issue.__elasticsearch__.as_indexed_json).to eq(expected_hash)
   end

ee/spec/models/project_spec.rb

@@ -2638,4 +2638,30 @@ RSpec.describe Project do
       project.add_template_export_job(current_user: user)
     end
   end
+
+  context 'indexing updates in Elasticsearch', :elastic do
+    before do
+      stub_ee_application_setting(elasticsearch_indexing: true)
+    end
+
+    context 'on update' do
+      let(:project) { create(:project, :public) }
+
+      context 'when updating the visibility_level' do
+        it 'triggers ElasticAssociationIndexerWorker to update issues' do
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, ['issues'])
+
+          project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+        end
+      end
+
+      context 'when changing the title' do
+        it 'does not trigger ElasticAssociationIndexerWorker to update issues' do
+          expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async)
+
+          project.update!(title: 'The new title')
+        end
+      end
+    end
+  end
 end

ee/spec/services/groups/transfer_service_spec.rb

@@ -21,14 +21,17 @@ RSpec.describe Groups::TransferService, '#execute' do
       stub_ee_application_setting(elasticsearch_indexing: true)
     end
 
-    it 'reindexes projects', :elastic do
+    it 'reindexes projects and associated issues', :elastic do
       project1 = create(:project, :repository, :public, namespace: group)
       project2 = create(:project, :repository, :public, namespace: group)
       project3 = create(:project, :repository, :private, namespace: group)
 
       expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project1)
+      expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project1.id, ['issues'])
       expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project2)
+      expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project2.id, ['issues'])
       expect(Elastic::ProcessBookkeepingService).not_to receive(:track!).with(project3)
+      expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async).with('Project', project3.id, ['issues'])
 
       transfer_service.execute(new_group)