Ensure associations are inherited even after subclasses are defined #39473

kamipo · 2020-05-29T15:03:43Z

This is a common class_attribute implementation issue (e.g. #39467,
#39468, #38871).

For now, we should take care of the change propagation by themselves if
we want to ensure the propagation after the class_attribute are
accessed in subclasses.

Fixes #20678.

This is a common `class_attribute` implementation issue (e.g. rails#39467, rails#39468, rails#38871). For now, we should take care of the change propagation by themselves if we want to ensure the propagation after the `class_attribute` are accessed in subclasses. Fixes rails#20678.

I've partly picked `LiloHash` from rails#39487. Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>

jonathanhefner · 2020-06-14T07:59:15Z

activesupport/lib/active_support/core_ext/class/attribute.rb

+      if default.is_a?(Hash)
+        class_methods << inplace_updatable_class_attribute(name)
+      end


This is a very interesting idea! I originally chose the class_store approach because I didn't want to add (yet another) option to class_attribute, and because some options did not seem suitable (e.g. instance_writer: true). But I really like your approach, so I changed to the same!

jonathanhefner · 2020-06-14T08:22:05Z

activesupport/lib/active_support/core_ext/class/attribute.rb

+          descendants.each do |subclass|
+            next if subclass.#{name}.equal?(#{name})
+
+            if !subclass.#{name}.key?(key) || prev_value&.equal?(subclass.#{name}[key])


This can produce different outcomes for flyweight / frozen objects versus other objects. For example:

A = Class.new A.class_attribute :things, default: {} B = Class.new(A) A.inplace_update_things(:point, [0, 0]) B.things = B.things.dup B.inplace_update_things(:point, [0, 0]) A.inplace_update_things(:point, [1, 1]) B.things[:point] # => [0, 0] A.inplace_update_things(:x, 0) B.things = B.things.dup B.inplace_update_things(:x, 0) A.inplace_update_things(:x, 1) B.things[:x] # => 1

My approach to fix this was to track overridden keys.

I know this does not perfectly detect overrides.
But I suppose this works well enough as a small utility that solves such like internal problems.

Note that this small utility is focused to avoid allocation as much as possible.

Yes, I am not a fan of extra allocation either. But I felt it was necessary because even if such problems are unlikely, they would be hard to diagnose when they did occur. Also, because it is implemented as an automatic feature of class_attribute, it seems more public, and so I wanted to make it more foolproof.

I added ff3e8f4 which should prevent Set allocation when the superclass's class_attribute value is empty? or undefined. If the update method is called on the superclass later (and its class_attribute value becomes non-empty), the subclass's Set will be allocated and properly back-filled.

jonathanhefner · 2020-06-14T08:22:35Z

activerecord/lib/active_record/reflection.rb

+    class LiloHash < Hash
+      def []=(key, value)
+        delete(key)
+        super
+      end
+    end


This reverts commit 5f0e546. The purpose of the commit is to demonstrate how to generalize. In this case, the generalize allocates extra `descendants` array so I've reverted the commit.

rails-bot · 2020-09-14T04:58:50Z