Fixes #3068 - KB search does not allow pagination on endpoint

app/controllers/application_controller/paginates.rb

@@ -0,0 +1,16 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+module ApplicationController::Paginates
+  extend ActiveSupport::Concern
+
+  def paginate_with(max: nil, default: nil)
+    @paginate_max     = max
+    @paginate_default = default
+  end
+
+  private
+
+  def pagination
+    @pagination ||= ::ApplicationController::Paginates::Pagination.new(params, max: @paginate_max, default: @paginate_default)
+  end
+end

app/controllers/application_controller/paginates/pagination.rb

@@ -0,0 +1,24 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class ApplicationController::Paginates::Pagination
+
+  def initialize(params, default: nil, max: nil)
+    @params  = params
+    @default = default.presence || 100
+    @max     = max.presence || 500
+  end
+
+  def limit
+    limit = @params[:per_page] || @params[:limit] || @default
+
+    [limit.to_i, @max].min
+  end
+
+  def page
+    @params[:page]&.to_i || 1
+  end
+
+  def offset
+    (page - 1) * limit
+  end
+end

app/controllers/application_controller/renders_models.rb

@@ -3,6 +3,8 @@
 module ApplicationController::RendersModels
   extend ActiveSupport::Concern
 
+  include ApplicationController::Paginates
+
   private
 
   # model helper
@@ -103,16 +105,14 @@ module ApplicationController::RendersModels
   end
 
   def model_index_render(object, params)
-    page = (params[:page] || 1).to_i
-    per_page = (params[:per_page] || 500).to_i
-    offset = (page - 1) * per_page
+    paginate_with(default: 500)
 
     sql_helper = ::SqlHelper.new(object: object)
     sort_by    = sql_helper.get_sort_by(params, 'id')
     order_by   = sql_helper.get_order_by(params, 'ASC')
     order_sql  = sql_helper.get_order(sort_by, order_by)
 
-    generic_objects = object.order(Arel.sql(order_sql)).offset(offset).limit(per_page)
+    generic_objects = object.order(Arel.sql(order_sql)).offset(pagination.offset).limit(pagination.limit)
 
     if response_expand?
       list = []

app/controllers/knowledge_base/search_controller.rb

@@ -6,9 +6,10 @@ class KnowledgeBase::SearchController < ApplicationController
 
   include KnowledgeBaseHelper
   include ActionView::Helpers::SanitizeHelper
+  include ApplicationController::Paginates
 
   # POST /api/v1/knowledge_bases/search
-  # knowledge_base_id, locale, flavor, index, limit, include_locale
+  # knowledge_base_id, locale, flavor, index, page, per_page, limit, include_locale
   def search
     knowledge_base = KnowledgeBase
                      .active
@@ -35,7 +36,7 @@ class KnowledgeBase::SearchController < ApplicationController
 
     include_locale = params[:include_locale] && KnowledgeBase.with_multiple_locales_exists?
 
-    result = search_backend.search params[:query], user: current_user
+    result = search_backend.search params[:query], user: current_user, pagination: pagination
 
     if (exclude_ids = params[:exclude_ids]&.map(&:to_i))
       result.reject! { |meta| meta[:type] == params[:index] && exclude_ids.include?(meta[:id]) }

app/controllers/organizations_controller.rb

@@ -4,6 +4,8 @@ class OrganizationsController < ApplicationController
   prepend_before_action -> { authorize! }, except: %i[index show]
   prepend_before_action { authentication_check }
 
+  include ApplicationController::Paginates
+
 =begin
 
 Format:
@@ -176,23 +178,14 @@ curl http://localhost/api/v1/organization/{id} -v -u #{login}:#{password} -H "Co
 
   # GET /api/v1/organizations/search
   def search
