From e0e35298d5459c2838a3921cfe0d31bb7f061351 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 22:14:45 +0200 Subject: [PATCH] Use Active Storage to handle cached attachments This fixes a few issues we've had for years. First, when attaching an image and then sending a form with validation errors, the image preview would not be rendered when the form was displayed once again. Now it's rendered as expected. Second, when attaching an image, removing it, and attaching a new one, browsers were displaying the image preview of the first one. That's because Paperclip generated the same URL from both files (as they both had the same hash data and prefix). Browsers usually cache images and render the cached image when getting the same URL. Since now we're storing each image in a different Blob, the images have different URLs and so the preview of the second one is correctly displayed. Finally, when users downloaded a document, they were getting files with a very long hexadecimal hash as filename. Now they get the original filename. --- app/controllers/direct_uploads_controller.rb | 4 +- app/models/concerns/attachable.rb | 42 +++++++++------- app/models/concerns/has_attachment.rb | 13 +++-- app/models/direct_upload.rb | 4 +- app/models/document.rb | 2 +- app/models/image.rb | 19 ++++--- db/dev_seeds/widgets.rb | 2 +- lib/tasks/files.rake | 16 ++---- spec/lib/tasks/active_storage_spec.rb | 14 +++--- spec/lib/tasks/files_spec.rb | 20 ++++---- spec/models/direct_upload_spec.rb | 11 ++-- spec/shared/system/nested_documentable.rb | 21 ++++---- spec/shared/system/nested_imageable.rb | 53 +++++++++++++++----- 13 files changed, 123 insertions(+), 98 deletions(-) diff --git a/app/controllers/direct_uploads_controller.rb b/app/controllers/direct_uploads_controller.rb index a1995c2e3..9bc565a29 100644 --- a/app/controllers/direct_uploads_controller.rb +++ b/app/controllers/direct_uploads_controller.rb @@ -15,9 +15,9 @@ class DirectUploadsController < ApplicationController @direct_upload.relation.set_cached_attachment_from_attachment render json: { cached_attachment: @direct_upload.relation.cached_attachment, - filename: @direct_upload.relation.attachment.original_filename, + filename: @direct_upload.relation.storage_attachment.filename.to_s, destroy_link: render_destroy_upload_link(@direct_upload), - attachment_url: @direct_upload.relation.attachment.url } + attachment_url: polymorphic_path(@direct_upload.relation.storage_attachment) } else render json: { errors: @direct_upload.errors[:attachment].join(", ") }, status: :unprocessable_entity diff --git a/app/models/concerns/attachable.rb b/app/models/concerns/attachable.rb index 9fb8c7a80..a8fda86ac 100644 --- a/app/models/concerns/attachable.rb +++ b/app/models/concerns/attachable.rb @@ -9,14 +9,10 @@ module Attachable # Paperclip do not allow to use Procs on valiations definition do_not_validate_attachment_file_type :attachment validate :attachment_presence - validate :validate_attachment_content_type, if: -> { attachment.present? } - validate :validate_attachment_size, if: -> { attachment.present? } + validate :validate_attachment_content_type, if: -> { storage_attachment.attached? } + validate :validate_attachment_size, if: -> { storage_attachment.attached? } - before_save :set_attachment_from_cached_attachment, if: -> { cached_attachment.present? } - - Paperclip.interpolates :prefix do |attachment, style| - attachment.instance.prefix(attachment, style) - end + before_validation :set_attachment_from_cached_attachment, if: -> { cached_attachment.present? } end def association_class @@ -26,29 +22,37 @@ module Attachable end def set_cached_attachment_from_attachment - self.cached_attachment = if filesystem_storage? - attachment.path - else - attachment.url - end + self.cached_attachment = storage_attachment.signed_id end def set_attachment_from_cached_attachment + self.storage_attachment = cached_attachment + if filesystem_storage? - File.open(cached_attachment) { |file| self.attachment = file } + File.open(file_path) do |file| + self.paperclip_attachment = file + end else - self.attachment = URI.parse(cached_attachment).open + self.paperclip_attachment = URI.parse(cached_attachment).open end end - def prefix(attachment, _style) - if attachment.instance.persisted? - ":attachment/:id_partition" + def attachment_content_type + storage_attachment.blob.content_type if storage_attachment.attached? + end + + def attachment_file_size + if storage_attachment.attached? + storage_attachment.blob.byte_size else - "cached_attachments/user/#{attachment.instance.user_id}" + 0 end end + def file_path + ActiveStorage::Blob.service.path_for(storage_attachment.blob.key) + end + private def filesystem_storage? @@ -73,7 +77,7 @@ module Attachable end def attachment_presence - if attachment.blank? && cached_attachment.blank? + unless storage_attachment.attached? errors.add(:attachment, I18n.t("errors.messages.blank")) end end diff --git a/app/models/concerns/has_attachment.rb b/app/models/concerns/has_attachment.rb index 6f3f67844..8fcafa3ec 100644 --- a/app/models/concerns/has_attachment.rb +++ b/app/models/concerns/has_attachment.rb @@ -5,18 +5,21 @@ module HasAttachment def has_attachment(attribute, paperclip_options = {}) has_one_attached :"storage_#{attribute}" - has_attached_file attribute, paperclip_options - alias_method :"paperclip_#{attribute}=", :"#{attribute}=" - - define_method :"#{attribute}=" do |file| - if file.is_a?(IO) || file.is_a?(Tempfile) && !file.is_a?(Ckeditor::Http::QqFile) + define_method :"storage_#{attribute}=" do |file| + if file.is_a?(IO) send(:"storage_#{attribute}").attach(io: file, filename: File.basename(file.path)) elsif file.nil? send(:"storage_#{attribute}").detach else send(:"storage_#{attribute}").attach(file) end + end + has_attached_file attribute, paperclip_options + alias_method :"paperclip_#{attribute}=", :"#{attribute}=" + + define_method :"#{attribute}=" do |file| + send(:"storage_#{attribute}=", file) send(:"paperclip_#{attribute}=", file) end end diff --git a/app/models/direct_upload.rb b/app/models/direct_upload.rb index 7e66375e7..66a9d45ab 100644 --- a/app/models/direct_upload.rb +++ b/app/models/direct_upload.rb @@ -34,7 +34,7 @@ class DirectUpload end def save_attachment - @relation.attachment.save + @relation.storage_attachment.blob.save! end def persisted? @@ -53,7 +53,7 @@ class DirectUpload def relation_attributtes { - paperclip_attachment: @attachment, + storage_attachment: @attachment, cached_attachment: @cached_attachment, user: @user } diff --git a/app/models/document.rb b/app/models/document.rb index a121e3bbd..4b9349f3e 100644 --- a/app/models/document.rb +++ b/app/models/document.rb @@ -1,7 +1,7 @@ class Document < ApplicationRecord include Attachable - has_attachment :attachment, url: "/system/:class/:prefix/:style/:hash.:extension", + has_attachment :attachment, url: "/system/:class/:attachment/:id_partition/:style/:hash.:extension", hash_data: ":class/:style/:custom_hash_data", use_timestamp: false, hash_secret: Rails.application.secrets.secret_key_base diff --git a/app/models/image.rb b/app/models/image.rb index a86091648..098515114 100644 --- a/app/models/image.rb +++ b/app/models/image.rb @@ -6,7 +6,7 @@ class Image < ApplicationRecord medium: "300x300#", thumb: "140x245#" }, - url: "/system/:class/:prefix/:style/:hash.:extension", + url: "/system/:class/:attachment/:id_partition/:style/:hash.:extension", hash_data: ":class/:style", use_timestamp: false, hash_secret: Rails.application.secrets.secret_key_base @@ -27,7 +27,7 @@ class Image < ApplicationRecord validates :user_id, presence: true validates :imageable_id, presence: true, if: -> { persisted? } validates :imageable_type, presence: true, if: -> { persisted? } - validate :validate_image_dimensions, if: -> { attachment.present? && attachment.dirty? } + validate :validate_image_dimensions, if: -> { storage_attachment.attached? && storage_attachment.new_record? } def self.max_file_size Setting["uploads.images.max_size"].to_i @@ -68,14 +68,17 @@ class Image < ApplicationRecord end def validate_image_dimensions - if attachment_of_valid_content_type? + if accepted_content_types.include?(attachment_content_type) return true if imageable_class == Widget::Card - dimensions = Paperclip::Geometry.from_file(attachment.queued_for_write[:original].path) + storage_attachment.analyze unless storage_attachment.analyzed? + + width = storage_attachment.metadata[:width] + height = storage_attachment.metadata[:height] min_width = Setting["uploads.images.min_width"].to_i min_height = Setting["uploads.images.min_height"].to_i - errors.add(:attachment, :min_image_width, required_min_width: min_width) if dimensions.width < min_width - errors.add(:attachment, :min_image_height, required_min_height: min_height) if dimensions.height < min_height + errors.add(:attachment, :min_image_width, required_min_width: min_width) if width < min_width + errors.add(:attachment, :min_image_height, required_min_height: min_height) if height < min_height end end @@ -93,8 +96,4 @@ class Image < ApplicationRecord end end end - - def attachment_of_valid_content_type? - attachment.present? && accepted_content_types.include?(attachment_content_type) - end end diff --git a/db/dev_seeds/widgets.rb b/db/dev_seeds/widgets.rb index b672d28d5..41711c937 100644 --- a/db/dev_seeds/widgets.rb +++ b/db/dev_seeds/widgets.rb @@ -1,7 +1,7 @@ section "Creating header and cards for the homepage" do def create_image_attachment(type) { - cached_attachment: Rails.root.join("db/dev_seeds/images/#{type}_background.jpg"), + attachment: File.new(Rails.root.join("db/dev_seeds/images/#{type}_background.jpg")), title: "#{type}_background.jpg", user: User.first } diff --git a/lib/tasks/files.rake b/lib/tasks/files.rake index ef46097cc..978b5f4f1 100644 --- a/lib/tasks/files.rake +++ b/lib/tasks/files.rake @@ -1,18 +1,8 @@ namespace :files do desc "Removes cached attachments which weren't deleted for some reason" task remove_old_cached_attachments: :environment do - paths = Dir.glob(Rails.root.join("public/system/*/cached_attachments/**/*")) - logger = ApplicationLogger.new - - paths.each do |path| - if File.file?(path) - if File.mtime(path) < 1.day.ago - File.delete(path) - logger.info "The file #{path} has been removed" - end - else - Dir.delete(path) if Dir.empty?(path) - end - end + ActiveStorage::Blob.unattached + .where("active_storage_blobs.created_at <= ?", 1.day.ago) + .find_each(&:purge_later) end end diff --git a/spec/lib/tasks/active_storage_spec.rb b/spec/lib/tasks/active_storage_spec.rb index f554d05de..2a8043556 100644 --- a/spec/lib/tasks/active_storage_spec.rb +++ b/spec/lib/tasks/active_storage_spec.rb @@ -11,9 +11,10 @@ describe "active storage tasks" do before { FileUtils.rm_rf storage_root } it "migrates records and attachments" do - document = create(:document, - attachment: nil, - paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) + document = build(:document, + attachment: nil, + paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) + document.save(validate: false) expect(ActiveStorage::Attachment.count).to eq 0 expect(ActiveStorage::Blob.count).to eq 0 @@ -30,9 +31,10 @@ describe "active storage tasks" do end it "migrates records with deleted files ignoring the files" do - document = create(:document, - attachment: nil, - paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) + document = build(:document, + attachment: nil, + paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) + document.save(validate: false) FileUtils.rm(document.attachment.path) run_rake_task diff --git a/spec/lib/tasks/files_spec.rb b/spec/lib/tasks/files_spec.rb index 2c464bc82..454abe7be 100644 --- a/spec/lib/tasks/files_spec.rb +++ b/spec/lib/tasks/files_spec.rb @@ -11,26 +11,26 @@ describe "files tasks" do image = build(:image) document = build(:document) - image.attachment.save - document.attachment.save + image.storage_attachment.blob.save! + document.storage_attachment.blob.save! travel_to(2.days.from_now) { run_rake_task } - expect(File.exists?(image.attachment.path)).to be false - expect(File.exists?(document.attachment.path)).to be false + expect(File.exists?(image.file_path)).to be false + expect(File.exists?(document.file_path)).to be false end it "does not delete recent cached attachments" do image = build(:image) document = build(:document) - image.attachment.save - document.attachment.save + image.storage_attachment.blob.save! + document.storage_attachment.blob.save! travel_to(2.minutes.from_now) { run_rake_task } - expect(File.exists?(image.attachment.path)).to be true - expect(File.exists?(document.attachment.path)).to be true + expect(File.exists?(image.file_path)).to be true + expect(File.exists?(document.file_path)).to be true end it "does not delete old regular attachments" do @@ -39,8 +39,8 @@ describe "files tasks" do travel_to(2.days.from_now) { run_rake_task } - expect(File.exists?(image.attachment.path)).to be true - expect(File.exists?(document.attachment.path)).to be true + expect(File.exists?(image.file_path)).to be true + expect(File.exists?(document.file_path)).to be true end end end diff --git a/spec/models/direct_upload_spec.rb b/spec/models/direct_upload_spec.rb index 9c5657d55..7f281cc22 100644 --- a/spec/models/direct_upload_spec.rb +++ b/spec/models/direct_upload_spec.rb @@ -34,13 +34,14 @@ describe DirectUpload do end context "save_attachment" do - it "saves uploaded file" do - proposal_document_direct_upload = build(:direct_upload, :proposal, :documents) + it "saves uploaded file without creating an attachment record" do + direct_upload = build(:direct_upload, :proposal, :documents) - proposal_document_direct_upload.save_attachment + direct_upload.save_attachment - expect(File.exist?(proposal_document_direct_upload.relation.attachment.path)).to eq(true) - expect(proposal_document_direct_upload.relation.attachment.path).to include("cached_attachments") + expect(File.exist?(direct_upload.relation.file_path)).to be true + expect(direct_upload.relation.storage_attachment.blob).to be_persisted + expect(direct_upload.relation.storage_attachment.attachment).not_to be_persisted end end end diff --git a/spec/shared/system/nested_documentable.rb b/spec/shared/system/nested_documentable.rb index da0a72b0a..3e5d03f96 100644 --- a/spec/shared/system/nested_documentable.rb +++ b/spec/shared/system/nested_documentable.rb @@ -137,9 +137,15 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na do_login_for user_to_login visit send(path, arguments) - documentable_attach_new_file(Rails.root.join("spec/fixtures/files/empty.pdf")) + click_link "Add new document" - expect_document_has_cached_attachment(0, ".pdf") + cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) + expect(cached_attachment_field.value).to be_empty + + attach_file "Choose document", Rails.root.join("spec/fixtures/files/empty.pdf") + + expect(page).to have_css(".loading-bar.complete") + expect(cached_attachment_field.value).not_to be_empty end scenario "Should not update document cached_attachment field after invalid file upload" do @@ -151,7 +157,8 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na false ) - expect_document_has_cached_attachment(0, "") + cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) + expect(cached_attachment_field.value).to be_empty end scenario "Should show document errors after documentable submit with @@ -350,14 +357,6 @@ def expect_document_has_title(index, title) end end -def expect_document_has_cached_attachment(index, extension) - document = all(".document")[index] - - within document do - expect(find("input[name$='[cached_attachment]']", visible: :hidden).value).to end_with(extension) - end -end - def documentable_fill_new_valid_proposal fill_in_new_proposal_title with: "Proposal title #{rand(9999)}" fill_in "Proposal summary", with: "Proposal summary" diff --git a/spec/shared/system/nested_imageable.rb b/spec/shared/system/nested_imageable.rb index 57c2329ec..f6fe1b723 100644 --- a/spec/shared/system/nested_imageable.rb +++ b/spec/shared/system/nested_imageable.rb @@ -90,9 +90,15 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + click_link "Add image" - expect_image_has_cached_attachment(".jpg") + cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) + expect(cached_attachment_field.value).to be_empty + + attach_file "Choose image", Rails.root.join("spec/fixtures/files/clippy.jpg") + + expect(page).to have_css(".loading-bar.complete") + expect(cached_attachment_field.value).not_to be_empty end scenario "Should not update image cached_attachment field after invalid file upload" do @@ -101,7 +107,9 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p imageable_attach_new_file(Rails.root.join("spec/fixtures/files/logo_header.png"), false) - expect_image_has_cached_attachment("") + cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) + + expect(cached_attachment_field.value).to be_empty end scenario "Should show nested image errors after invalid form submit" do @@ -120,6 +128,19 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p end end + scenario "Render image preview after sending the form with validation errors" do + skip "Question answers behave differently" if imageable.is_a?(Poll::Question::Answer) + do_login_for user + visit send(path, arguments) + + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + within_fieldset("Descriptive image") { fill_in "Title", with: "" } + click_on submit_button + + expect(page).to have_content "can't be blank" + expect(page).to have_css "img[src$='clippy.jpg']" + end + scenario "Should remove nested image after valid file upload and click on remove button" do do_login_for user visit send(path, arguments) @@ -180,6 +201,22 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p end end + scenario "Different URLs for different images" do + do_login_for user + visit send(path, arguments) + + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + + original_src = find(:fieldset, "Descriptive image").find("img")[:src] + + click_link "Remove image" + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/custom_map.jpg")) + + updated_src = find(:fieldset, "Descriptive image").find("img")[:src] + + expect(updated_src).not_to eq original_src + end + if path.include? "edit" scenario "show persisted image" do create(:image, imageable: imageable) @@ -268,16 +305,6 @@ def expect_image_has_title(title) end end -def expect_image_has_cached_attachment(extension) - within "#nested-image" do - image = find(".image") - - within image do - expect(find("input[name$='[cached_attachment]']", visible: :hidden).value).to end_with(extension) - end - end -end - def show_caption_for?(imageable_factory_name) imageable_factory_name != "budget" end