From da3e8e4726c05e381e68bbf464f93b344d8385fe Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 7 Jan 2021 15:59:56 -0600 Subject: [PATCH] searchable: fix bug with searching multiple association attributes. Fix a bug with searches like the following not working correctly: * https://danbooru.donmai.us/comments.json?search[creator][level]=20&search[creator_id]=1234 * https://danbooru.donmai.us/comments.json?search[creator][level]=20&search[creator_name]=abcd * https://danbooru.donmai.us/comments.json?search[post][rating]=s&search[post_tags_match]=touhou It wasn't possible to search for both `creator` and `creator_id` at the same time (or `post` and `post_tags_match`, etc). Only the `creator_id` param would be recognized. Also refactor some internals: * `search_includes` was renamed to `search_associated_attribute`. * `search_attribute` was split up into `search_basic_attribute` and `search_associated_attribute`. --- app/logical/concerns/searchable.rb | 74 +++++++++++++------------- test/unit/concerns/searchable.rb | 85 +++++++++++++++++++++++++++++- 2 files changed, 121 insertions(+), 38 deletions(-) diff --git a/app/logical/concerns/searchable.rb b/app/logical/concerns/searchable.rb index 245de2545..aa6e4971f 100644 --- a/app/logical/concerns/searchable.rb +++ b/app/logical/concerns/searchable.rb @@ -10,10 +10,6 @@ module Searchable 1 + params.values.map { |v| parameter_hash?(v) ? parameter_depth(v) : 1 }.max end - def prefix_matches(prefix, params) - params.keys.any? { |x| x.starts_with?(prefix) } - end - def negate_relation unscoped.where(all.where_clause.invert.ast) end @@ -172,8 +168,18 @@ module Searchable end def search_attribute(name, params, current_user) + if has_attribute?(name) + search_basic_attribute(name, params, current_user) + elsif reflections.has_key?(name.to_s) + search_association_attribute(name, params, current_user) + else + raise ArgumentError, "#{name} is not an attribute or association" + end + end + + def search_basic_attribute(name, params, current_user) column = column_for_attribute(name) - type = column.type || reflect_on_association(name)&.class_name + type = column.type if column.try(:array?) subtype = type @@ -183,12 +189,6 @@ module Searchable end case type - when "User" - search_user_attribute(name, params, current_user) - when "Post" - search_post_attribute(name, params, current_user) - when "Model" - search_polymorphic_attribute(name, params, current_user) when :string, :text search_text_attribute(name, params) when :boolean @@ -202,8 +202,7 @@ module Searchable when :array search_array_attribute(name, subtype, params) else - raise NotImplementedError, "unhandled attribute type: #{name}" if type.blank? - search_includes(name, params, type, current_user) + raise NotImplementedError, "unhandled attribute type: #{name}" end end @@ -265,33 +264,36 @@ module Searchable end end - def search_user_attribute(attr, params, current_user) - if params["#{attr}_name"].present? - where(attr => User.search(name_matches: params["#{attr}_name"]).reorder(nil)) - else - search_includes(attr, params, "User", current_user) - end - end + def search_association_attribute(attr, params, current_user) + association = reflect_on_association(attr) + relation = all - def search_post_attribute(attr, params, current_user) - if params["#{attr}_tags_match"] - where(attr => Post.user_tag_match(params["#{attr}_tags_match"], current_user).reorder(nil)) - else - search_includes(attr, params, "Post", current_user) + if association.polymorphic? + return search_polymorphic_attribute(attr, params, current_user) end - end - def search_includes(attr, params, type, current_user) - model = Kernel.const_get(type) - if prefix_matches("#{attr}_id", params) - search_attribute("#{attr}_id", params, current_user) - elsif params["has_#{attr}"].to_s.truthy? || params["has_#{attr}"].to_s.falsy? - search_has_include(attr, params["has_#{attr}"].to_s.truthy?, model) - elsif parameter_hash?(params[attr]) - where(attr => model.visible(current_user).search(params[attr]).reorder(nil)) - else - all + if association.belongs_to? + relation = relation.search_attribute(association.foreign_key, params, current_user) end + + model = association.klass + if model == User && params["#{attr}_name"].present? + relation = relation.where(attr => User.search(name_matches: params["#{attr}_name"]).reorder(nil)) + end + + if model == Post && params["#{attr}_tags_match"].present? + relation = relation.where(attr => Post.user_tag_match(params["#{attr}_tags_match"], current_user).reorder(nil)) + end + + if params["has_#{attr}"].to_s.truthy? || params["has_#{attr}"].to_s.falsy? + relation = relation.search_has_include(attr, params["has_#{attr}"].to_s.truthy?, model) + end + + if parameter_hash?(params[attr]) + relation = relation.where(attr => model.visible(current_user).search(params[attr]).reorder(nil)) + end + + relation end def search_polymorphic_attribute(attr, params, current_user) diff --git a/test/unit/concerns/searchable.rb b/test/unit/concerns/searchable.rb index 8dace9e16..8563680b4 100644 --- a/test/unit/concerns/searchable.rb +++ b/test/unit/concerns/searchable.rb @@ -1,8 +1,10 @@ require 'test_helper' class SearchableTest < ActiveSupport::TestCase - def assert_search_equals(results, **params) - assert_equal(Array(results).map(&:id), subject.search(**params).ids) + def assert_search_equals(results, current_user: User.anonymous, **params) + as(current_user) do + assert_equal(Array(results).map(&:id), subject.search(**params).ids) + end end context "#search method" do @@ -14,6 +16,14 @@ class SearchableTest < ActiveSupport::TestCase @p3 = create(:post, source: "c3", score: 3, is_deleted: false) end + context "for a nonexistent attribute" do + should "raise an error" do + assert_raises(ArgumentError) do + Post.search_attribute(:answer, 42, User.anonymous) + end + end + end + context "for a numeric attribute" do should "support basic operators" do assert_search_equals(@p1, score_eq: 1) @@ -120,5 +130,76 @@ class SearchableTest < ActiveSupport::TestCase assert_search_equals(@wp, other_name_count: 2) end end + + context "for a belongs_to association" do + context "for a user association" do + should "work" do + assert_search_equals(@p1, uploader_id: @p1.uploader_id) + assert_search_equals(@p1, uploader_name: @p1.uploader.name) + + assert_search_equals(@p1, uploader: { id: @p1.uploader_id }) + assert_search_equals(@p1, uploader: { name: @p1.uploader.name }) + + assert_search_equals(@p1, uploader: { name: @p1.uploader.name }, uploader_id: @p1.uploader.id) + assert_search_equals([], uploader: { name: @p1.uploader.name }, uploader_id: @p2.uploader.id) + end + end + + context "for a post association" do + should "work" do + @p1.update!(parent: @p2) + + assert_search_equals(@p1, parent_id: @p2.id) + assert_search_equals(@p1, parent: { id: @p2.id }) + + assert_search_equals(@p1, parent_tags_match: "id:#{@p2.id}") + assert_search_equals([], parent_tags_match: "id:0") + + assert_search_equals(@p2, children_tags_match: "id:#{@p1.id}") + assert_search_equals([], children_tags_match: "id:0") + + assert_search_equals(@p1, has_parent: true) + assert_search_equals([@p3, @p2], has_parent: false) + end + end + + context "for a polymorphic association" do + subject { ModerationReport } + + should "work" do + as(create(:user)) do + @mr1 = create(:moderation_report, model: create(:comment)) + @mr2 = create(:moderation_report, model: create(:forum_post)) + @mr3 = create(:moderation_report, model: create(:dmail)) + end + + assert_search_equals(@mr1, model_type: "Comment") + assert_search_equals(@mr2, model_type: "ForumPost") + assert_search_equals(@mr3, model_type: "Dmail") + + assert_search_equals(@mr1, model_type: "Comment", model_id: @mr1.model.id) + assert_search_equals(@mr2, model_type: "ForumPost", model_id: @mr2.model.id) + assert_search_equals(@mr3, model_type: "Dmail", model_id: @mr3.model.id) + + assert_search_equals(@mr1, Comment: { body: @mr1.model.body }) + assert_search_equals(@mr2, ForumPost: { body: @mr2.model.body }) + + assert_search_equals([], Dmail: { body: @mr3.model.body }, current_user: User.anonymous) + assert_search_equals(@mr3, Dmail: { body: @mr3.model.body }, current_user: @mr3.model.owner) + end + end + end + + context "for a has_many association" do + should "work" do + as(@p1.uploader) { create(:comment, post: @p1) } + + assert_search_equals(@p1, has_comments: true) + assert_search_equals([@p3, @p2], has_comments: false) + + assert_search_equals(@p1, comments: { id: @p1.comments.first.id }) + assert_search_equals(@p1, has_comments: true, comments: { id: @p1.comments.first.id }) + end + end end end