Merge branch...

Merge branch '300351-remove-joins-from-the-elasticsearch-query-for-project-group-scoped-notes-comments-search-3' into 'master'

Add permission for indexed notes & update when they change (2nd attempt)

See merge request gitlab-org/gitlab!54482

ee/app/models/ee/project.rb

@@ -108,6 +108,7 @@ module EE
       has_one :security_orchestration_policy_configuration, class_name: 'Security::OrchestrationPolicyConfiguration', foreign_key: :project_id, inverse_of: :project
 
       elastic_index_dependant_association :issues, on_change: :visibility_level
+      elastic_index_dependant_association :notes, on_change: :visibility_level
 
       scope :with_shared_runners_limit_enabled, -> do
         if ::Ci::Runner.has_shared_runners_with_non_zero_public_cost?

ee/app/models/ee/project_feature.rb

@@ -5,6 +5,12 @@ module EE
     extend ActiveSupport::Concern
 
     EE_FEATURES = %i(requirements).freeze
+    NOTES_PERMISSION_TRACKED_FIELDS = %w(
+      issues_access_level
+      repository_access_level
+      merge_requests_access_level
+      snippets_access_level
+    ).freeze
 
     prepended do
       set_available_features(EE_FEATURES)
@@ -14,7 +20,11 @@ module EE
         if project.maintaining_elasticsearch?
           project.maintain_elasticsearch_update
 
-          ElasticAssociationIndexerWorker.perform_async(self.project.class.name, project_id, ['issues']) if elasticsearch_project_associations_need_updating?
+          associations_to_update = []
+          associations_to_update << 'issues' if elasticsearch_project_issues_need_updating?
+          associations_to_update << 'notes' if elasticsearch_project_notes_need_updating?
+
+          ElasticAssociationIndexerWorker.perform_async(self.project.class.name, project_id, associations_to_update) if associations_to_update.any?
         end
       end
 
@@ -22,7 +32,11 @@ module EE
 
       private
 
-      def elasticsearch_project_associations_need_updating?
+      def elasticsearch_project_notes_need_updating?
+        self.previous_changes.keys.any? { |key| NOTES_PERMISSION_TRACKED_FIELDS.include?(key) }
+      end
+
+      def elasticsearch_project_issues_need_updating?
         self.previous_changes.key?(:issues_access_level)
       end
     end

ee/lib/elastic/instance_proxy_util.rb

@@ -54,6 +54,22 @@ module Elastic
       nil
     end
 
+    # protect against missing project_feature and set visibility to PRIVATE
+    # if the project_feature is missing on a project
+    def safely_read_project_feature_for_elasticsearch(feature)
+      if target.project.project_feature
+        target.project.project_feature.access_level(feature)
+      else
+        logger.warn(
+          message: 'Project is missing ProjectFeature',
+          project_id: target.project_id,
+          id: target.id,
+          class: target.class
+        )
+        ProjectFeature::PRIVATE
+      end
+    end
+
     def apply_field_limit(result)
       return result unless result.is_a? String
 

ee/lib/elastic/latest/issue_instance_proxy.rb

@@ -14,15 +14,7 @@ module Elastic
 
         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
-        begin
-          data['issues_access_level'] = target.project.project_feature.issues_access_level
-        rescue NoMethodError => e
-          Gitlab::ErrorTracking.track_exception(e, project_id: target.project_id, issue_id: target.id)
-          data['issues_access_level'] = ProjectFeature::PRIVATE
-        end
+        data['issues_access_level'] = safely_read_project_feature_for_elasticsearch(:issues)
 
         data.merge(generic_attributes)
       end

ee/lib/elastic/latest/note_instance_proxy.rb

@@ -22,8 +22,35 @@ module Elastic
           }
         end
 
+        # do not add the permission fields unless the `remove_permissions_data_from_notes_documents`
+        # migration has completed otherwise the migration will never finish
+        if 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)
+        end
+
         data.merge(generic_attributes)
       end
