Merge branch '335825-add-calculation-for-group-ancestry-for-issues' into 'master'

Add namespace_ancestry to issues index mapping

See merge request gitlab-org/gitlab!68796

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

@@ -3,7 +3,7 @@ module Elastic
   module ApplicationVersionedSearch
     extend ActiveSupport::Concern
 
-    FORWARDABLE_INSTANCE_METHODS = [:es_id, :es_parent].freeze
+    FORWARDABLE_INSTANCE_METHODS = [:es_id, :es_parent, :namespace_ancestry].freeze
     FORWARDABLE_CLASS_METHODS = [:elastic_search, :es_import, :es_type, :index_name, :document_type, :mapping, :mappings, :settings, :import].freeze
 
     # rubocop:disable Gitlab/ModuleWithInstanceVariables

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

@@ -31,21 +31,13 @@ module EE
       end
 
       def update_elasticsearch_hooks(updated_project_ids)
-        # Handle when group is moved to a new group. There is no way to know
-        # whether the group was using Elasticsearch before the transfer. If Elasticsearch limit indexing is
-        # enabled, each associated project has the ES cache is invalidated all associated data is indexed.
-        # If Elasticsearch limit indexing is disabled, all projects are indexed and only those with visibility
-        # changes have their ES cache entry invalidated.
-        if ::Gitlab::CurrentSettings.elasticsearch_limit_indexing?
-          ::Project.id_in(group.all_projects.select(:id)).find_each do |project|
-            project.invalidate_elasticsearch_indexes_cache!
-
-            ::Elastic::ProcessInitialBookkeepingService.backfill_projects!(project) if project.maintaining_elasticsearch?
-          end
-        else
-          ::Project.id_in(updated_project_ids).find_each do |project|
-            project.maintain_elasticsearch_update(updated_attributes: [:visibility_level]) if project.maintaining_elasticsearch?
-          end
+        # When a group is moved to a new group, there is no way to know whether the group was using Elasticsearch
+        # before the transfer. If Elasticsearch limit indexing is enabled, each project has the ES cache
+        # invalidated. Reindex all projects and associated data to make sure the namespace_ancestry field gets
+        # updated in each document.
+        ::Project.id_in(group.all_projects.select(:id)).find_each do |project|
+          project.invalidate_elasticsearch_indexes_cache! if ::Gitlab::CurrentSettings.elasticsearch_limit_indexing?
+          ::Elastic::ProcessInitialBookkeepingService.backfill_projects!(project) if project.maintaining_elasticsearch?
         end
       end
     end

ee/app/services/ee/projects/transfer_service.rb

@@ -35,15 +35,14 @@ module EE
       end
 
       def update_elasticsearch_hooks
-        return unless ::Gitlab::CurrentSettings.elasticsearch_limit_indexing?
-
-        # handle when project is moved to a new namespace with different elasticsearch settings
-        # than the old namespace
-        if old_namespace.use_elasticsearch? != new_namespace.use_elasticsearch?
+        # When a project is moved to a new namespace, invalidate the ES cache if Elasticsearch limit indexing is enabled
+        # and the Elasticsearch settings are different between the two namespaces. The project and all associated data
+        # is indexed to make sure the namespace_ancestry field gets updated in each document.
+        if ::Gitlab::CurrentSettings.elasticsearch_limit_indexing? && old_namespace.use_elasticsearch? != new_namespace.use_elasticsearch?
           project.invalidate_elasticsearch_indexes_cache!
-
-          ::Elastic::ProcessInitialBookkeepingService.backfill_projects!(project) if project.maintaining_elasticsearch?
         end
+
+        ::Elastic::ProcessInitialBookkeepingService.backfill_projects!(project) if project.maintaining_elasticsearch?
       end
 
       override :remove_paid_features

ee/app/workers/concerns/elastic/migration_helper.rb

@@ -102,5 +102,9 @@ module Elastic
         }
       }
     end
+
+    def update_mapping!(index_name, mappings)
+      helper.update_mapping(index_name: index_name, mappings: mappings)
+    end
   end
 end

