Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that the _blob association is properly loaded when attaching ::One #35412

Merged

Conversation

@abhchand
Copy link
Contributor

@abhchand abhchand commented Feb 26, 2019

Background

I had first added these changes into a separate commit on #35390.

Was encouraged by @georgeclaghorn in this comment to open a separate PR for this change.

Overview

Consider a model with One and Many attachments configured:

class User < ActiveRecord::Base
  has_one_attached :avatar
  has_many_attached :highlights
end

One Attachment

After attaching One attachment (:avatar), we can see that the associated
_blob record (:avatar_blob) still returns as nil.

user.avatar.attach(blob)
user.avatar_attachment.present?  => true
user.avatar_blob.present?        => false    # Incorrect!

This is a false negative. It happens because after the attachment and blob
are built:

  1. The record already has its _blob association loaded, as nil
  2. the ::Attachment is associated with the record but the ::Blob only gets
    associated with the ::Attachment, not the record itself

In reality, the blob does in fact exist. We can verify this as follows:

user.avatar.attach(blob)
user.avatar_attachment.blob.present?    => true  # Blob does exist!

The fix in this change is to simply assign the ::Blob when assigning
the ::Attachment. After this fix is applied, we correctly observe:

user.avatar.attach(blob)
user.avatar_attachment.present?  => true
user.avatar_blob.present?        => true    # Woohoo!

Many Attachments

We don't see this issue with Many attachments because the _blob association
is already loaded as part of attaching more/newer blobs.

user.highlights.attach(blob)
user.highlights_attachments.any?    => true
user.highlights_blobs.any?          => true
activestorage/test/models/attached/many_test.rb Outdated Show resolved Hide resolved
activestorage/test/models/attached/one_test.rb Outdated Show resolved Hide resolved
@georgeclaghorn georgeclaghorn self-assigned this Feb 26, 2019
…d when attaching `::One`

Consider a model with `One` and `Many` attachments configured:

    class User < ActiveRecord::Base
      has_one_attached :avatar
      has_many_attached :highlights
    end

=== One Attachment

After attaching `One` attachment (`:avatar`), we can see that the associated
`_blob` record (`:avatar_blob`) still returns as `nil`.

    user.avatar.attach(blob)
    user.avatar_attachment.present?  => true
    user.avatar_blob.present?        => false    # Incorrect!

This is a false negative. It happens because after the attachment and blob
are built:

  1. The record already has its `_blob` association loaded, as `nil`
  2. the `::Attachment` is associated with the record but the `::Blob` only gets
    associated with the `::Attachment`, not the record itself

In reality, the blob does in fact exist. We can verify this as follows:

    user.avatar.attach(blob)
    user.avatar_attachment.blob.present?    => true  # Blob does exist!

The fix in this change is to simply assign the `::Blob` when assigning
the `::Attachment`. After this fix is applied, we correctly observe:

    user.avatar.attach(blob)
    user.avatar_attachment.present?  => true
    user.avatar_blob.present?        => true    # Woohoo!

=== Many Attachments

We don't see this issue with `Many` attachments because the `_blob` association
is already loaded as part of attaching more/newer blobs.

    user.highlights.attach(blob)
    user.highlights_attachments.any?    => true
    user.highlights_blobs.any?          => true
@abhchand abhchand force-pushed the correctly-load-blob-association branch from 298e204 to 32438ed Feb 26, 2019
@abhchand
Copy link
Contributor Author

@abhchand abhchand commented Feb 26, 2019

@georgeclaghorn - Merged both test assertions as suggested

@georgeclaghorn georgeclaghorn merged commit 7c95c52 into rails:master Feb 26, 2019
3 checks passed
@georgeclaghorn
Copy link
Contributor

@georgeclaghorn georgeclaghorn commented Feb 26, 2019

Thank you!

@abhchand abhchand deleted the correctly-load-blob-association branch Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants