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.

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

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")
+      raise "Duplicate of post ##{Post.find_by_md5(media_file.md5).id}"
-      return
     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)

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

app/views/post_replacements/_new.html.erb

@@ -1,7 +1,7 @@
 <div class="replace-image-dialog-body">
   <%= 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 %>

app/views/post_replacements/create.js.erb

@@ -1,2 +0,0 @@
-Danbooru.notice("Post replaced.");
-location.reload();

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

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"))
+        @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: create(:admin_user), replacement_url: "https://danbooru.donmai.us")
+        @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 }