From 67aab0236d7d6814250811515fed792b3972d173 Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 7 May 2020 13:30:04 -0500 Subject: [PATCH] search: apply aliases after parsing searches. Make PostQueryBuilder apply aliases earlier, immediately after parsing the search. On the post index page there are multiple places where we need to apply aliases: * When running the search with PostQueryBuilder#build. * When calculating the search count with PostQueryBuilder#fast_count. * When calculating the related tags for the sidebar. * When tracking missed searches and popular searches for Reportbooru. * When looking up wiki excerpts. Applying aliases after parsing ensures we only have to apply aliases once for all of these things. We also normalize the order of tags in searches and strip repeated tags. This is so that we have consistent cache keys for fast_count. * Fixes searches for aliased tags being counted as missed searches (fixes #4433). * Fixes wiki excerpts not showing up when searching for aliased tags. --- app/helpers/posts_helper.rb | 6 +-- app/jobs/tag_batch_change_job.rb | 4 +- app/logical/post_query_builder.rb | 52 +++++++++++-------- app/logical/post_sets/post.rb | 2 +- app/logical/related_tag_query.rb | 2 +- app/models/post.rb | 4 +- app/models/saved_search.rb | 6 +-- app/models/upload.rb | 2 +- app/presenters/post_presenter.rb | 2 +- app/views/posts/index.html.erb | 4 +- .../posts/partials/index/_posts.html.erb | 2 +- test/functional/posts_controller_test.rb | 11 ++++ test/unit/post_query_builder_test.rb | 31 ++++++----- 13 files changed, 72 insertions(+), 56 deletions(-) diff --git a/app/helpers/posts_helper.rb b/app/helpers/posts_helper.rb index 57a2a9c61..c932758df 100644 --- a/app/helpers/posts_helper.rb +++ b/app/helpers/posts_helper.rb @@ -13,14 +13,12 @@ module PostsHelper params[:tags] =~ /order:rank/ || params[:action] =~ /searches|viewed/ end - def missed_post_search_count_js(post_set) - tags = post_set.query.normalize_query + def missed_post_search_count_js(tags) sig = generate_reportbooru_signature(tags) render "posts/partials/index/missed_search_count", sig: sig end - def post_search_count_js(post_set) - tags = post_set.query.normalize_query + def post_search_count_js(tags) sig = generate_reportbooru_signature("ps-#{tags}") render "posts/partials/index/search_count", sig: sig end diff --git a/app/jobs/tag_batch_change_job.rb b/app/jobs/tag_batch_change_job.rb index 00d9c5d7b..8667a37a1 100644 --- a/app/jobs/tag_batch_change_job.rb +++ b/app/jobs/tag_batch_change_job.rb @@ -6,8 +6,8 @@ class TagBatchChangeJob < ApplicationJob def perform(antecedent, consequent, updater, updater_ip_addr) raise Error.new("antecedent is missing") if antecedent.blank? - normalized_antecedent = TagAlias.to_aliased(PostQueryBuilder.new(antecedent.mb_chars.downcase).split_query) - normalized_consequent = TagAlias.to_aliased(PostQueryBuilder.new(consequent.mb_chars.downcase).parse_tag_edit) + normalized_antecedent = PostQueryBuilder.new(antecedent).split_query + normalized_consequent = PostQueryBuilder.new(consequent).parse_tag_edit CurrentUser.scoped(updater, updater_ip_addr) do migrate_posts(normalized_antecedent, normalized_consequent) diff --git a/app/logical/post_query_builder.rb b/app/logical/post_query_builder.rb index 0136a0e8c..36b8a3eaa 100644 --- a/app/logical/post_query_builder.rb +++ b/app/logical/post_query_builder.rb @@ -53,15 +53,18 @@ class PostQueryBuilder COUNT_METATAG_SYNONYMS.flat_map { |str| [str, "#{str}_asc"] } + CATEGORY_COUNT_METATAGS.flat_map { |str| [str, "#{str}_asc"] } - attr_reader :query_string, :current_user, :safe_mode, :hide_deleted_posts + attr_reader :query_string, :terms, :current_user, :safe_mode, :hide_deleted_posts alias_method :safe_mode?, :safe_mode alias_method :hide_deleted_posts?, :hide_deleted_posts - def initialize(query_string, current_user = User.anonymous, safe_mode: false, hide_deleted_posts: false) + def initialize(query_string, current_user = User.anonymous, normalize_aliases: true, normalize_order: true, safe_mode: false, hide_deleted_posts: false) @query_string = query_string @current_user = current_user @safe_mode = safe_mode @hide_deleted_posts = hide_deleted_posts + @terms = scan_query + normalize_aliases! if normalize_aliases + normalize_order! if normalize_order end def tags_match(tags, relation) @@ -71,9 +74,9 @@ class PostQueryBuilder optional_wildcard_tags, optional_tags = tags.select(&:optional).partition(&:wildcard) required_wildcard_tags, required_tags = tags.reject(&:negated).reject(&:optional).partition(&:wildcard) - negated_tags = TagAlias.to_aliased(negated_tags.map(&:name)) - optional_tags = TagAlias.to_aliased(optional_tags.map(&:name)) - required_tags = TagAlias.to_aliased(required_tags.map(&:name)) + negated_tags = negated_tags.map(&:name) + optional_tags = optional_tags.map(&:name) + required_tags = required_tags.map(&:name) negated_tags += negated_wildcard_tags.flat_map { |tag| Tag.wildcard_matches(tag.name) } optional_tags += optional_wildcard_tags.flat_map { |tag| Tag.wildcard_matches(tag.name) } @@ -671,7 +674,7 @@ class PostQueryBuilder end def split_query - scan_query.map do |term| + terms.map do |term| if term.type == :metatag && !term.negated && !term.quoted "#{term.name}:#{term.value}" elsif term.type == :metatag && !term.negated && term.quoted @@ -690,13 +693,18 @@ class PostQueryBuilder end end - def normalize_query(normalize_aliases: false, sort: true) - tags = split_query - tags = tags.map { |t| Tag.normalize_name(t) } - tags = TagAlias.to_aliased(tags) if normalize_aliases - tags = tags.sort if sort - tags = tags.uniq - tags.join(" ") + def normalize_aliases! + tag_names = tags.map(&:name) + tag_aliases = tag_names.zip(TagAlias.to_aliased(tag_names)).to_h + + terms.map! do |term| + term.name = tag_aliases[term.name] if term.type == :tag + term + end + end + + def normalize_order! + terms.sort_by!(&:to_s).uniq! end def parse_tag_edit @@ -816,7 +824,7 @@ class PostQueryBuilder concerning :CountMethods do def fast_count(timeout: 1_000, raise_on_timeout: false, skip_cache: false) - tags = normalize_query(normalize_aliases: true) + tags = to_s tags += " rating:s" if safe_mode? tags += " -status:deleted" if hide_deleted? tags = tags.strip @@ -876,7 +884,7 @@ class PostQueryBuilder end def get_count_from_cache(tags) - if PostQueryBuilder.new(tags).is_simple_tag? + if PostQueryBuilder.new(tags, normalize_aliases: false).is_simple_tag? count = Tag.find_by(name: tags).try(:post_count) else # this will only have a value for multi-tag searches or single metatag searches @@ -898,16 +906,16 @@ class PostQueryBuilder end concerning :UtilityMethods do - def terms - scan_query + def to_s + split_query.join(" ") end def tags - scan_query.select { |term| term.type == :tag } + terms.select { |term| term.type == :tag } end def metatags - scan_query.select { |term| term.type == :metatag } + terms.select { |term| term.type == :metatag } end def select_metatags(*names) @@ -935,11 +943,11 @@ class PostQueryBuilder end def is_empty_search? - scan_query.size == 0 + terms.size == 0 end def is_single_term? - scan_query.size == 1 + terms.size == 1 end def is_single_tag? @@ -956,5 +964,5 @@ class PostQueryBuilder end end - memoize :scan_query, :split_query, :normalize_query + memoize :split_query end diff --git a/app/logical/post_sets/post.rb b/app/logical/post_sets/post.rb index 8065699ef..fc8f0d836 100644 --- a/app/logical/post_sets/post.rb +++ b/app/logical/post_sets/post.rb @@ -103,7 +103,7 @@ module PostSets if is_random? temp = get_random_posts else - temp = ::Post.user_tag_match(tag_string).where("true /* PostSets::Post#posts:2 */").paginate(page, :count => post_count, :limit => per_page) + temp = query.build.paginate(page, count: post_count, limit: per_page) end end end diff --git a/app/logical/related_tag_query.rb b/app/logical/related_tag_query.rb index f3ac9beb0..de6c4dc01 100644 --- a/app/logical/related_tag_query.rb +++ b/app/logical/related_tag_query.rb @@ -56,7 +56,7 @@ class RelatedTagQuery versions = PostVersion.where(updater_id: user.id).where("updated_at > ?", since).order(id: :desc).limit(max_edits) tags = versions.flat_map(&:added_tags) - tags = tags.select { |tag| PostQueryBuilder.new(tag).is_simple_tag? } + tags = tags.select { |tag| PostQueryBuilder.new(tag, normalize_aliases: false).is_simple_tag? } tags = tags.group_by(&:itself).transform_values(&:size).sort_by { |tag, count| [-count, tag] }.map(&:first) tags.take(max_tags) end diff --git a/app/models/post.rb b/app/models/post.rb index dc9d9eb23..dbee7b602 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -604,7 +604,7 @@ class Post < ApplicationRecord # If someone else committed changes to this post before we did, # then try to merge the tag changes together. current_tags = tag_string_was.split - new_tags = PostQueryBuilder.new(tag_string).split_query + new_tags = PostQueryBuilder.new(tag_string, normalize_aliases: false).parse_tag_edit old_tags = old_tag_string.split kept_tags = current_tags & new_tags @@ -642,7 +642,7 @@ class Post < ApplicationRecord end def normalize_tags - normalized_tags = PostQueryBuilder.new(tag_string).split_query + normalized_tags = PostQueryBuilder.new(tag_string, normalize_aliases: false).parse_tag_edit normalized_tags = apply_casesensitive_metatags(normalized_tags) normalized_tags = normalized_tags.map(&:downcase) normalized_tags = filter_metatags(normalized_tags) diff --git a/app/models/saved_search.rb b/app/models/saved_search.rb index c194d5807..9c37f03d3 100644 --- a/app/models/saved_search.rb +++ b/app/models/saved_search.rb @@ -134,17 +134,17 @@ class SavedSearch < ApplicationRecord def queries_for(user_id, label: nil, options: {}) searches = SavedSearch.where(user_id: user_id) searches = searches.labeled(label) if label.present? - queries = searches.pluck(:query).map { |query| PostQueryBuilder.new(query).normalize_query(normalize_aliases: true, sort: true) } + queries = searches.pluck(:query).map { |query| PostQueryBuilder.new(query).to_s } queries.sort.uniq end end def normalized_query - PostQueryBuilder.new(query).normalize_query(sort: true) + PostQueryBuilder.new(query, normalize_aliases: false).to_s end def normalize_query - self.query = PostQueryBuilder.new(query).normalize_query(normalize_aliases: true, sort: false) + self.query = PostQueryBuilder.new(query, normalize_order: false).to_s end end diff --git a/app/models/upload.rb b/app/models/upload.rb index 036bbdb95..96f386fc2 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -237,7 +237,7 @@ class Upload < ApplicationRecord include SourceMethods def assign_rating_from_tags - if rating = PostQueryBuilder.new(tag_string).find_metatag(:rating) + if rating = PostQueryBuilder.new(tag_string, normalize_aliases: false).find_metatag(:rating) self.rating = rating.downcase.first end end diff --git a/app/presenters/post_presenter.rb b/app/presenters/post_presenter.rb index 1e0097094..7f1b0d5c6 100644 --- a/app/presenters/post_presenter.rb +++ b/app/presenters/post_presenter.rb @@ -163,7 +163,7 @@ class PostPresenter end def has_sequential_navigation?(params) - return false if PostQueryBuilder.new(params[:q]).has_metatag?(:order, :ordfav, :ordpool) + return false if PostQueryBuilder.new(params[:q], normalize_aliases: false).has_metatag?(:order, :ordfav, :ordpool) return false if params[:pool_id].present? || params[:favgroup_id].present? return CurrentUser.user.enable_sequential_post_navigation end diff --git a/app/views/posts/index.html.erb b/app/views/posts/index.html.erb index 4eff24ec9..0fecc10af 100644 --- a/app/views/posts/index.html.erb +++ b/app/views/posts/index.html.erb @@ -51,9 +51,9 @@ <% if post_search_counts_enabled? && @post_set.query.is_simple_tag? && @post_set.current_page == 1 %> <% if @post_set.post_count == 0 %> - <%= missed_post_search_count_js(@post_set) %> + <%= missed_post_search_count_js(@post_set.query.to_s) %> <% else %> - <%= post_search_count_js(@post_set) %> + <%= post_search_count_js(@post_set.query.to_s) %> <% end %> <% end %> diff --git a/app/views/posts/partials/index/_posts.html.erb b/app/views/posts/partials/index/_posts.html.erb index 759007c15..672d42e8e 100644 --- a/app/views/posts/partials/index/_posts.html.erb +++ b/app/views/posts/partials/index/_posts.html.erb @@ -20,7 +20,7 @@ <% end %> <% if CurrentUser.user.is_member? && post_set.tag.present? && post_set.current_page == 1 %> - <% cache("tag-change-notice:#{post_set.query.normalize_query}", expires_in: 4.hours) do %> + <% cache("tag-change-notice:#{post_set.tag.name}", expires_in: 4.hours) do %> <% if post_set.pending_bulk_update_requests.present? %>

