Commit 98bc2786 authored by Terri Chu's avatar Terri Chu Committed by Dylan Griffith

Remove project joins from notes ES query

Use new denormalized project visibility and project
feature permissions in the notes documents if the
ES migration has finished. The ES query supports
both before and after migration.
parent 02e3a87f
...@@ -13,6 +13,7 @@ module Elastic ...@@ -13,6 +13,7 @@ module Elastic
options[:in] = ['note'] options[:in] = ['note']
query_hash = basic_query_hash(%w[note], query, count_only: options[:count_only]) query_hash = basic_query_hash(%w[note], query, count_only: options[:count_only])
options[:no_join_project] = Elastic::DataMigrationService.migration_has_finished?(:add_permissions_data_to_notes_documents)
context.name(:note) do context.name(:note) do
query_hash = context.name(:authorized) { project_ids_filter(query_hash, options) } query_hash = context.name(:authorized) { project_ids_filter(query_hash, options) }
query_hash = context.name(:confidentiality) { confidentiality_filter(query_hash, options) } query_hash = context.name(:confidentiality) { confidentiality_filter(query_hash, options) }
...@@ -100,6 +101,9 @@ module Elastic ...@@ -100,6 +101,9 @@ module Elastic
override :project_ids_filter override :project_ids_filter
def project_ids_filter(query_hash, options) def project_ids_filter(query_hash, options)
# support for not using project joins in the query
no_join_project = options[:no_join_project]
query_hash[:query][:bool][:filter] ||= [] query_hash[:query][:bool][:filter] ||= []
project_query = context.name(:project) do project_query = context.name(:project) do
...@@ -107,7 +111,8 @@ module Elastic ...@@ -107,7 +111,8 @@ module Elastic
options[:current_user], options[:current_user],
options[:project_ids], options[:project_ids],
options[:public_and_internal_projects], options[:public_and_internal_projects],
options[:features] options[:features],
no_join_project
) )
end end
...@@ -124,9 +129,17 @@ module Elastic ...@@ -124,9 +129,17 @@ module Elastic
project_query[:should].flatten.each do |condition| project_query[:should].flatten.each do |condition|
noteable_type = condition.delete(:noteable_type).to_s noteable_type = condition.delete(:noteable_type).to_s
filters[:bool][:should] << { should_filter = {
bool: { bool: {
must: [ must: [
{ term: { noteable_type: { _name: context.name(:noteable, :is_a, noteable_type), value: noteable_type } } }
]
}
}
should_filter[:bool][:must] << if no_join_project
condition
else
{ {
has_parent: { has_parent: {
parent_type: "project", parent_type: "project",
...@@ -136,13 +149,12 @@ module Elastic ...@@ -136,13 +149,12 @@ module Elastic
} }
} }
} }
},
{ term: { noteable_type: { _name: context.name(:noteable, :is_a, noteable_type), value: noteable_type } } }
]
}
} }
end end
filters[:bool][:should] << should_filter
end
query_hash[:query][:bool][:filter] << filters query_hash[:query][:bool][:filter] << filters
query_hash query_hash
end end
...@@ -150,18 +162,18 @@ module Elastic ...@@ -150,18 +162,18 @@ module Elastic
# Query notes based on the various feature permission of the noteable_type. # Query notes based on the various feature permission of the noteable_type.
# Appends `noteable_type` (which will be removed in project_ids_filter) # Appends `noteable_type` (which will be removed in project_ids_filter)
# for base model filtering. # for base model filtering.
# We do not implement `no_join_project` argument for notes class yet
# as this is not supported. This will need to be fixed when we move
# notes to a new index.
override :pick_projects_by_membership override :pick_projects_by_membership
def pick_projects_by_membership(project_ids, user, _, _ = nil) def pick_projects_by_membership(project_ids, user, no_join_project, _ = nil)
# support for not using project joins in the query
project_id_key = no_join_project ? :project_id : :id
noteable_type_to_feature.map do |noteable_type, feature| noteable_type_to_feature.map do |noteable_type, feature|
context.name(feature) do context.name(feature) do
condition = condition =
if project_ids == :any if project_ids == :any
{ term: { visibility_level: { _name: context.name(:any), value: Project::PRIVATE } } } { term: { visibility_level: { _name: context.name(:any), value: Project::PRIVATE } } }
else else
{ terms: { _name: context.name(:membership, :id), id: filter_ids_by_feature(project_ids, user, feature) } } { terms: { _name: context.name(:membership, :id), project_id_key => filter_ids_by_feature(project_ids, user, feature) } }
end end
limit = limit =
......
...@@ -32,7 +32,7 @@ RSpec.shared_examples 'search notes shared examples' do ...@@ -32,7 +32,7 @@ RSpec.shared_examples 'search notes shared examples' do
end end
context 'when user can read confidential notes' do context 'when user can read confidential notes' do
it 'does not filter confidental notes' do it 'does not filter confidential notes' do
noteable.project.add_reporter(user) noteable.project.add_reporter(user)
expect_search_results(user, 'notes', expected_objects: [not_confidential_note, nil_confidential_note, confidential_note]) do |user| expect_search_results(user, 'notes', expected_objects: [not_confidential_note, nil_confidential_note, confidential_note]) do |user|
......
# frozen_string_literal: true
RSpec.shared_examples 'search query applies joins based on migrations shared examples' do |migration_name|
context 'using joins for global permission checks', :elastic do
let(:es_host) { Gitlab::CurrentSettings.elasticsearch_url[0] }
let(:search_url) { Addressable::Template.new("#{es_host}/{index}/doc/_search{?params*}") }
context "when #{migration_name} migration is finished" do
before do
allow(Elastic::DataMigrationService).to receive(:migration_has_finished?)
.with(migration_name)
.and_return(true)
end
it 'does not use joins to apply permissions' do
request = a_request(:get, search_url).with do |req|
expect(req.body).not_to include("has_parent")
end
results
expect(request).to have_been_made
end
end
context "when #{migration_name} migration is not finished" do
before do
allow(Elastic::DataMigrationService).to receive(:migration_has_finished?)
.with(migration_name)
.and_return(false)
end
it 'uses joins to apply permissions' do
request = a_request(:get, search_url).with do |req|
expect(req.body).to include("has_parent")
end
results
expect(request).to have_been_made
end
end
end
end
...@@ -356,7 +356,7 @@ RSpec.shared_context 'ProjectPolicyTable context' do ...@@ -356,7 +356,7 @@ RSpec.shared_context 'ProjectPolicyTable context' do
:private | :anonymous | 0 :private | :anonymous | 0
end end
# :snippet_level, :project_level, :feature_access_level, :membership, :expected_count # :snippet_level, :project_level, :feature_access_level, :membership, :admin_mode, :expected_count
def permission_table_for_project_snippet_access def permission_table_for_project_snippet_access
:public | :public | :enabled | :admin | true | 1 :public | :public | :enabled | :admin | true | 1
:public | :public | :enabled | :admin | false | 1 :public | :public | :enabled | :admin | false | 1
......
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