Refactor implementation of attributes_for_inspect default values

This is a follow up to rails/rails@e50182a. Based on feedback we should not use consider_requests_local for this. Instead we will set it to :all by default and :id in production (in production.rb). This will also allow this feature to be opt-in for upgrading apps.

Additionally, we will always set it to :all when running the console in production, to avoid interfering with debugging.

Co-authored-by: Levi Cole <[email protected]>

Author: andrewn617

activerecord/lib/active_record/core.rb

@@ -104,7 +104,7 @@ def self.configurations
       class_attribute :shard_selector, instance_accessor: false, default: nil
 
       # Specifies the attributes that will be included in the output of the #inspect method
-      class_attribute :attributes_for_inspect, instance_accessor: false, default: [:id]
+      class_attribute :attributes_for_inspect, instance_accessor: false, default: :all
 
       def self.application_record_class? # :nodoc:
         if ActiveRecord.application_record_class
@@ -732,7 +732,7 @@ def inspect
 
     # Returns the full contents of the record as a nicely formatted string.
     def full_inspect
-      inspect_with_attributes(attribute_names)
+      inspect_with_attributes(all_attributes_for_inspect)
     end
 
     # Takes a PP and prettily prints this record to it, allowing you to get a nice result from <tt>pp record</tt>
@@ -824,7 +824,13 @@ def inspect_with_attributes(attributes_to_list)
       end
 
       def attributes_for_inspect
-        self.class.attributes_for_inspect == :all ? attribute_names : self.class.attributes_for_inspect
+        self.class.attributes_for_inspect == :all ? all_attributes_for_inspect : self.class.attributes_for_inspect
+      end
+
+      def all_attributes_for_inspect
+        return [] unless @attributes
+
+        attribute_names
       end
   end
 end

activerecord/lib/active_record/railtie.rb

@@ -69,6 +69,7 @@ class Railtie < Rails::Railtie # :nodoc:
         Rails.logger.broadcast_to(console)
       end
       ActiveRecord.verbose_query_logs = false
+      ActiveRecord::Base.attributes_for_inspect = :all if Rails.env.production?
     end
 
     runner do
@@ -435,15 +436,5 @@ class Railtie < Rails::Railtie # :nodoc:
         end
       end
     end
-
-    initializer "active_record.attributes_for_inspect" do |app|
-      ActiveSupport.on_load(:active_record) do
-        if app.config.consider_all_requests_local
-          if app.config.active_record.attributes_for_inspect.nil?
-            ActiveRecord::Base.attributes_for_inspect = :all
-          end
-        end
-      end
-    end
   end
 end

activerecord/test/cases/core_test.rb

