Get rid of `mattr_accessor` in `ActiveRecord::Core` #42445

casperisfine · 2021-06-10T15:07:49Z

See #42442 for full context.

In short class variables are slow, particularly in classes with many ancestors, and ActiveRecord::Base has over 60 ancestors.

Only a small handful of these really matter performance wise, however the idea is to migrate them all regardless for consistency and to ensure future contributors will follow the new pattern rather than add new mattr.

There are a few more in various Active Record concerns, I'll clean them up in a followup.

I'm opening mostly to get a green CI as the general principle was agreed by others.

Followup: rails#42442

Otherwise initializer files with `Rails.application.active_record.*` might break.

kamipo · 2021-06-11T05:22:38Z

activerecord/lib/active_record/railtie.rb

+          # So to preserve backward compatibility we copy the config a second time.
+          if ActiveRecord.respond_to?(setter)
+            ActiveRecord.send(setter, v)


Do we still need copying the config a first time in L215-L221?

rails/activerecord/lib/active_record/railtie.rb

Lines 212 to 237 in ba877bd

initializer "active_record.set_configs" do |app|

configs = app.config.active_record

configs.each do |k, v|

next if k == :encryption

setter = "#{k}="

if ActiveRecord.respond_to?(setter)

ActiveRecord.send(setter, v)

end

end

ActiveSupport.on_load(:active_record) do

configs.each do |k, v|

next if k == :encryption

setter = "#{k}="

# Some existing initializers might rely on Active Record configuration

# being copied from the config object to their actual destination when

# `ActiveRecord::Base` is loaded.

# So to preserve backward compatibility we copy the config a second time.

if ActiveRecord.respond_to?(setter)

ActiveRecord.send(setter, v)

else

send(setter, v)

end

end

end

Yes, it's a bit unfortunate, but some places where looking up AR::Base.some_config causing AR::Base autoloading and the on_load callback.

But now that they access AR.some_config they no longer trigger it.

I'm not particularly happy with this situation and am open to suggestion, because it's a bit unclear when configs are supposed to be accessible, and this reliance on autoload means referencing AR::Base early can cause the copy to happen earlier than expected.

Rails 7 has introduced a breaking change which removed mattr_accessor methods in place of module instance methods rails/rails#42445

rafaelfranca · 2021-06-14T19:27:56Z

Most of those methods were public API in ActiveRecord::Base. We need to deprecate them before removing.

byroot · 2021-06-14T20:54:58Z

Most of those methods were public API in ActiveRecord::Base. We need to deprecate them before removing.

Ok, no problem I'll add deprecated delegators for all of them first thing tomorrow.

Quick question, how do you know what's public API? I checked for these in https://api.rubyonrails.org/, and since they didn't show I assumed they were private API.

zzak · 2021-06-15T00:01:12Z

@byroot That is a bug in RDoc where these methods just don't show up in documentation. See ruby/rdoc#795.

rafaelfranca · 2021-06-15T15:30:57Z

All methods that had a :singleton-method: comment are public API.

mjobin-mdsol · 2021-08-16T04:15:34Z

activerecord/lib/active_record/core.rb

+
+      def reading_role # :nodoc:
+        ActiveSupport::Deprecation.warn(<<~MSG)
+          ActiveRecord::Base.reading_role is deprecated and will be removed in Rails 7.0.


does this means this will be released in 6.2 or 6.1.x ?

No it was a typo that got corrected later.

Introduced in rails/rails#42445 Example of a warning: ``` DEPRECATION WARNING: ActiveRecord::Base.writing_role is deprecated and will be removed in Rails 7.1. Use `ActiveRecord.writing_role` instead. (called from block (2 levels) in <top (required)> at /__w/discourse/discourse/spec/rails_helper.rb:304) ```

* DEV: Fix reading_role/writing_role deprecations Introduced in rails/rails#42445 Example of a warning: ``` DEPRECATION WARNING: ActiveRecord::Base.writing_role is deprecated and will be removed in Rails 7.1. Use `ActiveRecord.writing_role` instead. (called from block (2 levels) in <top (required)> at /__w/discourse/discourse/spec/rails_helper.rb:304) ``` * changelog * Add rails 7.0 to the ci matrix * reorder

since rails/rails#42445, this method has been changed from cattr_accessor to attr_accessor in ActiveRecord.

…to ActiveRecord - ref: rails/rails#42445 It also solved the following CI failure. - NoMethodError: undefined method `schema_format=' for ActiveRecord::Base:Class

related: #126 In Rails 7, the .schema_format= API is moved from ActiveRecord::Base to ActiveRecord - ref: rails/rails#42445 It also solved the following CI failure. ``` NoMethodError: undefined method `schema_format=' for ActiveRecord::Base:Class ```

rails-bot bot added the activerecord label Jun 10, 2021

Make default_timezone a module instance variable

c6e4dbe

Followup: rails#42442

casperisfine force-pushed the active-record-cattr-2 branch 2 times, most recently from 1890a30 to d85c5bd Compare June 10, 2021 16:01

byroot added 3 commits June 10, 2021 18:24

Make reading_role and writing_role module instance variables

8ae1339

Followup: rails#42442

Make warn_on_records_fetched_greater_than a module instance variable

d0e2b00

Followup: rails#42442

Make application_record_class a module instance variable

40f6bd0

Followup: rails#42442

