From bc506ed1b8ca8a435de29d993f8e178c7cc1f78f Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 18 Oct 2021 04:54:52 -0500 Subject: [PATCH] uploads: refactor to simplify ugoira-handling and replacements: * Make it so replacing a post doesn't generate a dummy upload as a side effect. * Make it so you can't replace a post with itself (the post should be regenerated instead). * Refactor uploads and replacements to save the ugoira frame data when the MediaAsset is created, not when the post is created. This way it's possible to view the ugoira before the post is created. * Make `download_file!` in the Pixiv source strategy return a MediaFile with the ugoira frame data already attached to it, instead of returning it in the `data` field then passing it around separately in the `context` field of the upload. --- app/logical/media_file.rb | 2 + app/logical/media_file/ugoira.rb | 2 +- app/logical/sources/strategies/base.rb | 4 - app/logical/sources/strategies/pixiv.rb | 11 +- app/logical/upload_service.rb | 10 +- app/logical/upload_service/preprocessor.rb | 2 +- app/logical/upload_service/replacer.rb | 103 +++++------------- app/logical/upload_service/utils.rb | 25 ++--- app/models/media_asset.rb | 3 + app/models/pixiv_ugoira_frame_data.rb | 5 +- app/models/post.rb | 2 +- app/models/upload.rb | 8 +- ..._to_nullable_on_pixiv_ugoira_frame_data.rb | 5 + db/structure.sql | 5 +- test/unit/downloads/pixiv_test.rb | 5 +- test/unit/sources/pixiv_test.rb | 6 +- test/unit/upload_service_test.rb | 99 +---------------- 17 files changed, 76 insertions(+), 221 deletions(-) create mode 100644 db/migrate/20211018062916_change_post_id_to_nullable_on_pixiv_ugoira_frame_data.rb diff --git a/app/logical/media_file.rb b/app/logical/media_file.rb index d7991c61b..ac4dd8bcd 100644 --- a/app/logical/media_file.rb +++ b/app/logical/media_file.rb @@ -19,6 +19,8 @@ class MediaFile # @param options [Hash] extra options for the MediaFile subclass. # @return [MediaFile] the media file def self.open(file, **options) + return file.dup if file.is_a?(MediaFile) + file = Kernel.open(file, "r", binmode: true) unless file.respond_to?(:read) case file_ext(file) diff --git a/app/logical/media_file/ugoira.rb b/app/logical/media_file/ugoira.rb index 893457731..cfd765131 100644 --- a/app/logical/media_file/ugoira.rb +++ b/app/logical/media_file/ugoira.rb @@ -7,7 +7,7 @@ # zip file, so it must be passed around separately. class MediaFile::Ugoira < MediaFile class Error < StandardError; end - attr_reader :frame_data + attr_accessor :frame_data def initialize(file, frame_data: {}, **options) super(file, **options) diff --git a/app/logical/sources/strategies/base.rb b/app/logical/sources/strategies/base.rb index 89ec4124f..6fe42e0aa 100644 --- a/app/logical/sources/strategies/base.rb +++ b/app/logical/sources/strategies/base.rb @@ -276,10 +276,6 @@ module Sources image_url end - def data - {} - end - def tags (@tags || []).uniq end diff --git a/app/logical/sources/strategies/pixiv.rb b/app/logical/sources/strategies/pixiv.rb index fb80f8e6b..af7650438 100644 --- a/app/logical/sources/strategies/pixiv.rb +++ b/app/logical/sources/strategies/pixiv.rb @@ -199,6 +199,12 @@ module Sources tag.gsub(/\d+users入り\z/i, "") end + def download_file!(url = image_url) + file = super(url) + file.frame_data = ugoira_frame_data if is_ugoira? + file + end + def translate_tag(tag) translated_tags = super(tag) @@ -297,8 +303,9 @@ module Sources end end - def data - { ugoira_frame_data: api_ugoira[:frames] } + def ugoira_frame_data + return nil unless is_ugoira? + api_ugoira[:frames] end def ugoira_content_type diff --git a/app/logical/upload_service.rb b/app/logical/upload_service.rb index 12e95d90f..9e694cd64 100644 --- a/app/logical/upload_service.rb +++ b/app/logical/upload_service.rb @@ -43,7 +43,7 @@ class UploadService @upload.update(status: "processing") - @upload.file = Utils.get_file_for_upload(@upload, file: @upload.file&.tempfile) + @upload.file = Utils.get_file_for_upload(@upload.source_url, @upload.referer_url, @upload.file&.tempfile) Utils.process_file(upload, @upload.file) @upload.save! @@ -64,14 +64,6 @@ class UploadService @post = convert_to_post(upload) @post.save! - if upload.context && upload.context["ugoira"] - PixivUgoiraFrameData.create( - post_id: @post.id, - data: upload.context["ugoira"]["frame_data"], - content_type: upload.context["ugoira"]["content_type"] - ) - end - if upload.has_commentary? @post.create_artist_commentary( :original_title => upload.artist_commentary_title, diff --git a/app/logical/upload_service/preprocessor.rb b/app/logical/upload_service/preprocessor.rb index f8b298e17..a90444df5 100644 --- a/app/logical/upload_service/preprocessor.rb +++ b/app/logical/upload_service/preprocessor.rb @@ -71,7 +71,7 @@ class UploadService begin upload.update(status: "preprocessing") - file = Utils.get_file_for_upload(upload, file: params[:file]&.tempfile) + file = Utils.get_file_for_upload(upload.source_url, upload.referer_url, params[:file]&.tempfile) Utils.process_file(upload, file, original_post_id: original_post_id) upload.rating = params[:rating] diff --git a/app/logical/upload_service/replacer.rb b/app/logical/upload_service/replacer.rb index 739d7c161..684c032e9 100644 --- a/app/logical/upload_service/replacer.rb +++ b/app/logical/upload_service/replacer.rb @@ -61,69 +61,44 @@ class UploadService undoer.process! end - def source_strategy(upload) - Sources::Strategies.find(upload.source, upload.referer_url) - end - - def find_replacement_url(repl, upload) - if repl.replacement_file.present? - return "file://#{repl.replacement_file.original_filename}" + def replacement_url + if replacement.replacement_file.present? + "file://#{replacement.replacement_file.original_filename}" + else + Sources::Strategies.find(replacement.replacement_url).canonical_url end - - if upload.source.blank? - raise "No source found in upload for replacement" - end - - if source_strategy(upload).canonical_url.present? - return source_strategy(upload).canonical_url - end - - upload.source end def process! - preprocessor = Preprocessor.new( - rating: post.rating, - tag_string: replacement.tags, - source: replacement.replacement_url, - file: replacement.replacement_file, - replaced_post: post, - original_post_id: post.id - ) - upload = preprocessor.start! - raise Error, upload.status if upload.is_errored? - upload = preprocessor.finish!(upload) - raise Error, upload.status if upload.is_errored? - md5_changed = upload.md5 != post.md5 + media_file = Utils::get_file_for_upload(replacement.replacement_url, nil, replacement.replacement_file&.tempfile) - replacement.replacement_url = find_replacement_url(replacement, upload) - - if md5_changed - post.queue_delete_files(PostReplacement::DELETION_GRACE_PERIOD) + if media_file.md5 == post.md5 + raise Error, "Can't replace a post with itself; regenerate the post instead" + elsif Post.exists?(md5: media_file.md5) + raise Error, "Duplicate: post with md5 #{media_file.md5} already exists" end - replacement.file_ext = upload.file_ext - replacement.file_size = upload.file_size - replacement.image_height = upload.image_height - replacement.image_width = upload.image_width - replacement.md5 = upload.md5 + media_asset = MediaAsset.upload!(media_file) + post.queue_delete_files(PostReplacement::DELETION_GRACE_PERIOD) - post.md5 = upload.md5 - post.file_ext = upload.file_ext - post.image_width = upload.image_width - post.image_height = upload.image_height - post.file_size = upload.file_size + replacement.replacement_url = replacement_url + replacement.file_ext = media_asset.file_ext + replacement.file_size = media_asset.file_size + replacement.image_height = media_asset.image_height + replacement.image_width = media_asset.image_width + replacement.md5 = media_asset.md5 + + post.md5 = media_asset.md5 + post.file_ext = media_asset.file_ext + post.image_width = media_asset.image_width + post.image_height = media_asset.image_height + post.file_size = media_asset.file_size post.source = replacement.final_source.presence || replacement.replacement_url - post.tag_string = upload.tag_string + post.tag_string = replacement.tags rescale_notes(post) - update_ugoira_frame_data(post, upload) - if md5_changed - CurrentUser.scoped(User.system) { Comment.create!(post: post, creator: User.system, updater: User.system, body: comment_replacement_message(post, replacement), do_not_bump_post: true, creator_ip_addr: "127.0.0.1") } - else - purge_cached_urls(post) - end + CurrentUser.scoped(User.system) { Comment.create!(post: post, creator: User.system, updater: User.system, body: comment_replacement_message(post, replacement), do_not_bump_post: true, creator_ip_addr: "127.0.0.1") } replacement.save! post.save! @@ -131,18 +106,6 @@ class UploadService post.update_iqdb end - def purge_cached_urls(post) - urls = [ - post.preview_file_url, - post.large_file_url, - post.file_url, - post.tagged_large_file_url, - post.tagged_file_url - ] - - CloudflareService.new.purge_cache(urls) - end - def rescale_notes(post) x_scale = post.image_width.to_f / post.image_width_was.to_f y_scale = post.image_height.to_f / post.image_height_was.to_f @@ -151,19 +114,5 @@ class UploadService note.rescale!(x_scale, y_scale) end end - - def update_ugoira_frame_data(post, upload) - post.pixiv_ugoira_frame_data.destroy if post.pixiv_ugoira_frame_data.present? - - unless post.is_ugoira? - return - end - - PixivUgoiraFrameData.create( - post_id: post.id, - data: upload.context["ugoira"]["frame_data"], - content_type: upload.context["ugoira"]["content_type"] - ) - end end end diff --git a/app/logical/upload_service/utils.rb b/app/logical/upload_service/utils.rb index ed23c8fc9..f5a219706 100644 --- a/app/logical/upload_service/utils.rb +++ b/app/logical/upload_service/utils.rb @@ -7,9 +7,9 @@ class UploadService end def process_file(upload, file, original_post_id: nil) - upload.file = file - media_file = upload.media_file + media_file = MediaFile.open(file) + upload.file = media_file upload.file_ext = media_file.file_ext.to_s upload.file_size = media_file.file_size upload.md5 = media_file.md5 @@ -28,25 +28,14 @@ class UploadService tags.join(" ") end - def get_file_for_upload(upload, file: nil) - return file if file.present? - raise "No file or source URL provided" if upload.source_url.blank? + def get_file_for_upload(source_url, referer_url, file) + return MediaFile.open(file) if file.present? + raise "No file or source URL provided" if source_url.blank? - strategy = Sources::Strategies.find(upload.source_url, upload.referer_url) + strategy = Sources::Strategies.find(source_url, referer_url) raise NotImplementedError, "No login credentials configured for #{strategy.site_name}." unless strategy.class.enabled? - file = strategy.download_file! - - if strategy.data[:ugoira_frame_data].present? - upload.context = { - "ugoira" => { - "frame_data" => strategy.data[:ugoira_frame_data], - "content_type" => "image/jpeg" - } - } - end - - file + strategy.download_file! end end end diff --git a/app/models/media_asset.rb b/app/models/media_asset.rb index dff00ebf1..3be6214e5 100644 --- a/app/models/media_asset.rb +++ b/app/models/media_asset.rb @@ -1,5 +1,7 @@ class MediaAsset < ApplicationRecord has_one :media_metadata, dependent: :destroy + has_one :pixiv_ugoira_frame_data, class_name: "PixivUgoiraFrameData", dependent: :destroy, foreign_key: :md5, primary_key: :md5 + delegate :metadata, to: :media_metadata delegate :is_non_repeating_animation?, :is_greyscale?, :is_rotated?, to: :metadata @@ -136,6 +138,7 @@ class MediaAsset < ApplicationRecord self.image_height = media_file.height self.duration = media_file.duration self.media_metadata = MediaMetadata.new(file: media_file) + self.pixiv_ugoira_frame_data = PixivUgoiraFrameData.new(data: media_file.frame_data, content_type: "image/jpeg") if is_ugoira? end def delete_files! diff --git a/app/models/pixiv_ugoira_frame_data.rb b/app/models/pixiv_ugoira_frame_data.rb index bbb5a5498..89b7d97b0 100644 --- a/app/models/pixiv_ugoira_frame_data.rb +++ b/app/models/pixiv_ugoira_frame_data.rb @@ -1,5 +1,6 @@ class PixivUgoiraFrameData < ApplicationRecord - belongs_to :post + belongs_to :post, optional: true, foreign_key: :md5, primary_key: :md5 + belongs_to :media_asset, foreign_key: :md5, primary_key: :md5 serialize :data before_validation :normalize_data, on: :create @@ -9,7 +10,7 @@ class PixivUgoiraFrameData < ApplicationRecord end def self.search(params) - q = search_attributes(params, :id, :data, :content_type, :post) + q = search_attributes(params, :id, :data, :content_type, :post, :md5) q.apply_default_order(params) end diff --git a/app/models/post.rb b/app/models/post.rb index d1e11e3f5..e32ea6e4a 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -40,7 +40,7 @@ class Post < ApplicationRecord has_one :media_asset, foreign_key: :md5, primary_key: :md5 has_one :upload, :dependent => :destroy has_one :artist_commentary, :dependent => :destroy - has_one :pixiv_ugoira_frame_data, :class_name => "PixivUgoiraFrameData", :dependent => :destroy + has_one :pixiv_ugoira_frame_data, class_name: "PixivUgoiraFrameData", foreign_key: :md5, primary_key: :md5 has_many :flags, :class_name => "PostFlag", :dependent => :destroy has_many :appeals, :class_name => "PostAppeal", :dependent => :destroy has_many :votes, :class_name => "PostVote", :dependent => :destroy diff --git a/app/models/upload.rb b/app/models/upload.rb index 665dacbd5..12d636f65 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -19,7 +19,7 @@ class Upload < ApplicationRecord end def validate_integrity(record) - if record.media_file.is_corrupt? + if record.file.is_corrupt? record.errors.add(:file, "is corrupted") end end @@ -55,7 +55,7 @@ class Upload < ApplicationRecord end def validate_video_duration(record) - if !record.uploader.is_admin? && record.media_file.is_video? && record.media_file.duration > MAX_VIDEO_DURATION + if !record.uploader.is_admin? && record.file.is_video? && record.file.duration > MAX_VIDEO_DURATION record.errors.add(:base, "video must not be longer than #{MAX_VIDEO_DURATION.seconds.inspect}") end end @@ -105,10 +105,6 @@ class Upload < ApplicationRecord end concerning :FileMethods do - def media_file - @media_file ||= MediaFile.open(file, frame_data: context.to_h.dig("ugoira", "frame_data")) - end - def delete_files # md5 is blank if the upload errored out before downloading the file. if is_completed? || md5.blank? || Upload.exists?(md5: md5) || Post.exists?(md5: md5) diff --git a/db/migrate/20211018062916_change_post_id_to_nullable_on_pixiv_ugoira_frame_data.rb b/db/migrate/20211018062916_change_post_id_to_nullable_on_pixiv_ugoira_frame_data.rb new file mode 100644 index 000000000..0be2470eb --- /dev/null +++ b/db/migrate/20211018062916_change_post_id_to_nullable_on_pixiv_ugoira_frame_data.rb @@ -0,0 +1,5 @@ +class ChangePostIdToNullableOnPixivUgoiraFrameData < ActiveRecord::Migration[6.1] + def change + change_column_null :pixiv_ugoira_frame_data, :post_id, true + end +end diff --git a/db/structure.sql b/db/structure.sql index 8b8a2443e..318c286e7 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -1327,7 +1327,7 @@ ALTER SEQUENCE public.notes_id_seq OWNED BY public.notes.id; CREATE TABLE public.pixiv_ugoira_frame_data ( id integer NOT NULL, - post_id integer NOT NULL, + post_id integer, data text NOT NULL, content_type character varying NOT NULL, md5 character varying @@ -5048,6 +5048,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20211013011619'), ('20211014063943'), ('20211015223510'), -('20211018045429'); +('20211018045429'), +('20211018062916'); diff --git a/test/unit/downloads/pixiv_test.rb b/test/unit/downloads/pixiv_test.rb index 00248c1d6..f651a77e4 100644 --- a/test/unit/downloads/pixiv_test.rb +++ b/test/unit/downloads/pixiv_test.rb @@ -131,9 +131,10 @@ module Downloads context "An ugoira site for pixiv" do should "capture the data" do @strategy = Sources::Strategies.find("http://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247364") + media_file = @strategy.download_file! - assert_equal(2, @strategy.data[:ugoira_frame_data].size) - assert_equal([{"file" => "000000.jpg", "delay" => 125}, {"file" => "000001.jpg", "delay" => 125}], @strategy.data[:ugoira_frame_data]) + assert_equal(2, media_file.frame_data.size) + assert_equal([{"file" => "000000.jpg", "delay" => 125}, {"file" => "000001.jpg", "delay" => 125}], media_file.frame_data) end end end diff --git a/test/unit/sources/pixiv_test.rb b/test/unit/sources/pixiv_test.rb index 87b89f9d9..146a4292e 100644 --- a/test/unit/sources/pixiv_test.rb +++ b/test/unit/sources/pixiv_test.rb @@ -48,10 +48,10 @@ module Sources end should "capture the frame data" do - ugoira_frame_data = @site.data[:ugoira_frame_data] + media_file = @site.download_file! - assert_equal(2, ugoira_frame_data.size) - assert_equal([{"file" => "000000.jpg", "delay" => 125}, {"file" => "000001.jpg", "delay" => 125}], ugoira_frame_data) + assert_equal(2, media_file.frame_data.size) + assert_equal([{"file" => "000000.jpg", "delay" => 125}, {"file" => "000001.jpg", "delay" => 125}], media_file.frame_data) end end diff --git a/test/unit/upload_service_test.rb b/test/unit/upload_service_test.rb index 56eb59339..aa86d5a80 100644 --- a/test/unit/upload_service_test.rb +++ b/test/unit/upload_service_test.rb @@ -23,59 +23,6 @@ class UploadServiceTest < ActiveSupport::TestCase end context "::Utils" do - context "#get_file_for_upload" do - context "for a non-source site" do - setup do - @source = "https://upload.wikimedia.org/wikipedia/commons/c/c5/Moraine_Lake_17092005.jpg" - @upload = Upload.new - @upload.source = @source - end - - should "work on a jpeg" do - file = UploadService::Utils.get_file_for_upload(@upload) - assert_operator(File.size(file.path), :>, 0) - file.close - end - end - - context "for a pixiv" do - setup do - skip "Pixiv credentials not configured" unless Sources::Strategies::Pixiv.enabled? - - @source = "http://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350" - @upload = Upload.new - @upload.source = @source - end - - should "work on an ugoira url" do - begin - file = UploadService::Utils.get_file_for_upload(@upload) - assert_operator(File.size(file.path), :>, 0) - file.close - end - end - end - - context "for a pixiv ugoira" do - setup do - skip "Pixiv credentials not configured" unless Sources::Strategies::Pixiv.enabled? - - @source = "https://i.pximg.net/img-zip-ugoira/img/2017/04/04/08/57/38/62247364_ugoira1920x1080.zip" - @referer = "http://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247364" - @upload = Upload.new - @upload.source = @source - @upload.referer_url = @referer - end - - should "work on an ugoira url" do - file = UploadService::Utils.get_file_for_upload(@upload) - - assert_not_nil(@upload.context["ugoira"]) - assert_operator(File.size(file.path), :>, 0) - end - end - end - context ".process_file" do setup do @upload = FactoryBot.build(:jpg_upload) @@ -286,12 +233,6 @@ class UploadServiceTest < ActiveSupport::TestCase subject { UploadService::Replacer.new(post: @post, replacement: @replacement) } context "#process!" do - should "create a new upload" do - assert_difference(-> { Upload.count }) do - as(@user) { subject.process! } - end - end - should "create a comment" do assert_difference(-> { @post.comments.count }) do as(@user) { subject.process! } @@ -342,22 +283,13 @@ class UploadServiceTest < ActiveSupport::TestCase end context "a post with the same file" do - should "not raise a duplicate error" do + should "raise an error" do upload_file("test/files/test.png") do |file| - assert_nothing_raised do + assert_raises(UploadService::Replacer::Error) do as(@user) { @post.replace!(replacement_file: file, replacement_url: "") } end end end - - should "not queue a deletion or log a comment" do - upload_file("test/files/test.png") do |file| - assert_no_difference(-> { @post.comments.count }) do - as(@user) { @post.replace!(replacement_file: file, replacement_url: "") } - @post.reload - end - end - end end end @@ -407,9 +339,10 @@ class UploadServiceTest < ActiveSupport::TestCase subject { UploadService::Replacer.new(post: @post, replacement: @replacement) } context "when replacing with its own source" do - should "work" do - as(@user) { @post.replace!(replacement_url: @post.source) } - assert_equal(@post_md5, @post.md5) + should "raise an error" do + assert_raises(UploadService::Replacer::Error) do + as(@user) { @post.replace!(replacement_url: @post.source) } + end end end @@ -480,12 +413,6 @@ class UploadServiceTest < ActiveSupport::TestCase end context "#process!" do - should "create a new upload" do - assert_difference(-> { Upload.count }) do - as(@user) { subject.process! } - end - end - should "create a comment" do assert_difference(-> { @post.comments.count }) do as(@user) { subject.process! } @@ -911,20 +838,6 @@ class UploadServiceTest < ActiveSupport::TestCase end end - context "for a pixiv ugoira" do - setup do - @upload = FactoryBot.create(:ugoira_upload, file_size: 1000, md5: "12345", file_ext: "jpg", image_width: 100, image_height: 100, context: UGOIRA_CONTEXT) - end - - should "create a post" do - assert_difference(-> { PixivUgoiraFrameData.count }) do - post = subject.new({}).create_post_from_upload(@upload) - assert_equal([], post.errors.full_messages) - assert_not_nil(post.id) - end - end - end - context "for nijie" do should "record the canonical source" do page_url = "https://nijie.info/view.php?id=728995"