Merge branch...

Merge branch '300351-es-migration-add-permission-data-to-notes-attempt-3-as-indexed-json-changes' into 'master'

Delete orphaned notes from Elasticsearch index

See merge request gitlab-org/gitlab!56162

ee/lib/elastic/instance_proxy_util.rb

@@ -57,7 +57,7 @@ module Elastic
     # protect against missing project and project_feature and set visibility to PRIVATE
     # if the project_feature is missing on a project
     def safely_read_project_feature_for_elasticsearch(feature)
-      return unless target.project
+      return ProjectFeature::DISABLED unless target.project
 
       if target.project.project_feature
         target.project.project_feature.access_level(feature)

ee/lib/elastic/latest/document_should_be_deleted_from_index_error.rb

+# frozen_string_literal: true
+
+module Elastic
+  module Latest
+    class DocumentShouldBeDeletedFromIndexError < StandardError
+      attr_reader :class_name, :record_id
+
+      def initialize(class_name, record_id)
+        @class_name, @record_id = class_name, record_id
+      end
+
+      def message
+        "#{class_name} with id #{record_id} should be deleted from the index."
+      end
+    end
+  end
+end

ee/lib/elastic/latest/note_instance_proxy.rb

@@ -6,9 +6,15 @@ module Elastic
       delegate :noteable, to: :target
 
       def as_indexed_json(options = {})
+        # `noteable` can be sometimes be nil (eg. when a commit has been
+        # deleted) or somehow it was left orphaned in the database. In such
+        # cases we want to delete it from the index since there is no value in
+        # having orphaned notes be searchable.
+        raise Elastic::Latest::DocumentShouldBeDeletedFromIndexError.new(target.class.name, target.id) if noteable.nil?
+
         data = {}
 
-        # We don't use as_json(only: ...) because it calls all virtual and serialized attributtes
+        # We don't use as_json(only: ...) because it calls all virtual and serialized attributes
         # https://gitlab.com/gitlab-org/gitlab/issues/349
         [:id, :note, :project_id, :noteable_type, :noteable_id, :created_at, :updated_at, :confidential].each do |attr|
           data[attr.to_s] = safely_read_attribute_for_elasticsearch(attr)
@@ -22,12 +28,11 @@ module Elastic
           }
         end
 
-        # only attempt to set project permissions if associated to a project
         # do not add the permission fields unless the `remove_permissions_data_from_notes_documents`
         # migration has completed otherwise the migration will never finish
-        if target.project && Elastic::DataMigrationService.migration_has_finished?(:remove_permissions_data_from_notes_documents)
-          data['visibility_level'] = target.project.visibility_level
-          merge_project_feature_access_level(data, noteable)
+        if Elastic::DataMigrationService.migration_has_finished?(:remove_permissions_data_from_notes_documents)
+          data['visibility_level'] = target.project&.visibility_level || Gitlab::VisibilityLevel::PRIVATE
+          merge_project_feature_access_level(data)
         end
 
         data.merge(generic_attributes)
@@ -35,9 +40,7 @@ module Elastic
 
       private
 
-      def merge_project_feature_access_level(data, noteable)
-        return unless noteable
-
+      def merge_project_feature_access_level(data)
         case noteable
         when Snippet
           data['snippets_access_level'] = safely_read_project_feature_for_elasticsearch(:snippets)

ee/lib/gitlab/elastic/bulk_indexer.rb

@@ -63,6 +63,9 @@ module Gitlab
         op = build_op(ref, proxy)
 
         submit(ref, { index: op }, proxy.as_indexed_json)
+      rescue ::Elastic::Latest::DocumentShouldBeDeletedFromIndexError => error
+        logger.warn(message: error.message, record_id: error.record_id, class_name: error.class_name)
+        delete(ref)
       end
 
       def delete(ref)

ee/spec/lib/gitlab/elastic/bulk_indexer_spec.rb

@@ -146,6 +146,19 @@ RSpec.describe Gitlab::Elastic::BulkIndexer, :elastic do
 
         expect(search_one(Issue)).to have_attributes(issue_as_json)
       end
+
+      it 'deletes the issue from the index if DocumentShouldBeDeletedFromIndexException is raised' do
+        database_record = issue_as_ref.database_record
+        allow(database_record.__elasticsearch__)
+          .to receive(:as_indexed_json)
+                .and_raise ::Elastic::Latest::DocumentShouldBeDeletedFromIndexError.new(database_record.class.name, database_record.id)
+
+        expect(indexer.process(issue_as_ref).flush).to be_empty
+
+        refresh_index!
+
+        expect(search(Issue, '*').size).to eq(0)
+      end
     end
 
     context 'deleting an issue' do

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