diff --git a/test/functional/posts_controller_test.rb b/test/functional/posts_controller_test.rb index f45cbcf57..41cf2c168 100644 --- a/test/functional/posts_controller_test.rb +++ b/test/functional/posts_controller_test.rb @@ -68,6 +68,17 @@ class PostsControllerTest < ActionDispatch::IntegrationTest assert_select "#show-excerpt-link", count: 0 end + should "render for an aliased tag" do + create(:tag_alias, antecedent_name: "/lav", consequent_name: "looking_at_viewer") + as(@user) { create(:wiki_page, title: "looking_at_viewer") } + @post = create(:post, tag_string: "looking_at_viewer", rating: "s") + + get posts_path, params: { tags: "/lav" } + assert_response :success + assert_select "#post_#{@post.id}", count: 1 + assert_select "#excerpt .wiki-link[href='/wiki_pages/looking_at_viewer']", count: 1 + end + should "render for a wildcard tag search" do create(:post, tag_string: "1girl solo") get posts_path(tags: "*girl*") diff --git a/test/unit/post_query_builder_test.rb b/test/unit/post_query_builder_test.rb index 64d01e46c..205fde052 100644 --- a/test/unit/post_query_builder_test.rb +++ b/test/unit/post_query_builder_test.rb @@ -1018,7 +1018,7 @@ class PostQueryBuilderTest < ActiveSupport::TestCase context "Parsing:" do should "split a query" do assert_equal(%w(aaa bbb), PostQueryBuilder.new("aaa bbb").split_query) - assert_equal(%w(~aaa -bbb* -bbb*), PostQueryBuilder.new("~AAa -BBB* -bbb*").split_query) + assert_equal(%w(~aaa -bbb*), PostQueryBuilder.new("~AAa -BBB* -bbb*").split_query) end should "not strip out valid characters when scanning" do @@ -1054,19 +1054,18 @@ class PostQueryBuilderTest < ActiveSupport::TestCase should "work" do create(:tag_alias, antecedent_name: "gray", consequent_name: "grey") - assert_equal("foo", PostQueryBuilder.new("foo").normalize_query) - assert_equal("foo", PostQueryBuilder.new(" foo ").normalize_query) - assert_equal("foo", PostQueryBuilder.new("FOO").normalize_query) - assert_equal("foo", PostQueryBuilder.new("foo foo").normalize_query) - assert_equal("gray", PostQueryBuilder.new("gray").normalize_query) - assert_equal("grey", PostQueryBuilder.new("gray").normalize_query(normalize_aliases: true)) - assert_equal("aaa bbb", PostQueryBuilder.new("bbb aaa").normalize_query) - assert_equal("-aaa bbb", PostQueryBuilder.new("bbb -aaa").normalize_query) - assert_equal("~aaa ~bbb", PostQueryBuilder.new("~bbb ~aaa").normalize_query) - assert_equal("bbb commentary:true", PostQueryBuilder.new("bbb commentary:true").normalize_query) - assert_equal('bbb commentary:"true"', PostQueryBuilder.new("bbb commentary:'true'").normalize_query) - assert_equal('-commentary:true bbb', PostQueryBuilder.new("bbb -commentary:true").normalize_query) - assert_equal('-commentary:"true" bbb', PostQueryBuilder.new("bbb -commentary:'true'").normalize_query) + assert_equal("foo", PostQueryBuilder.new("foo").to_s) + assert_equal("foo", PostQueryBuilder.new(" foo ").to_s) + assert_equal("foo", PostQueryBuilder.new("FOO").to_s) + assert_equal("foo", PostQueryBuilder.new("foo foo").to_s) + assert_equal("grey", PostQueryBuilder.new("gray").to_s) + assert_equal("aaa bbb", PostQueryBuilder.new("bbb aaa").to_s) + assert_equal("-aaa bbb", PostQueryBuilder.new("bbb -aaa").to_s) + assert_equal("~aaa ~bbb", PostQueryBuilder.new("~bbb ~aaa").to_s) + assert_equal("commentary:true bbb", PostQueryBuilder.new("bbb commentary:true").to_s) + assert_equal('commentary:"true" bbb', PostQueryBuilder.new("bbb commentary:'true'").to_s) + assert_equal('-commentary:true bbb', PostQueryBuilder.new("bbb -commentary:true").to_s) + assert_equal('-commentary:"true" bbb', PostQueryBuilder.new("bbb -commentary:'true'").to_s) end end @@ -1107,7 +1106,7 @@ class PostQueryBuilderTest < ActiveSupport::TestCase context "for a multi-tag search" do should "return the cached count, if it exists" do - PostQueryBuilder.new(nil).set_count_in_cache("aaa score:42", 100) + PostQueryBuilder.new(nil).set_count_in_cache("score:42 aaa", 100) assert_fast_count(100, "aaa score:42") end @@ -1116,7 +1115,7 @@ class PostQueryBuilderTest < ActiveSupport::TestCase end should "set the expiration time" do - Cache.expects(:put).with(PostQueryBuilder.new(nil).count_cache_key("aaa score:42"), 1, 180) + Cache.expects(:put).with(PostQueryBuilder.new(nil).count_cache_key("score:42 aaa"), 1, 180) PostQueryBuilder.new("aaa score:42").fast_count end