From 36b7a69e7ea1a1b992ca21b1291ea07f509dd304 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Sun, 25 Apr 2021 20:46:33 +0100 Subject: [PATCH] Improve searching of petitions By using the btree_gin extension and merging the indexes on the various columns into a single index combined with state, debate_state and parliament_id for archived petitions we can achieve a significant performance gain of more than 10x when searching arbitrary text. This also adds in the petition id to the index so people can now search by that on the front end - it will even return cases where people refer to the petition by id in other petition text. --- Gemfile | 1 - Gemfile.lock | 3 - app/models/archived/petition.rb | 5 +- app/models/concerns/browseable.rb | 68 +++++++- app/models/department.rb | 6 +- app/models/invalidation.rb | 6 +- app/models/petition.rb | 6 +- app/models/tag.rb | 6 +- app/models/topic.rb | 5 +- app/views/admin/topics/index.html.erb | 2 +- ...53757_optimize_free_text_search_indexes.rb | 99 +++++++++++ ...optimize_search_indexes_for_departments.rb | 21 +++ ...timize_search_indexes_for_invalidations.rb | 46 +++++ ...143206_optimize_search_indexes_for_tags.rb | 33 ++++ ...3213_optimize_search_indexes_for_topics.rb | 21 +++ ...425185707_add_name_index_to_departments.rb | 11 ++ db/schema.rb | 22 +-- spec/models/concerns/browseable_spec.rb | 160 ++++++++++++++---- 18 files changed, 453 insertions(+), 68 deletions(-) create mode 100644 db/migrate/20210422053757_optimize_free_text_search_indexes.rb create mode 100644 db/migrate/20210425143147_optimize_search_indexes_for_departments.rb create mode 100644 db/migrate/20210425143158_optimize_search_indexes_for_invalidations.rb create mode 100644 db/migrate/20210425143206_optimize_search_indexes_for_tags.rb create mode 100644 db/migrate/20210425143213_optimize_search_indexes_for_topics.rb create mode 100644 db/migrate/20210425185707_add_name_index_to_departments.rb diff --git a/Gemfile b/Gemfile index fde3fc2c8..872c23653 100644 --- a/Gemfile +++ b/Gemfile @@ -17,7 +17,6 @@ gem 'faraday' gem 'faraday_middleware' gem 'net-http-persistent' gem 'sass-rails', '~> 5.0' -gem 'textacular' gem 'uglifier' gem 'bcrypt' gem 'faker', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 1daadf465..ee134f710 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -332,8 +332,6 @@ GEM actionpack (>= 4.0) activesupport (>= 4.0) sprockets (>= 3.0.0) - textacular (5.4.0) - activerecord (>= 5.0, < 6.2) thor (1.1.0) tilt (2.0.10) tzinfo (2.0.4) @@ -413,7 +411,6 @@ DEPENDENCIES shoulda-matchers simplecov slack-notifier - textacular uglifier webdrivers (~> 3.8.1) webmock diff --git a/app/models/archived/petition.rb b/app/models/archived/petition.rb index f32e10563..339e2d458 100644 --- a/app/models/archived/petition.rb +++ b/app/models/archived/petition.rb @@ -1,4 +1,3 @@ -require 'textacular/searchable' require_dependency 'archived' module Archived @@ -48,9 +47,11 @@ class Petition < ActiveRecord::Base before_save :update_debate_state, if: :scheduled_debate_date_changed? - extend Searchable(:action, :background, :additional_details) include Browseable, Taggable, Departments, Topics, Anonymization + query :id, :action, :background + query :additional_details, null: true + facet :all, -> { visible.by_most_signatures } facet :awaiting_response, -> { awaiting_response.by_waiting_for_response_longest } facet :awaiting_debate_date, -> { awaiting_debate_date.by_waiting_for_debate_longest } diff --git a/app/models/concerns/browseable.rb b/app/models/concerns/browseable.rb index 42e8c565d..2fdf32502 100644 --- a/app/models/concerns/browseable.rb +++ b/app/models/concerns/browseable.rb @@ -11,6 +11,9 @@ module Browseable class_attribute :filter_definitions, instance_writer: false self.filter_definitions = {} + class_attribute :query_columns, instance_writer: false + self.query_columns = [] + class_attribute :default_page_size, instance_writer: false self.default_page_size = 50 @@ -108,6 +111,60 @@ def filter_params end end + class Query + Column = Struct.new(:name, :config, :null) + + attr_reader :klass, :param + + delegate :query_columns, to: :klass + delegate :connection, to: :klass + delegate :quoted_table_name, to: :klass + delegate :quote_column_name, to: :connection + delegate :inspect, :present?, :==, to: :to_s + + def initialize(klass, param) + @klass, @param = klass, param.to_s + end + + def build + [sql, query: param] + end + + def to_s + param + end + + private + + def sql + "((#{search_conditions.join(" || ")}) @@ #{to_tsquery})" + end + + def search_conditions + query_columns.map(&method(:search_condition)) + end + + def search_condition(column) + "#{to_tsvector(column)}" + end + + def quoted_column(column) + quoted_table_name + "." + quote_column_name(column) + end + + def to_tsvector(column) + if column.null + "to_tsvector('#{column.config}', COALESCE(#{quoted_column(column.name)}, '')::text)" + else + "to_tsvector('#{column.config}', #{quoted_column(column.name)}::text)" + end + end + + def to_tsquery + "plainto_tsquery('english', :query)" + end + end + class Search include Enumerable @@ -119,6 +176,7 @@ class Search delegate :total_entries, :total_pages, to: :results delegate :to_a, :to_ary, to: :results delegate :each, :map, :size, to: :to_a + delegate :explain, to: :execute_search_with_pagination def initialize(klass, params = {}) @klass, @params = klass, params @@ -157,7 +215,7 @@ def last_page? end def query - @query ||= params[:q].to_s + @query ||= Query.new(klass, params[:q]) end def page_size @@ -239,9 +297,7 @@ def execute_search_with_pagination def execute_search if search? - relation = klass.basic_search(query) - relation = relation.except(:select).select(star) - relation = relation.except(:order) + relation = klass.where(query.build) else relation = klass end @@ -276,6 +332,10 @@ def filter(key, transformer) self.filter_definitions[key] = transformer end + def query(*columns, config: "english", null: false) + self.query_columns += columns.map { |column| Query::Column.new(column.to_s, config, null) } + end + def search(params) Search.new(all, params) end diff --git a/app/models/department.rb b/app/models/department.rb index ecf475040..a52c7de4c 100644 --- a/app/models/department.rb +++ b/app/models/department.rb @@ -1,9 +1,9 @@ -require 'textacular/searchable' - class Department < ActiveRecord::Base - extend Searchable(:name, :acronym) include Browseable + query :name + query :acronym, null: true + validates :external_id, length: { maximum: 30 } validates :name, presence: true, length: { maximum: 100 } validates :acronym, length: { maximum: 10 } diff --git a/app/models/invalidation.rb b/app/models/invalidation.rb index 9844a4dff..c7f2f363c 100644 --- a/app/models/invalidation.rb +++ b/app/models/invalidation.rb @@ -1,9 +1,9 @@ -require 'textacular/searchable' - class Invalidation < ActiveRecord::Base - extend Searchable(:id, :summary, :details, :petition_id) include Browseable + query :summary + query :details, :petition_id, null: true + belongs_to :petition, optional: true has_many :signatures diff --git a/app/models/petition.rb b/app/models/petition.rb index f63b89565..6c99fc27d 100644 --- a/app/models/petition.rb +++ b/app/models/petition.rb @@ -1,5 +1,3 @@ -require 'textacular/searchable' - class Petition < ActiveRecord::Base include PerishableTokenGenerator @@ -46,9 +44,11 @@ class Petition < ActiveRecord::Base before_save :update_moderation_lag, unless: :moderation_lag? after_create :update_last_petition_created_at - extend Searchable(:action, :background, :additional_details) include Browseable, Taggable, Departments, Topics, Anonymization + query :id, :action, :background + query :additional_details, null: true + facet :all, -> { by_most_popular } facet :open, -> { open_state.by_most_popular } facet :rejected, -> { rejected_state.by_most_recent } diff --git a/app/models/tag.rb b/app/models/tag.rb index 8d9d90f8a..3a0ac2430 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -1,9 +1,9 @@ -require 'textacular/searchable' - class Tag < ActiveRecord::Base - extend Searchable(:name, :description) include Browseable + query :name + query :description, null: true + validates_presence_of :name validates_uniqueness_of :name validates_length_of :name, maximum: 50 diff --git a/app/models/topic.rb b/app/models/topic.rb index 91bb4997d..a93f099ec 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -1,9 +1,8 @@ -require 'textacular/searchable' - class Topic < ActiveRecord::Base - extend Searchable(:code, :name) include Browseable + query :code, :name + with_options presence: true, uniqueness: true do validates :code, length: { maximum: 100 } validates :name, length: { maximum: 100 } diff --git a/app/views/admin/topics/index.html.erb b/app/views/admin/topics/index.html.erb index 5a5297c4d..08ece04e7 100644 --- a/app/views/admin/topics/index.html.erb +++ b/app/views/admin/topics/index.html.erb @@ -1,7 +1,7 @@ <%= render "admin/shared/tag_tabs" %>
- <%= form_tag admin_tags_path, method: :get, class: 'search-topics' do %> + <%= form_tag admin_topics_path, method: :get, class: 'search-topics' do %>
<%= label_tag :q, "Search topics", class: "visuallyhidden" %> diff --git a/db/migrate/20210422053757_optimize_free_text_search_indexes.rb b/db/migrate/20210422053757_optimize_free_text_search_indexes.rb new file mode 100644 index 000000000..0e4911940 --- /dev/null +++ b/db/migrate/20210422053757_optimize_free_text_search_indexes.rb @@ -0,0 +1,99 @@ +class OptimizeFreeTextSearchIndexes < ActiveRecord::Migration[6.1] + disable_ddl_transaction! + + def up + execute "CREATE EXTENSION IF NOT EXISTS btree_gin" + + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + index_petitions_for_search + ON petitions USING gin(( + to_tsvector('english', id::text) || + to_tsvector('english', action::text) || + to_tsvector('english', background::text) || + to_tsvector('english', COALESCE(additional_details, '')::text)), + state, debate_state + ); + SQL + + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + index_archived_petitions_for_search + ON archived_petitions USING gin(( + to_tsvector('english', id::text) || + to_tsvector('english', action::text) || + to_tsvector('english', background::text) || + to_tsvector('english', COALESCE(additional_details, '')::text)), + state, parliament_id, debate_state + ); + SQL + + execute "DROP INDEX CONCURRENTLY IF EXISTS index_petitions_on_action" + execute "DROP INDEX CONCURRENTLY IF EXISTS index_petitions_on_background" + execute "DROP INDEX CONCURRENTLY IF EXISTS index_petitions_on_additional_details" + execute "ANALYZE petitions" + + execute "DROP INDEX CONCURRENTLY IF EXISTS index_archived_petitions_on_action" + execute "DROP INDEX CONCURRENTLY IF EXISTS index_archived_petitions_on_background" + execute "DROP INDEX CONCURRENTLY IF EXISTS index_archived_petitions_on_additional_details" + execute "ANALYZE archived_petitions" + end + + def down + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + index_petitions_on_action + ON petitions USING gin( + to_tsvector('english', action) + ); + SQL + + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + index_petitions_on_background + ON petitions USING gin( + to_tsvector('english', background) + ); + SQL + + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + index_petitions_on_additional_details + ON petitions USING gin( + to_tsvector('english', additional_details) + ); + SQL + + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + index_archived_petitions_on_action + ON archived_petitions USING gin( + to_tsvector('english', action) + ); + SQL + + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + index_archived_petitions_on_background + ON archived_petitions USING gin( + to_tsvector('english', background) + ); + SQL + + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + index_archived_petitions_on_additional_details + ON archived_petitions USING gin( + to_tsvector('english', additional_details) + ); + SQL + + execute "DROP INDEX CONCURRENTLY IF EXISTS index_petitions_for_search" + execute "ANALYZE petitions" + + execute "DROP INDEX CONCURRENTLY IF EXISTS index_archived_petitions_for_search" + execute "ANALYZE archived_petitions" + + execute "DROP EXTENSION IF EXISTS btree_gin" + end +end diff --git a/db/migrate/20210425143147_optimize_search_indexes_for_departments.rb b/db/migrate/20210425143147_optimize_search_indexes_for_departments.rb new file mode 100644 index 000000000..b03074fa1 --- /dev/null +++ b/db/migrate/20210425143147_optimize_search_indexes_for_departments.rb @@ -0,0 +1,21 @@ +class OptimizeSearchIndexesForDepartments < ActiveRecord::Migration[6.1] + disable_ddl_transaction! + + def up + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + index_departments_for_search + ON departments USING gin(( + to_tsvector('english', name::text) || + to_tsvector('english', COALESCE(acronym, '')::text) + )); + SQL + + execute "ANALYZE departments" + end + + def down + execute "DROP INDEX CONCURRENTLY IF EXISTS index_departments_for_search" + execute "ANALYZE departments" + end +end diff --git a/db/migrate/20210425143158_optimize_search_indexes_for_invalidations.rb b/db/migrate/20210425143158_optimize_search_indexes_for_invalidations.rb new file mode 100644 index 000000000..a0ad961af --- /dev/null +++ b/db/migrate/20210425143158_optimize_search_indexes_for_invalidations.rb @@ -0,0 +1,46 @@ +class OptimizeSearchIndexesForInvalidations < ActiveRecord::Migration[6.1] + disable_ddl_transaction! + + def up + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + index_invalidations_for_search + ON invalidations USING gin(( + to_tsvector('english', summary::text) || + to_tsvector('english', COALESCE(details)::text) || + to_tsvector('english', COALESCE(petition_id)::text) + )); + SQL + + execute "DROP INDEX CONCURRENTLY IF EXISTS ft_index_invalidations_on_details" + execute "DROP INDEX CONCURRENTLY IF EXISTS ft_index_invalidations_on_id" + execute "DROP INDEX CONCURRENTLY IF EXISTS ft_index_invalidations_on_petition_id" + execute "DROP INDEX CONCURRENTLY IF EXISTS ft_index_invalidations_on_summary" + execute "ANALYZE invalidations" + end + + def down + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS ft_index_invalidations_on_details + ON invalidations USING gin(to_tsvector('english', details::text)); + SQL + + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS ft_index_invalidations_on_id + ON invalidations USING gin(to_tsvector('english', id::text)); + SQL + + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS ft_index_invalidations_on_petition_id + ON invalidations USING gin(to_tsvector('english', petition_id::text)); + SQL + + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS ft_index_invalidations_on_summary + ON invalidations USING gin(to_tsvector('english', summary::text)); + SQL + + execute "DROP INDEX CONCURRENTLY IF EXISTS index_invalidations_for_search" + execute "ANALYZE invalidations" + end +end diff --git a/db/migrate/20210425143206_optimize_search_indexes_for_tags.rb b/db/migrate/20210425143206_optimize_search_indexes_for_tags.rb new file mode 100644 index 000000000..28dad7c8f --- /dev/null +++ b/db/migrate/20210425143206_optimize_search_indexes_for_tags.rb @@ -0,0 +1,33 @@ +class OptimizeSearchIndexesForTags < ActiveRecord::Migration[6.1] + disable_ddl_transaction! + + def up + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + index_tags_for_search + ON tags USING gin(( + to_tsvector('english', name::text) || + to_tsvector('english', COALESCE(description)::text) + )); + SQL + + execute "DROP INDEX CONCURRENTLY IF EXISTS index_ft_tags_on_name" + execute "DROP INDEX CONCURRENTLY IF EXISTS index_ft_tags_on_description" + execute "ANALYZE tags" + end + + def down + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS index_ft_tags_on_name + ON tags USING gin(to_tsvector('english', name)); + SQL + + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS index_ft_tags_on_description + ON tags USING gin(to_tsvector('english', description)); + SQL + + execute "DROP INDEX CONCURRENTLY IF EXISTS index_tags_for_search" + execute "ANALYZE tags" + end +end diff --git a/db/migrate/20210425143213_optimize_search_indexes_for_topics.rb b/db/migrate/20210425143213_optimize_search_indexes_for_topics.rb new file mode 100644 index 000000000..e97675b18 --- /dev/null +++ b/db/migrate/20210425143213_optimize_search_indexes_for_topics.rb @@ -0,0 +1,21 @@ +class OptimizeSearchIndexesForTopics < ActiveRecord::Migration[6.1] + disable_ddl_transaction! + + def up + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + index_topics_for_search + ON topics USING gin(( + to_tsvector('english', code::text) || + to_tsvector('english', name::text) + )); + SQL + + execute "ANALYZE topics" + end + + def down + execute "DROP INDEX CONCURRENTLY IF EXISTS index_topics_for_search" + execute "ANALYZE topics" + end +end diff --git a/db/migrate/20210425185707_add_name_index_to_departments.rb b/db/migrate/20210425185707_add_name_index_to_departments.rb new file mode 100644 index 000000000..300b99826 --- /dev/null +++ b/db/migrate/20210425185707_add_name_index_to_departments.rb @@ -0,0 +1,11 @@ +class AddNameIndexToDepartments < ActiveRecord::Migration[6.1] + disable_ddl_transaction! + + def up + add_index(:departments, :name, algorithm: :concurrently) + end + + def down + remove_index(:departments, :name, algorithm: :concurrently) + end +end diff --git a/db/schema.rb b/db/schema.rb index 11272852a..6d1c2d395 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,9 +10,10 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_04_21_214559) do +ActiveRecord::Schema.define(version: 2021_04_25_185707) do # These are extensions that must be enabled in order to support this database + enable_extension "btree_gin" enable_extension "intarray" enable_extension "plpgsql" @@ -152,9 +153,7 @@ t.datetime "anonymized_at" t.integer "moderated_by_id" t.integer "topics", default: [], null: false, array: true - t.index "to_tsvector('english'::regconfig, (action)::text)", name: "index_archived_petitions_on_action", using: :gin - t.index "to_tsvector('english'::regconfig, (background)::text)", name: "index_archived_petitions_on_background", using: :gin - t.index "to_tsvector('english'::regconfig, additional_details)", name: "index_archived_petitions_on_additional_details", using: :gin + t.index "((((to_tsvector('english'::regconfig, (id)::text) || to_tsvector('english'::regconfig, (action)::text)) || to_tsvector('english'::regconfig, (background)::text)) || to_tsvector('english'::regconfig, COALESCE(additional_details, ''::text)))), state, parliament_id, debate_state", name: "index_archived_petitions_for_search", using: :gin t.index ["anonymized_at"], name: "index_archived_petitions_on_anonymized_at" t.index ["debate_state", "parliament_id"], name: "index_archived_petitions_on_debate_state_and_parliament_id" t.index ["departments"], name: "index_archived_petitions_on_departments", opclass: :gin__int_ops, using: :gin @@ -312,6 +311,8 @@ t.date "end_date" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.index "((to_tsvector('english'::regconfig, (name)::text) || to_tsvector('english'::regconfig, (COALESCE(acronym, ''::character varying))::text)))", name: "index_departments_for_search", using: :gin + t.index ["name"], name: "index_departments_on_name" end create_table "dissolution_notifications", id: :uuid, default: nil, force: :cascade do |t| @@ -393,10 +394,7 @@ t.datetime "created_at", null: false t.datetime "updated_at", null: false t.string "domain", limit: 255 - t.index "to_tsvector('english'::regconfig, (details)::text)", name: "ft_index_invalidations_on_details", using: :gin - t.index "to_tsvector('english'::regconfig, (id)::text)", name: "ft_index_invalidations_on_id", using: :gin - t.index "to_tsvector('english'::regconfig, (petition_id)::text)", name: "ft_index_invalidations_on_petition_id", using: :gin - t.index "to_tsvector('english'::regconfig, (summary)::text)", name: "ft_index_invalidations_on_summary", using: :gin + t.index "(((to_tsvector('english'::regconfig, (summary)::text) || to_tsvector('english'::regconfig, (COALESCE(details))::text)) || to_tsvector('english'::regconfig, (COALESCE(petition_id))::text)))", name: "index_invalidations_for_search", using: :gin t.index ["cancelled_at"], name: "index_invalidations_on_cancelled_at" t.index ["completed_at"], name: "index_invalidations_on_completed_at" t.index ["petition_id"], name: "index_invalidations_on_petition_id" @@ -508,10 +506,8 @@ t.integer "moderated_by_id" t.integer "deadline_extension", default: 0, null: false t.integer "topics", default: [], null: false, array: true + t.index "((((to_tsvector('english'::regconfig, (id)::text) || to_tsvector('english'::regconfig, (action)::text)) || to_tsvector('english'::regconfig, (background)::text)) || to_tsvector('english'::regconfig, COALESCE(additional_details, ''::text)))), state, debate_state", name: "index_petitions_for_search", using: :gin t.index "((last_signed_at > signature_count_validated_at))", name: "index_petitions_on_validated_at_and_signed_at" - t.index "to_tsvector('english'::regconfig, (action)::text)", name: "index_petitions_on_action", using: :gin - t.index "to_tsvector('english'::regconfig, (background)::text)", name: "index_petitions_on_background", using: :gin - t.index "to_tsvector('english'::regconfig, additional_details)", name: "index_petitions_on_additional_details", using: :gin t.index ["anonymized_at"], name: "index_petitions_on_anonymized_at" t.index ["archived_at"], name: "index_petitions_on_archived_at" t.index ["created_at", "state"], name: "index_petitions_on_created_at_and_state" @@ -680,8 +676,7 @@ t.string "description", limit: 200 t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.index "to_tsvector('english'::regconfig, (description)::text)", name: "index_ft_tags_on_description", using: :gin - t.index "to_tsvector('english'::regconfig, (name)::text)", name: "index_ft_tags_on_name", using: :gin + t.index "((to_tsvector('english'::regconfig, (name)::text) || to_tsvector('english'::regconfig, (COALESCE(description))::text)))", name: "index_tags_for_search", using: :gin t.index ["name"], name: "index_tags_on_name", unique: true end @@ -697,6 +692,7 @@ t.string "name", limit: 100, null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.index "((to_tsvector('english'::regconfig, (code)::text) || to_tsvector('english'::regconfig, (name)::text)))", name: "index_topics_for_search", using: :gin t.index ["code"], name: "index_topics_on_code", unique: true t.index ["name"], name: "index_topics_on_name", unique: true end diff --git a/spec/models/concerns/browseable_spec.rb b/spec/models/concerns/browseable_spec.rb index 5653fb58c..b0ca727ed 100644 --- a/spec/models/concerns/browseable_spec.rb +++ b/spec/models/concerns/browseable_spec.rb @@ -36,6 +36,23 @@ def self.all end end + describe ".query" do + it "adds column to search on" do + browseable.query(:name) + expect(browseable.query_columns).to eq([Browseable::Query::Column.new("name", "english", false)]) + end + + it "allows overriding the search configuration" do + browseable.query(:name, config: "simple") + expect(browseable.query_columns).to eq([Browseable::Query::Column.new("name", "simple", false)]) + end + + it "allows overriding whether the column accepts nulls" do + browseable.query(:name, null: true) + expect(browseable.query_columns).to eq([Browseable::Query::Column.new("name", "english", true)]) + end + end + describe ".search" do let(:params) { {} } let(:search) { browseable.search(params) } @@ -48,10 +65,13 @@ def self.all describe Browseable::Search do let(:scopes) { { all: -> { self }, open: -> { self } } } let(:filters) { {} } - let(:klass) { double(:klass, facet_definitions: scopes, filter_definitions: filters, default_page_size: 50, max_page_size: 50) } + let(:columns) { [Browseable::Query::Column.new('action', 'english', false)] } + let(:klass) { double(:klass, facet_definitions: scopes, filter_definitions: filters, query_columns: columns, default_page_size: 50, max_page_size: 50) } let(:params) { { q: 'search', page: '3'} } let(:search) { described_class.new(klass, params) } + let(:connection) { double(:connection) } + it "is enumerable" do expect(search).to respond_to(:each) end @@ -166,16 +186,14 @@ def self.all let(:petitions) { [petition] } let(:results) { double(:results, to_a: petitions) } + let(:sql) { %[((to_tsvector('english', "petitions"."action"::text)) @@ plainto_tsquery('english', :query))] } + before do - allow(klass).to receive(:basic_search).with('search').and_return(klass) - allow(arel_table).to receive(:[]).with("*").and_return("*") - allow(klass).to receive(:basic_search).with('search').and_return(klass) - allow(klass).to receive(:except).with(:select).and_return(klass) - allow(klass).to receive(:arel_table).and_return(arel_table) - allow(klass).to receive(:select).with("*").and_return(klass) - allow(klass).to receive(:except).with(:order).and_return(klass) + allow(klass).to receive(:quoted_table_name).and_return('"petitions"') + allow(klass).to receive(:connection).and_return(connection) + allow(connection).to receive(:quote_column_name).with('action').and_return('"action"') + allow(klass).to receive(:where).with([sql, query: "search"]).and_return(klass) allow(klass).to receive(:paginate).with(page: 3, per_page: 50).and_return(results) - allow(results).to receive(:to_a).and_return(petitions) allow(results).to receive(:previous_page).and_return(2) end @@ -214,16 +232,14 @@ def self.all let(:petitions) { [petition] } let(:results) { double(:results, to_a: petitions) } + let(:sql) { %[((to_tsvector('english', "petitions"."action"::text)) @@ plainto_tsquery('english', :query))] } + before do - allow(klass).to receive(:basic_search).with('search').and_return(klass) - allow(arel_table).to receive(:[]).with("*").and_return("*") - allow(klass).to receive(:basic_search).with('search').and_return(klass) - allow(klass).to receive(:except).with(:select).and_return(klass) - allow(klass).to receive(:arel_table).and_return(arel_table) - allow(klass).to receive(:select).with("*").and_return(klass) - allow(klass).to receive(:except).with(:order).and_return(klass) + allow(klass).to receive(:quoted_table_name).and_return('"petitions"') + allow(klass).to receive(:connection).and_return(connection) + allow(connection).to receive(:quote_column_name).with('action').and_return('"action"') + allow(klass).to receive(:where).with([sql, query: "search"]).and_return(klass) allow(klass).to receive(:paginate).with(page: 3, per_page: 50).and_return(results) - allow(results).to receive(:to_a).and_return(petitions) allow(results).to receive(:next_page).and_return(4) end @@ -376,20 +392,15 @@ def self.all let(:petitions) { [petition] } let(:results) { double(:results, to_a: petitions) } + let(:sql) { %[((to_tsvector('english', "petitions"."action"::text)) @@ plainto_tsquery('english', :query))] } + context "when there is a search term" do before do - # This list of stubs is effectively testing the implementation of the - # execute_search private method, however this is important because of - # the need to exclude the ranking column added by the textacular gem - # which can add a significant performance penalty. - - expect(arel_table).to receive(:[]).with("*").and_return("*") - expect(klass).to receive(:basic_search).with('search').and_return(klass) - expect(klass).to receive(:except).with(:select).and_return(klass) - expect(klass).to receive(:arel_table).and_return(arel_table) - expect(klass).to receive(:select).with("*").and_return(klass) - expect(klass).to receive(:except).with(:order).and_return(klass) - expect(klass).to receive(:paginate).with(page: 3, per_page: 50).and_return(results) + allow(klass).to receive(:quoted_table_name).and_return('"petitions"') + allow(klass).to receive(:connection).and_return(connection) + allow(connection).to receive(:quote_column_name).with('action').and_return('"action"') + allow(klass).to receive(:where).with([sql, query: "search"]).and_return(klass) + allow(klass).to receive(:paginate).with(page: 3, per_page: 50).and_return(results) expect(results).to receive(:to_a).and_return(petitions) end @@ -579,4 +590,95 @@ class BatchifiedArray < Array end end end + + describe Browseable::Query do + let(:klass) { double(:klass, query_columns: query_columns) } + let(:query_columns) { [] } + + subject { described_class.new(klass, "search") } + + it { is_expected.to delegate_method(:query_columns).to(:klass) } + it { is_expected.to delegate_method(:connection).to(:klass) } + it { is_expected.to delegate_method(:quoted_table_name).to(:klass) } + it { is_expected.to delegate_method(:quote_column_name).to(:connection) } + it { is_expected.to delegate_method(:inspect).to(:to_s) } + it { is_expected.to delegate_method(:present?).to(:to_s) } + + describe "#build" do + let(:connection) { double(:connection) } + + before do + allow(klass).to receive(:quoted_table_name).and_return('"petitions"') + allow(klass).to receive(:connection).and_return(connection) + allow(connection).to receive(:quote_column_name).with('id').and_return('"id"') + allow(connection).to receive(:quote_column_name).with('action').and_return('"action"') + allow(connection).to receive(:quote_column_name).with('background').and_return('"background"') + allow(connection).to receive(:quote_column_name).with('additional_details').and_return('"additional_details"') + end + + context "when there is one column" do + let(:query_columns) do + [ Browseable::Query::Column.new("action", "english", false) ] + end + + it "returns a where condition" do + expect(subject.build).to eq [ + %[((to_tsvector('english', "petitions"."action"::text)) @@ plainto_tsquery('english', :query))], { query: "search" } + ] + end + + context "and the column has a custom search configuration" do + let(:query_columns) do + [ Browseable::Query::Column.new("action", "simple", false) ] + end + + it "returns a where condition" do + expect(subject.build).to eq [ + %[((to_tsvector('simple', "petitions"."action"::text)) @@ plainto_tsquery('english', :query))], { query: "search" } + ] + end + end + + context "and the column allows nulls" do + let(:query_columns) do + [ Browseable::Query::Column.new("action", "english", true) ] + end + + it "returns a where condition" do + expect(subject.build).to eq [ + %[((to_tsvector('english', COALESCE("petitions"."action", '')::text)) @@ plainto_tsquery('english', :query))], { query: "search" } + ] + end + end + end + + context "when there is more than one column" do + let(:query_columns) do + [ + Browseable::Query::Column.new("id", "simple", false), + Browseable::Query::Column.new("action", "english", false), + Browseable::Query::Column.new("background", "english", false), + Browseable::Query::Column.new("additional_details", "english", true) + ] + end + + it "returns a where condition" do + expect(subject.build).to eq [ + %Q[((#{[ + %[to_tsvector('simple', "petitions"."id"::text)], + %[to_tsvector('english', "petitions"."action"::text)], + %[to_tsvector('english', "petitions"."background"::text)], + %[to_tsvector('english', COALESCE("petitions"."additional_details", '')::text)] + ].join(' || ')}) @@ plainto_tsquery('english', :query))], { query: "search" } + ] + end + end + end + + describe "#to_s" do + it "returns the query param" do + expect(subject.to_s).to eq("search") + end + end + end end