From ccd0dde081f34a5375961d3dac3b7c1097a64c08 Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 30 Apr 2022 19:25:18 -0500 Subject: [PATCH] Fix #5013: BUR model doesn't validate tags. Don't allow users to request aliases, implications, or renames for invalid tag names. As a side effect, it's no longer possible to request shortcut aliases like `/hr -> hakurei_reimu` (slash abbreviations still exist, but they can't be overridden with aliases). Tests involving these types of aliases are removed. --- app/logical/bulk_update_request_processor.rb | 10 ++++-- app/models/tag_relationship.rb | 4 +-- config/locales/en.yml | 12 +++++++ .../bulk_update_requests_controller_test.rb | 6 ++-- test/functional/posts_controller_test.rb | 4 +-- test/unit/autocomplete_service_test.rb | 8 ----- test/unit/bulk_update_request_test.rb | 36 +++++++++++++++++++ test/unit/post_query_builder_test.rb | 5 ++- test/unit/post_test.rb | 7 ---- test/unit/tag_alias_test.rb | 5 ++- 10 files changed, 66 insertions(+), 31 deletions(-) diff --git a/app/logical/bulk_update_request_processor.rb b/app/logical/bulk_update_request_processor.rb index 165e4084e..9197ac8d2 100644 --- a/app/logical/bulk_update_request_processor.rb +++ b/app/logical/bulk_update_request_processor.rb @@ -112,11 +112,15 @@ class BulkUpdateRequestProcessor end when :rename - tag = Tag.find_by_name(args[0]) - if tag.nil? + old_tag = Tag.find_by_name(args[0]) + new_tag = Tag.find_by_name(args[1]) || Tag.new(name: args[1]) + + if old_tag.nil? errors.add(:base, "Can't rename #{args[0]} -> #{args[1]} (the '#{args[0]}' tag doesn't exist)") - elsif tag.post_count > MAXIMUM_RENAME_COUNT + elsif old_tag.post_count > MAXIMUM_RENAME_COUNT errors.add(:base, "Can't rename #{args[0]} -> #{args[1]} ('#{args[0]}' has more than #{MAXIMUM_RENAME_COUNT} posts, use an alias instead)") + elsif new_tag.invalid?(:name) + errors.add(:base, "Can't rename #{args[0]} -> #{args[1]} (#{new_tag.errors.full_messages.join("; ")})") end when :mass_update diff --git a/app/models/tag_relationship.rb b/app/models/tag_relationship.rb index c68c104fa..6c3f3b1d6 100644 --- a/app/models/tag_relationship.rb +++ b/app/models/tag_relationship.rb @@ -26,8 +26,8 @@ class TagRelationship < ApplicationRecord before_validation :normalize_names validates :status, inclusion: { in: STATUSES } - validates :antecedent_name, presence: true - validates :consequent_name, presence: true + validates :antecedent_name, presence: true, tag_name: true, if: :antecedent_name_changed? + validates :consequent_name, presence: true, tag_name: true, if: :consequent_name_changed? validates :approver, presence: { message: "must exist" }, if: -> { approver_id.present? } validates :forum_topic, presence: { message: "must exist" }, if: -> { forum_topic_id.present? } validate :antecedent_and_consequent_are_different diff --git a/config/locales/en.yml b/config/locales/en.yml index c39935903..eaa3ed634 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -80,8 +80,20 @@ en: messages: record_invalid: "%{errors}" models: + tag: + attributes: + name: + format: "%{message}" + tag_alias: + attributes: + antecedent_name: + format: "%{message}" + consequent_name: + format: "%{message}" tag_implication: attributes: antecedent_name: taken: "Implication already exists" format: "%{message}" + consequent_name: + format: "%{message}" diff --git a/test/functional/bulk_update_requests_controller_test.rb b/test/functional/bulk_update_requests_controller_test.rb index 9aa36af54..599707dc6 100644 --- a/test/functional/bulk_update_requests_controller_test.rb +++ b/test/functional/bulk_update_requests_controller_test.rb @@ -146,14 +146,14 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest context "for a builder" do should "fail when moving a non-artist tag" do - create(:tag, name: "sfw", post_count: 0) - @bulk_update_request = create(:bulk_update_request, script: "alias sfw -> rating:s") + create(:tag, name: "foo", post_count: 0) + @bulk_update_request = create(:bulk_update_request, script: "alias foo -> bar") post_auth approve_bulk_update_request_path(@bulk_update_request), @builder assert_response 403 assert_equal("pending", @bulk_update_request.reload.status) - assert_equal(false, TagAlias.exists?(antecedent_name: "sfw", consequent_name: "rating:s")) + assert_equal(false, TagAlias.exists?(antecedent_name: "foo", consequent_name: "bar")) end should "fail for a large artist move" do diff --git a/test/functional/posts_controller_test.rb b/test/functional/posts_controller_test.rb index c2d2444cc..b1e393409 100644 --- a/test/functional/posts_controller_test.rb +++ b/test/functional/posts_controller_test.rb @@ -148,11 +148,11 @@ class PostsControllerTest < ActionDispatch::IntegrationTest end should "render for an aliased tag" do - create(:tag_alias, antecedent_name: "/lav", consequent_name: "looking_at_viewer") + 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" } + 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 diff --git a/test/unit/autocomplete_service_test.rb b/test/unit/autocomplete_service_test.rb index 690a82c5a..d5aba0577 100644 --- a/test/unit/autocomplete_service_test.rb +++ b/test/unit/autocomplete_service_test.rb @@ -102,14 +102,6 @@ class AutocompleteServiceTest < ActiveSupport::TestCase assert_autocomplete_equals([], "/xxxxxxxxxx", :tag_query) assert_autocomplete_equals([], "/_", :tag_query) end - - should "list aliases before abbreviations" do - create(:tag, name: "hair_ribbon", post_count: 300_000) - create(:tag, name: "hakurei_reimu", post_count: 50_000) - create(:tag_alias, antecedent_name: "/hr", consequent_name: "hakurei_reimu") - - assert_autocomplete_equals(%w[hakurei_reimu hair_ribbon], "/hr", :tag_query) - end end should "autocomplete tags from wiki and artist other names" do diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index 094c2b230..c942c11fb 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/test/unit/bulk_update_request_test.rb @@ -151,6 +151,20 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase assert_equal(["Can't create alias aaa -> bbb (bbb is already aliased to ccc)"], @bur.errors.full_messages) end + should "fail if the antecedent name is invalid" do + @bur = build(:bulk_update_request, script: "create alias tag_ -> tag") + + assert_equal(false, @bur.valid?) + assert_equal(["Can't create alias tag_ -> tag ('tag_' cannot end with an underscore)"], @bur.errors.full_messages) + end + + should "fail if the consequent name is invalid" do + @bur = build(:bulk_update_request, script: "create alias tag -> tag_") + + assert_equal(false, @bur.valid?) + assert_equal(["Can't create alias tag -> tag_ ('tag_' cannot end with an underscore)"], @bur.errors.full_messages) + end + should "be case-insensitive" do @bur = create_bur!("CREATE ALIAS AAA -> BBB", @admin) @@ -221,6 +235,20 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase assert_equal(false, @bur.valid?) assert_equal(["Can't create implication white_shirt -> shirt ('white_shirt' must have at least 100 posts)"], @bur.errors.full_messages) end + + should "fail if the antecedent name is invalid" do + @bur = build(:bulk_update_request, script: "imply tag_ -> tag") + + assert_equal(false, @bur.valid?) + assert_equal(["Can't create implication tag_ -> tag ('tag_' cannot end with an underscore)"], @bur.errors.full_messages) + end + + should "fail if the consequent name is invalid" do + @bur = build(:bulk_update_request, script: "imply tag -> tag_") + + assert_equal(false, @bur.valid?) + assert_equal(["Can't create implication tag -> tag_ ('tag_' cannot end with an underscore)"], @bur.errors.full_messages) + end end context "the remove alias command" do @@ -382,6 +410,14 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase assert_equal(["Can't rename aaa -> bbb ('aaa' has more than 200 posts, use an alias instead)"], @bur.errors.full_messages) end + should "fail if the consequent name is invalid" do + create(:tag, name: "tag") + @bur = build(:bulk_update_request, script: "rename tag -> tag_") + + assert_equal(false, @bur.valid?) + assert_equal(["Can't rename tag -> tag_ ('tag_' cannot end with an underscore)"], @bur.errors.full_messages) + end + context "when moving an artist" do should "add the artist's old tag name to their other names" do assert_equal(["foo"], @artist.reload.other_names) diff --git a/test/unit/post_query_builder_test.rb b/test/unit/post_query_builder_test.rb index e83063bbc..e04d4eb51 100644 --- a/test/unit/post_query_builder_test.rb +++ b/test/unit/post_query_builder_test.rb @@ -1276,12 +1276,11 @@ class PostQueryBuilderTest < ActiveSupport::TestCase should "resolve abbreviations to the actual tag" do tag1 = create(:tag, name: "hair_ribbon", post_count: 300_000) tag2 = create(:tag, name: "hakurei_reimu", post_count: 50_000) - ta1 = create(:tag_alias, antecedent_name: "/hr", consequent_name: "hakurei_reimu") post1 = create(:post, tag_string: "hair_ribbon") post2 = create(:post, tag_string: "hakurei_reimu") - assert_tag_match([post2], "/hr") - assert_tag_match([post1], "-/hr") + assert_tag_match([post1], "/hr") + assert_tag_match([post2], "-/hr") end should "fail if the search exceeds the tag limit" do diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index 697febcf3..9a1203355 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -1005,13 +1005,6 @@ class PostTest < ActiveSupport::TestCase assert_equal("aaa ccc", @post.tag_string) end - should "resolve aliases" do - FactoryBot.create(:tag_alias, :antecedent_name => "/tr", :consequent_name => "translation_request") - @post.update(:tag_string => "aaa translation_request -/tr") - - assert_equal("aaa", @post.tag_string) - end - should "resolve aliases before removing negated tags" do create(:tag_alias, antecedent_name: "female_focus", consequent_name: "female") diff --git a/test/unit/tag_alias_test.rb b/test/unit/tag_alias_test.rb index 0d8836193..6c93d6523 100644 --- a/test/unit/tag_alias_test.rb +++ b/test/unit/tag_alias_test.rb @@ -83,11 +83,10 @@ class TagAliasTest < ActiveSupport::TestCase create(:tag, name: "hakurei_reimu", post_count: 50_000) create(:tag, name: "kirisama_marisa", post_count: 50_000) create(:tag, name: "kaname_madoka", post_count: 20_000) - create(:tag_alias, antecedent_name: "/hr", consequent_name: "hakurei_reimu") - assert_equal(["hakurei_reimu"], TagAlias.to_aliased(["/hr"])) + assert_equal(["hair_ribbon"], TagAlias.to_aliased(["/hr"])) assert_equal(["kirisama_marisa"], TagAlias.to_aliased(["/km"])) - assert_equal(["hakurei_reimu", "kirisama_marisa"], TagAlias.to_aliased(["/hr", "/km"])) + assert_equal(["hair_ribbon", "kirisama_marisa"], TagAlias.to_aliased(["/hr", "/km"])) end context "saved searches" do