From ad02e0f62c437ed11b0ded26495312de5e17e1bc Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 12 May 2020 21:08:52 -0500 Subject: [PATCH] posts/index: fix rating:s being included in page title in safe mode. Fixes bug described in https://github.com/danbooru/danbooru/commit/d3e4ac7c177486e962cdec328843db9568960f9a#commitcomment-39049351 When dealing with searches, there are several variables we have to keep in mind: * Whether tag aliases should be applied. * Whether search terms should be sorted. * Whether the rating:s and -status:deleted metatags should be added by safe mode and the hide deleted posts setting. Which of these things we need to do depends on the context: * We want to apply aliases when actually doing the search, calculating the count, looking up the wiki excerpt, recording missed/popular searches in Reportbooru, and calculating related tags for the sidebar, but not when displaying the raw search as typed by the user (for example, in the page title or in the tag search box). * We want to sort the search when calculating cache keys for fast_count or related tags, and when recording missed/popular searches, but not in the page title or when displaying the raw search. * We want to add rating:s and -status:deleted when performing the search, calculating the count, or recording missed/popular searches, but not when calculating related tags for the sidebar, or when displaying the page title or raw search. Here we introduce normalized_query and try to use it in contexts where query normalization is necessary. When to use the normalized query versus the raw unnormalized query is still subtle and prone to error. --- app/controllers/counts_controller.rb | 2 +- app/logical/post_query_builder.rb | 72 ++++++++++++++---------- app/logical/post_sets/post.rb | 15 ++--- app/logical/related_tag_query.rb | 2 +- app/models/post.rb | 7 ++- app/models/saved_search.rb | 4 +- app/models/upload.rb | 2 +- app/presenters/post_presenter.rb | 2 +- app/presenters/user_presenter.rb | 4 +- test/functional/posts_controller_test.rb | 7 +++ test/unit/post_query_builder_test.rb | 43 +++++++------- 11 files changed, 91 insertions(+), 69 deletions(-) diff --git a/app/controllers/counts_controller.rb b/app/controllers/counts_controller.rb index 556d4a028..35694b3ee 100644 --- a/app/controllers/counts_controller.rb +++ b/app/controllers/counts_controller.rb @@ -4,6 +4,6 @@ class CountsController < ApplicationController def posts estimate_count = params.fetch(:estimate_count, "true").truthy? skip_cache = params.fetch(:skip_cache, "false").truthy? - @count = PostQueryBuilder.new(params[:tags], CurrentUser.user).fast_count(timeout: CurrentUser.statement_timeout, estimate_count: estimate_count, skip_cache: skip_cache) + @count = PostQueryBuilder.new(params[:tags], CurrentUser.user).normalized_query.fast_count(timeout: CurrentUser.statement_timeout, estimate_count: estimate_count, skip_cache: skip_cache) end end diff --git a/app/logical/post_query_builder.rb b/app/logical/post_query_builder.rb index 6c98545d4..9783eb2ca 100644 --- a/app/logical/post_query_builder.rb +++ b/app/logical/post_query_builder.rb @@ -53,22 +53,15 @@ class PostQueryBuilder COUNT_METATAG_SYNONYMS.flat_map { |str| [str, "#{str}_asc"] } + CATEGORY_COUNT_METATAGS.flat_map { |str| [str, "#{str}_asc"] } - attr_reader :query_string, :terms, :current_user, :safe_mode, :hide_deleted_posts + attr_reader :query_string, :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, normalize_aliases: true, normalize_order: true, safe_mode: false, hide_deleted_posts: false) + def initialize(query_string, current_user = User.anonymous, 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 - @terms << OpenStruct.new(type: :metatag, name: "rating", value: "s") if safe_mode? - @terms << OpenStruct.new(type: :metatag, name: "status", value: "deleted", negated: true) if hide_deleted? - - normalize_aliases! if normalize_aliases - normalize_order! if normalize_order end def tags_match(tags, relation) @@ -457,11 +450,6 @@ class PostQueryBuilder relation end - def hide_deleted? - has_status_metatag = select_metatags(:status).any? { |metatag| metatag.value.downcase.in?(%w[deleted active any all]) } - hide_deleted_posts? && !has_status_metatag - end - def build tag_count = terms.count { |term| !Danbooru.config.is_unlimited_tag?(term) } if tag_count > Danbooru.config.tag_query_limit @@ -695,20 +683,6 @@ class PostQueryBuilder end end - 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 split_query end @@ -884,11 +858,51 @@ class PostQueryBuilder end end + concerning :NormalizationMethods do + def normalized_query(implicit: true, sort: true) + post_query = dup + post_query.terms.concat(implicit_metatags) if implicit + post_query.normalize_aliases! + post_query.normalize_order! if sort + post_query + end + + 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 implicit_metatags + metatags = [] + metatags << OpenStruct.new(type: :metatag, name: "rating", value: "s") if safe_mode? + metatags << OpenStruct.new(type: :metatag, name: "status", value: "deleted", negated: true) if hide_deleted? + metatags + end + + def hide_deleted? + has_status_metatag = select_metatags(:status).any? { |metatag| metatag.value.downcase.in?(%w[deleted active any all]) } + hide_deleted_posts? && !has_status_metatag + end + end + concerning :UtilityMethods do def to_s split_query.join(" ") end + def terms + @terms ||= scan_query + end + def tags terms.select { |term| term.type == :tag } end @@ -943,5 +957,5 @@ class PostQueryBuilder end end - memoize :split_query + memoize :split_query, :normalized_query end diff --git a/app/logical/post_sets/post.rb b/app/logical/post_sets/post.rb index dc8deafff..1757a1527 100644 --- a/app/logical/post_sets/post.rb +++ b/app/logical/post_sets/post.rb @@ -4,6 +4,7 @@ module PostSets MAX_SIDEBAR_TAGS = 25 attr_reader :page, :random, :post_count, :format, :tag_string, :query + delegate :normalized_query, to: :query def initialize(tags, page = 1, per_page = nil, random: false, format: "html") @query = PostQueryBuilder.new(tags, CurrentUser.user, safe_mode: CurrentUser.safe_mode?, hide_deleted_posts: CurrentUser.hide_deleted_posts?) @@ -29,8 +30,8 @@ module PostSets end def tag - return nil unless query.has_single_tag? - @tag ||= Tag.find_by(name: query.tags.first.name) + return nil unless normalized_query.has_single_tag? + @tag ||= Tag.find_by(name: normalized_query.tags.first.name) end def artist @@ -40,7 +41,7 @@ module PostSets end def pool - pool_names = query.select_metatags(:pool, :ordpool).map(&:value) + pool_names = normalized_query.select_metatags(:pool, :ordpool).map(&:value) name = pool_names.first return nil unless pool_names.size == 1 @@ -48,7 +49,7 @@ module PostSets end def favgroup - favgroup_names = query.select_metatags(:favgroup, :ordfavgroup).map(&:value) + favgroup_names = normalized_query.select_metatags(:favgroup, :ordfavgroup).map(&:value) name = favgroup_names.first return nil unless favgroup_names.size == 1 @@ -88,7 +89,7 @@ module PostSets # no need to get counts for formats that don't use a paginator nil else - query.fast_count + normalized_query.fast_count end end @@ -105,7 +106,7 @@ module PostSets if is_random? temp = get_random_posts else - temp = query.build.paginate(page, count: post_count, search_count: !post_count.nil?, limit: per_page) + temp = normalized_query.build.paginate(page, count: post_count, search_count: !post_count.nil?, limit: per_page) end end end @@ -176,7 +177,7 @@ module PostSets end def similar_tags - RelatedTagCalculator.cached_similar_tags_for_search(query, MAX_SIDEBAR_TAGS) + RelatedTagCalculator.cached_similar_tags_for_search(normalized_query(implicit: false), MAX_SIDEBAR_TAGS) end def frequent_tags diff --git a/app/logical/related_tag_query.rb b/app/logical/related_tag_query.rb index f59b64049..6a86b10b0 100644 --- a/app/logical/related_tag_query.rb +++ b/app/logical/related_tag_query.rb @@ -6,7 +6,7 @@ class RelatedTagQuery def initialize(query:, user: User.anonymous, category: nil, type: nil, limit: nil) @user = user - @post_query = PostQueryBuilder.new(query, user) + @post_query = PostQueryBuilder.new(query, user).normalized_query @query = @post_query.to_s @category = category @type = type diff --git a/app/models/post.rb b/app/models/post.rb index 6ae679bd6..5a01ee579 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, normalize_aliases: false).parse_tag_edit + new_tags = PostQueryBuilder.new(tag_string).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, normalize_aliases: false).parse_tag_edit + normalized_tags = PostQueryBuilder.new(tag_string).parse_tag_edit normalized_tags = apply_casesensitive_metatags(normalized_tags) normalized_tags = normalized_tags.map(&:downcase) normalized_tags = filter_metatags(normalized_tags) @@ -1475,7 +1475,8 @@ class Post < ApplicationRecord end def user_tag_match(query, user = CurrentUser.user, safe_mode: CurrentUser.safe_mode?, hide_deleted_posts: user.hide_deleted_posts?) - PostQueryBuilder.new(query, user, safe_mode: safe_mode, hide_deleted_posts: hide_deleted_posts).build + post_query = PostQueryBuilder.new(query, user, safe_mode: safe_mode, hide_deleted_posts: hide_deleted_posts) + post_query.normalized_query.build end def search(params) diff --git a/app/models/saved_search.rb b/app/models/saved_search.rb index 6849ba1d9..005f7dfa6 100644 --- a/app/models/saved_search.rb +++ b/app/models/saved_search.rb @@ -140,11 +140,11 @@ class SavedSearch < ApplicationRecord end def normalized_query - PostQueryBuilder.new(query).to_s + PostQueryBuilder.new(query).normalized_query.to_s end def normalize_query - self.query = PostQueryBuilder.new(query, normalize_order: false).to_s + self.query = PostQueryBuilder.new(query).normalized_query(sort: false).to_s end end diff --git a/app/models/upload.rb b/app/models/upload.rb index 96f386fc2..036bbdb95 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, normalize_aliases: false).find_metatag(:rating) + if rating = PostQueryBuilder.new(tag_string).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 7f1b0d5c6..1e0097094 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], normalize_aliases: false).has_metatag?(:order, :ordfav, :ordpool) + return false if PostQueryBuilder.new(params[:q]).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/presenters/user_presenter.rb b/app/presenters/user_presenter.rb index 10a876249..3a3add269 100644 --- a/app/presenters/user_presenter.rb +++ b/app/presenters/user_presenter.rb @@ -76,7 +76,7 @@ class UserPresenter end def commented_posts_count(template) - count = PostQueryBuilder.new("commenter:#{user.name}", User.system).fast_count + count = PostQueryBuilder.new("commenter:#{user.name}").fast_count count = "?" if count.nil? template.link_to(count, template.posts_path(:tags => "commenter:#{user.name} order:comment_bumped")) end @@ -90,7 +90,7 @@ class UserPresenter end def noted_posts_count(template) - count = PostQueryBuilder.new("noteupdater:#{user.name}", User.system).fast_count + count = PostQueryBuilder.new("noteupdater:#{user.name}").fast_count count = "?" if count.nil? template.link_to(count, template.posts_path(:tags => "noteupdater:#{user.name} order:note")) end diff --git a/test/functional/posts_controller_test.rb b/test/functional/posts_controller_test.rb index 8c90bca94..8d20d332b 100644 --- a/test/functional/posts_controller_test.rb +++ b/test/functional/posts_controller_test.rb @@ -305,6 +305,13 @@ class PostsControllerTest < ActionDispatch::IntegrationTest assert_select "#post_#{@post.id}", 1 end end + + context "in safe mode" do + should "not include the rating:s tag in the page title" do + get posts_path(tags: "1girl", safe_mode: true) + assert_select "title", text: "1girl Art | Safebooru" + end + end end context "show_seq action" do diff --git a/test/unit/post_query_builder_test.rb b/test/unit/post_query_builder_test.rb index e3eb07629..d74b08e8e 100644 --- a/test/unit/post_query_builder_test.rb +++ b/test/unit/post_query_builder_test.rb @@ -6,7 +6,7 @@ class PostQueryBuilderTest < ActiveSupport::TestCase end def assert_fast_count(count, query, query_options = {}, fast_count_options = {}) - assert_equal(count, PostQueryBuilder.new(query, **query_options).fast_count(**fast_count_options)) + assert_equal(count, PostQueryBuilder.new(query, **query_options).normalized_query.fast_count(**fast_count_options)) end setup do @@ -1018,7 +1018,6 @@ 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*), PostQueryBuilder.new("~AAa -BBB* -bbb*").split_query) end should "not strip out valid characters when scanning" do @@ -1050,22 +1049,22 @@ class PostQueryBuilderTest < ActiveSupport::TestCase end end - context "The normalize_query method" do + context "The normalized_query method" do should "work" do create(:tag_alias, antecedent_name: "gray", consequent_name: "grey") - 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) + assert_equal("foo", PostQueryBuilder.new("foo").normalized_query.to_s) + assert_equal("foo", PostQueryBuilder.new(" foo ").normalized_query.to_s) + assert_equal("foo", PostQueryBuilder.new("FOO").normalized_query.to_s) + assert_equal("foo", PostQueryBuilder.new("foo foo").normalized_query.to_s) + assert_equal("grey", PostQueryBuilder.new("gray").normalized_query.to_s) + assert_equal("aaa bbb", PostQueryBuilder.new("bbb aaa").normalized_query.to_s) + assert_equal("-aaa bbb", PostQueryBuilder.new("bbb -aaa").normalized_query.to_s) + assert_equal("~aaa ~bbb", PostQueryBuilder.new("~bbb ~aaa").normalized_query.to_s) + assert_equal("commentary:true bbb", PostQueryBuilder.new("bbb commentary:true").normalized_query.to_s) + assert_equal('commentary:"true" bbb', PostQueryBuilder.new("bbb commentary:'true'").normalized_query.to_s) + assert_equal('-commentary:true bbb', PostQueryBuilder.new("bbb -commentary:true").normalized_query.to_s) + assert_equal('-commentary:"true" bbb', PostQueryBuilder.new("bbb -commentary:'true'").normalized_query.to_s) end end @@ -1114,7 +1113,7 @@ class PostQueryBuilderTest < ActiveSupport::TestCase should "set the expiration time" do Cache.expects(:put).with(PostQueryBuilder.new("score:42 aaa").count_cache_key, 1, 180) - PostQueryBuilder.new("aaa score:42").fast_count + assert_fast_count(1, "aaa score:42") end should "work with the hide_deleted_posts option turned on" do @@ -1126,8 +1125,8 @@ class PostQueryBuilderTest < ActiveSupport::TestCase context "a blank search" do should "should execute a search" do - assert_equal(1, PostQueryBuilder.new("").fast_count(estimate_count: false)) - assert_nothing_raised { PostQueryBuilder.new("").fast_count(estimate_count: true) } + assert_fast_count(1, "", {}, { estimate_count: false }) + assert_nothing_raised { PostQueryBuilder.new("").normalized_query.fast_count(estimate_count: true) } end should "return 0 for a nonexisting tag" do @@ -1136,13 +1135,13 @@ class PostQueryBuilderTest < ActiveSupport::TestCase context "in safe mode" do should "work for a blank search" do - assert_equal(2, PostQueryBuilder.new("").fast_count(estimate_count: false)) - assert_nothing_raised { PostQueryBuilder.new("").fast_count(estimate_count: true) } + assert_fast_count(0, "", { safe_mode: true }, { estimate_count: false }) + assert_nothing_raised { PostQueryBuilder.new("", safe_mode: true).normalized_query.fast_count(estimate_count: true) } end should "work for a nil search" do - assert_equal(2, PostQueryBuilder.new(nil).fast_count(estimate_count: false)) - assert_nothing_raised { PostQueryBuilder.new(nil).fast_count(estimate_count: true) } + assert_fast_count(0, nil, { safe_mode: true }, { estimate_count: false }) + assert_nothing_raised { PostQueryBuilder.new("", safe_mode: true).normalized_query.fast_count(estimate_count: true) } end should "not fail for a two tag search by a member" do