-    per_page = params[:per_page] || params[:limit] || 100
-    per_page = per_page.to_i
-    if per_page > 500
-      per_page = 500
-    end
-    page = params[:page] || 1
-    page = page.to_i
-    offset = (page - 1) * per_page
-
     query = params[:query]
     if query.respond_to?(:permit!)
       query = query.permit!.to_h
     end
     query_params = {
       query:        query,
-      limit:        per_page,
-      offset:       offset,
+      limit:        pagination.limit,
+      offset:       pagination.offset,
       sort_by:      params[:sort_by],
       order_by:     params[:order_by],
       current_user: current_user,

app/controllers/tickets_controller.rb

@@ -5,28 +5,19 @@ class TicketsController < ApplicationController
   include ClonesTicketArticleAttachments
   include ChecksUserAttributesByCurrentUserPermission
   include TicketStats
+  include ApplicationController::Paginates
 
   prepend_before_action -> { authorize! }, only: %i[create selector import_example import_start ticket_customer ticket_history ticket_related ticket_recent ticket_merge ticket_split]
   prepend_before_action :authentication_check
 
   # GET /api/v1/tickets
   def index
-    offset = 0
-    per_page = 100
-
-    if params[:page] && params[:per_page]
-      offset = (params[:page].to_i - 1) * params[:per_page].to_i
-      per_page = params[:per_page].to_i
-    end
-
-    if per_page > 100
-      per_page = 100
-    end
+    paginate_with(max: 100)
 
     tickets = TicketPolicy::ReadScope.new(current_user).resolve
                                      .order(id: :asc)
-                                     .offset(offset)
-                                     .limit(per_page)
+                                     .offset(pagination.offset)
+                                     .limit(pagination.limit)
 
     if response_expand?
       list = []
@@ -448,14 +439,7 @@ class TicketsController < ApplicationController
       params.require(:condition).permit!
     end
 
-    per_page = params[:per_page] || params[:limit] || 50
-    per_page = per_page.to_i
-    if per_page > 200
-      per_page = 200
-    end
-    page = params[:page] || 1
-    page = page.to_i
-    offset = (page - 1) * per_page
+    paginate_with(max: 200, default: 50)
 
     query = params[:query]
     if query.respond_to?(:permit!)
@@ -466,8 +450,8 @@ class TicketsController < ApplicationController
     tickets = Ticket.search(
       query:        query,
       condition:    params[:condition].to_h,
-      limit:        per_page,
-      offset:       offset,
+      limit:        pagination.limit,
+      offset:       pagination.offset,
       order_by:     params[:order_by],
       sort_by:      params[:sort_by],
       current_user: current_user,

app/controllers/users_controller.rb

@@ -2,6 +2,7 @@
 
 class UsersController < ApplicationController
   include ChecksUserAttributesByCurrentUserPermission
+  include ApplicationController::Paginates
 
   prepend_before_action -> { authorize! }, only: %i[import_example import_start search history unlock]
   prepend_before_action :authentication_check, except: %i[create password_reset_send password_reset_verify image email_verify email_verify_send]
@@ -17,18 +18,7 @@ class UsersController < ApplicationController
   # @response_message 200 [Array<User>] List of matching User records.
   # @response_message 403               Forbidden / Invalid session.
   def index
-    offset = 0
-    per_page = 500
-    if params[:page] && params[:per_page]
-      offset = (params[:page].to_i - 1) * params[:per_page].to_i
-      per_page = params[:per_page].to_i
-    end
-
-    if per_page > 500
-      per_page = 500
-    end
-
-    users = policy_scope(User).order(id: :asc).offset(offset).limit(per_page)
+    users = policy_scope(User).order(id: :asc).offset(pagination.offset).limit(pagination.limit)
 
     if response_expand?
       list = []
@@ -229,15 +219,6 @@ class UsersController < ApplicationController
   # @response_message 200 [Array<User>] A list of User records matching the search term.
   # @response_message 403               Forbidden / Invalid session.
   def search
-    per_page = params[:per_page] || params[:limit] || 100
-    per_page = per_page.to_i
-    if per_page > 500
-      per_page = 500
-    end
-    page = params[:page] || 1
-    page = page.to_i
-    offset = (page - 1) * per_page
-
     query = params[:query]
     if query.respond_to?(:permit!)
       query.permit!.to_h
@@ -250,8 +231,8 @@ class UsersController < ApplicationController
 
     query_params = {
       query:        query,
-      limit:        per_page,
-      offset:       offset,
+      limit:        pagination.limit,
+      offset:       pagination.offset,
       sort_by:      params[:sort_by],
       order_by:     params[:order_by],
       current_user: current_user,

lib/search_index_backend.rb

@@ -337,22 +337,24 @@ remove whole data from index
   end
 
   def self.search_by_index_sort(sort_by = nil, order_by = nil)
-    result = []
+    result = (sort_by || [])
+      .map(&:to_s)
+      .each_with_object([])
+      .each_with_index do |(elem, memo), index|
+        next if elem.blank?
+        next if order_by&.at(index).blank?
 
-    sort_by&.each_with_index do |value, index|
-      next if value.blank?
-      next if order_by&.at(index).blank?
+        # for sorting values use .keyword values (no analyzer is used - plain values)
+        if elem !~ %r{\.} && elem !~ %r{_(time|date|till|id|ids|at)$} && elem != 'id'
+          elem += '.keyword'
+        end
 
-      # for sorting values use .keyword values (no analyzer is used - plain values)
-      if value !~ %r{\.} && value !~ %r{_(time|date|till|id|ids|at)$}
-        value += '.keyword'
+        memo.push(
+          elem => {
+            order: order_by[index],
+          },
+        )
       end
-      result.push(
-        value => {
-          order: order_by[index],
-        },
-      )
-    end
 
     if result.blank?
       result.push(

lib/search_knowledge_base_backend.rb

@@ -9,7 +9,9 @@ class SearchKnowledgeBaseBackend
   # @option params [KnowledgeBase::Category] :scope (nil) optional search scope
   # @option params [Symbol]  :flavor (:public) agent or public to indicate source and narrow down to internal or public answers accordingly
   # @option params [String, Array<String>] :index (nil) indexes to limit search to, searches all indexes if nil
+  # @option params [Integer] :limit per page param for paginatin
   # @option params [Boolean] :highlight_enabled (true) highlight matching text
+  # @option params [Hash<String=>String>, Hash<Symbol=>Symbol>] :order_by hash with column => asc/desc
 
   def initialize(params)
     @params = params.compact
@@ -17,23 +19,18 @@ class SearchKnowledgeBaseBackend
     prepare_scope_ids
   end
 
-  def search(query, user: nil)
-    raw_results = if SearchIndexBackend.enabled?
-                    SearchIndexBackend
-                      .search(query, indexes, options)
-                      .map do |hash|
-                        hash[:id] = hash[:id].to_i
-                        hash
-                      end
-                  else
-                    search_fallback(query, indexes, { user: user })
-                  end
+  def search(query, user: nil, pagination: nil)
+    raw_results = raw_results(query, user, pagination: pagination)
 
-    if (limit = @params.fetch(:limit, nil))
-      raw_results = raw_results[0, limit]
+    filtered = filter_results raw_results, user
+
+    if pagination
+      filtered = filtered.slice pagination.offset, pagination.limit
+    elsif @params[:limit]
+      filtered = filtered.slice 0, @params[:limit]
     end
 
-    filter_results raw_results, user
+    filtered
   end
 
   def search_fallback(query, indexes, options)
@@ -47,10 +44,26 @@ class SearchKnowledgeBaseBackend
       .constantize
       .search_fallback("%#{query}%", @cached_scope_ids, options: options)
       .where(kb_locale: kb_locales)
+      .order(**search_fallback_order)
       .pluck(:id)
       .map { |id| { id: id, type: index } }
   end
 
+  def search_fallback_order
+    @params[:order_by].presence || { updated_at: :desc }
+  end
+
+  def raw_results(query, user, pagination: nil)
+    return search_fallback(query, indexes, { user: user }) if !SearchIndexBackend.enabled?
+
+    SearchIndexBackend
+      .search(query, indexes, options(pagination: pagination))
+      .map do |hash|
+        hash[:id] = hash[:id].to_i
+        hash
+      end
+  end
+
   def filter_results(raw_results, user)
     raw_results
       .group_by { |result| result[:type] }
@@ -162,28 +175,56 @@ class SearchKnowledgeBaseBackend
     @params.fetch(:flavor, :public).to_sym
   end
 
-  def options
-    output = {
+  def base_options
+    {
       query_extension: {
         bool: {
           must: [ { terms: { kb_locale_id: kb_locale_ids } } ]
         }
       }
     }
+  end
 
-    if @params.fetch(:highlight_enabled, true)
-      output[:highlight_fields_by_indexes] = {
-        'KnowledgeBase::Answer::Translation':   %w[title content.body attachment.content tags],
-        'KnowledgeBase::Category::Translation': %w[title],
-        'KnowledgeBase::Translation':           %w[title]
-      }
+  def options_apply_highlight(hash)
+    return if !@params.fetch(:highlight_enabled, true)
+
+    hash[:highlight_fields_by_indexes] = {
+      'KnowledgeBase::Answer::Translation':   %w[title content.body attachment.content tags],
+      'KnowledgeBase::Category::Translation': %w[title],
+      'KnowledgeBase::Translation':           %w[title]
+    }
+  end
+
+  def options_apply_scope(hash)
+    return if !@params.fetch(:scope, nil)
+
+    hash[:query_extension][:bool][:must].push({ terms: { scope_id: @cached_scope_ids } })
+  end
+
+  def options_apply_pagination(hash, pagination)
+    if @params[:from] && @params[:limit]
+      hash[:from]  = @params[:from]
+      hash[:limit] = @params[:limit]
+    elsif pagination
+      hash[:from]  = 0
+      hash[:limit] = pagination.limit * 99
     end
+  end
 
-    if @params.fetch(:scope, nil)
-      scope = { terms: { scope_id: @cached_scope_ids } }
+  def options_apply_order(hash)
+    return if @params[:order_by].blank?
 
-      output[:query_extension][:bool][:must].push scope
-    end
+    hash[:sort_by]  = @params[:order_by].keys
+    hash[:order_by] = @params[:order_by].values
+  end
+
+  def options(pagination: nil)
+    output = base_options
+
+    options_apply_highlight(output)
+    options_apply_scope(output)
+    options_apply_pagination(output, pagination)
+    options_apply_order(output)
 
     output
   end

spec/controllers/application_controller/paginates/pagination_spec.rb

@@ -0,0 +1,81 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe ApplicationController::Paginates::Pagination do
+  describe '#limit' do
+    it 'returns as set in params' do
+      instance = described_class.new({ per_page: 123 })
+      expect(instance.limit).to be 123
+    end
+
+    it 'ensures that per_page is an integer' do
+      instance = described_class.new({ per_page: '123' })
+      expect(instance.limit).to be 123
+    end
+
+    it 'when missing, returns as set in limit attribute' do
+      instance = described_class.new({ limit: 123 })
+      expect(instance.limit).to be 123
+    end
+
+    it 'falls back to default' do
+      instance = described_class.new({})
+      expect(instance.limit).to be 100
+    end
+
+    it 'falls back to custom default' do
+      instance = described_class.new({}, default: 222)
+      expect(instance.limit).to be 222
+    end
+
+    it 'per_page attribute preferred over limit' do
+      instance = described_class.new({ per_page: 123, limit: 321 })
+      expect(instance.limit).to be 123
+    end
+
+    it 'capped by limit' do
+      instance = described_class.new({ per_page: 9999 })
+      expect(instance.limit).to be 500
+    end
+
+    it 'capped by custom default' do
+      instance = described_class.new({ per_page: 9999 }, max: 10)
+      expect(instance.limit).to be 10
+    end
+  end
+
+  describe '#page' do
+    it 'returns page number' do
+      instance = described_class.new({ page: 123 })
+      expect(instance.page).to be 123
+    end
+
+    it 'defaults to 1 when missing' do
+      instance = described_class.new({})
+      expect(instance.page).to be 1
+    end
+
+    it 'ensures that page is an integer' do
+      instance = described_class.new({ page: '123' })
+      expect(instance.page).to be 123
+    end
+  end
+
+  describe '#offset' do
+    it 'returns 0 when no page given' do
+      instance = described_class.new({})
+      expect(instance.offset).to be 0
+    end
+
+    it 'returns offset for page' do
+      instance = described_class.new({ page: 3 })
+      expect(instance.offset).to be 200
+    end
+
+    it 'returns offset based on custom per_page value' do
+      instance = described_class.new({ page: 3, per_page: 15 })
+      expect(instance.offset).to be 30
+    end
+  end
+end

spec/factories/controllers/application_controller/paginates/pagination.rb

@@ -0,0 +1,12 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+FactoryBot.define do
+  factory :'application_controller/paginates/pagination', aliases: %i[pagination] do
+
+    params  { {} }
+    default { nil }
+    max { nil }
+
+    initialize_with { new(params, default: default, max: max) }
+  end
+end

spec/lib/search_knowledge_base_backend_spec.rb

@@ -42,6 +42,67 @@ RSpec.describe SearchKnowledgeBaseBackend do
     end
   end
 
+  context 'with paging' do
+    let(:answers) do
+      Array.new(20) do |nth|
+        create(:knowledge_base_answer, :published, :with_attachment, category: category, translation_attributes: { title: "#{search_phrase} #{nth}" })
+      end
+    end
+
+    let(:search_phrase) { 'paging test' }
+
+    let(:options) do
+      {
+        knowledge_base: knowledge_base,
+        locale:         primary_locale,
+        scope:          nil,
+        order_by:       { id: :desc }
+      }
+    end
+
+    shared_examples 'verify paging' do |elasticsearch:|
+      context "when elastic search is #{elasticsearch}", searchindex: elasticsearch do
+        before do
+          answers
+          configure_elasticsearch(required: true, rebuild: true) if elasticsearch
+        end
+
+        it 'first page is first 5 answers' do
+          results = instance.search(search_phrase, user: user, pagination: build(:pagination, params: { page: 1, per_page: 5 }))
+
+          first_5 = answers.reverse.slice(0, 5)
+
+          expect(results.pluck(:id)).to eq first_5.map { |answer| answer.translations.first.id }
+        end
+
+        it 'second page is next 5 answers' do
+          results = instance.search(search_phrase, user: user, pagination: build(:pagination, params: { page: 2, per_page: 5 }))
+
+          next_5 = answers.reverse.slice(5, 5)
+
+          expect(results.pluck(:id)).to eq next_5.map { |answer| answer.translations.first.id }
+        end
+
+        it 'last page may be partial' do
+          results = instance.search(search_phrase, user: user, pagination: build(:pagination, params: { page: 4, per_page: 6 }))
+
+          last_page = answers.reverse.slice(18, 6)
+
+          expect(results.pluck(:id)).to eq last_page.map { |answer| answer.translations.first.id }
+        end
+
+        it '5th page is empty' do
+          results = instance.search(search_phrase, user: user, pagination: build(:pagination, params: { page: 5, per_page: 5 }))
+
+          expect(results).to be_blank
+        end
+      end
+    end
+
+    include_examples 'verify paging', elasticsearch: true
+    include_examples 'verify paging', elasticsearch: false
+  end
+
   context 'with successful API response' do
     shared_examples 'verify response' do |elasticsearch:|
       it "ID is an Integer when ES=#{elasticsearch}", searchindex: elasticsearch do

spec/requests/knowledge_base/search_with_details_spec.rb

@@ -87,4 +87,32 @@ RSpec.describe 'Knowledge Base search with details', type: :request, searchindex
       expect(json_response['details'][0]['subtitle']).to eq("#{category4.translations.first.title} > #{category5.translations.first.title}")
     end
   end
+
+  context 'when using paging' do
+    let(:answers) do
+      Array.new(20) do |nth|
+        create(:knowledge_base_answer, :published, :with_attachment, category: category, translation_attributes: { title: "#{search_phrase} #{nth}" })
+      end
+    end
+
+    let(:search_phrase) { 'paging test' }
+
+    before do
+      configure_elasticsearch(required: true, rebuild: true) do
+        answers
+      end
+    end
+
+    it 'returns success' do
+      post endpoint, params: { query: search_phrase, per_page: 10, page: 0 }
+
+      expect(response).to have_http_status(:ok)
+    end
+
+    it 'returns defined amount of items' do
+      post endpoint, params: { query: search_phrase, per_page: 7, page: 0 }
+
+      expect(json_response['result'].count).to be 7
+    end
+  end
 end

spec/requests/organization_spec.rb

@@ -42,17 +42,7 @@ RSpec.describe 'Organization', type: :request, searchindex: true do
   end
 
   before do
-    configure_elasticsearch do
-
-      travel 1.minute
-
-      rebuild_searchindex
-
-      # execute background jobs
-      Scheduler.worker(true)
-
-      sleep 6
-    end
+    configure_elasticsearch rebuild: true, required: true
   end
 
   describe 'request handling' do