From c20c4abe3fd716776133bbc0b70804bae2cf45d7 Mon Sep 17 00:00:00 2001 From: rhymes Date: Thu, 29 Apr 2021 12:00:49 +0200 Subject: [PATCH] [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 | 8 +- app/queries/homepage/articles_query.rb | 13 +-- app/services/search/postgres/reading_list.rb | 19 +--- spec/models/article_spec.rb | 105 +++++++++++++++++++ 4 files changed, 111 insertions(+), 34 deletions(-) diff --git a/app/models/article.rb b/app/models/article.rb index 7174817ed..098ba2997 100644 --- a/app/models/article.rb +++ b/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 } diff --git a/app/queries/homepage/articles_query.rb b/app/queries/homepage/articles_query.rb index 98d1a81e7..d56e151fa 100644 --- a/app/queries/homepage/articles_query.rb +++ b/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 diff --git a/app/services/search/postgres/reading_list.rb b/app/services/search/postgres/reading_list.rb index d9fe6abf0..72232d65d 100644 --- a/app/services/search/postgres/reading_list.rb +++ b/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 diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index 91a43952e..7cfa6d9fa 100644 --- a/spec/models/article_spec.rb +++ b/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