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

Fix #4483: Wrong order for BUR caused 12k mistags.

Browse Source 
Bug: if a BUR contained a mass update followed by an alias, then the
alias would become active before the mass update, which could cause
the mass update to return incorrect results if both the alias and mass
update touched the same tags.

This happened because all aliases and implications in the BUR were set
to a queued state before the mass update was processed, but putting an
alias in the queued state effectively made it active.

The fix is to remove the queued state. This was only used anyway as a
debugging tool anyway to monitor the state of BURs as they were being
processed.
This commit is contained in:
evazion
2020-11-12 15:59:01 -06:00
parent cc64f8b7ee
commit 9a287cd71f
11 changed files with 56 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
app/jobs/process_tag_alias_job.rb
View File

@@ -1,7 +1,7 @@
class ProcessTagAliasJob < ApplicationJob
queue_as :bulk_update
def perform(tag_alias)
tag_alias.process!
def perform(tag_alias, approver)
tag_alias.process!(approver)
end
end

4
app/jobs/process_tag_implication_job.rb
View File

@@ -1,7 +1,7 @@
class ProcessTagImplicationJob < ApplicationJob
queue_as :bulk_update
def perform(tag_implication)
tag_implication.process!
def perform(tag_implication, approver)
tag_implication.process!(approver)
end
end

4
app/logical/bulk_update_request_processor.rb
View File

@@ -98,11 +98,11 @@ class BulkUpdateRequestProcessor
case command
when :create_alias
tag_alias = TagAlias.create!(creator: approver, forum_topic_id: forum_topic_id, status: "pending", antecedent_name: args[0], consequent_name: args[1], skip_secondary_validations: skip_secondary_validations)
tag_alias.approve!(approver: approver)
tag_alias.approve!(approver)
when :create_implication
tag_implication = TagImplication.create!(creator: approver, forum_topic_id: forum_topic_id, status: "pending", antecedent_name: args[0], consequent_name: args[1], skip_secondary_validations: skip_secondary_validations)
tag_implication.approve!(approver: approver)
tag_implication.approve!(approver)
when :remove_alias
tag_alias = TagAlias.active.find_by!(antecedent_name: args[0], consequent_name: args[1])

2
app/models/artist.rb
View File

@@ -193,7 +193,7 @@ class Artist < ApplicationRecord
# potential race condition but unlikely
unless TagImplication.where(:antecedent_name => name, :consequent_name => "banned_artist").exists?
tag_implication = TagImplication.create!(antecedent_name: name, consequent_name: "banned_artist", skip_secondary_validations: true, creator: banner)
tag_implication.approve!(approver: banner)
tag_implication.approve!(banner)
end
update!(is_banned: true)

15
app/models/tag_alias.rb
View File

@@ -3,15 +3,10 @@ class TagAlias < TagRelationship
validates_uniqueness_of :antecedent_name, scope: :status, conditions: -> { active }
validate :absence_of_transitive_relation
module ApprovalMethods
def approve!(approver: CurrentUser.user)
update(approver: approver, status: "queued")
ProcessTagAliasJob.perform_later(self)
end
def approve!(approver)
ProcessTagAliasJob.perform_later(self, approver)
end
include ApprovalMethods
def self.to_aliased(names)
names = Array(names).map(&:to_s)
return [] if names.empty?
@@ -19,10 +14,10 @@ class TagAlias < TagRelationship
names.map { |name| aliases[name] || name }
end
def process!(user: User.system)
update!(status: "processing")
def process!(approver)
update!(approver: approver, status: "processing")
move_aliases_and_implications
TagMover.new(antecedent_name, consequent_name, user: user).move!
TagMover.new(antecedent_name, consequent_name, user: User.system).move!
update!(status: "active")
rescue Exception => e
update!(status: "error: #{e}")

9
app/models/tag_implication.rb
View File

@@ -120,13 +120,13 @@ class TagImplication < TagRelationship
end
module ApprovalMethods
def process!
def process!(approver)
unless valid?
raise errors.full_messages.join("; ")
end
CurrentUser.scoped(User.system) do
update(status: "processing")
update(approver: approver, status: "processing")
update_posts
update(status: "active")
end
@@ -135,9 +135,8 @@ class TagImplication < TagRelationship
DanbooruLogger.log(e, tag_implication_id: id, antecedent_name: antecedent_name, consequent_name: consequent_name)
end
def approve!(approver: CurrentUser.user)
update(approver: approver, status: "queued")
ProcessTagImplicationJob.perform_later(self)
def approve!(approver)
ProcessTagImplicationJob.perform_later(self, approver)
end
def create_mod_action

10
app/models/tag_relationship.rb
View File