@@ -17,9 +17,9 @@ def test_inspect_class
     assert_match(/^Topic\(id: integer, title: string/, Topic.inspect)
   end
 
-  def test_inspect_instance_includes_just_id_by_default
+  def test_inspect_instance
     topic = topics(:first)
-    assert_equal %(#<Topic id: 1>), topic.inspect
+    assert_equal %(#<Topic id: 1, title: "The First Topic", author_name: "David", author_email_address: "[email protected]", written_on: "#{topic.written_on.to_fs(:inspect)}", bonus_time: "#{topic.bonus_time.to_fs(:inspect)}", last_read: "#{topic.last_read.to_fs(:inspect)}", content: "Have a nice day", important: nil, binary_content: nil, approved: false, replies_count: 1, unique_replies_count: 0, parent_id: nil, parent_title: nil, type: nil, group: nil, created_at: "#{topic.created_at.to_fs(:inspect)}", updated_at: "#{topic.updated_at.to_fs(:inspect)}">), topic.inspect
   end
 
   def test_inspect_includes_attributes_from_attributes_for_inspect
@@ -107,7 +107,26 @@ def test_pretty_print_new
     actual = +""
     PP.pp(topic, StringIO.new(actual))
     expected = <<~PRETTY
-      #<Topic:0xXXXXXX id: nil>
+      #<Topic:0xXXXXXX
+       id: nil,
+       title: nil,
+       author_name: nil,
+       author_email_address: "[email protected]",
+       written_on: nil,
+       bonus_time: nil,
+       last_read: nil,
+       content: nil,
+       important: nil,
+       binary_content: nil,
+       approved: true,
+       replies_count: 0,
+       unique_replies_count: 0,
+       parent_id: nil,
+       parent_title: nil,
+       type: nil,
+       group: nil,
+       created_at: nil,
+       updated_at: nil>
     PRETTY
     assert actual.start_with?(expected.split("XXXXXX").first)
     assert actual.end_with?(expected.split("XXXXXX").last)
@@ -118,7 +137,26 @@ def test_pretty_print_persisted
     actual = +""
     PP.pp(topic, StringIO.new(actual))
     expected = <<~PRETTY
-      #<Topic:0x\\w+ id: 1>
+      #<Topic:0x\\w+
+       id: 1,
+       title: "The First Topic",
+       author_name: "David",
+       author_email_address: "[email protected]",
+       written_on: "2003-07-16 14:28:11\\.223300000 \\+0000",
+       bonus_time: "2000-01-01 14:28:00\\.000000000 \\+0000",
+       last_read: "2004-04-15",
+       content: "Have a nice day",
+       important: nil,
+       binary_content: nil,
+       approved: false,
+       replies_count: 1,
+       unique_replies_count: 0,
+       parent_id: nil,
+       parent_title: nil,
+       type: nil,
+       group: nil,
+       created_at: [^,]+,
+       updated_at: [^,>]+>
     PRETTY
     assert_match(/\A#{expected}\z/, actual)
   end

activerecord/test/cases/filter_attributes_test.rb

@@ -11,15 +11,12 @@ class FilterAttributesTest < ActiveRecord::TestCase
   fixtures :"admin/users", :"admin/accounts"
 
   setup do
-    @previous_attributes_for_inspect = ActiveRecord::Base.attributes_for_inspect
-    ActiveRecord::Base.attributes_for_inspect = :all
     @previous_filter_attributes = ActiveRecord::Base.filter_attributes
     ActiveRecord::Base.filter_attributes = [:name]
     ActiveRecord.use_yaml_unsafe_load = true
   end
 
   teardown do
-    ActiveRecord::Base.attributes_for_inspect = @previous_attributes_for_inspect
     ActiveRecord::Base.filter_attributes = @previous_filter_attributes
   end
 

railties/lib/rails/generators/rails/app/templates/config/environments/production.rb.tt

@@ -114,6 +114,9 @@ Rails.application.configure do
 
   # Do not dump schema after migrations.
   config.active_record.dump_schema_after_migration = false
+
+  # Only use :id for inspections in production.
+  config.active_record.attributes_for_inspect = [:id]
   <%- end -%>
 
   # Enable DNS rebinding protection and other `Host` header attacks.

railties/test/application/configuration_test.rb

@@ -4701,33 +4701,15 @@ class Post < ActiveRecord::Base
       assert_equal(:html4, Rails.application.config.dom_testing_default_html_version)
     end
 
-    test "sets ActiveRecord::Base.attributes_for_inspect to [:id] when config.consider_all_requests_local = false" do
-      add_to_config "config.consider_all_requests_local = false"
-
-      app "production"
-
-      assert_equal [:id], ActiveRecord::Base.attributes_for_inspect
-    end
-
-    test "sets ActiveRecord::Base.attributes_for_inspect to :all when config.consider_all_requests_local = true" do
-      add_to_config "config.consider_all_requests_local = true"
-
-      app "development"
-
-      assert_equal :all, ActiveRecord::Base.attributes_for_inspect
-    end
-
-    test "app configuration takes precedence over default" do
-      add_to_config "config.consider_all_requests_local = true"
+    test "app attributes_for_inspect configuration takes precedence over default" do
       add_to_config "config.active_record.attributes_for_inspect = [:foo]"
 
       app "development"
 
       assert_equal [:foo], ActiveRecord::Base.attributes_for_inspect
     end
 
-    test "model's configuration takes precedence over default" do
-      add_to_config "config.consider_all_requests_local = true"
+    test "model's attributes_for_inspect configuration takes precedence over default" do
       app_file "app/models/foo.rb", <<-RUBY
         class Foo < ApplicationRecord
           self.attributes_for_inspect = [:foo]