Ensure target is an attribute for `alias_attribute` #48972

ipc103 · 2023-08-18T17:09:56Z

Motivation / Background

We ran into a few cases at GitHub where we were using alias_attribute incorrectly to target non-attribute methods. Previously, this "worked", because alias_attribute just forwarded the method call, but this is no longer the case.

In most cases, this raised a deprecation warning, although the message was a bit unclear. The original deprecation warning was intended to look for manually defined alias methods that won't be forwarded in the future.

If the target happens to be a generated attribute method, no deprecation is raised. For example, assuming a model has a title attribute, alias_attribute :saved_title, :title_in_database was using the new behavior, because we only raise the deprecation if the target method is not an attribute method.

Detail

This Pull Request adds a check to alias_attribute_method_definition to ensure that you actually are targeting an attribute, and raises a deprecation warning if not. In the future, I think we should raise if you try to use alias_attribute with a non-attribute method.

Note that the following code will still behave correctly, because has_attribute? will forward the aliases to the far target automatically.

class Topic
  alias_attribute :heading, :title
  alias_attribute :subject, :heading
end

This correctly issues a deprecation warning for generated attribute methods, and also makes the warning for any non-attribute methods more clear.

Additional information

We originally tried to make this warning even more specific in #48929, but the logic got kind of complicated. Issuing one warning for this case felt like a good compromise.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

nvasilevski

Thank you! And sorry for the troubles with a misleading message

Just a few non-blocking suggestions on tests

activerecord/test/cases/attribute_methods_test.rb

ipc103 · 2023-08-18T18:46:37Z

Thank you! And sorry for the troubles with a misleading message

Thank you for the suggestions ❤️ And no worries - we were doing some wacky stuff, happy that we're able to remove these usages 🙏

nvasilevski · 2023-08-21T15:52:29Z

activerecord/lib/active_record/attribute_methods.rb

