From 60a13fd2d5b4fe0c51313dcc628564a6feab1b6e Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 1 Feb 2022 00:25:31 -0600 Subject: [PATCH] Fix #4913: Invalid replacements created if an error is raised during replacement Perform the replacement in a before_create callback so that it runs in a transaction and if it fails, the transaction will rollback and the replacement record won't be created. Doing the replacement in a transaction isn't great because, for one thing, it could hold the transaction open a long time, which isn't good for the database. And two, if the transaction rolls back, the database changes will be undone, but if the replacement file has already been saved to disk, then it won't be undone, which could result in a dangling file. --- .../post_replacements_controller.rb | 9 ++++++-- app/logical/post_replacement_processor.rb | 9 ++++---- app/models/post_replacement.rb | 6 +++-- app/views/post_replacements/_new.html.erb | 2 +- app/views/post_replacements/create.js.erb | 2 -- test/factories/post_replacement.rb | 3 ++- .../post_replacements_controller_test.rb | 22 ++++++++++++++++--- 7 files changed, 37 insertions(+), 16 deletions(-) delete mode 100644 app/views/post_replacements/create.js.erb diff --git a/app/controllers/post_replacements_controller.rb b/app/controllers/post_replacements_controller.rb index 325773a19..05ec2a175 100644 --- a/app/controllers/post_replacements_controller.rb +++ b/app/controllers/post_replacements_controller.rb @@ -11,9 +11,14 @@ class PostReplacementsController < ApplicationController def create @post_replacement = authorize PostReplacement.new(creator: CurrentUser.user, post_id: params[:post_id], **permitted_attributes(PostReplacement)) @post_replacement.save - @post_replacement.process! - respond_with(@post_replacement, location: @post_replacement.post, notice: "Post replaced") + if request.format.html? && @post_replacement.errors.any? + flash[:notice] = @post_replacement.errors.full_messages.join("; ") + redirect_to @post_replacement.post + else + flash[:notice] = "Post replaced" + respond_with(@post_replacement, location: @post_replacement.post) + end end def update diff --git a/app/logical/post_replacement_processor.rb b/app/logical/post_replacement_processor.rb index 44e4533e6..14b0590c6 100644 --- a/app/logical/post_replacement_processor.rb +++ b/app/logical/post_replacement_processor.rb @@ -12,8 +12,7 @@ class PostReplacementProcessor media_file = get_file_for_upload(replacement.replacement_url, nil, replacement.replacement_file&.tempfile) if Post.where.not(id: post.id).exists?(md5: media_file.md5) - replacement.errors.add(:base, "Duplicate: post with md5 #{media_file.md5} already exists") - return + raise "Duplicate of post ##{Post.find_by_md5(media_file.md5).id}" end if media_file.md5 == post.md5 @@ -44,11 +43,11 @@ class PostReplacementProcessor post.tag_string = replacement.tags rescale_notes(post) - - replacement.save! post.save! - post.update_iqdb + rescue Exception => exception + replacement.errors.add(:base, exception.message) + raise ActiveRecord::Rollback end def rescale_notes(post) diff --git a/app/models/post_replacement.rb b/app/models/post_replacement.rb index 0b1ffc6b9..099a9c4fd 100644 --- a/app/models/post_replacement.rb +++ b/app/models/post_replacement.rb @@ -3,9 +3,11 @@ class PostReplacement < ApplicationRecord belongs_to :post belongs_to :creator, class_name: "User" - before_validation :initialize_fields, on: :create - attr_accessor :replacement_file, :final_source, :tags + before_validation :initialize_fields, on: :create + before_create :process! + + attr_accessor :replacement_file, :final_source, :tags attribute :replacement_url, default: "" def initialize_fields diff --git a/app/views/post_replacements/_new.html.erb b/app/views/post_replacements/_new.html.erb index f6552a7a0..d60441fcb 100644 --- a/app/views/post_replacements/_new.html.erb +++ b/app/views/post_replacements/_new.html.erb @@ -1,7 +1,7 @@
<%= embed_wiki("help:replacement_notice") %> - <%= edit_form_for(post_replacement, url: post_replacements_path(post_id: post_replacement.post_id), method: :post, remote: true) do |f| %> + <%= edit_form_for(post_replacement, url: post_replacements_path(post_id: post_replacement.post_id), method: :post) do |f| %> <%= f.input :replacement_file, label: "File", as: :file %> <%= f.input :replacement_url, label: "Replacement URL", hint: "The source URL to download the replacement from.", as: :string, input_html: { value: post_replacement.post.normalized_source } %> <%= f.input :final_source, label: "Final Source", hint: "If present, the source field will be changed to this after replacement.", as: :string %> diff --git a/app/views/post_replacements/create.js.erb b/app/views/post_replacements/create.js.erb deleted file mode 100644 index 7662f71ec..000000000 --- a/app/views/post_replacements/create.js.erb +++ /dev/null @@ -1,2 +0,0 @@ -Danbooru.notice("Post replaced."); -location.reload(); diff --git a/test/factories/post_replacement.rb b/test/factories/post_replacement.rb index 20d2a8acb..0379493a2 100644 --- a/test/factories/post_replacement.rb +++ b/test/factories/post_replacement.rb @@ -2,7 +2,8 @@ FactoryBot.define do factory(:post_replacement) do post factory: :post, source: FFaker::Internet.http_url original_url { FFaker::Internet.http_url } - replacement_url { FFaker::Internet.http_url } + replacement_url { "" } + replacement_file { Rack::Test::UploadedFile.new("test/files/test.jpg") } creator end end diff --git a/test/functional/post_replacements_controller_test.rb b/test/functional/post_replacements_controller_test.rb index cebb18484..9380dfd6e 100644 --- a/test/functional/post_replacements_controller_test.rb +++ b/test/functional/post_replacements_controller_test.rb @@ -154,6 +154,23 @@ class PostReplacementsControllerTest < ActionDispatch::IntegrationTest end end + context "a replacement that fails" do + should "not create a post replacement record" do + @post = create(:post) + + assert_no_difference("PostReplacement.count") do + post_auth post_replacements_path, create(:moderator_user), params: { + post_id: @post.id, + post_replacement: { + replacement_file: Rack::Test::UploadedFile.new("test/files/ugoira.json"), + } + } + + assert_redirected_to @post + end + end + end + should "not allow non-mods to replace posts" do assert_difference("PostReplacement.count", 0) do @post = create(:post) @@ -187,8 +204,8 @@ class PostReplacementsControllerTest < ActionDispatch::IntegrationTest @admin = create(:admin_user) @mod = create(:moderator_user, name: "yukari") - @post_replacement = create(:post_replacement, creator: @mod, post: create(:post, tag_string: "touhou")) - @admin_replacement = create(:post_replacement, creator: create(:admin_user), replacement_url: "https://danbooru.donmai.us") + @post_replacement = create(:post_replacement, creator: @mod, post: create(:post, tag_string: "touhou"), replacement_file: Rack::Test::UploadedFile.new("test/files/test.png")) + @admin_replacement = create(:post_replacement, creator: @admin, replacement_file: Rack::Test::UploadedFile.new("test/files/test.jpg")) end should "render" do @@ -197,7 +214,6 @@ class PostReplacementsControllerTest < ActionDispatch::IntegrationTest end should respond_to_search({}).with { [@admin_replacement, @post_replacement] } - should respond_to_search(replacement_url_like: "*danbooru*").with { @admin_replacement } context "using includes" do should respond_to_search(post_tags_match: "touhou").with { @post_replacement }