diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb
index 6cb765b7d..90d47fd15 100644
--- a/app/controllers/search_controller.rb
+++ b/app/controllers/search_controller.rb
@@ -120,20 +120,31 @@ class SearchController < ApplicationController
end
def feed_content
- feed_docs = if params[:class_name].blank?
- # If we are in the main feed and not filtering by type return
- # all articles, podcast episodes, and users
- feed_content_search.concat(user_search)
- elsif params[:class_name] == "User"
- # No need to check for articles or podcast episodes if we know we only want users
- user_search
- else
- # if params[:class_name] == PodcastEpisode, Article, or Comment then skip user lookup
- feed_content_search
- end
+ class_name = feed_params[:class_name].to_s.inquiry
+
+ result =
+ if class_name.blank?
+ # If we are in the main feed and not filtering by type return
+ # all articles, podcast episodes, and users
+ feed_content_search.concat(user_search)
+ elsif class_name.Comment? && FeatureFlag.enabled?(:search_2_comments)
+ Search::Postgres::Comment.search_documents(
+ page: feed_params[:page],
+ per_page: feed_params[:per_page],
+ sort_by: feed_params[:sort_by],
+ sort_direction: feed_params[:sort_direction],
+ term: feed_params[:search_fields],
+ )
+ elsif class_name.User?
+ # No need to check for articles or podcast episodes if we know we only want users
+ user_search
+ else
+ # if params[:class_name] == PodcastEpisode, Article, or Comment then skip user lookup
+ feed_content_search
+ end
render json: {
- result: feed_docs,
+ result: result,
display_jobs_banner: SiteConfig.display_jobs_banner,
jobs_url: SiteConfig.jobs_url
}
diff --git a/app/models/comment.rb b/app/models/comment.rb
index 999ca143b..49e28857c 100644
--- a/app/models/comment.rb
+++ b/app/models/comment.rb
@@ -2,6 +2,7 @@ class Comment < ApplicationRecord
has_ancestry
resourcify
+ include PgSearch::Model
include Reactable
include Searchable
@@ -9,7 +10,9 @@ class Comment < ApplicationRecord
SEARCH_CLASS = Search::FeedContent
BODY_MARKDOWN_SIZE_RANGE = (1..25_000).freeze
+
COMMENTABLE_TYPES = %w[Article PodcastEpisode].freeze
+
TITLE_DELETED = "[deleted]".freeze
TITLE_HIDDEN = "[hidden by post author]".freeze
@@ -28,7 +31,6 @@ class Comment < ApplicationRecord
has_many :mentions, as: :mentionable, inverse_of: :mentionable, dependent: :destroy
has_many :notifications, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all
has_many :notification_subscriptions, as: :notifiable, inverse_of: :notifiable, dependent: :destroy
-
before_validation :evaluate_markdown, if: -> { body_markdown }
before_save :set_markdown_character_count, if: :body_markdown
before_save :synchronous_spam_score_check
@@ -67,6 +69,25 @@ class Comment < ApplicationRecord
after_commit :remove_from_elasticsearch, on: [:destroy]
+ # [@atsmith813] this is adapted from the `search_field` property in
+ # `config/elasticsearch/mappings/feed_content.json` and
+ # `app/serializers/search/comment_serializer.rb`
+ #
+ # highlighter settings are taken from
+ # Search::QueryBuildersFeedContent#add_highlight_fields
+ pg_search_scope :search_comments,
+ against: %i[body_markdown],
+ using: {
+ tsearch: {
+ prefix: true,
+ highlight: {
+ StartSel: "",
+ StopSel: "",
+ MaxFragments: 2
+ }
+ }
+ }
+
scope :eager_load_serialized_data, -> { includes(:user, :commentable) }
alias touch_by_reaction save
diff --git a/app/serializers/search/postgres_comment_serializer.rb b/app/serializers/search/postgres_comment_serializer.rb
new file mode 100644
index 000000000..105ace8f5
--- /dev/null
+++ b/app/serializers/search/postgres_comment_serializer.rb
@@ -0,0 +1,48 @@
+module Search
+ # TODO[@atsmith813]: Rename this to CommentSerializer once Elasticsearch is removed
+ class PostgresCommentSerializer < ApplicationSerializer
+ attribute :id, &:search_id
+
+ attributes :path do |comment, params|
+ user = params[:users][comment.user_id]
+
+ if user
+ "/#{user.username}/comment/#{comment.id_code_generated}"
+ else
+ "/404.html"
+ end
+ end
+
+ attributes :public_reactions_count
+
+ attribute :body_text, &:body_markdown
+
+ attribute :class_name, -> { "Comment" }
+
+ attribute :highlight do |comment, _params|
+ {
+ body_text: [comment.pg_search_highlight]
+ }
+ rescue PgSearch::PgSearchHighlightNotSelected
+ # This is needed because in Search::Postgres::Comment we only call the
+ # search if a term is provided. This means if a user searches with a
+ # blank term, we skip the line that executes .with_pg_search_highlight.
+ # Skipping this AND trying to call .pg_search_highlight raises an error
+ # which we ignore here - basically filling in highlights if they're
+ # there.
+ end
+
+ attribute :hotness_score, &:score
+ attribute :published, -> { true }
+ attribute :published_at, &:created_at
+ attribute :readable_publish_date_string, &:readable_publish_date
+ attribute :title, &:commentable_title
+
+ # NOTE: not using the `NestedUserSerializer` to avoid hitting Redis to
+ # fetch the cached value
+ attribute :user do |comment, params|
+ user = params[:users][comment.user_id]
+ user.slice(:name, :profile_image_90, :username).symbolize_keys
+ end
+ end
+end
diff --git a/app/services/search/postgres/comment.rb b/app/services/search/postgres/comment.rb
new file mode 100644
index 000000000..2a94f3078
--- /dev/null
+++ b/app/services/search/postgres/comment.rb
@@ -0,0 +1,118 @@
+module Search
+ module Postgres
+ class Comment
+ ATTRIBUTES = [
+ "COALESCE(articles.published, false) AS commentable_published",
+ "COALESCE(articles.title, '') AS commentable_title",
+ "comments.body_markdown",
+ "comments.commentable_id",
+ "comments.commentable_type",
+ "comments.created_at",
+ "comments.id AS id",
+ "comments.public_reactions_count",
+ "comments.score",
+ "comments.user_id",
+ ].freeze
+ private_constant :ATTRIBUTES
+
+ USER_ATTRIBUTES = %i[
+ id
+ name
+ profile_image
+ username
+ ].freeze
+ private_constant :USER_ATTRIBUTES
+
+ ARTICLE_COMMENTABLE_QUERY = <<-SQL.freeze
+ LEFT JOIN articles
+ ON comments.commentable_id = articles.id
+ AND comments.commentable_type = 'Article'
+ SQL
+ private_constant :ARTICLE_COMMENTABLE_QUERY
+
+ DEFAULT_PER_PAGE = 60
+ private_constant :DEFAULT_PER_PAGE
+
+ DEFAULT_SORT_BY = "comments.score".freeze
+ private_constant :DEFAULT_SORT_BY
+
+ DEFAULT_SORT_DIRECTION = :desc
+ private_constant :DEFAULT_SORT_DIRECTION
+
+ MAX_PER_PAGE = 120 # to avoid querying too many items, we set a maximum amount for a page
+ private_constant :MAX_PER_PAGE
+
+ # We filter comments for those that are:
+ # 1. On Articles
+ # 2. Not deleted
+ # 3. Not hidden by commentable user (i.e. an Article author didn't hide the comment)
+ # 4. Are attached to published articles
+ QUERY_FILTER = <<-SQL.freeze
+ comments.commentable_type = 'Article' AND
+ comments.deleted = false AND
+ comments.hidden_by_commentable_user = false AND
+ articles.published = true
+ SQL
+ private_constant :QUERY_FILTER
+
+ def self.search_documents(
+ page: 0,
+ per_page: DEFAULT_PER_PAGE,
+ sort_by: DEFAULT_SORT_BY,
+ sort_direction: DEFAULT_SORT_DIRECTION,
+ term: nil
+ )
+ sort_by ||= DEFAULT_SORT_BY
+ # The UI and serializer rename created_at (the actual DB column name) to
+ # published_at
+ sort_by = "comments.created_at" if sort_by == "published_at"
+
+ sort_direction ||= DEFAULT_SORT_DIRECTION
+
+ # NOTE: [@rhymes/atsmith813] we should eventually update the frontend
+ # to start from page 1
+ page = page.to_i + 1
+ per_page = [(per_page || DEFAULT_PER_PAGE).to_i, MAX_PER_PAGE].min
+
+ relation = ::Comment.joins(ARTICLE_COMMENTABLE_QUERY).where(QUERY_FILTER)
+
+ relation = relation.search_comments(term).with_pg_search_highlight if term.present?
+
+ relation = relation.select(*ATTRIBUTES).reorder("#{sort_by}": sort_direction)
+
+ results = relation.page(page).per(per_page)
+
+ # NOTE: [@rhymes/atsmith813] an earlier version used `.includes(:user)`
+ # to preload users, unfortunately it's not possible in Rails to specify
+ # which fields of the included relation's table to select ahead of time.
+ # The `users` table is massive (115 columns on March 2021) and thus we
+ # shouldn't load it all in memory just to select a few fields.
+ # For these reasons I decided to avoid preloading altogether and issue
+ # an additional SQL query to load User objects
+ # (see https://github.com/forem/forem/pull/4744#discussion_r345698674
+ # and https://github.com/rails/rails/issues/15185#issuecomment-351868335
+ # for additional context)
+ user_ids = results.pluck(:user_id)
+ users = find_users(user_ids)
+
+ serialize(results, users)
+ end
+
+ def self.find_users(user_ids)
+ ::User
+ .where(id: user_ids)
+ .select(*USER_ATTRIBUTES)
+ .index_by(&:id)
+ end
+ private_class_method :find_users
+
+ def self.serialize(results, users)
+ Search::PostgresCommentSerializer
+ .new(results, params: { users: users }, is_collection: true)
+ .serializable_hash[:data]
+ .pluck(:attributes)
+ end
+ private_class_method :serialize
+ end
+ end
+end
diff --git a/spec/requests/search_spec.rb b/spec/requests/search_spec.rb
index c9b91dd5f..a735ed298 100644
--- a/spec/requests/search_spec.rb
+++ b/spec/requests/search_spec.rb
@@ -166,61 +166,87 @@ RSpec.describe "Search", type: :request, proper_status: true do
end
describe "GET /search/feed_content" do
- let(:mock_documents) { [{ "title" => "article1" }] }
+ context "when using Elasticsearch" do
+ let(:mock_documents) { [{ "title" => "article1" }] }
- it "returns json" do
- allow(Search::FeedContent).to receive(:search_documents).and_return(
- mock_documents,
- )
+ it "returns json" do
+ allow(Search::FeedContent).to receive(:search_documents).and_return(
+ mock_documents,
+ )
- get "/search/feed_content"
- expect(response.parsed_body["result"]).to eq(mock_documents)
+ get "/search/feed_content"
+ expect(response.parsed_body["result"]).to eq(mock_documents)
+ end
+
+ it "queries only the user index if class_name=User" do
+ allow(Search::FeedContent).to receive(:search_documents)
+ allow(Search::User).to receive(:search_documents).and_return(
+ mock_documents,
+ )
+
+ get "/search/feed_content?class_name=User"
+ expect(Search::User).to have_received(:search_documents)
+ expect(Search::FeedContent).not_to have_received(:search_documents)
+ end
+
+ it "queries for Articles, Podcast Episodes and Users if no class_name filter is present" do
+ allow(Search::FeedContent).to receive(:search_documents).and_return(
+ mock_documents,
+ )
+ allow(Search::User).to receive(:search_documents).and_return(
+ mock_documents,
+ )
+
+ get "/search/feed_content"
+ expect(Search::User).to have_received(:search_documents)
+ expect(Search::FeedContent).to have_received(:search_documents)
+ end
+
+ it "queries for only Articles and Podcast Episodes if class_name!=User" do
+ allow(Search::FeedContent).to receive(:search_documents).and_return(
+ mock_documents,
+ )
+ allow(Search::User).to receive(:search_documents)
+
+ get "/search/feed_content?class_name=Article"
+ expect(Search::User).not_to have_received(:search_documents)
+ expect(Search::FeedContent).to have_received(:search_documents)
+ end
+
+ it "queries for approved" do
+ allow(Search::FeedContent).to receive(:search_documents).and_return(
+ mock_documents,
+ )
+
+ get "/search/feed_content?class_name=Article&approved=true"
+ expect(Search::FeedContent).to have_received(:search_documents).with(
+ params: { "approved" => "true", "class_name" => "Article" },
+ )
+ end
end
- it "queries only the user index if class_name=User" do
- allow(Search::FeedContent).to receive(:search_documents)
- allow(Search::User).to receive(:search_documents).and_return(
- mock_documents,
- )
+ context "when using PostgreSQL" do
+ before do
+ allow(FeatureFlag).to receive(:enabled?).with(:search_2_comments).and_return(true)
+ end
- get "/search/feed_content?class_name=User"
- expect(Search::User).to have_received(:search_documents)
- expect(Search::FeedContent).not_to have_received(:search_documents)
- end
+ it "returns the correct keys for comments" do
+ create(:comment, body_markdown: "Ruby on Rails rocks!")
+ get search_feed_content_path(search_fields: "rails", class_name: "Comment")
+ expect(response.parsed_body["result"]).to be_present
+ end
- it "queries for Articles, Podcast Episodes and Users if no class_name filter is present" do
- allow(Search::FeedContent).to receive(:search_documents).and_return(
- mock_documents,
- )
- allow(Search::User).to receive(:search_documents).and_return(
- mock_documents,
- )
+ it "supports the search params for comments" do
+ comment = create(:comment, body_markdown: "Ruby on Rails rocks!")
+ get search_feed_content_path(
+ search_fields: "rails",
+ class_name: "Comment",
+ page: 0,
+ per_page: 1,
+ )
- get "/search/feed_content"
- expect(Search::User).to have_received(:search_documents)
- expect(Search::FeedContent).to have_received(:search_documents)
- end
-
- it "queries for only Articles and Podcast Episodes if class_name!=User" do
- allow(Search::FeedContent).to receive(:search_documents).and_return(
- mock_documents,
- )
- allow(Search::User).to receive(:search_documents)
-
- get "/search/feed_content?class_name=Article"
- expect(Search::User).not_to have_received(:search_documents)
- expect(Search::FeedContent).to have_received(:search_documents)
- end
-
- it "queries for approved" do
- allow(Search::FeedContent).to receive(:search_documents).and_return(
- mock_documents,
- )
-
- get "/search/feed_content?class_name=Article&approved=true"
- expect(Search::FeedContent).to have_received(:search_documents).with(
- params: { "approved" => "true", "class_name" => "Article" },
- )
+ expect(response.parsed_body["result"].first).to include("body_text" => comment.body_markdown)
+ end
end
end
diff --git a/spec/services/search/postgres/comment_spec.rb b/spec/services/search/postgres/comment_spec.rb
new file mode 100644
index 000000000..411cc1685
--- /dev/null
+++ b/spec/services/search/postgres/comment_spec.rb
@@ -0,0 +1,118 @@
+require "rails_helper"
+
+RSpec.describe Search::Postgres::Comment, type: :service do
+ let(:comment) { create(:comment) }
+
+ describe "::search_documents" do
+ context "when filtering Commentables" do
+ it "does not include comments from Articles that are unpublished", :aggregate_failures do
+ comment_text = "Ruby on Rails rocks!"
+ published_article = create(:article, title: "Published Article", published: true)
+ unpublished_article = create(:article, title: "Unpublished Article", published: true)
+ comment_on_published_article = create(:comment, body_markdown: comment_text, commentable: published_article)
+ comment_on_unpublished_article = create(:comment, body_markdown: comment_text, commentable: unpublished_article)
+ unpublished_article.update_columns(published: false)
+
+ result = described_class.search_documents(term: "rails")
+ # rubocop:disable Rails/PluckId
+ ids = result.pluck(:id)
+ # rubocop:enable Rails/PluckId
+
+ expect(ids).not_to include(comment_on_unpublished_article.search_id)
+ expect(ids).to include(comment_on_published_article.search_id)
+ end
+ end
+
+ context "when describing the result format" do
+ let(:result) { described_class.search_documents(term: comment.body_markdown) }
+
+ it "returns the correct attributes for the result" do
+ expected_keys = %i[
+ id path public_reactions_count body_text class_name highlight
+ hotness_score published published_at readable_publish_date_string
+ title user
+ ]
+
+ expect(result.first.keys).to match_array(expected_keys)
+ end
+
+ it "returns the correct attributes for the user" do
+ expected_keys = %i[username name profile_image_90]
+ expect(result.first[:user].keys).to match_array(expected_keys)
+ end
+
+ it "returns highlights" do
+ expected_keys = %i[body_text]
+ expect(result.first[:highlight].keys).to match_array(expected_keys)
+ highlights = result.first[:highlight][:body_text].first
+ expect(highlights).to include("", "")
+ end
+
+ it "orders the results by score (hotness_score) in descending order by default" do
+ comment_text = "Ruby on Rails rocks!"
+ comment = create(:comment, body_markdown: comment_text, score: 0)
+ hotter_comment = create(:comment, body_markdown: comment_text, score: 99)
+
+ result = described_class.search_documents(term: "rails")
+ scores = result.pluck(:hotness_score)
+
+ expect(scores).to eq([hotter_comment.score, comment.score])
+ end
+
+ it "orders the results by published_at (created_at) in descending order" do
+ comment_text = "Ruby on Rails rocks!"
+ comment = create(:comment, body_markdown: comment_text, score: 0)
+ older_comment = create(:comment, body_markdown: comment_text, score: 99, created_at: 1.day.ago)
+
+ result = described_class.search_documents(sort_by: "published_at", sort_direction: "desc", term: "rails")
+ # rubocop:disable Rails/PluckId
+ ids = result.pluck(:id)
+ # rubocop:enable Rails/PluckId
+
+ expect(ids).to eq([comment.search_id, older_comment.search_id])
+ end
+
+ it "orders the results by published_at (created_at) in ascending order" do
+ comment_text = "Ruby on Rails rocks!"
+ comment = create(:comment, body_markdown: comment_text, score: 0)
+ older_comment = create(:comment, body_markdown: comment_text, score: 99, created_at: 1.day.ago)
+
+ result = described_class.search_documents(sort_by: "published_at", sort_direction: "asc", term: "rails")
+ # rubocop:disable Rails/PluckId
+ ids = result.pluck(:id)
+ # rubocop:enable Rails/PluckId
+
+ expect(ids).to eq([older_comment.search_id, comment.search_id])
+ end
+ end
+
+ context "when searching for a term" do
+ it "matches against the comment's body_markdown (body_text)", :aggregate_failures do
+ comment.update_columns(body_markdown: "Ruby on Rails rocks!")
+ result = described_class.search_documents(term: "rails")
+
+ expect(result.first[:body_text]).to eq comment.body_markdown
+
+ result = described_class.search_documents(term: "javascript")
+ expect(result).to be_empty
+ end
+ end
+
+ context "when paginating" do
+ before { create_list(:comment, 2) }
+
+ it "returns no results when out of pagination bounds" do
+ result = described_class.search_documents(page: 99)
+ expect(result).to be_empty
+ end
+
+ it "returns paginated results", :aggregate_failures do
+ result = described_class.search_documents(page: 0, per_page: 1)
+ expect(result.length).to eq(1)
+
+ result = described_class.search_documents(page: 1, per_page: 1)
+ expect(result.length).to eq(1)
+ end
+ end
+ end
+end