@@ -90,7 +90,18 @@ def alias_attribute_method_definition(code_generator, pattern, new_name, old_nam
        manually_defined = method_defined && self.instance_method(target_name).owner != generated_attribute_methods
        reserved_method_name = ::ActiveRecord::AttributeMethods.dangerous_attribute_methods.include?(target_name)

-        if manually_defined && !reserved_method_name
+        if !has_attribute?(old_name)


In an unrelated conversation I realized that this is something that will disallow alias attribute definitions in an abstract class. For example in an inheritance chain like MyModel < AbstractClassA < AbstractClassB < ApplicationRecord < ActiveRecord::Base alias_attribute defined in abstract classes will most likely raise a deprecation or even fail since these abstract classes have no attributes while it used to be possible to define alias attributes on abstract classes and it was a valid use-case

So here we should most likely ignore if the definition is happening on an abstract class as we can't know if the attribute exists on a concrete class inherited from the abstract class

Nice, thanks for catching that. I'll add a test for that case and add that fix.

I incorporated the changes from main, which included #48991, and added the check for the abstract_class, but it looks like this change broke some other behavior in a different test file. I'm going to investigate why this broke...

byroot · 2023-08-25T10:03:21Z

The CI errors look legit:

NumericalityValidationTest#test_aliased_attribute:
RuntimeError: can't add a new key into hash during iteration
    /rails/activemodel/lib/active_model/attribute_methods.rb:206:in `alias_attribute'
    /rails/activerecord/lib/active_record/attribute_methods.rb:54:in `alias_attribute'
    /rails/activerecord/lib/active_record/model_schema.rb:621:in `block in load_schema!'

ipc103 · 2023-08-25T12:26:27Z

Yep, sorry for the delay here. I have a couple of TODOs here:

I need to incorporate the changes from main to get Allow parent being the owner of an alias attribute method #48996
I might need to make some additional adjustments to the implementation here to account for the fact that we're always generating an id_value alias for every model: Use alias_attribute to provide id_value alias for id attribute #48930, but I'm not sure yet.

If it's helpful, I can close this out while I make those changes and re-open when it's ready for review again?

byroot · 2023-08-25T13:06:31Z

If it's helpful, I can close this out while I make those changes and re-open when it's ready for review again?

Nah, no worries, it can stay open it's fine.

We ran into a few cases at GitHub where we were using alias_attribute incorrectly and the new behavior either didn't warn or raised an unclear deprecation warning. This attempts to add clarity to the deprecation reason when you try to alias something that isn't actually an attribute. Previously, trying to alias a generated attribute method, such as `attribute_in_database`, would still hit `define_proxy_call`, because we were only checking the owner of the target method. In the future, we should probably raise if you try to use alias_attribute for a non-attribute. Note that we don't raise the warning for abstract classes, because the attribute may be implemented by a child class. We could potentially figure out a way to raise in these cases as well, but this hopefully is good enough for now. Finally, I also updated the way we're setting `local_alias_attributes` when `alias_attribute` is first called. This was causing problems since we now use `alias_attribute` early in the `load_schema!` call for some models: https://buildkite.com/rails/rails/builds/98910

ipc103 · 2023-08-30T17:22:02Z

activemodel/lib/active_model/attribute_methods.rb

@@ -370,6 +370,8 @@ def local_attribute_aliases # :nodoc:
      end

      private
+        attr_writer :local_attribute_aliases # :nodoc:


Added to resolve these failures - https://buildkite.com/rails/rails/builds/98910#018a198e-03ae-4d10-820e-67f72d4b14c0. Also matches the signature for attribute_aliases, which might be better anyway.

ipc103 · 2023-08-30T17:23:46Z

@nvasilevski Apologies for the delay here, but this is ready for review now! I had to make a couple of minor tweaks to account for the changes from #48930

rails-bot bot added the activerecord label Aug 18, 2023

ipc103 mentioned this pull request Aug 18, 2023

Refine the alias_attribute warning message to be more specific #48929

Closed

4 tasks

eileencodes requested a review from nvasilevski August 18, 2023 17:22

nvasilevski approved these changes Aug 18, 2023

View reviewed changes

activerecord/test/cases/attribute_methods_test.rb Outdated Show resolved Hide resolved

activerecord/test/cases/attribute_methods_test.rb Show resolved Hide resolved

ipc103 force-pushed the alias-attribute-additional-warnings branch from 02eaf46 to 22f8548 Compare August 18, 2023 18:45

nvasilevski reviewed Aug 21, 2023

View reviewed changes

ipc103 force-pushed the alias-attribute-additional-warnings branch from 22f8548 to 078235b Compare August 21, 2023 19:23

ipc103 mentioned this pull request Aug 22, 2023

DEPRECATION WARNING: AbstractStiPost model aliases #48994

Closed

ipc103 force-pushed the alias-attribute-additional-warnings branch from 078235b to eae26ca Compare August 30, 2023 17:20

ipc103 commented Aug 30, 2023

View reviewed changes

ipc103 requested a review from nvasilevski August 30, 2023 17:22

rails-bot bot added the activemodel label Aug 30, 2023

byroot merged commit 967b8d4 into rails:main Aug 30, 2023
3 of 4 checks passed

akhilgkrishnan mentioned this pull request Oct 19, 2023

alias_attribute raises a deprecation warning. Rails 7.1 #49699

Closed

easydatawarehousing mentioned this pull request Dec 8, 2023

alias_attribute gives a deprecation warning for store attributes in Rails 7.1 #50303

Open

fdocr mentioned this pull request Dec 20, 2023

alias_attribute Rails deprecation warnings spree/spree#11977

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure target is an attribute for `alias_attribute` #48972

Ensure target is an attribute for `alias_attribute` #48972

ipc103 commented Aug 18, 2023

nvasilevski left a comment

ipc103 commented Aug 18, 2023

nvasilevski Aug 21, 2023

ipc103 Aug 21, 2023

ipc103 Aug 21, 2023

byroot commented Aug 25, 2023

ipc103 commented Aug 25, 2023

byroot commented Aug 25, 2023

ipc103 Aug 30, 2023

ipc103 commented Aug 30, 2023

Ensure target is an attribute for alias_attribute #48972

Ensure target is an attribute for alias_attribute #48972

Conversation

ipc103 commented Aug 18, 2023

Motivation / Background

Detail

Additional information

Checklist

nvasilevski left a comment

Choose a reason for hiding this comment

ipc103 commented Aug 18, 2023

nvasilevski Aug 21, 2023

Choose a reason for hiding this comment

ipc103 Aug 21, 2023

Choose a reason for hiding this comment

ipc103 Aug 21, 2023

Choose a reason for hiding this comment

byroot commented Aug 25, 2023

ipc103 commented Aug 25, 2023

byroot commented Aug 25, 2023

ipc103 Aug 30, 2023

Choose a reason for hiding this comment

ipc103 commented Aug 30, 2023

Ensure target is an attribute for `alias_attribute` #48972

Ensure target is an attribute for `alias_attribute` #48972