From 082544ab037ae29002e0da2f1fafb3e465a153dc Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 26 Oct 2021 19:11:37 -0500 Subject: [PATCH] StorageManager: remove Post-specific code. Refactor StorageManager to remove all image URL generation code. Instead the image URL generation code lives in MediaAsset. Now StorageManager is only concerned with how to read and write files to remote storage backends like S3 or SFTP, not with how image URLs should be generated. This way the file storage code isn't tightly coupled to posts, so it can be used to store any kind of file, not just images belonging to posts. --- app/logical/storage_manager.rb | 98 +------------------ app/models/media_asset.rb | 4 +- app/models/post.rb | 31 ++++-- lib/tasks/danbooru.rake | 3 +- .../post_regenerations_controller_test.rb | 4 +- test/unit/post_test.rb | 12 +-- test/unit/storage_manager_test.rb | 53 ---------- 7 files changed, 33 insertions(+), 172 deletions(-) diff --git a/app/logical/storage_manager.rb b/app/logical/storage_manager.rb index 2d46e6514..4970dec26 100644 --- a/app/logical/storage_manager.rb +++ b/app/logical/storage_manager.rb @@ -48,102 +48,8 @@ class StorageManager raise NotImplementedError, "open not implemented" end - # Store or replace the given file belonging to the given post. - # @param io [IO] the file to store - # @param post [Post] the post the image belongs to - # @param type [Symbol] the image variant to store (:preview, :crop, :large, :original) - def store_file(io, post, type) - store(io, file_path(post.md5, post.file_ext, type)) - end - - # Delete the file belonging to the given post. - # @param post_id [Integer] the post's id - # @param md5 [String] the post's md5 - # @param file_ext [String] the post's file extension - # @param type [Symbol] the image variant to delete (:preview, :crop, :large, :original) - def delete_file(post_id, md5, file_ext, type) - delete(file_path(md5, file_ext, type)) - end - - # Return a readonly copy of the image belonging to the given post. - # @param post [Post] the post - # @param type [Symbol] the image variant to open (:preview, :crop, :large, :original) - # @return [MediaFile] the image file - def open_file(post, type) - self.open(file_path(post.md5, post.file_ext, type)) - end - - # Generate the image URL for the given post. - # @param post [Post] the post - # @param type [Symbol] the post's image variant (:preview, :crop, :large, :original) - # @param tagged_filename [Boolean] whether the URL should contain the post's tags - # @return [String] the image URL - def file_url(post, type, tagged_filenames: false) - subdir = subdir_for(post.md5) - file = file_name(post.md5, post.file_ext, type) - seo_tags = seo_tags(post) if tagged_filenames - - if type == :preview && !post.has_preview? - "#{root_url}/images/download-preview.png" - elsif type == :preview - "#{base_url}/preview/#{subdir}#{file}" - elsif type == :crop - "#{base_url}/crop/#{subdir}#{file}" - elsif type == :large && post.has_large? - "#{base_url}/sample/#{subdir}#{seo_tags}#{file}" - else - "#{base_url}/original/#{subdir}#{seo_tags}#{post.md5}.#{post.file_ext}" - end - end - - def root_url - origin = Addressable::URI.parse(base_url).origin - origin = "" if origin == "null" # base_url was relative - origin - end - - def file_path(post_or_md5, file_ext, type) - md5 = post_or_md5.is_a?(String) ? post_or_md5 : post_or_md5.md5 - subdir = subdir_for(md5) - file = file_name(md5, file_ext, type) - - case type - when :preview - "/preview/#{subdir}#{file}" - when :crop - "/crop/#{subdir}#{file}" - when :large - "/sample/#{subdir}#{file}" - when :original - "/original/#{subdir}#{file}" - end - end - - def file_name(md5, file_ext, type) - large_file_ext = (file_ext == "zip") ? "webm" : "jpg" - - case type - when :preview - "#{md5}.jpg" - when :crop - "#{md5}.jpg" - when :large - "sample-#{md5}.#{large_file_ext}" - when :original - "#{md5}.#{file_ext}" - end - end - - def subdir_for(md5) - "#{md5[0..1]}/#{md5[2..3]}/" - end - - # Generate the tags in the image URL. - def seo_tags(post) - return "" if !tagged_filenames - - tags = post.presenter.humanized_essential_tag_string.gsub(/[^a-z0-9]+/, "_").gsub(/(?:^_+)|(?:_+$)/, "").gsub(/_{2,}/, "_") - "__#{tags}__" + def file_url(path) + File.join(base_url, path) end def full_path(path) diff --git a/app/models/media_asset.rb b/app/models/media_asset.rb index 9f037d1f2..fb2c70675 100644 --- a/app/models/media_asset.rb +++ b/app/models/media_asset.rb @@ -38,7 +38,9 @@ class MediaAsset < ApplicationRecord end def open_file - storage_service.open(file_path) + file = storage_service.open(file_path) + frame_data = media_asset.pixiv_ugoira_frame_data&.data if media_asset.is_ugoira? + MediaFile.open(file, frame_data: frame_data) end def convert_file(media_file) diff --git a/app/models/post.rb b/app/models/post.rb index 341070a28..2aceaa562 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -74,36 +74,47 @@ class Post < ApplicationRecord module FileMethods extend ActiveSupport::Concern - def storage_manager - Danbooru.config.storage_manager + def seo_tags + presenter.humanized_essential_tag_string.gsub(/[^a-z0-9]+/, "_").gsub(/(?:^_+)|(?:_+$)/, "").gsub(/_{2,}/, "_") end def file(type = :original) - storage_manager.open_file(self, type) + media_asset.variant(type).open_file end def tagged_file_url(tagged_filenames: !CurrentUser.user.disable_tagged_filenames?) - storage_manager.file_url(self, :original, tagged_filenames: tagged_filenames) + slug = seo_tags if tagged_filenames + media_asset.variant(:original).file_url(slug) end def tagged_large_file_url(tagged_filenames: !CurrentUser.user.disable_tagged_filenames?) - storage_manager.file_url(self, :large, tagged_filenames: tagged_filenames) + slug = seo_tags if tagged_filenames + + if media_asset.has_variant?(:sample) + media_asset.variant(:sample).file_url(slug) + else + media_asset.variant(:original).file_url(slug) + end end def file_url - storage_manager.file_url(self, :original) + media_asset.variant(:original).file_url end def large_file_url - storage_manager.file_url(self, :large) + if media_asset.has_variant?(:sample) + media_asset.variant(:sample).file_url + else + media_asset.variant(:original).file_url + end end def preview_file_url - storage_manager.file_url(self, :preview) + media_asset.variant(:preview).file_url end def crop_file_url - storage_manager.file_url(self, :crop) + media_asset.variant(:crop).file_url end def open_graph_image_url @@ -1162,7 +1173,7 @@ class Post < ApplicationRecord ModAction.log("<@#{user.name}> regenerated IQDB for post ##{id}", :post_regenerate_iqdb, user) else - media_file = MediaFile.open(file, frame_data: pixiv_ugoira_frame_data&.data.to_a) + media_file = media_asset.variant(:original).open_file media_asset.distribute_files!(media_file) update!( diff --git a/lib/tasks/danbooru.rake b/lib/tasks/danbooru.rake index e50998835..21665bb5b 100644 --- a/lib/tasks/danbooru.rake +++ b/lib/tasks/danbooru.rake @@ -70,8 +70,7 @@ namespace :danbooru do MediaMetadata.joins(:media_asset).where(metadata: {}).find_each do |metadata| asset = metadata.media_asset - file = sm.open(sm.file_path(asset.md5, asset.file_ext, :original)) - media_file = MediaFile.open(file) + media_file = asset.variant(:original).open_file metadata.update!(metadata: media_file.metadata) puts "metadata[id=#{metadata.id}, md5=#{asset.md5}]: #{media_file.metadata.count}" diff --git a/test/functional/post_regenerations_controller_test.rb b/test/functional/post_regenerations_controller_test.rb index 443fbe047..281472aba 100644 --- a/test/functional/post_regenerations_controller_test.rb +++ b/test/functional/post_regenerations_controller_test.rb @@ -40,8 +40,8 @@ class PostRegenerationsControllerTest < ActionDispatch::IntegrationTest context "for an image sample regeneration" do should "regenerate missing thumbnails" do - @preview_file_size = @post.file(:preview).size - @post.storage_manager.delete_file(@post.id, @post.md5, @post.file_ext, :preview) + @preview_file_size = @post.media_asset.variant(:preview).open_file.size + @post.media_asset.variant(:preview).delete_file! assert_raise(Errno::ENOENT) { @post.file(:preview) } post_auth post_regenerations_path, @mod, params: { post_id: @post.id } diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index b7a2d23fc..a4f0a6f53 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -1626,14 +1626,11 @@ class PostTest < ActiveSupport::TestCase context "URLs:" do should "generate the correct urls for animated gifs" do - manager = StorageManager::Local.new(base_url: "https://test.com/data", base_dir: "/") - Danbooru.config.stubs(:storage_manager).returns(manager) + @post = create(:post_with_file, filename: "test-animated-86x52.gif") - @post = build(:post, md5: "deadbeef", file_ext: "gif", tag_string: "animated_gif") - - assert_equal("https://test.com/data/preview/de/ad/deadbeef.jpg", @post.preview_file_url) - assert_equal("https://test.com/data/original/de/ad/deadbeef.gif", @post.large_file_url) - assert_equal("https://test.com/data/original/de/ad/deadbeef.gif", @post.file_url) + assert_equal("https://www.example.com/data/preview/77/d8/77d89bda37ea3af09158ed3282f8334f.jpg", @post.preview_file_url) + assert_equal("https://www.example.com/data/original/77/d8/77d89bda37ea3af09158ed3282f8334f.gif", @post.large_file_url) + assert_equal("https://www.example.com/data/original/77/d8/77d89bda37ea3af09158ed3282f8334f.gif", @post.file_url) end end @@ -1642,7 +1639,6 @@ class PostTest < ActiveSupport::TestCase setup do @post = FactoryBot.create(:post) - @post.stubs(:queue_delete_files) end should "update the post" do diff --git a/test/unit/storage_manager_test.rb b/test/unit/storage_manager_test.rb index 41437a8e6..1b8c6fd95 100644 --- a/test/unit/storage_manager_test.rb +++ b/test/unit/storage_manager_test.rb @@ -37,58 +37,5 @@ class StorageManagerTest < ActiveSupport::TestCase assert_nothing_raised { @storage_manager.delete("dne.txt") } end end - - context "#store_file and #delete_file methods" do - setup do - @post = FactoryBot.create(:post, file_ext: "png") - - @storage_manager.store_file(StringIO.new("data"), @post, :preview) - @storage_manager.store_file(StringIO.new("data"), @post, :large) - @storage_manager.store_file(StringIO.new("data"), @post, :original) - subdir = "#{@post.md5[0..1]}/#{@post.md5[2..3]}" - - @file_path = "#{@temp_dir}/preview/#{subdir}/#{@post.md5}.jpg" - @large_file_path = "#{@temp_dir}/sample/#{subdir}/sample-#{@post.md5}.jpg" - @preview_file_path = "#{@temp_dir}/original/#{subdir}/#{@post.md5}.#{@post.file_ext}" - end - - should "store the files at the correct path" do - assert(File.exist?(@file_path)) - assert(File.exist?(@large_file_path)) - assert(File.exist?(@preview_file_path)) - end - - should "delete the files" do - @storage_manager.delete_file(@post.id, @post.md5, @post.file_ext, :preview) - @storage_manager.delete_file(@post.id, @post.md5, @post.file_ext, :large) - @storage_manager.delete_file(@post.id, @post.md5, @post.file_ext, :original) - - assert_not(File.exist?(@file_path)) - assert_not(File.exist?(@large_file_path)) - assert_not(File.exist?(@preview_file_path)) - end - end - - context "#file_url method" do - should "return the correct urls" do - @post = FactoryBot.create(:post, file_ext: "png") - @storage_manager.stubs(:tagged_filenames).returns(false) - subdir = "#{@post.md5[0..1]}/#{@post.md5[2..3]}" - - assert_equal("/data/original/#{subdir}/#{@post.md5}.png", @storage_manager.file_url(@post, :original)) - assert_equal("/data/sample/#{subdir}/sample-#{@post.md5}.jpg", @storage_manager.file_url(@post, :large)) - assert_equal("/data/preview/#{subdir}/#{@post.md5}.jpg", @storage_manager.file_url(@post, :preview)) - end - - should "return the correct url for flash files" do - @post = FactoryBot.create(:post, file_ext: "swf") - - @storage_manager.stubs(:base_url).returns("/data") - assert_equal("/images/download-preview.png", @storage_manager.file_url(@post, :preview)) - - @storage_manager.stubs(:base_url).returns("http://localhost/data") - assert_equal("http://localhost/images/download-preview.png", @storage_manager.file_url(@post, :preview)) - end - end end end