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

@@ -112,11 +112,15 @@ class BulkUpdateRequestProcessor
           end
 
         when :rename
-          tag = Tag.find_by_name(args[0])
+          old_tag = Tag.find_by_name(args[0])
-          if tag.nil?
+          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

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 :antecedent_name, presence: true, tag_name: true, if: :antecedent_name_changed?
-  validates :consequent_name, presence: true
+  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

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}"

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)
+          create(:tag, name: "foo", post_count: 0)
-          @bulk_update_request = create(:bulk_update_request, script: "alias sfw -> rating:s")
+          @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

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

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

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)

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

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")
 

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