diff --git a/app/extensions/alchemy/pg_search/ingredient_extension.rb b/app/extensions/alchemy/pg_search/ingredient_extension.rb deleted file mode 100644 index 2c377f0..0000000 --- a/app/extensions/alchemy/pg_search/ingredient_extension.rb +++ /dev/null @@ -1,30 +0,0 @@ -module Alchemy::PgSearch::IngredientExtension - - def self.multisearch_config - { - against: [ - :value, - ], - additional_attributes: ->(ingredient) { { page_id: ingredient.element.page.id } }, - if: :searchable? - } - end - - def self.prepended(base) - base.include PgSearch::Model - base.multisearchable(multisearch_config) - end - - def searchable? - Alchemy::PgSearch.is_searchable?(type) && - (definition.key?(:searchable) ? definition[:searchable] : true) && - value.present? && !!element&.searchable? - end -end - -# add the PgSearch model to all ingredients -Alchemy::Ingredient.prepend(Alchemy::PgSearch::IngredientExtension) - -# add custom content fields for Richtext, and Picture -Alchemy::Ingredients::Picture.multisearchable(Alchemy::PgSearch::IngredientExtension.multisearch_config.merge({against: [:caption]})) -Alchemy::Ingredients::Richtext.multisearchable(Alchemy::PgSearch::IngredientExtension.multisearch_config.merge({against: [:stripped_body]})) diff --git a/app/extensions/alchemy/pg_search/pg_search_document_extension.rb b/app/extensions/alchemy/pg_search/pg_search_document_extension.rb index e7cf73c..0b0e96f 100644 --- a/app/extensions/alchemy/pg_search/pg_search_document_extension.rb +++ b/app/extensions/alchemy/pg_search/pg_search_document_extension.rb @@ -2,20 +2,6 @@ module Alchemy::PgSearch::PgSearchDocumentExtension def self.prepended(base) base.belongs_to :page, class_name: "::Alchemy::Page", foreign_key: "page_id", optional: true end - - ## - # get a list of excerpts of the searched phrase - # The JSON_AGG - method will transform the grouped content entries into json which have to be "unpacked". - # @return [array] - def excerpts - return [] if content.blank? - begin - parsed_content = JSON.parse content - parsed_content.kind_of?(Array) ? parsed_content : [] - rescue JSON::ParserError - [] - end - end end PgSearch::Document.prepend(Alchemy::PgSearch::PgSearchDocumentExtension) diff --git a/app/extensions/alchemy/pg_search/element_extension.rb b/app/extensions/alchemy/search/element_extension.rb similarity index 70% rename from app/extensions/alchemy/pg_search/element_extension.rb rename to app/extensions/alchemy/search/element_extension.rb index 0ef3823..85ff1fc 100644 --- a/app/extensions/alchemy/pg_search/element_extension.rb +++ b/app/extensions/alchemy/search/element_extension.rb @@ -1,4 +1,4 @@ -module Alchemy::PgSearch::ElementExtension +module Alchemy::Search::ElementExtension def self.prepended(base) base.attr_writer :searchable end @@ -12,4 +12,4 @@ def searchable? end end -Alchemy::Element.prepend(Alchemy::PgSearch::ElementExtension) +Alchemy::Element.prepend(Alchemy::Search::ElementExtension) diff --git a/app/extensions/alchemy/search/ingredient_extension.rb b/app/extensions/alchemy/search/ingredient_extension.rb new file mode 100644 index 0000000..b74c593 --- /dev/null +++ b/app/extensions/alchemy/search/ingredient_extension.rb @@ -0,0 +1,14 @@ +module Alchemy::Search::IngredientExtension + def searchable_content + send(Alchemy.searchable_ingredients[type.to_sym])&.squish + end + + def searchable? + Alchemy.searchable_ingredients.has_key?(type.to_sym) && + (definition.key?(:searchable) ? definition[:searchable] : true) && + !!element&.searchable? + end +end + +# add the PgSearch model to all ingredients +Alchemy::Ingredient.prepend(Alchemy::Search::IngredientExtension) diff --git a/app/extensions/alchemy/search/page_extension.rb b/app/extensions/alchemy/search/page_extension.rb new file mode 100644 index 0000000..b59dc5b --- /dev/null +++ b/app/extensions/alchemy/search/page_extension.rb @@ -0,0 +1,10 @@ +# Enable Postgresql full text indexing. +# +module Alchemy::Search::PageExtension + def searchable? + (definition.key?(:searchable) ? definition[:searchable] : true) && + searchable && public? && !layoutpage? + end +end + +Alchemy::Page.prepend(Alchemy::Search::PageExtension) diff --git a/app/views/alchemy/search/_result.html.erb b/app/views/alchemy/search/_result.html.erb index f7bea6f..923783c 100644 --- a/app/views/alchemy/search/_result.html.erb +++ b/app/views/alchemy/search/_result.html.erb @@ -1,12 +1,6 @@
  • - <% page = result.page %> + <% page = result.searchable %>

    <%= link_to page.name, show_alchemy_page_path(page) %>

    - <% if result.excerpts.any? %> - <% result.excerpts.each do |excerpt| %> -

    <%= highlighted_excerpt(excerpt, params[:query]) %>

    - <% end %> - <% else %> -

    <%= page.meta_description %>

    - <% end %> +

    <%= highlighted_excerpt(result.content, params[:query]) %>

    <%= link_to page.urlname, show_alchemy_page_path(page) %>

    • diff --git a/lib/alchemy-pg_search.rb b/lib/alchemy-pg_search.rb index 2e79354..bfee160 100644 --- a/lib/alchemy-pg_search.rb +++ b/lib/alchemy-pg_search.rb @@ -5,24 +5,22 @@ module Alchemy mattr_accessor :search_class @@search_class = PgSearch - module PgSearch - SEARCHABLE_INGREDIENTS = %w[Text Richtext Picture] + mattr_accessor :searchable_ingredients + @@searchable_ingredients = { + "Alchemy::Ingredients::Text": :value, + "Alchemy::Ingredients::Richtext": :stripped_body, + "Alchemy::Ingredients::Picture": :caption, + } + module PgSearch extend Config ## - # is ingredient searchable? - # @param ingredient_type [string] - # @return [boolean] - def self.is_searchable?(ingredient_type) - SEARCHABLE_INGREDIENTS.include?(ingredient_type.gsub(/Alchemy::Ingredients::/, "")) - end - - ## - # index all supported Alchemy models + # index all supported Alchemy pages def self.rebuild - [Alchemy::Page, Alchemy::Ingredient].each do |model| - ::PgSearch::Multisearch.rebuild(model) + ActiveRecord::Base.transaction do + ::PgSearch::Document.delete_all + Alchemy::Page.all.each{ |page| index_page(page) } end end @@ -39,14 +37,16 @@ def self.remove_page(page) # # @param page [Alchemy::Page] def self.index_page(page) - remove_page page - page.update_pg_search_document - page.all_elements.includes(:ingredients).find_each do |element| - element.ingredients.select { |i| Alchemy::PgSearch.is_searchable?(i.type) }.each do |ingredient| - ingredient.update_pg_search_document - end - end + + document = page.pg_search_document + return if document.nil? + + ingredient_content = page.all_elements.includes(ingredients: {element: :page}).map do |element| + element.ingredients.select { |i| i.searchable? }.map(&:searchable_content).join(" ") + end.join(" ") + + document.update_column(:content, "#{document.content} #{ingredient_content}".squish) end ## @@ -56,13 +56,17 @@ def self.index_page(page) # @param ability [nil|CanCan::Ability] # @return [ActiveRecord::Relation] def self.search(query, ability: nil) - query = ::PgSearch.multisearch(query) - .select("JSON_AGG(content) as content", :page_id) - .reorder("") - .group(:page_id) - .joins(:page) + query = ::PgSearch.multisearch(query).includes(:searchable) - query = query.merge(Alchemy::Page.accessible_by(ability, :read)) if ability + if ability + # left_joins method is not usable here, because the order of the joins are incorrect + # and would result in a SQL error. We can receive the correct query order with these + # odd left join string + # Ref: https://guides.rubyonrails.org/active_record_querying.html#using-a-string-sql-fragment + query = query + .joins("LEFT JOIN alchemy_pages ON alchemy_pages.id = pg_search_documents.page_id") + .merge(Alchemy::Page.accessible_by(ability, :read)) + end query end diff --git a/spec/dummy/config/alchemy/elements.yml b/spec/dummy/config/alchemy/elements.yml index 90581b1..5d7dd84 100644 --- a/spec/dummy/config/alchemy/elements.yml +++ b/spec/dummy/config/alchemy/elements.yml @@ -34,11 +34,11 @@ searchable: false - role: public type: Richtext - default: "This is some public text." + default: "This is some public richtext." - role: confidential type: Richtext searchable: false - default: "This is some confidential text." + default: "This is some confidential richtext." - role: image type: Picture - role: secret_image diff --git a/spec/dummy/config/alchemy/page_layouts.yml b/spec/dummy/config/alchemy/page_layouts.yml index f30c0f6..8fc755f 100644 --- a/spec/dummy/config/alchemy/page_layouts.yml +++ b/spec/dummy/config/alchemy/page_layouts.yml @@ -6,6 +6,12 @@ - article - secrets +- name: mixed + elements: + - mixed + autogenerate: + - mixed + - name: search searchresults: true unique: true diff --git a/spec/features/fulltext_search_feature_spec.rb b/spec/features/fulltext_search_feature_spec.rb index 43e48c4..c47ccf4 100644 --- a/spec/features/fulltext_search_feature_spec.rb +++ b/spec/features/fulltext_search_feature_spec.rb @@ -21,15 +21,19 @@ image.save end + before do + Alchemy::PgSearch.rebuild + end + it "displays search results from text ingredients" do - visit("/suche?query=search") + visit("/suche?query=headline") within(".search_results") do expect(page).to have_content("This is a headline everybody should be able to search for.") end end - it "displays search results from richtext essences" do - visit("/suche?query=search") + it "displays search results from richtext ingredient" do + visit("/suche?query=text%20block") within(".search_results") do expect(page).to have_content("This is a text block everybody should be able to search for.") end @@ -59,7 +63,6 @@ it "does not display results placed on global pages" do # A layout page is configured and the page is indexed after publish public_page.update!(layoutpage: true) - Alchemy::PgSearch.index_page public_page visit("/suche?query=search") expect(page).to have_css("h2.no_search_results") @@ -131,6 +134,7 @@ before do nested_element.ingredient_by_role("headline").update!({ value: "Content from nested element" }) + Alchemy::PgSearch.rebuild end it "displays search results from nested elements" do @@ -184,6 +188,7 @@ page_version: create(:alchemy_page, :public).public_version, ) end + Alchemy::PgSearch.rebuild end context "when default config is used" do diff --git a/spec/lib/alchemy-pg_search_spec.rb b/spec/lib/alchemy-pg_search_spec.rb index 1ae1c1d..a8339ad 100644 --- a/spec/lib/alchemy-pg_search_spec.rb +++ b/spec/lib/alchemy-pg_search_spec.rb @@ -1,187 +1,105 @@ require 'spec_helper' describe Alchemy::PgSearch do - let(:page_version) { create(:alchemy_page_version, :published) } - let(:ingredient_element) { create(:alchemy_element, :with_ingredients, name: "ingredient_test", public: true, page_version: page_version) } - - let(:prepared_ingredients) do - { :ingredient_text => :value, :ingredient_richtext => :value, :ingredient_picture => :value }.each do |ingredient_name, field| - ingredient = ingredient_element.ingredient_by_role(ingredient_name) - ingredient[field] = "foo" - ingredient.save + let!(:first_page) { create(:alchemy_page, :public) } + let!(:second_page) { create(:alchemy_page, :public) } + + describe '#rebuild' do + subject { described_class.rebuild } + + it 'should have both created pages indexed documents' do + expect(PgSearch::Document.count).to be(2) end - end - let(:first_page) { Alchemy::Page.first } - let(:second_page) { Alchemy::Page.last } - - context 'rebuild' do - it 'should have zero indexed documents' do - expect(PgSearch::Document.count).to be(0) - end - - context 'after rebuild' do - before do - prepared_ingredients - Alchemy::PgSearch.rebuild - end - - it 'should have entries (2 Pages + 3 Ingredients)' do - expect(PgSearch::Document.count).to eq(5) - end - - it "should have three ingredients" do - expect(PgSearch::Document.where(searchable_type: "Alchemy::Ingredient").count).to eq(3) - end + it "has 3 Pages (Root Page + 2 created pages)" do + subject + expect(PgSearch::Document.count).to be(3) end end - context 'remove_page' do - before do - prepared_ingredients - Alchemy::PgSearch.rebuild - end - - context 'remove first page' do - before { Alchemy::PgSearch.remove_page first_page } - - it 'should have only one page and relative ingredients (1 Page + 3 Ingredients)' do - expect(PgSearch::Document.count).to eq(4) - end - - it 'should have one page entry' do - expect(PgSearch::Document.where(searchable_type: "Alchemy::Page").count).to eq(1) - end - end - - context 'remove second page' do - before { Alchemy::PgSearch.remove_page second_page } + describe "#remove_page" do + subject { described_class.remove_page(first_page) } - it 'should have only one page (1 Page)' do - expect(PgSearch::Document.count).to eq(1) - end - - it 'should have one page entry' do - expect(PgSearch::Document.where(searchable_type: "Alchemy::Page").count).to eq(1) - end + it "should remove the page from search index" do + expect { subject }.to change { PgSearch::Document.count }.by(-1) + expect(first_page.reload.pg_search_document).to be_nil end end - context 'index_page' do + describe "#index_page" do + let!(:first_page) { create(:alchemy_page, :public, name: "Mixed", page_layout: "mixed", autogenerate_elements: true) } + let(:content) { first_page.pg_search_document.content } before do - prepared_ingredients PgSearch::Document.destroy_all # clean the whole index end - it 'should have zero indexed documents' do - expect(PgSearch::Document.count).to be(0) - end - - context 'first_page' do - before do - Alchemy::PgSearch.index_page first_page - end + subject { described_class.index_page(first_page.reload) } - it 'should have only one entry' do - expect(PgSearch::Document.count).to be(1) - end - - it 'should be the first page' do - expect(PgSearch::Document.first.page_id).to be(first_page.id) - end + it "creates a new pg_search document" do + expect { subject }.to change { PgSearch::Document.count }.by(1) end - context 'second_page' do - before do - Alchemy::PgSearch.index_page second_page - end - - it 'should have four entries (1 Page + 3 Ingredients)' do - expect(PgSearch::Document.count).to be(4) - end - - it 'should be all relate to the same page ' do - PgSearch::Document.all.each do |document| - expect(document.page_id).to be(second_page.id) - end - end + it "has the page title as content" do + subject + expect(content).to include("Mixed ") end - context 'nested elements' do - let!(:nested_element) { create(:alchemy_element, :with_ingredients, name: "article", public: true, page_version: page_version, parent_element: ingredient_element) } - - before do - Alchemy::PgSearch.index_page second_page - end - - it 'should have 6 documents' do - # 1 Page + 3 previous ingredients + 2 new article ingredients - expect(PgSearch::Document.count).to be(6) - end - - it 'should be all relate to the same page ' do - PgSearch::Document.all.each do |document| - expect(document.page_id).to be(second_page.id) - end - end + it "has ingredient as content" do + subject + expect(content).to include("public title") end - context 'page searchable' do - let(:searchable) { true } - let!(:page) { create(:alchemy_page, :public, name: "Searchable Page", searchable: searchable) } - let(:result) { Alchemy::PgSearch.search "searchable" } + it "hasn't not searchable ingredient in content" do + subject + expect(content).to_not include("secret password") + end - before do - Alchemy::PgSearch.rebuild - end + it "stores stripped content from ingredient" do + subject + expect(content).to include("public richtext") + end - it 'should find one page' do - expect(result.length).to eq(1) - end + it "removes whitespace from content" do + subject + expect(content).to start_with("Mixed") + end - context 'searchable disabled' do - let(:searchable) { false } + context "hidden page" do + let!(:first_page) { create(:alchemy_page) } - it 'should not find any page' do - expect(result.length).to eq(0) - end + it "creates nothing" do + expect { subject }.to change { PgSearch::Document.count }.by(0) end end end context 'search' do - let(:result) { Alchemy::PgSearch.search "foo" } - - before do - create(:alchemy_page, :restricted, :public, name: "foo") - prepared_ingredients - Alchemy::PgSearch.rebuild - end + let!(:third_page) { create(:alchemy_page, :restricted, :public, name: "Third Page") } + let(:ability) { nil } + + subject(:result) { Alchemy::PgSearch.search "page", ability: } - it 'should find two pages' do - expect(result.length).to eq(2) + it 'should find three pages' do + expect(result.length).to eq(3) end context 'ability' do let(:user) { User.create(alchemy_roles: ["member"]) } - let(:result) { Alchemy::PgSearch.search "foo", ability: Alchemy::Permissions.new(user) } + let(:ability) { Alchemy::Permissions.new(user) } context 'with a logged in user' do - it 'should find two pages' do - expect(result.length).to eq(2) + it 'should find the restricted page' do + expect(result.length).to eq(3) + expect(result.last.page).to eq(third_page) end end context 'with an unknown user' do let(:user) { User.create(alchemy_roles: []) } - it 'should find one page' do - expect(result.length).to eq(1) - end - - it 'should find only the second page' do - expect(result.take.page).to eq(second_page) + it 'should find two pages' do + expect(result.length).to eq(2) end end end diff --git a/spec/models/ingredient_spec.rb b/spec/models/ingredient_spec.rb index 41fa0c0..a51f13b 100644 --- a/spec/models/ingredient_spec.rb +++ b/spec/models/ingredient_spec.rb @@ -1,6 +1,6 @@ require "spec_helper" -RSpec.shared_examples_for "it is searchable" do +RSpec.shared_examples_for "it is searchable" do |attribute| describe "searchable?" do subject { ingredient.searchable? } @@ -20,13 +20,6 @@ end end - context "ingredient has no content" do - it "should be not searchable" do - ingredient.value = nil - expect(ingredient.searchable?).to be(false) - end - end - context "element is not public" do it "should be not searchable" do element.public = false @@ -44,43 +37,16 @@ end end -RSpec.shared_examples_for "it is in search index" do - describe "search index" do - let(:document) { PgSearch::Document.first } - - subject do - ingredient - ::PgSearch::Multisearch.rebuild described_class - end - - it "should have one entry" do - subject - expect(PgSearch::Document.where(searchable_type: "Alchemy::Ingredient").count).to eq(1) - end - - it "should have the content" do - subject - expect(PgSearch::Document.first.content).to eq(content) - end - - context "configured as not searchable" do - before do - allow_any_instance_of(ingredient.class).to receive(:definition).at_least(:once) do - { - searchable: false, - } - end - end +RSpec.shared_examples_for "a searchable content" do + it "has searchable content" do + expect(ingredient.searchable_content).to eq("foo bar") + end - it "should have no index entry" do - subject - expect(PgSearch::Document.where(searchable_type: "Alchemy::Ingredient").count).to eq(0) - end - end + context "whitespaces" do + let(:content) { " foo\n bar "} - it "should be the current ingredient" do - subject - expect(document.searchable).to eq(ingredient) + it "has removed unnecessary whitespaces" do + expect(ingredient.searchable_content).to eq("foo bar") end end end @@ -91,26 +57,26 @@ create(:alchemy_element, :with_ingredients, name: "ingredient_test", public: true, page_version: page_version) end - let(:content) { "foo bar"} + let(:content) { "foo bar" } describe Alchemy::Ingredients::Text do let(:ingredient) { create(:alchemy_ingredient_text, value: content, element: element) } - it_behaves_like "it is searchable" - it_behaves_like "it is in search index" + it_behaves_like "it is searchable", field: :value + it_behaves_like "a searchable content" end describe Alchemy::Ingredients::Richtext do let(:ingredient) { create(:alchemy_ingredient_richtext, value: "foo bar", element: element) } - it_behaves_like "it is searchable" - it_behaves_like "it is in search index" + it_behaves_like "it is searchable", field: :stripped_body + it_behaves_like "a searchable content" end describe Alchemy::Ingredients::Picture do let(:ingredient) { create(:alchemy_ingredient_picture, value: create(:alchemy_picture), caption: content, element: element) } - it_behaves_like "it is searchable" - it_behaves_like "it is in search index" + it_behaves_like "it is searchable", field: :caption + it_behaves_like "a searchable content" end end diff --git a/spec/models/pg_search_document_spec.rb b/spec/models/pg_search_document_spec.rb index 5c997bb..e0db0c9 100644 --- a/spec/models/pg_search_document_spec.rb +++ b/spec/models/pg_search_document_spec.rb @@ -17,26 +17,4 @@ end end end - - describe "excerpts" do - it "should be empty if the content is nil" do - expect(document.excerpts).to eq([]) - end - - context "with no json" do - let(:document) { PgSearch::Document.new(content: "123") } - - it "should be empty if the content is a not valid json" do - expect(document.excerpts).to eq([]) - end - end - - context "with valid json" do - let(:document) { PgSearch::Document.new(content: '["123", "456"]') } - - it "should have an array of the json content" do - expect(document.excerpts).to eq(%w[123 456]) - end - end - end end