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

@@ -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)

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)

app/logical/sources/strategies/base.rb

@@ -276,10 +276,6 @@ module Sources
         image_url
       end
 
-      def data
-        {}
-      end
-
       def tags
         (@tags || []).uniq
       end

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
+      def ugoira_frame_data
-        { ugoira_frame_data: api_ugoira[:frames] }
+        return nil unless is_ugoira?
+        api_ugoira[:frames]
       end
 
       def ugoira_content_type

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,

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]

app/logical/upload_service/replacer.rb

@@ -61,69 +61,44 @@ class UploadService
       undoer.process!
     end
 
-    def source_strategy(upload)
+    def replacement_url
-      Sources::Strategies.find(upload.source, upload.referer_url)
+      if replacement.replacement_file.present?
-    end
+        "file://#{replacement.replacement_file.original_filename}"
-
+      else
-    def find_replacement_url(repl, upload)
+        Sources::Strategies.find(replacement.replacement_url).canonical_url
-      if repl.replacement_file.present?
-        return "file://#{repl.replacement_file.original_filename}"
       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(
+      media_file = Utils::get_file_for_upload(replacement.replacement_url, nil, replacement.replacement_file&.tempfile)
-        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
 
-      replacement.replacement_url = find_replacement_url(replacement, upload)
+      if media_file.md5 == post.md5
-
+        raise Error, "Can't replace a post with itself; regenerate the post instead"
-      if md5_changed
+      elsif Post.exists?(md5: media_file.md5)
-        post.queue_delete_files(PostReplacement::DELETION_GRACE_PERIOD)
+        raise Error, "Duplicate: post with md5 #{media_file.md5} already exists"
       end
 
-      replacement.file_ext = upload.file_ext
+      media_asset = MediaAsset.upload!(media_file)
-      replacement.file_size = upload.file_size
+      post.queue_delete_files(PostReplacement::DELETION_GRACE_PERIOD)
-      replacement.image_height = upload.image_height
-      replacement.image_width = upload.image_width
-      replacement.md5 = upload.md5
 
-      post.md5 = upload.md5
+      replacement.replacement_url = replacement_url
-      post.file_ext = upload.file_ext
+      replacement.file_ext = media_asset.file_ext
-      post.image_width = upload.image_width
+      replacement.file_size = media_asset.file_size
-      post.image_height = upload.image_height
+      replacement.image_height = media_asset.image_height
-      post.file_size = upload.file_size
+      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") }
-        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
 
       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

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 = MediaFile.open(file)
-      media_file = upload.media_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)
+    def get_file_for_upload(source_url, referer_url, file)
-      return file if file.present?
+      return MediaFile.open(file) if file.present?
-      raise "No file or source URL provided" if upload.source_url.blank?
+      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!
+      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
     end
   end
 end

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!

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
 

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

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)

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

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');
 
 

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(2, media_file.frame_data.size)
-        assert_equal([{"file" => "000000.jpg", "delay" => 125}, {"file" => "000001.jpg", "delay" => 125}], @strategy.data[:ugoira_frame_data])
+        assert_equal([{"file" => "000000.jpg", "delay" => 125}, {"file" => "000001.jpg", "delay" => 125}], media_file.frame_data)
       end
     end
   end

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(2, media_file.frame_data.size)
-          assert_equal([{"file" => "000000.jpg", "delay" => 125}, {"file" => "000001.jpg", "delay" => 125}], ugoira_frame_data)
+          assert_equal([{"file" => "000000.jpg", "delay" => 125}, {"file" => "000001.jpg", "delay" => 125}], media_file.frame_data)
         end
       end
 

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
+        should "raise an error" do
-          as(@user) { @post.replace!(replacement_url: @post.source) }
+          assert_raises(UploadService::Replacer::Error) do
-          assert_equal(@post_md5, @post.md5)
+            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"