Improve ActiveStorageLoader example for STI models

[letiesperon]

Update ActiveStorageLoader to Support STI Models

Description:

This pull request updates the ActiveStorageLoader example to make it smarter so that it better supports Single Table Inheritance (STI) models.

Long context:

Previously, the ActiveStorageLoader class assumed that the record_type (the model that has the attachment) was always the class where the has_one_attached or has_many_attached was defined. However, in an STI setup, the attachment could be defined on any of the ancestor classes of the model, not just the model itself.

For example, consider the following STI setup:

class Vehicle < ApplicationRecord
  has_one_attached :document
end

class Car < Vehicle; end
class Truck < Vehicle; end

In this case, both Car and Truck models inherit the document attachment from the Vehicle parent class. If we were to use the ActiveStorageLoader with record_type set to "Car" or "Truck", it would not find the attachment, because it's defined on the Vehicle parent class, so the record_type is actually "Vehicle".

The following would NOT load the attachment:

Loaders::ActiveStorageLoader.for("Car", "document").load(id)

In plain sight this might seem easily avoidable by simply calling the ActiveStorageLoader with Vehicle. However, there are times where we call the loader dynamically from a more complex context so it's better to make the ActiveStorageLoader so that it can be called with a subclass and it would still find the attachment.

For instance, you might have an extension to load the attachment blob url:

# Usage example:
# 
#  field :document_url, String, null: true, extension: Extensions::AttachmentUrlField, attachment: :document

module Extensions
  class AttachmentUrlField < GraphQL::Schema::FieldExtension
    attr_reader :attachment_name

    def apply
      @attachment_name = options&.[](:attachment) || field.original_name.to_s.sub(/_url$/, '')
    end

    def resolve(object:, arguments:, **_rest)
      record = object.object # Because object is a `Field` instance, not the ActiveRecord.
      id = record.id
      class_sym = record.class.name.to_sym # <- !!! This would be "Car" instead of "Vehicle"
      variant = arguments[:variant]

      Loaders::ActiveStorageLoader.for(class_sym, attachment_name).load(id).then do |value|
        next if value.nil?

        variant_blob = variant ? value.variant(variant.to_sym) : value
        Rails.application.routes.url_helpers.url_for(variant_blob)
      end
    end
  end
end

---

Solution

To address this, this PR updates the ActiveStorageLoader class to get all ancestor classes of record_type that are descendants of ActiveRecord::Base. This ensures that we correctly handle attachments for all possible record_types, whether it's the model itself or any of its ancestors in the STI hierarchy.

[swalkinshaw]

Thanks for the detailed issue.

In plain sight this might seem easily avoidable by simply calling the ActiveStorageLoader with Vehicle. However, there are times where we call the loader dynamically from a more complex context so it's better to make the ActiveStorageLoader so that it can be called with a subclass and it would still find the attachment.

I understand you want to be able to call either of these and have it work:

• Loaders::ActiveStorageLoader.for("Car", "document").load(id)
• Loaders::ActiveStorageLoader.for("Vehicle", "document").load(id)

My question is if a ActiveStorage::Attachment will ever be saved to the database with the STI subclass types. Would it always be Vehicle? Or could record_type be any of the subclasses?

[letiesperon]

@swalkinshaw Thanks for looking at it!

You're right, that's a good question.

My understanding is that in the context of STI and polymorphic associations combined, when associating a record from an STI model (e.g., Car or Truck) with another model through a polymorphic association, Rails by default stores the base class of the STI hierarchy in the polymorphic <association>_type column if you're interacting with the base class directly. However, if you're working with the subclass directly (like Car.find(1).documents.create(...)), it will store the subclass's name ("Car" or "Truck") in the <association>_type column.

But, in the context of ActiveStorage, I can't think of any case in which the subclass would be stored as the record_type unless either:

1. someone creates the attachment manually and not by using some_car.document.attach(...) .
2. someone updates the attachment records manually for whatever reason
3. someone modifies or customizes the configuration of Active Storage to alter its default behavior to save the subclass's name in the record_type column.

But it's not obvious to me why someone would do that. However, I don't think that checking for all the ancestor classes hurt.

What are you thinking?

[swalkinshaw]

I was thinking it would really simplify the logic and remove the need for the ancestors_record_types complexity if you were guaranteed to always not have the subclass stored.

Regardless, I don't know enough about the nuances of STI to really review this code. I'd be okay with including an STI example however I think it should be a separate file to keep the existing one simpler as a starting point for people.

[letiesperon]

@swalkinshaw

it should be a separate file to keep the existing one simpler

Absolutely, sorry for the delay. It's addressed now!

[swalkinshaw]

Thank you 🎉