jaculabilis/danbooru
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

BURs: fix validation of multi-step BURs.

Browse Source 
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.
This commit is contained in:
evazion
2020-12-01 17:28:42 -06:00
parent 293b2c0be8
commit 0be0395776
2 changed files with 100 additions and 112 deletions
Download Patch File Download Diff File Expand all files Collapse all files

116
app/logical/bulk_update_request_processor.rb
View File

@@ -39,62 +39,70 @@ class BulkUpdateRequestProcessor
end end
def validate_script def validate_script
commands.each do |command, *args| BulkUpdateRequest.transaction(requires_new: true) do
case command commands.each do |command, *args|
when :create_alias case command
tag_alias = TagAlias.new(creator: User.system, antecedent_name: args[0], consequent_name: args[1]) when :create_alias
if tag_alias.invalid? tag_alias = TagAlias.create(creator: User.system, antecedent_name: args[0], consequent_name: args[1])
errors[:base] << "Can't create alias #{tag_alias.antecedent_name} -> #{tag_alias.consequent_name} (#{tag_alias.errors.full_messages.join("; ")})" 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 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 end
raise ActiveRecord::Rollback
end end
end end

96
test/unit/bulk_update_request_test.rb
View File

@@ -1,6 +1,12 @@
require 'test_helper' require 'test_helper'
class BulkUpdateRequestTest < ActiveSupport::TestCase 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 context "a bulk update request" do
setup do setup do
@admin = FactoryBot.create(:admin_user) @admin = FactoryBot.create(:admin_user)
@@ -22,8 +28,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
context "the category command" do context "the category command" do
should "change the tag's category" do should "change the tag's category" do
@tag = create(:tag, name: "hello") @tag = create(:tag, name: "hello")
@bur = create(:bulk_update_request, script: "category hello -> artist") @bur = create_bur!("category hello -> artist", @admin)
@bur.approve!(@admin)
assert_equal(true, @bur.valid?) assert_equal(true, @bur.valid?)
assert_equal(true, @tag.reload.artist?) assert_equal(true, @tag.reload.artist?)
@@ -41,10 +46,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
setup do setup do
@wiki = create(:wiki_page, title: "foo") @wiki = create(:wiki_page, title: "foo")
@artist = create(:artist, name: "foo") @artist = create(:artist, name: "foo")
@bur = create(:bulk_update_request, script: "create alias foo -> bar") @bur = create_bur!("create alias foo -> bar", @admin)
@bur.approve!(@admin)
perform_enqueued_jobs
end end
should "create an alias" do should "create an alias" do
@@ -59,12 +61,8 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
end end
should "move any active aliases from the old tag to the new tag" do should "move any active aliases from the old tag to the new tag" do
@bur1 = create(:bulk_update_request, script: "alias aaa -> bbb") @bur1 = create_bur!("alias aaa -> bbb", @admin)
@bur2 = create(:bulk_update_request, script: "alias bbb -> ccc") @bur2 = create_bur!("alias bbb -> ccc", @admin)
@bur1.approve!(@admin)
@bur2.approve!(@admin)
perform_enqueued_jobs
assert_equal(false, TagAlias.where(antecedent_name: "aaa", consequent_name: "bbb", status: "active").exists?) 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?) assert_equal(true, TagAlias.where(antecedent_name: "bbb", consequent_name: "ccc", status: "active").exists?)
@@ -72,19 +70,13 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
end end
should "move any active implications from the old tag to the new tag" do should "move any active implications from the old tag to the new tag" do
@bur1 = create(:bulk_update_request, script: "imply aaa -> bbb") @bur1 = create_bur!("imply aaa -> bbb", @admin)
@bur2 = create(:bulk_update_request, script: "alias bbb -> ccc") @bur2 = create_bur!("alias bbb -> ccc", @admin)
@bur1.approve!(@admin)
@bur2.approve!(@admin)
perform_enqueued_jobs
assert_equal(false, TagImplication.where(antecedent_name: "aaa", consequent_name: "bbb", status: "active").exists?) 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?) assert_equal(true, TagImplication.where(antecedent_name: "aaa", consequent_name: "ccc", status: "active").exists?)
@bur3 = create(:bulk_update_request, script: "alias aaa -> ddd") @bur3 = create_bur!("alias aaa -> ddd", @admin)
@bur3.approve!(@admin)
perform_enqueued_jobs
assert_equal(false, TagImplication.where(antecedent_name: "aaa", consequent_name: "ccc", status: "active").exists?) 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?) assert_equal(true, TagImplication.where(antecedent_name: "ddd", consequent_name: "ccc", status: "active").exists?)
@@ -99,9 +91,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
end end
should "be case-insensitive" do should "be case-insensitive" do
@bur = create(:bulk_update_request, script: "CREATE ALIAS AAA -> BBB") @bur = create_bur!("CREATE ALIAS AAA -> BBB", @admin)
@bur.approve!(@admin)
perform_enqueued_jobs
@alias = TagAlias.find_by(antecedent_name: "aaa", consequent_name: "bbb") @alias = TagAlias.find_by(antecedent_name: "aaa", consequent_name: "bbb")
assert_equal(true, @alias.present?) assert_equal(true, @alias.present?)
@@ -111,9 +101,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
context "the create implication command" do context "the create implication command" do
should "create an implication" do should "create an implication" do
@bur = create(:bulk_update_request, script: "create implication foo -> bar") @bur = create_bur!("create implication foo -> bar", @admin)
@bur.approve!(@admin)
perform_enqueued_jobs
@implication = TagImplication.find_by(antecedent_name: "foo", consequent_name: "bar") @implication = TagImplication.find_by(antecedent_name: "foo", consequent_name: "bar")
assert_equal(true, @implication.present?) 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) assert_equal(["Can't create implication a -> b (Implication already exists)"], @bur.errors.full_messages)
end 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") create(:tag_implication, antecedent_name: "b", consequent_name: "c")
@bur = build(:bulk_update_request, script: "imply a -> b\nimply a -> 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 context "the remove alias command" do
should "remove an alias" do should "remove an alias" do
create(:tag_alias, antecedent_name: "foo", consequent_name: "bar") create(:tag_alias, antecedent_name: "foo", consequent_name: "bar")
@bur = create(:bulk_update_request, script: "remove alias foo -> bar") @bur = create_bur!("remove alias foo -> bar", @admin)
@bur.approve!(@admin)
@alias = TagAlias.find_by(antecedent_name: "foo", consequent_name: "bar") @alias = TagAlias.find_by(antecedent_name: "foo", consequent_name: "bar")
assert_equal(true, @alias.present?) assert_equal(true, @alias.present?)
@@ -158,7 +145,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
end end
should "fail if the alias isn't active" do 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") @bur = build(:bulk_update_request, script: "remove alias foo -> bar")
assert_equal(false, @bur.valid?) assert_equal(false, @bur.valid?)
@@ -176,8 +163,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
context "the remove implication command" do context "the remove implication command" do
should "remove an implication" do should "remove an implication" do
create(:tag_implication, antecedent_name: "foo", consequent_name: "bar", status: "active") create(:tag_implication, antecedent_name: "foo", consequent_name: "bar", status: "active")
@bur = create(:bulk_update_request, script: "remove implication foo -> bar") @bur = create_bur!("remove implication foo -> bar", @admin)
@bur.approve!(@admin)
@implication = TagImplication.find_by(antecedent_name: "foo", consequent_name: "bar") @implication = TagImplication.find_by(antecedent_name: "foo", consequent_name: "bar")
assert_equal(true, @implication.present?) assert_equal(true, @implication.present?)
@@ -185,7 +171,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
end end
should "fail if the implication isn't active" do 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") @bur = build(:bulk_update_request, script: "remove implication foo -> bar")
assert_equal(false, @bur.valid?) assert_equal(false, @bur.valid?)
@@ -203,9 +189,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
context "the mass update command" do context "the mass update command" do
setup do setup do
@post = create(:post, tag_string: "foo") @post = create(:post, tag_string: "foo")
@bur = create(:bulk_update_request, script: "mass update foo -> bar") @bur = create_bur!("mass update foo -> bar", @admin)
@bur.approve!(@admin)
perform_enqueued_jobs
end end
should "update the tags" do should "update the tags" do
@@ -214,10 +198,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
should "be case-sensitive" do should "be case-sensitive" do
@post = create(:post, source: "imageboard") @post = create(:post, source: "imageboard")
@bur = create(:bulk_update_request, script: "mass update source:imageboard -> source:Imageboard") @bur = create_bur!("mass update source:imageboard -> source:Imageboard", @admin)
@bur.approve!(@admin)
perform_enqueued_jobs
assert_equal("Imageboard", @post.reload.source) assert_equal("Imageboard", @post.reload.source)
end end
@@ -228,9 +209,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
@artist = create(:artist, name: "foo") @artist = create(:artist, name: "foo")
@wiki = create(:wiki_page, title: "foo", body: "[[foo]]") @wiki = create(:wiki_page, title: "foo", body: "[[foo]]")
@post = create(:post, tag_string: "foo blah") @post = create(:post, tag_string: "foo blah")
@bur = create(:bulk_update_request, script: "rename foo -> bar") @bur = create_bur!("rename foo -> bar", @admin)
@bur.approve!(@admin)
perform_enqueued_jobs
end end
should "rename the tags" do should "rename the tags" do
@@ -257,9 +236,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
@wiki = create(:wiki_page, title: "toosaka_rin_(cosplay)") @wiki = create(:wiki_page, title: "toosaka_rin_(cosplay)")
@ta = create(:tag_alias, antecedent_name: "toosaka_rin", consequent_name: "tohsaka_rin") @ta = create(:tag_alias, antecedent_name: "toosaka_rin", consequent_name: "tohsaka_rin")
@bur = create(:bulk_update_request, script: "rename toosaka_rin -> tohsaka_rin") create_bur!("rename toosaka_rin -> tohsaka_rin", @admin)
@bur.approve!(@admin)
perform_enqueued_jobs
assert_equal("cosplay tohsaka_rin tohsaka_rin_(cosplay)", @post.reload.tag_string) assert_equal("cosplay tohsaka_rin tohsaka_rin_(cosplay)", @post.reload.tag_string)
assert_equal("tohsaka_rin_(cosplay)", @wiki.reload.title) 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 context "that contains a mass update followed by an alias" do
should "make the alias take effect after the mass update" 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") @p1 = create(:post, tag_string: "maid_dress")
@p2 = create(:post, tag_string: "maid") @p2 = create(:post, tag_string: "maid")
@bur.approve!(@admin) create_bur!("mass update maid_dress -> maid dress\nalias maid_dress -> maid", @admin)
perform_enqueued_jobs
assert_equal("dress maid", @p1.reload.tag_string) assert_equal("dress maid", @p1.reload.tag_string)
assert_equal("maid", @p2.reload.tag_string) assert_equal("maid", @p2.reload.tag_string)
end end
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 end
context "when validating a script" do context "when validating a script" do
@@ -361,11 +346,7 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
mass update aaa -> bbb mass update aaa -> bbb
' '
@bur = create(:bulk_update_request, script: @script, user: @admin) @bur = create_bur!(@script, @admin)
@bur.approve!(@admin)
assert_enqueued_jobs(3)
perform_enqueued_jobs
@ta = TagAlias.where(:antecedent_name => "foo", :consequent_name => "bar").first @ta = TagAlias.where(:antecedent_name => "foo", :consequent_name => "bar").first
@ti = TagImplication.where(:antecedent_name => "bar", :consequent_name => "baz").first @ti = TagImplication.where(:antecedent_name => "bar", :consequent_name => "baz").first
@@ -462,9 +443,8 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
context "when searching" do context "when searching" do
setup do setup do
@bur1 = FactoryBot.create(:bulk_update_request, title: "foo", script: "create alias aaa -> bbb", user_id: @admin.id) @bur1 = create(:bulk_update_request, title: "foo", script: "create alias aaa -> bbb", user: @admin, approver: @admin, status: "approved")
@bur2 = FactoryBot.create(:bulk_update_request, title: "bar", script: "create implication bbb -> ccc", user_id: @admin.id) @bur2 = create(:bulk_update_request, title: "bar", script: "create implication bbb -> ccc", user: @admin)
@bur1.approve!(@admin)
end end
should "work" do should "work" do