@@ -121,36 +121,35 @@ RSpec.describe Note, :elastic do
       expect(note.__elasticsearch__.as_indexed_json).to eq(expected_hash)
     end
 
-    it 'does not raise error for notes with null noteable references' do
+    it 'raises Elastic::Latest::DocumentShouldBeDeletedFromIndexError when noteable is nil' do
       note = create(:note_on_issue)
       allow(note).to receive(:noteable).and_return(nil)
 
-      expect { note.__elasticsearch__.as_indexed_json }.not_to raise_error
+      expect { note.__elasticsearch__.as_indexed_json }.to raise_error(::Elastic::Latest::DocumentShouldBeDeletedFromIndexError)
     end
 
-    where(:note_type, :project_permission, :access_level) do
+    where(:note_type, :project_feature_permission, :access_level) do
       :note_on_issue                              | ProjectFeature::ENABLED      | 'issues_access_level'
       :note_on_project_snippet                    | ProjectFeature::DISABLED     | 'snippets_access_level'
-      :note_on_personal_snippet                   | nil                          | false
+      :note_on_personal_snippet                   | nil                          | nil
       :note_on_merge_request                      | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
       :note_on_commit                             | ProjectFeature::PRIVATE      | 'repository_access_level'
       :diff_note_on_merge_request                 | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
       :diff_note_on_commit                        | ProjectFeature::PRIVATE      | 'repository_access_level'
-      :diff_note_on_design                        | ProjectFeature::ENABLED      | false
+      :diff_note_on_design                        | ProjectFeature::ENABLED      | nil
       :legacy_diff_note_on_merge_request          | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
       :legacy_diff_note_on_commit                 | ProjectFeature::PRIVATE      | 'repository_access_level'
-      :note_on_alert                              | ProjectFeature::PRIVATE      | false
-      :note_on_design                             | ProjectFeature::ENABLED      | false
-      :note_on_epic                               | nil                          | false
-      :note_on_vulnerability                      | ProjectFeature::PRIVATE      | false
-      :discussion_note_on_vulnerability           | ProjectFeature::PRIVATE      | false
+      :note_on_alert                              | ProjectFeature::PRIVATE      | nil
+      :note_on_design                             | ProjectFeature::ENABLED      | nil
+      :note_on_epic                               | nil                          | nil
+      :note_on_vulnerability                      | ProjectFeature::PRIVATE      | nil
+      :discussion_note_on_vulnerability           | ProjectFeature::PRIVATE      | nil
       :discussion_note_on_merge_request           | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
       :discussion_note_on_issue                   | ProjectFeature::ENABLED      | 'issues_access_level'
       :discussion_note_on_project_snippet         | ProjectFeature::DISABLED     | 'snippets_access_level'
-      :discussion_note_on_personal_snippet        | nil                          | false
-      :note_on_merge_request                      | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
+      :discussion_note_on_personal_snippet        | nil                          | nil
       :discussion_note_on_commit                  | ProjectFeature::PRIVATE      | 'repository_access_level'
-      :track_mr_picking_note                      | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
+      :track_mr_picking_note                      | nil                          | nil
     end
 
     with_them do
@@ -160,7 +159,7 @@ RSpec.describe Note, :elastic do
       let(:note_json) { note.__elasticsearch__.as_indexed_json }
 
       before do
-        project.project_feature.update_attribute(access_level.to_sym, project_permission) if access_level.present?
+        project.project_feature.update_attribute(access_level.to_sym, project_feature_permission) if access_level.present?
       end
 
       it 'does not contain permissions if remove_permissions_data_from_notes_documents is not finished' do
@@ -175,14 +174,24 @@ RSpec.describe Note, :elastic do
       it 'contains the correct permissions', :aggregate_failures do
         if access_level
           expect(note_json).to have_key(access_level)
-          expect(note_json[access_level]).to eq(project_permission)
+          expect(note_json[access_level]).to eq(project_feature_permission)
+        end
+
+        expected_visibility_level = project&.visibility_level || Gitlab::VisibilityLevel::PRIVATE
+        expect(note_json).to have_key('visibility_level')
+        expect(note_json['visibility_level']).to eq(expected_visibility_level)
+      end
+
+      context 'when the project does not exist' do
+        before do
+          note.project = nil
         end
 
-        if project_permission
-          expect(note_json).to have_key('visibility_level')
-          expect(note_json['visibility_level']).to eq(project.visibility_level)
-        else
-          expect(note_json).not_to have_key('visibility_level')
+        it 'has DISABLED access_level' do
+          if access_level
+            expect(note_json).to have_key(access_level)
+            expect(note_json[access_level]).to eq(ProjectFeature::DISABLED)
+          end
         end
       end
     end