Fix #4965: Account for metatag prefixes when searching/linking

Drop the ability to write e.g. `create alias foo -> char:bar` in a BUR
to change the tag's type as a side effect. You can only use these
tag type prefixes in tag edits now.

This feature was only intended to be used in tag edits. The fact it
worked elsewhere was unintended behavior.

This feature was problematic because it relied on `Tag.find_or_create_by_name`
automagically changing the tag's category when the tag name contained a
tag category prefix, e.g. `char:hatsune_miku`. This meant that merely
looking up a tag could have the side effect of changing its category.
It was also bad because `find_or_create_by_name` had a hidden dependency
on the current user, which may not be set or available in all contexts.

app/models/artist.rb

@@ -215,7 +215,7 @@ class Artist < ApplicationRecord
 
         # potential race condition but unlikely
         unless TagImplication.where(:antecedent_name => name, :consequent_name => "banned_artist").exists?
-          Tag.find_or_create_by_name("artist:banned_artist") # ensure the banned_artist exists and is an artist tag.
+          Tag.find_or_create_by_name("banned_artist", category: "artist", current_user: banner)
           TagImplication.approve!(antecedent_name: name, consequent_name: "banned_artist", approver: banner)
         end
 

app/models/post.rb

@@ -590,7 +590,7 @@ class Post < ApplicationRecord
           self.source = value
 
         in category, name if category.in?(PostEdit::CATEGORIZATION_METATAGS)
-          Tag.find_or_create_by_name("#{category}:#{name}", creator: CurrentUser.user)
+          Tag.find_or_create_by_name(name, category: category, current_user: CurrentUser.user)
 
         else
           nil

app/models/tag.rb

@@ -193,39 +193,14 @@ class Tag < ApplicationRecord
         names.map {|x| find_or_create_by_name(x).name}
       end
 
-      def find_or_create_by_name(name, creator: CurrentUser.user)
-        name = normalize_name(name)
-        category = nil
+      def find_or_create_by_name(name, category: nil, current_user: nil)
+        tag = find_or_create_by(name: normalize_name(name))
 
-        if name =~ /\A(#{categories.regexp}):(.+)\Z/
-          category = $1
-          name = $2
+        if category.present? && current_user.present? && Pundit.policy!(current_user, tag).can_change_category?
+          tag.update!(category: categories.value_for(category))
         end
 
-        tag = find_by_name(name)
-
-        if tag
-          if category
-            category_id = categories.value_for(category)
-
-            # in case a category change hasn't propagated to this server yet,
-            # force an update the local cache. This may get overwritten in the
-            # next few lines if the category is changed.
-            tag.update_category_cache
-
-            if Pundit.policy!(creator, tag).can_change_category?
-              tag.update(category: category_id)
-            end
-          end
-
-          tag
-        else
-          Tag.new.tap do |t|
-            t.name = name
-            t.category = categories.value_for(category)
-            t.save
-          end
-        end
+        tag
       end
     end
   end
@@ -440,7 +415,7 @@ class Tag < ApplicationRecord
   concerning :CosplayTagMethods do
     def create_character_tag_for_cosplay_tag
       character_name = name.delete_suffix("_(cosplay)")
-      Tag.find_or_create_by_name("char:#{character_name}")
+      Tag.find_or_create_by_name(character_name, category: "character", current_user: CurrentUser.user)
     end
 
     def is_cosplay_tag?

test/unit/artist_test.rb

@@ -98,6 +98,10 @@ class ArtistTest < ActiveSupport::TestCase
         assert_equal("aaa banned_artist", @post.reload.tag_string)
       end
 
+      should "create the banned_artist tag if it doesn't already exist" do
+        assert_equal(true, Tag.exists?(name: "banned_artist", category: Tag.categories.artist))
+      end
+
       should "set the approver of the banned_artist implication" do
         ta = TagImplication.where(:antecedent_name => "aaa", :consequent_name => "banned_artist").first
         assert_equal(@admin.id, ta.approver.id)

test/unit/bulk_update_request_test.rb

@@ -165,6 +165,13 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
           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 contains a tag type prefix" do
+          @bur = build(:bulk_update_request, script: "alias blah -> char:bar")
+
+          assert_equal(false, @bur.valid?)
+          assert_equal(["Can't create alias blah -> char:bar ('char:bar' cannot begin with 'char:')"], @bur.errors.full_messages)
+        end
+
         should "be case-insensitive" do
           @bur = create_bur!("CREATE ALIAS AAA -> BBB", @admin)
 

test/unit/tag_test.rb

@@ -93,7 +93,7 @@ class TagTest < ActiveSupport::TestCase
       tag = FactoryBot.create(:tag)
       assert_difference("Tag.count", 0) do
         assert_equal(Tag.categories.general, tag.category)
-        Tag.find_or_create_by_name("artist:#{tag.name}")
+        Tag.find_or_create_by_name(tag.name, category: "artist", current_user: @builder)
         tag.reload
         assert_equal(Tag.categories.artist, tag.category)
       end
@@ -101,14 +101,14 @@ class TagTest < ActiveSupport::TestCase
 
     should "not change category when the tag is too large to be changed by a builder" do
       tag = FactoryBot.create(:tag, post_count: 1001)
-      Tag.find_or_create_by_name("artist:#{tag.name}", creator: @builder)
+      Tag.find_or_create_by_name(tag.name, category: "artist", current_user: @builder)
 
       assert_equal(0, tag.reload.category)
     end
 
     should "not change category when the tag is too large to be changed by a member" do
       tag = FactoryBot.create(:tag, post_count: 51)
-      Tag.find_or_create_by_name("artist:#{tag.name}", creator: FactoryBot.create(:member_user))
+      Tag.find_or_create_by_name(tag.name, category: "artist", current_user: create(:member_user))
 
       assert_equal(0, tag.reload.category)
     end
@@ -118,7 +118,7 @@ class TagTest < ActiveSupport::TestCase
       assert_equal(1, post.tag_count_general)
       assert_equal(0, post.tag_count_character)
 
-      tag = Tag.find_or_create_by_name("char:test")
+      tag = Tag.find_or_create_by_name("test", category: "char", current_user: @builder)
       post.reload
       assert_equal(0, post.tag_count_general)
       assert_equal(1, post.tag_count_character)
@@ -134,7 +134,7 @@ class TagTest < ActiveSupport::TestCase
 
     should "be created with the type when one doesn't exist" do
       assert_difference("Tag.count", 1) do
-        tag = Tag.find_or_create_by_name("artist:hoge")
+        tag = Tag.find_or_create_by_name("hoge", category: "artist", current_user: @builder)
         assert_equal("hoge", tag.name)
         assert_equal(Tag.categories.artist, tag.category)
       end