From 322e62842ce656e4bb7b6562efc2a7f314fe5d43 Mon Sep 17 00:00:00 2001
From: Kevin Dew <kevindew@me.com>
Date: Fri, 25 Mar 2022 20:41:28 +0000
Subject: [PATCH] Remove override of ActiveModel#attribute_names

In https://github.com/rails/rails/pull/43036 an optimisation was applied
to ActiveModel#Serialization to speed up the generation of a
serialized_hash by avoiding loading the subjects attributes by using an
attribute_names method. A fallback method,
ActiveModel::Serialization#attribute_names` was added as #attribute_names
isn't part of the public API of ActiveModel.

Unfortunately, this fallback method happens to override the ActiveRecord
method (as ActiveModel::Serialization is a later mixin than
ActiveRecord::AttributeMethods), so this change didn't actually provide
an optimisation - the full attribute data was loaded as per [1]

This change also, in our case, produced some severe performance issues
as it introduced an N+1 query in a situation where we had one gem,
Globalize [2], which adds in dynamic attributes that are loaded by a query;
and another gem, Transitions [3], that checks attribute names at
initialization. The combination of these meant that for every model that
was initialized an extra query would run - no matter what includes or
eager_load steps were in place. This rapidly hindered our applications'
performance and meant we had to rollback the Rails 7 upgrade.

Following rafaelfranca's suggestion [4] this adds a
`attribute_names_for_serialization` method to Serialization modules in
ActiveRecord and ActiveModel. This allows the ActiveRecord one to
override the ActiveModel fallback and thus be optimised.

Some basic benchmarks of this follow - they use code from
https://github.com/ollietreend/rails-demo and have some pretty large
arrays set as serialized attributes [5] to demonstrate impacts.

Loading attribute names:

Rails 7.0.2.3

```
> Benchmark.ms { Widget.all.map(&:attribute_names) }
  Widget Load (131.1ms)  SELECT "widgets".* FROM "widgets"
=> 20108.852999983355
```

This patch

```
> Benchmark.ms { Widget.all.map(&:attribute_names) }
  Widget Load (144.0ms)  SELECT "widgets".* FROM "widgets"
=> 237.96699999365956
```

Using serializable_hash:

Rails 7.0.2.3

```
> widgets = Widget.all.to_a; Benchmark.ms { widgets.map { |w| w.serializable_hash(only: []) } }
  Widget Load (133.3ms)  SELECT "widgets".* FROM "widgets"
=> 22071.45000001765
```

This patch

```
> widgets = Widget.all.to_a; Benchmark.ms { widgets.map { |w| w.serializable_hash(only: []) } }
  Widget Load (83.5ms)  SELECT "widgets".* FROM "widgets"
=> 67.9039999959059
```

[1]: https://github.com/rails/rails/blob/eeb2cfb6864169d7b9cb78744d44b7da17b2b288/activemodel/lib/active_model/serialization.rb#L151-L154
[2]: https://github.com/globalize/globalize
[3]: https://github.com/troessner/transitions
[4]: https://github.com/rails/rails/pull/44770#pullrequestreview-922209612
[5]: https://github.com/ollietreend/rails-demo/blob/525f88887bb1605a72f7b944b59b0d0e204f92aa/db/seeds.rb
---
 activemodel/lib/active_model/serialization.rb   | 11 +++++------
 activerecord/lib/active_record/serialization.rb |  5 +++++
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/activemodel/lib/active_model/serialization.rb b/activemodel/lib/active_model/serialization.rb
index 66e5ce88cc..7907fec86b 100644
--- a/activemodel/lib/active_model/serialization.rb
+++ b/activemodel/lib/active_model/serialization.rb
@@ -123,7 +123,7 @@ module ActiveModel
     #   user.serializable_hash(include: { notes: { only: 'title' }})
     #   # => {"name" => "Napoleon", "notes" => [{"title"=>"Battle of Austerlitz"}]}
     def serializable_hash(options = nil)
-      attribute_names = self.attribute_names
+      attribute_names = attribute_names_for_serialization
 
       return serializable_attributes(attribute_names) if options.blank?
 
@@ -148,12 +148,11 @@ module ActiveModel
       hash
     end
 
-    # Returns an array of attribute names as strings
-    def attribute_names # :nodoc:
-      attributes.keys
-    end
-
     private
+      def attribute_names_for_serialization
+        attributes.keys
+      end
+
       # Hook method defining how an attribute value should be retrieved for
       # serialization. By default this is assumed to be an instance named after
       # the attribute. Override this method in subclasses should you need to
diff --git a/activerecord/lib/active_record/serialization.rb b/activerecord/lib/active_record/serialization.rb
index 212b6c4654..e9e5159e51 100644
--- a/activerecord/lib/active_record/serialization.rb
+++ b/activerecord/lib/active_record/serialization.rb
@@ -20,5 +20,10 @@ module ActiveRecord # :nodoc:
 
       super(options)
     end
+
+    private
+      def attribute_names_for_serialization
+        attribute_names
+      end
   end
 end