casperisfine force-pushed the active-record-cattr-2 branch from d85c5bd to 427dbcb Compare June 10, 2021 16:25

byroot added 7 commits June 10, 2021 18:31

Get rid of the unused ActiveRecord::Base.sqlite3_production_warning

a50ff73

Make action_on_strict_loading_violation a module instance variable

e650427

Followup: rails#42442

Convert many ActiveRecord::Base class variable into instance variables

bcd6c0f

Followup: rails#42442

Make verbose_query_logs a module instance variable

03e569b

Followup: rails#42442

Make queues a module instance variable

9579214

Followup: rails#42442

Make maintain_test_schema a module instance variable

f4a3bf0

Followup: rails#42442

Move async_query confurations on the ActiveRecord module

85622b9

Followup: rails#42442

casperisfine force-pushed the active-record-cattr-2 branch from 427dbcb to 85622b9 Compare June 10, 2021 16:31

Copy config.active_record.* again when ActiveRecord::Base is loaded

ba877bd

Otherwise initializer files with `Rails.application.active_record.*` might break.

casperisfine force-pushed the active-record-cattr-2 branch from d602f66 to ba877bd Compare June 10, 2021 17:13

byroot merged commit 9bb2512 into rails:main Jun 10, 2021

kamipo reviewed Jun 11, 2021

View reviewed changes

casperisfine deleted the active-record-cattr-2 branch June 11, 2021 06:48

Tonkpils added a commit to github/github-ds that referenced this pull request Jun 11, 2021

Ensure GitHub::SQL works on Rails 7

4d5e894

Rails 7 has introduced a breaking change which removed mattr_accessor methods in place of module instance methods rails/rails#42445

Tonkpils mentioned this pull request Jun 11, 2021

Ensure GitHub::SQL works on Rails 7 github/github-ds#55

Merged

casperisfine mentioned this pull request Jun 15, 2021

Define deprecated delegators for the cattr that were moved out of AR::Base #42489

Merged

yahonda mentioned this pull request Jun 15, 2021

CI against Rails master fails since rails/rails#42445 activerecord-hackery/ransack#1229

Closed

yahonda mentioned this pull request Jun 16, 2021

Address "NoMethodError: undefined method `partial_writes?'" rsim/oracle-enhanced#2188

Merged

keeran mentioned this pull request Aug 2, 2021

Add Marginalia to Rails, via QueryLogs #42240

Merged

mjobin-mdsol reviewed Aug 16, 2021

View reviewed changes

CvX mentioned this pull request Jun 20, 2022

DEV: Fix reading_role/writing_role deprecations discourse/rails_failover#48

Merged

etiennebarrie mentioned this pull request Feb 24, 2023

Use the newer serialize signature of Rails 7.1 if available Shopify/maintenance_tasks#776

Merged

stefannibrasil added a commit to ConvertKit/impressionist that referenced this pull request Dec 26, 2023

ActiveRecord::Base is not accessible anymore

7722ad8

since rails/rails#42445, this method has been changed from cattr_accessor to attr_accessor in ActiveRecord.

otegami mentioned this pull request Mar 3, 2024

Adjust .schema_format= API Usage for Rails 7 Compatibility clear-code/redmine_full_text_search#128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of `mattr_accessor` in `ActiveRecord::Core` #42445

Get rid of `mattr_accessor` in `ActiveRecord::Core` #42445

casperisfine commented Jun 10, 2021

kamipo Jun 11, 2021 •

edited

casperisfine Jun 11, 2021

rafaelfranca commented Jun 14, 2021

byroot commented Jun 14, 2021

zzak commented Jun 15, 2021

rafaelfranca commented Jun 15, 2021

mjobin-mdsol Aug 16, 2021

byroot Aug 16, 2021

	initializer "active_record.set_configs" do \|app\|
	configs = app.config.active_record

	configs.each do \|k, v\|
	next if k == :encryption
	setter = "#{k}="
	if ActiveRecord.respond_to?(setter)
	ActiveRecord.send(setter, v)
	end
	end

	ActiveSupport.on_load(:active_record) do
	configs.each do \|k, v\|
	next if k == :encryption
	setter = "#{k}="
	# Some existing initializers might rely on Active Record configuration
	# being copied from the config object to their actual destination when
	# `ActiveRecord::Base` is loaded.
	# So to preserve backward compatibility we copy the config a second time.
	if ActiveRecord.respond_to?(setter)
	ActiveRecord.send(setter, v)
	else
	send(setter, v)
	end
	end
	end

Get rid of mattr_accessor in ActiveRecord::Core #42445

Get rid of mattr_accessor in ActiveRecord::Core #42445

Conversation

casperisfine commented Jun 10, 2021

kamipo Jun 11, 2021 • edited

Choose a reason for hiding this comment

casperisfine Jun 11, 2021

Choose a reason for hiding this comment

rafaelfranca commented Jun 14, 2021

byroot commented Jun 14, 2021

zzak commented Jun 15, 2021

rafaelfranca commented Jun 15, 2021

mjobin-mdsol Aug 16, 2021

Choose a reason for hiding this comment

byroot Aug 16, 2021

Choose a reason for hiding this comment

Get rid of `mattr_accessor` in `ActiveRecord::Core` #42445

Get rid of `mattr_accessor` in `ActiveRecord::Core` #42445

kamipo Jun 11, 2021 •

edited