Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid private ActiveRecord API when lazily registering container attributes #214

Merged
merged 2 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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