ee/elastic/migrate/20210813134600_add_namespace_ancestry_to_issues_mapping.rb

+# frozen_string_literal: true
+
+class AddNamespaceAncestryToIssuesMapping < Elastic::Migration
+  include Elastic::MigrationHelper
+
+  DOCUMENT_KLASS = Issue
+
+  def migrate
+    if completed?
+      log 'Skipping adding namespace_ancestry to issues mapping migration since it is already applied'
+      return
+    end
+
+    log 'Adding namespace_ancestry to issues mapping'
+    update_mapping!(index_name, { properties: { namespace_ancestry: { type: 'text', index_prefixes: { min_chars: 1, max_chars: 19 } } } })
+  end
+
+  def completed?
+    helper.refresh_index(index_name: index_name)
+
+    mappings = helper.get_mapping(index_name: index_name)
+    mappings.dig('namespace_ancestry').present?
+  end
+
+  private
+
+  def index_name
+    DOCUMENT_KLASS.__elasticsearch__.index_name
+  end
+end

ee/lib/elastic/latest/application_instance_proxy.rb

@@ -17,6 +17,12 @@ module Elastic
         "#{es_type}_#{target.id}"
       end
 
+      def namespace_ancestry
+        project = target.is_a?(Project) ? target : target.project
+        namespace = project.namespace
+        namespace.self_and_ancestor_ids(hierarchy_order: :desc).join('-')
+      end
+
       private
 
       def generic_attributes

ee/lib/elastic/latest/issue_class_proxy.rb

@@ -30,7 +30,7 @@ module Elastic
 
       # rubocop: disable CodeReuse/ActiveRecord
       def preload_indexing_data(relation)
-        relation.includes(:issue_assignees, project: [:project_feature])
+        relation.includes(:issue_assignees, project: [:project_feature, :namespace])
       end
       # rubocop: enable CodeReuse/ActiveRecord
 

ee/lib/elastic/latest/issue_config.rb

@@ -36,6 +36,7 @@ module Elastic
         indexes :visibility_level, type: :integer
         indexes :issues_access_level, type: :integer
         indexes :upvotes, type: :integer
+        indexes :namespace_ancestry, type: :text, index_prefixes: { min_chars: 1, max_chars: 19 }
       end
     end
   end

ee/lib/elastic/latest/issue_instance_proxy.rb

@@ -21,6 +21,7 @@ module Elastic
         data['issues_access_level'] = safely_read_project_feature_for_elasticsearch(:issues)
 
         data['upvotes'] = target.upvotes_count
+        data['namespace_ancestry'] = target.namespace_ancestry if Elastic::DataMigrationService.migration_has_finished?(:add_namespace_ancestry_to_issues_mapping)
 
         data.merge(generic_attributes)
       end

ee/lib/gitlab/elastic/helper.rb

@@ -229,10 +229,20 @@ module Gitlab
         settings.dig(index, 'settings', 'index')
       end
 
+      def get_mapping(index_name: nil)
+        index = target_index_name(target: index_name)
+        mappings = client.indices.get_mapping(index: index)
+        mappings.dig(index, 'mappings', 'properties')
+      end
+
       def update_settings(index_name: nil, settings:)
         client.indices.put_settings(index: index_name || target_index_name, body: settings)
       end
 
