From c2f4755a6c4147e70a0bba4fd07a6cfa40bb1252 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Tue, 6 Nov 2018 17:11:10 +0100 Subject: [PATCH] Search query extension refactoring to use params and not arguments. --- app/controllers/form_controller.rb | 6 +- app/controllers/search_controller.rb | 2 +- app/models/chat/session/search.rb | 2 +- app/models/organization/search.rb | 5 +- app/models/ticket/search.rb | 14 +- app/models/user/search.rb | 15 ++- ...up_user_preferences_notification_sound2.rb | 2 +- lib/search_index_backend.rb | 127 +++++++++++------- test/unit/search_index_backend_test.rb | 16 +++ 9 files changed, 127 insertions(+), 62 deletions(-) diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb index 2c2778711..681d28bd0 100644 --- a/app/controllers/form_controller.rb +++ b/app/controllers/form_controller.rb @@ -205,21 +205,21 @@ class FormController < ApplicationController return false if !SearchIndexBackend.enabled? form_limit_by_ip_per_hour = Setting.get('form_ticket_create_by_ip_per_hour') || 20 - result = SearchIndexBackend.search("preferences.form.remote_ip:'#{request.remote_ip}' AND created_at:>now-1h", form_limit_by_ip_per_hour, 'Ticket') + result = SearchIndexBackend.search("preferences.form.remote_ip:'#{request.remote_ip}' AND created_at:>now-1h", 'Ticket', limit: form_limit_by_ip_per_hour) if result.count >= form_limit_by_ip_per_hour.to_i response_access_deny return true end form_limit_by_ip_per_day = Setting.get('form_ticket_create_by_ip_per_day') || 240 - result = SearchIndexBackend.search("preferences.form.remote_ip:'#{request.remote_ip}' AND created_at:>now-1d", form_limit_by_ip_per_day, 'Ticket') + result = SearchIndexBackend.search("preferences.form.remote_ip:'#{request.remote_ip}' AND created_at:>now-1d", 'Ticket', limit: form_limit_by_ip_per_day) if result.count >= form_limit_by_ip_per_day.to_i response_access_deny return true end form_limit_per_day = Setting.get('form_ticket_create_per_day') || 5000 - result = SearchIndexBackend.search('preferences.form.remote_ip:* AND created_at:>now-1d', form_limit_per_day, 'Ticket') + result = SearchIndexBackend.search('preferences.form.remote_ip:* AND created_at:>now-1d', 'Ticket', limit: form_limit_per_day) if result.count >= form_limit_per_day.to_i response_access_deny return true diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index dddb9c284..1d0f7ba47 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -64,7 +64,7 @@ class SearchController < ApplicationController # do only one query to index search backend if objects_with_direct_search_index.present? - items = SearchIndexBackend.search(query, limit, objects_with_direct_search_index) + items = SearchIndexBackend.search(query, objects_with_direct_search_index, limit: limit) items.each do |item| require_dependency item[:type].to_filename local_class = Kernel.const_get(item[:type]) diff --git a/app/models/chat/session/search.rb b/app/models/chat/session/search.rb index 13eda294a..47c7df9d2 100644 --- a/app/models/chat/session/search.rb +++ b/app/models/chat/session/search.rb @@ -64,7 +64,7 @@ returns # try search index backend if SearchIndexBackend.enabled? - items = SearchIndexBackend.search(query, limit, 'Chat::Session', {}, offset) + items = SearchIndexBackend.search(query, 'Chat::Session', limit: limit, from: offset) chat_sessions = [] items.each do |item| chat_session = Chat::Session.lookup(id: item[:id]) diff --git a/app/models/organization/search.rb b/app/models/organization/search.rb index c266993f8..f236fe12a 100644 --- a/app/models/organization/search.rb +++ b/app/models/organization/search.rb @@ -83,7 +83,10 @@ returns # try search index backend if SearchIndexBackend.enabled? - items = SearchIndexBackend.search(query, limit, 'Organization', {}, offset, sort_by, order_by) + items = SearchIndexBackend.search(query, 'Organization', limit: limit, + from: offset, + sort_by: sort_by, + order_by: order_by) organizations = [] items.each do |item| organization = Organization.lookup(id: item[:id]) diff --git a/app/models/ticket/search.rb b/app/models/ticket/search.rb index bf6d6925c..e883d3eb4 100644 --- a/app/models/ticket/search.rb +++ b/app/models/ticket/search.rb @@ -127,9 +127,9 @@ returns # try search index backend if condition.blank? && SearchIndexBackend.enabled? - query_extention = {} - query_extention['bool'] = {} - query_extention['bool']['must'] = [] + query_extension = {} + query_extension['bool'] = {} + query_extension['bool']['must'] = [] if current_user.permissions?('ticket.agent') group_ids = current_user.group_ids_access('read') @@ -152,9 +152,13 @@ returns end end - query_extention['bool']['must'].push access_condition + query_extension['bool']['must'].push access_condition - items = SearchIndexBackend.search(query, limit, 'Ticket', query_extention, offset, sort_by, order_by) + items = SearchIndexBackend.search(query, 'Ticket', limit: limit, + query_extension: query_extension, + from: offset, + sort_by: sort_by, + order_by: order_by) if !full ids = [] items.each do |item| diff --git a/app/models/user/search.rb b/app/models/user/search.rb index 63daa5ded..006b18d94 100644 --- a/app/models/user/search.rb +++ b/app/models/user/search.rb @@ -101,19 +101,24 @@ returns # try search index backend if SearchIndexBackend.enabled? - query_extention = {} + query_extension = {} if params[:role_ids].present? - query_extention['bool'] = {} - query_extention['bool']['must'] = [] + query_extension['bool'] = {} + query_extension['bool']['must'] = [] if !params[:role_ids].is_a?(Array) params[:role_ids] = [params[:role_ids]] end access_condition = { 'query_string' => { 'default_field' => 'role_ids', 'query' => "\"#{params[:role_ids].join('" OR "')}\"" } } - query_extention['bool']['must'].push access_condition + query_extension['bool']['must'].push access_condition end - items = SearchIndexBackend.search(query, limit, 'User', query_extention, offset, sort_by, order_by) + + items = SearchIndexBackend.search(query, 'User', limit: limit, + query_extension: query_extension, + from: offset, + sort_by: sort_by, + order_by: order_by) users = [] items.each do |item| user = User.lookup(id: item[:id]) diff --git a/db/migrate/20180410000001_cleanup_user_preferences_notification_sound2.rb b/db/migrate/20180410000001_cleanup_user_preferences_notification_sound2.rb index 208e6dd86..10e860cd7 100644 --- a/db/migrate/20180410000001_cleanup_user_preferences_notification_sound2.rb +++ b/db/migrate/20180410000001_cleanup_user_preferences_notification_sound2.rb @@ -36,7 +36,7 @@ class CleanupUserPreferencesNotificationSound2 < ActiveRecord::Migration[5.1] user.save! end - items = SearchIndexBackend.search('preferences.notification_sound.enabled:*', 3000, 'User') || [] + items = SearchIndexBackend.search('preferences.notification_sound.enabled:*', 'User', limit: 3000) || [] items.each do |item| next if !item[:id] diff --git a/lib/search_index_backend.rb b/lib/search_index_backend.rb index 725bf7995..8629d8f9f 100644 --- a/lib/search_index_backend.rb +++ b/lib/search_index_backend.rb @@ -274,13 +274,19 @@ remove whole data from index =begin -return search result +@param query [String] search query +@param index [String, Array, nil] indexes to search in (see search_by_index) +@param options [Hash] search options (see build_query) - result = SearchIndexBackend.search('search query', limit, ['User', 'Organization']) +@return search result - result = SearchIndexBackend.search('search query', limit, 'User') +@example Sample queries - result = SearchIndexBackend.search('search query', limit, 'User', ['updated_at'], ['desc']) + result = SearchIndexBackend.search('search query', ['User', 'Organization'], limit: limit) + + result = SearchIndexBackend.search('search query', 'User', limit: limit) + + result = SearchIndexBackend.search('search query', 'User', limit: limit, sort_by: ['updated_at'], order_by: ['desc']) result = [ { @@ -299,25 +305,28 @@ return search result =end - # rubocop:disable Metrics/ParameterLists - def self.search(query, limit = 10, index = nil, query_extention = {}, from = 0, sort_by = [], order_by = []) - # rubocop:enable Metrics/ParameterLists - return [] if query.blank? - - if index.class == Array - ids = [] - index.each do |local_index| - local_ids = search_by_index(query, limit, local_index, query_extention, from, sort_by, order_by ) - ids = ids.concat(local_ids) - end - return ids + def self.search(query, index = nil, options = {}) + if !index.is_a? Array + return search_by_index(query, index, options) end - search_by_index(query, limit, index, query_extention, from, sort_by, order_by) + + index + .map { |local_index| search_by_index(query, local_index, options) } + .compact + .flatten(1) end - # rubocop:disable Metrics/ParameterLists - def self.search_by_index(query, limit = 10, index = nil, query_extention = {}, from = 0, sort_by = [], order_by = []) - # rubocop:enable Metrics/ParameterLists +=begin + +@param query [String] search query +@param index [String, Array, nil] index name or list of index names. If index is nil or not present will, search will be performed globally +@param options [Hash] search options (see build_query) + +@return search result + +=end + + def self.search_by_index(query, index = nil, options = {}) return [] if query.blank? url = build_url @@ -332,15 +341,6 @@ return search result else '/_search' end - data = {} - data['from'] = from - data['size'] = limit - - data['sort'] = search_by_index_sort(sort_by, order_by) - - data['query'] = query_extention || {} - data['query']['bool'] ||= {} - data['query']['bool']['must'] ||= [] # real search condition condition = { @@ -350,14 +350,15 @@ return search result 'analyze_wildcard' => true, } } - data['query']['bool']['must'].push condition + + query_data = build_query(condition, options) Rails.logger.info "# curl -X POST \"#{url}\" \\" - Rails.logger.debug { " -d'#{data.to_json}'" } + Rails.logger.debug { " -d'#{query_data.to_json}'" } response = UserAgent.get( url, - data, + query_data, { json: true, open_timeout: 5, @@ -372,35 +373,31 @@ return search result Rails.logger.error humanized_error( verb: 'GET', url: url, - payload: data, + payload: query_data, response: response, ) return [] end - data = response.data + data = response.data&.dig('hits', 'hits') - ids = [] - return ids if !data - return ids if !data['hits'] - return ids if !data['hits']['hits'] + return [] if !data - data['hits']['hits'].each do |item| + data.map do |item| Rails.logger.info "... #{item['_type']} #{item['_id']}" - data = { + + { id: item['_id'], type: item['_type'], } - ids.push data end - ids end - def self.search_by_index_sort(sort_by = [], order_by = []) + def self.search_by_index_sort(sort_by = nil, order_by = nil) result = [] - sort_by.each_with_index do |value, index| + sort_by&.each_with_index do |value, index| next if value.blank? - next if order_by[index].blank? + next if order_by&.at(index).blank? if value !~ /\./ && value !~ /_(time|date|till|id|ids|at)$/ value += '.raw' @@ -735,4 +732,44 @@ return true if backend is configured query += '*' if !query.match?(/:/) query end + +=begin + +@param condition [Hash] search condition +@param options [Hash] search options +@option options [Integer] :from +@option options [Integer] :limit +@option options [Hash] :query_extension applied to ElasticSearch query +@option options [Array] :order_by ordering directions, desc or asc +@option options [Array] :sort_by fields to sort by + +=end + + DEFAULT_QUERY_OPTIONS = { + from: 0, + limit: 10 + }.freeze + + def self.build_query(condition, options = {}) + options = DEFAULT_QUERY_OPTIONS.merge(options.deep_symbolize_keys) + + data = { + from: options[:from], + size: options[:limit], + sort: search_by_index_sort(options[:sort_by], options[:order_by]), + query: { + bool: { + must: [] + } + } + } + + if (extension = options.dig(:query_extension)) + data[:query].deep_merge! extension.deep_dup + end + + data[:query][:bool][:must].push condition + + data + end end diff --git a/test/unit/search_index_backend_test.rb b/test/unit/search_index_backend_test.rb index 94b404dee..dbe2a230f 100644 --- a/test/unit/search_index_backend_test.rb +++ b/test/unit/search_index_backend_test.rb @@ -1,6 +1,21 @@ require 'test_helper' class SearchIndexBackendTest < ActiveSupport::TestCase + test 'query extension keys are normalized to symbols' do + query_strings = SearchIndexBackend.build_query('', query_extension: { 'bool' => { 'filter' => { 'term' => { 'a' => 'b' } } } }) + query_symbols = SearchIndexBackend.build_query('', query_extension: { bool: { filter: { term: { a: 'b' } } } }) + + assert_equal query_strings, query_symbols + assert_not_nil query_strings.dig(:query, :bool, :filter, :term, :a) + end + + test 'search with ES off never returns nil in array' do + index_one = SearchIndexBackend.search('preferences.notification_sound.enabled:*', 'User', limit: 3000) + index_multi = SearchIndexBackend.search('preferences.notification_sound.enabled:*', %w[User Organization], limit: 3000) + + assert_nil index_one + assert index_multi.empty? + end test 'simple_query_append_wildcard correctly modifies simple queries' do def clean_queries(query_string) @@ -48,6 +63,7 @@ class SearchIndexBackendTest < ActiveSupport::TestCase simple_queries = clean_queries %( M + Max Max. # dot and underscore are acceptable characters in simple queries A_