jaculabilis/danbooru
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

media assets: prevent duplicate media assets.

Browse Source 
Add a md5 uniqueness constraint on media assets to prevent duplicate
assets from being created. This way we can guarantee that there is one
active media asset per uploaded file.

Also make it so that if two people are uploading the same file at the
same time, the file is processed only once.
This commit is contained in:
evazion
2021-10-24 02:43:29 -05:00
parent a58aa8efa7
commit 5c7a0f225c
7 changed files with 121 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

38
app/models/media_asset.rb
View File

@@ -10,8 +10,11 @@ class MediaAsset < ApplicationRecord
active: 200, active: 200,
deleted: 300, deleted: 300,
expunged: 400, expunged: 400,
failed: 500,
} }
validates :md5, uniqueness: { conditions: -> { where(status: [:processing, :active]) } }
class Variant class Variant
attr_reader :media_asset, :variant attr_reader :media_asset, :variant
delegate :md5, :storage_service, :backup_storage_service, to: :media_asset delegate :md5, :storage_service, :backup_storage_service, to: :media_asset
@@ -120,11 +123,46 @@ class MediaAsset < ApplicationRecord
concerning :FileMethods do concerning :FileMethods do
class_methods do class_methods do
# Upload a file to Danbooru. Resize and distribute it then return a MediaAsset.
#
# If the file has already been uploaded to Danbooru, then return the
# existing MediaAsset. If someone else is uploading the same file at the
# same time, wait until they're finished and return the existing
# MediaAsset. If distributing the file fails, then mark the MediaAsset as
# failed and raise an exception.
#
# This can't be called inside a transaction because the transaction will
# fail if there's a RecordNotUnique error when the asset already exists.
def upload!(media_file) def upload!(media_file)
media_asset = create!(file: media_file, status: :processing) media_asset = create!(file: media_file, status: :processing)
media_asset.distribute_files!(media_file) media_asset.distribute_files!(media_file)
media_asset.update!(status: :active) media_asset.update!(status: :active)
media_asset media_asset
# If the file has already been uploaded, then the `create!` call will raise one of these errors.
rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique => e
raise if e.is_a?(ActiveRecord::RecordInvalid) && !e.record.errors.of_kind?(:md5, :taken)
media_asset = find_by!(md5: media_file.md5, status: [:processing, :active])
# XXX If the asset is still being processed by another thread, wait up
# to 30 seconds for it to finish.
if media_asset.processing?
30.times do
break if !media_asset.processing?
sleep 1
media_asset.reload
end
# If the asset is still processing after 30 seconds, or if it moved
# from the processing state to the failed state, then fail.
raise "Upload failed" if !media_asset.active?
end
media_asset
rescue Exception
media_asset&.update!(status: :failed)
raise
end end
end end

2
app/models/post.rb
View File

@@ -859,14 +859,12 @@ class Post < ApplicationRecord
end end
def replace!(params) def replace!(params)
transaction do
replacement = replacements.create(params) replacement = replacements.create(params)
processor = UploadService::Replacer.new(post: self, replacement: replacement) processor = UploadService::Replacer.new(post: self, replacement: replacement)
processor.process! processor.process!
replacement replacement
end end
end end
end
module VersionMethods module VersionMethods
def create_version(force = false) def create_version(force = false)

5
db/migrate/20211023225730_add_unique_md5_constraint_on_media_assets.rb Normal file
View File

@@ -0,0 +1,5 @@
class AddUniqueMd5ConstraintOnMediaAssets < ActiveRecord::Migration[6.1]
def change
add_index :media_assets, :md5, name: "index_media_assets_on_md5_and_status", unique: true, where: "status IN (100, 200)"
end
end

10
db/structure.sql
View File

@@ -3779,6 +3779,13 @@ CREATE INDEX index_media_assets_on_image_width ON public.media_assets USING btre
CREATE INDEX index_media_assets_on_md5 ON public.media_assets USING btree (md5); CREATE INDEX index_media_assets_on_md5 ON public.media_assets USING btree (md5);
--
-- Name: index_media_assets_on_md5_and_status; Type: INDEX; Schema: public; Owner: -
--
CREATE UNIQUE INDEX index_media_assets_on_md5_and_status ON public.media_assets USING btree (md5) WHERE (status = ANY (ARRAY[100, 200]));
-- --
-- Name: index_media_assets_on_status; Type: INDEX; Schema: public; Owner: - -- Name: index_media_assets_on_status; Type: INDEX; Schema: public; Owner: -
-- --
@@ -5049,6 +5056,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('20211014063943'), ('20211014063943'),
('20211015223510'), ('20211015223510'),
('20211018045429'), ('20211018045429'),
('20211018062916'); ('20211018062916'),
('20211023225730');

