Make the output of `ActiveRecord::Core#inspect` configurable. #49765

andrewn617 · 2023-10-24T14:20:10Z

Motivation / Background

This Pull Request has been created because we noticed that ActiveRecord::Core#inspect was taking >9s to run for some large objects in production. This is because the parameter filter is be executed for every attribute.

Detail

This Pull Request introduces configuration for ActiveRecord::Core#inspect. By default, inspect just returns the object with its id (eg #<Post id: 1>. The attributes to include can be configured with Post.attributes_to_inspect=. If you want to full output with all the attributes you can call Post.full_inspect.

Additional information

In my first draft I had a config to disable this, if you just always want to use full_inspect. But it seems easy enough just to do alias_method :inspect, :full_inspect on the model, or in ApplicationRecord if you want to disable it globally. So the config didn't seem worth it to me.

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.

andrewn617 · 2023-10-24T14:41:18Z

activerecord/test/models/developer.rb

@@ -132,6 +132,8 @@ class SymbolIgnoredDeveloper < ActiveRecord::Base
 class AuditLog < ActiveRecord::Base
  belongs_to :developer, validate: true
  belongs_to :unvalidated_developer, class_name: "Developer"
+
+  self.attributes_to_inspect = [:id, :message]


Fixes AssociationsTest#test_inspect_does_not_reload_a_not_yet_loaded_target

andrewn617 · 2023-10-24T14:43:16Z

activerecord/test/cases/core_test.rb

@@ -51,9 +65,9 @@ def test_inspect_class_without_table
    assert_equal "NonExistentTable(Table doesn't exist)", NonExistentTable.inspect
  end

-  def test_inspect_relation_with_virtual_field
-    relation = Topic.limit(1).select("1 as virtual_field")
-    assert_match(/virtual_field: 1/, relation.inspect)


I decided to remove this test. Relation#inspect calls inspect on each of the records. It seems unnatural that virtual columns would be included in attributes_to_expect so I didn't want to add that as a test case.

I think we could add a full_inspect method to Relation if we want to preserve this behaviour.

I think you can rewrite this test in a way that makes sure virtual attributes are inside the loaded record and can be inspected. You could call full_inspect in relation.first

adrianna-chang-shopify

Would there be value in adding a global config, e.g. config.active_record.attributes_to_inspect? If we opted for an :all option, might be useful to be able to set this across all Active Records in order to maintain the existing behaviour.

I think the argument is less strong if we stick with #full_inspect instead of an :all option, although you could make the argument that users may always want inspect to include certain fields (e.g. timestamps).

activerecord/lib/active_record/core.rb

adrianna-chang-shopify · 2023-10-24T16:05:35Z

activerecord/CHANGELOG.md

