Updatable's ActiveStorage implementation broadcasts to multiple/wrong keys

[julianrubisch]

This issue emerged when I was first trying to integrate has_one_attached in the same way we have has_many_attached. I added has_one_attached to Updatable like so:

      def has_one_attached(name, **options)
        option = options.delete(:enable_updates)
        broadcast = option.present?
        result = super
        enrich_attachments_with_updates(name, option) if broadcast
        result
      end

After I wired up the User model:

class User < ApplicationRecord
  # ...
  has_one_attached :avatar, enable_updates: true
end

I got the following test failure from :

unexpected invocation: #<Mock:server>.broadcast("gid://dummy/Dugong/1:avatar", {:changed => ["id", "name", "record_type", "record_id", "blob_id", "created_at"]})

which is wrong simply because Dugong has no avatar AS association.

cc @andrewerlanger

[julianrubisch]

my guess is that due to the polymorphic nature of the ActiveStorage::Attachment association, simply all record_types are pinged?

[andrewerlanger]

@julianrubisch I think the problem here is that broadcast_for!(model, operation) (over here) is being called on the ActiveStorage::Attachment class, which has two registered collections (in the dummy app).

Specifically, we have:

# user.rb
has_one_attached :avatar, enable_updates: true

# dugong.rb
has_many_attached :images, enable_updates: true

This means that @registered_collections ends up looking something like the following for the ActiveStorage::Attachment class:

[{klass: User, name: :avatar, ...}, {klass: Dugong, name: :images, ...}]

When attaching an avatar to a user, for example, we then iterate through these @registered_collections and run the following line for each of them:

collection[:klass].cable_ready_update_collection(resource, collection[:name], model)

I haven't tested extensively, but I think we could likely get around this by adding another condition to the existing guard on L16:

next if resource.nil? || collection[:klass] != resource.class

That way we only end up calling collection[:klass].cable_ready_update_collection() if the class in question (User, to use the above example) aligns with the :klass specified in the collection.

Do you think that might work?

[julianrubisch]

Interesting! We could give this a try, yes!

[andrewerlanger]

Awesome :)

One thing to double-check here, although I think we should be okay, is that this doesn't have any unintended side effects for how we handle non-ActiveStorage collections (e.g. user.posts).

[julianrubisch]

Yeah it boils down on having good tests. I think I have a branch lying around somewhere, I'll take a look!

[leastbad]

I just wanted to follow up and ask if anything came of this investigation?

Otherwise, I'm going to close this ticket soon, okay?

[marcoroth]

We decided that we are going to close this one for now. @julianrubisch please feel free to re-open this one if you encounter this again or have some tests to upstream. Thank you!