@@ -16,7 +16,7 @@ class TagRelationship < ApplicationRecord
belongs_to :consequent_wiki, class_name: "WikiPage", foreign_key: "consequent_name", primary_key: "title", optional: true
scope :active, -> {approved}
scope :approved, -> {where(status: %w[active processing queued])}
scope :approved, -> {where(status: %w[active processing])}
scope :deleted, -> {where(status: "deleted")}
scope :expired, -> {where("created_at < ?", EXPIRY.days.ago)}
scope :old, -> {where("created_at >= ? and created_at < ?", EXPIRY.days.ago, EXPIRY_WARNING.days.ago)}
@@ -24,7 +24,7 @@ class TagRelationship < ApplicationRecord
scope :retired, -> {where(status: "retired")}
before_validation :normalize_names
validates_format_of :status, :with => /\A(active|deleted|pending|processing|queued|retired|error: .*)\Z/
validates_format_of :status, :with => /\A(active|deleted|pending|processing|retired|error: .*)\Z/
validates_presence_of :antecedent_name, :consequent_name
validates :approver, presence: { message: "must exist" }, if: -> { approver_id.present? }
validates :forum_topic, presence: { message: "must exist" }, if: -> { forum_topic_id.present? }
@@ -36,7 +36,7 @@ class TagRelationship < ApplicationRecord
end
def is_approved?
status.in?(%w[active processing queued])
status.in?(%w[active processing])
end
def is_rejected?
@@ -76,7 +76,7 @@ class TagRelationship < ApplicationRecord
status = status.downcase
if status == "approved"
where(status: %w[active processing queued])
where(status: %w[active processing])
else
where(status: status)
end
@@ -89,7 +89,7 @@ class TagRelationship < ApplicationRecord
def pending_first
# unknown statuses return null and are sorted first
order(Arel.sql("array_position(array['queued', 'processing', 'pending', 'active', 'deleted', 'retired'], status::text) NULLS FIRST, id DESC"))
order(Arel.sql("array_position(array['processing', 'pending', 'active', 'deleted', 'retired'], status::text) NULLS FIRST, id DESC"))
end
def search(params)

2
app/views/tag_relationships/_search.html.erb
View File

@@ -9,7 +9,7 @@
<%= f.simple_fields_for :consequent_tag do |fa| %>
<%= fa.input :category, label: "To Category", collection: TagCategory.canonical_mapping.to_a, include_blank: true, selected: params.dig(:search, :consequent_tag, :category) %>
<% end %>
<%= f.input :status, label: "Status", collection: %w[Approved Active Pending Deleted Retired Processing Queued], include_blank: true, selected: params[:search][:status] %>
<%= f.input :status, label: "Status", collection: %w[Approved Active Pending Deleted Retired Processing], include_blank: true, selected: params[:search][:status] %>
<%= f.input :order, label: "Order", collection: [%w[Created created_at], %w[Updated updated_at], %w[Name name], %w[Tag\ count tag_count], %w[Status status]], include_blank: true, selected: params[:search][:order] %>
<%= f.submit "Search" %>
<% end %>

14
test/unit/bulk_update_request_test.rb
View File

@@ -226,6 +226,20 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase
end
end
end
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
assert_equal("dress maid", @p1.reload.tag_string)
assert_equal("maid", @p2.reload.tag_string)
end
end
end
context "when validating a script" do

33
test/unit/tag_alias_test.rb
View File

