From 310846b8b4ca591518cc4b00ecd0f6b73477a629 Mon Sep 17 00:00:00 2001 From: Mantas Masalskis Date: Tue, 17 Aug 2021 07:44:54 +0200 Subject: [PATCH] Fixes #3068 - KB search does not allow pagination on endpoint --- .../application_controller/paginates.rb | 16 ++++ .../paginates/pagination.rb | 24 +++++ .../application_controller/renders_models.rb | 8 +- .../knowledge_base/search_controller.rb | 5 +- app/controllers/organizations_controller.rb | 15 +-- app/controllers/tickets_controller.rb | 30 ++---- app/controllers/users_controller.rb | 27 +----- lib/search_index_backend.rb | 28 +++--- lib/search_knowledge_base_backend.rb | 93 +++++++++++++------ .../paginates/pagination_spec.rb | 81 ++++++++++++++++ .../paginates/pagination.rb | 12 +++ .../lib/search_knowledge_base_backend_spec.rb | 61 ++++++++++++ .../search_with_details_spec.rb | 28 ++++++ spec/requests/organization_spec.rb | 12 +-- 14 files changed, 327 insertions(+), 113 deletions(-) create mode 100644 app/controllers/application_controller/paginates.rb create mode 100644 app/controllers/application_controller/paginates/pagination.rb create mode 100644 spec/controllers/application_controller/paginates/pagination_spec.rb create mode 100644 spec/factories/controllers/application_controller/paginates/pagination.rb diff --git a/app/controllers/application_controller/paginates.rb b/app/controllers/application_controller/paginates.rb new file mode 100644 index 000000000..f0928c016 --- /dev/null +++ b/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 diff --git a/app/controllers/application_controller/paginates/pagination.rb b/app/controllers/application_controller/paginates/pagination.rb new file mode 100644 index 000000000..73a31d34d --- /dev/null +++ b/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 diff --git a/app/controllers/application_controller/renders_models.rb b/app/controllers/application_controller/renders_models.rb index de39faf33..0dc1e50f0 100644 --- a/app/controllers/application_controller/renders_models.rb +++ b/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 = [] diff --git a/app/controllers/knowledge_base/search_controller.rb b/app/controllers/knowledge_base/search_controller.rb index d8d926c99..71b86da56 100644 --- a/app/controllers/knowledge_base/search_controller.rb +++ b/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]) } diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb index c123cb1f2..e6bf14734 100644 --- a/app/controllers/organizations_controller.rb +++ b/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, diff --git a/app/controllers/tickets_controller.rb b/app/controllers/tickets_controller.rb index fd60c92fe..95989b7f0 100644 --- a/app/controllers/tickets_controller.rb +++ b/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, diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 87324b0ae..ed1354ff3 100644 --- a/app/controllers/users_controller.rb +++ b/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] 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] 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, diff --git a/lib/search_index_backend.rb b/lib/search_index_backend.rb index 9275a910f..ddbad655d 100644 --- a/lib/search_index_backend.rb +++ b/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( diff --git a/lib/search_knowledge_base_backend.rb b/lib/search_knowledge_base_backend.rb index 61484ce42..98d445955 100644 --- a/lib/search_knowledge_base_backend.rb +++ b/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] :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 [HashString>, HashSymbol>] :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 diff --git a/spec/controllers/application_controller/paginates/pagination_spec.rb b/spec/controllers/application_controller/paginates/pagination_spec.rb new file mode 100644 index 000000000..3fd1bd5a3 --- /dev/null +++ b/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 diff --git a/spec/factories/controllers/application_controller/paginates/pagination.rb b/spec/factories/controllers/application_controller/paginates/pagination.rb new file mode 100644 index 000000000..781dae616 --- /dev/null +++ b/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 diff --git a/spec/lib/search_knowledge_base_backend_spec.rb b/spec/lib/search_knowledge_base_backend_spec.rb index 3e6db6188..5af8dd140 100644 --- a/spec/lib/search_knowledge_base_backend_spec.rb +++ b/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 diff --git a/spec/requests/knowledge_base/search_with_details_spec.rb b/spec/requests/knowledge_base/search_with_details_spec.rb index 1fbca3b49..287217fd8 100644 --- a/spec/requests/knowledge_base/search_with_details_spec.rb +++ b/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 diff --git a/spec/requests/organization_spec.rb b/spec/requests/organization_spec.rb index fad42f56a..95e8dfe87 100644 --- a/spec/requests/organization_spec.rb +++ b/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