Delete commit notes from index that are orphaned

We discovered at
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55878 that there
are sometimes orphaned commit notes. Since these are making it difficult
to migrate as we can't set the permissions correctly for these and also
since you can't actually navigate to a note on a deleted commit there is
no point in keeping these in the index.

This commit was extracted from a combined MR at
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55883 so it could
be merged independently from the migration.

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

@@ -3,12 +3,17 @@
 module Elastic
   module Latest
     class NoteInstanceProxy < ApplicationInstanceProxy
-      delegate :noteable, to: :target
+      delegate :noteable, :noteable_type, to: :target
 
       def as_indexed_json(options = {})
+        # Notes on commits should return the commit object when `notable` is called. However, `noteable` can be null
+        # when a commit has been deleted so an error is raised to alert the caller that the document should be deleted
+        # from the index.
+        raise Elastic::Latest::DocumentShouldBeDeletedFromIndexError.new(target.class.name, target.id) if noteable_type == 'Commit' && 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 +27,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,17 +39,18 @@ module Elastic
 
       private
 
-      def merge_project_feature_access_level(data, noteable)
-        return unless noteable
+      def merge_project_feature_access_level(data)
+        return unless noteable_type
 
-        case noteable
-        when Snippet
+        case noteable_type
+        when 'Snippet'
           data['snippets_access_level'] = safely_read_project_feature_for_elasticsearch(:snippets)
-        when Commit
+        when 'Commit'
           data['repository_access_level'] = safely_read_project_feature_for_elasticsearch(:repository)
-        when Issue, MergeRequest
-          access_level_attribute = ProjectFeature.access_level_attribute(noteable)
-          data[access_level_attribute.to_s] = safely_read_project_feature_for_elasticsearch(noteable)
+        when 'Issue', 'MergeRequest'
+          klass = noteable_type.constantize
+          access_level_attribute = ProjectFeature.access_level_attribute(klass)
+          data[access_level_attribute.to_s] = safely_read_project_feature_for_elasticsearch(klass)
         else
           # do nothing for other note types (DesignManagement::Design, AlertManagement::Alert, Epic, Vulnerability )
           # are indexed but not currently searchable so we will not add permission

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
-      note = create(:note_on_issue)
+    it 'does raises Elastic::Latest::DocumentShouldBeDeletedFromIndexError when commit is not found and noteable is null' do
+      note = create(:note_on_commit)
       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,15 +174,12 @@ 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
 
-        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')
-        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
     end
   end