Skip to content

Commit

Permalink
Avoid private ActiveRecord API when lazily registering container attr…
Browse files Browse the repository at this point in the history
…ibutes

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.
  • Loading branch information
jrochkind committed Nov 2, 2023
1 parent a8a5484 commit bd3e366
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 14 deletions.
28 changes: 28 additions & 0 deletions lib/attr_json/attribute_definition/registry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
20 changes: 6 additions & 14 deletions lib/attr_json/record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit bd3e366

Please sign in to comment.