From bd3e3663e8c181661144dc042e8fe48c352f4101 Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Thu, 2 Nov 2023 13:27:24 -0400 Subject: [PATCH 1/2] Avoid private ActiveRecord API when lazily registering container attributes We were using a `attributes_to_define_after_schema_loads` method which was private API, and which breaks in current Rails main branch post 7.1 release. We switch to only using standard attribute API, and keeping track ourselves internally if this is the "first time" a container attrib is encountered, so if it needs an attribute registration. We don't want to multiply register the same attribute many times becaues although it would work, it's just messy and extra objects and calls for rails. This is working -- we had pretty good test coverage, which helped us realize we needed to track specific sub-class too, and tests now all pass. In the long-run, we might like a different public API that doesn't require us to implicitly know when first time container attrib is encountered. Like requires you to explicitly group things by container attribute. Or... this works and is fine? Could be! This is one part of #212 although other failures are revealed once we fix this one. --- .../attribute_definition/registry.rb | 28 +++++++++++++++++++ lib/attr_json/record.rb | 20 ++++--------- 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/lib/attr_json/attribute_definition/registry.rb b/lib/attr_json/attribute_definition/registry.rb index 2fb9220..8657fc3 100644 --- a/lib/attr_json/attribute_definition/registry.rb +++ b/lib/attr_json/attribute_definition/registry.rb @@ -26,6 +26,8 @@ def initialize(hash = {}) @name_to_definition = hash.dup @store_key_to_definition = {} definitions.each { |d| store_key_index!(d) } + + @container_attributes_registered = Hash.new { Set.new } end def fetch(key, *args, &block) @@ -73,6 +75,32 @@ def with(*definitions) end end + + # We need to lazily set the container type only the FIRST time + # + # While also avoiding this triggering ActiveRecord to actually go to DB, + # we don't want DB connection forced on boot, that's a problem for many apps, + # including that may not have a DB connection in initial development. + # (#type_for_attribute forces DB connection) + # + # AND we need to call container attriubte on SUB-CLASS AGAIN, iff sub-class + # has any of it's own new registrations, to make sure we get the right type in + # sub-class! + # + # So we just keep track of whether we've registered ourselves, so we can + # first time we need to. + # + # While current implementation is simple, this has ended up a bit fragile, + # a different API that doesn't require us to do this implicitly lazily + # might be preferred! But this is what we got for now. + def register_container_attribute(attribute_name:, model:) + @container_attributes_registered[attribute_name.to_sym] << model + end + + def container_attribute_registered?(attribute_name:, model:) + @container_attributes_registered[attribute_name.to_sym].include?(model) + end + protected def add!(definition) diff --git a/lib/attr_json/record.rb b/lib/attr_json/record.rb index 745675f..904358e 100644 --- a/lib/attr_json/record.rb +++ b/lib/attr_json/record.rb @@ -160,22 +160,14 @@ def attr_json(name, type, **options) options.assert_valid_keys(AttributeDefinition::VALID_OPTIONS + [:validate, :accepts_nested_attributes]) container_attribute = options[:container_attribute] - # TODO arg check container_attribute make sure it exists. Hard cause - # schema isn't loaded yet when class def is loaded. Maybe not. - # Want to lazily add an attribute cover to the json container attribute, - # only if it hasn't already been done. WARNING we are using internal - # Rails API here, but only way to do this lazily, which I thought was - # worth it. On the other hand, I think .attribute is idempotent, maybe we don't need it... - # - # We set default to empty hash, because that 'tricks' AR into knowing any - # application of defaults is a change that needs to be saved. - unless attributes_to_define_after_schema_loads[container_attribute.to_s] && - attributes_to_define_after_schema_loads[container_attribute.to_s].first.is_a?(AttrJson::Type::ContainerAttribute) && - attributes_to_define_after_schema_loads[container_attribute.to_s].first.model == self - # If this is already defined, but was for superclass, we need to define it again for - # this class. + # Make sure to "lazily" register attribute for *container* class if this is the first time + # this container attribute hsa been encountered for this specific class. The registry + # helps us keep track. Kinda messy, in future we may want a more explicit API + # that does not require us to implicitly track first-time per-container. + unless self.attr_json_registry.container_attribute_registered?(model: self, attribute_name: container_attribute.to_sym) attribute container_attribute.to_sym, AttrJson::Type::ContainerAttribute.new(self, container_attribute), default: -> { {} } + self.attr_json_registry.register_container_attribute(model: self, attribute_name: container_attribute.to_sym) end self.attr_json_registry = attr_json_registry.with( From f4fbfbe0f160923b8abb8e8cc85d58a6a3c1197a Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Thu, 2 Nov 2023 13:55:58 -0400 Subject: [PATCH 2/2] CHANGES for 214 --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97e1bff..73ed8e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * +### Changed + +* Avoid private ActiveRecord API when lazily registering container attributes. (Compat with Rails post 7.1) https://github.com/jrochkind/attr_json/pull/214 + ## [2.2.0](https://github.com/jrochkind/attr_json/compare/v2.1.0...v2.2.0) ### Added