+
+      private
+
+      def merge_project_feature_access_level(data, noteable)
+        return unless noteable
+
+        case noteable
+        when Snippet
+          data['snippets_access_level'] = safely_read_project_feature_for_elasticsearch(:snippets)
+        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)
+        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
+          # data for them until the search capability is implemented
+        end
+      end
     end
   end
 end

ee/spec/elastic/migrate/20210127154600_remove_permissions_data_from_notes_documents_spec.rb

@@ -46,6 +46,13 @@ RSpec.describe RemovePermissionsDataFromNotesDocuments, :elastic, :sidekiq_inlin
     context 'migration process' do
       before do
         add_permission_data_for_notes([note_on_commit, note_on_issue, note_on_merge_request, note_on_snippet])
+
+        allow(Elastic::DataMigrationService).to receive(:migration_has_finished?).and_call_original
+        # migrations are completed by default in test environments
+        # required to prevent the `as_indexed_json` method from populating the permissions fields
+        allow(Elastic::DataMigrationService).to receive(:migration_has_finished?)
+                                                  .with(:remove_permissions_data_from_notes_documents)
+                                                  .and_return(false)
       end
 
       it 'queues documents for indexing' do

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

@@ -132,12 +132,11 @@ RSpec.describe Issue, :elastic do
     expect(issue.__elasticsearch__.as_indexed_json).to eq(expected_hash)
   end
 
-  it 'handles a project missing project_feature' do
+  it 'handles a project missing project_feature', :aggregate_failures do
     issue = create :issue, project: project
     allow(issue.project).to receive(:project_feature).and_return(nil)
 
-    expect(Gitlab::ErrorTracking).to receive(:track_exception)
-
+    expect { issue.__elasticsearch__.as_indexed_json }.not_to raise_error
     expect(issue.__elasticsearch__.as_indexed_json['issues_access_level']).to eq(ProjectFeature::PRIVATE)
   end
 

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

@@ -85,34 +85,102 @@ RSpec.describe Note, :elastic do
     expect(described_class.elastic_search('term', options: options).total_count).to eq(4)
   end
 
