From 8d7b95ea15cb388d413b23cbf142bbfed74047aa Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 19 Feb 2020 06:55:38 -0800 Subject: [PATCH] Index ClassifiedListings to Elasticsearch (#6132) [deploy] * Add ClassifiedListingEsIndexWorker & spec * Update ClassifiedListing model - Add es index methods (inline and async) - Index ClassifiedListing to es after_commit - Add serialized_search_hash - Update model specs for new methods - Add serializers referenced in new model method * Update specs * Create DataUpdateScript to index ClassifiedListings * Fix formatting * Fix codeclimate :/ * Use match_array instead of keys.sort * Change find_by! to find * Update spec * Update mappings for ClassifiedListing * Update specs * Implement refactor with Searchable module --- app/models/classified_listing.rb | 6 +++ .../classified_listing_author_serializer.rb | 7 +++ .../search/classified_listing_serializer.rb | 25 +++++++++++ .../classified_listing_es_index_worker.rb | 12 +++++ .../mappings/classified_listings.json | 12 ++++- ...ex_classified_listings_to_elasticsearch.rb | 9 ++++ ...assified_listings_to_elasticsearch_spec.rb | 11 +++++ spec/models/classified_listing_spec.rb | 45 +++++++++++++++++++ .../search/classified_listing_spec.rb | 13 ++++-- ...classified_listing_es_index_worker_spec.rb | 24 ++++++++++ 10 files changed, 158 insertions(+), 6 deletions(-) create mode 100644 app/serializers/search/classified_listing_author_serializer.rb create mode 100644 app/serializers/search/classified_listing_serializer.rb create mode 100644 app/workers/search/classified_listing_es_index_worker.rb create mode 100644 lib/data_update_scripts/20200217215802_index_classified_listings_to_elasticsearch.rb create mode 100644 spec/lib/data_update_scripts/index_classified_listings_to_elasticsearch_spec.rb create mode 100644 spec/workers/search/classified_listing_es_index_worker_spec.rb diff --git a/app/models/classified_listing.rb b/app/models/classified_listing.rb index 5e1ededf4..54af4c6c0 100644 --- a/app/models/classified_listing.rb +++ b/app/models/classified_listing.rb @@ -1,5 +1,10 @@ class ClassifiedListing < ApplicationRecord include AlgoliaSearch + include Searchable + + SEARCH_INDEX_WORKER = Search::ClassifiedListingEsIndexWorker + SEARCH_SERIALIZER = Search::ClassifiedListingSerializer + SEARCH_CLASS = Search::ClassifiedListing CATEGORIES_AVAILABLE = { cfp: { cost: 1, name: "Conference CFP", rules: "Currently open for proposals, with link to form." }, @@ -22,6 +27,7 @@ class ClassifiedListing < ApplicationRecord before_save :evaluate_markdown before_create :create_slug before_validation :modify_inputs + after_commit :index_to_elasticsearch acts_as_taggable_on :tags has_many :credits, as: :purchase, inverse_of: :purchase, dependent: :nullify diff --git a/app/serializers/search/classified_listing_author_serializer.rb b/app/serializers/search/classified_listing_author_serializer.rb new file mode 100644 index 000000000..2c20e4428 --- /dev/null +++ b/app/serializers/search/classified_listing_author_serializer.rb @@ -0,0 +1,7 @@ +module Search + class ClassifiedListingAuthorSerializer + include FastJsonapi::ObjectSerializer + + attributes :username, :name, :profile_image_90 + end +end diff --git a/app/serializers/search/classified_listing_serializer.rb b/app/serializers/search/classified_listing_serializer.rb new file mode 100644 index 000000000..a5b39bc3d --- /dev/null +++ b/app/serializers/search/classified_listing_serializer.rb @@ -0,0 +1,25 @@ +module Search + class ClassifiedListingSerializer + include FastJsonapi::ObjectSerializer + + attributes :id, + :body_markdown, + :bumped_at, + :category, + :contact_via_connect, + :expires_at, + :location, + :processed_html, + :slug, + :title, + :user_id + + attribute :tags, &:tag_list + + attribute :author do |cl| + ClassifiedListingAuthorSerializer.new(cl.author). + serializable_hash. + dig(:data, :attributes) + end + end +end diff --git a/app/workers/search/classified_listing_es_index_worker.rb b/app/workers/search/classified_listing_es_index_worker.rb new file mode 100644 index 000000000..f660291da --- /dev/null +++ b/app/workers/search/classified_listing_es_index_worker.rb @@ -0,0 +1,12 @@ +module Search + class ClassifiedListingEsIndexWorker + include Sidekiq::Worker + + sidekiq_options queue: :high_priority + + def perform(classified_listing_id) + classified_listing = ::ClassifiedListing.find(classified_listing_id) + classified_listing.index_to_elasticsearch_inline + end + end +end diff --git a/config/elasticsearch/mappings/classified_listings.json b/config/elasticsearch/mappings/classified_listings.json index 7dd7c1c11..bbe14c567 100644 --- a/config/elasticsearch/mappings/classified_listings.json +++ b/config/elasticsearch/mappings/classified_listings.json @@ -19,7 +19,8 @@ } }, "body_markdown": { - "type": "text" + "type": "text", + "copy_to": "classified_listing_search" }, "bumped_at": { "type": "date" @@ -27,6 +28,9 @@ "category": { "type": "keyword" }, + "classified_listing_search": { + "type": "text" + }, "contact_via_connect": { "type": "boolean" }, @@ -35,6 +39,7 @@ }, "location": { "type": "text", + "copy_to": "classified_listing_search", "fields": { "raw": { "type": "keyword" @@ -46,6 +51,7 @@ }, "slug": { "type": "text", + "copy_to": "classified_listing_search", "fields": { "raw": { "type": "keyword" @@ -53,10 +59,12 @@ } }, "tags": { - "type": "keyword" + "type": "keyword", + "copy_to": "classified_listing_search" }, "title": { "type": "text", + "copy_to": "classified_listing_search", "fields": { "raw": { "type": "keyword" diff --git a/lib/data_update_scripts/20200217215802_index_classified_listings_to_elasticsearch.rb b/lib/data_update_scripts/20200217215802_index_classified_listings_to_elasticsearch.rb new file mode 100644 index 000000000..e652454e3 --- /dev/null +++ b/lib/data_update_scripts/20200217215802_index_classified_listings_to_elasticsearch.rb @@ -0,0 +1,9 @@ +module DataUpdateScripts + class IndexClassifiedListingsToElasticsearch + def run + # Choose to do inline so development envs are ready immediately after + # this is run + ClassifiedListing.find_each(&:index_to_elasticsearch_inline) + end + end +end diff --git a/spec/lib/data_update_scripts/index_classified_listings_to_elasticsearch_spec.rb b/spec/lib/data_update_scripts/index_classified_listings_to_elasticsearch_spec.rb new file mode 100644 index 000000000..5e13f6a57 --- /dev/null +++ b/spec/lib/data_update_scripts/index_classified_listings_to_elasticsearch_spec.rb @@ -0,0 +1,11 @@ +require "rails_helper" +require Rails.root.join("lib/data_update_scripts/20200217215802_index_classified_listings_to_elasticsearch.rb") + +describe DataUpdateScripts::IndexClassifiedListingsToElasticsearch, elasticsearch: true do + it "indexes classified_listings to Elasticsearch" do + classified_listing = FactoryBot.create(:classified_listing) + expect { classified_listing.elasticsearch_doc }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) + described_class.new.run + expect(classified_listing.elasticsearch_doc).not_to be_nil + end +end diff --git a/spec/models/classified_listing_spec.rb b/spec/models/classified_listing_spec.rb index 0ccfcab00..093ac5853 100644 --- a/spec/models/classified_listing_spec.rb +++ b/spec/models/classified_listing_spec.rb @@ -71,4 +71,49 @@ RSpec.describe ClassifiedListing, type: :model do expect(credit.reload.purchase).to be_nil end end + + describe "#index_to_elasticsearch" do + it "enqueues job to index classified_listing to elasticsearch" do + sidekiq_assert_enqueued_with(job: Search::ClassifiedListingEsIndexWorker, args: [classified_listing.id]) do + classified_listing.index_to_elasticsearch + end + end + end + + describe "#index_to_elasticsearch_inline" do + it "indexed classified_listing to elasticsearch inline" do + allow(Search::ClassifiedListing).to receive(:index) + classified_listing.index_to_elasticsearch_inline + expect(Search::ClassifiedListing).to have_received(:index).with(classified_listing.id, hash_including(:id, :body_markdown)) + end + end + + describe "#after_commit" do + it "enqueues worker to index tag to elasticsearch" do + classified_listing.save + + sidekiq_assert_enqueued_with(job: Search::ClassifiedListingEsIndexWorker, args: [classified_listing.id]) do + classified_listing.save + end + end + end + + describe "#serialized_search_hash" do + it "creates a valid serialized hash to send to elasticsearch" do + # classified_listing_search is a copy_to field and will never be included + # in the searialized_search_hash, so we skip it + mapping_keys = Search::ClassifiedListing::MAPPINGS.dig(:properties).except(:classified_listing_search).keys + search_hash_keys = classified_listing.serialized_search_hash.symbolize_keys.keys + + expect(search_hash_keys).to match_array(mapping_keys) + end + end + + describe "#elasticsearch_doc" do + it "finds document in elasticsearch", elasticsearch: true do + allow(Search::ClassifiedListing).to receive(:find_document) + classified_listing.elasticsearch_doc + expect(Search::ClassifiedListing).to have_received(:find_document) + end + end end diff --git a/spec/services/search/classified_listing_spec.rb b/spec/services/search/classified_listing_spec.rb index e5f559eb4..a9af0471b 100644 --- a/spec/services/search/classified_listing_spec.rb +++ b/spec/services/search/classified_listing_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Search::ClassifiedListing, type: :service, elasticsearch: true do it "indexes a classified_listing to elasticsearch" do classified_listing = FactoryBot.create(:classified_listing) expect { described_class.find_document(classified_listing.id) }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) - described_class.index(classified_listing.id, id: classified_listing.id) + described_class.index(classified_listing.id, classified_listing.serialized_search_hash) expect(described_class.find_document(classified_listing.id)).not_to be_nil end end @@ -13,7 +13,7 @@ RSpec.describe Search::ClassifiedListing, type: :service, elasticsearch: true do describe "::find_document" do it "fetches a document for a given ID from elasticsearch" do classified_listing = FactoryBot.create(:classified_listing) - described_class.index(classified_listing.id, id: classified_listing.id) + described_class.index(classified_listing.id, classified_listing.serialized_search_hash) expect { described_class.find_document(classified_listing.id) }.not_to raise_error end end @@ -77,9 +77,14 @@ RSpec.describe Search::ClassifiedListing, type: :service, elasticsearch: true do initial_mapping = SearchClient.indices.get_mapping(index: other_name).dig(other_name, "mappings") expect(initial_mapping).to be_empty + # This might look a little strange...it's because es_mapping_keys returns + # certain fields like copy_to as an Array and our mappings don't have it + # in that format. As a result, we're just comparing keys instead. described_class.update_mappings(index_alias: other_name) - mapping = SearchClient.indices.get_mapping(index: other_name).dig(other_name, "mappings") - expect(mapping.deep_stringify_keys).to include(described_class::MAPPINGS.deep_stringify_keys) + es_mapping_keys = SearchClient.indices.get_mapping(index: other_name).dig(other_name, "mappings", "properties").symbolize_keys.keys + mapping_keys = described_class::MAPPINGS.dig(:properties).keys + + expect(mapping_keys).to match_array(es_mapping_keys) # Have to cleanup index since it wont automatically be handled by our cluster class bc of the unexpected name described_class.delete_index(index_name: other_name) diff --git a/spec/workers/search/classified_listing_es_index_worker_spec.rb b/spec/workers/search/classified_listing_es_index_worker_spec.rb new file mode 100644 index 000000000..cf1c971fd --- /dev/null +++ b/spec/workers/search/classified_listing_es_index_worker_spec.rb @@ -0,0 +1,24 @@ +require "rails_helper" + +RSpec.describe Search::ClassifiedListingEsIndexWorker, type: :worker, elasticsearch: true do + let(:worker) { subject } + + include_examples "#enqueues_on_correct_queue", "high_priority", [1] + + it "raises an error if record is not found" do + expect { worker.perform(1234) }.to raise_error(ActiveRecord::RecordNotFound) + end + + it "indexes classified_listing" do + classified_listing = FactoryBot.create(:classified_listing) + expect { classified_listing.elasticsearch_doc }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) + worker.perform(classified_listing.id) + + elasticsearch_doc = classified_listing.elasticsearch_doc.dig("_source").deep_symbolize_keys + cl_serialized_search_hash = classified_listing.serialized_search_hash.deep_symbolize_keys + + expect(elasticsearch_doc[:id]).to eq(cl_serialized_search_hash[:id]) + expect(elasticsearch_doc[:author][:name]).to eq(cl_serialized_search_hash[:author][:name]) + expect(elasticsearch_doc[:body_markdown]).to eq(cl_serialized_search_hash[:body_markdown]) + end +end