Merge pull request #32 from rails/explore-polymorphism

Use regular polymorphic associations rather than record_gid

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).
 
@@ -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
 ```
 

app/models/active_storage/attachment.rb

@@ -1,24 +1,15 @@
 require "active_storage/blob"
-require "global_id"
 require "active_support/core_ext/module/delegation"
 
 # Schema: id, record_gid, blob_id, created_at
 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

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
 

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
@@ -30,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)
@@ -38,6 +45,11 @@ 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"
+
+    scope :"with_attached_#{name}", -> { includes("#{name}_attachments": :blob) }
+
     if dependent == :purge_later
       before_destroy { public_send(name).purge_later }
     end

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 ||= ActiveStorage::Attachment.where(record_gid: record.to_gid.to_s, name: name)
+    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))
-    end
+    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
 
   # Checks the presence of attachments.
@@ -34,15 +34,14 @@ def attached?
   def purge
     if attached?
       attachments.each(&:purge)
-      @attachments = nil
+      attachments.reload
     end
   end
 
   # Purges each associated attachment through the queuing system.
   def purge_later
     if attached?
       attachments.each(&:purge_later)
-      @attachments = nil
     end
   end
 end

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 ||= ActiveStorage::Attachment.find_by(record_gid: record.to_gid.to_s, name: name)
+    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))
+    write_attachment \
+      ActiveStorage::Attachment.create!(record: record, name: name, blob: create_blob_from(attachable))
   end
 
   # Checks the presence of the attachment.
@@ -32,15 +33,19 @@ def attached?
   def purge
     if attached?
       attachment.purge
-      @attachment = nil
+      write_attachment nil
     end
   end
 
   # Purges the attachment through the queuing system.
   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

lib/active_storage/migration.rb

@@ -14,15 +14,14 @@ 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, :name, :blob_id ], name: "index_active_storage_attachments_uniqueness", unique: true
     end
   end
 end

test/models/attachments_test.rb

@@ -68,6 +68,17 @@ 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" })
+
+    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
     @user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "wonky.jpg")
     highlight_keys = @user.highlights.collect(&:key)

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