From 9a287cd71f17db1066c8b6a927abb621b6a62d90 Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 12 Nov 2020 15:59:01 -0600 Subject: [PATCH] Fix #4483: Wrong order for BUR caused 12k mistags. 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. --- app/jobs/process_tag_alias_job.rb | 4 +-- app/jobs/process_tag_implication_job.rb | 4 +-- app/logical/bulk_update_request_processor.rb | 4 +-- app/models/artist.rb | 2 +- app/models/tag_alias.rb | 15 +++------ app/models/tag_implication.rb | 9 +++--- app/models/tag_relationship.rb | 10 +++--- app/views/tag_relationships/_search.html.erb | 2 +- test/unit/bulk_update_request_test.rb | 14 +++++++++ test/unit/tag_alias_test.rb | 33 ++++++++++---------- test/unit/tag_implication_test.rb | 9 +++--- 11 files changed, 56 insertions(+), 50 deletions(-) diff --git a/app/jobs/process_tag_alias_job.rb b/app/jobs/process_tag_alias_job.rb index 9098dd14e..120171928 100644 --- a/app/jobs/process_tag_alias_job.rb +++ b/app/jobs/process_tag_alias_job.rb @@ -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 diff --git a/app/jobs/process_tag_implication_job.rb b/app/jobs/process_tag_implication_job.rb index 41bff5c45..457f12386 100644 --- a/app/jobs/process_tag_implication_job.rb +++ b/app/jobs/process_tag_implication_job.rb @@ -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 diff --git a/app/logical/bulk_update_request_processor.rb b/app/logical/bulk_update_request_processor.rb index ecbd5997c..2d072640d 100644 --- a/app/logical/bulk_update_request_processor.rb +++ b/app/logical/bulk_update_request_processor.rb @@ -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]) diff --git a/app/models/artist.rb b/app/models/artist.rb index dccde5f68..dfee85f2d 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -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) diff --git a/app/models/tag_alias.rb b/app/models/tag_alias.rb index 40a0c7759..6e7303d5d 100644 --- a/app/models/tag_alias.rb +++ b/app/models/tag_alias.rb @@ -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}") diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index e4c0321b6..c91605004 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -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 diff --git a/app/models/tag_relationship.rb b/app/models/tag_relationship.rb index 0afc16ff9..02675032c 100644 --- a/app/models/tag_relationship.rb +++ b/app/models/tag_relationship.rb @@ -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) diff --git a/app/views/tag_relationships/_search.html.erb b/app/views/tag_relationships/_search.html.erb index 58389ce77..32d197ae0 100644 --- a/app/views/tag_relationships/_search.html.erb +++ b/app/views/tag_relationships/_search.html.erb @@ -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 %> diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index da2e1221d..ce7e05f8a 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/test/unit/bulk_update_request_test.rb @@ -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 diff --git a/test/unit/tag_alias_test.rb b/test/unit/tag_alias_test.rb index bd5af29d1..9b36d76c9 100644 --- a/test/unit/tag_alias_test.rb +++ b/test/unit/tag_alias_test.rb @@ -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) diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index 6dddde7a6..508c66221 100644 --- a/test/unit/tag_implication_test.rb +++ b/test/unit/tag_implication_test.rb @@ -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)