Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
easydatawarehousing opened this issue Dec 8, 2023 · 2 comments

Comments

@easydatawarehousing
Copy link

When upgrading an app from Rails 7.0 to Rails 7.1 I'm getting these warnings:

DEPRECATION WARNING: MyModel model aliases `t`, but `t` is not an attribute.
Starting in Rails 7.2, alias_attribute with non-attribute targets will raise.

The attribute I want to alias is defined in an Active Record Store.
This behavior was introduced in this PR 48972.

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "activerecord", "~> 7.1.2"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :my_models, force: true do |t|
    t.string :properties
  end
end

class MyModel < ActiveRecord::Base
  store :properties, accessors: [:t], coder: JSON
  alias_attribute :title, :t
end

class BugTest < Minitest::Test
  def test_alias_attribute
    my_model = MyModel.create!(title: "My title")
    assert_equal my_model.title, "My title"
    assert_equal my_model.t, "My title"
  end
end

Expected behavior

No warnings are given.

Actual behavior

-- create_table(:my_models, {:force=>true})
Run options: --seed 9361
# Running:

DEPRECATION WARNING: MyModel model aliases `t`, but `t` is not an attribute. Starting in Rails 7.2, alias_attribute with non-attribute targets will raise. Use `alias_method :title, :t` or define the method manually.

Finished in 0.004296s, 232.7610 runs/s, 465.5219 assertions/s.
1 runs, 2 assertions, 0 failures, 0 errors, 0 skips

System configuration

Rails version: 7.1.2

Ruby version: 3.2.2

Possible solutions

  1. Change the test in ActiveRecord::AttributeMethods::ClassMethods to include attributes defined in stores (preferred)
  2. Add a method like alias_store_attribute which adds getter and setter methods for the aliased name
@nvasilevski
Copy link
Contributor

This is a bit tricky. The deprecation should be correct and aliasing AR store attributes should be broken once we drop the deprecation because store attributes implement direct reader&writer and don't go through the attribute(:attr_name) method to get the value

define_method(accessor_key) do
read_store_attribute(store_attribute, key)

At the same time store attributes mimic Active Model attributes by implementing most (all?) of the related methods

define_method("#{accessor_key}_changed?") do

So just silencing the deprecation won't work, in order to fix this we need to either change AR store or provide a new helper to define an AR store attribute alias (suggestion n2) but unfortunately none of these changes can be back-ported so we may need to consider delaying the removal of this deprecation if aliasing AR store attributes is essential and must continue to work.

@easydatawarehousing
Copy link
Author

easydatawarehousing commented Dec 12, 2023

I'm not very familiar with AR internals. But I figured that alias_attribute only defines some methods to access an existing store attribute so nothing bad would happen. If the changes in Rails 7.2 would break this particular situation then just suppressing the deprecation warning is obviously not going to work.

A bit of background why I'm using these aliases. I have some attributes in an AR store where the proper name is quite long (like: 'general_product_category') and the table has a lot of records (> 100M). Just using a single character as the attribute name saves a lot of storage space for such a large table. This is certainly true when using a text or json database field and probably true when using a (postgres) jsonb field.

For me having aliases on store attributes is essential. But on the other hand easy to work around by adding a getter and a setter for each attribute (using define_method). Basically suggestion # 2 in a model concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants