Merge pull request #32 from rails/explore-polymorphism

Use regular polymorphic associations rather than record_gid

5 files changed, 29 insertions, 16 deletions

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/lib/active_storage/attached/macros.rb b/lib/active_storage/attached/macros.rb
index 1e0f9a6b7e..5915793f8a 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
@@ -30,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)
@@ -38,6 +45,11 @@ 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"
+
+    scope :"with_attached_#{name}", -> { includes("#{name}_attachments": :blob) }
+
     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..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 ||= 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,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 80e4cb6234..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 ||= 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,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/lib/active_storage/migration.rb b/lib/active_storage/migration.rb
index c56e7a1786..e843c1b630 100644
--- a/lib/active_storage/migration.rb
+++ b/lib/active_storage/migration.rb
@@ -14,15 +14,14 @@ 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, :name, :blob_id ], name: "index_active_storage_attachments_uniqueness", unique: true
     end
   end
 end