Centralize search error handling (#6227) [deploy]

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

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

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

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)

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

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

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

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

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

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
 

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
 

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
 

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(

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

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,