Merge branch 'revert-d25b4bf4' into 'master'

Revert 53415 Add permission for indexed notes and update when permissions change

See merge request gitlab-org/gitlab!54417

ee/app/models/ee/project.rb

@@ -108,7 +108,6 @@ 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,7 +5,6 @@ module EE
     extend ActiveSupport::Concern
 
     EE_FEATURES = %i(requirements security_and_compliance).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)
@@ -15,11 +14,7 @@ module EE
         if project.maintaining_elasticsearch?
           project.maintain_elasticsearch_update
 
-          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?
+          ElasticAssociationIndexerWorker.perform_async(self.project.class.name, project_id, ['issues']) if elasticsearch_project_associations_need_updating?
         end
       end
 
@@ -28,11 +23,7 @@ module EE
 
       private
 
-      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?
+      def elasticsearch_project_associations_need_updating?
         self.previous_changes.key?(:issues_access_level)
       end
     end

ee/lib/elastic/instance_proxy_util.rb

@@ -54,22 +54,6 @@ 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,7 +14,15 @@ module Elastic
 
         data['assignee_id'] = safely_read_attribute_for_elasticsearch(:assignee_ids)
         data['visibility_level'] = target.project.visibility_level
-        data['issues_access_level'] = safely_read_project_feature_for_elasticsearch(:issues)
+
+        # 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.merge(generic_attributes)
       end

ee/lib/elastic/latest/note_instance_proxy.rb

@@ -22,31 +22,8 @@ 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)
-        else
-          access_level_attribute = ProjectFeature.access_level_attribute(noteable)
-          data[access_level_attribute.to_s] = safely_read_project_feature_for_elasticsearch(noteable)
-        end
-      end
     end
   end
 end

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

@@ -11,13 +11,6 @@ RSpec.describe RemovePermissionsDataFromNotesDocuments, :elastic, :sidekiq_inlin
   before do
     stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
     allow(migration).to receive(:helper).and_return(helper)
-
-    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
 
   describe 'migration_options' do

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

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

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

@@ -85,80 +85,34 @@ RSpec.describe Note, :elastic do
     expect(described_class.elastic_search('term', options: options).total_count).to eq(4)
   end
 
-  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_merge_request              | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
-      :note_on_commit                     | ProjectFeature::PRIVATE      | 'repository_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)
-      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)
-        expect(note_json).not_to have_key('visibility_level')
-      end
-
-      it 'contains the correct permissions', :aggregate_failures do
-        expect(note_json).to have_key(access_level)
-        expect(note_json[access_level]).to eq(permission)
-        expect(note_json).to have_key('visibility_level')
-      end
-    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)
   end
 
   it 'does not track system note updates' do

ee/spec/models/project_feature_spec.rb

@@ -28,14 +28,13 @@ RSpec.describe ProjectFeature do
       allow(project).to receive(:maintaining_elasticsearch?).and_return(true)
     end
 
-    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]
-      'pages'           | false       | nil
+    where(:feature, :worker_expected) do
+      'issues'          | true
+      'wiki'            | false
+      'builds'          | false
+      'merge_requests'  | false
+      'repository'      | false
+      'pages'           | false
     end
 
     with_them do
@@ -43,7 +42,7 @@ RSpec.describe ProjectFeature do
         expect(project).to receive(:maintain_elasticsearch_update)
 
         if worker_expected
-          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, associations)
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, ['issues'])
         else
           expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async)
         end

ee/spec/models/project_spec.rb

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

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, %w[issues notes])
+          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, %w[issues notes])
+          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, %w[issues notes])
+          expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async).with('Project', project3.id, ['issues'])
 
           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 and notes' do
+      it 'reindexes the project and associated issues' do
         expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project)
-        expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, %w[issues notes])
+        expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, ['issues'])
 
         subject.execute(group)
       end