+      def update_mapping(index_name: nil, mappings:)
+        client.indices.put_mapping(index: index_name || target_index_name, body: mappings)
+      end
+
       def switch_alias(from: target_index_name, alias_name: target_name, to:)
         actions = [
           {

ee/spec/elastic/migrate/20210813134600_add_namespace_ancestry_to_issues_mapping_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+require File.expand_path('ee/elastic/migrate/20210813134600_add_namespace_ancestry_to_issues_mapping.rb')
+
+RSpec.describe AddNamespaceAncestryToIssuesMapping, :elastic, :sidekiq_inline do
+  let(:version) { 20210813134600 }
+  let(:migration) { described_class.new(version) }
+  let(:helper) { Gitlab::Elastic::Helper.new }
+
+  before do
+    allow(migration).to receive(:helper).and_return(helper)
+  end
+
+  describe '.migrate' do
+    subject { migration.migrate }
+
+    context 'when migration is already completed' do
+      it 'does not modify data' do
+        expect(helper).not_to receive(:update_mapping)
+
+        subject
+      end
+    end
+
+    context 'migration process' do
+      before do
+        allow(helper).to receive(:get_mapping).and_return({})
+      end
+
+      it 'updates the issues index mappings' do
+        expect(helper).to receive(:update_mapping)
+
+        subject
+      end
+    end
+  end
+
+  describe '.completed?' do
+    context 'mapping has been updated' do
+      specify { expect(migration).to be_completed }
+    end
+
+    context 'mapping has not been updated' do
+      before do
+        allow(helper).to receive(:get_mapping).and_return({})
+      end
+
+      specify { expect(migration).not_to be_completed }
+    end
+  end
+end

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

@@ -105,33 +105,48 @@ RSpec.describe Issue, :elastic do
     expect(results.first.title).to eq('bla-bla issue')
   end
 
-  it "returns json with all needed elements" do
-    assignee = create(:user)
-    project = create(:project, :internal)
-    issue = create :issue, project: project, assignees: [assignee]
-    create(:award_emoji, :upvote, awardable: issue)
-
-    expected_hash = issue.attributes.extract!(
-      'id',
-      'iid',
-      'title',
-      'description',
-      'created_at',
-      'updated_at',
-      'project_id',
-      'author_id',
-      'confidential'
-    ).merge({
-      'type' => issue.es_type,
-      'state' => issue.state,
-      'upvotes' => 1
-    })
-
-    expected_hash['assignee_id'] = [assignee.id]
-    expected_hash['issues_access_level'] = ProjectFeature::ENABLED
-    expected_hash['visibility_level'] = Gitlab::VisibilityLevel::INTERNAL
-
-    expect(issue.__elasticsearch__.as_indexed_json).to eq(expected_hash)
+  context 'json' do
+    let_it_be(:assignee) { create(:user) }
+    let_it_be(:group) { create(:group) }
+    let_it_be(:subgroup) { create(:group, parent: group) }
+    let_it_be(:project) { create(:project, :internal, namespace: subgroup) }
+    let_it_be(:issue) { create(:issue, project: project, assignees: [assignee]) }
+    let_it_be(:award_emoji) { create(:award_emoji, :upvote, awardable: issue) }
+
+    context 'when add_namespace_ancestry_to_issues_mapping migration is not done' do
+      before do
+        set_elasticsearch_migration_to :add_namespace_ancestry_to_issues_mapping, including: false
+      end
+
+      it "returns json without namespace_ancestry" do
+        expect(issue.__elasticsearch__.as_indexed_json.keys).not_to include('namespace_ancestry')
+      end
+    end
+
+    it "returns json with all needed elements" do
+      expected_hash = issue.attributes.extract!(
+        'id',
+        'iid',
+        'title',
+        'description',
+        'created_at',
+        'updated_at',
+        'project_id',
+        'author_id',
+        'confidential'
+      ).merge({
+                'type' => issue.es_type,
+                'state' => issue.state,
+                'upvotes' => 1,
+                'namespace_ancestry' => "#{group.id}-#{subgroup.id}"
+              })
+
+      expected_hash['assignee_id'] = [assignee.id]
+      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
   end
 
   it 'handles a project missing project_feature', :aggregate_failures do

ee/spec/services/groups/transfer_service_spec.rb

@@ -45,7 +45,7 @@ RSpec.describe Groups::TransferService, '#execute' do
           create(:elasticsearch_indexed_namespace, namespace: new_group)
         end
 
-        it 'invalidates the cache and indexes the project and associated issues only' do
+        it 'invalidates the cache and indexes the project and all associated data' do
           expect(project).not_to receive(:maintain_elasticsearch_update)
           expect(project).not_to receive(:maintain_elasticsearch_destroy)
           expect(::Elastic::ProcessInitialBookkeepingService).to receive(:backfill_projects!).with(project)
@@ -57,27 +57,22 @@ RSpec.describe Groups::TransferService, '#execute' do
     end
 
     context 'when elasticsearch_limit_indexing is off' do
-      context 'when visibility changes' do
-        let(:new_group) { create(:group, :private) }
-
-        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 merge_requests notes])
-          expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project2)
-          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 merge_requests notes])
+      let(:new_group) { create(:group, :private) }
 
