Fix defining alias_attribute for STI classes with abstract class in the inheritance chain

[fatkodima]

Fixes #50154.

[nvasilevski]

I'm a little hesitant about this particular fix. It feels a bit hacky as it doesn't address the original problem - we are trying to add a new alias in the middle of generating alias methods which at least on paper could lead to a bug where we won't define methods for some aliases because we used a duped version of the list and definitions were added to the original one.

In our case alias_attribute :id_value, :id is the line that defines the attribute in the middle of the iteration. So I think a proper long-term fix should focus around the id_value alias.
There is a quick fix that I see as more suitable - make sure that schema is loaded before entering the loop to define alias methods. This way we will ensure alias_attribute :id_value is registered before we start defining alias methods and we won't enter load_schema method since it will be already loaded. We can achieve this by explicitly calling load_schema right before the loop:

rails/activerecord/lib/active_record/attribute_methods.rb

Line 66 in 808ea30

def generate_alias_attributes # :nodoc:

Like:

      def generate_alias_attributes # :nodoc:
        load_schema
        superclass.generate_alias_attributes unless superclass == Base
        ....

This will ensure the schema is loaded before we enter the loop to define methods which require schema to be available anyway. We will just load it a bit earlier.

Even though I like this solution more, it still makes attribute_methods "dependent" or at least aware of the load_schema concept and I'd prefer alias methods to never know about existence of schema and related process.

So I think there is a better approach we could look into - decouple alias_attribute :id_value definition from the load_schema method. load_schema method should only care about loading the schema and nothing more, especially it shouldn't recursively try to load_schema again. Though I can't immediately tell about how it can be decoupled but I feel like that load_schema should not be doing anything other than loading schema 😕

For example we could try defining it "when id attribute is being defined - define id_value alias", which roughly could mean adding something like:
alias_attribute(:id_value, :id) if attribute_names.include?("id")
to define_attribute_methods

rails/activerecord/lib/active_record/attribute_methods.rb

Line 125 in 808ea30

def define_attribute_methods # :nodoc:

(might be better to add it to define_attribute_method(attr_name) directly to check for == equality instead of include? )

[fatkodima]

Agreed. Thanks for more investigation!

[nvasilevski]

👍 Thank you!

I do believe that finding a different place for the id_value alias definition is a nicer fix as it decouples the logic from schema load and moves it closer to attributes logic where it looks much more relevant.

Also not concerned about the performance of include?("id") as we will only execute it once