Merge pull request #3240 from evazion/fix-3235

Fix #3235: Replacements deleting files currently in use.

app/models/post.rb

@@ -67,9 +67,11 @@ class Post < ApplicationRecord
     module ClassMethods
       def delete_files(post_id, file_path, large_file_path, preview_file_path, force: false)
         unless force
-          post = Post.find(post_id)
+          # XXX should pass in the md5 instead of parsing it.
+          preview_file_path =~ %r!/data/preview/(?:test\.)?([a-z0-9]{32})\.jpg\z!
+          md5 = $1
 
-          if post.file_path == file_path || post.large_file_path == large_file_path || post.preview_file_path == preview_file_path
+          if Post.where(md5: md5).exists?
             raise DeletionError.new("Files still in use; skipping deletion.")
           end
         end

app/models/post_replacement.rb

@@ -19,7 +19,14 @@ class PostReplacement < ApplicationRecord
 
   def process!
     transaction do
-      upload = Upload.create!(file: replacement_file, source: replacement_url, rating: post.rating, tag_string: self.tags)
+      upload = Upload.create!(
+        file: replacement_file,
+        source: replacement_url,
+        rating: post.rating,
+        tag_string: self.tags,
+        replaced_post: post,
+      )
+
       upload.process_upload
       upload.update(status: "completed", post_id: post.id)
 

app/models/upload.rb

@@ -7,7 +7,7 @@ class Upload < ApplicationRecord
   attr_accessor :file, :image_width, :image_height, :file_ext, :md5, 
     :file_size, :as_pending, :artist_commentary_title, 
     :artist_commentary_desc, :include_artist_commentary,
-    :referer_url, :downloaded_source
+    :referer_url, :downloaded_source, :replaced_post
   belongs_to :uploader, :class_name => "User"
   belongs_to :post
   before_validation :initialize_uploader, :on => :create
@@ -22,7 +22,7 @@ class Upload < ApplicationRecord
     :tag_string, :status, :backtrace, :post_id, :md5_confirmation, 
     :parent_id, :server, :artist_commentary_title,
     :artist_commentary_desc, :include_artist_commentary,
-    :referer_url
+    :referer_url, :replaced_post
 
   module ValidationMethods
     def uploader_is_not_limited
@@ -46,7 +46,10 @@ class Upload < ApplicationRecord
     # Because uploads are processed serially, there's no race condition here.
     def validate_md5_uniqueness
       md5_post = Post.find_by_md5(md5)
-      if md5_post
+
+      if md5_post && replaced_post
+        raise "duplicate: #{md5_post.id}" if replaced_post != md5_post
+      elsif md5_post
         raise "duplicate: #{md5_post.id}"
       end
     end

test/unit/post_replacement_test.rb

@@ -4,6 +4,15 @@ require 'helpers/iqdb_test_helper'
 class PostReplacementTest < ActiveSupport::TestCase
   include IqdbTestHelper
 
+  def upload_file(path, filename, &block)
+    Tempfile.open do |file|
+      file.write(File.read(path))
+      file.seek(0)
+      uploaded_file = ActionDispatch::Http::UploadedFile.new(tempfile: file, filename: filename)
+      yield uploaded_file
+    end
+  end
+
   def setup
     mock_iqdb_service!
     Delayed::Worker.delay_jobs = true # don't delete the old images right away
@@ -168,7 +177,7 @@ class PostReplacementTest < ActiveSupport::TestCase
         assert_equal("cad1da177ef309bf40a117c17b8eecf5", @post.md5)
         assert_equal("cad1da177ef309bf40a117c17b8eecf5", Digest::MD5.file(@post.file_path).hexdigest)
 
-        assert_equal("https://i1.pixiv.net/img-zip-ugoira/img/2017/04/04/08/57/38/62247364_ugoira1920x1080.zip", @post.source)
+        assert_equal("https://i.pximg.net/img-zip-ugoira/img/2017/04/04/08/57/38/62247364_ugoira1920x1080.zip", @post.source)
         assert_equal([{"file"=>"000000.jpg", "delay"=>125}, {"file"=>"000001.jpg", "delay"=>125}], @post.pixiv_ugoira_frame_data.data)
       end
     end
@@ -193,15 +202,32 @@ class PostReplacementTest < ActiveSupport::TestCase
       end
     end
 
+    context "two posts that have had their files swapped" do
+      should "not delete the still active files" do
+        @post1 = FactoryGirl.create(:post)
+        @post2 = FactoryGirl.create(:post)
+
+        # swap the images between @post1 and @post2.
+        @post1.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350")
+        @post2.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247364")
+        @post2.replace!(replacement_url: "https://www.google.com/intl/en_ALL/images/logo.gif")
+        @post1.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247364")
+        @post2.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350")
+
+        Timecop.travel(Time.now + PostReplacement::DELETION_GRACE_PERIOD + 1.day) do
+          Delayed::Worker.new.work_off
+        end
+
+        assert(File.exists?(@post1.file_path))
+        assert(File.exists?(@post2.file_path))
+      end
+    end
+
     context "a post with an uploaded file" do
       should "work" do
-        Tempfile.open do |file|
-          file.write(File.read("#{Rails.root}/test/files/test.png"))
-          file.seek(0)
-          uploaded_file = ActionDispatch::Http::UploadedFile.new(tempfile: file, filename: "test.png")
-
-          @post.replace!(replacement_file: uploaded_file, replacement_url: "")
-          assert_equal(@post.md5, Digest::MD5.file(file).hexdigest)
+        upload_file("#{Rails.root}/test/files/test.png", "test.png") do |file|
+          @post.replace!(replacement_file: file, replacement_url: "")
+          assert_equal(@post.md5, Digest::MD5.file(file.tempfile).hexdigest)
           assert_equal("file://test.png", @post.replacements.last.replacement_url)
         end
       end
@@ -226,5 +252,15 @@ class PostReplacementTest < ActiveSupport::TestCase
         assert_equal(image_url, @post.replacements.last.replacement_url)
       end
     end
+
+    context "a post with the same file" do
+      should "not raise a duplicate error" do
+        upload_file("#{Rails.root}/test/files/test.jpg", "test.jpg") do |file|
+          assert_nothing_raised do
+            @post.replace!(replacement_file: file, replacement_url: "")
+          end
+        end
+      end
+    end
   end
 end