-          transfer_service.execute(new_group)
+      it 'does not invalidate the cache and reindexes projects and associated data' do
+        project1 = create(:project, :repository, :public, namespace: group)
+        project2 = create(:project, :repository, :public, namespace: group)
+        project3 = create(:project, :repository, :private, namespace: group)
 
-          expect(transfer_service.error).not_to be
-          expect(group.parent).to eq(new_group)
-        end
+        expect(::Gitlab::CurrentSettings).not_to receive(:invalidate_elasticsearch_indexes_cache_for_project!)
+        expect(::Elastic::ProcessInitialBookkeepingService).to receive(:backfill_projects!).with(project1)
+        expect(::Elastic::ProcessInitialBookkeepingService).to receive(:backfill_projects!).with(project2)
+        expect(::Elastic::ProcessInitialBookkeepingService).to receive(:backfill_projects!).with(project3)
+
+        transfer_service.execute(new_group)
+
+        expect(transfer_service.error).not_to be
+        expect(group.parent).to eq(new_group)
       end
     end
   end

ee/spec/services/projects/transfer_service_spec.rb

@@ -70,17 +70,6 @@ RSpec.describe Projects::TransferService do
       stub_ee_application_setting(elasticsearch_indexing: true)
     end
 
-    context 'when visibility level changes' do
-      let_it_be(:group) { create(:group, :private) }
-
-      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 merge_requests notes])
-
-        subject.execute(group)
-      end
-    end
-
     context 'when elasticsearch_limit_indexing is on' do
       before do
         stub_ee_application_setting(elasticsearch_limit_indexing: true)
@@ -93,7 +82,6 @@ RSpec.describe Projects::TransferService do
 
         it 'invalidates the cache and indexes the project and all associated data' do
           expect(Elastic::ProcessInitialBookkeepingService).to receive(:backfill_projects!).with(project)
-          expect(project).not_to receive(:maintain_elasticsearch_destroy)
           expect(::Gitlab::CurrentSettings).to receive(:invalidate_elasticsearch_indexes_cache_for_project!).with(project.id).and_call_original
 
           subject.execute(group)
@@ -106,9 +94,8 @@ RSpec.describe Projects::TransferService do
           create(:elasticsearch_indexed_namespace, namespace: project.namespace)
         end
 
-        it 'does not invalidate the cache does not index or delete anything' do
-          expect(Elastic::ProcessInitialBookkeepingService).not_to receive(:backfill_projects!).with(project)
-          expect(project).not_to receive(:maintain_elasticsearch_destroy)
+        it 'does not invalidate the cache and indexes the project and associated data' do
+          expect(Elastic::ProcessInitialBookkeepingService).to receive(:backfill_projects!).with(project)
           expect(::Gitlab::CurrentSettings).not_to receive(:invalidate_elasticsearch_indexes_cache_for_project!)
 
           subject.execute(group)
@@ -117,10 +104,9 @@ RSpec.describe Projects::TransferService do
     end
 
     context 'when elasticsearch_limit_indexing is off' do
-      it 'does not invalidate the cache and reindexes the project only' do
-        expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project)
-        expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async)
-        expect(::Gitlab::CurrentSettings).not_to receive(:invalidate_elasticsearch_indexes_cache_for_project!).with(project.id).and_call_original
+      it 'does not invalidate the cache and indexes the project and all associated data' do
+        expect(Elastic::ProcessInitialBookkeepingService).to receive(:backfill_projects!).with(project)
+        expect(::Gitlab::CurrentSettings).not_to receive(:invalidate_elasticsearch_indexes_cache_for_project!)
 
         subject.execute(group)
       end