15
test/factories/post.rb
View File

@@ -13,5 +13,20 @@ FactoryBot.define do
rating {"q"} rating {"q"}
source { FFaker::Internet.http_url } source { FFaker::Internet.http_url }
media_asset { build(:media_asset) } media_asset { build(:media_asset) }
factory(:post_with_file) do
transient do
filename { "test.jpg" }
media_file { MediaFile.open("test/files/#{filename}") }
end
md5 { media_file.md5 }
image_width { media_file.width }
image_height { media_file.height }
file_ext { media_file.file_ext }
file_size { media_file.file_size }
before(:create) { |post, evaluator| MediaAsset.upload!(evaluator.media_file) }
end
end end
end end

3
test/unit/post_test.rb
View File

@@ -31,8 +31,7 @@ class PostTest < ActiveSupport::TestCase
context "Deletion:" do context "Deletion:" do
context "Expunging a post" do context "Expunging a post" do
setup do setup do
@upload = UploadService.new(FactoryBot.attributes_for(:jpg_upload)).start! @post = create(:post_with_file, uploader: @user, filename: "test.jpg")
@post = @upload.post
Favorite.create!(post: @post, user: @user) Favorite.create!(post: @post, user: @user)
create(:favorite_group, post_ids: [@post.id]) create(:favorite_group, post_ids: [@post.id])
perform_enqueued_jobs # perform IqdbAddPostJob perform_enqueued_jobs # perform IqdbAddPostJob

106
test/unit/upload_service_test.rb
View File

