DEV: Change HashtagAutocompleteService to use DiscoursePluginRegistry (#19491)

Follow up to a review in #18937, this commit changes the HashtagAutocompleteService to no longer use class variables to register hashtag data sources or types in context priority order. This is to address multisite concerns, where one site could e.g. have chat disabled and another might not. The filtered plugin registers I added will not be included if the plugin is disabled.

app/services/category_hashtag_data_source.rb

@@ -8,6 +8,10 @@ class CategoryHashtagDataSource
     "folder"
   end
 
+  def self.type
+    "category"
+  end
+
   def self.category_to_hashtag_item(category)
     HashtagAutocompleteService::HashtagItem.new.tap do |item|
       item.text = category.name

app/services/hashtag_autocomplete_service.rb

@@ -3,49 +3,61 @@
 class HashtagAutocompleteService
   HASHTAGS_PER_REQUEST = 20
   SEARCH_MAX_LIMIT = 50
+  DEFAULT_DATA_SOURCES = [CategoryHashtagDataSource, TagHashtagDataSource]
+  DEFAULT_CONTEXTUAL_TYPE_PRIORITIES = [
+    { type: "category", context: "topic-composer", priority: 100 },
+    { type: "tag", context: "topic-composer", priority: 50 },
+  ]
 
   def self.search_conditions
     @search_conditions ||= Enum.new(contains: 0, starts_with: 1)
   end
 
   attr_reader :guardian
-  cattr_reader :data_sources, :contexts
 
-  def self.register_data_source(type, klass)
+  def self.data_sources
-    @@data_sources[type] = klass
+    # Category and Tag data sources are in core and always should be
+    # included for searches and lookups.
+    Set.new(DEFAULT_DATA_SOURCES | DiscoursePluginRegistry.hashtag_autocomplete_data_sources)
   end
 
-  def self.clear_registered
+  def self.contextual_type_priorities
-    @@data_sources = {}
+    # Category and Tag type priorities for the composer are default and
-    @@contexts = {}
+    # always are included.
-
+    Set.new(
-    register_data_source("category", CategoryHashtagDataSource)
+      DEFAULT_CONTEXTUAL_TYPE_PRIORITIES |
-    register_data_source("tag", TagHashtagDataSource)
+        DiscoursePluginRegistry.hashtag_autocomplete_contextual_type_priorities,
-
+    )
-    register_type_in_context("category", "topic-composer", 100)
-    register_type_in_context("tag", "topic-composer", 50)
   end
 
-  def self.register_type_in_context(type, context, priority)
+  def self.data_source_types
-    @@contexts[context] = @@contexts[context] || {}
+    data_sources.map(&:type)
-    @@contexts[context][type] = priority
   end
 
   def self.data_source_icons
-    @@data_sources.values.map(&:icon)
+    data_sources.map(&:icon)
+  end
+
+  def self.data_source_from_type(type)
+    data_sources.find { |ds| ds.type == type }
+  end
+
+  def self.find_priorities_for_context(context)
+    contextual_type_priorities.select { |ctp| ctp[:context] == context }
+  end
+
+  def self.unique_contexts
+    contextual_type_priorities.map { |ctp| ctp[:context] }.uniq
   end
 
   def self.ordered_types_for_context(context)
-    return [] if @@contexts[context].blank?
+    find_priorities_for_context(context).sort_by { |ctp| -ctp[:priority] }.map { |ctp| ctp[:type] }
-    @@contexts[context].sort_by { |param, priority| priority }.reverse.map(&:first)
   end
 
   def self.contexts_with_ordered_types
-    Hash[@@contexts.keys.map { |context| [context, ordered_types_for_context(context)] }]
+    Hash[unique_contexts.map { |context| [context, ordered_types_for_context(context)] }]
   end
 
-  clear_registered
-
   class HashtagItem
     # The text to display in the UI autocomplete menu for the item.
     attr_accessor :text
@@ -120,13 +132,15 @@ class HashtagAutocompleteService
     raise Discourse::InvalidParameters.new(:order) if !types_in_priority_order.is_a?(Array)
 
     types_in_priority_order =
-      types_in_priority_order.select { |type| @@data_sources.keys.include?(type) }
+      types_in_priority_order.select do |type|
+        HashtagAutocompleteService.data_source_types.include?(type)
+      end
     lookup_results = Hash[types_in_priority_order.collect { |type| [type.to_sym, []] }]
     limited_slugs = slugs[0..HashtagAutocompleteService::HASHTAGS_PER_REQUEST]
 
     slugs_without_suffixes =
       limited_slugs.reject do |slug|
-        @@data_sources.keys.any? { |type| slug.ends_with?("::#{type}") }
+        HashtagAutocompleteService.data_source_types.any? { |type| slug.ends_with?("::#{type}") }
       end
     slugs_with_suffixes = (limited_slugs - slugs_without_suffixes)
 
@@ -208,7 +222,9 @@ class HashtagAutocompleteService
     top_ranked_type = nil
     term = term.downcase
     types_in_priority_order =
-      types_in_priority_order.select { |type| @@data_sources.keys.include?(type) }
+      types_in_priority_order.select do |type|
+        HashtagAutocompleteService.data_source_types.include?(type)
+      end
 
     # Float exact matches by slug to the top of the list, any of these will be excluded
     # from further results.
@@ -318,7 +334,7 @@ class HashtagAutocompleteService
     return limited_results if search_results.empty?
 
     search_results =
-      @@data_sources[type].search_sort(
+      HashtagAutocompleteService.data_source_from_type(type).search_sort(
         search_results.reject do |item|
           limited_results.any? { |exact| exact.type == type && exact.slug === item.slug }
         end,
@@ -334,7 +350,12 @@ class HashtagAutocompleteService
 
     types_in_priority_order.each do |type|
       search_results =
-        filter_valid_data_items(@@data_sources[type].search_without_term(guardian, split_limit))
+        filter_valid_data_items(
+          HashtagAutocompleteService.data_source_from_type(type).search_without_term(
+            guardian,
+            split_limit,
+          ),
+        )
       next if search_results.empty?
 
       # This is purposefully unsorted as search_without_term should sort
@@ -369,7 +390,17 @@ class HashtagAutocompleteService
     condition = HashtagAutocompleteService.search_conditions[:contains]
   )
     filter_valid_data_items(
-      set_types(set_refs(@@data_sources[type].search(guardian, term, limit, condition)), type),
+      set_types(
+        set_refs(
+          HashtagAutocompleteService.data_source_from_type(type).search(
+            guardian,
+            term,
+            limit,
+            condition,
+          ),
+        ),
+        type,
+      ),
     )
   end
 
@@ -385,7 +416,10 @@ class HashtagAutocompleteService
   end
 
   def lookup_for_type(type, guardian, slugs)
-    set_types(set_refs(@@data_sources[type].lookup(guardian, slugs)), type)
+    set_types(
+      set_refs(HashtagAutocompleteService.data_source_from_type(type).lookup(guardian, slugs)),
+      type,
+    )
   end
 
   def append_types_to_conflicts(limited_results, top_ranked_type, limit)

app/services/tag_hashtag_data_source.rb

@@ -8,6 +8,10 @@ class TagHashtagDataSource
     "tag"
   end
 
+  def self.type
+    "tag"
+  end
+
   def self.tag_to_hashtag_item(tag, include_count: false)
     tag = Tag.new(tag.slice(:id, :name, :description).merge(topic_count: tag[:count])) if tag.is_a?(
       Hash,

lib/discourse_plugin_registry.rb

@@ -5,11 +5,16 @@
 #
 class DiscoursePluginRegistry
 
+  # Plugins often need to be able to register additional handlers, data, or
+  # classes that will be used by core classes. This should be used if you
+  # need to control which type the registry is, and if it doesn't need to
+  # be removed if the plugin is disabled.
+  #
   # Shortcut to create new register in the plugin registry
   #   - Register is created in a class variable using the specified name/type
   #   - Defines singleton method to access the register
   #   - Defines instance method as a shortcut to the singleton method
-  #   - Automatically deletes the register on ::clear!
+  #   - Automatically deletes the register on registry.reset!
   def self.define_register(register_name, type)
     @@register_names ||= Set.new
     @@register_names << register_name
@@ -24,11 +29,15 @@ class DiscoursePluginRegistry
     end
   end
 
+  # Plugins often need to add values to a list, and we need to filter those
+  # lists at runtime to ignore values from disabled plugins. Unlike define_register,
+  # the type of the register cannot be defined, and is always Array.
+  #
   # Create a new register (see `define_register`) with some additions:
   #   - Register is created in a class variable using the specified name/type
   #   - Defines singleton method to access the register
   #   - Defines instance method as a shortcut to the singleton method
-  #   - Automatically deletes the register on ::clear!
+  #   - Automatically deletes the register on registry.reset!
   def self.define_filtered_register(register_name)
     define_register(register_name, Array)
 
@@ -100,6 +109,9 @@ class DiscoursePluginRegistry
 
   define_filtered_register :user_destroyer_on_content_deletion_callbacks
 
+  define_filtered_register :hashtag_autocomplete_data_sources
+  define_filtered_register :hashtag_autocomplete_contextual_type_priorities
+
   def self.register_auth_provider(auth_provider)
     self.auth_providers << auth_provider
   end

lib/plugin/instance.rb

@@ -1097,13 +1097,21 @@ class Plugin::Instance
   # Used to register data sources for HashtagAutocompleteService to look
   # up results based on a #hashtag string.
   #
-  # @param {String} type - Roughly corresponding to a model, this is used as a unique
-  #                        key for the datasource and is also used when allowing different
-  #                        contexts to search for and lookup these types. The `category`
-  #                        and `tag` types are registered by default.
   # @param {Class} klass - Must be a class that implements methods with the following
   # signatures:
   #
+  #   Roughly corresponding to a model, this is used as a unique
+  #   key for the datasource and is also used when allowing different
+  #   contexts to search for and lookup these types. The `category`
+  #   and `tag` types are registered by default.
+  #   def self.type
+  #   end
+  #
+  #   The FontAwesome icon to use for the data source in the search results
+  #   and cooked markdown.
+  #   def self.icon
+  #   end
+  #
   #   @param {Guardian} guardian - Current user's guardian, used for permission-based filtering
   #   @param {Array} slugs - An array of strings that represent slugs to search this type for,
   #                          e.g. category slugs.
@@ -1117,8 +1125,20 @@ class Plugin::Instance
   #   @returns {Array} An Array of HashtagAutocompleteService::HashtagItem
   #   def self.search(guardian, term, limit)
   #   end
-  def register_hashtag_data_source(type, klass)
+  #
-    HashtagAutocompleteService.register_data_source(type, klass)
+  #   @param {Array} search_results - An array of HashtagAutocompleteService::HashtagItem to sort
+  #   @param {String} term - The search term which was used, which may help with sorting.
+  #   @returns {Array} An Array of HashtagAutocompleteService::HashtagItem
+  #   def self.search_sort(search_results, term)
+  #   end
+  #
+  #   @param {Guardian} guardian - Current user's guardian, used for permission-based filtering
+  #   @param {Integer} limit - The number of search results that should be returned by the query
+  #   @returns {Array} An Array of HashtagAutocompleteService::HashtagItem
+  #   def self.search_without_term(guardian, limit)
+  #   end
+  def register_hashtag_data_source(klass)
+    DiscoursePluginRegistry.register_hashtag_autocomplete_data_source(klass, self)
   end
 
   ##
@@ -1134,8 +1154,10 @@ class Plugin::Instance
   #                           for certain types of hashtag result.
   # @param {Integer} priority - A number value for ordering type results when hashtag searches
   #                             or lookups occur. Priority is ordered by DESCENDING order.
-  def register_hashtag_type_in_context(type, context, priority)
+  def register_hashtag_type_priority_for_context(type, context, priority)
-    HashtagAutocompleteService.register_type_in_context(type, context, priority)
+    DiscoursePluginRegistry.register_hashtag_autocomplete_contextual_type_priority(
+      { type: type, context: context, priority: priority }, self
+    )
   end
 
   ##

plugins/chat/lib/chat_channel_hashtag_data_source.rb

@@ -5,6 +5,10 @@ class Chat::ChatChannelHashtagDataSource
     "comment"
   end
 
+  def self.type
+    "channel"
+  end
+
   def self.channel_to_hashtag_item(guardian, channel)
     HashtagAutocompleteService::HashtagItem.new.tap do |item|
       item.text = channel.title

plugins/chat/plugin.rb

@@ -730,11 +730,11 @@ after_initialize do
   register_about_stat_group("chat_users") { Chat::Statistics.about_users }
 
   # Make sure to update spec/system/hashtag_autocomplete_spec.rb when changing this.
-  register_hashtag_data_source("channel", Chat::ChatChannelHashtagDataSource)
+  register_hashtag_data_source(Chat::ChatChannelHashtagDataSource)
-  register_hashtag_type_in_context("channel", "chat-composer", 200)
+  register_hashtag_type_priority_for_context("channel", "chat-composer", 200)
-  register_hashtag_type_in_context("category", "chat-composer", 100)
+  register_hashtag_type_priority_for_context("category", "chat-composer", 100)
-  register_hashtag_type_in_context("tag", "chat-composer", 50)
+  register_hashtag_type_priority_for_context("tag", "chat-composer", 50)
-  register_hashtag_type_in_context("channel", "topic-composer", 10)
+  register_hashtag_type_priority_for_context("channel", "topic-composer", 10)
 
   Site.markdown_additional_options["chat"] = {
     limited_pretty_text_features: ChatMessage::MARKDOWN_FEATURES,

plugins/chat/spec/models/chat_message_spec.rb

@@ -5,18 +5,6 @@ require "rails_helper"
 describe ChatMessage do
   fab!(:message) { Fabricate(:chat_message, message: "hey friend, what's up?!") }
 
-  # TODO (martin) Remove this after https://github.com/discourse/discourse/pull/19491 is merged
-  def register_hashtag_contexts
-    # This is annoying, but we need to reset the hashtag data sources inbetween
-    # tests, and since this is normally done in plugin.rb with the plugin API
-    # there is not an easier way to do this.
-    HashtagAutocompleteService.register_data_source("channel", Chat::ChatChannelHashtagDataSource)
-    HashtagAutocompleteService.register_type_in_context("channel", "chat-composer", 200)
-    HashtagAutocompleteService.register_type_in_context("category", "chat-composer", 100)
-    HashtagAutocompleteService.register_type_in_context("tag", "chat-composer", 50)
-    HashtagAutocompleteService.register_type_in_context("channel", "topic-composer", 10)
-  end
-
   describe ".cook" do
     it "does not support HTML tags" do
       cooked = ChatMessage.cook("<h1>test</h1>")
@@ -258,7 +246,7 @@ describe ChatMessage do
     end
 
     it "supports hashtag-autocomplete plugin" do
-      register_hashtag_contexts
+      SiteSetting.chat_enabled = true
       SiteSetting.enable_experimental_hashtag_autocomplete = true
 
       category = Fabricate(:category)
@@ -521,7 +509,7 @@ describe ChatMessage do
       fab!(:secure_category) { Fabricate(:private_category, group: group) }
 
       before do
-        register_hashtag_contexts
+        SiteSetting.chat_enabled = true
         SiteSetting.enable_experimental_hashtag_autocomplete = true
         SiteSetting.suppress_secured_categories_from_admin = true
       end

plugins/chat/spec/system/hashtag_autocomplete_spec.rb

@@ -19,15 +19,6 @@ describe "Using #hashtag autocompletion to search for and lookup channels",
   before do
     SiteSetting.enable_experimental_hashtag_autocomplete = true
 
-    # This is annoying, but we need to reset the hashtag data sources inbetween
-    # tests, and since this is normally done in plugin.rb with the plugin API
-    # there is not an easier way to do this.
-    HashtagAutocompleteService.register_data_source("channel", Chat::ChatChannelHashtagDataSource)
-    HashtagAutocompleteService.register_type_in_context("channel", "chat-composer", 200)
-    HashtagAutocompleteService.register_type_in_context("category", "chat-composer", 100)
-    HashtagAutocompleteService.register_type_in_context("tag", "chat-composer", 50)
-    HashtagAutocompleteService.register_type_in_context("channel", "topic-composer", 10)
-
     chat_system_bootstrap(user, [channel1, channel2])
     sign_in(user)
   end

spec/multisite/hashtag_autocomplete_service_spec.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+describe "HashtagAutocompleteService multisite registry", type: :multisite do
+  class MockPlugin
+    def initialize(setting_provider)
+      @setting_provider = setting_provider
+    end
+
+    def enabled?
+      @setting_provider.find("bookmark_hashtag_enabled")&.value == "true"
+    end
+  end
+
+  it "does not include the data source if one of the multisites has the plugin disabled" do
+    setting_provider = SiteSettings::LocalProcessProvider.new
+    DiscoursePluginRegistry.register_hashtag_autocomplete_data_source(
+      FakeBookmarkHashtagDataSource,
+      MockPlugin.new(setting_provider),
+    )
+
+    test_multisite_connection("default") do
+      setting_provider.save("bookmark_hashtag_enabled", true, 5)
+      expect(HashtagAutocompleteService.data_source_types).to eq(%w[category tag bookmark])
+    end
+
+    test_multisite_connection("second") do
+      expect(HashtagAutocompleteService.data_source_types).to eq(%w[category tag])
+    end
+  end
+end

spec/rails_helper.rb

@@ -154,9 +154,6 @@ module TestSetup
     # Make sure the default Post and Topic bookmarkables are registered
     Bookmark.reset_bookmarkables
 
-    # Make sure only the default category and tag hashtag data sources are registered.
-    HashtagAutocompleteService.clear_registered
-
     OmniAuth.config.test_mode = false
   end
 end

spec/services/hashtag_autocomplete_service_spec.rb

@@ -9,64 +9,21 @@ RSpec.describe HashtagAutocompleteService do
 
   subject { described_class.new(guardian) }
 
-  before { Site.clear_cache }
+  after { DiscoursePluginRegistry.reset! }
-
-  class BookmarkDataSource
-    def self.icon
-      "bookmark"
-    end
-
-    def self.lookup(guardian_scoped, slugs)
-      guardian_scoped
-        .user
-        .bookmarks
-        .where("LOWER(name) IN (:slugs)", slugs: slugs)
-        .map do |bm|
-          HashtagAutocompleteService::HashtagItem.new.tap do |item|
-            item.text = bm.name
-            item.slug = bm.name.gsub(" ", "-")
-            item.icon = icon
-          end
-        end
-    end
-
-    def self.search(
-      guardian_scoped,
-      term,
-      limit,
-      condition = HashtagAutocompleteService.search_conditions[:starts_with]
-    )
-      query = guardian_scoped.user.bookmarks
-
-      if condition == HashtagAutocompleteService.search_conditions[:starts_with]
-        query = query.where("name ILIKE ?", "#{term}%")
-      else
-        query = query.where("name ILIKE ?", "%#{term}%")
-      end
-
-      query
-        .limit(limit)
-        .map do |bm|
-          HashtagAutocompleteService::HashtagItem.new.tap do |item|
-            item.text = bm.name
-            item.slug = bm.name.gsub(" ", "-")
-            item.icon = icon
-          end
-        end
-    end
-
-    def self.search_sort(search_results, _)
-      search_results.sort_by { |item| item.text.downcase }
-    end
-  end
 
   describe ".contexts_with_ordered_types" do
     it "returns a hash of all the registrered search contexts and their types in the defined priority order" do
       expect(HashtagAutocompleteService.contexts_with_ordered_types).to eq(
         { "topic-composer" => %w[category tag] },
       )
-      HashtagAutocompleteService.register_type_in_context("category", "awesome-composer", 50)
+      DiscoursePluginRegistry.register_hashtag_autocomplete_contextual_type_priority(
-      HashtagAutocompleteService.register_type_in_context("tag", "awesome-composer", 100)
+        { type: "category", context: "awesome-composer", priority: 50 },
+        stub(enabled?: true),
+      )
+      DiscoursePluginRegistry.register_hashtag_autocomplete_contextual_type_priority(
+        { type: "tag", context: "awesome-composer", priority: 100 },
+        stub(enabled?: true),
+      )
       expect(HashtagAutocompleteService.contexts_with_ordered_types).to eq(
         { "topic-composer" => %w[category tag], "awesome-composer" => %w[tag category] },
       )
@@ -148,7 +105,10 @@ RSpec.describe HashtagAutocompleteService do
       Fabricate(:bookmark, user: user, name: "cool rock song")
       guardian.user.reload
 
-      HashtagAutocompleteService.register_data_source("bookmark", BookmarkDataSource)
+      DiscoursePluginRegistry.register_hashtag_autocomplete_data_source(
+        FakeBookmarkHashtagDataSource,
+        stub(enabled?: true),
+      )
 
       expect(subject.search("book", %w[category tag bookmark]).map(&:text)).to eq(
         ["The Book Club", "great-books x 22", "read review of this fantasy book"],
@@ -173,7 +133,10 @@ RSpec.describe HashtagAutocompleteService do
       Fabricate(:bookmark, user: user, name: "book club")
       guardian.user.reload
 
-      HashtagAutocompleteService.register_data_source("bookmark", BookmarkDataSource)
+      DiscoursePluginRegistry.register_hashtag_autocomplete_data_source(
+        FakeBookmarkHashtagDataSource,
+        stub(enabled?: true),
+      )
 
       expect(subject.search("book", %w[category tag bookmark]).map(&:ref)).to eq(
         %w[book-club the-book-club great-books the-book-club::tag],
@@ -428,7 +391,10 @@ RSpec.describe HashtagAutocompleteService do
       Fabricate(:bookmark, user: user, name: "coolrock")
       guardian.user.reload
 
-      HashtagAutocompleteService.register_data_source("bookmark", BookmarkDataSource)
+      DiscoursePluginRegistry.register_hashtag_autocomplete_data_source(
+        FakeBookmarkHashtagDataSource,
+        stub(enabled?: true),
+      )
 
       result = subject.lookup(["coolrock"], %w[category tag bookmark])
       expect(result[:bookmark].map(&:slug)).to eq(["coolrock"])

spec/support/fake_bookmark_hashtag_data_source.rb

@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+class FakeBookmarkHashtagDataSource
+  def self.icon
+    "bookmark"
+  end
+
+  def self.type
+    "bookmark"
+  end
+
+  def self.lookup(guardian_scoped, slugs)
+    guardian_scoped
+      .user
+      .bookmarks
+      .where("LOWER(name) IN (:slugs)", slugs: slugs)
+      .map do |bm|
+        HashtagAutocompleteService::HashtagItem.new.tap do |item|
+          item.text = bm.name
+          item.slug = bm.name.gsub(" ", "-")
+          item.icon = icon
+        end
+      end
+  end
+
+  def self.search(
+    guardian_scoped,
+    term,
+    limit,
+    condition = HashtagAutocompleteService.search_conditions[:starts_with]
+  )
+    query = guardian_scoped.user.bookmarks
+
+    if condition == HashtagAutocompleteService.search_conditions[:starts_with]
+      query = query.where("name ILIKE ?", "#{term}%")
+    else
+      query = query.where("name ILIKE ?", "%#{term}%")
+    end
+
+    query
+      .limit(limit)
+      .map do |bm|
+        HashtagAutocompleteService::HashtagItem.new.tap do |item|
+          item.text = bm.name
+          item.slug = bm.name.gsub(" ", "-")
+          item.icon = icon
+        end
+      end
+  end
+
+  def self.search_sort(search_results, _)
+    search_results.sort_by { |item| item.text.downcase }
+  end
+end