Skip to content

Commit

Permalink
Fix ActiveStorage::Blob inverse association:
Browse files Browse the repository at this point in the history
- This is a fix needed to unblock
  rails#50284,
  because Active Storage relies on a Active Record bug.

  The problem is best understood with a small snippet:

  ```
    blob = ActiveStorage::Blob.new

    ActiveStorage::Attachment.new(blob: blob)
    ActiveStorage::Attachment.new(blob: blob)

    # Currently:
    p blob.attachments #=> #<ActiveRecord::Associations::CollectionProxy []>

    # Once the Active Record bug is fixed:
    p blob.attachments #=> #<ActiveRecord::Associations::CollectionProxy [#<ActiveStorage::Attachment id: nil, name: nil, record_type: nil, record_id: nil, blob_id: nil, created_at: nil>, #<ActiveStorage::Attachment id: nil, name: nil, record_type: nil, record_id: nil, blob_id: nil, created_at: nil>]>

    # Trying to save the blob would result in trying to create 2 attachments which
    # fails because of unique constrainsts.
  ```

  ### Code path

  The real code path that does what the snippet above does is located here:

  https://github.com/rails/rails/blob/9c3ffab47c3bf59320ba08e9dafdb0275cf91a5a/activestorage/lib/active_storage/attached/many.rb#L52

  It's basically doing this:

  ```
    user.images.attach "photo1.png"
    # Initialize a Blob record and an Attachment

    user.images.attach "photo2.png"
    # Get the Blob from above, create another Attachment
    # Initialize a new Blob record and an new Attachment

    # rinse and repeat every time `attach` is called
  ```

  Basically each time we call `attach`, we grab the previous blobs that were attached
  (and that already have an Attachment record), and

  ### Solution

  - Explicitly set the `inverse_of`, so that it behaves as if rails#50284 is shipped
  - Don't build a new attachment for blob already having one.

  ### Tests

  I didn't add tests, the test suite is already well covered, adding the `inverse_of`
  without any changes breaks the test suite already. You can try by running
  for instance the `activestorage/test/models/attached/many_test.rb`.
  • Loading branch information
Edouard-chin committed Jan 19, 2024
1 parent a9562e2 commit 82d4ad5
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 2 deletions.
2 changes: 1 addition & 1 deletion activestorage/app/models/active_storage/attachment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class ActiveStorage::Attachment < ActiveStorage::Record
# :method:
#
# Returns the associated ActiveStorage::Blob.
belongs_to :blob, class_name: "ActiveStorage::Blob", autosave: true
belongs_to :blob, class_name: "ActiveStorage::Blob", autosave: true, inverse_of: :attachments

delegate_missing_to :blob
delegate :signed_id, to: :blob
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ module ActiveStorage
class Attached::Changes::CreateOneOfMany < Attached::Changes::CreateOne # :nodoc:
private
def find_attachment
record.public_send("#{name}_attachments").detect { |attachment| attachment.blob_id == blob.id }
if blob.persisted?
record.public_send("#{name}_attachments").detect { |attachment| attachment.blob_id == blob.id }
else
blob.attachments.find { |attachment| attachment.record == record }
end
end
end
end

0 comments on commit 82d4ad5

Please sign in to comment.