@@ -28,7 +28,6 @@ class TagAliasTest < ActiveSupport::TestCase
should allow_value('deleted').for(:status)
should allow_value('pending').for(:status)
should allow_value('processing').for(:status)
should allow_value('queued').for(:status)
should allow_value('error: derp').for(:status)
should_not allow_value('ACTIVE').for(:status)
@@ -93,7 +92,7 @@ class TagAliasTest < ActiveSupport::TestCase
@ss5 = create(:saved_search, query: "123 ...", user: CurrentUser.user)
@ta = create(:tag_alias, antecedent_name: "...", consequent_name: "bbb")
@ta.approve!(approver: @admin)
@ta.approve!(@admin)
perform_enqueued_jobs
assert_equal("123 bbb 456", @ss1.reload.query)
@@ -115,7 +114,7 @@ class TagAliasTest < ActiveSupport::TestCase
@u7 = create(:user, blacklisted_tags: "111 ...\r\n222 333\n")
@ta = create(:tag_alias, antecedent_name: "...", consequent_name: "aaa")
@ta.approve!(approver: @admin)
@ta.approve!(@admin)
perform_enqueued_jobs
assert_equal("111 aaa 222", @u1.reload.blacklisted_tags)
@@ -133,7 +132,7 @@ class TagAliasTest < ActiveSupport::TestCase
post2 = FactoryBot.create(:post, :tag_string => "ccc ddd")
ta = FactoryBot.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "ccc")
ta.approve!(approver: @admin)
ta.approve!(@admin)
perform_enqueued_jobs
assert_equal("bbb ccc", post1.reload.tag_string)
@@ -155,7 +154,7 @@ class TagAliasTest < ActiveSupport::TestCase
@wiki = create(:wiki_page, title: "aaa")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
@ta.approve!(approver: @admin)
@ta.approve!(@admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
@@ -167,7 +166,7 @@ class TagAliasTest < ActiveSupport::TestCase
@wiki2 = create(:wiki_page, title: "bbb", other_names: "111 333", body: "second")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
@ta.approve!(approver: @admin)
@ta.approve!(@admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
@@ -185,7 +184,7 @@ class TagAliasTest < ActiveSupport::TestCase
@wiki2 = create(:wiki_page, title: "bbb", other_names: "111 333", body: "second")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
@ta.approve!(approver: @admin)
@ta.approve!(@admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
@@ -202,7 +201,7 @@ class TagAliasTest < ActiveSupport::TestCase
@wiki = create(:wiki_page, body: "foo [[aaa]] bar")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb")
@ta.approve!(approver: @admin)
@ta.approve!(@admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
@@ -215,7 +214,7 @@ class TagAliasTest < ActiveSupport::TestCase
@artist = create(:artist, name: "aaa")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
@ta.approve!(approver: @admin)
@ta.approve!(@admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
@@ -228,7 +227,7 @@ class TagAliasTest < ActiveSupport::TestCase
@artist2 = create(:artist, name: "bbb", other_names: "111 333", url_string: "https://twitter.com/111\n-https://twitter.com/333\nhttps://twitter.com/444")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
@ta.approve!(approver: @admin)
@ta.approve!(@admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
@@ -248,7 +247,7 @@ class TagAliasTest < ActiveSupport::TestCase
@artist2 = create(:artist, name: "bbb", other_names: "111 333", url_string: "https://twitter.com/111\n-https://twitter.com/333\nhttps://twitter.com/444")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
@ta.approve!(approver: @admin)
@ta.approve!(@admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
@@ -270,8 +269,8 @@ class TagAliasTest < ActiveSupport::TestCase
ta2 = FactoryBot.create(:tag_alias, :antecedent_name => "bbb", :consequent_name => "ccc", :status => "pending")
# XXX this is broken, it depends on the order the jobs are executed in.
ta2.approve!(approver: @admin)
ta1.approve!(approver: @admin)
ta2.approve!(@admin)
ta1.approve!(@admin)
perform_enqueued_jobs
assert_equal("ccc", ta1.reload.consequent_name)
@@ -280,7 +279,7 @@ class TagAliasTest < ActiveSupport::TestCase
should "move existing implications" do
ti = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb")
ta = FactoryBot.create(:tag_alias, :antecedent_name => "bbb", :consequent_name => "ccc")
ta.approve!(approver: @admin)
ta.approve!(@admin)
perform_enqueued_jobs
assert_equal("ccc", ti.reload.consequent_name)
@@ -291,7 +290,7 @@ class TagAliasTest < ActiveSupport::TestCase
tag2 = create(:tag, name: "artist", category: 1)
ta = create(:tag_alias, antecedent_name: "general", consequent_name: "artist")
ta.approve!(approver: @admin)
ta.approve!(@admin)
perform_enqueued_jobs
assert_equal(1, tag1.reload.category)
@@ -303,7 +302,7 @@ class TagAliasTest < ActiveSupport::TestCase
tag2 = create(:tag, name: "general", category: 0)
ta = create(:tag_alias, antecedent_name: "artist", consequent_name: "general")
ta.approve!(approver: @admin)
ta.approve!(@admin)
perform_enqueued_jobs
assert_equal(1, tag1.reload.category)
@@ -315,7 +314,7 @@ class TagAliasTest < ActiveSupport::TestCase
tag2 = create(:tag, name: "copyright", category: 3)
ta = create(:tag_alias, antecedent_name: "character", consequent_name: "copyright")
ta.approve!(approver: @admin)
ta.approve!(@admin)
perform_enqueued_jobs
assert_equal(4, tag1.reload.category)

9
test/unit/tag_implication_test.rb
View File

@@ -3,8 +3,8 @@ require 'test_helper'
class TagImplicationTest < ActiveSupport::TestCase
context "A tag implication" do
setup do
user = FactoryBot.create(:admin_user)
CurrentUser.user = user
@admin = create(:admin_user)
CurrentUser.user = @admin
CurrentUser.ip_addr = "127.0.0.1"
end
@@ -24,7 +24,6 @@ class TagImplicationTest < ActiveSupport::TestCase
should allow_value('deleted').for(:status)
should allow_value('pending').for(:status)
should allow_value('processing').for(:status)
should allow_value('queued').for(:status)
should allow_value('error: derp').for(:status)
should_not allow_value('ACTIVE').for(:status)
@@ -135,8 +134,8 @@ class TagImplicationTest < ActiveSupport::TestCase
ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "xxx")
ti2 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "yyy")
ti1.approve!
ti2.approve!
ti1.approve!(@admin)
ti2.approve!(@admin)
perform_enqueued_jobs
assert_equal("aaa bbb ccc xxx yyy", p1.reload.tag_string)