TEST_CODEOWNERS/rails
3
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Fix ActiveStorage::Blob inverse association:

Browse Source 
- This is a fix needed to unblock
  https://github.com/rails/rails/pull/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:

  9c3ffab47c/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 #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`.
This commit is contained in:
Edouard CHIN 2024-01-19 03:45:01 +01:00
parent a9562e28c4
commit 82d4ad5da3
2 changed files with 6 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
activestorage/app/models/active_storage/attachment.rb
View File

@ -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

6
activestorage/lib/active_storage/attached/changes/create_one_of_many.rb
View File

@ -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