Search query extension refactoring to use params and not arguments.

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

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])

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])

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])

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|

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])

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]
 

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<String>, 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<String>, 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<String>] :order_by ordering directions, desc or asc
+@option options [Array<String>] :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

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_