[Search 2.0] Use Article.cached_tagged_with* in queries (#13566)

* Add Symbol to Article.cached_tagged_with* and fix error bug

* Use Article.cached_tagged_with* in search

* Apply PR feedback suggestions

app/models/article.rb

@@ -199,7 +199,7 @@ class Article < ApplicationRecord
 
   scope :cached_tagged_with, lambda { |tag|
     case tag
-    when String
+    when String, Symbol
       # In Postgres regexes, the [[:<:]] and [[:>:]] are equivalent to "start of
       # word" and "end of word", respectively. They're similar to `\b` in Perl-
       # compatible regexes (PCRE), but that matches at either end of a word.
@@ -216,16 +216,16 @@ class Article < ApplicationRecord
 
   scope :cached_tagged_with_any, lambda { |tags|
     case tags
-    when String
+    when String, Symbol
       cached_tagged_with(tags)
     when Array
       tags
         .map { |tag| cached_tagged_with(tag) }
         .reduce { |acc, elem| acc.or(elem) }
     when Tag
-      cached_tagged_with(tag.name)
+      cached_tagged_with(tags.name)
     else
-      raise TypeError, "Cannot search tags for: #{tag.inspect}"
+      raise TypeError, "Cannot search tags for: #{tags.inspect}"
     end
   }
 

app/queries/homepage/articles_query.rb

@@ -64,18 +64,7 @@ module Homepage
       @relation = @relation.where(published_at: published_at) if published_at.present?
       @relation = @relation.where(user_id: user_id) if user_id.present?
       @relation = @relation.where(organization_id: organization_id) if organization_id.present?
-
-      # as tags are in `OR` mode we can't use ActiveRecord's `.or()` because it
-      # would put all the previous filters in `OR` mode with tags, but what we need
-      # is to only consider tags as a `OR` sub-condition
-      if tags.present?
-        # `~` is the regexp operator, the `\m` modifier signifies the "beginning of word",
-        # and the `\M` modifier signifies the "end of word".
-        # see https://www.postgresql.org/docs/11/functions-matching.html#FUNCTIONS-POSIX-REGEXP
-        # for additional info
-        conditions = tags.map { |tag| relation.sanitize_sql_array(["cached_tag_list ~ ?", "\\m#{tag}\\M"]) }
-        @relation = @relation.where(conditions.join(" OR "))
-      end
+      @relation = @relation.cached_tagged_with_any(tags) if tags.any?
 
       relation
     end

app/services/search/postgres/reading_list.rb

@@ -78,24 +78,7 @@ module Search
 
         relation = relation.search_articles(term) if term.present?
 
-        # NOTE: [@rhymes] A previous version was implemented with:
-        # `.tagged_with(tags, any: false).reselect(*ATTRIBUTES)`
-        #
-        # =>`.tagged_with()` merges `articles.*` to the SQL, thus we needed to
-        #    use `reselect()`, see https://github.com/forem/forem/pull/12420
-        # => `.tagged_with()` with multiple tags constructs a monster query,
-        #    see https://explain.depesz.com/s/CqQV / https://explain.dalibo.com/plan/1Lm
-        # This is because the `acts-as-taggable-on` query creates a separate INNER JOIN
-        # per each tag that is added to the list, each new clause uses the `LIKE` operator on `tags.name`.
-        # That could have been improved by by adding a GIN index on `tags.name`, see
-        # https://www.cybertec-postgresql.com/en/postgresql-more-performance-for-like-and-ilike-statements/
-        # and a similar discussion https://github.com/forem/forem/pull/12584#discussion_r570756176
-        #
-        # The preferred solution, as we don't need the `Tag` model itself, is to use
-        # `articles.cached_tag_list` and the `~` regexp operator with it
-        tags.each do |tag|
-          relation = relation.where("articles.cached_tag_list ~ ?", "\\m#{tag}\\M")
-        end
+        relation = relation.cached_tagged_with(tags) if tags.any?
 
         # here we issue a COUNT(*) after all the conditions are applied,
         # because we need to fetch the total number of articles, pre pagination

spec/models/article_spec.rb

@@ -789,6 +789,30 @@ RSpec.describe Article, type: :model do
       expect(articles.to_a).to eq described_class.tagged_with("includeme").to_a
     end
 
+    it "can search for a single tag when given a symbol" do
+      included = create(:article, tags: "includeme")
+      excluded = create(:article, tags: "lol, nope")
+
+      articles = described_class.cached_tagged_with(:includeme)
+
+      expect(articles).to include(included)
+      expect(articles).not_to include(excluded)
+      expect(articles.to_a).to eq(described_class.tagged_with("includeme").to_a)
+    end
+
+    it "can search for a single tag when given a Tag object" do
+      included = create(:article, tags: "includeme")
+      excluded = create(:article, tags: "lol, nope")
+
+      tag = Tag.find_by(name: :includeme)
+
+      articles = described_class.cached_tagged_with(tag)
+
+      expect(articles).to include included
+      expect(articles).not_to include excluded
+      expect(articles.to_a).to eq described_class.tagged_with("includeme").to_a
+    end
+
     it "can search among multiple tags" do
       included = [
         create(:article, tags: "omg, wtf"),
@@ -815,6 +839,33 @@ RSpec.describe Article, type: :model do
       expect(articles).not_to include excluded_no_match
       expect(articles.to_a).to eq described_class.tagged_with(%w[includeme please]).to_a
     end
+
+    it "can search for multiple tags passed as an array of symbols" do
+      included = create(:article, tags: "includeme, please, lol")
+      excluded_partial_match = create(:article, tags: "excluded, please")
+      excluded_no_match = create(:article, tags: "excluded, omg")
+
+      articles = described_class.cached_tagged_with(%i[includeme please])
+
+      expect(articles).to include(included)
+      expect(articles).not_to include(excluded_partial_match)
+      expect(articles).not_to include(excluded_no_match)
+      expect(articles.to_a).to eq(described_class.tagged_with(%i[includeme please]).to_a)
+    end
+
+    it "can search for multiple tags passed as an array of Tag objects" do
+      included = create(:article, tags: "includeme, please, lol")
+      excluded_partial_match = create(:article, tags: "excluded, please")
+      excluded_no_match = create(:article, tags: "excluded, omg")
+
+      tags = Tag.where(name: %i[includeme please]).to_a
+      articles = described_class.cached_tagged_with(tags)
+
+      expect(articles).to include(included)
+      expect(articles).not_to include(excluded_partial_match)
+      expect(articles).not_to include(excluded_no_match)
+      expect(articles.to_a).to eq(described_class.tagged_with(%i[includeme please]).to_a)
+    end
   end
 
   describe ".cached_tagged_with_any" do
@@ -829,6 +880,29 @@ RSpec.describe Article, type: :model do
       expect(articles.to_a).to eq described_class.tagged_with("includeme", any: true).to_a
     end
 
+    it "can search for a single tag when given a symbol" do
+      included = create(:article, tags: "includeme")
+      excluded = create(:article, tags: "lol, nope")
+
+      articles = described_class.cached_tagged_with_any(:includeme)
+
+      expect(articles).to include(included)
+      expect(articles).not_to include(excluded)
+      expect(articles.to_a).to eq(described_class.tagged_with("includeme", any: true).to_a)
+    end
+
+    it "can search for a single tag when given a Tag object" do
+      included = create(:article, tags: "includeme")
+      excluded = create(:article, tags: "lol, nope")
+
+      tag = Tag.find_by(name: :includeme)
+      articles = described_class.cached_tagged_with_any(tag)
+
+      expect(articles).to include(included)
+      expect(articles).not_to include(excluded)
+      expect(articles.to_a).to eq(described_class.tagged_with("includeme", any: true).to_a)
+    end
+
     it "can search among multiple tags" do
       included = [
         create(:article, tags: "omg, wtf"),
@@ -858,6 +932,37 @@ RSpec.describe Article, type: :model do
 
       expect(articles.to_a).to include(*expected)
     end
+
+    it "can search for multiple tags when given an array of symbols" do
+      included = create(:article, tags: "includeme, please, lol")
+      included_partial_match = create(:article, tags: "includeme, omg")
+      excluded_no_match = create(:article, tags: "excluded, omg")
+
+      articles = described_class.cached_tagged_with_any(%i[includeme please])
+      expected = described_class.tagged_with(%i[includeme please], any: true).to_a
+
+      expect(articles).to include(included)
+      expect(articles).to include(included_partial_match)
+      expect(articles).not_to include(excluded_no_match)
+
+      expect(articles.to_a).to include(*expected)
+    end
+
+    it "can search for multiple tags when given an array of Tag objects" do
+      included = create(:article, tags: "includeme, please, lol")
+      included_partial_match = create(:article, tags: "includeme, omg")
+      excluded_no_match = create(:article, tags: "excluded, omg")
+
+      tags = Tag.where(name: %i[includeme please]).to_a
+      articles = described_class.cached_tagged_with_any(tags)
+      expected = described_class.tagged_with(%i[includeme please], any: true).to_a
+
+      expect(articles).to include(included)
+      expect(articles).to include(included_partial_match)
+      expect(articles).not_to include(excluded_no_match)
+
+      expect(articles.to_a).to include(*expected)
+    end
   end
 
   context "when callbacks are triggered before save" do