From 0be039577699ba4772d6d34d6b7aa8fe48c7289c Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 1 Dec 2020 17:28:42 -0600 Subject: [PATCH] BURs: fix validation of multi-step BURs. Bug: When validating a BUR, we didn't properly simulate running each line of the BUR in order, which could cause validation to incorrectly fail in multi-line BURs where some lines depended on previous lines. This bug meant you couldn't reverse an alias in a single BUR. The old alias wasn't removed before validating the new alias, so the BUR would fail with an alias conflict. This bug also meant that BURs containing duplicate aliases or redundant implications weren't caught. The fix is for BUR validation to actually simulate creating and removing aliases in sequential order, just as they would be when the BUR is approved. This is done by running the BUR in a transaction, then rolling back the transaction at the end. This is hacky but it works. --- app/logical/bulk_update_request_processor.rb | 116 ++++++++++--------- test/unit/bulk_update_request_test.rb | 96 ++++++--------- 2 files changed, 100 insertions(+), 112 deletions(-) diff --git a/app/logical/bulk_update_request_processor.rb b/app/logical/bulk_update_request_processor.rb index 0b6d96b18..836ceb43f 100644 --- a/app/logical/bulk_update_request_processor.rb +++ b/app/logical/bulk_update_request_processor.rb @@ -39,62 +39,70 @@ class BulkUpdateRequestProcessor end def validate_script - commands.each do |command, *args| - case command - when :create_alias - tag_alias = TagAlias.new(creator: User.system, antecedent_name: args[0], consequent_name: args[1]) - if tag_alias.invalid? - errors[:base] << "Can't create alias #{tag_alias.antecedent_name} -> #{tag_alias.consequent_name} (#{tag_alias.errors.full_messages.join("; ")})" + BulkUpdateRequest.transaction(requires_new: true) do + commands.each do |command, *args| + case command + when :create_alias + tag_alias = TagAlias.create(creator: User.system, antecedent_name: args[0], consequent_name: args[1]) + if tag_alias.invalid? + errors[:base] << "Can't create alias #{tag_alias.antecedent_name} -> #{tag_alias.consequent_name} (#{tag_alias.errors.full_messages.join("; ")})" + end + + when :create_implication + tag_implication = TagImplication.create(creator: User.system, antecedent_name: args[0], consequent_name: args[1], status: "active") + if tag_implication.invalid? + errors[:base] << "Can't create implication #{tag_implication.antecedent_name} -> #{tag_implication.consequent_name} (#{tag_implication.errors.full_messages.join("; ")})" + end + + if !tag_implication.antecedent_tag.empty? && tag_implication.antecedent_wiki.blank? + errors[:base] << "'#{tag_implication.antecedent_tag.name}' must have a wiki page" + end + + if !tag_implication.consequent_tag.empty? && tag_implication.consequent_wiki.blank? + errors[:base] << "'#{tag_implication.consequent_tag.name}' must have a wiki page" + end + + when :remove_alias + tag_alias = TagAlias.active.find_by(antecedent_name: args[0], consequent_name: args[1]) + if tag_alias.nil? + errors[:base] << "Can't remove alias #{args[0]} -> #{args[1]} (alias doesn't exist)" + else + tag_alias.update(status: "deleted") + end + + when :remove_implication + tag_implication = TagImplication.active.find_by(antecedent_name: args[0], consequent_name: args[1]) + if tag_implication.nil? + errors[:base] << "Can't remove implication #{args[0]} -> #{args[1]} (implication doesn't exist)" + else + tag_implication.update(status: "deleted") + end + + when :change_category + tag = Tag.find_by_name(args[0]) + if tag.nil? + errors[:base] << "Can't change category #{args[0]} -> #{args[1]} (the '#{args[0]}' tag doesn't exist)" + end + + when :rename + tag = Tag.find_by_name(args[0]) + if tag.nil? + errors[:base] << "Can't rename #{args[0]} -> #{args[1]} (the '#{args[0]}' tag doesn't exist)" + end + + when :mass_update + # okay + + when :invalid_line + errors[:base] << "Invalid line: #{args[0]}" + + else + # should never happen + raise Error, "Unknown command: #{command}" end - - when :create_implication - tag_implication = TagImplication.new(creator: User.system, antecedent_name: args[0], consequent_name: args[1], status: "active") - if tag_implication.invalid? - errors[:base] << "Can't create implication #{tag_implication.antecedent_name} -> #{tag_implication.consequent_name} (#{tag_implication.errors.full_messages.join("; ")})" - end - - if !tag_implication.antecedent_tag.empty? && tag_implication.antecedent_wiki.blank? - errors[:base] << "'#{tag_implication.antecedent_tag.name}' must have a wiki page" - end - - if !tag_implication.consequent_tag.empty? && tag_implication.consequent_wiki.blank? - errors[:base] << "'#{tag_implication.consequent_tag.name}' must have a wiki page" - end - - when :remove_alias - tag_alias = TagAlias.active.find_by(antecedent_name: args[0], consequent_name: args[1]) - if tag_alias.nil? - errors[:base] << "Can't remove alias #{args[0]} -> #{args[1]} (alias doesn't exist)" - end - - when :remove_implication - tag_implication = TagImplication.active.find_by(antecedent_name: args[0], consequent_name: args[1]) - if tag_implication.nil? - errors[:base] << "Can't remove implication #{args[0]} -> #{args[1]} (implication doesn't exist)" - end - - when :change_category - tag = Tag.find_by_name(args[0]) - if tag.nil? - errors[:base] << "Can't change category #{args[0]} -> #{args[1]} (the '#{args[0]}' tag doesn't exist)" - end - - when :rename - tag = Tag.find_by_name(args[0]) - if tag.nil? - errors[:base] << "Can't rename #{args[0]} -> #{args[1]} (the '#{args[0]}' tag doesn't exist)" - end - - when :mass_update - # okay - - when :invalid_line - errors[:base] << "Invalid line: #{args[0]}" - - else - # should never happen - raise Error, "Unknown command: #{command}" end + + raise ActiveRecord::Rollback end end diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index 03f5deefc..01c4a1502 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/test/unit/bulk_update_request_test.rb @@ -1,6 +1,12 @@ require 'test_helper' class BulkUpdateRequestTest < ActiveSupport::TestCase + def create_bur!(script, approver) + bur = create(:bulk_update_request, script: script) + perform_enqueued_jobs { bur.approve!(approver) } + bur + end + context "a bulk update request" do setup do @admin = FactoryBot.create(:admin_user) @@ -22,8 +28,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase context "the category command" do should "change the tag's category" do @tag = create(:tag, name: "hello") - @bur = create(:bulk_update_request, script: "category hello -> artist") - @bur.approve!(@admin) + @bur = create_bur!("category hello -> artist", @admin) assert_equal(true, @bur.valid?) assert_equal(true, @tag.reload.artist?) @@ -41,10 +46,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase setup do @wiki = create(:wiki_page, title: "foo") @artist = create(:artist, name: "foo") - @bur = create(:bulk_update_request, script: "create alias foo -> bar") - - @bur.approve!(@admin) - perform_enqueued_jobs + @bur = create_bur!("create alias foo -> bar", @admin) end should "create an alias" do @@ -59,12 +61,8 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase end should "move any active aliases from the old tag to the new tag" do - @bur1 = create(:bulk_update_request, script: "alias aaa -> bbb") - @bur2 = create(:bulk_update_request, script: "alias bbb -> ccc") - - @bur1.approve!(@admin) - @bur2.approve!(@admin) - perform_enqueued_jobs + @bur1 = create_bur!("alias aaa -> bbb", @admin) + @bur2 = create_bur!("alias bbb -> ccc", @admin) assert_equal(false, TagAlias.where(antecedent_name: "aaa", consequent_name: "bbb", status: "active").exists?) assert_equal(true, TagAlias.where(antecedent_name: "bbb", consequent_name: "ccc", status: "active").exists?) @@ -72,19 +70,13 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase end should "move any active implications from the old tag to the new tag" do - @bur1 = create(:bulk_update_request, script: "imply aaa -> bbb") - @bur2 = create(:bulk_update_request, script: "alias bbb -> ccc") - - @bur1.approve!(@admin) - @bur2.approve!(@admin) - perform_enqueued_jobs + @bur1 = create_bur!("imply aaa -> bbb", @admin) + @bur2 = create_bur!("alias bbb -> ccc", @admin) assert_equal(false, TagImplication.where(antecedent_name: "aaa", consequent_name: "bbb", status: "active").exists?) assert_equal(true, TagImplication.where(antecedent_name: "aaa", consequent_name: "ccc", status: "active").exists?) - @bur3 = create(:bulk_update_request, script: "alias aaa -> ddd") - @bur3.approve!(@admin) - perform_enqueued_jobs + @bur3 = create_bur!("alias aaa -> ddd", @admin) assert_equal(false, TagImplication.where(antecedent_name: "aaa", consequent_name: "ccc", status: "active").exists?) assert_equal(true, TagImplication.where(antecedent_name: "ddd", consequent_name: "ccc", status: "active").exists?) @@ -99,9 +91,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase end should "be case-insensitive" do - @bur = create(:bulk_update_request, script: "CREATE ALIAS AAA -> BBB") - @bur.approve!(@admin) - perform_enqueued_jobs + @bur = create_bur!("CREATE ALIAS AAA -> BBB", @admin) @alias = TagAlias.find_by(antecedent_name: "aaa", consequent_name: "bbb") assert_equal(true, @alias.present?) @@ -111,9 +101,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase context "the create implication command" do should "create an implication" do - @bur = create(:bulk_update_request, script: "create implication foo -> bar") - @bur.approve!(@admin) - perform_enqueued_jobs + @bur = create_bur!("create implication foo -> bar", @admin) @implication = TagImplication.find_by(antecedent_name: "foo", consequent_name: "bar") assert_equal(true, @implication.present?) @@ -137,7 +125,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase assert_equal(["Can't create implication a -> b (Implication already exists)"], @bur.errors.full_messages) end - should_eventually "fail for an implication that is redundant with another implication in the same BUR" do + should "fail for an implication that is redundant with another implication in the same BUR" do create(:tag_implication, antecedent_name: "b", consequent_name: "c") @bur = build(:bulk_update_request, script: "imply a -> b\nimply a -> c") @@ -149,8 +137,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase context "the remove alias command" do should "remove an alias" do create(:tag_alias, antecedent_name: "foo", consequent_name: "bar") - @bur = create(:bulk_update_request, script: "remove alias foo -> bar") - @bur.approve!(@admin) + @bur = create_bur!("remove alias foo -> bar", @admin) @alias = TagAlias.find_by(antecedent_name: "foo", consequent_name: "bar") assert_equal(true, @alias.present?) @@ -158,7 +145,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase end should "fail if the alias isn't active" do - create(:tag_alias, antecedent_name: "foo", consequent_name: "bar", status: "pending") + create(:tag_alias, antecedent_name: "foo", consequent_name: "bar", status: "deleted") @bur = build(:bulk_update_request, script: "remove alias foo -> bar") assert_equal(false, @bur.valid?) @@ -176,8 +163,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase context "the remove implication command" do should "remove an implication" do create(:tag_implication, antecedent_name: "foo", consequent_name: "bar", status: "active") - @bur = create(:bulk_update_request, script: "remove implication foo -> bar") - @bur.approve!(@admin) + @bur = create_bur!("remove implication foo -> bar", @admin) @implication = TagImplication.find_by(antecedent_name: "foo", consequent_name: "bar") assert_equal(true, @implication.present?) @@ -185,7 +171,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase end should "fail if the implication isn't active" do - create(:tag_implication, antecedent_name: "foo", consequent_name: "bar", status: "pending") + create(:tag_implication, antecedent_name: "foo", consequent_name: "bar", status: "deleted") @bur = build(:bulk_update_request, script: "remove implication foo -> bar") assert_equal(false, @bur.valid?) @@ -203,9 +189,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase context "the mass update command" do setup do @post = create(:post, tag_string: "foo") - @bur = create(:bulk_update_request, script: "mass update foo -> bar") - @bur.approve!(@admin) - perform_enqueued_jobs + @bur = create_bur!("mass update foo -> bar", @admin) end should "update the tags" do @@ -214,10 +198,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase should "be case-sensitive" do @post = create(:post, source: "imageboard") - @bur = create(:bulk_update_request, script: "mass update source:imageboard -> source:Imageboard") - - @bur.approve!(@admin) - perform_enqueued_jobs + @bur = create_bur!("mass update source:imageboard -> source:Imageboard", @admin) assert_equal("Imageboard", @post.reload.source) end @@ -228,9 +209,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase @artist = create(:artist, name: "foo") @wiki = create(:wiki_page, title: "foo", body: "[[foo]]") @post = create(:post, tag_string: "foo blah") - @bur = create(:bulk_update_request, script: "rename foo -> bar") - @bur.approve!(@admin) - perform_enqueued_jobs + @bur = create_bur!("rename foo -> bar", @admin) end should "rename the tags" do @@ -257,9 +236,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase @wiki = create(:wiki_page, title: "toosaka_rin_(cosplay)") @ta = create(:tag_alias, antecedent_name: "toosaka_rin", consequent_name: "tohsaka_rin") - @bur = create(:bulk_update_request, script: "rename toosaka_rin -> tohsaka_rin") - @bur.approve!(@admin) - perform_enqueued_jobs + create_bur!("rename toosaka_rin -> tohsaka_rin", @admin) assert_equal("cosplay tohsaka_rin tohsaka_rin_(cosplay)", @post.reload.tag_string) assert_equal("tohsaka_rin_(cosplay)", @wiki.reload.title) @@ -269,17 +246,25 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase context "that contains a mass update followed by an alias" do should "make the alias take effect after the mass update" do - @bur = create(:bulk_update_request, script: "mass update maid_dress -> maid dress\nalias maid_dress -> maid") @p1 = create(:post, tag_string: "maid_dress") @p2 = create(:post, tag_string: "maid") - @bur.approve!(@admin) - perform_enqueued_jobs + create_bur!("mass update maid_dress -> maid dress\nalias maid_dress -> maid", @admin) assert_equal("dress maid", @p1.reload.tag_string) assert_equal("maid", @p2.reload.tag_string) end end + + context "that reverses an alias by removing and recreating it" do + should "not fail with an alias conflict" do + @ta = create(:tag_alias, antecedent_name: "rabbit", consequent_name: "bunny") + create_bur!("unalias rabbit -> bunny\nalias bunny -> rabbit", @admin) + + assert_equal("deleted", @ta.reload.status) + assert_equal("active", TagAlias.find_by(antecedent_name: "bunny", consequent_name: "rabbit").status) + end + end end context "when validating a script" do @@ -361,11 +346,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase mass update aaa -> bbb ' - @bur = create(:bulk_update_request, script: @script, user: @admin) - @bur.approve!(@admin) - - assert_enqueued_jobs(3) - perform_enqueued_jobs + @bur = create_bur!(@script, @admin) @ta = TagAlias.where(:antecedent_name => "foo", :consequent_name => "bar").first @ti = TagImplication.where(:antecedent_name => "bar", :consequent_name => "baz").first @@ -462,9 +443,8 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase context "when searching" do setup do - @bur1 = FactoryBot.create(:bulk_update_request, title: "foo", script: "create alias aaa -> bbb", user_id: @admin.id) - @bur2 = FactoryBot.create(:bulk_update_request, title: "bar", script: "create implication bbb -> ccc", user_id: @admin.id) - @bur1.approve!(@admin) + @bur1 = create(:bulk_update_request, title: "foo", script: "create alias aaa -> bbb", user: @admin, approver: @admin, status: "approved") + @bur2 = create(:bulk_update_request, title: "bar", script: "create implication bbb -> ccc", user: @admin) end should "work" do