From 886006d4976d94d58fbc19d2a66ec18297be38ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 25 Jul 2021 22:15:59 +0200 Subject: [PATCH 01/18] Enable ActiveStorage This reverts commit 7496c1bcb9. --- config/application.rb | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/config/application.rb b/config/application.rb index 650c09369..cd42eaf90 100644 --- a/config/application.rb +++ b/config/application.rb @@ -1,17 +1,6 @@ require_relative "boot" -require "rails" -# Pick the frameworks you want: -require "active_model/railtie" -require "active_job/railtie" -require "active_record/railtie" -# require "active_storage/engine" -require "action_controller/railtie" -require "action_mailer/railtie" -require "action_view/railtie" -require "action_cable/engine" -require "sprockets/railtie" -require "rails/test_unit/railtie" +require "rails/all" # Require the gems listed in Gemfile, including any gems # you've limited to :test, :development, or :production. From 66ef345a2da074212ecf3db47d9cf20c53c702a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 25 Jul 2021 22:20:31 +0200 Subject: [PATCH 02/18] Create ActiveStorage tables This migration was generated using the `active_storage:install` task. --- ...te_active_storage_tables.active_storage.rb | 27 +++++++++++++++++++ db/schema.rb | 22 +++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 db/migrate/20210619201902_create_active_storage_tables.active_storage.rb diff --git a/db/migrate/20210619201902_create_active_storage_tables.active_storage.rb b/db/migrate/20210619201902_create_active_storage_tables.active_storage.rb new file mode 100644 index 000000000..ce71f5783 --- /dev/null +++ b/db/migrate/20210619201902_create_active_storage_tables.active_storage.rb @@ -0,0 +1,27 @@ +# This migration comes from active_storage (originally 20170806125915) +class CreateActiveStorageTables < ActiveRecord::Migration[5.2] + def change + create_table :active_storage_blobs do |t| + t.string :key, null: false + t.string :filename, null: false + t.string :content_type + t.text :metadata + t.bigint :byte_size, null: false + t.string :checksum, null: false + t.datetime :created_at, null: false + + t.index [:key], unique: true + end + + create_table :active_storage_attachments do |t| + t.string :name, null: false + t.references :record, null: false, polymorphic: true, index: false + t.references :blob, null: false + + t.datetime :created_at, null: false + + t.index [:record_type, :record_id, :name, :blob_id], name: "index_active_storage_attachments_uniqueness", unique: true + t.foreign_key :active_storage_blobs, column: :blob_id + end + end +end diff --git a/db/schema.rb b/db/schema.rb index ab95ffdfc..0537d3c8f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -32,6 +32,27 @@ ActiveRecord::Schema.define(version: 2021_08_11_195800) do t.datetime "updated_at", null: false end + create_table "active_storage_attachments", force: :cascade do |t| + t.string "name", null: false + t.string "record_type", null: false + t.bigint "record_id", null: false + t.bigint "blob_id", null: false + t.datetime "created_at", null: false + t.index ["blob_id"], name: "index_active_storage_attachments_on_blob_id" + t.index ["record_type", "record_id", "name", "blob_id"], name: "index_active_storage_attachments_uniqueness", unique: true + end + + create_table "active_storage_blobs", force: :cascade do |t| + t.string "key", null: false + t.string "filename", null: false + t.string "content_type" + t.text "metadata" + t.bigint "byte_size", null: false + t.string "checksum", null: false + t.datetime "created_at", null: false + t.index ["key"], name: "index_active_storage_blobs_on_key", unique: true + end + create_table "activities", id: :serial, force: :cascade do |t| t.integer "user_id" t.string "action" @@ -1726,6 +1747,7 @@ ActiveRecord::Schema.define(version: 2021_08_11_195800) do t.datetime "updated_at", null: false end + add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id" add_foreign_key "administrators", "users" add_foreign_key "budget_administrators", "administrators" add_foreign_key "budget_administrators", "budgets" From 73a0a210013cd13310ca05ba371155ee2b6aa93e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 25 Jul 2021 22:24:29 +0200 Subject: [PATCH 03/18] Configure Active Storage --- .gitignore | 1 + config/application.rb | 3 +++ config/deploy.rb | 2 +- config/environments/test.rb | 3 +++ config/storage.yml | 7 +++++++ 5 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 config/storage.yml diff --git a/.gitignore b/.gitignore index d76769206..f973f1158 100644 --- a/.gitignore +++ b/.gitignore @@ -40,3 +40,4 @@ public/assets/ public/machine_learning/data/ public/system/ /public/ckeditor_assets/ +storage/ diff --git a/config/application.rb b/config/application.rb index cd42eaf90..69a422e14 100644 --- a/config/application.rb +++ b/config/application.rb @@ -24,6 +24,9 @@ module Consul # Handle custom exceptions config.action_dispatch.rescue_responses["FeatureFlags::FeatureDisabled"] = :forbidden + # Store files locally. + config.active_storage.service = :local + # Set Time.zone default to the specified zone and make Active Record auto-convert to this zone. # Run "rake -D time" for a list of tasks for finding time zone names. Default is UTC. # config.time_zone = 'Central Time (US & Canada)' diff --git a/config/deploy.rb b/config/deploy.rb index 07288e85c..4e1d3a515 100644 --- a/config/deploy.rb +++ b/config/deploy.rb @@ -22,7 +22,7 @@ set :pty, true set :use_sudo, false set :linked_files, %w[config/database.yml config/secrets.yml] -set :linked_dirs, %w[.bundle log tmp public/system public/assets public/ckeditor_assets public/machine_learning/data] +set :linked_dirs, %w[.bundle log tmp public/system public/assets public/ckeditor_assets public/machine_learning/data storage] set :keep_releases, 5 diff --git a/config/environments/test.rb b/config/environments/test.rb index 14aab67dd..23ac6efa3 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -47,6 +47,9 @@ Rails.application.configure do # Raises error for missing translations # config.action_view.raise_on_missing_translations = true + # Store files in tmp folders. + config.active_storage.service = :test + config.cache_store = :null_store config.after_initialize do diff --git a/config/storage.yml b/config/storage.yml new file mode 100644 index 000000000..1f93f7323 --- /dev/null +++ b/config/storage.yml @@ -0,0 +1,7 @@ +local: + service: Disk + root: <%= Rails.root.join("storage") %> + +test: + service: Disk + root: <%= Rails.root.join("tmp/storage") %> From f412ab2c9a624ff7fc2da6f87af57f93877c19e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 26 Jul 2021 01:07:40 +0200 Subject: [PATCH 04/18] Add CKEditor support for ActiveStorage ActiveStorage support was added to CKEditor in version 5.1.0. However, we can't upgrade to version 5.0.0 or later because it only supports loading CKEditor via CDN. So we're copying the code related to ActiveStorage instead. I tried to move it to the `vendor/` folder, but couldn't find a way to load it from there and if I found one I wouldn't be sure it'd work on production environments. --- .rubocop.yml | 1 + lib/ckeditor/backend/active_storage.rb | 73 ++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 lib/ckeditor/backend/active_storage.rb diff --git a/.rubocop.yml b/.rubocop.yml index 0f92631ba..483b38bbe 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -8,6 +8,7 @@ AllCops: DisplayStyleGuide: true Exclude: - "db/schema.rb" + - "lib/ckeditor/backend/active_storage.rb" DisabledByDefault: true Bundler/DuplicatedGem: diff --git a/lib/ckeditor/backend/active_storage.rb b/lib/ckeditor/backend/active_storage.rb new file mode 100644 index 000000000..710db0aed --- /dev/null +++ b/lib/ckeditor/backend/active_storage.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +# Code copied from the ckeditor gem: +# https://github.com/galetahub/ckeditor/pull/853 +module Ckeditor + module Backend + module ActiveStorage + def self.included(base) + base.send(:include, Rails.application.routes.url_helpers) + base.send(:include, InstanceMethods) + base.send(:extend, ClassMethods) + end + + module ClassMethods + def self.extended(base) + base.class_eval do + before_save :apply_data + validate do + if data.nil? || file.nil? + errors.add(:data, :not_data_present, message: "data must be present") + end + end + end + end + end + + module InstanceMethods + def url + rails_blob_path(self.storage_data, only_path: true) + end + + def path + rails_blob_path(self.storage_data, only_path: true) + end + + def styles + end + + def content_type + self.storage_data.content_type + end + + def content_type=(_content_type) + self.storage_data.content_type = _content_type + end + + protected + + def file + @file ||= storage_data + end + + def blob + @blob ||= ::ActiveStorage::Blob.find(file.attachment.blob_id) + end + + def apply_data + if data.is_a?(Ckeditor::Http::QqFile) + storage_data.attach(io: data, filename: data.original_filename) + else + storage_data.attach(data) + end + + self.data_file_name = storage_data.blob.filename + self.data_content_type = storage_data.blob.content_type + self.data_file_size = storage_data.blob.byte_size + end + end + end + + autoload :ActiveStorage, "ckeditor/backend/active_storage" + end +end From 1290e2ecd36470517ee72f80d37a0531ca99de04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 26 Jul 2021 01:21:00 +0200 Subject: [PATCH 05/18] Store files with both Paperclip and ActiveStorage In order to migrate existing files from Paperclip to ActiveStorage, we need Paperclip to find out the files associated to existing database records. So we can't simply replace Paperclip with ActiveStorage. That's why it's usually recommended [1] to first run the migration and then replace Paperclip with ActiveStorage using two consecutive deployments. However, in our case we can't rely on two consecutive deployments because we have to make an easy process so existing CONSUL installations don't run into any issues. We can't just release version 1.4.0 and 1.5.0 and day and ask everyone to upgrade twice on the same day. Instead, we're following a different plan: * We're going to provide a Rake task (which will require Paperclip) to migrate existing files * We still use Paperclip to generate link and image tags * New files are handled using both Paperclip and ActiveStorage; that way, when we make the switch, we won't have to migrate them, and in the meantime they'll be accessible thanks to Paperclip * After we make the switch, we'll update the `name` column in the active storage attachments tables in order to remove the `storage_` prefix Regarding our handling of new files, the exception are cached attachments. Since those attachments are temporary files used while submitting a form and we have to delete them afterwards, we're only handling them with Paperclip. We'll handle these ones in version 1.5.0. Note the task creating the dev seeds was failing after these changes with an `ActiveStorage::IntegrityError` exception because we were opening some files without closing them. If the same file was attached twice, it failed the second time. We're solving it by closing the files with `File.open` and a block. Even though we didn't get any errors, we're doing the same thing in the `Attachable` concern because it's a good practice to close files after we're done with them. Also note we have to change the CKEditor Active Storage code so it's compatible with Paperclip. In this case, I haven't been able to write a test to confirm the attachment exists; I was getting the same `ActiveStorage::IntegrityError` mentioned above. Finally, we're updating the site customization image controller to use `update` so the image and the attachment are updated within the same transaction. This is also what we do in most controllers. [1] https://www.youtube.com/watch?v=tZ_WNUytO9o --- .../site_customization/images_controller.rb | 3 +-- app/models/ckeditor/asset.rb | 1 + app/models/ckeditor/picture.rb | 10 ++++---- app/models/concerns/attachable.rb | 11 +++++---- app/models/concerns/has_attachment.rb | 24 +++++++++++++++++++ app/models/direct_upload.rb | 2 +- app/models/document.rb | 8 +++---- app/models/image.rb | 18 +++++++------- app/models/site_customization/image.rb | 4 +++- db/dev_seeds/budgets.rb | 24 ++++++++++--------- db/dev_seeds/proposals.rb | 22 +++++++++-------- lib/ckeditor/backend/active_storage.rb | 20 ++++++++++------ spec/models/document_spec.rb | 10 ++++++++ spec/models/image_spec.rb | 10 ++++++++ spec/models/site_customization/image_spec.rb | 11 +++++++++ 15 files changed, 124 insertions(+), 54 deletions(-) create mode 100644 app/models/concerns/has_attachment.rb diff --git a/app/controllers/admin/site_customization/images_controller.rb b/app/controllers/admin/site_customization/images_controller.rb index 3280c690d..da0a53412 100644 --- a/app/controllers/admin/site_customization/images_controller.rb +++ b/app/controllers/admin/site_customization/images_controller.rb @@ -26,8 +26,7 @@ class Admin::SiteCustomization::ImagesController < Admin::SiteCustomization::Bas end def destroy - @image.image = nil - if @image.save + if @image.update(image: nil) notice = t("admin.site_customization.images.destroy.notice") else notice = t("admin.site_customization.images.destroy.error") diff --git a/app/models/ckeditor/asset.rb b/app/models/ckeditor/asset.rb index 5c166fdfc..494d4fc76 100644 --- a/app/models/ckeditor/asset.rb +++ b/app/models/ckeditor/asset.rb @@ -1,4 +1,5 @@ class Ckeditor::Asset < ApplicationRecord include Ckeditor::Orm::ActiveRecord::AssetBase + include Ckeditor::Backend::ActiveStorage include Ckeditor::Backend::Paperclip end diff --git a/app/models/ckeditor/picture.rb b/app/models/ckeditor/picture.rb index 4a579b9d2..bcce7ee99 100644 --- a/app/models/ckeditor/picture.rb +++ b/app/models/ckeditor/picture.rb @@ -1,8 +1,10 @@ class Ckeditor::Picture < Ckeditor::Asset - has_attached_file :data, - url: "/ckeditor_assets/pictures/:id/:style_:basename.:extension", - path: ":rails_root/public/ckeditor_assets/pictures/:id/:style_:basename.:extension", - styles: { content: "800>", thumb: "118x100#" } + include HasAttachment + + has_attachment :data, + url: "/ckeditor_assets/pictures/:id/:style_:basename.:extension", + path: ":rails_root/public/ckeditor_assets/pictures/:id/:style_:basename.:extension", + styles: { content: "800>", thumb: "118x100#" } validates_attachment_presence :data validates_attachment_size :data, less_than: 2.megabytes diff --git a/app/models/concerns/attachable.rb b/app/models/concerns/attachable.rb index fbc29a055..fb13dc517 100644 --- a/app/models/concerns/attachable.rb +++ b/app/models/concerns/attachable.rb @@ -1,4 +1,5 @@ module Attachable + include HasAttachment extend ActiveSupport::Concern included do @@ -33,11 +34,11 @@ module Attachable end def set_attachment_from_cached_attachment - self.attachment = if Paperclip::Attachment.default_options[:storage] == :filesystem - File.open(cached_attachment) - else - URI.parse(cached_attachment) - end + if Paperclip::Attachment.default_options[:storage] == :filesystem + File.open(cached_attachment) { |file| self.attachment = file } + else + self.attachment = URI.parse(cached_attachment) + end end def prefix(attachment, _style) diff --git a/app/models/concerns/has_attachment.rb b/app/models/concerns/has_attachment.rb new file mode 100644 index 000000000..450e515ea --- /dev/null +++ b/app/models/concerns/has_attachment.rb @@ -0,0 +1,24 @@ +module HasAttachment + extend ActiveSupport::Concern + + class_methods do + 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) + 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 + + send(:"paperclip_#{attribute}=", file) + end + end + end +end diff --git a/app/models/direct_upload.rb b/app/models/direct_upload.rb index 2b2760011..7e66375e7 100644 --- a/app/models/direct_upload.rb +++ b/app/models/direct_upload.rb @@ -53,7 +53,7 @@ class DirectUpload def relation_attributtes { - attachment: @attachment, + paperclip_attachment: @attachment, cached_attachment: @cached_attachment, user: @user } diff --git a/app/models/document.rb b/app/models/document.rb index 49ad4be9c..a121e3bbd 100644 --- a/app/models/document.rb +++ b/app/models/document.rb @@ -1,10 +1,10 @@ class Document < ApplicationRecord include Attachable - has_attached_file :attachment, url: "/system/:class/:prefix/:style/:hash.:extension", - hash_data: ":class/:style/:custom_hash_data", - use_timestamp: false, - hash_secret: Rails.application.secrets.secret_key_base + has_attachment :attachment, url: "/system/:class/:prefix/:style/:hash.:extension", + hash_data: ":class/:style/:custom_hash_data", + use_timestamp: false, + hash_secret: Rails.application.secrets.secret_key_base belongs_to :user belongs_to :documentable, polymorphic: true, touch: true diff --git a/app/models/image.rb b/app/models/image.rb index 0449042d8..de1405f2b 100644 --- a/app/models/image.rb +++ b/app/models/image.rb @@ -1,15 +1,15 @@ class Image < ApplicationRecord include Attachable - has_attached_file :attachment, styles: { - large: "x#{Setting["uploads.images.min_height"]}", - medium: "300x300#", - thumb: "140x245#" - }, - url: "/system/:class/:prefix/:style/:hash.:extension", - hash_data: ":class/:style", - use_timestamp: false, - hash_secret: Rails.application.secrets.secret_key_base + has_attachment :attachment, styles: { + large: "x#{Setting["uploads.images.min_height"]}", + medium: "300x300#", + thumb: "140x245#" + }, + url: "/system/:class/:prefix/:style/:hash.:extension", + hash_data: ":class/:style", + use_timestamp: false, + hash_secret: Rails.application.secrets.secret_key_base belongs_to :user belongs_to :imageable, polymorphic: true, touch: true diff --git a/app/models/site_customization/image.rb b/app/models/site_customization/image.rb index fc6bcfd71..592ea3b47 100644 --- a/app/models/site_customization/image.rb +++ b/app/models/site_customization/image.rb @@ -1,4 +1,6 @@ class SiteCustomization::Image < ApplicationRecord + include HasAttachment + VALID_IMAGES = { "logo_header" => [260, 80], "social_media_icon" => [470, 246], @@ -9,7 +11,7 @@ class SiteCustomization::Image < ApplicationRecord "logo_email" => [400, 80] }.freeze - has_attached_file :image + has_attachment :image validates :name, presence: true, uniqueness: true, inclusion: { in: VALID_IMAGES.keys } validates_attachment_content_type :image, content_type: ["image/png", "image/jpeg"] diff --git a/db/dev_seeds/budgets.rb b/db/dev_seeds/budgets.rb index 42da7c03c..dcb18be57 100644 --- a/db/dev_seeds/budgets.rb +++ b/db/dev_seeds/budgets.rb @@ -6,21 +6,23 @@ INVESTMENT_IMAGE_FILES = %w[ olesya-grichina-218176-unsplash_713x475.jpg sole-d-alessandro-340443-unsplash_713x475.jpg ].map do |filename| - File.new(Rails.root.join("db", - "dev_seeds", - "images", - "budget", - "investments", filename)) + Rails.root.join("db", + "dev_seeds", + "images", + "budget", + "investments", filename) end def add_image_to(imageable) # imageable should respond to #title & #author - imageable.image = Image.create!({ - imageable: imageable, - title: imageable.title, - attachment: INVESTMENT_IMAGE_FILES.sample, - user: imageable.author - }) + File.open(INVESTMENT_IMAGE_FILES.sample) do |file| + imageable.image = Image.create!({ + imageable: imageable, + title: imageable.title, + attachment: file, + user: imageable.author + }) + end imageable.save! end diff --git a/db/dev_seeds/proposals.rb b/db/dev_seeds/proposals.rb index 8341fd58b..756dd00b3 100644 --- a/db/dev_seeds/proposals.rb +++ b/db/dev_seeds/proposals.rb @@ -4,20 +4,22 @@ IMAGE_FILES = %w[ steve-harvey-597760-unsplash_713x475.jpg tim-mossholder-302931-unsplash_713x475.jpg ].map do |filename| - File.new(Rails.root.join("db", - "dev_seeds", - "images", - "proposals", filename)) + Rails.root.join("db", + "dev_seeds", + "images", + "proposals", filename) end def add_image_to(imageable) # imageable should respond to #title & #author - imageable.image = Image.create!({ - imageable: imageable, - title: imageable.title, - attachment: IMAGE_FILES.sample, - user: imageable.author - }) + File.open(IMAGE_FILES.sample) do |file| + imageable.image = Image.create!({ + imageable: imageable, + title: imageable.title, + attachment: file, + user: imageable.author + }) + end imageable.save! end diff --git a/lib/ckeditor/backend/active_storage.rb b/lib/ckeditor/backend/active_storage.rb index 710db0aed..7ce8aa611 100644 --- a/lib/ckeditor/backend/active_storage.rb +++ b/lib/ckeditor/backend/active_storage.rb @@ -16,7 +16,7 @@ module Ckeditor base.class_eval do before_save :apply_data validate do - if data.nil? || file.nil? + if data.nil? || storage_file.nil? errors.add(:data, :not_data_present, message: "data must be present") end end @@ -46,19 +46,25 @@ module Ckeditor protected - def file - @file ||= storage_data + def storage_file + @storage_file ||= storage_data end def blob - @blob ||= ::ActiveStorage::Blob.find(file.attachment.blob_id) + @blob ||= ::ActiveStorage::Blob.find(storage_file.attachment.blob_id) end def apply_data - if data.is_a?(Ckeditor::Http::QqFile) - storage_data.attach(io: data, filename: data.original_filename) + non_paperclip_data = if data.is_a?(::Paperclip::Attachment) + file.instance_variable_get("@target") + else + data + end + + if non_paperclip_data.is_a?(Ckeditor::Http::QqFile) + storage_data.attach(io: non_paperclip_data, filename: non_paperclip_data.original_filename) else - storage_data.attach(data) + storage_data.attach(non_paperclip_data) end self.data_file_name = storage_data.blob.filename diff --git a/spec/models/document_spec.rb b/spec/models/document_spec.rb index 59a27e036..2f8cf65fe 100644 --- a/spec/models/document_spec.rb +++ b/spec/models/document_spec.rb @@ -4,6 +4,16 @@ describe Document do it_behaves_like "document validations", "budget_investment_document" it_behaves_like "document validations", "proposal_document" + it "stores attachments with both Paperclip and Active Storage" do + document = create(:document, attachment: File.new("spec/fixtures/files/clippy.pdf")) + + expect(document.attachment).to exist + expect(document.attachment_file_name).to eq "clippy.pdf" + + expect(document.storage_attachment).to be_attached + expect(document.storage_attachment.filename).to eq "clippy.pdf" + end + context "scopes" do describe "#admin" do it "returns admin documents" do diff --git a/spec/models/image_spec.rb b/spec/models/image_spec.rb index 5f1725521..d119c8138 100644 --- a/spec/models/image_spec.rb +++ b/spec/models/image_spec.rb @@ -3,4 +3,14 @@ require "rails_helper" describe Image do it_behaves_like "image validations", "budget_investment_image" it_behaves_like "image validations", "proposal_image" + + it "stores attachments with both Paperclip and Active Storage" do + image = create(:image, attachment: File.new("spec/fixtures/files/clippy.jpg")) + + expect(image.attachment).to exist + expect(image.attachment_file_name).to eq "clippy.jpg" + + expect(image.storage_attachment).to be_attached + expect(image.storage_attachment.filename).to eq "clippy.jpg" + end end diff --git a/spec/models/site_customization/image_spec.rb b/spec/models/site_customization/image_spec.rb index 5c755dbfe..ddd266d0e 100644 --- a/spec/models/site_customization/image_spec.rb +++ b/spec/models/site_customization/image_spec.rb @@ -1,6 +1,17 @@ require "rails_helper" describe SiteCustomization::Image do + it "stores images with both Paperclip and Active Storage" do + image = create(:site_customization_image, name: "map", + image: File.new("spec/fixtures/files/custom_map.jpg")) + + expect(image.image).to exist + expect(image.image_file_name).to eq "custom_map.jpg" + + expect(image.storage_image).to be_attached + expect(image.storage_image.filename).to eq "custom_map.jpg" + end + describe "logo" do it "is valid with a 260x80 image" do image = build(:site_customization_image, From 619d402f9220a02e5c591d2eed718a5f9175b9b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 26 Jul 2021 22:36:21 +0200 Subject: [PATCH 06/18] Add task to migrate files to Active Storage This task was copied from Thoughtbot's migration guide [1]. They use a migration file, but we usually use rake tasks to migrate data. [1] https://github.com/thoughtbot/paperclip/blob/master/MIGRATING.md --- lib/tasks/active_storage.rake | 78 +++++++++++++++++++++++++++++++++++ lib/tasks/consul.rake | 8 +++- 2 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 lib/tasks/active_storage.rake diff --git a/lib/tasks/active_storage.rake b/lib/tasks/active_storage.rake new file mode 100644 index 000000000..f6ce1a5ea --- /dev/null +++ b/lib/tasks/active_storage.rake @@ -0,0 +1,78 @@ +# This code is based on Thoughtbot's guide to migrating from Paperclip +# to Active Storage: +# https://github.com/thoughtbot/paperclip/blob/master/MIGRATING.md +namespace :active_storage do + desc "Copy paperclip's attachment database columns to active storage" + task migrate_from_paperclip: :environment do + logger = ApplicationLogger.new + get_blob_id = "LASTVAL()" + + ActiveRecord::Base.connection.raw_connection.prepare("active_storage_blob_statement", <<-SQL) + INSERT INTO active_storage_blobs ( + key, filename, content_type, metadata, byte_size, checksum, created_at + ) VALUES ($1, $2, $3, '{}', $4, $5, $6) + SQL + + ActiveRecord::Base.connection.raw_connection.prepare("active_storage_attachment_statement", <<-SQL) + INSERT INTO active_storage_attachments ( + name, record_type, record_id, blob_id, created_at + ) VALUES ($1, $2, $3, #{get_blob_id}, $4) + SQL + + Rails.application.eager_load! + models = ActiveRecord::Base.descendants.reject(&:abstract_class?) + + ActiveRecord::Base.transaction do + models.each do |model| + attachments = model.column_names.map do |c| + if c =~ /(.+)_file_name$/ + $1 + end + end.compact + + if attachments.blank? + next + end + + model.find_each.each do |instance| + attachments.each do |attachment| + next if instance.send(attachment).path.blank? + + ActiveRecord::Base.connection.raw_connection.exec_prepared( + "active_storage_blob_statement", [ + SecureRandom.uuid, # Alternatively instance.send("#{attachment}_file_name"), + instance.send("#{attachment}_file_name"), + instance.send("#{attachment}_content_type"), + instance.send("#{attachment}_file_size"), + Digest::MD5.base64digest(File.read(instance.send(attachment).path)), + instance.updated_at.iso8601 + ]) + + ActiveRecord::Base.connection.raw_connection.exec_prepared( + "active_storage_attachment_statement", [ + attachment, + model.name, + instance.id, + instance.updated_at.iso8601 + ]) + end + end + end + end + + ActiveStorage::Attachment.find_each do |attachment| + name = attachment.name + source = attachment.record.send(name).path + + dest_dir = File.join( + "storage", + attachment.blob.key.first(2), + attachment.blob.key.first(4).last(2)) + dest = File.join(dest_dir, attachment.blob.key) + + FileUtils.mkdir_p(dest_dir) + logger.info "Copying #{source} to #{dest}" + FileUtils.cp(source, dest) + end + end +end diff --git a/lib/tasks/consul.rake b/lib/tasks/consul.rake index 0cf100930..50bcf81a9 100644 --- a/lib/tasks/consul.rake +++ b/lib/tasks/consul.rake @@ -1,7 +1,13 @@ namespace :consul do desc "Runs tasks needed to upgrade to the latest version" task execute_release_tasks: ["settings:rename_setting_keys", - "settings:add_new_settings"] + "settings:add_new_settings", + "execute_release_1.4.0_tasks"] + + desc "Runs tasks needed to upgrade from 1.3.0 to 1.4.0" + task "execute_release_1.4.0_tasks": [ + "active_storage:migrate_from_paperclip" + ] desc "Runs tasks needed to upgrade from 1.2.0 to 1.3.0" task "execute_release_1.3.0_tasks": [ From a6025d3a123b2bf687f55764def285c5b7900502 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 26 Jul 2021 23:39:08 +0200 Subject: [PATCH 07/18] Simplify copying files in Active Storage migration We can use the `ActiveStorage::Blob` class to find where the file is supposed to be stored instead of manually setting the path. This is also more robust because it works with Active Storage configurations which don't store files in the default folder. --- lib/tasks/active_storage.rake | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/lib/tasks/active_storage.rake b/lib/tasks/active_storage.rake index f6ce1a5ea..762e2d33e 100644 --- a/lib/tasks/active_storage.rake +++ b/lib/tasks/active_storage.rake @@ -61,16 +61,11 @@ namespace :active_storage do end ActiveStorage::Attachment.find_each do |attachment| + dest = ActiveStorage::Blob.service.path_for(attachment.blob.key) name = attachment.name source = attachment.record.send(name).path - dest_dir = File.join( - "storage", - attachment.blob.key.first(2), - attachment.blob.key.first(4).last(2)) - dest = File.join(dest_dir, attachment.blob.key) - - FileUtils.mkdir_p(dest_dir) + FileUtils.mkdir_p(File.dirname(dest)) logger.info "Copying #{source} to #{dest}" FileUtils.cp(source, dest) end From a92e82326b2e31ab02a7e1e0390db517ecabb3a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 26 Jul 2021 23:48:47 +0200 Subject: [PATCH 08/18] Ignore OldPassword in Active Storage migration This model is added by the devise-security gem, but we don't use it. We were getting an error while migrating: ActiveRecord::StatementInvalid: PG::UndefinedTable: ERROR: relation "old_passwords" does not exist LINE 8: WHERE a.attrelid = '"old_passwords"'::regclas... ^ SELECT a.attname, format_type(a.atttypid, a.atttypmod), pg_get_expr(d.adbin, d.adrelid), a.attnotnull, a.atttypid, a.atttypmod, c.collname, col_description(a.attrelid, a.attnum) AS comment FROM pg_attribute a LEFT JOIN pg_attrdef d ON a.attrelid = d.adrelid AND a.attnum = d.adnum LEFT JOIN pg_type t ON a.atttypid = t.oid LEFT JOIN pg_collation c ON a.attcollation = c.oid AND a.attcollation <> t.typcollation WHERE a.attrelid = '"old_passwords"'::regclass AND a.attnum > 0 AND NOT a.attisdropped ORDER BY a.attnum lib/tasks/active_storage.rake:27:in `block (4 levels) in ' lib/tasks/active_storage.rake:26:in `each' lib/tasks/active_storage.rake:26:in `block (3 levels) in ' lib/tasks/active_storage.rake:25:in `block (2 levels) in ' Caused by: PG::UndefinedTable: ERROR: relation "old_passwords" does not exist --- lib/tasks/active_storage.rake | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/tasks/active_storage.rake b/lib/tasks/active_storage.rake index 762e2d33e..7eb4ab919 100644 --- a/lib/tasks/active_storage.rake +++ b/lib/tasks/active_storage.rake @@ -24,6 +24,8 @@ namespace :active_storage do ActiveRecord::Base.transaction do models.each do |model| + next if model.name == "OldPassword" && !model.table_exists? + attachments = model.column_names.map do |c| if c =~ /(.+)_file_name$/ $1 From b4b42c7cc9354f67378c88fbd9c19c9e44b7fe26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 00:22:08 +0200 Subject: [PATCH 09/18] Fix duplicate entries in Active Storage migation Using `LASTVAL()` wasn't working 100% of the time for some reason after inserting and deleting some rows. So we're using `RETURNING` instead. --- lib/tasks/active_storage.rake | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/lib/tasks/active_storage.rake b/lib/tasks/active_storage.rake index 7eb4ab919..067957368 100644 --- a/lib/tasks/active_storage.rake +++ b/lib/tasks/active_storage.rake @@ -5,18 +5,16 @@ namespace :active_storage do desc "Copy paperclip's attachment database columns to active storage" task migrate_from_paperclip: :environment do logger = ApplicationLogger.new - get_blob_id = "LASTVAL()" - ActiveRecord::Base.connection.raw_connection.prepare("active_storage_blob_statement", <<-SQL) - INSERT INTO active_storage_blobs ( - key, filename, content_type, metadata, byte_size, checksum, created_at - ) VALUES ($1, $2, $3, '{}', $4, $5, $6) - SQL - - ActiveRecord::Base.connection.raw_connection.prepare("active_storage_attachment_statement", <<-SQL) - INSERT INTO active_storage_attachments ( - name, record_type, record_id, blob_id, created_at - ) VALUES ($1, $2, $3, #{get_blob_id}, $4) + ActiveRecord::Base.connection.raw_connection.prepare("active_storage_statement", <<-SQL) + with rows as( + INSERT INTO active_storage_blobs ( + key, filename, content_type, metadata, byte_size, checksum, created_at + ) VALUES ($1, $2, $3, '{}', $4, $5, $6) RETURNING id + ) + INSERT INTO active_storage_attachments ( + name, record_type, record_id, blob_id, created_at + ) VALUES ($7, $8, $9, (SELECT id FROM rows), $10) SQL Rails.application.eager_load! @@ -41,17 +39,13 @@ namespace :active_storage do next if instance.send(attachment).path.blank? ActiveRecord::Base.connection.raw_connection.exec_prepared( - "active_storage_blob_statement", [ + "active_storage_statement", [ SecureRandom.uuid, # Alternatively instance.send("#{attachment}_file_name"), instance.send("#{attachment}_file_name"), instance.send("#{attachment}_content_type"), instance.send("#{attachment}_file_size"), Digest::MD5.base64digest(File.read(instance.send(attachment).path)), - instance.updated_at.iso8601 - ]) - - ActiveRecord::Base.connection.raw_connection.exec_prepared( - "active_storage_attachment_statement", [ + instance.updated_at.iso8601, attachment, model.name, instance.id, From 9900a21fd553dfc4c68d269a4e1baa4ada2cbf4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 00:38:14 +0200 Subject: [PATCH 10/18] Use the `storage_` prefix for migrated attachments Just like we add the `storage_` prefix for new records so we can use both Active Storage and Paperclip at the same time. Now the migration actually works, at least for basic cases. --- lib/tasks/active_storage.rake | 4 +-- spec/lib/tasks/active_storage_spec.rb | 40 +++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 spec/lib/tasks/active_storage_spec.rb diff --git a/lib/tasks/active_storage.rake b/lib/tasks/active_storage.rake index 067957368..76ac1e5e5 100644 --- a/lib/tasks/active_storage.rake +++ b/lib/tasks/active_storage.rake @@ -46,7 +46,7 @@ namespace :active_storage do instance.send("#{attachment}_file_size"), Digest::MD5.base64digest(File.read(instance.send(attachment).path)), instance.updated_at.iso8601, - attachment, + "storage_#{attachment}", model.name, instance.id, instance.updated_at.iso8601 @@ -58,7 +58,7 @@ namespace :active_storage do ActiveStorage::Attachment.find_each do |attachment| dest = ActiveStorage::Blob.service.path_for(attachment.blob.key) - name = attachment.name + name = attachment.name.delete_prefix("storage_") source = attachment.record.send(name).path FileUtils.mkdir_p(File.dirname(dest)) diff --git a/spec/lib/tasks/active_storage_spec.rb b/spec/lib/tasks/active_storage_spec.rb new file mode 100644 index 000000000..f7150f41b --- /dev/null +++ b/spec/lib/tasks/active_storage_spec.rb @@ -0,0 +1,40 @@ +require "rails_helper" + +describe "active storage tasks" do + describe "migrate_from_paperclip" do + let(:run_rake_task) do + Rake::Task["active_storage:migrate_from_paperclip"].reenable + Rake.application.invoke_task("active_storage:migrate_from_paperclip") + end + + let(:storage_root) { ActiveStorage::Blob.service.root } + 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")) + + expect(ActiveStorage::Attachment.count).to eq 0 + expect(ActiveStorage::Blob.count).to eq 0 + expect(test_storage_file_paths.count).to eq 0 + + run_rake_task + document.reload + + expect(ActiveStorage::Attachment.count).to eq 1 + expect(ActiveStorage::Blob.count).to eq 1 + expect(document.storage_attachment.filename).to eq "clippy.pdf" + expect(test_storage_file_paths.count).to eq 1 + expect(storage_file_path(document)).to eq test_storage_file_paths.first + end + + def test_storage_file_paths + Dir.glob("#{storage_root}/**/*").select { |file_or_folder| File.file?(file_or_folder) } + end + + def storage_file_path(record) + ActiveStorage::Blob.service.path_for(record.storage_attachment.blob.key) + end + end +end From 038fb7689671e2d360b48f862f04aac43a4cb893 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 00:49:34 +0200 Subject: [PATCH 11/18] Allow executing the migration task twice in tests We were getting a duplicate prepared statement error when trying to execute more than one test using this task. So we're checking whether the prepared statement exists before defining it. --- lib/tasks/active_storage.rake | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/lib/tasks/active_storage.rake b/lib/tasks/active_storage.rake index 76ac1e5e5..8766c1b4b 100644 --- a/lib/tasks/active_storage.rake +++ b/lib/tasks/active_storage.rake @@ -5,17 +5,21 @@ namespace :active_storage do desc "Copy paperclip's attachment database columns to active storage" task migrate_from_paperclip: :environment do logger = ApplicationLogger.new + connection = ActiveRecord::Base.connection.raw_connection + statement_name = "active_storage_statement" - ActiveRecord::Base.connection.raw_connection.prepare("active_storage_statement", <<-SQL) - with rows as( - INSERT INTO active_storage_blobs ( - key, filename, content_type, metadata, byte_size, checksum, created_at - ) VALUES ($1, $2, $3, '{}', $4, $5, $6) RETURNING id - ) - INSERT INTO active_storage_attachments ( - name, record_type, record_id, blob_id, created_at - ) VALUES ($7, $8, $9, (SELECT id FROM rows), $10) - SQL + unless connection.exec("SELECT COUNT(*) FROM pg_prepared_statements WHERE name='#{statement_name}'").each_row.to_a[0][0] > 0 + connection.prepare(statement_name, <<-SQL) + with rows as( + INSERT INTO active_storage_blobs ( + key, filename, content_type, metadata, byte_size, checksum, created_at + ) VALUES ($1, $2, $3, '{}', $4, $5, $6) RETURNING id + ) + INSERT INTO active_storage_attachments ( + name, record_type, record_id, blob_id, created_at + ) VALUES ($7, $8, $9, (SELECT id FROM rows), $10) + SQL + end Rails.application.eager_load! models = ActiveRecord::Base.descendants.reject(&:abstract_class?) @@ -38,8 +42,8 @@ namespace :active_storage do attachments.each do |attachment| next if instance.send(attachment).path.blank? - ActiveRecord::Base.connection.raw_connection.exec_prepared( - "active_storage_statement", [ + connection.exec_prepared( + statement_name, [ SecureRandom.uuid, # Alternatively instance.send("#{attachment}_file_name"), instance.send("#{attachment}_file_name"), instance.send("#{attachment}_content_type"), From 7330bfb6aee35489b1fa643f61c47fb0195bcc77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 01:06:37 +0200 Subject: [PATCH 12/18] Ignore deleted files in Active Storage migration Files might be missing for whatever reason or records might not point to any files; in these edge cases, we were getting an exception. --- lib/tasks/active_storage.rake | 16 +++++++++++----- spec/lib/tasks/active_storage_spec.rb | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/lib/tasks/active_storage.rake b/lib/tasks/active_storage.rake index 8766c1b4b..a803840f9 100644 --- a/lib/tasks/active_storage.rake +++ b/lib/tasks/active_storage.rake @@ -40,7 +40,11 @@ namespace :active_storage do model.find_each.each do |instance| attachments.each do |attachment| - next if instance.send(attachment).path.blank? + source = instance.send(attachment).path + + next if source.blank? + + file = File.read(source) if File.exist?(source) connection.exec_prepared( statement_name, [ @@ -48,7 +52,7 @@ namespace :active_storage do instance.send("#{attachment}_file_name"), instance.send("#{attachment}_content_type"), instance.send("#{attachment}_file_size"), - Digest::MD5.base64digest(File.read(instance.send(attachment).path)), + file && Digest::MD5.base64digest(file) || SecureRandom.hex(32), instance.updated_at.iso8601, "storage_#{attachment}", model.name, @@ -65,9 +69,11 @@ namespace :active_storage do name = attachment.name.delete_prefix("storage_") source = attachment.record.send(name).path - FileUtils.mkdir_p(File.dirname(dest)) - logger.info "Copying #{source} to #{dest}" - FileUtils.cp(source, dest) + if source && File.exist?(source) + FileUtils.mkdir_p(File.dirname(dest)) + logger.info "Copying #{source} to #{dest}" + FileUtils.cp(source, dest) + end end end end diff --git a/spec/lib/tasks/active_storage_spec.rb b/spec/lib/tasks/active_storage_spec.rb index f7150f41b..06ef03a41 100644 --- a/spec/lib/tasks/active_storage_spec.rb +++ b/spec/lib/tasks/active_storage_spec.rb @@ -29,6 +29,21 @@ describe "active storage tasks" do expect(storage_file_path(document)).to eq test_storage_file_paths.first 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")) + FileUtils.rm(document.attachment.path) + + run_rake_task + document.reload + + expect(ActiveStorage::Attachment.count).to eq 1 + expect(ActiveStorage::Blob.count).to eq 1 + expect(document.storage_attachment.filename).to eq "clippy.pdf" + expect(test_storage_file_paths.count).to eq 0 + end + def test_storage_file_paths Dir.glob("#{storage_root}/**/*").select { |file_or_folder| File.file?(file_or_folder) } end From fd67477281e0bd5333921d6a06fc387a35a87f9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 01:18:57 +0200 Subject: [PATCH 13/18] Don't migrate files already in Active Storage This way we reduce the hypothetical problems we could find if executing the task several times. --- lib/tasks/active_storage.rake | 5 +++++ spec/lib/tasks/active_storage_spec.rb | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/lib/tasks/active_storage.rake b/lib/tasks/active_storage.rake index a803840f9..bfc2fd2e0 100644 --- a/lib/tasks/active_storage.rake +++ b/lib/tasks/active_storage.rake @@ -40,6 +40,8 @@ namespace :active_storage do model.find_each.each do |instance| attachments.each do |attachment| + next if instance.send(:"storage_#{attachment}").attached? + source = instance.send(attachment).path next if source.blank? @@ -66,6 +68,9 @@ namespace :active_storage do ActiveStorage::Attachment.find_each do |attachment| dest = ActiveStorage::Blob.service.path_for(attachment.blob.key) + + next if File.exist?(dest) + name = attachment.name.delete_prefix("storage_") source = attachment.record.send(name).path diff --git a/spec/lib/tasks/active_storage_spec.rb b/spec/lib/tasks/active_storage_spec.rb index 06ef03a41..359a00ecb 100644 --- a/spec/lib/tasks/active_storage_spec.rb +++ b/spec/lib/tasks/active_storage_spec.rb @@ -44,6 +44,26 @@ describe "active storage tasks" do expect(test_storage_file_paths.count).to eq 0 end + it "does not migrate already migrated records" do + document = create(:document, attachment: File.new("spec/fixtures/files/clippy.pdf")) + + migrated_file = test_storage_file_paths.first + attachment_id = document.storage_attachment.attachment.id + blob_id = document.storage_attachment.blob.id + + run_rake_task + document.reload + + expect(ActiveStorage::Attachment.count).to eq 1 + expect(ActiveStorage::Blob.count).to eq 1 + expect(document.storage_attachment.attachment.id).to eq attachment_id + expect(document.storage_attachment.blob.id).to eq blob_id + + expect(test_storage_file_paths.count).to eq 1 + expect(storage_file_path(document)).to eq migrated_file + expect(test_storage_file_paths.first).to eq migrated_file + end + def test_storage_file_paths Dir.glob("#{storage_root}/**/*").select { |file_or_folder| File.file?(file_or_folder) } end From b5026e12a79b6a1ba34b2e4c10d9481264ea77d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 01:29:03 +0200 Subject: [PATCH 14/18] Ignore missing records in Active Storage migration There could be inconsistencies in the database and an attachment might have a `record_id` pointing to a record which no longer exist. We were getting an exception in this case. --- lib/tasks/active_storage.rake | 2 +- spec/lib/tasks/active_storage_spec.rb | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/tasks/active_storage.rake b/lib/tasks/active_storage.rake index bfc2fd2e0..ecf3d0388 100644 --- a/lib/tasks/active_storage.rake +++ b/lib/tasks/active_storage.rake @@ -69,7 +69,7 @@ namespace :active_storage do ActiveStorage::Attachment.find_each do |attachment| dest = ActiveStorage::Blob.service.path_for(attachment.blob.key) - next if File.exist?(dest) + next if File.exist?(dest) || !attachment.record name = attachment.name.delete_prefix("storage_") source = attachment.record.send(name).path diff --git a/spec/lib/tasks/active_storage_spec.rb b/spec/lib/tasks/active_storage_spec.rb index 359a00ecb..f554d05de 100644 --- a/spec/lib/tasks/active_storage_spec.rb +++ b/spec/lib/tasks/active_storage_spec.rb @@ -64,6 +64,19 @@ describe "active storage tasks" do expect(test_storage_file_paths.first).to eq migrated_file end + it "does not migrate files for deleted records" do + document = create(:document, attachment: File.new("spec/fixtures/files/clippy.pdf")) + FileUtils.rm storage_file_path(document) + Document.delete_all + + run_rake_task + + expect(ActiveStorage::Attachment.count).to eq 1 + expect(ActiveStorage::Blob.count).to eq 1 + expect(document.storage_attachment.filename).to eq "clippy.pdf" + expect(test_storage_file_paths.count).to eq 0 + end + def test_storage_file_paths Dir.glob("#{storage_root}/**/*").select { |file_or_folder| File.file?(file_or_folder) } end From 5a4921a1afc4b27786b00a4f681bb0a8859bb75c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Wed, 15 Sep 2021 19:05:58 +0200 Subject: [PATCH 15/18] Use URI.open to assign external cached attachments We were having issues with cached attachments and external services. A `Tempfile` is returned by `URI.open` when using S3, so we're dealing with this case as well. --- app/models/concerns/attachable.rb | 2 +- app/models/concerns/has_attachment.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/concerns/attachable.rb b/app/models/concerns/attachable.rb index fb13dc517..1fbe15512 100644 --- a/app/models/concerns/attachable.rb +++ b/app/models/concerns/attachable.rb @@ -37,7 +37,7 @@ module Attachable if Paperclip::Attachment.default_options[:storage] == :filesystem File.open(cached_attachment) { |file| self.attachment = file } else - self.attachment = URI.parse(cached_attachment) + self.attachment = URI.open(cached_attachment) end end diff --git a/app/models/concerns/has_attachment.rb b/app/models/concerns/has_attachment.rb index 450e515ea..6f3f67844 100644 --- a/app/models/concerns/has_attachment.rb +++ b/app/models/concerns/has_attachment.rb @@ -9,7 +9,7 @@ module HasAttachment alias_method :"paperclip_#{attribute}=", :"#{attribute}=" define_method :"#{attribute}=" do |file| - if file.is_a?(IO) + if file.is_a?(IO) || file.is_a?(Tempfile) && !file.is_a?(Ckeditor::Http::QqFile) send(:"storage_#{attribute}").attach(io: file, filename: File.basename(file.path)) elsif file.nil? send(:"storage_#{attribute}").detach From 87e427f4335df91479e2a74196893e54e40485be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Fri, 17 Sep 2021 15:00:30 +0200 Subject: [PATCH 16/18] Allow remote storages in Active Storage migration So forks that connected Paperclip with S3 buckets can migrate files too. --- lib/tasks/active_storage.rake | 36 ++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/lib/tasks/active_storage.rake b/lib/tasks/active_storage.rake index ecf3d0388..d548ab21d 100644 --- a/lib/tasks/active_storage.rake +++ b/lib/tasks/active_storage.rake @@ -23,6 +23,7 @@ namespace :active_storage do Rails.application.eager_load! models = ActiveRecord::Base.descendants.reject(&:abstract_class?) + paperclip_storage = Paperclip::Attachment.default_options[:storage] ActiveRecord::Base.transaction do models.each do |model| @@ -42,11 +43,19 @@ namespace :active_storage do attachments.each do |attachment| next if instance.send(:"storage_#{attachment}").attached? - source = instance.send(attachment).path + source = if paperclip_storage == :filesystem + instance.send(attachment).path + else + instance.send(attachment).url + end - next if source.blank? + next if source.blank? || source == "/images/original/missing.png" - file = File.read(source) if File.exist?(source) + file = if paperclip_storage == :filesystem + File.read(source) if File.exist?(source) + else + Net::HTTP.get(URI(source)) + end connection.exec_prepared( statement_name, [ @@ -67,18 +76,23 @@ namespace :active_storage do end ActiveStorage::Attachment.find_each do |attachment| - dest = ActiveStorage::Blob.service.path_for(attachment.blob.key) + blob = attachment.blob - next if File.exist?(dest) || !attachment.record + next if blob.service.exist?(blob.key) || !attachment.record name = attachment.name.delete_prefix("storage_") - source = attachment.record.send(name).path + paperclip_attachment = attachment.record.send(name) - if source && File.exist?(source) - FileUtils.mkdir_p(File.dirname(dest)) - logger.info "Copying #{source} to #{dest}" - FileUtils.cp(source, dest) - end + next unless paperclip_attachment.exists? + + source_file = if paperclip_storage == :filesystem + paperclip_attachment.path + else + URI.open(paperclip_attachment.url, &:read) + end + + logger.info "Copying #{paperclip_attachment.url} to active storage" + blob.service.upload(blob.key, source_file) end end end From 259cd8e108796079594a6f218e5d09e0e939e6b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 20 Sep 2021 23:39:00 +0200 Subject: [PATCH 17/18] Add sample configurations for external storages These configurations are based on the ones ActiveStorage generates automatically for Rails 6 applications. --- config/storage.yml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/config/storage.yml b/config/storage.yml index 1f93f7323..590c3bb76 100644 --- a/config/storage.yml +++ b/config/storage.yml @@ -5,3 +5,28 @@ local: test: service: Disk root: <%= Rails.root.join("tmp/storage") %> + +# s3: +# service: S3 +# access_key_id: <%= Rails.application.secrets.dig(:s3, :access_key_id) %> +# secret_access_key: <%= Rails.application.secrets.dig(:s3, :secret_access_key) %> +# region: <%= Rails.application.secrets.dig(:s3, :region) %> +# bucket: <%= Rails.application.secrets.dig(:s3, :bucket) %> + +# Remember not to checkin your GCS keyfile to a repository +# gcs: +# service: GCS +# project: <%= Rails.application.secrets.dig(:gcs, :project) %> +# credentials: <%= Rails.root.join(Rails.application.secrets.dig(:gcs, :credentials).to_s) %> +# bucket: <%= Rails.application.secrets.dig(:gcs, :bucket) %> + +# azure: +# service: AzureStorage +# storage_account_name: <%= Rails.application.secrets.dig(:azure, :storage_account_name) %> +# storage_access_key: <%= Rails.application.secrets.dig(:azure, :storage_access_key) %> +# container: <%= Rails.application.secrets.dig(:azure, :container) %> + +# mirror: +# service: Mirror +# primary: local +# mirrors: [ s3, gcs, azure ] From be9c272ce490e65e989decb7958c5270f3017f24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 23 Sep 2021 00:38:19 +0200 Subject: [PATCH 18/18] Remove default Active Storage direct upload action We're already using a custom controller to handle direct uploads. Besides, as mentioned by one of Active Storage co-authors [1], the Active Storage DirectUploadsController doesn't provide any authentication or validation at all, meaning anyone could create blobs in our database by posting to `/rails/active_storage/direct_uploads`. The response there could be then used to upload any file (again, without validation) to `/rails/active_storage/disk/`. For now, we're monkey-patching the controllers in order to send unauthorized responses, since we aren't using these routes. If we ever enable direct uploads with Active Storage, we'll have to add some sort of authentication. Similar upload solutions like CKEditor don't have this issue since their controllers inherit from `ApplicationController` (which includes authorization rules), while Active Storage controllers inherit from `ActionController::Base`. [1] https://discuss.rubyonrails.org/t/activestorage-direct-uploads-safe-by-default-how-to-make-it-safe/74863/2 --- .../initializers/disable_active_storage_uploads.rb | 11 +++++++++++ .../direct_uploads_controller_spec.rb | 14 ++++++++++++++ .../active_storage/disk_controller_spec.rb | 13 +++++++++++++ spec/factories/files.rb | 6 ++++++ 4 files changed, 44 insertions(+) create mode 100644 config/initializers/disable_active_storage_uploads.rb create mode 100644 spec/controllers/active_storage/direct_uploads_controller_spec.rb create mode 100644 spec/controllers/active_storage/disk_controller_spec.rb diff --git a/config/initializers/disable_active_storage_uploads.rb b/config/initializers/disable_active_storage_uploads.rb new file mode 100644 index 000000000..fa5a52a91 --- /dev/null +++ b/config/initializers/disable_active_storage_uploads.rb @@ -0,0 +1,11 @@ +ActiveStorage::DirectUploadsController.class_eval do + def create + head :unauthorized + end +end + +ActiveStorage::DiskController.class_eval do + def update + head :unauthorized + end +end diff --git a/spec/controllers/active_storage/direct_uploads_controller_spec.rb b/spec/controllers/active_storage/direct_uploads_controller_spec.rb new file mode 100644 index 000000000..e4bb0e8e6 --- /dev/null +++ b/spec/controllers/active_storage/direct_uploads_controller_spec.rb @@ -0,0 +1,14 @@ +require "rails_helper" + +describe ActiveStorage::DirectUploadsController do + describe "POST create" do + it "doesn't allow anonymous users to upload files" do + blob_attributes = { filename: "logo.pdf", byte_size: 30000, checksum: SecureRandom.hex(32) } + + post :create, params: { blob: blob_attributes } + + expect(ActiveStorage::Blob.count).to eq 0 + expect(response).to be_unauthorized + end + end +end diff --git a/spec/controllers/active_storage/disk_controller_spec.rb b/spec/controllers/active_storage/disk_controller_spec.rb new file mode 100644 index 000000000..48fc34d7a --- /dev/null +++ b/spec/controllers/active_storage/disk_controller_spec.rb @@ -0,0 +1,13 @@ +require "rails_helper" + +describe ActiveStorage::DiskController do + describe "PUT update" do + it "doesn't allow anonymous users to upload files" do + blob = create(:active_storage_blob) + + put :update, params: { encoded_token: blob.signed_id } + + expect(response).to be_unauthorized + end + end +end diff --git a/spec/factories/files.rb b/spec/factories/files.rb index 2290ccab5..0926ee576 100644 --- a/spec/factories/files.rb +++ b/spec/factories/files.rb @@ -55,4 +55,10 @@ FactoryBot.define do end initialize_with { new(attributes) } end + + factory :active_storage_blob, class: "ActiveStorage::Blob" do + filename { "sample.pdf" } + byte_size { 3000 } + checksum { SecureRandom.hex(32) } + end end