From 11ef460db01291b4d0f8a8dedb01793452627174 Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 24 Jul 2017 22:19:51 -0500 Subject: [PATCH 1/2] post replacements: allow replacing post with identical file. --- app/models/post_replacement.rb | 9 ++++++++- app/models/upload.rb | 9 ++++++--- test/unit/post_replacement_test.rb | 31 ++++++++++++++++++++++-------- 3 files changed, 37 insertions(+), 12 deletions(-) diff --git a/app/models/post_replacement.rb b/app/models/post_replacement.rb index 0351e1d7a..8396daf73 100644 --- a/app/models/post_replacement.rb +++ b/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) diff --git a/app/models/upload.rb b/app/models/upload.rb index 828d767e7..d0b9abd8b 100644 --- a/app/models/upload.rb +++ b/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 diff --git a/test/unit/post_replacement_test.rb b/test/unit/post_replacement_test.rb index 619b70a92..5aaae9186 100644 --- a/test/unit/post_replacement_test.rb +++ b/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 @@ -195,13 +204,9 @@ class PostReplacementTest < ActiveSupport::TestCase 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 +231,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 From b53371b698b8a4cb5273eb3296b37a8e62919711 Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 24 Jul 2017 23:04:01 -0500 Subject: [PATCH 2/2] Fix #3235: Replacements deleting files currently in use. --- app/models/post.rb | 6 ++++-- test/unit/post_replacement_test.rb | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index 34c769f39..960e58831 100644 --- a/app/models/post.rb +++ b/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 diff --git a/test/unit/post_replacement_test.rb b/test/unit/post_replacement_test.rb index 5aaae9186..3fe223943 100644 --- a/test/unit/post_replacement_test.rb +++ b/test/unit/post_replacement_test.rb @@ -202,6 +202,27 @@ 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 upload_file("#{Rails.root}/test/files/test.png", "test.png") do |file|