Commit a5b8eff2 authored by Markus Koller's avatar Markus Koller

Support ES searches for project snippets

These were previously only accessible to admins, where we didn't filter
the ES results by their parent projects.

This adds the parent relation on project snippets, properly filters the
results for all the various cases, and aligns the ES snippet search
service and result classes to work more similarly to the other models.
parent b70abe63
# frozen_string_literal: true
module Search
class SnippetService
attr_accessor :current_user, :params
def initialize(user, params)
@current_user, @params = user, params.dup
end
class SnippetService < Search::GlobalService
def execute
Gitlab::SnippetSearchResults.new(current_user, params[:search])
end
......
---
title: Support ES searches for project snippets
merge_request: 18459
author:
type: fixed
......@@ -4,7 +4,7 @@ module Elastic
extend ActiveSupport::Concern
FORWARDABLE_INSTANCE_METHODS = [:es_id, :es_parent].freeze
FORWARDABLE_CLASS_METHODS = [:elastic_search, :es_import, :nested?, :es_type, :index_name, :document_type, :mapping, :mappings, :settings, :import].freeze
FORWARDABLE_CLASS_METHODS = [:elastic_search, :es_import, :es_type, :index_name, :document_type, :mapping, :mappings, :settings, :import].freeze
def __elasticsearch__(&block)
@__elasticsearch__ ||= ::Elastic::MultiVersionInstanceProxy.new(self)
......
......@@ -9,7 +9,7 @@ module EE
def execute
return super unless use_elasticsearch?
::Gitlab::Elastic::SnippetSearchResults.new(current_user, params[:search])
::Gitlab::Elastic::SnippetSearchResults.new(current_user, params[:search], elastic_projects, nil, true)
end
# This method is used in the top-level SearchService, so cannot be in-lined into #execute
......
......@@ -15,7 +15,7 @@ module Elastic
record.__elasticsearch__.client = client
import(record, record.class.nested?, indexing)
import(record, indexing)
initial_index_project(record) if record.class == Project && indexing
......@@ -65,12 +65,12 @@ module Elastic
raise ImportError.new(errors.inspect)
end
def import(record, nested, indexing)
def import(record, indexing)
operation = indexing ? 'index_document' : 'update_document'
response = nil
IMPORT_RETRY_COUNT.times do
response = if nested
response = if record.es_parent
record.__elasticsearch__.__send__ operation, routing: record.es_parent # rubocop:disable GitlabSecurity/PublicSend
else
record.__elasticsearch__.__send__ operation # rubocop:disable GitlabSecurity/PublicSend
......
......@@ -18,12 +18,12 @@ class ElasticIndexerWorker
options
)
when /delete/
if klass.nested?
if options['es_parent']
client.delete(
index: klass.index_name,
type: klass.document_type,
id: es_id,
routing: options["es_parent"]
routing: options['es_parent']
)
else
clear_project_data(record_id, es_id) if klass == Project
......
......@@ -5,11 +5,6 @@ module Elastic
class ApplicationClassProxy < Elasticsearch::Model::Proxy::ClassMethodsProxy
include ClassProxyUtil
# Should be overridden for all nested models
def nested?
false
end
def es_type
target.name.underscore
end
......
......@@ -20,13 +20,16 @@ module Elastic
private
def generic_attributes
{
'join_field' => {
attributes = { 'type' => es_type }
if es_parent
attributes['join_field'] = {
'name' => es_type,
'parent' => es_parent
},
'type' => es_type
}
}
end
attributes
end
end
end
......
......@@ -66,6 +66,7 @@ module Elastic
blob
wiki_blob
commit
snippet
)
}
# ES6 requires a single type per index, so we implement our own "type"
......
......@@ -3,10 +3,6 @@
module Elastic
module Latest
class IssueClassProxy < ApplicationClassProxy
def nested?
true
end
def elastic_search(query, options: {})
query_hash =
if query =~ /#(\d+)\z/
......
......@@ -3,10 +3,6 @@
module Elastic
module Latest
class MergeRequestClassProxy < ApplicationClassProxy
def nested?
true
end
def elastic_search(query, options: {})
query_hash =
if query =~ /\!(\d+)\z/
......
......@@ -3,10 +3,6 @@
module Elastic
module Latest
class MilestoneClassProxy < ApplicationClassProxy
def nested?
true
end
def elastic_search(query, options: {})
options[:in] = %w(title^2 description)
......
......@@ -7,10 +7,6 @@ module Elastic
'note'
end
def nested?
true
end
def elastic_search(query, options: {})
options[:in] = ['note']
......
......@@ -16,9 +16,9 @@ module Elastic
if noteable.is_a?(Issue)
data['issue'] = {
assignee_id: noteable.assignee_ids,
author_id: noteable.author_id,
confidential: noteable.confidential
'assignee_id' => noteable.assignee_ids,
'author_id' => noteable.author_id,
'confidential' => noteable.confidential
}
end
......
......@@ -5,14 +5,14 @@ module Elastic
class SnippetClassProxy < ApplicationClassProxy
def elastic_search(query, options: {})
query_hash = basic_query_hash(%w(title file_name), query)
query_hash = filter(query_hash, options[:user])
query_hash = filter(query_hash, options)
search(query_hash)
end
def elastic_search_code(query, options: {})
query_hash = basic_query_hash(%w(content), query)
query_hash = filter(query_hash, options[:user])
query_hash = filter(query_hash, options)
search(query_hash)
end
......@@ -23,50 +23,91 @@ module Elastic
private
def filter(query_hash, user)
return query_hash if user && user.full_private_access?
filter =
if user
{
bool: {
should: [
{ term: { author_id: user.id } },
{ terms: { project_id: authorized_project_ids_for_user(user) } },
{
bool: {
filter: [
{ terms: { visibility_level: [Snippet::PUBLIC, Snippet::INTERNAL] } },
{ term: { type: self.es_type } }
],
must_not: { exists: { field: 'project_id' } }
}
}
]
}
}
else
{
bool: {
filter: [
{ term: { visibility_level: Snippet::PUBLIC } },
{ term: { type: self.es_type } }
],
must_not: { exists: { field: 'project_id' } }
}
}
end
def filter(query_hash, options)
user = options[:current_user]
return query_hash if user&.full_private_access?
filter_conditions =
filter_personal_snippets(user, options) +
filter_project_snippets(user, options)
# Match any of the filter conditions, in addition to the existing conditions
query_hash[:query][:bool][:filter] << {
bool: {
should: filter_conditions
}
}
query_hash[:query][:bool][:filter] = filter
query_hash
end
def authorized_project_ids_for_user(user)
if Ability.allowed?(user, :read_cross_project)
user.authorized_projects.pluck_primary_key
else
[]
def filter_personal_snippets(user, options)
filter_conditions = []
# Include accessible personal snippets
filter_conditions << {
bool: {
filter: [
{ terms: { visibility_level: Gitlab::VisibilityLevel.levels_for_user(user) } }
],
must_not: { exists: { field: 'project_id' } }
}
}
# Include authored personal snippets
if user
filter_conditions << {
bool: {
filter: [
{ term: { author_id: user.id } }
],
must_not: { exists: { field: 'project_id' } }
}
}
end
filter_conditions
end
def filter_project_snippets(user, options)
return [] unless Ability.allowed?(user, :read_cross_project)
filter_conditions = []
# Include public/internal project snippets for accessible projects
filter_conditions << {
bool: {
filter: [
{ terms: { visibility_level: Gitlab::VisibilityLevel.levels_for_user(user) } },
{
has_parent: {
parent_type: 'project',
query: {
bool: project_ids_query(
user,
options[:project_ids],
options[:public_and_internal_projects],
'snippets'
)
}
}
}
]
}
}
# Include all project snippets for authorized projects
if user
filter_conditions << {
bool: {
must: [
{ terms: { project_id: user.authorized_projects(Gitlab::Access::GUEST).pluck_primary_key } }
]
}
}
end
filter_conditions
end
end
end
......
......@@ -28,10 +28,7 @@ module Elastic
data['content'] = data['content'].mb_chars.limit(MAX_INDEX_SIZE).to_s # rubocop: disable CodeReuse/ActiveRecord
end
# ES6 is now single-type per index, so we implement our own typing
data['type'] = es_type
data
data.merge(generic_attributes)
end
end
end
......
......@@ -2,7 +2,18 @@
module Gitlab
module Elastic
class SnippetSearchResults < ::Gitlab::SnippetSearchResults
class SnippetSearchResults < Gitlab::Elastic::SearchResults
def objects(scope, page = 1)
page = (page || 1).to_i
case scope
when 'snippet_titles'
eager_load(snippet_titles, page, eager: { project: [:route, :namespace] })
when 'snippet_blobs'
eager_load(snippet_blobs, page, eager: { project: [:route, :namespace] })
end
end
def formatted_count(scope)
case scope
when 'snippet_titles'
......@@ -25,11 +36,11 @@ module Gitlab
private
def snippet_titles
Snippet.elastic_search(query, options: search_params)
Snippet.elastic_search(query, options: base_options)
end
def snippet_blobs
Snippet.elastic_search_code(query, options: search_params)
Snippet.elastic_search_code(query, options: base_options)
end
def limited_snippet_titles_count
......@@ -43,10 +54,6 @@ module Gitlab
def paginated_objects(relation, page)
super.records
end
def search_params
{ user: current_user }
end
end
end
end
......@@ -3,33 +3,111 @@
require 'spec_helper'
describe 'Snippet elastic search', :js, :elastic do
let(:user) { create(:user) }
let(:project) { create(:project, namespace: user.namespace) }
let(:public_project) { create(:project, :public) }
let(:authorized_user) { create(:user) }
let(:authorized_project) { create(:project, namespace: authorized_user.namespace) }
before do
stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
project.add_maintainer(user)
sign_in(user)
authorized_project.add_maintainer(authorized_user)
create(:personal_snippet, :public, content: 'public personal snippet')
create(:project_snippet, :public, content: 'public project snippet', project: public_project)
create(:personal_snippet, :internal, content: 'internal personal snippet')
create(:project_snippet, :internal, content: 'internal project snippet', project: public_project)
create(:personal_snippet, :private, content: 'private personal snippet')
create(:project_snippet, :private, content: 'private project snippet', project: public_project)
create(:personal_snippet, :private, content: 'authorized personal snippet', author: authorized_user)
create(:project_snippet, :private, content: 'authorized project snippet', project: authorized_project)
Gitlab::Elastic::Helper.refresh_index
sign_in(current_user) if current_user
visit explore_snippets_path
submit_search('snippet')
end
describe 'searching' do
it 'finds a personal snippet' do
create(:personal_snippet, author: user, content: 'Test searching for personal snippets')
context 'as anonymous user' do
let(:current_user) { nil }
it 'finds only public snippets' do
within('.results') do
expect(page).to have_content('public personal snippet')
expect(page).to have_content('public project snippet')
visit explore_snippets_path
submit_search('Test')
expect(page).not_to have_content('internal personal snippet')
expect(page).not_to have_content('internal project snippet')
expect(page).to have_selector('.results', text: 'Test searching for personal snippets')
expect(page).not_to have_content('authorized personal snippet')
expect(page).not_to have_content('authorized project snippet')
expect(page).not_to have_content('private personal snippet')
expect(page).not_to have_content('private project snippet')
end
end
end
context 'as logged in user' do
let(:current_user) { create(:user) }
it 'finds only public and internal snippets' do
within('.results') do
expect(page).to have_content('public personal snippet')
expect(page).to have_content('public project snippet')
expect(page).to have_content('internal personal snippet')
expect(page).to have_content('internal project snippet')
expect(page).not_to have_content('private personal snippet')
expect(page).not_to have_content('private project snippet')
expect(page).not_to have_content('authorized personal snippet')
expect(page).not_to have_content('authorized project snippet')
end
end
end
context 'as authorized user' do
let(:current_user) { authorized_user }
it 'finds only public, internal, and authorized private snippets' do
within('.results') do
expect(page).to have_content('public personal snippet')
expect(page).to have_content('public project snippet')
expect(page).to have_content('internal personal snippet')
expect(page).to have_content('internal project snippet')
expect(page).not_to have_content('private personal snippet')
expect(page).not_to have_content('private project snippet')
expect(page).to have_content('authorized personal snippet')
expect(page).to have_content('authorized project snippet')
end
end
end
context 'as administrator' do
let(:current_user) { create(:admin) }
it 'finds all snippets' do
within('.results') do
expect(page).to have_content('public personal snippet')
expect(page).to have_content('public project snippet')
it 'finds a project snippet' do
create(:project_snippet, project: project, content: 'Test searching for personal snippets')
expect(page).to have_content('internal personal snippet')
expect(page).to have_content('internal project snippet')
visit explore_snippets_path
submit_search('Test')
expect(page).to have_content('private personal snippet')
expect(page).to have_content('private project snippet')
expect(page).to have_selector('.results', text: 'Test searching for personal snippets')
expect(page).to have_content('authorized personal snippet')
expect(page).to have_content('authorized project snippet')
end
end
end
end
......@@ -4,7 +4,7 @@ require 'spec_helper'
describe Gitlab::Elastic::SnippetSearchResults, :elastic do
let(:snippet) { create(:personal_snippet, content: 'foo', file_name: 'foo') }
let(:results) { described_class.new(snippet.author, 'foo') }
let(:results) { described_class.new(snippet.author, 'foo', []) }
before do
stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
......@@ -26,7 +26,7 @@ describe Gitlab::Elastic::SnippetSearchResults, :elastic do
end
context 'when user is not author' do
let(:results) { described_class.new(create(:user), 'foo') }
let(:results) { described_class.new(create(:user), 'foo', []) }
it 'returns nothing' do
expect(results.snippet_titles_count).to eq(0)
......@@ -35,7 +35,7 @@ describe Gitlab::Elastic::SnippetSearchResults, :elastic do
end
context 'when user is nil' do
let(:results) { described_class.new(nil, 'foo') }
let(:results) { described_class.new(nil, 'foo', []) }
it 'returns nothing' do
expect(results.snippet_titles_count).to eq(0)
......@@ -54,7 +54,7 @@ describe Gitlab::Elastic::SnippetSearchResults, :elastic do
context 'when user has full_private_access' do
let(:user) { create(:admin) }
let(:results) { described_class.new(user, 'foo') }
let(:results) { described_class.new(user, 'foo', :any) }
it 'returns matched snippets' do
expect(results.snippet_titles_count).to eq(1)
......@@ -67,8 +67,8 @@ describe Gitlab::Elastic::SnippetSearchResults, :elastic do
let(:snippet) { create(:personal_snippet, :public, content: content) }
it 'indexes up to a limit' do
expect(described_class.new(nil, 'abc').snippet_blobs_count).to eq(1)
expect(described_class.new(nil, 'xyz').snippet_blobs_count).to eq(0)
expect(described_class.new(nil, 'abc', []).snippet_blobs_count).to eq(1)
expect(described_class.new(nil, 'xyz', []).snippet_blobs_count).to eq(0)
end
end
end
......@@ -101,16 +101,24 @@ describe Issue, :elastic do
assignee = create(:user)
issue = create :issue, project: project, assignees: [assignee]
expected_hash = issue.attributes.extract!('id', 'iid', 'title', 'description', 'created_at',
'updated_at', 'state', 'project_id', 'author_id',
'confidential')
.merge({
'join_field' => {
'name' => issue.es_type,
'parent' => issue.es_parent
},
'type' => issue.es_type
})
expected_hash = issue.attributes.extract!(
'id',
'iid',
'title',
'description',
'created_at',
'updated_at',
'state',
'project_id',
'author_id',
'confidential'
).merge({
'type' => issue.es_type,
'join_field' => {
'name' => issue.es_type,
'parent' => issue.es_parent
}
})
expected_hash['assignee_id'] = [assignee.id]
......
......@@ -79,12 +79,12 @@ describe MergeRequest, :elastic do
'target_project_id',
'author_id'
).merge({
'join_field' => {
'name' => merge_request.es_type,
'parent' => merge_request.es_parent
},
'type' => merge_request.es_type
})
'type' => merge_request.es_type,
'join_field' => {
'name' => merge_request.es_type,
'parent' => merge_request.es_parent
}
})
expect(merge_request.__elasticsearch__.as_indexed_json).to eq(expected_hash)
end
......
......@@ -48,11 +48,11 @@ describe Milestone, :elastic do
'created_at',
'updated_at'
).merge({
'type' => milestone.es_type,
'join_field' => {
'name' => milestone.es_type,
'parent' => milestone.es_parent
},
'type' => milestone.es_type
}
})
expect(milestone.__elasticsearch__.as_indexed_json).to eq(expected_hash)
......
......@@ -75,22 +75,32 @@ describe Note, :elastic do
end
it "returns json with all needed elements" do
note = create :note
expected_hash_keys = %w(
id
note
project_id
noteable_type
noteable_id
created_at
updated_at
issue
join_field
type
)
expect(note.__elasticsearch__.as_indexed_json.keys).to eq(expected_hash_keys)
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'
).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 create ElasticIndexerWorker job for system messages" do
......
......@@ -34,7 +34,7 @@ describe Snippet, :elastic do
end
it 'returns only public snippets when user is blank' do
result = described_class.elastic_search_code('password', options: { user: nil })
result = described_class.elastic_search_code('password', options: { current_user: nil })
expect(result.total_count).to eq(1)
expect(result.records).to match_array [public_snippet]
......@@ -43,7 +43,7 @@ describe Snippet, :elastic do
it 'returns only public and internal personal snippets for non-members' do
non_member = create(:user)
result = described_class.elastic_search_code('password', options: { user: non_member })
result = described_class.elastic_search_code('password', options: { current_user: non_member })
expect(result.total_count).to eq(2)
expect(result.records).to match_array [public_snippet, internal_snippet]
......@@ -53,14 +53,14 @@ describe Snippet, :elastic do
member = create(:user)
project.add_developer(member)
result = described_class.elastic_search_code('password', options: { user: member })
result = described_class.elastic_search_code('password', options: { current_user: member })
expect(result.total_count).to eq(5)
expect(result.records).to match_array [public_snippet, internal_snippet, project_public_snippet, project_internal_snippet, project_private_snippet]
end
it 'returns private snippets where the user is the author' do
result = described_class.elastic_search_code('password', options: { user: author })
result = described_class.elastic_search_code('password', options: { current_user: author })
expect(result.total_count).to eq(3)
expect(result.records).to match_array [public_snippet, internal_snippet, private_snippet]
......@@ -70,7 +70,7 @@ describe Snippet, :elastic do
member = create(:user)
project.add_reporter(member)
result = described_class.elastic_search_code('password +(123 | 789)', options: { user: member })
result = described_class.elastic_search_code('password +(123 | 789)', options: { current_user: member })
expect(result.total_count).to eq(2)
expect(result.records).to match_array [project_public_snippet, project_private_snippet]
......@@ -80,7 +80,7 @@ describe Snippet, :elastic do
it "returns all snippets for #{user_type}" do
superuser = create(user_type)
result = described_class.elastic_search_code('password', options: { user: superuser })
result = described_class.elastic_search_code('password', options: { current_user: superuser })
expect(result.total_count).to eq(6)
expect(result.records).to match_array [public_snippet, internal_snippet, private_snippet, project_public_snippet, project_internal_snippet, project_private_snippet]
......@@ -97,7 +97,7 @@ describe Snippet, :elastic do
project.add_developer(member)
expect(Ability).to receive(:allowed?).with(member, :read_cross_project) { false }
result = described_class.elastic_search_code('password', options: { user: member })
result = described_class.elastic_search_code('password', options: { current_user: member })
expect(result.records).to match_array [public_snippet, internal_snippet]
end
......@@ -116,7 +116,7 @@ describe Snippet, :elastic do
Gitlab::Elastic::Helper.refresh_index
end
options = { user: user }
options = { current_user: user }
expect(described_class.elastic_search('home', options: options).total_count).to eq(1)
expect(described_class.elastic_search('index.php', options: options).total_count).to eq(1)
......@@ -136,7 +136,13 @@ describe Snippet, :elastic do
'project_id',
'author_id',
'visibility_level'
).merge({ 'type' => snippet.es_type })
).merge({
'type' => snippet.es_type,
'join_field' => {
'name' => snippet.es_type,
'parent' => snippet.es_parent
}
})
expect(snippet.__elasticsearch__.as_indexed_json).to eq(expected_hash)
end
......
......@@ -78,8 +78,32 @@ describe Elastic::IndexRecordService, :elastic do
end
Gitlab::Elastic::Helper.refresh_index
## All database objects + data from repository. The absolute value does not matter
expect(Elasticsearch::Model.search('*').total_count).to be > 40
# Fetch all child documents
children = Elasticsearch::Model.search(
size: 100,
query: {
has_parent: {
parent_type: 'project',
query: {
term: { id: project.id }
}
}
}
)
# The absolute value does not matter
expect(children.total_count).to be > 40
# Make sure all types are present
expect(children.pluck(:_source).pluck(:type).uniq).to contain_exactly(
'blob',
'commit',
'issue',
'merge_request',
'milestone',
'note',
'snippet'
)
end
it 'does not index records not associated with the project' do
......
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