rails: Documentation issue: StiPreload causes circular dependency issues

tl;dr

The way how to autoload as described in the “Single Table Inheritance” section in the “Autoloading and Reloading Constants” guide seems to be broken with Rails 7.

Steps to reproduce

Check out https://github.com/marvinthepa/sti_preload_broken. Follow the steps described in https://github.com/marvinthepa/sti_preload_broken/blob/main/README.md

This repo is a very minimal rails application that uses STI and the recommended way to load the STI hierarchy as described in https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#single-table-inheritance

Expected behavior

When loading the Dog STI model from the controller, the Animal parent model should load without error.

Actual behavior

An error is thrown.

uninitialized constant Dog

Breakdown on why this happens:

The controller references the Dog model, which zeitwerk tries to load.
As Dog < Animal, zeitwerk tries to load Animal
Animal contains a before_save hook, which is installed, eventually leading to ActiveSupport::Callbacks::__update_callbacks being called.
As of ffae3bd8d69, __update_callbacks calls Animal.descendants
StiPreload kicks in, and finds and entry with type == Dog in the db, and tries to load it
As zeitwerk is already in the process of loading Dog, it fails the above mentioned error message

I have only found a very stupid workaround, which I do not want to keep using:

      def descendants
        # HACK
        return super unless caller.grep(/__update_callbacks/).empty?

        preload_sti unless preloaded
        super   
      end

Maybe there is some way to ask zeitwerk which classes it currently tries to load and skip loading those? Anyway, I think the documentation should be updated with a StiPreload that also works with rails 7.

System configuration

Rails version: 7.0.3

Ruby version: 3.0.2p107

About this issue

Original URL
State: closed
Created 2 years ago
Comments: 21 (13 by maintainers)

Commits related to this issue

Fix for Rails 7 breaking recommended single-table-inheritance approach We were using an approach to Single-Table Inheritance recommended by Rails guide at: https://guides.rubyonrails.org/v7.0/autoloa... — committed to sciencehistory/kithe by jrochkind 2 years ago
Fix for Rails 7 breaking recommended single-table-inheritance approach We were using an approach to Single-Table Inheritance recommended by Rails guide at: https://guides.rubyonrails.org/v7.0/autoloa... — committed to sciencehistory/kithe by jrochkind 2 years ago

Most upvoted comments

@marvinthepa We need to do something with that section.

Off the top of my head, you could move the same idea to a to_prepare block (untested):

# config/initializers/eager_load_stis.rb
unless Rails.configuration.eager_load
  Rails.configuration.to_prepare do
    [Animal, Shape].each do |sti_root|
      # perform the query + constantize on sti_root
    end
  end
end

The difference is that this is not lazy, you always eager load, but at least it works (if it does 😃).

fxn on Jun 15, 2022

@fxn Thank you very much for that thorough explanation!

feliperaul on Aug 30, 2023

@feliperaul in general, my approach is to eager load what has to be eager loaded because that is what the semantics are aligned with.

So, while one may make an effort to make applications be as lean as possible when they boot, at some point you need to consider something as required during boot, and I do for these use cases.

Also, if the app is big and you run

bin/rails runner 'p $LOADED_FEATURES.size'

probably that will be several thousands. Do 10 files more matter? Probably not. So you do those 10 more, and live in peace because the STIs are loaded, the descendants are loaded, the APIs and semantics have no surprises, and there is nothing to mantain manually.

Having said that 😃, if you are still willing to mantain a registry, I believe a list of class names to be lazily constantized would work. Something like

class MyBase
  DESCENDANTS = %w(...)
  private_constant :DESCENDANTS

  def self.descendants
    @descendants ||= DESCENDANTS.map { _1.constantize }
  end
end

I wrote that off the top of my head, but I believe it should be OK.

fxn on Aug 29, 2023

@helmutrs Please don’t get blocked by this. My intention is to delete that technique from the docs for several reasons, but need to ponder it. To unblock, you could for example implement some of the other patterns, and as a last resort preload all constants one by one if you need fine control on order of evaluation (because in some scenarios you cannot just autoload as you walk a directory or database table, you might need to force certain order that respects dependencies).

fxn on Jan 25, 2023

Hey @p8:

The STI section in the Autoloading guide is exactly what is broken (IMVHO).

As I wrote in line 4 of the bug report:

This repo is a very minimal rails application that uses STI and the recommended way to load the STI hierarchy as described in https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#single-table-inheritance

Check https://github.com/marvinthepa/sti_preload_broken/blob/main/lib/sti_preload.rb and see if you can spot a difference apart from the hacky workaround that I added as a comment…

marvinthepa on Jun 9, 2022