`ActiveRecord::Base.attributes_for_inspect` should default to `:all` #52753

levicole · 2024-08-30T20:35:24Z

Motivation / Background

After updating to Rails 7.2, we noticed that attributes were missing when inspecting AR objects in a production-like console. We found #49765. After discussing this with @rafaelfranca and the author of the PR, @andrewn617, we came to the conclusion that this should default to :all.

Detail

This PR changes the default of attributes_for_inspect to :all. There's a commit on this branch that also updated the Rails 7.2 new defaults initializer, but I removed that as it's not on main and I'm unsure how to handle that.

Additional information

This PR is going off of a discussion with @rafaelfranca. It's possible there is more to change here to make this work properly, and I apologize if thats the case. Happy to make more updates. I'm curious if, since we want new Rails applications to have this default set to true, if the production.rb environment file should be updated to include the configuration change.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Unrelated changes 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.

andrewn617 · 2024-08-30T20:44:44Z

railties/lib/rails/application/configuration.rb

@@ -291,6 +291,7 @@ def load_defaults(target_version)
            active_record.marshalling_format_version = 7.1
            active_record.run_after_transaction_callbacks_in_order_defined = true
            active_record.generate_secure_token_on = :initialize
+            active_record.attributes_for_inspect = :all


Actually, I think we want this in the 7.2 (line 324) clause of this case statement, as this method will not exist in 7.1.

Got it! I also see this caused a linter issue by adding this. Working out how to fix that.

guides/source/configuring.md

fatkodima · 2024-08-31T07:55:40Z

Please, keep only 1 commit in the PR.

Co-authored-by: Petrik de Heus <petrik@deheus.net>

skipkayhil

Thanks for working on this!

To maintain backwards compatibility with < 7.2, we'll need the default to be :all, not just when load_defaults 7.2. I believe that change needs to be in activerecord/lib/active_record/core.rb.

skipkayhil · 2024-08-31T23:17:38Z

railties/lib/rails/application/configuration.rb

@@ -336,6 +336,7 @@ def load_defaults(target_version)
          if respond_to?(:active_record)
            active_record.postgresql_adapter_decode_dates = true
            active_record.validate_migration_timestamps = true
+            active_record.attributes_for_inspect = :all


Suggested change

active_record.attributes_for_inspect = :all

active_record.attributes_for_inspect = [:id]

This one should be the new default that apps can opt into when they load_defaults 7.2 after upgrading.

(The line may have to be more complicated than this... maybe
[:id] unless Rails.env.local? or something similar?)

skipkayhil · 2024-08-31T23:18:46Z

guides/source/configuring.md

+
+| Starting with version | The default value is |
+| --------------------- | -------------------- |
+| 7.2                   | :all                 |


Suggested change

| 7.2 | :all |

| (original) | :all |

| 7.2 | [:id] |

(or whatever the :id value ends up being)

skipkayhil · 2024-08-31T23:19:00Z

guides/source/configuring.md