+    Post.first.inspect #=> "#<(Post id: 1)>"
+    ```
+
+    The attributes to be included in the out of `inspect` can be configured with


Suggested change

The attributes to be included in the out of `inspect` can be configured with

The attributes to be included in the output of `inspect` can be configured with

adrianna-chang-shopify · 2023-10-24T16:15:18Z

activerecord/test/cases/core_test.rb

-    assert_match(/virtual_field: 1/, relation.inspect)
+  def test_full_inspect_instance
+    topic = topics(:first)
+    assert_equal %(#<Topic id: 1, title: "The First Topic", author_name: "David", author_email_address: "david@loudthinking.com", 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.full_inspect


We could pull this out into a HEREDOC to make it easier to read?

rafaelfranca · 2023-10-26T19:29:21Z

activerecord/lib/active_record/core.rb

@@ -680,21 +683,18 @@ def connection_handler
      self.class.connection_handler
    end

-    # Returns the contents of the record as a nicely formatted string.
+    # Returns the attributes specified by #attributes_for_inspect as a nicely formatted string.


Suggested change

# Returns the attributes specified by #attributes_for_inspect as a nicely formatted string.

# Returns the attributes specified by <tt>.attributes_for_inspect</tt> as a nicely formatted string.

rafaelfranca · 2023-10-26T19:32:59Z

activerecord/test/cases/core_test.rb

@@ -51,9 +65,9 @@ def test_inspect_class_without_table
    assert_equal "NonExistentTable(Table doesn't exist)", NonExistentTable.inspect
  end

-  def test_inspect_relation_with_virtual_field
-    relation = Topic.limit(1).select("1 as virtual_field")
-    assert_match(/virtual_field: 1/, relation.inspect)


I think you can rewrite this test in a way that makes sure virtual attributes are inside the loaded record and can be inspected. You could call full_inspect in relation.first

p8 · 2023-10-26T20:00:01Z

activerecord/lib/active_record/core.rb

-          end
-        end.join(", ")
+      if attributes_for_inspect == :all
+        inspect_with_attributes(attribute_names)


Suggested change

inspect_with_attributes(attribute_names)

full_inspect

Argh, thanks. I think I left it like this when I was playing with a version that had inspect(:all) instead of full_inspect

p8 · 2023-10-26T20:07:10Z

We also already have a filter_attributes and filter_attributes= method.
It seems these have a lot of overlap with this feature:
https://github.com/rails/rails/blob/9289e5bbc3b5b5d43e477f00f5ce2d8446d657fc/activerecord/lib/active_record/core.rb#L308-L321

rafaelfranca · 2023-10-26T20:20:00Z

It doesn't overlap. filter_attributes tell active Record to show FILTERED for each attribute. We don't want to show anything, we don't even want to run the parameter filtering, or reading the value, etc.

BTW, maybe we should implement another improvement on this entire pipeline because it doesn't make sense to:

read the attribute

rails/activerecord/lib/active_record/attribute_methods.rb

Line 365 in 58bfc4d

value = _read_attribute(attr_name)

do transformations with that value

rails/activerecord/lib/active_record/attribute_methods.rb

Lines 495 to 501 in 58bfc4d

    
           inspected_value = if value.is_a?(String) && value.length > 50 
        
             "#{value[0, 50]}...".inspect 
        
           elsif value.is_a?(Date) || value.is_a?(Time) 
        
             %("#{value.to_fs(:inspect)}") 
        
           else 
        
             value.inspect 
        
           end

Just to show [FILTERED] and don't use the value for anything.

p8 · 2023-10-26T20:25:28Z

Ah sorry, my mistake. I thought it would bypass the parameter filter, but instead it creates a customer filter if filter_attributes is set.

Earlopain · 2023-10-27T10:49:01Z

Currently there doesn't seem to be a distinction between local/remote here, which the original issue mentions.

I have accidentally printed much more than I intended in production and because of slow connections that took quite a while. IRB had its autocomplete disabled in production because of similar reasons.

In development however I want to see everything. I know I can set the required config to do that, but it really should be the default in my opinion.

rafaelfranca · 2023-10-27T13:36:07Z

I do agree with that. Let's automatically set it to :all in active_record/railtie.rb when consider_all_requests_local is true, of course respecting any explicit config the user might set.

Add a test to make sure it is set to :all in test and development in the railties test as well.

andrewn617 · 2023-10-30T15:01:48Z

I added the tests to railties/test/application/configuration_test.rb based on conversation with @eileencodes about proper way to test railties 👍

andrewn617 · 2023-11-01T14:51:50Z

Okay, after playing with this on my app in the debugger, I added this functionality to pretty_print as well. Let me know if anyone has any objections, but it seems like the right thing to do to keep the behaviour consistent and it is nice to have the compact output in the debugger (if you have opted into it of course).

This also pushed me to do a bit of refactoring as well. Instead of needing to call attributes_for_inspect and check if it is set to :all and do something different (call attribute_names), I got rid of the instance_accessor and defined a attributes_for_inspect method on the instance that does the check for you.

This is in a state I am pretty happy with now.

activerecord/.gitignore

activerecord/CHANGELOG.md

rafaelfranca · 2023-10-27T18:59:39Z

activerecord/lib/active_record/railtie.rb

+    initializer "active_record.attributes_for_inspect" do |app|
+      ActiveSupport.on_load(:active_record) do
+        if app.config.consider_all_requests_local
+          ActiveRecord::Base.attributes_for_inspect = :all


This will override any config the user set, no?

I don't think so, since it is run in the :active_record load hook, it will be set before Base is inherited by anything. I tested in my app and it worked as expected. But I will add a test like you suggested in your other comment.

What I mean is setting config.active_record.attributes_for_inspect = [:uuid]

Ah sorry, wasn't thinking of that angle. Fixed it and added a test. Thanks!

rafaelfranca · 2023-10-27T19:00:50Z

activerecord/test/cases/railtie_test.rb

+require "cases/helper"
+require "active_support/testing/isolation"
+
+class RailtieTest < ActiveRecord::TestCase


This test should be in the railties framework folder, no here. We should create a full integration test, not an unit test.

Generate a Rails application, boot it in development, assert the config value is correct. There are other tests similar to that inside railties/test.

The test that is missing is to not use :all when used explicitly set config.active_record.attributes_for_inspect to something else.

This comment was out of date - I already move the tests to railties/test/application/configuration_test.rb. But thanks for the suggestion about the missing test case, I added that.

activerecord/test/database.yml

By default, calling `inspect` on a record will yield a formatted string including just the `id`. ```ruby Post.first.inspect #=> "#<Post id: 1>" ``` The attributes to be included in the output of `inspect` can be configured with `ActiveRecord::Core#attributes_for_inspect`. ```ruby Post.attributes_for_inspect = [:id, :title] Post.first.inspect #=> "#<Post id: 1, title: "Hello, World!">" ``` With the `attributes_for_inspect` set to `:all`, `inspect` will list all the record's attributes. ```ruby Post.attributes_for_inspect = :all Post.first.inspect #=> "#<Post id: 1, title: "Hello, World!", published_at: "2023-10-23 14:28:11 +0000">" ```

