From 0a78ae739d0b14b6bfc2654c51be5907c28d3f8f Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 22 Dec 2016 13:13:14 +1100 Subject: [PATCH] Remove SearchObserver, aim is to remove all observers rails-observers gem is mostly unmaintained and is a pain to carry forward new implementation contains significantly less magic as a bonus --- app/models/category.rb | 34 +++++++++++-------- app/models/post.rb | 6 ++++ app/models/topic.rb | 2 ++ app/models/user.rb | 5 +++ .../search_indexer.rb} | 31 ++++++++++------- config/application.rb | 17 +++++++++- lib/search.rb | 4 +-- lib/search/grouped_search_results.rb | 2 +- lib/tasks/search.rake | 8 ++--- spec/components/search_spec.rb | 2 +- spec/controllers/search_controller_spec.rb | 2 +- spec/controllers/users_controller_spec.rb | 2 +- spec/models/topic_spec.rb | 2 +- spec/models/user_search_spec.rb | 2 +- spec/rails_helper.rb | 2 ++ .../search_indexer_spec.rb} | 8 ++--- 16 files changed, 85 insertions(+), 44 deletions(-) rename app/{models/search_observer.rb => services/search_indexer.rb} (83%) rename spec/{models/search_observer_spec.rb => services/search_indexer_spec.rb} (75%) diff --git a/app/models/category.rb b/app/models/category.rb index 458c24228ab..c4278b38870 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -41,20 +41,26 @@ class Category < ActiveRecord::Base validate :email_in_validator validate :ensure_slug + + after_create :create_category_definition + before_save :apply_permissions before_save :downcase_email before_save :downcase_name - after_create :create_category_definition - - after_save :publish_category - after_destroy :publish_category_deletion - - after_update :rename_category_definition, if: :name_changed? - - after_create :delete_category_permalink - after_update :create_category_permalink, if: :slug_changed? after_save :publish_discourse_stylesheet + after_save :publish_category + after_save :reset_topic_ids_cache + after_save :clear_url_cache + after_save :index_search + + after_destroy :reset_topic_ids_cache + after_destroy :publish_category_deletion + + after_create :delete_category_permalink + + after_update :rename_category_definition, if: :name_changed? + after_update :create_category_permalink, if: :slug_changed? has_one :category_search_data belongs_to :parent_category, class_name: 'Category' @@ -65,8 +71,6 @@ class Category < ActiveRecord::Base has_many :category_tag_groups, dependent: :destroy has_many :tag_groups, through: :category_tag_groups - after_save :reset_topic_ids_cache - after_destroy :reset_topic_ids_cache scope :latest, -> { order('topic_count DESC') } @@ -425,9 +429,7 @@ SQL @@url_cache = DistributedCache.new('category_url') - after_save do - # parent takes part in url calculation - # any change could invalidate multiples + def clear_url_cache @@url_cache.clear end @@ -491,6 +493,10 @@ SQL DiscourseStylesheets.cache.clear end + def index_search + SearchIndexer.index(self) + end + def self.find_by_slug(category_slug, parent_category_slug=nil) if parent_category_slug parent_category_id = self.where(slug: parent_category_slug, parent_category_id: nil).pluck(:id).first diff --git a/app/models/post.rb b/app/models/post.rb index e262f259ba7..5fe4606f78b 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -51,6 +51,8 @@ class Post < ActiveRecord::Base validates_with ::Validators::PostValidator + after_save :index_search + # We can pass several creating options to a post via attributes attr_accessor :image_sizes, :quoted_post_numbers, :no_bump, :invalidate_oneboxes, :cooking_options, :skip_unique_check @@ -636,6 +638,10 @@ class Post < ActiveRecord::Base PostTiming.where(topic_id: topic_id, post_number: post_number, user_id: user.id).exists? end + def index_search + SearchIndexer.index(self) + end + private def parse_quote_into_arguments(quote) diff --git a/app/models/topic.rb b/app/models/topic.rb index c7bf3638105..7ce360bc93f 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -198,6 +198,8 @@ class Topic < ActiveRecord::Base TagUser.auto_track(topic_id: id) self.tags_changed = false end + + SearchIndexer.index(self) end def initialize_default_values diff --git a/app/models/user.rb b/app/models/user.rb index e363f5dd55a..b1c5273b495 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -99,6 +99,7 @@ class User < ActiveRecord::Base after_save :refresh_avatar after_save :badge_grant after_save :expire_old_email_tokens + after_save :index_search before_destroy do # These tables don't have primary keys, so destroying them with activerecord is tricky: @@ -913,6 +914,10 @@ class User < ActiveRecord::Base end end + def index_search + SearchIndexer.index(self) + end + def clear_global_notice_if_needed if admin && SiteSetting.has_login_hint SiteSetting.has_login_hint = false diff --git a/app/models/search_observer.rb b/app/services/search_indexer.rb similarity index 83% rename from app/models/search_observer.rb rename to app/services/search_indexer.rb index e45e5b2d9b4..08478265679 100644 --- a/app/models/search_observer.rb +++ b/app/services/search_indexer.rb @@ -1,7 +1,14 @@ require_dependency 'search' -class SearchObserver < ActiveRecord::Observer - observe :topic, :post, :user, :category +class SearchIndexer + + def self.disable + @disabled = true + end + + def self.enable + @disabled = false + end def self.scrub_html_for_search(html) HtmlScrubber.scrub(html) @@ -72,17 +79,19 @@ class SearchObserver < ActiveRecord::Observer end def self.index(obj) + return if @disabled + if obj.class == Post && obj.cooked_changed? if obj.topic category_name = obj.topic.category.name if obj.topic.category - SearchObserver.update_posts_index(obj.id, obj.cooked, obj.topic.title, category_name) - SearchObserver.update_topics_index(obj.topic_id, obj.topic.title, obj.cooked) if obj.is_first_post? + SearchIndexer.update_posts_index(obj.id, obj.cooked, obj.topic.title, category_name) + SearchIndexer.update_topics_index(obj.topic_id, obj.topic.title, obj.cooked) if obj.is_first_post? else - Rails.logger.warn("Orphan post skipped in search_observer, topic_id: #{obj.topic_id} post_id: #{obj.id} raw: #{obj.raw}") + Rails.logger.warn("Orphan post skipped in search_indexer, topic_id: #{obj.topic_id} post_id: #{obj.id} raw: #{obj.raw}") end end if obj.class == User && (obj.username_changed? || obj.name_changed?) - SearchObserver.update_users_index(obj.id, obj.username_lower || '', obj.name ? obj.name.downcase : '') + SearchIndexer.update_users_index(obj.id, obj.username_lower || '', obj.name ? obj.name.downcase : '') end if obj.class == Topic && obj.title_changed? @@ -90,21 +99,17 @@ class SearchObserver < ActiveRecord::Observer post = obj.posts.find_by(post_number: 1) if post category_name = obj.category.name if obj.category - SearchObserver.update_posts_index(post.id, post.cooked, obj.title, category_name) - SearchObserver.update_topics_index(obj.id, obj.title, post.cooked) + SearchIndexer.update_posts_index(post.id, post.cooked, obj.title, category_name) + SearchIndexer.update_topics_index(obj.id, obj.title, post.cooked) end end end if obj.class == Category && obj.name_changed? - SearchObserver.update_categories_index(obj.id, obj.name) + SearchIndexer.update_categories_index(obj.id, obj.name) end end - def after_save(object) - SearchObserver.index(object) - end - class HtmlScrubber < Nokogiri::XML::SAX::Document attr_reader :scrubbed diff --git a/config/application.rb b/config/application.rb index a7a5531a691..249f4e90d9a 100644 --- a/config/application.rb +++ b/config/application.rb @@ -85,7 +85,6 @@ module Discourse :user_email_observer, :user_action_observer, :post_alert_observer, - :search_observer ] # Set Time.zone default to the specified zone and make Active Record auto-convert to this zone. @@ -169,6 +168,22 @@ module Discourse end config.after_initialize do + # require common dependencies that are often required by plugins + # in the past observers would load them as side-effects + # correct behavior is for plugins to require stuff they need, + # however it would be a risky and breaking change not to require here + require_dependency 'category' + require_dependency 'post' + require_dependency 'topic' + require_dependency 'user' + require_dependency 'post_action' + require_dependency 'post_revision' + require_dependency 'notification' + require_dependency 'topic_user' + require_dependency 'group' + require_dependency 'user_field' + require_dependency 'post_action_type' + # So open id logs somewhere sane OpenID::Util.logger = Rails.logger if plugins = Discourse.plugins diff --git a/lib/search.rb b/lib/search.rb index 6f0e374faa3..cdad501bd86 100644 --- a/lib/search.rb +++ b/lib/search.rb @@ -54,7 +54,7 @@ class Search posts.each do |post| # force indexing post.cooked += " " - SearchObserver.index(post) + SearchIndexer.index(post) end posts = Post.joins(:topic) @@ -67,7 +67,7 @@ class Search posts.each do |post| # force indexing post.cooked += " " - SearchObserver.index(post) + SearchIndexer.index(post) end nil diff --git a/lib/search/grouped_search_results.rb b/lib/search/grouped_search_results.rb index 2fa450bb745..47e13254883 100644 --- a/lib/search/grouped_search_results.rb +++ b/lib/search/grouped_search_results.rb @@ -49,7 +49,7 @@ class Search def self.blurb_for(cooked, term=nil, blurb_length=200) - cooked = SearchObserver::HtmlScrubber.scrub(cooked).squish + cooked = SearchIndexer::HtmlScrubber.scrub(cooked).squish blurb = nil if term diff --git a/lib/tasks/search.rake b/lib/tasks/search.rake index 4f118227c69..c944812d793 100644 --- a/lib/tasks/search.rake +++ b/lib/tasks/search.rake @@ -18,8 +18,8 @@ def reindex_search(db=RailsMultisite::ConnectionManagement.current_db) post_number = p["post_number"].to_i topic_id = p["topic_id"].to_i - SearchObserver.update_posts_index(post_id, cooked, title, category) - SearchObserver.update_topics_index(topic_id, title , cooked) if post_number == 1 + SearchIndexer.update_posts_index(post_id, cooked, title, category) + SearchIndexer.update_topics_index(topic_id, title , cooked) if post_number == 1 putc "." end @@ -30,7 +30,7 @@ def reindex_search(db=RailsMultisite::ConnectionManagement.current_db) id = u["id"] name = u["name"] username = u["username"] - SearchObserver.update_users_index(id, username, name) + SearchIndexer.update_users_index(id, username, name) putc "." end @@ -41,7 +41,7 @@ def reindex_search(db=RailsMultisite::ConnectionManagement.current_db) Category.exec_sql("select id, name from categories").each do |c| id = c["id"] name = c["name"] - SearchObserver.update_categories_index(id, name) + SearchIndexer.update_categories_index(id, name) end puts diff --git a/spec/components/search_spec.rb b/spec/components/search_spec.rb index 8d054157b1d..a9184ef0906 100644 --- a/spec/components/search_spec.rb +++ b/spec/components/search_spec.rb @@ -10,7 +10,7 @@ describe Search do end before do - ActiveRecord::Base.observers.enable :search_observer + SearchIndexer.enable end context 'post indexing observer' do diff --git a/spec/controllers/search_controller_spec.rb b/spec/controllers/search_controller_spec.rb index ceb07f93de8..7e967f0b7d2 100644 --- a/spec/controllers/search_controller_spec.rb +++ b/spec/controllers/search_controller_spec.rb @@ -5,7 +5,7 @@ describe SearchController do context "integration" do before do - ActiveRecord::Base.observers.enable :search_observer + SearchIndexer.enable end it "can search correctly" do diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 694fd7bd4e7..5d4f5cb80bf 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -1331,7 +1331,7 @@ describe UsersController do let(:user) { Fabricate :user, username: "joecabot", name: "Lawrence Tierney" } before do - ActiveRecord::Base.observers.enable :all + SearchIndexer.enable Fabricate :post, user: user, topic: topic end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index d87009fe458..71c7731818a 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -303,7 +303,7 @@ describe Topic do context 'with a similar topic' do let!(:topic) { - ActiveRecord::Base.observers.enable :search_observer + SearchIndexer.enable post = create_post(title: "Evil trout is the dude who posted this topic") post.topic } diff --git a/spec/models/user_search_spec.rb b/spec/models/user_search_spec.rb index 5d5a55f4f23..47621ef60f8 100644 --- a/spec/models/user_search_spec.rb +++ b/spec/models/user_search_spec.rb @@ -18,7 +18,7 @@ describe UserSearch do let(:staged) { Fabricate :staged } before do - ActiveRecord::Base.observers.enable :all + SearchIndexer.enable Fabricate :post, user: user1, topic: topic Fabricate :post, user: user2, topic: topic2 diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 478af450ca2..24efd0106ff 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -107,6 +107,8 @@ Spork.prefork do # disable all observers, enable as needed during specs # ActiveRecord::Base.observers.disable :all + SearchIndexer.disable + SiteSetting.provider.all.each do |setting| SiteSetting.remove_override!(setting.name) end diff --git a/spec/models/search_observer_spec.rb b/spec/services/search_indexer_spec.rb similarity index 75% rename from spec/models/search_observer_spec.rb rename to spec/services/search_indexer_spec.rb index fccf8612207..74ac1c2b4fb 100644 --- a/spec/models/search_observer_spec.rb +++ b/spec/services/search_indexer_spec.rb @@ -1,13 +1,13 @@ require 'rails_helper' -describe SearchObserver do +describe SearchIndexer do it 'correctly indexes chinese' do SiteSetting.default_locale = 'zh_CN' data = "你好世界" expect(data.split(" ").length).to eq(1) - SearchObserver.update_posts_index(99, "你好世界", "", nil) + SearchIndexer.update_posts_index(99, "你好世界", "", nil) raw_data = PostSearchData.where(post_id: 99).pluck(:raw_data)[0] expect(raw_data.split(' ').length).to eq(2) @@ -16,13 +16,13 @@ describe SearchObserver do it 'correctly indexes a post' do data = "This is a test" - SearchObserver.update_posts_index(99, data, "", nil) + SearchIndexer.update_posts_index(99, data, "", nil) raw_data, locale = PostSearchData.where(post_id: 99).pluck(:raw_data, :locale)[0] expect(raw_data).to eq("This is a test") expect(locale).to eq("en") - SearchObserver.update_posts_index(99, "tester", "", nil) + SearchIndexer.update_posts_index(99, "tester", "", nil) raw_data = PostSearchData.where(post_id: 99).pluck(:raw_data)[0] expect(raw_data).to eq("tester")