@@ -66,6 +66,7 @@ Below are the default values associated with each target version. In cases of co
 #### Default Values for Target Version 7.2

 - [`config.active_job.enqueue_after_transaction_commit`](#config-active-job-enqueue-after-transaction-commit): `:default`
+- [`config.active_record.attributes_for_inspect`](#config-active-record-attributes-for-inspect): `:all`


Suggested change

- [`config.active_record.attributes_for_inspect`](#config-active-record-attributes-for-inspect): `:all`

- [`config.active_record.attributes_for_inspect`](#config-active-record-attributes-for-inspect): `[:id]`

andrewn617 · 2024-09-01T15:08:40Z

Yes @skipkayhil is right, the default in load_defaults should be [:id] unless Rails.env.local? (and in the framework defaults as well).

But, we want existing apps to not get that by default, so we need to also change the default for the attribute in AR base to be :all. (here). Then 7.1 apps upgrading to 7.2 will get :all and have to opt in with load_defaults or by enabling the new framework default.

Additionally, I believe that this initializer will now serve no purpose, since :all will be the default and [:id] will be set in production by the framework defaults. So it can be removed.

For an example of this being done right you can take a look at this PR which introduces ActiveJob#enqueue_after_transaction_commit. The class_attribute default is set to :never to preserve existing behaviour of upgrading apps, but the default in load_defaults and the framework defaults is :default which is what we want for new apps.

Sorry for missing this detail on friday @levicole !

rafaelfranca · 2024-09-02T15:05:36Z

Implementing this properly is very hard. When load_defaults is executed, consider_all_requests_local isn't set yet, so we need the initializer. But even with the initializer, because load_defaults set the config, the initializer doesn't run. So in order to make this work, we can't allow this to be configured in development and test globally.

I still think the best solution here is the custom inspector for console.

rafaelfranca · 2024-09-02T15:21:46Z

Even the custom inspector for the console will not help much because the pretty print format also uses the attributes_for_inspect config and pp object will just return [:id] in production.

I think I'll decide that we should not mix the two concerns on this implementation. The only thing I care about right now is to not inadvertently inspect records with all attributes in production when calling Hash#to_s since this can take down applications (depending on your timeout), and people would not even understand why.

Allowing people to configure how to see things in console and debug is a nice to have, but I'd prefer to not change it if it makes things harder to debug. So changing inspect in production by default to :id, outside of the console makes sense to me. Changing pp doesn't since nobody would by accident call pp in a production request.

levicole · 2024-09-02T15:54:15Z

Even the custom inspector for the console will not help much because the pretty print format also uses the attributes_for_inspect config and pp object will just return [:id] in production.

I think I'll decide that we should not mix the two concerns on this implementation. The only thing I care about right now is to not inadvertently inspect records with all attributes in production when calling Hash#to_s since this can take down applications (depending on your timeout), and people would not even understand why.

Allowing people to configure how to see things in console and debug is a nice to have, but I'd prefer to not change it if it makes things harder to debug. So changing inspect in production by default to :id, outside of the console makes sense to me. Changing pp doesn't since nobody would by accident call pp in a production request.

@rafaelfranca do you want me to just close this then?

rafaelfranca · 2024-09-02T16:06:11Z

Let's see what Andrew has to say. I'll update this PR and merge so we don't lose your work.

andrewn617 · 2024-09-03T14:04:46Z

@rafaelfranca Instead of trying to change the framework defaults to get precisely the settings we want, why don't we just set attributes_to_inspect to :all in the console in production only. I think we can do it with an .irbrc file that we generate for the application, or perhaps with a hook in active record. But basically this:

if Rails.env.production?
  ActiveSupport.on_load(:active_record) { ActiveRecord::Base.attributes_for_inspect = :all }
end

That even still allows to the user to overwrite it further if they actually do want different attributes in production.

Then we don't have to deal with adding the new framework default at this point, which tbh I am still not convinced is needed since this is a performance fix and shouldn't be noticed in production (outside debugging).

What do you think?

andrewn617 · 2024-09-04T19:28:21Z

@levicole I discussed with Rafael offline and decided we should set :all by default and [:id] in production from production.rb. I also fixed the issue with the console, so it will still be :all in the console in production. So I put those commits all on a branch and opened #52801 Thanks for your help with this 👍

levicole · 2024-09-04T19:47:01Z

@andrewn617 Awesome! Thanks for YOUR help! I'll close this!

rails-bot bot added the railties label Aug 30, 2024

andrewn617 reviewed Aug 30, 2024

View reviewed changes

rails-bot bot added the docs label Aug 30, 2024

n-rodriguez reviewed Aug 30, 2024

View reviewed changes

guides/source/configuring.md Outdated Show resolved Hide resolved

p8 reviewed Aug 31, 2024

View reviewed changes

guides/source/configuring.md Outdated Show resolved Hide resolved

attributes_for_inspect should default to :all

43dfd8b

Co-authored-by: Petrik de Heus <petrik@deheus.net>

levicole force-pushed the inspect-all-by-defualt branch from bef007a to 43dfd8b Compare August 31, 2024 16:58

p8 linked an issue Aug 31, 2024 that may be closed by this pull request

Only id is shown when inspecting records in Rails console in production #52728

Closed

skipkayhil reviewed Aug 31, 2024

View reviewed changes

rafaelfranca modified the milestones: 7.2.2, 8.0.0 Sep 2, 2024

fatkodima mentioned this pull request Sep 4, 2024

Set attributes_for_inspect to [:id] in the production environment only #52801

Merged

4 tasks

levicole closed this Sep 4, 2024

levicole deleted the inspect-all-by-defualt branch September 4, 2024 19:47

	active_record.attributes_for_inspect = :all
	active_record.attributes_for_inspect = [:id]

	- [`config.active_record.attributes_for_inspect`](#config-active-record-attributes-for-inspect): `:all`
	- [`config.active_record.attributes_for_inspect`](#config-active-record-attributes-for-inspect): `[:id]`

ActiveRecord::Base.attributes_for_inspect should default to :all #52753

ActiveRecord::Base.attributes_for_inspect should default to :all #52753

Uh oh!

Conversation

levicole commented Aug 30, 2024

Motivation / Background

Detail

Additional information

Checklist

Uh oh!

andrewn617 Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

levicole Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fatkodima commented Aug 31, 2024

Uh oh!

skipkayhil left a comment

Choose a reason for hiding this comment

Uh oh!

skipkayhil Aug 31, 2024

Choose a reason for hiding this comment

Uh oh!

skipkayhil Aug 31, 2024

Choose a reason for hiding this comment

Uh oh!

skipkayhil Aug 31, 2024

Choose a reason for hiding this comment

Uh oh!

andrewn617 commented Sep 1, 2024

Uh oh!

rafaelfranca commented Sep 2, 2024

Uh oh!

rafaelfranca commented Sep 2, 2024

Uh oh!

levicole commented Sep 2, 2024

Uh oh!

rafaelfranca commented Sep 2, 2024

Uh oh!

andrewn617 commented Sep 3, 2024

Uh oh!

andrewn617 commented Sep 4, 2024

Uh oh!

levicole commented Sep 4, 2024

Uh oh!

Uh oh!

`ActiveRecord::Base.attributes_for_inspect` should default to `:all` #52753

`ActiveRecord::Base.attributes_for_inspect` should default to `:all` #52753