From 5963766d840ddcdb577a1bd10eb1491a4ef9132f Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 9 Jul 2017 18:48:26 +0200 Subject: 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 20c619aa5a..1dd202ca45 100644 --- a/app/models/active_storage/attachment.rb +++ b/app/models/active_storage/attachment.rb @@ -6,19 +6,11 @@ require "active_support/core_ext/module/delegation" 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 1e0f9a6b7e..0452089a5f 100644 --- a/lib/active_storage/attached/macros.rb +++ b/lib/active_storage/attached/macros.rb @@ -16,6 +16,9 @@ module ActiveStorage::Attached::Macros 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 @@ module ActiveStorage::Attached::Macros 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 99d980196a..129f166776 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 80e4cb6234..02fc9c9abc 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 c56e7a1786..99d8b8554b 100644 --- a/lib/active_storage/migration.rb +++ b/lib/active_storage/migration.rb @@ -14,15 +14,16 @@ class ActiveStorageCreateTables < ActiveRecord::Migration[5.1] # :nodoc: 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 c0f5db819d..1a9fc6f932 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) -- cgit v1.2.3 From 212f925654f944067f18429ca02d902473214722 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Sun, 23 Jul 2017 21:19:34 +0200 Subject: 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 99d8b8554b..e843c1b630 100644 --- a/lib/active_storage/migration.rb +++ b/lib/active_storage/migration.rb @@ -21,9 +21,7 @@ class ActiveStorageCreateTables < ActiveRecord::Migration[5.1] # :nodoc: 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 -- cgit v1.2.3 From a4f36f957e013f6da34e04f0d3f1d86d86491454 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Sun, 23 Jul 2017 21:41:16 +0200 Subject: 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 129f166776..704369ba89 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 @@ class ActiveStorage::Attached::Many < ActiveStorage::Attached def purge if attached? attachments.each(&:purge) - @attachments = nil + attachments.reload end end @@ -42,7 +42,6 @@ class ActiveStorage::Attached::Many < ActiveStorage::Attached 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 02fc9c9abc..d255412842 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 @@ class ActiveStorage::Attached::One < ActiveStorage::Attached def purge if attached? attachment.purge - @attachment = nil + write_attachment nil end end @@ -40,7 +41,11 @@ class ActiveStorage::Attached::One < ActiveStorage::Attached 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 1a9fc6f932..45f62b0bbf 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 -- cgit v1.2.3 From e16739d4b2dd4910540cf926aa526ed9c96253b7 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 23 Jul 2017 16:27:33 -0500 Subject: 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 60b15b1fdd..8f309d0b28 100644 --- a/test/controllers/direct_uploads_controller_test.rb +++ b/test/controllers/direct_uploads_controller_test.rb @@ -22,7 +22,7 @@ if SERVICE_CONFIGURATIONS[:s3] 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 @@ if SERVICE_CONFIGURATIONS[:gcs] 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 -- cgit v1.2.3 From eb9b019fee51cff69dfcbf19e6c326c426acc297 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 23 Jul 2017 16:28:45 -0500 Subject: 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 435033f980..002d2bde5b 100644 --- a/app/models/active_storage/variant.rb +++ b/app/models/active_storage/variant.rb @@ -11,7 +11,7 @@ class ActiveStorage::Variant end def processed - process unless service.exist?(key) + process unless processed? self end @@ -25,6 +25,10 @@ class ActiveStorage::Variant private + def processed? + service.exist?(key) + end + def process service.upload key, transform(service.download(blob.key)) end -- cgit v1.2.3 From 68b5d274a365c1babdb92dedfcf2e600138be5eb Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 23 Jul 2017 16:47:11 -0500 Subject: 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 0452089a5f..5915793f8a 100644 --- a/lib/active_storage/attached/macros.rb +++ b/lib/active_storage/attached/macros.rb @@ -33,6 +33,10 @@ module ActiveStorage::Attached::Macros # 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 @@ module ActiveStorage::Attached::Macros 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 45f62b0bbf..eac3cbe680 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 -- cgit v1.2.3 From e16d0c9ceacd771c99048385dc886c6026c7bc45 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 23 Jul 2017 16:57:26 -0500 Subject: 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 9e4749b1c6..c1bfe12b77 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 1dd202ca45..d491c7224e 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 9fa7b8e021..6b81545897 100644 --- a/lib/active_storage/attached.rb +++ b/lib/active_storage/attached.rb @@ -4,8 +4,6 @@ require "active_storage/attachment" 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 650e997205..a6e228c4d2 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -62,7 +62,3 @@ end require "active_storage/attached" ActiveRecord::Base.send :extend, ActiveStorage::Attached::Macros - -require "global_id" -GlobalID.app = "ActiveStorageExampleApp" -ActiveRecord::Base.send :include, GlobalID::Identification -- cgit v1.2.3 From 2bbfaf0c9f6ad23cb2c64a917848ca180917ebe2 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 23 Jul 2017 16:58:09 -0500 Subject: Demonstrate preloading in example --- README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/README.md b/README.md index c1bfe12b77..8f340d7013 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 ``` -- cgit v1.2.3