DEV: Refactor `TopicsFilter` (#21071)

Why this change?

Previously `TopicsFilter` was designed in such a way that we act on a
filter sequentially based on the order it was matched. However, this
made it hard to support filters composition where a similar filter may
be present further in the query string. Because of this limitation, I
previously introduced a private API `TopicsFilter.register_scope` which
allows us to act on a filter only after the entire query string has been
scanned. However, I felt that it made the code complicated and hard to
reason about.

In thie commit, I've changed it such that we scan through the entire
query string and group the values of each filter together. This allows
us to act on the values of a given filter in one go which I find easier
to reason about. This also opens up the possibility for us to ignore
certain filters when it has been specified multiple times.

lib/topics_filter.rb

@@ -1,66 +1,51 @@
 # frozen_string_literal: true
 
 class TopicsFilter
-  def initialize(guardian:, scope: Topic)
+  attr_reader :topic_notification_levels
+
+  def initialize(guardian:, scope: Topic.all)
     @guardian = guardian
     @scope = scope
-    @notification_levels = Set.new
+    @topic_notification_levels = Set.new
-    @registered_scopes = {}
   end
 
-  def topic_notification_levels
+  FILTER_ALIASES = { "categories" => "category" }
-    @registered_scopes.dig(:topic_notification_levels, :params, :notification_levels) || []
+  private_constant :FILTER_ALIASES
-  end
 
   def filter_from_query_string(query_string)
     return @scope if query_string.blank?
 
+    filters = {}
+
     query_string.scan(
       /(?<key_prefix>[-=])?(?<key>[\w-]+):(?<value>[^\s]+)/,
     ) do |key_prefix, key, value|
-      case key
+      key = FILTER_ALIASES[key] || key
-      when "created-by"
-        filter_created_by_user(usernames: value.split(","))
-      when "in"
-        @scope = filter_state(state: value)
-      when "status"
-        @scope = filter_status(status: value)
-      when "tags"
-        value.scan(
-          /^(?<tag_names>([a-zA-Z0-9\-]+)(?<delimiter>[,+])?([a-zA-Z0-9\-]+)?(\k<delimiter>[a-zA-Z0-9\-]+)*)$/,
-        ) do |tag_names, delimiter|
-          break if key_prefix && key_prefix != "-"
 
-          match_all =
+      filters[key] ||= {}
-            if delimiter == ","
+      filters[key]["key_prefixes"] ||= []
-              false
+      filters[key]["key_prefixes"] << key_prefix
-            else
+      filters[key]["values"] ||= []
-              true
+      filters[key]["values"] << value
-            end
-
-          @scope =
-            filter_tags(
-              tag_names: tag_names.split(delimiter),
-              exclude: key_prefix.presence,
-              match_all: match_all,
-            )
-        end
-      when "category", "categories"
-        value.scan(
-          /^(?<category_slugs>([a-zA-Z0-9\-:]+)(?<delimiter>[,])?([a-zA-Z0-9\-:]+)?(\k<delimiter>[a-zA-Z0-9\-:]+)*)$/,
-        ) do |category_slugs, delimiter|
-          break if key_prefix && key_prefix != "="
-
-          @scope =
-            filter_categories(
-              category_slugs: category_slugs.split(delimiter),
-              exclude_subcategories: key_prefix.presence,
-            )
-        end
-      end
     end
 
-    @registered_scopes.each_value { |value| @scope = value[:block].call(*value[:params].values) }
+    filters.each do |filter, hash|
+      key_prefixes = hash["key_prefixes"]
+      values = hash["values"]
+
+      case filter
+      when "category"
+        filter_categories(values: key_prefixes.zip(values))
+      when "created-by"
+        filter_created_by_user(usernames: values.flat_map { |value| value.split(",") })
+      when "in"
+        filter_in(values: values)
+      when "status"
+        values.each { |status| @scope = filter_status(status: status) }
+      when "tags"
+        filter_tags(values: key_prefixes.zip(values))
+      end
+    end
 
     @scope
   end
@@ -92,63 +77,108 @@ class TopicsFilter
 
   private
 
-  def filter_created_by_user(usernames:)
+  def filter_categories(values:)
-    register_scope(
+    exclude_subcategories_category_slugs = []
-      key: :created_by_user,
+    include_subcategories_category_slugs = []
-      params: {
+
-        usernames: usernames.map(&:downcase),
+    values.each do |key_prefix, value|
-      },
+      break if key_prefix && key_prefix != "="
-    ) do |usernames_lower|
+
-      @scope.joins(:user).where("users.username_lower IN (?)", usernames_lower)
+      value
+        .scan(
+          /^(?<category_slugs>([a-zA-Z0-9\-:]+)(?<delimiter>[,])?([a-zA-Z0-9\-:]+)?(\k<delimiter>[a-zA-Z0-9\-:]+)*)$/,
+        )
+        .each do |category_slugs, delimiter|
+          (
+            if key_prefix.presence
+              exclude_subcategories_category_slugs
+            else
+              include_subcategories_category_slugs
+            end
+          ).concat(category_slugs.split(delimiter))
+        end
+    end
+
+    category_ids = []
+
+    if exclude_subcategories_category_slugs.present?
+      category_ids =
+        get_category_ids_from_slugs(
+          exclude_subcategories_category_slugs,
+          exclude_subcategories: true,
+        )
+    end
+
+    if include_subcategories_category_slugs.present?
+      category_ids.concat(
+        get_category_ids_from_slugs(
+          include_subcategories_category_slugs,
+          exclude_subcategories: false,
+        ),
+      )
+    end
+
+    if category_ids.present?
+      @scope = @scope.where("topics.category_id IN (?)", category_ids)
+    elsif exclude_subcategories_category_slugs.present? ||
+          include_subcategories_category_slugs.present?
+      @scope = @scope.none
     end
   end
 
-  def filter_state(state:)
+  def filter_created_by_user(usernames:)
-    case state
+    @scope =
-    when "pinned"
+      @scope.joins(:user).where(
-      @scope.where(
+        "users.username_lower IN (:usernames)",
-        "topics.pinned_at IS NOT NULL AND topics.pinned_until > topics.pinned_at AND ? < topics.pinned_until",
+        usernames: usernames.map(&:downcase),
-        Time.zone.now,
       )
-    when "bookmarked"
+  end
-      return @scope.none unless @guardian.user
 
-      @scope.joins(:topic_users).where(
+  def filter_in(values:)
-        "topic_users.bookmarked AND topic_users.user_id = ?",
+    values.uniq!
-        @guardian.user.id,
-      )
-    else
-      return @scope.none if !@guardian.user
 
-      topic_notification_levels = Set.new
+    if values.delete("pinned")
-
+      @scope =
-      state
+        @scope.where(
-        .split(",")
+          "topics.pinned_at IS NOT NULL AND topics.pinned_until > topics.pinned_at AND ? < topics.pinned_until",
-        .each do |topic_notification_level|
+          Time.zone.now,
-          if level = TopicUser.notification_levels[topic_notification_level.to_sym]
-            topic_notification_levels << level
-          end
-        end
-
-      register_scope(
-        key: :topic_notification_levels,
-        params: {
-          notification_levels: topic_notification_levels.to_a,
-        },
-      ) do |notification_levels|
-        @scope.joins(:topic_users).where(
-          "topic_users.notification_level IN (:topic_notification_levels) AND topic_users.user_id = :user_id",
-          topic_notification_levels: notification_levels,
-          user_id: @guardian.user.id,
         )
+    end
+
+    if @guardian.user
+      if values.delete("bookmarked")
+        @scope =
+          @scope.joins(:topic_users).where(
+            "topic_users.bookmarked AND topic_users.user_id = ?",
+            @guardian.user.id,
+          )
       end
 
-      @scope
+      if values.present?
+        values.each do |value|
+          value
+            .split(",")
+            .each do |topic_notification_level|
+              if level = TopicUser.notification_levels[topic_notification_level.to_sym]
+                @topic_notification_levels << level
+              end
+            end
+        end
+
+        @scope =
+          @scope.joins(:topic_users).where(
+            "topic_users.notification_level IN (:topic_notification_levels) AND topic_users.user_id = :user_id",
+            topic_notification_levels: @topic_notification_levels.to_a,
+            user_id: @guardian.user.id,
+          )
+      end
+    elsif values.present?
+      @scope = @scope.none
     end
   end
 
-  def filter_categories(category_slugs:, exclude_subcategories: false)
+  def get_category_ids_from_slugs(slugs, exclude_subcategories: false)
-    category_ids = Category.ids_from_slugs(category_slugs)
+    category_ids = Category.ids_from_slugs(slugs)
 
     category_ids =
       Category
@@ -156,46 +186,95 @@ class TopicsFilter
         .filter { |category| @guardian.can_see_category?(category) }
         .map(&:id)
 
-    # Don't return any records if the user does not have access to any of the categories
-    return @scope.none if category_ids.length < category_slugs.length
-
     if !exclude_subcategories
       category_ids = category_ids.flat_map { |category_id| Category.subcategory_ids(category_id) }
     end
 
-    register_scope(key: :categories, params: { category_ids: category_ids }) do |in_category_ids|
+    category_ids
-      @scope.joins(:category).where(
-        "categories.id IN (:category_ids)",
-        category_ids: in_category_ids,
-      )
-    end
-
-    @scope
   end
 
-  def filter_tags(tag_names:, match_all: true, exclude: false)
+  # Accepts an array of tag names and returns an array of tag ids and the tag ids of aliases for the tag names which the user can see.
-    return @scope if !SiteSetting.tagging_enabled?
+  # If a block is given, it will be called with the tag ids and alias tag ids as arguments.
-    return @scope if tag_names.blank?
+  def tag_ids_from_tag_names(tag_names)
-
+    tag_ids, alias_tag_ids =
-    tag_ids =
       DiscourseTagging
         .filter_visible(Tag, @guardian)
         .where_name(tag_names)
         .pluck(:id, :target_tag_id)
 
-    tag_ids.flatten!
+    tag_ids ||= []
-    tag_ids.uniq!
+    alias_tag_ids ||= []
-    tag_ids.compact!
 
-    return @scope.none if match_all && tag_ids.length != tag_names.length
+    yield(tag_ids, alias_tag_ids) if block_given?
-    return @scope if tag_ids.empty?
 
-    self.send(
+    all_tag_ids = tag_ids.concat(alias_tag_ids)
-      "#{exclude ? "exclude" : "include"}_topics_with_#{match_all ? "all" : "any"}_tags",
+    all_tag_ids.compact!
-      tag_ids,
+    all_tag_ids.uniq!
-    )
+    all_tag_ids
+  end
 
-    @scope
+  def filter_tags(values:)
+    return if !SiteSetting.tagging_enabled?
+
+    exclude_all_tags = []
+    exclude_any_tags = []
+    include_any_tags = []
+    include_all_tags = []
+
+    values.each do |key_prefix, value|
+      break if key_prefix && key_prefix != "-"
+
+      value.scan(
+        /^(?<tag_names>([a-zA-Z0-9\-]+)(?<delimiter>[,+])?([a-zA-Z0-9\-]+)?(\k<delimiter>[a-zA-Z0-9\-]+)*)$/,
+      ) do |tag_names, delimiter|
+        match_all =
+          if delimiter == ","
+            false
+          else
+            true
+          end
+
+        (
+          case [key_prefix, match_all]
+          in ["-", true]
+            exclude_all_tags
+          in ["-", false]
+            exclude_any_tags
+          in [nil, true]
+            include_all_tags
+          in [nil, false]
+            include_any_tags
+          end
+        ).concat(tag_names.split(delimiter))
+      end
+    end
+
+    if exclude_all_tags.present?
+      exclude_topics_with_all_tags(tag_ids_from_tag_names(exclude_all_tags))
+    end
+
+    if exclude_any_tags.present?
+      exclude_topics_with_any_tags(tag_ids_from_tag_names(exclude_any_tags))
+    end
+
+    if include_any_tags.present?
+      include_topics_with_any_tags(tag_ids_from_tag_names(include_any_tags))
+    end
+
+    if include_all_tags.present?
+      has_invalid_tags = false
+
+      all_tag_ids =
+        tag_ids_from_tag_names(include_all_tags) do |tag_ids, _|
+          has_invalid_tags = tag_ids.length < include_all_tags.length
+        end
+
+      if has_invalid_tags
+        @scope = @scope.none
+      else
+        include_topics_with_all_tags(all_tag_ids)
+      end
+    end
   end
 
   def topic_tags_alias
@@ -242,18 +321,4 @@ class TopicsFilter
   def include_topics_with_any_tags(tag_ids)
     @scope = @scope.joins(:topic_tags).where("topic_tags.tag_id IN (?)", tag_ids).distinct(:id)
   end
-
-  def register_scope(key:, params:, &block)
-    if registered_scope = @registered_scopes[key]
-      registered_scope[:params].each do |param_name, param_value|
-        if param_value.is_a?(Array)
-          registered_scope[:params][param_name].concat(params[param_name]).tap(&:uniq!)
-        else
-          registered_scope[:params][param_name] = params[param_name]
-        end
-      end
-    else
-      @registered_scopes[key] = { block: block, params: params }
-    end
-  end
 end