From 5963766d840ddcdb577a1bd10eb1491a4ef9132f Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 9 Jul 2017 18:48:26 +0200 Subject: [PATCH 1/8] Explore regular polymorphic associations rather than record_gid --- app/models/active_storage/attachment.rb | 10 +--------- lib/active_storage/attached/macros.rb | 6 ++++++ lib/active_storage/attached/many.rb | 4 ++-- lib/active_storage/attached/one.rb | 4 ++-- lib/active_storage/migration.rb | 9 +++++---- test/models/attachments_test.rb | 8 ++++++++ 6 files changed, 24 insertions(+), 17 deletions(-) diff --git a/app/models/active_storage/attachment.rb b/app/models/active_storage/attachment.rb index 20c619aa5add6..1dd202ca45e84 100644 --- a/app/models/active_storage/attachment.rb +++ b/app/models/active_storage/attachment.rb @@ -6,19 +6,11 @@ class ActiveStorage::Attachment < ActiveRecord::Base self.table_name = "active_storage_attachments" + belongs_to :record, polymorphic: true belongs_to :blob, class_name: "ActiveStorage::Blob" delegate_missing_to :blob - def record - @record ||= GlobalID::Locator.locate(record_gid) - end - - def record=(record) - @record = record - self.record_gid = record&.to_gid - end - def purge blob.purge destroy diff --git a/lib/active_storage/attached/macros.rb b/lib/active_storage/attached/macros.rb index 1e0f9a6b7e4ff..0452089a5fe70 100644 --- a/lib/active_storage/attached/macros.rb +++ b/lib/active_storage/attached/macros.rb @@ -16,6 +16,9 @@ def has_one_attached(name, dependent: :purge_later) instance_variable_set("@active_storage_attached_#{name}", ActiveStorage::Attached::One.new(name, self)) end + has_one :"#{name}_attachment", -> { where(name: name) }, class_name: "ActiveStorage::Attachment", as: :record + has_one :"#{name}_blob", through: :"#{name}_attachment" + if dependent == :purge_later before_destroy { public_send(name).purge_later } end @@ -38,6 +41,9 @@ def has_many_attached(name, dependent: :purge_later) instance_variable_set("@active_storage_attached_#{name}", ActiveStorage::Attached::Many.new(name, self)) end + has_many :"#{name}_attachments", -> { where(name: name) }, as: :record, class_name: "ActiveStorage::Attachment" + has_many :"#{name}_blobs", through: :"#{name}_attachments" + if dependent == :purge_later before_destroy { public_send(name).purge_later } end diff --git a/lib/active_storage/attached/many.rb b/lib/active_storage/attached/many.rb index 99d980196a1a8..129f1667764a2 100644 --- a/lib/active_storage/attached/many.rb +++ b/lib/active_storage/attached/many.rb @@ -7,14 +7,14 @@ class ActiveStorage::Attached::Many < ActiveStorage::Attached # You don't have to call this method to access the attachments' methods as # they are all available at the model level. def attachments - @attachments ||= ActiveStorage::Attachment.where(record_gid: record.to_gid.to_s, name: name) + @attachments ||= record.public_send("#{name}_attachments") end # Associates one or several attachments with the current record, saving # them to the database. def attach(*attachables) @attachments = attachments | Array(attachables).flatten.collect do |attachable| - ActiveStorage::Attachment.create!(record_gid: record.to_gid.to_s, name: name, blob: create_blob_from(attachable)) + ActiveStorage::Attachment.create!(record: record, name: name, blob: create_blob_from(attachable)) end end diff --git a/lib/active_storage/attached/one.rb b/lib/active_storage/attached/one.rb index 80e4cb623489c..02fc9c9abcd83 100644 --- a/lib/active_storage/attached/one.rb +++ b/lib/active_storage/attached/one.rb @@ -7,13 +7,13 @@ class ActiveStorage::Attached::One < ActiveStorage::Attached # You don't have to call this method to access the attachment's methods as # they are all available at the model level. def attachment - @attachment ||= ActiveStorage::Attachment.find_by(record_gid: record.to_gid.to_s, name: name) + @attachment ||= record.public_send("#{name}_attachment") end # Associates a given attachment with the current record, saving it to the # database. def attach(attachable) - @attachment = ActiveStorage::Attachment.create!(record_gid: record.to_gid.to_s, name: name, blob: create_blob_from(attachable)) + @attachment = ActiveStorage::Attachment.create!(record: record, name: name, blob: create_blob_from(attachable)) end # Checks the presence of the attachment. diff --git a/lib/active_storage/migration.rb b/lib/active_storage/migration.rb index c56e7a1786c8d..99d8b8554b0a8 100644 --- a/lib/active_storage/migration.rb +++ b/lib/active_storage/migration.rb @@ -14,15 +14,16 @@ def change create_table :active_storage_attachments do |t| t.string :name - t.string :record_gid + t.string :record_type + t.integer :record_id t.integer :blob_id t.datetime :created_at - t.index :record_gid t.index :blob_id - t.index [ :record_gid, :name ] - t.index [ :record_gid, :blob_id ], unique: true + t.index [ :record_type, :record_id ] + t.index [ :record_type, :record_id, :name ], name: "index_active_storage_attachments_record_and_name" + t.index [ :record_type, :record_id, :blob_id ], name: "index_active_storage_attachments_uniqueness", unique: true end end end diff --git a/test/models/attachments_test.rb b/test/models/attachments_test.rb index c0f5db819dc30..1a9fc6f932e86 100644 --- a/test/models/attachments_test.rb +++ b/test/models/attachments_test.rb @@ -68,6 +68,14 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase assert_equal "country.jpg", @user.highlights.second.filename.to_s end + test "find attached blobs" do + @user.highlights.attach( + { io: StringIO.new("STUFF"), filename: "town.jpg", content_type: "image/jpg" }, + { io: StringIO.new("IT"), filename: "country.jpg", content_type: "image/jpg" }) + + User.where(id: @user.id).includes(highlights_attachments: :blob).first + end + test "purge attached blobs" do @user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "wonky.jpg") highlight_keys = @user.highlights.collect(&:key) From 212f925654f944067f18429ca02d902473214722 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Sun, 23 Jul 2017 21:19:34 +0200 Subject: [PATCH 2/8] Collapse indeces per Jeremy. --- lib/active_storage/migration.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/active_storage/migration.rb b/lib/active_storage/migration.rb index 99d8b8554b0a8..e843c1b630051 100644 --- a/lib/active_storage/migration.rb +++ b/lib/active_storage/migration.rb @@ -21,9 +21,7 @@ def change t.datetime :created_at t.index :blob_id - t.index [ :record_type, :record_id ] - t.index [ :record_type, :record_id, :name ], name: "index_active_storage_attachments_record_and_name" - t.index [ :record_type, :record_id, :blob_id ], name: "index_active_storage_attachments_uniqueness", unique: true + t.index [ :record_type, :record_id, :name, :blob_id ], name: "index_active_storage_attachments_uniqueness", unique: true end end end From a4f36f957e013f6da34e04f0d3f1d86d86491454 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Sun, 23 Jul 2017 21:41:16 +0200 Subject: [PATCH 3/8] Fix attaching with standard Rails associations. Removes needless ivar caching (a Rails association handles that). Inserts a reload and a nil assign, since the association proxy doesn't seem to that it's been destroyed through `purge`. --- lib/active_storage/attached/many.rb | 9 ++++----- lib/active_storage/attached/one.rb | 13 +++++++++---- test/models/attachments_test.rb | 4 ++-- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/lib/active_storage/attached/many.rb b/lib/active_storage/attached/many.rb index 129f1667764a2..704369ba892cf 100644 --- a/lib/active_storage/attached/many.rb +++ b/lib/active_storage/attached/many.rb @@ -7,15 +7,15 @@ class ActiveStorage::Attached::Many < ActiveStorage::Attached # You don't have to call this method to access the attachments' methods as # they are all available at the model level. def attachments - @attachments ||= record.public_send("#{name}_attachments") + record.public_send("#{name}_attachments") end # Associates one or several attachments with the current record, saving # them to the database. def attach(*attachables) - @attachments = attachments | Array(attachables).flatten.collect do |attachable| + record.public_send("#{name}_attachments=", attachments | Array(attachables).flat_map do |attachable| ActiveStorage::Attachment.create!(record: record, name: name, blob: create_blob_from(attachable)) - end + end) end # Checks the presence of attachments. @@ -34,7 +34,7 @@ def attached? def purge if attached? attachments.each(&:purge) - @attachments = nil + attachments.reload end end @@ -42,7 +42,6 @@ def purge def purge_later if attached? attachments.each(&:purge_later) - @attachments = nil end end end diff --git a/lib/active_storage/attached/one.rb b/lib/active_storage/attached/one.rb index 02fc9c9abcd83..d25541284290a 100644 --- a/lib/active_storage/attached/one.rb +++ b/lib/active_storage/attached/one.rb @@ -7,13 +7,14 @@ class ActiveStorage::Attached::One < ActiveStorage::Attached # You don't have to call this method to access the attachment's methods as # they are all available at the model level. def attachment - @attachment ||= record.public_send("#{name}_attachment") + record.public_send("#{name}_attachment") end # Associates a given attachment with the current record, saving it to the # database. def attach(attachable) - @attachment = ActiveStorage::Attachment.create!(record: record, name: name, blob: create_blob_from(attachable)) + write_attachment \ + ActiveStorage::Attachment.create!(record: record, name: name, blob: create_blob_from(attachable)) end # Checks the presence of the attachment. @@ -32,7 +33,7 @@ def attached? def purge if attached? attachment.purge - @attachment = nil + write_attachment nil end end @@ -40,7 +41,11 @@ def purge def purge_later if attached? attachment.purge_later - @attachment = nil end end + + private + def write_attachment(attachment) + record.public_send("#{name}_attachment=", attachment) + end end diff --git a/test/models/attachments_test.rb b/test/models/attachments_test.rb index 1a9fc6f932e86..45f62b0bbfacc 100644 --- a/test/models/attachments_test.rb +++ b/test/models/attachments_test.rb @@ -70,9 +70,9 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase test "find attached blobs" do @user.highlights.attach( - { io: StringIO.new("STUFF"), filename: "town.jpg", content_type: "image/jpg" }, + { io: StringIO.new("STUFF"), filename: "town.jpg", content_type: "image/jpg" }, { io: StringIO.new("IT"), filename: "country.jpg", content_type: "image/jpg" }) - + User.where(id: @user.id).includes(highlights_attachments: :blob).first end From e16739d4b2dd4910540cf926aa526ed9c96253b7 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 23 Jul 2017 16:27:33 -0500 Subject: [PATCH 4/8] Work-around until @response.parsed_body problem is solved --- test/controllers/direct_uploads_controller_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/controllers/direct_uploads_controller_test.rb b/test/controllers/direct_uploads_controller_test.rb index 60b15b1fddee7..8f309d0b28ba0 100644 --- a/test/controllers/direct_uploads_controller_test.rb +++ b/test/controllers/direct_uploads_controller_test.rb @@ -22,7 +22,7 @@ class ActiveStorage::S3DirectUploadsControllerTest < ActionController::TestCase post :create, params: { blob: { filename: "hello.txt", byte_size: 6, checksum: Digest::MD5.base64digest("Hello"), content_type: "text/plain" } } - @response.parsed_body.tap do |details| + JSON.parse(@response.body).tap do |details| assert_match(/#{SERVICE_CONFIGURATIONS[:s3][:bucket]}\.s3.(\S+)?amazonaws\.com/, details["upload_to_url"]) assert_equal "hello.txt", ActiveStorage::Blob.find_signed(details["signed_blob_id"]).filename.to_s end @@ -52,7 +52,7 @@ class ActiveStorage::GCSDirectUploadsControllerTest < ActionController::TestCase post :create, params: { blob: { filename: "hello.txt", byte_size: 6, checksum: Digest::MD5.base64digest("Hello"), content_type: "text/plain" } } - @response.parsed_body.tap do |details| + JSON.parse(@response.body).tap do |details| assert_match %r{storage\.googleapis\.com/#{@config[:bucket]}}, details["upload_to_url"] assert_equal "hello.txt", ActiveStorage::Blob.find_signed(details["signed_blob_id"]).filename.to_s end From eb9b019fee51cff69dfcbf19e6c326c426acc297 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 23 Jul 2017 16:28:45 -0500 Subject: [PATCH 5/8] Return to same level of abstraction --- app/models/active_storage/variant.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/active_storage/variant.rb b/app/models/active_storage/variant.rb index 435033f980fc9..002d2bde5bc85 100644 --- a/app/models/active_storage/variant.rb +++ b/app/models/active_storage/variant.rb @@ -11,7 +11,7 @@ def initialize(blob, variation) end def processed - process unless service.exist?(key) + process unless processed? self end @@ -25,6 +25,10 @@ def url(expires_in: 5.minutes, disposition: :inline) private + def processed? + service.exist?(key) + end + def process service.upload key, transform(service.download(blob.key)) end From 68b5d274a365c1babdb92dedfcf2e600138be5eb Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 23 Jul 2017 16:47:11 -0500 Subject: [PATCH 6/8] Add and test preloading scope --- lib/active_storage/attached/macros.rb | 6 ++++++ test/models/attachments_test.rb | 5 ++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/active_storage/attached/macros.rb b/lib/active_storage/attached/macros.rb index 0452089a5fe70..5915793f8a41d 100644 --- a/lib/active_storage/attached/macros.rb +++ b/lib/active_storage/attached/macros.rb @@ -33,6 +33,10 @@ def has_one_attached(name, dependent: :purge_later) # There are no columns defined on the model side, Active Storage takes # care of the mapping between your records and the attachments. # + # To avoid N+1 queries, you can include the attached blobs in your query like so: + # + # Gallery.where(user: Current.user).with_attached_photos + # # If the +:dependent+ option isn't set, all the attachments will be purged # (i.e. destroyed) whenever the record is destroyed. def has_many_attached(name, dependent: :purge_later) @@ -44,6 +48,8 @@ def has_many_attached(name, dependent: :purge_later) has_many :"#{name}_attachments", -> { where(name: name) }, as: :record, class_name: "ActiveStorage::Attachment" has_many :"#{name}_blobs", through: :"#{name}_attachments" + scope :"with_attached_#{name}", -> { includes("#{name}_attachments": :blob) } + if dependent == :purge_later before_destroy { public_send(name).purge_later } end diff --git a/test/models/attachments_test.rb b/test/models/attachments_test.rb index 45f62b0bbfacc..eac3cbe680a14 100644 --- a/test/models/attachments_test.rb +++ b/test/models/attachments_test.rb @@ -73,7 +73,10 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase { io: StringIO.new("STUFF"), filename: "town.jpg", content_type: "image/jpg" }, { io: StringIO.new("IT"), filename: "country.jpg", content_type: "image/jpg" }) - User.where(id: @user.id).includes(highlights_attachments: :blob).first + highlights = User.where(id: @user.id).with_attached_highlights.first.highlights + + assert_equal "town.jpg", highlights.first.filename.to_s + assert_equal "country.jpg", highlights.second.filename.to_s end test "purge attached blobs" do From e16d0c9ceacd771c99048385dc886c6026c7bc45 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 23 Jul 2017 16:57:26 -0500 Subject: [PATCH 7/8] No more GlobalID --- README.md | 2 +- app/models/active_storage/attachment.rb | 1 - lib/active_storage/attached.rb | 2 -- test/test_helper.rb | 4 ---- 4 files changed, 1 insertion(+), 8 deletions(-) diff --git a/README.md b/README.md index 9e4749b1c6aec..c1bfe12b77e55 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ Furthermore, this repository is likely to be in heavy flux prior to the merge to ## Compared to other storage solutions -A key difference to how Active Storage works compared to other attachment solutions in Rails is through the use of built-in [Blob](https://github.com/rails/activestorage/blob/master/lib/active_storage/blob.rb) and [Attachment](https://github.com/rails/activestorage/blob/master/lib/active_storage/attachment.rb) models (backed by Active Record). This means existing application models do not need to be modified with additional columns to associate with files. Active Storage uses GlobalID to provide polymorphic associations via the join model of `Attachment`, which then connects to the actual `Blob`. +A key difference to how Active Storage works compared to other attachment solutions in Rails is through the use of built-in [Blob](https://github.com/rails/activestorage/blob/master/lib/active_storage/blob.rb) and [Attachment](https://github.com/rails/activestorage/blob/master/lib/active_storage/attachment.rb) models (backed by Active Record). This means existing application models do not need to be modified with additional columns to associate with files. Active Storage uses polymorphic associations via the join model of `Attachment`, which then connects to the actual `Blob`. These `Blob` models are intended to be immutable in spirit. One file, one blob. You can associate the same blob with multiple application models as well. And if you want to do transformations of a given `Blob`, the idea is that you'll simply create a new one, rather than attempt to mutate the existing (though of course you can delete that later if you don't need it). diff --git a/app/models/active_storage/attachment.rb b/app/models/active_storage/attachment.rb index 1dd202ca45e84..d491c7224e32c 100644 --- a/app/models/active_storage/attachment.rb +++ b/app/models/active_storage/attachment.rb @@ -1,5 +1,4 @@ require "active_storage/blob" -require "global_id" require "active_support/core_ext/module/delegation" # Schema: id, record_gid, blob_id, created_at diff --git a/lib/active_storage/attached.rb b/lib/active_storage/attached.rb index 9fa7b8e0212c1..6b815458972f0 100644 --- a/lib/active_storage/attached.rb +++ b/lib/active_storage/attached.rb @@ -4,8 +4,6 @@ require "action_dispatch/http/upload" require "active_support/core_ext/module/delegation" -require "global_id/locator" - class ActiveStorage::Attached attr_reader :name, :record diff --git a/test/test_helper.rb b/test/test_helper.rb index 650e9972054bd..a6e228c4d2e8d 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -62,7 +62,3 @@ class ActionController::TestCase require "active_storage/attached" ActiveRecord::Base.send :extend, ActiveStorage::Attached::Macros - -require "global_id" -GlobalID.app = "ActiveStorageExampleApp" -ActiveRecord::Base.send :include, GlobalID::Identification From 2bbfaf0c9f6ad23cb2c64a917848ca180917ebe2 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 23 Jul 2017 16:58:09 -0500 Subject: [PATCH 8/8] Demonstrate preloading in example --- README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/README.md b/README.md index c1bfe12b77e55..8f340d7013d1c 100644 --- a/README.md +++ b/README.md @@ -66,6 +66,11 @@ class MessagesController < ApplicationController message.images.attach(params[:message][:images]) redirect_to message end + + def show + # Use the built-in with_attached_images scope to avoid N+1 + @message = Message.find(params[:id]).with_attached_images + end end ```