diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 995dd1eff..77a5a56c3 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -5,8 +5,7 @@ class SearchController < ApplicationController tag_docs = Search::Tag.search_documents("name:#{params[:name]}* AND supported:true") render json: { result: tag_docs } - rescue Elasticsearch::Transport::Transport::Errors::BadRequest - DatadogStatsClient.increment("elasticsearch.errors", tags: ["error:BadRequest"]) + rescue Search::Errors::Transport::BadRequest render json: { result: [] } end end diff --git a/app/errors/search.rb b/app/errors/search.rb new file mode 100644 index 000000000..0559103f8 --- /dev/null +++ b/app/errors/search.rb @@ -0,0 +1,17 @@ +module Search + module Errors + class Error < StandardError + end + + module Transport + class TransportError < Error + end + + class BadRequest < Error + end + + class NotFound < Error + end + end + end +end diff --git a/app/services/search/base.rb b/app/services/search/base.rb index 4fea555a6..738b4c985 100644 --- a/app/services/search/base.rb +++ b/app/services/search/base.rb @@ -2,15 +2,19 @@ module Search class Base class << self def index(doc_id, serialized_data) - SearchClient.index( - id: doc_id, - index: self::INDEX_ALIAS, - body: serialized_data, - ) + request do + SearchClient.index( + id: doc_id, + index: self::INDEX_ALIAS, + body: serialized_data, + ) + end end def find_document(doc_id) - SearchClient.get(id: doc_id, index: self::INDEX_ALIAS) + request do + SearchClient.get(id: doc_id, index: self::INDEX_ALIAS) + end end def delete_document(doc_id) @@ -18,27 +22,56 @@ module Search end def create_index(index_name: self::INDEX_NAME) - SearchClient.indices.create(index: index_name, body: settings) + request do + SearchClient.indices.create(index: index_name, body: settings) + end end def delete_index(index_name: self::INDEX_NAME) - SearchClient.indices.delete(index: index_name) + request do + SearchClient.indices.delete(index: index_name) + end end def refresh_index(index_name: self::INDEX_ALIAS) - SearchClient.indices.refresh(index: index_name) + request do + SearchClient.indices.refresh(index: index_name) + end end def add_alias(index_name: self::INDEX_NAME, index_alias: self::INDEX_ALIAS) - SearchClient.indices.put_alias(index: index_name, name: index_alias) + request do + SearchClient.indices.put_alias(index: index_name, name: index_alias) + end end def update_mappings(index_alias: self::INDEX_ALIAS) - SearchClient.indices.put_mapping(index: index_alias, body: self::MAPPINGS) + request do + SearchClient.indices.put_mapping(index: index_alias, body: self::MAPPINGS) + end end private + TRANSPORT_EXCEPTIONS = [ + Elasticsearch::Transport::Transport::Errors::BadRequest, + Elasticsearch::Transport::Transport::Errors::NotFound, + ].freeze + + def request + yield + rescue *TRANSPORT_EXCEPTIONS => e + class_name = e.class.name.demodulize + + DatadogStatsClient.increment("elasticsearch.errors", tags: ["error:#{class_name}"], message: e.message) + + # raise specific error if known, generic one if unknown + error_class = "::Search::Errors::Transport::#{class_name}".safe_constantize + raise error_class, e.message if error_class + + raise ::Search::Errors::TransportError, e.message + end + def settings { settings: { index: index_settings } } end diff --git a/app/services/search/tag.rb b/app/services/search/tag.rb index a5b1bbe3d..f0cffd34a 100644 --- a/app/services/search/tag.rb +++ b/app/services/search/tag.rb @@ -6,21 +6,23 @@ module Search class << self def search(query_string) - SearchClient.search( - index: INDEX_ALIAS, - body: { - query: { - query_string: { - query: query_string, - analyze_wildcard: true, - allow_leading_wildcard: false + request do + SearchClient.search( + index: INDEX_ALIAS, + body: { + query: { + query_string: { + query: query_string, + analyze_wildcard: true, + allow_leading_wildcard: false + } + }, + sort: { + hotness_score: "desc" } }, - sort: { - hotness_score: "desc" - } - }, - ) + ) + end end def search_documents(query_string) diff --git a/spec/lib/data_update_scripts/index_chat_channel_memberships_to_elasticsearch_spec.rb b/spec/lib/data_update_scripts/index_chat_channel_memberships_to_elasticsearch_spec.rb index eadfcde58..788a2c074 100644 --- a/spec/lib/data_update_scripts/index_chat_channel_memberships_to_elasticsearch_spec.rb +++ b/spec/lib/data_update_scripts/index_chat_channel_memberships_to_elasticsearch_spec.rb @@ -4,7 +4,7 @@ require Rails.root.join("lib/data_update_scripts/20200218195023_index_chat_chann describe DataUpdateScripts::IndexChatChannelMembershipsToElasticsearch, elasticsearch: true do it "indexes chat channel memberships to Elasticsearch" do chat_channel_membership = FactoryBot.create(:chat_channel_membership) - expect { chat_channel_membership.elasticsearch_doc }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) + expect { chat_channel_membership.elasticsearch_doc }.to raise_error(Search::Errors::Transport::NotFound) described_class.new.run expect(chat_channel_membership.elasticsearch_doc).not_to be_nil 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 index 5e13f6a57..a497f4426 100644 --- 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 @@ -3,8 +3,8 @@ require Rails.root.join("lib/data_update_scripts/20200217215802_index_classified 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) + classified_listing = create(:classified_listing) + expect { classified_listing.elasticsearch_doc }.to raise_error(Search::Errors::Transport::NotFound) described_class.new.run expect(classified_listing.elasticsearch_doc).not_to be_nil end diff --git a/spec/lib/data_update_scripts/index_tags_to_elasticsearch_spec.rb b/spec/lib/data_update_scripts/index_tags_to_elasticsearch_spec.rb index 29b1cf546..3b46a4cac 100644 --- a/spec/lib/data_update_scripts/index_tags_to_elasticsearch_spec.rb +++ b/spec/lib/data_update_scripts/index_tags_to_elasticsearch_spec.rb @@ -4,7 +4,7 @@ require Rails.root.join("lib/data_update_scripts/20200214171607_index_tags_to_el describe DataUpdateScripts::IndexTagsToElasticsearch, elasticsearch: true do it "indexes tags to Elasticsearch" do tag = FactoryBot.create(:tag) - expect { tag.elasticsearch_doc }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) + expect { tag.elasticsearch_doc }.to raise_error(Search::Errors::Transport::NotFound) described_class.new.run expect(tag.elasticsearch_doc).not_to be_nil end diff --git a/spec/requests/search_spec.rb b/spec/requests/search_spec.rb index 08f510576..c1f70b878 100644 --- a/spec/requests/search_spec.rb +++ b/spec/requests/search_spec.rb @@ -16,9 +16,9 @@ RSpec.describe "Search", type: :request, proper_status: true do expect(response.parsed_body).to eq("result" => mock_documents) end - it "returns an empty array when a Elasticsearch Bad Request error is raised" do + it "returns an empty array when a Bad Request error is raised" do sign_in authorized_user - allow(Search::Tag).to receive(:search_documents).and_raise(Elasticsearch::Transport::Transport::Errors::BadRequest) + allow(SearchClient).to receive(:search).and_raise(Search::Errors::Transport::BadRequest) get "/search/tags" expect(response.parsed_body).to eq("result" => []) end diff --git a/spec/requests/stories_index_spec.rb b/spec/requests/stories_index_spec.rb index 66de2d073..cdfe25aaf 100644 --- a/spec/requests/stories_index_spec.rb +++ b/spec/requests/stories_index_spec.rb @@ -6,7 +6,7 @@ RSpec.describe "StoriesIndex", type: :request do describe "GET stories index" do it "renders page with article list" do get "/" - expect(response.body).to include(article.title) + expect(response.body).to include(CGI.escapeHTML(article.title)) end it "renders page with min read" do diff --git a/spec/services/search/chat_channel_membership_spec.rb b/spec/services/search/chat_channel_membership_spec.rb index 14a4de3b1..3d63a3b2b 100644 --- a/spec/services/search/chat_channel_membership_spec.rb +++ b/spec/services/search/chat_channel_membership_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Search::ChatChannelMembership, type: :service, elasticsearch: tru describe "::index" do it "indexes a chat_channel_membership to elasticsearch" do chat_channel_membership = FactoryBot.create(:chat_channel_membership) - expect { described_class.find_document(chat_channel_membership.id) }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) + expect { described_class.find_document(chat_channel_membership.id) }.to raise_error(Search::Errors::Transport::NotFound) described_class.index(chat_channel_membership.id, id: chat_channel_membership.id) expect(described_class.find_document(chat_channel_membership.id)).not_to be_nil end @@ -24,7 +24,7 @@ RSpec.describe Search::ChatChannelMembership, type: :service, elasticsearch: tru chat_channel_membership.index_to_elasticsearch_inline expect { described_class.find_document(chat_channel_membership.id) }.not_to raise_error described_class.delete_document(chat_channel_membership.id) - expect { described_class.find_document(chat_channel_membership.id) }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) + expect { described_class.find_document(chat_channel_membership.id) }.to raise_error(Search::Errors::Transport::NotFound) end end diff --git a/spec/services/search/classified_listing_spec.rb b/spec/services/search/classified_listing_spec.rb index 013920268..417657464 100644 --- a/spec/services/search/classified_listing_spec.rb +++ b/spec/services/search/classified_listing_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Search::ClassifiedListing, type: :service, elasticsearch: true do describe "::index" 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) + expect { described_class.find_document(classified_listing.id) }.to raise_error(Search::Errors::Transport::NotFound) described_class.index(classified_listing.id, classified_listing.serialized_search_hash) expect(described_class.find_document(classified_listing.id)).not_to be_nil end @@ -24,7 +24,7 @@ RSpec.describe Search::ClassifiedListing, type: :service, elasticsearch: true do classified_listing.index_to_elasticsearch_inline expect { described_class.find_document(classified_listing.id) }.not_to raise_error described_class.delete_document(classified_listing.id) - expect { described_class.find_document(classified_listing.id) }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) + expect { described_class.find_document(classified_listing.id) }.to raise_error(Search::Errors::Transport::NotFound) end end diff --git a/spec/services/search/tag_spec.rb b/spec/services/search/tag_spec.rb index f22dc1f05..8d36eeadc 100644 --- a/spec/services/search/tag_spec.rb +++ b/spec/services/search/tag_spec.rb @@ -3,8 +3,10 @@ require "rails_helper" RSpec.describe Search::Tag, type: :service, elasticsearch: true do describe "::index" do it "indexes a tag to elasticsearch" do - tag = FactoryBot.create(:tag) - expect { described_class.find_document(tag.id) }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) + tag = create(:tag) + + expect { described_class.find_document(tag.id) }.to raise_error(Search::Errors::Transport::NotFound) + described_class.index(tag.id, tag.serialized_search_hash) expect(described_class.find_document(tag.id)).not_to be_nil end @@ -12,7 +14,7 @@ RSpec.describe Search::Tag, type: :service, elasticsearch: true do describe "::search" do it "searches with a given query string" do - tag1 = FactoryBot.create(:tag, :search_indexed, name: "tag1") + tag1 = create(:tag, :search_indexed, name: "tag1") described_class.refresh_index hits = described_class.search("name:tag1").dig("hits", "hits") tag_names = hits.map { |t| t.dig("_source", "name") } @@ -21,9 +23,9 @@ RSpec.describe Search::Tag, type: :service, elasticsearch: true do end it "analyzes wildcards" do - tag1 = FactoryBot.create(:tag, :search_indexed, name: "tag1") - tag2 = FactoryBot.create(:tag, :search_indexed, name: "tag2") - tag3 = FactoryBot.create(:tag, :search_indexed, name: "3tag") + tag1 = create(:tag, :search_indexed, name: "tag1") + tag2 = create(:tag, :search_indexed, name: "tag2") + tag3 = create(:tag, :search_indexed, name: "3tag") described_class.refresh_index hits = described_class.search("name:tag*").dig("hits", "hits") tag_names = hits.map { |t| t.dig("_source", "name") } @@ -32,8 +34,7 @@ RSpec.describe Search::Tag, type: :service, elasticsearch: true do end it "does not allow leading wildcards" do - bad_request_error = Elasticsearch::Transport::Transport::Errors::BadRequest - expect { described_class.search("name:*tag") }.to raise_error(bad_request_error) + expect { described_class.search("name:*tag") }.to raise_error(Search::Errors::Transport::BadRequest) end end @@ -61,7 +62,7 @@ RSpec.describe Search::Tag, type: :service, elasticsearch: true do describe "::find_document" do it "fetches a document for a given ID from elasticsearch" do - tag = FactoryBot.create(:tag) + tag = create(:tag) described_class.index(tag.id, tag.serialized_search_hash) expect { described_class.find_document(tag.id) }.not_to raise_error end @@ -69,11 +70,11 @@ RSpec.describe Search::Tag, type: :service, elasticsearch: true do describe "::delete_document" do it "deletes a document for a given ID from elasticsearch" do - tag = FactoryBot.create(:tag) + tag = create(:tag) tag.index_to_elasticsearch_inline expect { described_class.find_document(tag.id) }.not_to raise_error described_class.delete_document(tag.id) - expect { described_class.find_document(tag.id) }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) + expect { described_class.find_document(tag.id) }.to raise_error(Search::Errors::Transport::NotFound) end end diff --git a/spec/workers/search/chat_channel_membership_es_index_worker_spec.rb b/spec/workers/search/chat_channel_membership_es_index_worker_spec.rb index c3ebee24f..e48eb32b7 100644 --- a/spec/workers/search/chat_channel_membership_es_index_worker_spec.rb +++ b/spec/workers/search/chat_channel_membership_es_index_worker_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Search::ChatChannelMembershipEsIndexWorker, type: :worker, elasti it "indexes chat_channel_membership" do chat_channel_membership = FactoryBot.create(:chat_channel_membership) - expect { chat_channel_membership.elasticsearch_doc }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) + expect { chat_channel_membership.elasticsearch_doc }.to raise_error(Search::Errors::Transport::NotFound) worker.perform(chat_channel_membership.id) expected_hash = chat_channel_membership.serialized_search_hash.stringify_keys expect(chat_channel_membership.elasticsearch_doc.dig("_source")).to include( diff --git a/spec/workers/search/classified_listing_es_index_worker_spec.rb b/spec/workers/search/classified_listing_es_index_worker_spec.rb index cf1c971fd..ab1d7ce93 100644 --- a/spec/workers/search/classified_listing_es_index_worker_spec.rb +++ b/spec/workers/search/classified_listing_es_index_worker_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Search::ClassifiedListingEsIndexWorker, type: :worker, elasticsea it "indexes classified_listing" do classified_listing = FactoryBot.create(:classified_listing) - expect { classified_listing.elasticsearch_doc }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) + expect { classified_listing.elasticsearch_doc }.to raise_error(Search::Errors::Transport::NotFound) worker.perform(classified_listing.id) elasticsearch_doc = classified_listing.elasticsearch_doc.dig("_source").deep_symbolize_keys diff --git a/spec/workers/search/tag_es_index_worker_spec.rb b/spec/workers/search/tag_es_index_worker_spec.rb index 732c91bf3..b3700013a 100644 --- a/spec/workers/search/tag_es_index_worker_spec.rb +++ b/spec/workers/search/tag_es_index_worker_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Search::TagEsIndexWorker, type: :worker, elasticsearch: true do it "indexes tag" do tag = FactoryBot.create(:tag) - expect { tag.elasticsearch_doc }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) + expect { tag.elasticsearch_doc }.to raise_error(Search::Errors::Transport::NotFound) worker.perform(tag.id) expect(tag.elasticsearch_doc.dig("_source")).to eq( tag.serialized_search_hash.stringify_keys,