Skip to content

G
gitlab-ce
Commit 2f543705 authored by Mayra Cabrera's avatar Mayra Cabrera
Browse files
Options

Merge branch 'revert-5555428e' into 'master' 

Revert "Denormalize notes permission data & update when permissions change"

See merge request gitlab-org/gitlab!53388
parents 88359376 fd68a8df
Hide whitespace changes
Inline Side-by-side
Showing with 34 additions and 351 deletions
ee/app/models/ee/project.rb
View file @ 2f543705
......@@ -105,7 +105,6 @@ module EE
has_many :incident_management_oncall_schedules, class_name: 'IncidentManagement::OncallSchedule', 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
View file @ 2f543705
......@@ -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/elastic/migrate/20210128163600_add_permissions_data_to_notes_documents.rb deleted 100644 → 0
View file @ 88359376
# frozen_string_literal: true
class AddPermissionsDataToNotesDocuments < Elastic::Migration
batched!
throttle_delay 1.minute
QUERY_BATCH_SIZE = 5000
UPDATE_BATCH_SIZE = 100
def migrate
if completed?
log "Skipping adding permission data to notes documents migration since it is already applied"
return
end
log "Adding permission data to notes documents for batch of #{QUERY_BATCH_SIZE} documents"
# use filter query to prevent scores from being calculated
query = {
size: QUERY_BATCH_SIZE,
query: {
bool: {
filter: {
bool: {
must: note_type_filter,
should: [
field_does_not_exist('visibility_level'),
field_does_not_exist_for_type('issues_access_level', 'Issue'),
field_does_not_exist_for_type('repository_access_level', 'Commit'),
field_does_not_exist_for_type('merge_requests_access_level', 'MergeRequest'),
field_does_not_exist_for_type('snippets_access_level', 'Snippet')
],
minimum_should_match: 1
}
}
}
}
}
results = client.search(index: helper.target_index_name, body: query)
hits = results.dig('hits', 'hits') || []
document_references = hits.map do |hit|
id = hit.dig('_source', 'id')
es_id = hit.dig('_id')
es_parent = hit.dig('_source', 'join_field', 'parent')
# ensure that any notes missing from the database will be removed from Elasticsearch
# as the data is back-filled
Gitlab::Elastic::DocumentReference.new(Note, id, es_id, es_parent)
end
document_references.each_slice(UPDATE_BATCH_SIZE) do |refs|
Elastic::ProcessBookkeepingService.track!(*refs)
end
log "Adding permission data to notes documents is completed for batch of #{document_references.size} documents"
end
def completed?
log "completed check: Refreshing #{helper.target_index_name}"
helper.refresh_index(index_name: helper.target_index_name)
query = {
size: 0,
query: note_type_filter,
aggs: {
notes: {
filter: {
bool: {
should: [
field_does_not_exist('visibility_level'),
field_does_not_exist_for_type('issues_access_level', 'Issue'),
field_does_not_exist_for_type('repository_access_level', 'Commit'),
field_does_not_exist_for_type('merge_requests_access_level', 'MergeRequest'),
field_does_not_exist_for_type('snippets_access_level', 'Snippet')
],
minimum_should_match: 1
}
}
}
}
}
results = client.search(index: helper.target_index_name, body: query)
doc_count = results.dig('aggregations', 'notes', 'doc_count')
log "Migration has #{doc_count} documents remaining" if doc_count
doc_count && doc_count == 0
end
private
def note_type_filter
{
term: {
type: {
value: 'note'
}
}
}
end
def field_does_not_exist(field)
{
bool: {
must_not: {
exists: {
field: field
}
}
}
}
end
def field_does_not_exist_for_type(field, type)
query = field_does_not_exist(field)
query[:bool][:must] = {
term: {
noteable_type: {
value: type
}
}
}
query
end
end
ee/lib/elastic/instance_proxy_util.rb
View file @ 2f543705
......@@ -54,15 +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)
target.project.project_feature.issues_access_level
rescue NoMethodError => e
Gitlab::ErrorTracking.track_exception(e, project_id: target.project_id, id: target.id, class: target.class)
ProjectFeature::PRIVATE
end
def apply_field_limit(result)
return result unless result.is_a? String
......
ee/lib/elastic/latest/issue_instance_proxy.rb
View file @ 2f543705
......@@ -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_access_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.merge(generic_attributes)
end
......
ee/lib/elastic/latest/note_instance_proxy.rb
View file @ 2f543705
......@@ -14,23 +14,14 @@ module Elastic
data[attr.to_s] = safely_read_attribute_for_elasticsearch(attr)
end
if for_issue?
if noteable.is_a?(Issue)
data['issue'] = {
'assignee_id' => noteable.assignee_ids,
'author_id' => noteable.author_id,
'confidential' => noteable.confidential
}
data['issues_access_level'] = safely_read_project_feature_for_elasticsearch(:issues_access_level)
elsif for_snippet?
data['snippets_access_level'] = safely_read_project_feature_for_elasticsearch(:snippets_access_level)
elsif for_merge_request?
data['merge_requests_access_level'] = safely_read_project_feature_for_elasticsearch(:merge_requests_access_level)
elsif for_commit?
data['repository_access_level'] = safely_read_project_feature_for_elasticsearch(:repository_access_level)
end
data['visibility_level'] = target.project.visibility_level
data.merge(generic_attributes)
end
end
......
ee/spec/elastic/migrate/202101281636000_add_permissions_data_to_notes_documents_spec.rb deleted 100644 → 0
View file @ 88359376
# frozen_string_literal: true
require 'spec_helper'
require File.expand_path('ee/elastic/migrate/20210128163600_add_permissions_data_to_notes_documents.rb')
RSpec.describe AddPermissionsDataToNotesDocuments, :elastic, :sidekiq_inline do
let(:version) { 20210128163600 }
let(:migration) { described_class.new(version) }
let(:helper) { Gitlab::Elastic::Helper.new }
before do
stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
allow(migration).to receive(:helper).and_return(helper)
end
describe 'migration_options' do
it 'has migration options set', :aggregate_failures do
expect(migration.batched?).to be_truthy
expect(migration.throttle_delay).to eq(1.minute)
end
end
describe '#migrate' do
let!(:note_on_commit) { create(:note_on_commit) }
let!(:note_on_issue) { create(:note_on_issue) }
let!(:note_on_merge_request) { create(:note_on_merge_request) }
let!(:note_on_snippet) { create(:note_on_project_snippet) }
before do
ensure_elasticsearch_index!
end
context 'when migration is completed' do
it 'does not execute delete_by_query' do
expect(migration.completed?).to be_truthy
expect(::Elastic::ProcessBookkeepingService).not_to receive(:track!)
migration.migrate
end
end
context 'migration process' do
before do
remove_permission_data_for_notes([note_on_commit, note_on_issue, note_on_merge_request, note_on_snippet])
end
it 'adds permission data to the index' do
# track calls are batched in groups of 100
expect(::Elastic::ProcessBookkeepingService).to receive(:track!).once do |*tracked_refs|
expect(tracked_refs.count).to eq(4)
end
migration.migrate
end
it 'only updates note documents missing data', :aggregate_failures do
add_permission_data_for_notes([note_on_issue, note_on_snippet, note_on_merge_request])
expected = [Gitlab::Elastic::DocumentReference.new(Note, note_on_commit.id, note_on_commit.es_id, note_on_commit.es_parent)]
expect(::Elastic::ProcessBookkeepingService).to receive(:track!).with(*expected).once
migration.migrate
end
it 'processes in batches', :aggregate_failures do
stub_const("#{described_class}::QUERY_BATCH_SIZE", 2)
stub_const("#{described_class}::UPDATE_BATCH_SIZE", 1)
expect(::Elastic::ProcessBookkeepingService).to receive(:track!).exactly(4).times.and_call_original
migration.migrate
ensure_elasticsearch_index!
migration.migrate
end
end
end
describe '#completed?' do
let!(:note_on_commit) { create(:note_on_commit) }
before do
ensure_elasticsearch_index!
end
subject { migration.completed? }
context 'when documents are missing permissions data' do
before do
remove_permission_data_for_notes([note_on_commit])
end
it { is_expected.to be_falsey }
end
context 'when no documents are missing permissions data' do
it { is_expected.to be_truthy }
end
it 'refreshes the index' do
expect(helper).to receive(:refresh_index)
subject
end
end
private
def add_permission_data_for_notes(notes)
script = {
source: "ctx._source['visibility_level'] = params.visibility_level; ctx._source['issues_access_level'] = params.visibility_level; ctx._source['merge_requests_access_level'] = params.visibility_level; ctx._source['snippets_access_level'] = params.visibility_level; ctx._source['repository_access_level'] = params.visibility_level;",
lang: "painless",
params: {
visibility_level: Gitlab::VisibilityLevel::PRIVATE
}
}
update_by_query(notes, script)
end
def remove_permission_data_for_notes(notes)
script = {
source: "ctx._source.remove('visibility_level'); ctx._source.remove('repository_access_level'); ctx._source.remove('snippets_access_level'); ctx._source.remove('merge_requests_access_level'); ctx._source.remove('issues_access_level');"
}
update_by_query(notes, script)
end
def update_by_query(notes, script)
note_ids = notes.map { |i| i.id }
client = Note.__elasticsearch__.client
client.update_by_query({
index: Note.__elasticsearch__.index_name,
wait_for_completion: true, # run synchronously
refresh: true, # make operation visible to search
body: {
script: script,
query: {
bool: {
must: [
{
terms: {
id: note_ids
}
},
{
term: {
type: {
value: 'note'
}
}
}
]
}
}
}
})
end
end
ee/spec/models/concerns/elastic/note_spec.rb
View file @ 2f543705
......@@ -88,12 +88,9 @@ RSpec.describe Note, :elastic do
it "returns json with all needed elements" do
assignee = create(:user)
issue = create(:issue, assignees: [assignee])
issue_note = create(:note, noteable: issue, project: issue.project)
commit_note = create(:note_on_commit, project: issue.project)
merge_request_note = create(:note_on_merge_request, project: issue.project)
snippet_note = create(:note_on_project_snippet, project: issue.project)
note = create(:note, noteable: issue, project: issue.project)
expected_hash = issue_note.attributes.extract!(
expected_hash = note.attributes.extract!(
'id',
'note',
'project_id',
......@@ -108,19 +105,14 @@ RSpec.describe Note, :elastic do
'author_id' => issue.author_id,
'confidential' => issue.confidential
},
'type' => issue_note.es_type,
'type' => note.es_type,
'join_field' => {
'name' => issue_note.es_type,
'parent' => issue_note.es_parent
},
'visibility_level' => issue.project.visibility_level,
'issues_access_level' => issue.project.issues_access_level
'name' => note.es_type,
'parent' => note.es_parent
}
})
expect(issue_note.__elasticsearch__.as_indexed_json).to eq(expected_hash)
expect(commit_note.__elasticsearch__.as_indexed_json).to have_key('repository_access_level')
expect(merge_request_note.__elasticsearch__.as_indexed_json).to have_key('merge_requests_access_level')
expect(snippet_note.__elasticsearch__.as_indexed_json).to have_key('snippets_access_level')
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
View file @ 2f543705
......@@ -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
View file @ 2f543705
......@@ -2638,8 +2638,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
View file @ 2f543705
......@@ -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
View file @ 2f543705
......@@ -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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7