Skip to content

Commit

Permalink
Merge pull request #214 from jrochkind/avoid_ar_private_attribute_reg…
Browse files Browse the repository at this point in the history
…istration

Avoid private ActiveRecord API when lazily registering container attributes
  • Loading branch information
jrochkind authored Nov 2, 2023
2 parents a8a5484 + f4fbfbe commit b2cd414
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 14 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
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 b2cd414

Please sign in to comment.