@@ -224,31 +224,29 @@ class UploadServiceTest < ActiveSupport::TestCase
end end
end end
subject { UploadService::Replacer.new(post: @post, replacement: @replacement) }
context "#process!" do context "#process!" do
should "create a comment" do should "create a comment" do
assert_difference(-> { @post.comments.count }) do assert_difference(-> { @post.reload.comments.count }) do
as(@user) { subject.process! } as(@user) { @post.reload.replace!(replacement_url: "", replacement_file: @new_file) }
@post.reload
end end
end end
should "not create a new post" do should "not create a new post" do
assert_difference(-> { Post.count }, 0) do assert_difference(-> { Post.count }, 0) do
as(@user) { subject.process! } as(@user) { @post.reload.replace!(replacement_url: "", replacement_file: @new_file) }
end end
end end
should "update the post's MD5" do should "update the post's MD5" do
assert_changes(-> { @post.md5 }) do assert_changes(-> { @post.reload.md5 }) do
as(@user) { subject.process! } as(@user) { @post.reload.replace!(replacement_url: "", replacement_file: @new_file) }
@post.reload
end end
end end
should "preserve the old values" do should "preserve the old values" do
as(@user) { subject.process! } as(@user) { @post.reload.replace!(replacement_url: "", replacement_file: @new_file) }
@replacement = @post.replacements.last
assert_equal(1500, @replacement.old_image_width) assert_equal(1500, @replacement.old_image_width)
assert_equal(1000, @replacement.old_image_height) assert_equal(1000, @replacement.old_image_height)
assert_equal(2000, @replacement.old_file_size) assert_equal(2000, @replacement.old_file_size)
@@ -257,8 +255,10 @@ class UploadServiceTest < ActiveSupport::TestCase
end end
should "record the new values" do should "record the new values" do
as(@user) { subject.process! } as(@user) { @post.reload.replace!(replacement_url: "", replacement_file: @new_file) }
assert_equal(500, @replacement.image_width) @replacement = @post.replacements.last
assert_equal(500, @replacement.reload.image_width)
assert_equal(335, @replacement.image_height) assert_equal(335, @replacement.image_height)
assert_equal(28086, @replacement.file_size) assert_equal(28086, @replacement.file_size)
assert_equal("jpg", @replacement.file_ext) assert_equal("jpg", @replacement.file_ext)
@@ -266,7 +266,9 @@ class UploadServiceTest < ActiveSupport::TestCase
end end
should "correctly update the attributes" do should "correctly update the attributes" do
as(@user) { subject.process! } as(@user) { @post.reload.replace!(replacement_url: "", replacement_file: @new_file) }
@replacement = @post.replacements.last
assert_equal(500, @post.image_width) assert_equal(500, @post.image_width)
assert_equal(335, @post.image_height) assert_equal(335, @post.image_height)
assert_equal(28086, @post.file_size) assert_equal(28086, @post.file_size)
@@ -280,7 +282,7 @@ class UploadServiceTest < ActiveSupport::TestCase
should "raise an error" do should "raise an error" do
upload_file("test/files/test.png") do |file| upload_file("test/files/test.png") do |file|
assert_raises(UploadService::Replacer::Error) do assert_raises(UploadService::Replacer::Error) do
as(@user) { @post.replace!(replacement_file: file, replacement_url: "") } as(@user) { @post.reload.replace!(replacement_file: file, replacement_url: "") }
end end
end end
end end
@@ -304,14 +306,10 @@ class UploadServiceTest < ActiveSupport::TestCase
end end
end end
subject { UploadService::Replacer.new(post: @post, replacement: @replacement) }
should "replace the post" do should "replace the post" do
as(@user) { subject.process! } as(@user) { @post.reload.replace!(replacement_url: @new_url) }
@post.reload assert_equal(@new_url, @post.reload.replacements.last.replacement_url)
assert_equal(@new_url, @post.replacements.last.replacement_url)
end end
end end
@@ -330,12 +328,10 @@ class UploadServiceTest < ActiveSupport::TestCase
end end
end end
subject { UploadService::Replacer.new(post: @post, replacement: @replacement) }
context "when replacing with its own source" do context "when replacing with its own source" do
should "raise an error" do should "raise an error" do
assert_raises(UploadService::Replacer::Error) do assert_raises(UploadService::Replacer::Error) do
as(@user) { @post.replace!(replacement_url: @post.source) } as(@user) { @post.reload.replace!(replacement_url: @post.source) }
end end
end end
end end
@@ -351,7 +347,7 @@ class UploadServiceTest < ActiveSupport::TestCase
should "throw an error" do should "throw an error" do
assert_raises(UploadService::Replacer::Error) do assert_raises(UploadService::Replacer::Error) do
as(@user) { @post2.replace!(replacement_url: @new_url) } as(@user) { @post2.reload.replace!(replacement_url: @new_url) }
end end
end end
end end
@@ -361,7 +357,7 @@ class UploadServiceTest < ActiveSupport::TestCase
replacement_url = "https://cdn.donmai.us/original/fd/b4/fdb47f79fb8da82e66eeb1d84a1cae8d.jpg" replacement_url = "https://cdn.donmai.us/original/fd/b4/fdb47f79fb8da82e66eeb1d84a1cae8d.jpg"
final_source = "https://cdn.donmai.us/original/71/0f/710fd9cba4ef37260f9152ffa9d154d8.png" final_source = "https://cdn.donmai.us/original/71/0f/710fd9cba4ef37260f9152ffa9d154d8.png"
as(@user) { @post.replace!(replacement_url: replacement_url, final_source: final_source) } as(@user) { @post.reload.replace!(replacement_url: replacement_url, final_source: final_source) }
assert_equal(final_source, @post.source) assert_equal(final_source, @post.source)
end end
@@ -372,7 +368,7 @@ class UploadServiceTest < ActiveSupport::TestCase
skip "Twitter key not set" unless Danbooru.config.twitter_api_key skip "Twitter key not set" unless Danbooru.config.twitter_api_key
replacement_url = "https://twitter.com/nounproject/status/540944400767922176" replacement_url = "https://twitter.com/nounproject/status/540944400767922176"
image_url = "https://pbs.twimg.com/media/B4HSEP5CUAA4xyu.png:orig" image_url = "https://pbs.twimg.com/media/B4HSEP5CUAA4xyu.png:orig"
as(@user) { @post.replace!(replacement_url: replacement_url) } as(@user) { @post.reload.replace!(replacement_url: replacement_url) }
assert_equal(replacement_url, @post.replacements.last.replacement_url) assert_equal(replacement_url, @post.replacements.last.replacement_url)
end end
@@ -384,7 +380,8 @@ class UploadServiceTest < ActiveSupport::TestCase
as(@user) do as(@user) do
@post = FactoryBot.create(:post, source: "https://cdn.donmai.us/original/d3/4e/d34e4cf0a437a5d65f8e82b7bcd02606.jpg") @post = FactoryBot.create(:post, source: "https://cdn.donmai.us/original/d3/4e/d34e4cf0a437a5d65f8e82b7bcd02606.jpg")
@post.stubs(:queue_delete_files) @post.stubs(:queue_delete_files)
@post.replace!(replacement_url: "https://cdn.donmai.us/original/fd/b4/fdb47f79fb8da82e66eeb1d84a1cae8d.jpg", tags: "-tag1 tag2") @new_url = "https://cdn.donmai.us/original/fd/b4/fdb47f79fb8da82e66eeb1d84a1cae8d.jpg"
@post.reload.replace!(replacement_url: @new_url, tags: "-tag1 tag2")
end end
@replacement = @post.replacements.last @replacement = @post.replacements.last
@@ -392,7 +389,8 @@ class UploadServiceTest < ActiveSupport::TestCase
should "update the attributes" do should "update the attributes" do
as(@user) do as(@user) do
subject.undo! replacer = UploadService::Replacer.new(post: @post.reload, replacement: @replacement)
replacer.undo!
end end
assert_equal("tag2", @post.tag_string) assert_equal("tag2", @post.tag_string)
@@ -408,41 +406,39 @@ class UploadServiceTest < ActiveSupport::TestCase
context "#process!" do context "#process!" do
should "create a comment" do should "create a comment" do
assert_difference(-> { @post.comments.count }) do assert_difference(-> { @post.reload.comments.count }) do
as(@user) { subject.process! } as(@user) { @post.reload.replace!(replacement_url: @new_url) }
@post.reload
end end
end end
should "not create a new post" do should "not create a new post" do
assert_difference(-> { Post.count }, 0) do assert_difference(-> { Post.count }, 0) do
as(@user) { subject.process! } as(@user) { @post.reload.replace!(replacement_url: @new_url) }
end end
end end
should "update the post's MD5" do should "update the post's MD5" do
assert_changes(-> { @post.md5 }) do assert_changes(-> { @post.reload.md5 }) do
as(@user) { subject.process! } as(@user) { @post.reload.replace!(replacement_url: @new_url) }
@post.reload
end end
end end
should "update the post's source" do should "update the post's source" do
assert_changes(-> { @post.source }, nil, from: @post.source, to: @new_url) do assert_changes(-> { @post.reload.source }, nil, from: @post.source, to: @new_url) do
as(@user) { subject.process! } as(@user) { @post.reload.replace!(replacement_url: @new_url) }
@post.reload @post.reload
end end
end end
should "not change the post status or uploader" do should "not change the post status or uploader" do
assert_no_changes(-> { {ip_addr: @post.uploader_ip_addr.to_s, uploader: @post.uploader_id, pending: @post.is_pending?} }) do assert_no_changes(-> { {ip_addr: @post.uploader_ip_addr.to_s, uploader: @post.uploader_id, pending: @post.is_pending?} }) do
as(@user) { subject.process! } as(@user) { @post.reload.replace!(replacement_url: @new_url) }
@post.reload @post.reload
end end
end end
should "leave a system comment" do should "leave a system comment" do
as(@user) { subject.process! } as(@user) { @post.reload.replace!(replacement_url: @new_url) }
comment = @post.comments.last comment = @post.comments.last
assert_not_nil(comment) assert_not_nil(comment)
assert_equal(User.system.id, comment.creator_id) assert_equal(User.system.id, comment.creator_id)
@@ -457,7 +453,7 @@ class UploadServiceTest < ActiveSupport::TestCase
should "replace with the full size image" do should "replace with the full size image" do
as(@user) do as(@user) do
@post.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350") @post.reload.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350")
end end
assert_equal(80, @post.image_width) assert_equal(80, @post.image_width)
@@ -474,7 +470,7 @@ class UploadServiceTest < ActiveSupport::TestCase
@post.unstub(:queue_delete_files) @post.unstub(:queue_delete_files)
FileUtils.expects(:rm_f).times(3) FileUtils.expects(:rm_f).times(3)
as(@user) { @post.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350") } as(@user) { @post.reload.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350") }
travel_to((PostReplacement::DELETION_GRACE_PERIOD + 1).days.from_now) do travel_to((PostReplacement::DELETION_GRACE_PERIOD + 1).days.from_now) do
perform_enqueued_jobs perform_enqueued_jobs
@@ -488,7 +484,7 @@ class UploadServiceTest < ActiveSupport::TestCase
skip "Pixiv credentials not configured" unless Sources::Strategies::Pixiv.enabled? skip "Pixiv credentials not configured" unless Sources::Strategies::Pixiv.enabled?
begin begin
as(@user) { @post.replace!(replacement_url: "http://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247364") } as(@user) { @post.reload.replace!(replacement_url: "http://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247364") }
@post.reload @post.reload
assert_equal(80, @post.image_width) assert_equal(80, @post.image_width)
@@ -515,12 +511,10 @@ class UploadServiceTest < ActiveSupport::TestCase
#FileUtils.expects(:rm_f).times(3) #FileUtils.expects(:rm_f).times(3)
as(@user) do as(@user) do
@post.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350") @post.reload.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350")
@post.reload @post.reload.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247364")
@post.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247364")
@post.reload
Upload.destroy_all Upload.destroy_all
@post.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350") @post.reload.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350")
end end
assert_nothing_raised { @post.file(:original) } assert_nothing_raised { @post.file(:original) }
@@ -550,20 +544,17 @@ class UploadServiceTest < ActiveSupport::TestCase
as(@user) do as(@user) do
skip unless MediaFile::Ugoira.videos_enabled? skip unless MediaFile::Ugoira.videos_enabled?
@post1.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350") @post1.reload.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.reload.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247364")
assert_equal("4ceadc314938bc27f3574053a3e1459a", @post1.md5) assert_equal("4ceadc314938bc27f3574053a3e1459a", @post1.md5)
assert_equal("cad1da177ef309bf40a117c17b8eecf5", @post2.md5) assert_equal("cad1da177ef309bf40a117c17b8eecf5", @post2.md5)
@post2.reload @post2.reload.replace!(replacement_url: "https://cdn.donmai.us/original/d3/4e/d34e4cf0a437a5d65f8e82b7bcd02606.jpg")
@post2.replace!(replacement_url: "https://cdn.donmai.us/original/d3/4e/d34e4cf0a437a5d65f8e82b7bcd02606.jpg")
assert_equal("d34e4cf0a437a5d65f8e82b7bcd02606", @post2.md5) assert_equal("d34e4cf0a437a5d65f8e82b7bcd02606", @post2.md5)
Upload.destroy_all Upload.destroy_all
@post1.reload
@post2.reload
@post1.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247364") @post1.reload.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") @post2.reload.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350")
assert_equal("cad1da177ef309bf40a117c17b8eecf5", @post1.md5) assert_equal("cad1da177ef309bf40a117c17b8eecf5", @post1.md5)
assert_equal("4ceadc314938bc27f3574053a3e1459a", @post2.md5) assert_equal("4ceadc314938bc27f3574053a3e1459a", @post2.md5)
end end
@@ -599,7 +590,7 @@ class UploadServiceTest < ActiveSupport::TestCase
assert_difference(-> { @note.versions.count }) do assert_difference(-> { @note.versions.count }) do
# replacement image is 80x82, so we're downscaling by 50% (160x164 -> 80x82). # replacement image is 80x82, so we're downscaling by 50% (160x164 -> 80x82).
as(@user) do as(@user) do
@post.replace!( @post.reload.replace!(
replacement_url: "https://i.pximg.net/img-original/img/2017/04/04/08/54/15/62247350_p0.png", replacement_url: "https://i.pximg.net/img-original/img/2017/04/04/08/54/15/62247350_p0.png",
final_source: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350" final_source: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350"
) )
@@ -690,7 +681,8 @@ class UploadServiceTest < ActiveSupport::TestCase
context "when the file has already been uploaded" do context "when the file has already been uploaded" do
setup do setup do
@post = create(:post, md5: "d34e4cf0a437a5d65f8e82b7bcd02606") @asset = MediaAsset.find_by_md5("d34e4cf0a437a5d65f8e82b7bcd02606")
@post = create(:post, md5: "d34e4cf0a437a5d65f8e82b7bcd02606", media_asset: @asset)
@service = subject.new(source: @source) @service = subject.new(source: @source)
end end