-  it "returns json with all needed elements" do
-    assignee = create(:user)
-    issue = create(:issue, assignees: [assignee])
-    note = create(:note, noteable: issue, project: issue.project)
-
-    expected_hash = note.attributes.extract!(
-      'id',
-      'note',
-      'project_id',
-      'noteable_type',
-      'noteable_id',
-      'created_at',
-      'updated_at',
-      'confidential'
-    ).merge({
-      'issue' => {
-        'assignee_id' => issue.assignee_ids,
-        'author_id' => issue.author_id,
-        'confidential' => issue.confidential
-      },
-      'type' => note.es_type,
-      'join_field' => {
-        'name' => note.es_type,
-        'parent' => note.es_parent
-      }
-    })
-
-    expect(note.__elasticsearch__.as_indexed_json).to eq(expected_hash)
+  describe 'json serialization' do
+    using RSpec::Parameterized::TableSyntax
+
+    it "returns json with all needed elements" do
+      assignee = create(:user)
+      project = create(:project)
+      issue = create(:issue, project: project, assignees: [assignee])
+      note = create(:note, noteable: issue, project: project)
+
+      expected_hash = note.attributes.extract!(
+        'id',
+        'note',
+        'project_id',
+        'noteable_type',
+        'noteable_id',
+        'created_at',
+        'updated_at',
+        'confidential'
+      ).merge({
+                'issue' => {
+                  'assignee_id' => issue.assignee_ids,
+                  'author_id' => issue.author_id,
+                  'confidential' => issue.confidential
+                },
+                'type' => note.es_type,
+                'join_field' => {
+                  'name' => note.es_type,
+                  'parent' => note.es_parent
+                },
+                'visibility_level' => project.visibility_level,
+                'issues_access_level' => project.issues_access_level
+              })
+
+      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)
+      allow(note).to receive(:noteable).and_return(nil)
+
+      expect { note.__elasticsearch__.as_indexed_json }.not_to raise_error
+    end
+
+    where(:note_type, :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                   | ProjectFeature::DISABLED     | 'snippets_access_level'
+      :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
+      :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                               | ProjectFeature::ENABLED      | false
+      :note_on_vulnerability                      | ProjectFeature::PRIVATE      | false
+      :discussion_note_on_vulnerability           | ProjectFeature::PRIVATE      | false
+      :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        | ProjectFeature::DISABLED     | 'snippets_access_level'
+      :note_on_merge_request                      | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
+      :discussion_note_on_commit                  | ProjectFeature::PRIVATE      | 'repository_access_level'
+      :track_mr_picking_note                      | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
+    end
+
+    with_them do
+      let_it_be(:project) { create(:project, :repository) }
+      let!(:note) { create(note_type, project: project) } # rubocop:disable Rails/SaveBang
+      let(:note_json) { note.__elasticsearch__.as_indexed_json }
+
+      before do
+        project.project_feature.update_attribute(access_level.to_sym, permission) if access_level.present?
+      end
+
+      it 'does not contain permissions if remove_permissions_data_from_notes_documents is not finished' do
+        allow(Elastic::DataMigrationService).to receive(:migration_has_finished?)
+                                                  .with(:remove_permissions_data_from_notes_documents)
+                                                  .and_return(false)
+
+        expect(note_json).not_to have_key(access_level) if access_level.present?
+        expect(note_json).not_to have_key('visibility_level')
+      end
+
+      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(permission)
+        end
+
+        expect(note_json).to have_key('visibility_level')
+        expect(note_json['visibility_level']).to eq(project.visibility_level)
+      end
+    end
   end
 
   it 'does not track system note updates' do

ee/spec/models/project_feature_spec.rb

@@ -28,13 +28,16 @@ RSpec.describe ProjectFeature 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
+    where(:feature, :worker_expected, :associations) do
+      'issues'                      | true        | %w[issues notes]
+      'wiki'                        | false       | nil
+      'builds'                      | false       | nil
+      'merge_requests'              | true        | %w[notes]
+      'repository'                  | true        | %w[notes]
+      'snippets'                    | true        | %w[notes]
+      'operations'                  | false       | nil
+      'security_and_compliance'     | false       | nil
+      'pages'                       | false       | nil
     end
 
     with_them do
@@ -42,12 +45,12 @@ RSpec.describe ProjectFeature do
         expect(project).to receive(:maintain_elasticsearch_update)
 
         if worker_expected
-          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, ['issues'])
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, associations)
         else
           expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async)
         end
 
-        project.project_feature.update_attribute("#{feature}_access_level".to_sym, ProjectFeature::PRIVATE)
+        project.project_feature.update_attribute("#{feature}_access_level".to_sym, ProjectFeature::DISABLED)
       end
     end
   end

ee/spec/models/project_spec.rb

@@ -2707,8 +2707,8 @@ RSpec.describe Project do
       let!(:issue) { create(:issue, project: project) }
 
       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'])
+        it 'triggers ElasticAssociationIndexerWorker to update issues and notes' do
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, %w[issues notes])
 
           project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
         end

ee/spec/services/groups/transfer_service_spec.rb

@@ -67,11 +67,11 @@ RSpec.describe Groups::TransferService, '#execute' do
 
           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, ['issues'])
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project1.id, %w[issues notes])
           expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project2)
-          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project2.id, ['issues'])
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project2.id, %w[issues notes])
           expect(Elastic::ProcessBookkeepingService).not_to receive(:track!).with(project3)
-          expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async).with('Project', project3.id, ['issues'])
+          expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async).with('Project', project3.id, %w[issues notes])
 
           transfer_service.execute(new_group)
 

ee/spec/services/projects/transfer_service_spec.rb

@@ -72,9 +72,9 @@ RSpec.describe Projects::TransferService do
     context 'when visibility level changes' do
       let_it_be(:group) { create(:group, :private) }
 
-      it 'reindexes the project and associated issues' do
+      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, ['issues'])
+        expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, %w[issues notes])
 
         subject.execute(group)
       end