Rails changed what it prints in `inspect` method (rails/rails#49765). Only `id` is printed by default.

rails-bot bot added the activerecord label Oct 24, 2023

andrewn617 force-pushed the configurable-inspect branch 4 times, most recently from a419cda to 6c384a2 Compare October 24, 2023 14:40

andrewn617 commented Oct 24, 2023

View reviewed changes

andrewn617 mentioned this pull request Oct 24, 2023

ActiveRecord::Core#inspect should have an option to not show the attributes in production #49707

Closed

adrianna-chang-shopify reviewed Oct 24, 2023

View reviewed changes

andrewn617 force-pushed the configurable-inspect branch 4 times, most recently from 3053798 to e22000f Compare October 25, 2023 20:38

rafaelfranca reviewed Oct 26, 2023

View reviewed changes

andrewn617 force-pushed the configurable-inspect branch from e22000f to 9289e5b Compare October 26, 2023 19:44

p8 reviewed Oct 26, 2023

View reviewed changes

andrewn617 force-pushed the configurable-inspect branch from 9289e5b to add9776 Compare October 26, 2023 21:33

andrewn617 force-pushed the configurable-inspect branch 4 times, most recently from 848bfe7 to a80564a Compare October 30, 2023 14:21

rails-bot bot added the railties label Oct 30, 2023

andrewn617 force-pushed the configurable-inspect branch 3 times, most recently from c3fbfc4 to 62e9693 Compare October 30, 2023 14:34

andrewn617 force-pushed the configurable-inspect branch 4 times, most recently from 828bf88 to 9563777 Compare November 1, 2023 14:48

andrewn617 force-pushed the configurable-inspect branch from 9563777 to 0a328e0 Compare November 8, 2023 00:04

rafaelfranca reviewed Nov 8, 2023

View reviewed changes

andrewn617 force-pushed the configurable-inspect branch 5 times, most recently from da9d692 to 3b6c10b Compare November 8, 2023 19:18

andrewn617 force-pushed the configurable-inspect branch from 3b6c10b to e50182a Compare November 8, 2023 19:23

rafaelfranca merged commit ee42128 into rails:main Nov 8, 2023
4 checks passed

fatkodima added a commit to fatkodima/columns_trace that referenced this pull request Nov 8, 2023

Fix tests for activerecord main branch

1e39612

Rails changed what it prints in `inspect` method (rails/rails#49765). Only `id` is printed by default.

floehopper mentioned this pull request Nov 19, 2023

ActiveRecord::Base.mocha_inspect is too verbose freerange/mocha#151

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the output of `ActiveRecord::Core#inspect` configurable. #49765

Make the output of `ActiveRecord::Core#inspect` configurable. #49765

andrewn617 commented Oct 24, 2023 •

edited

andrewn617 Oct 24, 2023

andrewn617 Oct 24, 2023

rafaelfranca Oct 26, 2023

adrianna-chang-shopify left a comment •

edited

adrianna-chang-shopify Oct 24, 2023

adrianna-chang-shopify Oct 24, 2023

rafaelfranca Oct 26, 2023

rafaelfranca Oct 26, 2023

p8 Oct 26, 2023

rafaelfranca Oct 26, 2023

andrewn617 Oct 26, 2023

p8 commented Oct 26, 2023 •

edited

rafaelfranca commented Oct 26, 2023

p8 commented Oct 26, 2023

Earlopain commented Oct 27, 2023

rafaelfranca commented Oct 27, 2023 •

edited

andrewn617 commented Oct 30, 2023

andrewn617 commented Nov 1, 2023

rafaelfranca Oct 27, 2023 •

edited

andrewn617 Nov 8, 2023 •

edited

rafaelfranca Nov 8, 2023

andrewn617 Nov 8, 2023

rafaelfranca Oct 27, 2023

andrewn617 Nov 8, 2023

	The attributes to be included in the out of `inspect` can be configured with
	The attributes to be included in the output of `inspect` can be configured with

	# Returns the attributes specified by #attributes_for_inspect as a nicely formatted string.
	# Returns the attributes specified by <tt>.attributes_for_inspect</tt> as a nicely formatted string.

Make the output of ActiveRecord::Core#inspect configurable. #49765

Make the output of ActiveRecord::Core#inspect configurable. #49765

Conversation

andrewn617 commented Oct 24, 2023 • edited

Motivation / Background

Detail

Additional information

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianna-chang-shopify left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p8 commented Oct 26, 2023 • edited

rafaelfranca commented Oct 26, 2023

p8 commented Oct 26, 2023

Earlopain commented Oct 27, 2023

rafaelfranca commented Oct 27, 2023 • edited

andrewn617 commented Oct 30, 2023

andrewn617 commented Nov 1, 2023

rafaelfranca Oct 27, 2023 • edited

Choose a reason for hiding this comment

andrewn617 Nov 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make the output of `ActiveRecord::Core#inspect` configurable. #49765

Make the output of `ActiveRecord::Core#inspect` configurable. #49765

andrewn617 commented Oct 24, 2023 •

edited

adrianna-chang-shopify left a comment •

edited

p8 commented Oct 26, 2023 •

edited

rafaelfranca commented Oct 27, 2023 •

edited

rafaelfranca Oct 27, 2023 •

edited

andrewn617 Nov 8, 2023 •

edited