From 0d7667ac315a7382146f39395914406fbb923f90 Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Sat, 4 Jul 2026 22:47:16 -0700 Subject: [PATCH 01/12] Bug fixes --- CHANGELOG.md | 21 ++ README.md | 4 + VERSION | 2 +- lib/support_table_data.rb | 208 +++++++++++++----- .../documentation/source_file.rb | 2 +- lib/support_table_data/railtie.rb | 2 +- lib/support_table_data/tasks/utils.rb | 10 +- spec/data/sizes.yml | 19 ++ spec/data/sizes_override.yml | 2 + spec/models.rb | 8 + spec/models/size.rb | 10 + .../documentation/source_file_spec.rb | 24 ++ spec/support_table_data_spec.rb | 96 +++++++- 13 files changed, 341 insertions(+), 67 deletions(-) create mode 100644 spec/data/sizes.yml create mode 100644 spec/data/sizes_override.yml create mode 100644 spec/models/size.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index abeff51..39a8cd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,27 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## 1.6.2 + +### Fixed + +- `sync_table_data!` with `delete_missing: true` now raises an `ArgumentError` instead of deleting every row in the table when the data files contain no rows (for example, when a data file was accidentally emptied or truncated). +- Generated predicate methods (e.g. `record.active?`) now cast the data file value to the attribute type before comparing. Previously the raw file value was compared to the cast database attribute, so predicates silently returned `false` whenever the types differed (guaranteed for CSV data files, where all values are strings). +- Single table inheritance subclasses no longer raise `NoMethodError` from `instance_names`, `instance_keys`, `protected_instance?`, and other class methods. Subclasses now share the support table state defined on their base class. +- Named instance helper methods are now redefined when a later data file overrides an attribute or key value, so the helpers always return the merged values that are synced to the database. Previously they permanently returned the values from the first file that defined the named instance. +- `named_instance_attribute_helpers` can now be called again with an attribute that was already registered without raising an `ArgumentError`. +- YAML data files can now use anchors/aliases and date/time values. Previously these raised `Psych::AliasesNotEnabled` or `Psych::DisallowedClass` errors. +- `protected_instance?` no longer returns stale results when data files are added after the protected keys were first computed. +- Fixed broken cycle detection in the autosave association check during syncs that could cause infinite recursion on cyclic autosave associations. +- `sync_table_data!` now retries once on `ActiveRecord::RecordNotUnique` errors caused by concurrent syncs inserting the same rows from another process. +- `sync_table_data!` now returns an empty array instead of `nil` when the table does not exist. +- `named_instance` now raises a clear `ActiveRecord::RecordNotFound` error for undefined named instances instead of querying the database for a `nil` key (which could silently return a row with a `NULL` key value). +- Memoized class-level state is now consistently synchronized with the class mutex to avoid races on non-MRI Ruby implementations. +- Setting `config.support_table.auto_sync = false` before the gem is loaded is no longer overwritten back to `true` by the Railtie. +- The documentation tasks no longer corrupt model source files that contain duplicated generated YARD doc blocks (e.g. from a bad merge); the regex that finds the generated block is no longer greedy. +- Data file names containing extra dots no longer break the class name detection used by `SupportTableData.sync_all!` to eager load models. +- Error messages for invalid named instance definitions now include the model class name instead of repeating the instance name. + ## 1.6.1 ### Fixed diff --git a/README.md b/README.md index 535693f..2a74f9c 100644 --- a/README.md +++ b/README.md @@ -279,6 +279,10 @@ SupportTableData.sync_all!(delete_missing: true) > [!CAUTION] > Use `delete_missing` with care. It will delete any records in the table that are not defined in the data files, which may include user-created data or fail due to foreign key constraints. +As a safeguard, `sync_table_data!` will raise an `ArgumentError` rather than delete anything when `delete_missing` is enabled but the data files contain no rows (for instance, when a data file was accidentally emptied or truncated). + +It is recommended to add a unique database index on the key attribute column. Concurrent syncs from multiple processes (for example, parallel deployment jobs) could otherwise insert duplicate rows. If a sync hits a uniqueness violation from a concurrent insert, it will automatically retry once to pick up the other process' changes. + The number of records contained in data files should be fairly small (ideally fewer than 100). It is possible to load just a subset of rows in a large table because only the rows listed in the data files will be synced. You can use this feature if your table allows user-entered data, but has a few rows that must exist for the code to work. Loading data is done inside a database transaction. No changes will be persisted to the database unless all rows for a model can be synced. diff --git a/VERSION b/VERSION index 9c6d629..fdd3be6 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.6.1 +1.6.2 diff --git a/lib/support_table_data.rb b/lib/support_table_data.rb index 8e4dfd4..83f0300 100644 --- a/lib/support_table_data.rb +++ b/lib/support_table_data.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "active_support/core_ext/module/redefine_method" + # This concern can be mixed into models that represent static support tables. These are small tables # that have a limited number of rows, and have values that are often tied to the logic in the code. # @@ -97,15 +99,22 @@ def support_table_yard_docs=(value) # files will be deleted. Use with caution. # @return [Array] List of saved changes for each record that was created or modified. def sync_table_data!(delete_missing: false) - return unless table_exists? + return [] unless table_exists? - canonical_data = support_table_data.each_with_object({}) do |attributes, hash| - hash[attributes[support_table_key_attribute].to_s] = attributes - end - records = where(support_table_key_attribute => canonical_data.keys) - changes = [] + retried = false begin + canonical_data = support_table_data.each_with_object({}) do |attributes, hash| + hash[attributes[support_table_key_attribute].to_s] = attributes + end + + if delete_missing && canonical_data.empty? && exists? + raise ArgumentError.new("Refusing to sync #{name} with delete_missing enabled because the data files contain no rows; this would delete every row in the table") + end + + records = where(support_table_key_attribute => canonical_data.keys) + changes = [] + ActiveSupport::Notifications.instrument("support_table_data.sync", class: self) do synced_ids = [] @@ -141,11 +150,17 @@ def sync_table_data!(delete_missing: false) end end end + changes rescue ActiveRecord::RecordInvalid => e raise SupportTableData::ValidationError.new(e.record) - end + rescue ActiveRecord::RecordNotUnique + # A concurrent sync from another process may have inserted the same rows. + # The transaction was rolled back, so retry once to pick up those rows. + raise if retried - changes + retried = true + retry + end end # Add a data file that contains the support table data. This method can be called multiple times to @@ -157,9 +172,10 @@ def sync_table_data!(delete_missing: false) # @return [void] def add_support_table_data(data_file_path) root_dir = (support_table_data_directory || SupportTableData.data_directory || Dir.pwd) - @mutex.synchronize do - @support_table_data_files += [File.expand_path(data_file_path, root_dir)] + support_table_mutex.synchronize do + @support_table_data_files = support_table_data_files + [File.expand_path(data_file_path, root_dir)] @support_table_instance_keys = nil + @protected_keys = nil end define_support_table_named_instances end @@ -172,9 +188,11 @@ def add_support_table_data(data_file_path) # @param attributes [String, Symbol] The names of the attributes to add helper methods for. # @return [void] def named_instance_attribute_helpers(*attributes) - @mutex.synchronize do + support_table_mutex.synchronize do attributes.flatten.collect(&:to_s).each do |attribute| - @support_table_attribute_helpers = @support_table_attribute_helpers.merge(attribute => []) + next if support_table_attribute_helpers_map.include?(attribute) + + @support_table_attribute_helpers = support_table_attribute_helpers_map.merge(attribute => []) end end define_support_table_named_instances @@ -185,7 +203,7 @@ def named_instance_attribute_helpers(*attributes) # # @return [Array] List of attribute names. def support_table_attribute_helpers - @support_table_attribute_helpers.keys + support_table_attribute_helpers_map.keys end # Get the data for the support table from the data files. @@ -193,7 +211,7 @@ def support_table_attribute_helpers # @return [Array] List of attributes for all records in the data files. def support_table_data data = {} - @support_table_data_files.each do |data_file_path| + support_table_data_files.each do |data_file_path| file_data = support_table_parse_data_file(data_file_path) file_data = file_data.values if file_data.is_a?(Hash) file_data = Array(file_data).flatten @@ -218,7 +236,7 @@ def named_instance_data(name) data = {} name = name.to_s - @support_table_data_files.each do |data_file_path| + support_table_data_files.each do |data_file_path| file_data = support_table_parse_data_file(data_file_path) next unless file_data.is_a?(Hash) @@ -237,7 +255,7 @@ def named_instance_data(name) # # @return [Array] List of all instance names. def instance_names - @support_table_instance_names.keys + support_table_instance_names_map.keys end # Load a named instance from the database. @@ -247,36 +265,54 @@ def instance_names # @raise [ActiveRecord::RecordNotFound] If the instance does not exist. def named_instance(instance_name) instance_name = instance_name.to_s - find_by!(support_table_key_attribute => @support_table_instance_names[instance_name]) + instances = support_table_instance_names_map + unless instances.include?(instance_name) + raise ActiveRecord::RecordNotFound.new("Couldn't find #{name} named instance #{instance_name.inspect}") + end + + find_by!(support_table_key_attribute => instances[instance_name]) end # Get the key values for all instances loaded from the data files. # # @return [Array] List of all the key attribute values. def instance_keys - if @support_table_instance_keys.nil? - values = [] - support_table_data.each do |attributes| - key_value = attributes[support_table_key_attribute] - instance = new - instance.send(:"#{support_table_key_attribute}=", key_value) - values << instance.send(support_table_key_attribute) + keys = @support_table_instance_keys + if keys.nil? + support_table_mutex.synchronize do + keys = @support_table_instance_keys + if keys.nil? + values = [] + support_table_data.each do |attributes| + key_value = attributes[support_table_key_attribute] + instance = new + instance.send(:"#{support_table_key_attribute}=", key_value) + values << instance.send(support_table_key_attribute) + end + keys = values.uniq + @support_table_instance_keys = keys + end end - @support_table_instance_keys = values.uniq end - @support_table_instance_keys + keys end # Return true if the instance has data being managed from a data file. # # @return [Boolean] def protected_instance?(instance) - unless defined?(@protected_keys) - keys = support_table_data.collect { |attributes| attributes[support_table_key_attribute].to_s } - @protected_keys = keys + keys = @protected_keys + if keys.nil? + support_table_mutex.synchronize do + keys = @protected_keys + if keys.nil? + keys = support_table_data.collect { |attributes| attributes[support_table_key_attribute].to_s } + @protected_keys = keys + end + end end - @protected_keys.include?(instance[support_table_key_attribute].to_s) + keys.include?(instance[support_table_key_attribute].to_s) end # Explicitly define other support tables that this model depends on. A support table depends @@ -290,22 +326,36 @@ def protected_instance?(instance) # @param class_names [String] List of class names that this support table depends on. # @return [void] def support_table_dependency(*class_names) - @support_table_dependencies += class_names.flatten.collect(&:to_s) + support_table_mutex.synchronize do + @support_table_dependencies = support_table_dependency_names + class_names.flatten.collect(&:to_s) + end end private def define_support_table_named_instances - @support_table_data_files.each do |file_path| + merged_data = {} + + support_table_data_files.each do |file_path| data = support_table_parse_data_file(file_path) next unless data.is_a?(Hash) data.each do |name, attributes| - @mutex.synchronize do - define_support_table_named_instance_methods(name, attributes) + name = name.to_s + existing = merged_data[name] + merged_data[name] = if existing.is_a?(Hash) && attributes.is_a?(Hash) + existing.merge(attributes) + else + attributes end end end + + merged_data.each do |name, attributes| + support_table_mutex.synchronize do + define_support_table_named_instance_methods(name, attributes) + end + end end def define_support_table_named_instance_methods(name, attributes) @@ -313,32 +363,43 @@ def define_support_table_named_instance_methods(name, attributes) return if method_name.start_with?("_") unless attributes.is_a?(Hash) - raise ArgumentError.new("Cannot define named instance #{method_name} on #{name}; value must be a Hash") + raise ArgumentError.new("Cannot define named instance #{method_name} on #{self.name}; value must be a Hash") end unless method_name.match?(/\A[a-z][a-z0-9_]+\z/) - raise ArgumentError.new("Cannot define named instance #{method_name} on #{name}; name contains illegal characters") + raise ArgumentError.new("Cannot define named instance #{method_name} on #{self.name}; name contains illegal characters") end key_value = attributes[support_table_key_attribute] + instance_names_map = support_table_instance_names_map - unless @support_table_instance_names.include?(method_name) + if instance_names_map.include?(method_name) + if instance_names_map[method_name] != key_value + define_support_table_instance_helper(method_name, support_table_key_attribute, key_value, redefine: true) + define_support_table_predicates_helper("#{method_name}?", support_table_key_attribute, key_value, redefine: true) + @support_table_instance_names = instance_names_map.merge(method_name => key_value) + end + else define_support_table_instance_helper(method_name, support_table_key_attribute, key_value) define_support_table_predicates_helper("#{method_name}?", support_table_key_attribute, key_value) - @support_table_instance_names = @support_table_instance_names.merge(method_name => key_value) + @support_table_instance_names = instance_names_map.merge(method_name => key_value) end - @support_table_attribute_helpers.each do |attribute_name, defined_methods| + support_table_attribute_helpers_map.each do |attribute_name, defined_methods| attribute_method_name = "#{method_name}_#{attribute_name}" - next if defined_methods.include?(attribute_method_name) - - define_support_table_instance_attribute_helper(attribute_method_name, attributes[attribute_name]) - defined_methods << attribute_method_name + if defined_methods.include?(attribute_method_name) + define_support_table_instance_attribute_helper(attribute_method_name, attributes[attribute_name], redefine: true) + else + define_support_table_instance_attribute_helper(attribute_method_name, attributes[attribute_name]) + defined_methods << attribute_method_name + end end end - def define_support_table_instance_helper(method_name, attribute_name, attribute_value) - if respond_to?(method_name, true) + def define_support_table_instance_helper(method_name, attribute_name, attribute_value, redefine: false) + if redefine + singleton_class.silence_redefinition_of_method(method_name) + elsif respond_to?(method_name, true) raise ArgumentError.new("Could not define support table helper method #{name}.#{method_name} because it is already a defined method") end @@ -349,8 +410,10 @@ def self.#{method_name} RUBY end - def define_support_table_instance_attribute_helper(method_name, attribute_value) - if respond_to?(method_name, true) + def define_support_table_instance_attribute_helper(method_name, attribute_value, redefine: false) + if redefine + singleton_class.silence_redefinition_of_method(method_name) + elsif respond_to?(method_name, true) raise ArgumentError.new("Could not define support table helper method #{name}.#{method_name} because it is already a defined method") end @@ -361,14 +424,16 @@ def self.#{method_name} RUBY end - def define_support_table_predicates_helper(method_name, attribute_name, attribute_value) - if method_defined?(method_name) || private_method_defined?(method_name) + def define_support_table_predicates_helper(method_name, attribute_name, attribute_value, redefine: false) + if redefine + silence_redefinition_of_method(method_name) + elsif method_defined?(method_name) || private_method_defined?(method_name) raise ArgumentError.new("Could not define support table helper method #{name}##{method_name} because it is already a defined method") end class_eval <<~RUBY, __FILE__, __LINE__ + 1 def #{method_name} - #{attribute_name} == #{attribute_value.inspect} + #{attribute_name} == self.class.type_for_attribute(#{attribute_name.inspect}).cast(#{attribute_value.inspect}) end RUBY end @@ -390,7 +455,13 @@ def support_table_parse_data_file(file_path) end else require "yaml" unless defined?(YAML) - data = YAML.safe_load(file_data) + require "date" unless defined?(Date) + data = if Psych::VERSION.to_f >= 3.1 + YAML.safe_load(file_data, permitted_classes: [Date, Time], aliases: true) + else + # Positional arguments for Psych < 3.1 (Ruby 2.5). + YAML.safe_load(file_data, [Date, Time], [], true) + end end data @@ -399,7 +470,7 @@ def support_table_parse_data_file(file_path) def support_table_record_changed?(record, seen = Set.new) return true if record.changed? - seen << self + seen << record record.class.reflect_on_all_associations.detect do |reflection| next false if reflection.belongs_to? next false unless reflection.options[:autosave] @@ -409,6 +480,31 @@ def support_table_record_changed?(record, seen = Set.new) end end end + + # The class level state used by the concern is stored in instance variables on the + # class where the concern was included. These readers fall back to the superclass + # so that single table inheritance subclasses share the state defined on their + # base class rather than crashing on uninitialized instance variables. + + def support_table_mutex + @mutex || (superclass.include?(SupportTableData) ? superclass.send(:support_table_mutex) : nil) + end + + def support_table_data_files + @support_table_data_files || (superclass.include?(SupportTableData) ? superclass.send(:support_table_data_files) : []) + end + + def support_table_instance_names_map + @support_table_instance_names || (superclass.include?(SupportTableData) ? superclass.send(:support_table_instance_names_map) : {}) + end + + def support_table_attribute_helpers_map + @support_table_attribute_helpers || (superclass.include?(SupportTableData) ? superclass.send(:support_table_attribute_helpers_map) : {}) + end + + def support_table_dependency_names + @support_table_dependencies || (superclass.include?(SupportTableData) ? superclass.send(:support_table_dependency_names) : []) + end end class << self @@ -481,7 +577,7 @@ def support_table_classes(*extra_classes) if SupportTableData.data_directory && File.exist?(SupportTableData.data_directory) && File.directory?(SupportTableData.data_directory) Dir.glob(File.join(SupportTableData.data_directory, "**", "*")).sort.each do |file_name| file_name = file_name.delete_prefix("#{SupportTableData.data_directory}#{File::SEPARATOR}") - class_name = file_name.sub(/\.[^.]*/, "").singularize.camelize + class_name = file_name.sub(/\.[^.]*\z/, "").singularize.camelize class_name.safe_constantize end end @@ -514,7 +610,7 @@ def support_table_classes(*extra_classes) # # @return [Array] def support_table_dependencies(klass) - dependencies = klass.instance_variable_get(:@support_table_dependencies).collect(&:constantize) + dependencies = klass.send(:support_table_dependency_names).collect(&:constantize) klass.reflections.values.each do |reflection| next if reflection.polymorphic? @@ -523,8 +619,8 @@ def support_table_dependencies(klass) next unless reflection.belongs_to? || reflection.through_reflection? next if dependencies.include?(reflection.klass) - explicit_dependencies = reflection.klass.instance_variable_get(:@support_table_dependencies) - next if explicit_dependencies&.include?(klass.name) + explicit_dependencies = reflection.klass.send(:support_table_dependency_names) + next if explicit_dependencies.include?(klass.name) dependencies << reflection.klass rescue => e diff --git a/lib/support_table_data/documentation/source_file.rb b/lib/support_table_data/documentation/source_file.rb index 40d2ce2..67591f0 100644 --- a/lib/support_table_data/documentation/source_file.rb +++ b/lib/support_table_data/documentation/source_file.rb @@ -7,7 +7,7 @@ class SourceFile BEGIN_YARD_COMMENT = "# Begin YARD docs for support_table_data" END_YARD_COMMENT = "# End YARD docs for support_table_data" - YARD_COMMENT_REGEX = /^(?[ \t]*)#{BEGIN_YARD_COMMENT}.*^[ \t]*#{END_YARD_COMMENT}$/m + YARD_COMMENT_REGEX = /^(?[ \t]*)#{BEGIN_YARD_COMMENT}.*?^[ \t]*#{END_YARD_COMMENT}$/m CLASS_DEF_REGEX = /^[ \t]*class [a-zA-Z_0-9:]+.*?$/ UPDATE_COMMAND_COMMENT = "# To update these docs, run `bundle exec rake support_table_data:yard_docs`" diff --git a/lib/support_table_data/railtie.rb b/lib/support_table_data/railtie.rb index 4e0dd33..6c32d0d 100644 --- a/lib/support_table_data/railtie.rb +++ b/lib/support_table_data/railtie.rb @@ -7,7 +7,7 @@ class Railtie < Rails::Railtie end config.support_table.data_directory ||= "db/support_tables" - config.support_table.auto_sync ||= true + config.support_table.auto_sync = true if config.support_table.auto_sync.nil? initializer "support_table_data" do |app| SupportTableData.data_directory ||= app.root.join(app.config.support_table&.data_directory).to_s diff --git a/lib/support_table_data/tasks/utils.rb b/lib/support_table_data/tasks/utils.rb index c552bd6..ce5328d 100644 --- a/lib/support_table_data/tasks/utils.rb +++ b/lib/support_table_data/tasks/utils.rb @@ -30,13 +30,9 @@ def support_table_sources(file_path = nil) ActiveRecord::Base.descendants.each do |klass| next unless klass.include?(SupportTableData) - - begin - next if klass.instance_names.empty? - rescue NoMethodError - # Skip models where instance_names is not properly initialized - next - end + # Skip STI subclasses; only the class that owns the data files gets documented. + next unless klass.instance_variable_defined?(:@support_table_data_files) + next if klass.instance_names.empty? model_file_path = SupportTableData::Tasks::Utils.model_file_path(klass) next unless model_file_path&.file? && model_file_path.readable? diff --git a/spec/data/sizes.yml b/spec/data/sizes.yml new file mode 100644 index 0000000..55797a6 --- /dev/null +++ b/spec/data/sizes.yml @@ -0,0 +1,19 @@ +small: &small + id: "1" + name: small + label: Small + active: true + introduced_on: 2020-01-15 + +medium: + <<: *small + id: 2 + name: medium + label: Medium + +large: + id: 3 + name: large + label: Big + active: false + introduced_on: 2021-06-30 diff --git a/spec/data/sizes_override.yml b/spec/data/sizes_override.yml new file mode 100644 index 0000000..dd6ce4a --- /dev/null +++ b/spec/data/sizes_override.yml @@ -0,0 +1,2 @@ +large: + label: Large diff --git a/spec/models.rb b/spec/models.rb index 706eb2b..a398752 100644 --- a/spec/models.rb +++ b/spec/models.rb @@ -46,6 +46,13 @@ t.string :type t.integer :side_count end + + connection.create_table(:sizes) do |t| + t.string :name + t.string :label + t.boolean :active + t.date :introduced_on + end end # Lazy load model classes @@ -57,6 +64,7 @@ autoload :Polygon, File.expand_path("models/polygon.rb", __dir__) autoload :Rectangle, File.expand_path("models/rectangle.rb", __dir__) autoload :Shade, File.expand_path("models/shade.rb", __dir__) +autoload :Size, File.expand_path("models/size.rb", __dir__) autoload :ShadeHue, File.expand_path("models/shade_hue.rb", __dir__) autoload :Thing, File.expand_path("models/thing.rb", __dir__) autoload :Triangle, File.expand_path("models/triangle.rb", __dir__) diff --git a/spec/models/size.rb b/spec/models/size.rb new file mode 100644 index 0000000..9be621d --- /dev/null +++ b/spec/models/size.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class Size < ActiveRecord::Base + include SupportTableData + + named_instance_attribute_helpers :label + + add_support_table_data "sizes.yml" + add_support_table_data "sizes_override.yml" +end diff --git a/spec/support_table_data/documentation/source_file_spec.rb b/spec/support_table_data/documentation/source_file_spec.rb index 69f12c5..01c9b01 100644 --- a/spec/support_table_data/documentation/source_file_spec.rb +++ b/spec/support_table_data/documentation/source_file_spec.rb @@ -45,6 +45,30 @@ class Color expect(result).not_to include("End YARD docs") end + it "does not swallow user code between duplicated documentation blocks" do + source_with_duplicate_blocks = <<~RUBY + class Color < ActiveRecord::Base + # Begin YARD docs for support_table_data + # Some YARD docs + # End YARD docs for support_table_data + + def user_method + end + + # Begin YARD docs for support_table_data + # Some other YARD docs + # End YARD docs for support_table_data + end + RUBY + + source_file = SupportTableData::Documentation::SourceFile.new(Color, color_path) + allow(source_file).to receive(:source).and_return(source_with_duplicate_blocks) + + result = source_file.source_without_yard_docs + + expect(result).to include("def user_method") + end + it "preserves trailing newline if present in original" do source_with_newline = "class Color\nend\n" source_file = SupportTableData::Documentation::SourceFile.new(Color, color_path) diff --git a/spec/support_table_data_spec.rb b/spec/support_table_data_spec.rb index 35d6f6d..88d6e5b 100644 --- a/spec/support_table_data_spec.rb +++ b/spec/support_table_data_spec.rb @@ -88,6 +88,45 @@ expect(Group.count).to eq 3 expect(Group.pluck(:name)).to match_array ["primary", "secondary", "gray"] end + + it "refuses to delete all rows when delete_missing is true and the data files contain no rows" do + Group.sync_table_data! + allow(Group).to receive(:support_table_data).and_return([]) + expect { Group.sync_table_data!(delete_missing: true) }.to raise_error(ArgumentError, /Refusing to sync Group/) + expect(Group.count).to eq 3 + end + + it "does not raise on empty data files with delete_missing when the table is already empty" do + Invalid.delete_all + allow(Invalid).to receive(:support_table_data).and_return([]) + expect(Invalid.sync_table_data!(delete_missing: true)).to eq [] + end + + it "returns an empty array if the table does not exist" do + allow(Group).to receive(:table_exists?).and_return(false) + expect(Group.sync_table_data!).to eq [] + end + + it "retries once if a concurrent sync inserted the same rows" do + calls = 0 + allow(Group).to receive(:transaction).and_wrap_original do |original, *args, &block| + calls += 1 + raise ActiveRecord::RecordNotUnique.new("duplicate key") if calls == 1 + original.call(*args, &block) + end + expect(Group.sync_table_data!.size).to eq 3 + expect(calls).to eq 2 + end + + it "reraises the error if the retried sync also hits a uniqueness violation" do + calls = 0 + allow(Group).to receive(:transaction) do + calls += 1 + raise ActiveRecord::RecordNotUnique.new("duplicate key") + end + expect { Group.sync_table_data! }.to raise_error(ActiveRecord::RecordNotUnique) + expect(calls).to eq 2 + end end describe "sync_all!" do @@ -202,6 +241,39 @@ it "raises an error if the method is already defined" do expect { Invalid.add_support_table_data("invalid.yml") }.to raise_error(ArgumentError) end + + it "casts the data file value when comparing in predicate methods" do + Size.sync_table_data! + small = Size.find_by!(name: "small") + expect(small.id).to eq 1 + expect(small.small?).to eq true + expect(small.medium?).to eq false + end + + it "supports YAML aliases and date values in data files" do + medium = Size.named_instance_data("medium") + expect(medium["active"]).to eq true + expect(medium["introduced_on"]).to eq Date.new(2020, 1, 15) + expect(medium["label"]).to eq "Medium" + end + + it "redefines attribute helpers when a later data file overrides an attribute" do + expect(Size.small_label).to eq "Small" + expect(Size.large_label).to eq "Large" + end + end + + describe "single table inheritance" do + it "shares support table state with subclasses" do + expect(Rectangle.instance_names).to eq Polygon.instance_names + expect(Rectangle.instance_keys).to eq Polygon.instance_keys + end + + it "determines protected instances for subclass records" do + Polygon.sync_table_data! + expect(Polygon.rectangle.protected_instance?).to eq true + expect(Triangle.new(name: "Scalene").protected_instance?).to eq false + end end describe "instance_names" do @@ -226,6 +298,12 @@ expect(Group.gray_name).to eq "gray" end + it "is idempotent when called again with an attribute that is already registered" do + expect { Group.named_instance_attribute_helpers(:name) }.to_not raise_error + expect(Group.primary_name).to eq "primary" + expect(Group.support_table_attribute_helpers).to match_array ["group_id", "name"] + end + it "can get a list of the defined attribute helpers" do expect(Group.support_table_attribute_helpers).to match_array ["group_id", "name"] expect(Color.support_table_attribute_helpers).to match_array [] @@ -249,11 +327,27 @@ expect(orange.protected_instance?).to eq true expect(brown.protected_instance?).to eq false end + + it "picks up instances from data files added after the protected keys were memoized" do + klass = Class.new(ActiveRecord::Base) do + include SupportTableData + + self.table_name = "colors" + end + klass.add_support_table_data("colors/named_colors.yml") + + light_gray = klass.new + light_gray.id = 8 + expect(light_gray.protected_instance?).to eq false + + klass.add_support_table_data("colors/colors.json") + expect(light_gray.protected_instance?).to eq true + end end describe "support_table_classes" do it "gets a list of all loaded support table classes with dependencies listed first" do - expect(SupportTableData.support_table_classes).to eq [Shade, Group, Hue, Color, Invalid, Polygon] + expect(SupportTableData.support_table_classes).to eq [Shade, Group, Hue, Color, Invalid, Polygon, Size] end end From 85342ce36510ac0ffe7afe339d8a4a59ae0a5c8f Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Wed, 22 Jul 2026 09:13:07 -0700 Subject: [PATCH 02/12] update gem setup --- .github/workflows/continuous_integration.yml | 49 ++++++-------------- .gitignore | 36 ++++++++++---- .standard.yml | 11 +---- Gemfile | 24 ++++++---- Rakefile | 29 +++++++++++- gemfiles/activerecord_6.1.gemfile | 9 +--- gemfiles/activerecord_7.0.gemfile | 9 +--- gemfiles/activerecord_7.1.gemfile | 9 +--- gemfiles/activerecord_7.2.gemfile | 9 +--- gemfiles/activerecord_8.0.gemfile | 9 +--- gemfiles/activerecord_8.1.gemfile | 9 +--- support_table_data.gemspec | 4 +- 12 files changed, 98 insertions(+), 109 deletions(-) diff --git a/.github/workflows/continuous_integration.yml b/.github/workflows/continuous_integration.yml index 61a92a6..91a961c 100644 --- a/.github/workflows/continuous_integration.yml +++ b/.github/workflows/continuous_integration.yml @@ -12,12 +12,6 @@ on: - actions-* workflow_dispatch: -env: - BUNDLE_CLEAN: "true" - BUNDLE_PATH: vendor/bundle - BUNDLE_JOBS: 3 - BUNDLE_RETRY: 3 - jobs: build: runs-on: ubuntu-latest @@ -41,33 +35,16 @@ jobs: - ruby: "2.5" appraisal: "activerecord_6.1" steps: - - uses: actions/checkout@v6 - - name: Set up Ruby ${{ matrix.ruby }} - uses: ruby/setup-ruby@v1 - with: - ruby-version: "${{ matrix.ruby }}" - - name: Install packages - run: | - sudo apt-get update - sudo apt-get install libsqlite3-dev - - name: Setup bundler - if: matrix.bundler != '' - run: | - gem uninstall bundler --all - gem install bundler --no-document --version ${{ matrix.bundler }} - - name: Set Appraisal bundle - if: matrix.appraisal != '' - run: | - echo "using gemfile gemfiles/${{ matrix.appraisal }}.gemfile" - bundle config set gemfile "gemfiles/${{ matrix.appraisal }}.gemfile" - - name: Install gems - run: | - bundle install - - name: Run Tests - run: bundle exec rake - - name: standardrb - if: matrix.standardrb == true - run: bundle exec standardrb - - name: yard - if: matrix.yard == true - run: bundle exec yard doc --fail-on-warning + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + - name: Ruby CI + uses: bdurand/github-actions/ruby-ci@b7560883d5f8557f5d69fe8bd82721bbb5ed76df # v1.0.4 + with: + ruby: ${{ matrix.ruby }} + appraisal: ${{ matrix.appraisal }} + bundler: ${{ matrix.bundler }} + standardrb: ${{ matrix.standardrb }} + yard: ${{ matrix.yard }} + frozen_strings: ${{ matrix.frozen_strings }} diff --git a/.gitignore b/.gitignore index 90f9586..84ba71d 100644 --- a/.gitignore +++ b/.gitignore @@ -1,13 +1,33 @@ .DS_Store -.bundle/ + +/.bundle/ +/vendor/bundle +/lib/bundler/man/ + .rspec -.ruby-version -.yardoc/ .env +*.gem +*.rbc + +/spec/reports/ +/spec/examples.txt +.yardoc/ +_yardoc/ +/doc/ +/rdoc/ +/coverage/ +/log/*.log +/pkg/ +/tmp/ + +.ruby-version +.ruby-gemset Gemfile.lock -coverage/ gemfiles/*.gemfile.lock -log/*.log -pkg/ -rdoc/ -doc/ + +.byebug_history + +.claude/ +.conductor/ +.cursor/ +/.config diff --git a/.standard.yml b/.standard.yml index d4e5868..1699115 100644 --- a/.standard.yml +++ b/.standard.yml @@ -1,10 +1,3 @@ -format: progress - -ruby_version: 2.5 +ruby_version: 2.6 -ignore: - - "**/*": - - Style/RedundantParentheses - - "spec/**/*": - - Lint/ConstantDefinitionInBlock - - Lint/UselessAssignment +format: progress diff --git a/Gemfile b/Gemfile index 25d24cc..71eede4 100644 --- a/Gemfile +++ b/Gemfile @@ -2,12 +2,18 @@ source "https://rubygems.org" gemspec -gem "rspec", "~> 3.0" -gem "rake" -gem "irb" -gem "sqlite3" -gem "appraisal" -gem "standard", "~>1.0" -gem "pry-byebug" -gem "yard" -gem "csv" +# Exclude development-only gems from dependabot. +unless ENV["DEPENDABOT"] + gem "sqlite3" + + gem "rake" + gem "rspec", "~> 3.11" + + # Exclude development-only gems from the gemfiles generated by appraisal. + unless defined?(::Appraisal::Gemfile) && is_a?(::Appraisal::Gemfile) + gem "appraisal", require: false + gem "standard", require: false + gem "simplecov", require: false + gem "yard", require: false + end +end diff --git a/Rakefile b/Rakefile index 3deea70..48f3eb5 100644 --- a/Rakefile +++ b/Rakefile @@ -1,3 +1,5 @@ +# frozen_string_literal: true + begin require "bundler/setup" rescue LoadError @@ -13,10 +15,33 @@ task :verify_release_branch do end end -Rake::Task[:release].enhance([:verify_release_branch]) +Rake::Task[:release].prerequisites.prepend("verify_release_branch") require "rspec/core/rake_task" RSpec::Core::RakeTask.new(:spec) -task default: :spec +task default: [:spec] + +namespace :appraisal do + desc "Update the appraisal gemfiles" + task :update do + Dir.glob("gemfiles/*.gemfile*") do |file| + File.delete(file) if File.file?(file) + end + + system "bundle exec appraisal generate" || abort("appraisal generate failed") + + Dir.glob("gemfiles/*.gemfile") do |file| + puts "Locking #{file}" + Bundler.with_unbundled_env do + system( + { + "BUNDLE_GEMFILE" => file + }, + "bundle", "lock", "--update" + ) || abort("appraisal lock failed on #{file}") + end + end + end +end diff --git a/gemfiles/activerecord_6.1.gemfile b/gemfiles/activerecord_6.1.gemfile index a246139..c4e23d3 100644 --- a/gemfiles/activerecord_6.1.gemfile +++ b/gemfiles/activerecord_6.1.gemfile @@ -2,14 +2,9 @@ source "https://rubygems.org" -gem "rspec", "~> 3.0" -gem "rake" gem "sqlite3", "~> 1.4.0" -gem "appraisal" -gem "standard", "~>1.0" -gem "pry-byebug" -gem "yard" -gem "csv" +gem "rake" +gem "rspec", "~> 3.11" gem "activerecord", "~> 6.1.0" gem "concurrent-ruby", "1.3.4" diff --git a/gemfiles/activerecord_7.0.gemfile b/gemfiles/activerecord_7.0.gemfile index f25d6c6..c86ce61 100644 --- a/gemfiles/activerecord_7.0.gemfile +++ b/gemfiles/activerecord_7.0.gemfile @@ -2,14 +2,9 @@ source "https://rubygems.org" -gem "rspec", "~> 3.0" -gem "rake" gem "sqlite3", "~> 1.4.0" -gem "appraisal" -gem "standard", "~>1.0" -gem "pry-byebug" -gem "yard" -gem "csv" +gem "rake" +gem "rspec", "~> 3.11" gem "activerecord", "~> 7.0.0" gem "concurrent-ruby", "1.3.4" diff --git a/gemfiles/activerecord_7.1.gemfile b/gemfiles/activerecord_7.1.gemfile index f25d6c6..c86ce61 100644 --- a/gemfiles/activerecord_7.1.gemfile +++ b/gemfiles/activerecord_7.1.gemfile @@ -2,14 +2,9 @@ source "https://rubygems.org" -gem "rspec", "~> 3.0" -gem "rake" gem "sqlite3", "~> 1.4.0" -gem "appraisal" -gem "standard", "~>1.0" -gem "pry-byebug" -gem "yard" -gem "csv" +gem "rake" +gem "rspec", "~> 3.11" gem "activerecord", "~> 7.0.0" gem "concurrent-ruby", "1.3.4" diff --git a/gemfiles/activerecord_7.2.gemfile b/gemfiles/activerecord_7.2.gemfile index f25d6c6..c86ce61 100644 --- a/gemfiles/activerecord_7.2.gemfile +++ b/gemfiles/activerecord_7.2.gemfile @@ -2,14 +2,9 @@ source "https://rubygems.org" -gem "rspec", "~> 3.0" -gem "rake" gem "sqlite3", "~> 1.4.0" -gem "appraisal" -gem "standard", "~>1.0" -gem "pry-byebug" -gem "yard" -gem "csv" +gem "rake" +gem "rspec", "~> 3.11" gem "activerecord", "~> 7.0.0" gem "concurrent-ruby", "1.3.4" diff --git a/gemfiles/activerecord_8.0.gemfile b/gemfiles/activerecord_8.0.gemfile index b1709a1..0cf2899 100644 --- a/gemfiles/activerecord_8.0.gemfile +++ b/gemfiles/activerecord_8.0.gemfile @@ -2,14 +2,9 @@ source "https://rubygems.org" -gem "rspec", "~> 3.0" -gem "rake" gem "sqlite3", "~> 2.5.0" -gem "appraisal" -gem "standard", "~>1.0" -gem "pry-byebug" -gem "yard" -gem "csv" +gem "rake" +gem "rspec", "~> 3.11" gem "activerecord", "~> 8.0.0" gemspec path: "../" diff --git a/gemfiles/activerecord_8.1.gemfile b/gemfiles/activerecord_8.1.gemfile index 0e7ee30..6884b1f 100644 --- a/gemfiles/activerecord_8.1.gemfile +++ b/gemfiles/activerecord_8.1.gemfile @@ -2,14 +2,9 @@ source "https://rubygems.org" -gem "rspec", "~> 3.0" -gem "rake" gem "sqlite3", "~> 2.9.0" -gem "appraisal" -gem "standard", "~>1.0" -gem "pry-byebug" -gem "yard" -gem "csv" +gem "rake" +gem "rspec", "~> 3.11" gem "activerecord", "~> 8.1.0" gemspec path: "../" diff --git a/support_table_data.gemspec b/support_table_data.gemspec index 43a460e..cdc4896 100644 --- a/support_table_data.gemspec +++ b/support_table_data.gemspec @@ -35,9 +35,7 @@ Gem::Specification.new do |spec| spec.require_paths = ["lib"] - spec.required_ruby_version = ">= 2.5" + spec.required_ruby_version = ">= 2.6" spec.add_dependency "activerecord", ">= 6" - - spec.add_development_dependency "bundler" end From b951993b4db5e9743c98f35759096927c53ae74a Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Wed, 22 Jul 2026 09:14:13 -0700 Subject: [PATCH 03/12] update .gitignore --- .gitignore | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 84ba71d..a876bb2 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,6 @@ .DS_Store -/.bundle/ +.bundle/ /vendor/bundle /lib/bundler/man/ @@ -30,4 +30,4 @@ gemfiles/*.gemfile.lock .claude/ .conductor/ .cursor/ -/.config +.config From 1daa9f7f91dd7fe45a70d72364c446b192fd86bf Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Sat, 25 Jul 2026 09:30:29 -0700 Subject: [PATCH 04/12] Enhance support table data handling and documentation --- CHANGELOG.md | 3 +- Gemfile | 1 + README.md | 2 + gemfiles/activerecord_6.1.gemfile | 1 + gemfiles/activerecord_7.0.gemfile | 1 + gemfiles/activerecord_7.1.gemfile | 1 + gemfiles/activerecord_7.2.gemfile | 1 + gemfiles/activerecord_8.0.gemfile | 1 + gemfiles/activerecord_8.1.gemfile | 1 + lib/support_table_data.rb | 65 ++++++++++++++----- .../documentation/source_file.rb | 8 ++- .../documentation/source_file_spec.rb | 30 +++++++++ spec/support_table_data_spec.rb | 19 ++++++ 13 files changed, 114 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39a8cd8..7a9e4a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Single table inheritance subclasses no longer raise `NoMethodError` from `instance_names`, `instance_keys`, `protected_instance?`, and other class methods. Subclasses now share the support table state defined on their base class. - Named instance helper methods are now redefined when a later data file overrides an attribute or key value, so the helpers always return the merged values that are synced to the database. Previously they permanently returned the values from the first file that defined the named instance. - `named_instance_attribute_helpers` can now be called again with an attribute that was already registered without raising an `ArgumentError`. +- Data files that override attributes on a named instance are now merged by the instance name rather than only by the key attribute. Previously, an override that did not repeat the key attribute value (for example, a file containing only `large:` with a `label`) was treated as a brand new record; the override was never applied to the real row and a row with only the overridden attributes was inserted on every sync. - YAML data files can now use anchors/aliases and date/time values. Previously these raised `Psych::AliasesNotEnabled` or `Psych::DisallowedClass` errors. - `protected_instance?` no longer returns stale results when data files are added after the protected keys were first computed. - Fixed broken cycle detection in the autosave association check during syncs that could cause infinite recursion on cyclic autosave associations. @@ -21,7 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `named_instance` now raises a clear `ActiveRecord::RecordNotFound` error for undefined named instances instead of querying the database for a `nil` key (which could silently return a row with a `NULL` key value). - Memoized class-level state is now consistently synchronized with the class mutex to avoid races on non-MRI Ruby implementations. - Setting `config.support_table.auto_sync = false` before the gem is loaded is no longer overwritten back to `true` by the Railtie. -- The documentation tasks no longer corrupt model source files that contain duplicated generated YARD doc blocks (e.g. from a bad merge); the regex that finds the generated block is no longer greedy. +- The documentation tasks no longer corrupt model source files that contain duplicated generated YARD doc blocks (e.g. from a bad merge); the regex that finds the generated block is no longer greedy and all duplicated blocks are now removed so the file is left with exactly one block. - Data file names containing extra dots no longer break the class name detection used by `SupportTableData.sync_all!` to eager load models. - Error messages for invalid named instance definitions now include the model class name instead of repeating the instance name. diff --git a/Gemfile b/Gemfile index 71eede4..468be06 100644 --- a/Gemfile +++ b/Gemfile @@ -5,6 +5,7 @@ gemspec # Exclude development-only gems from dependabot. unless ENV["DEPENDABOT"] gem "sqlite3" + gem "csv" gem "rake" gem "rspec", "~> 3.11" diff --git a/README.md b/README.md index 2a74f9c..319852a 100644 --- a/README.md +++ b/README.md @@ -68,6 +68,8 @@ class Status < ApplicationRecord You cannot update the value of the key attribute in a record in the data file. If you do, a new record will be created and the existing record will be left unchanged. +When you add multiple data files, records defined in more than one file are merged together, with later files taking precedence. Records in files defined as a hash of named instances are matched by the instance name, so an override file only needs to specify the attributes it is changing. Records in files defined as a list are matched by the key attribute value, which must be repeated in each file. + You can specify data files as relative paths. This can be done by setting the `SupportTableData.data_directory` value. You can override this value for a model by setting the `support_table_data_directory` attribute on its class. Otherwise, relative file paths will be resolved from the current working directory. You must define the directory to load relative files from before loading your model classes. In a Rails application, `SupportTableData.data_directory` will be automatically set to `db/support_tables/`. This can be overridden by setting the `config.support_table.data_directory` option in the Rails application configuration. diff --git a/gemfiles/activerecord_6.1.gemfile b/gemfiles/activerecord_6.1.gemfile index c4e23d3..0d454b9 100644 --- a/gemfiles/activerecord_6.1.gemfile +++ b/gemfiles/activerecord_6.1.gemfile @@ -5,6 +5,7 @@ source "https://rubygems.org" gem "sqlite3", "~> 1.4.0" gem "rake" gem "rspec", "~> 3.11" +gem "csv" gem "activerecord", "~> 6.1.0" gem "concurrent-ruby", "1.3.4" diff --git a/gemfiles/activerecord_7.0.gemfile b/gemfiles/activerecord_7.0.gemfile index c86ce61..a4692d1 100644 --- a/gemfiles/activerecord_7.0.gemfile +++ b/gemfiles/activerecord_7.0.gemfile @@ -5,6 +5,7 @@ source "https://rubygems.org" gem "sqlite3", "~> 1.4.0" gem "rake" gem "rspec", "~> 3.11" +gem "csv" gem "activerecord", "~> 7.0.0" gem "concurrent-ruby", "1.3.4" diff --git a/gemfiles/activerecord_7.1.gemfile b/gemfiles/activerecord_7.1.gemfile index c86ce61..a4692d1 100644 --- a/gemfiles/activerecord_7.1.gemfile +++ b/gemfiles/activerecord_7.1.gemfile @@ -5,6 +5,7 @@ source "https://rubygems.org" gem "sqlite3", "~> 1.4.0" gem "rake" gem "rspec", "~> 3.11" +gem "csv" gem "activerecord", "~> 7.0.0" gem "concurrent-ruby", "1.3.4" diff --git a/gemfiles/activerecord_7.2.gemfile b/gemfiles/activerecord_7.2.gemfile index c86ce61..a4692d1 100644 --- a/gemfiles/activerecord_7.2.gemfile +++ b/gemfiles/activerecord_7.2.gemfile @@ -5,6 +5,7 @@ source "https://rubygems.org" gem "sqlite3", "~> 1.4.0" gem "rake" gem "rspec", "~> 3.11" +gem "csv" gem "activerecord", "~> 7.0.0" gem "concurrent-ruby", "1.3.4" diff --git a/gemfiles/activerecord_8.0.gemfile b/gemfiles/activerecord_8.0.gemfile index 0cf2899..5b2f385 100644 --- a/gemfiles/activerecord_8.0.gemfile +++ b/gemfiles/activerecord_8.0.gemfile @@ -5,6 +5,7 @@ source "https://rubygems.org" gem "sqlite3", "~> 2.5.0" gem "rake" gem "rspec", "~> 3.11" +gem "csv" gem "activerecord", "~> 8.0.0" gemspec path: "../" diff --git a/gemfiles/activerecord_8.1.gemfile b/gemfiles/activerecord_8.1.gemfile index 6884b1f..db2737e 100644 --- a/gemfiles/activerecord_8.1.gemfile +++ b/gemfiles/activerecord_8.1.gemfile @@ -5,6 +5,7 @@ source "https://rubygems.org" gem "sqlite3", "~> 2.9.0" gem "rake" gem "rspec", "~> 3.11" +gem "csv" gem "activerecord", "~> 8.1.0" gemspec path: "../" diff --git a/lib/support_table_data.rb b/lib/support_table_data.rb index 83f0300..f86d385 100644 --- a/lib/support_table_data.rb +++ b/lib/support_table_data.rb @@ -21,7 +21,7 @@ module SupportTableData included do # Internal variables used for memoization. - @mutex = Mutex.new + @support_table_mutex = Mutex.new @support_table_data_files = [] @support_table_attribute_helpers = {} @support_table_instance_names = {} @@ -171,7 +171,7 @@ def sync_table_data!(delete_missing: false) # this model or the global directory set with SupportTableData.data_directory. # @return [void] def add_support_table_data(data_file_path) - root_dir = (support_table_data_directory || SupportTableData.data_directory || Dir.pwd) + root_dir = support_table_data_directory || SupportTableData.data_directory || Dir.pwd support_table_mutex.synchronize do @support_table_data_files = support_table_data_files + [File.expand_path(data_file_path, root_dir)] @support_table_instance_keys = nil @@ -189,10 +189,15 @@ def add_support_table_data(data_file_path) # @return [void] def named_instance_attribute_helpers(*attributes) support_table_mutex.synchronize do + # Single table inheritance subclasses read the map from their base class. Copy it + # (including the lists of method names that have been defined) the first time a + # subclass registers its own helpers so the two classes don't mutate each other's state. + @support_table_attribute_helpers ||= support_table_attribute_helpers_map.transform_values(&:dup) + attributes.flatten.collect(&:to_s).each do |attribute| - next if support_table_attribute_helpers_map.include?(attribute) + next if @support_table_attribute_helpers.include?(attribute) - @support_table_attribute_helpers = support_table_attribute_helpers_map.merge(attribute => []) + @support_table_attribute_helpers = @support_table_attribute_helpers.merge(attribute => []) end end define_support_table_named_instances @@ -210,19 +215,47 @@ def support_table_attribute_helpers # # @return [Array] List of attributes for all records in the data files. def support_table_data - data = {} + records = [] + named_records = {} + support_table_data_files.each do |data_file_path| file_data = support_table_parse_data_file(data_file_path) - file_data = file_data.values if file_data.is_a?(Hash) - file_data = Array(file_data).flatten - file_data.each do |attributes| - key_value = attributes[support_table_key_attribute].to_s - existing = data[key_value] - if existing - existing.merge!(attributes) - else - data[key_value] = attributes + + if file_data.is_a?(Hash) + file_data.each do |instance_name, attributes| + unless attributes.is_a?(Hash) + # A name mapped to a list of records (i.e. a name beginning with an underscore) + # holds anonymous records that are only identified by the key attribute. + Array(attributes).flatten.each { |record| records << record.dup } + next + end + + # Records are merged by their name so that a later data file can override + # attributes on a named record without having to repeat the key attribute. + instance_name = instance_name.to_s + existing = named_records[instance_name] + if existing + existing.merge!(attributes) + else + record = attributes.dup + named_records[instance_name] = record + records << record + end end + else + Array(file_data).flatten.each { |record| records << record.dup } + end + end + + # Records that resolve to the same key attribute value are merged together. + data = {} + records.each do |attributes| + key_value = attributes[support_table_key_attribute].to_s + existing = data[key_value] + if existing + existing.merge!(attributes) + else + data[key_value] = attributes end end @@ -456,7 +489,7 @@ def support_table_parse_data_file(file_path) else require "yaml" unless defined?(YAML) require "date" unless defined?(Date) - data = if Psych::VERSION.to_f >= 3.1 + data = if Gem::Version.new(Psych::VERSION) >= Gem::Version.new("3.1.0.pre1") YAML.safe_load(file_data, permitted_classes: [Date, Time], aliases: true) else # Positional arguments for Psych < 3.1 (Ruby 2.5). @@ -487,7 +520,7 @@ def support_table_record_changed?(record, seen = Set.new) # base class rather than crashing on uninitialized instance variables. def support_table_mutex - @mutex || (superclass.include?(SupportTableData) ? superclass.send(:support_table_mutex) : nil) + @support_table_mutex || (superclass.include?(SupportTableData) ? superclass.send(:support_table_mutex) : nil) end def support_table_data_files diff --git a/lib/support_table_data/documentation/source_file.rb b/lib/support_table_data/documentation/source_file.rb index 67591f0..8b0c7bb 100644 --- a/lib/support_table_data/documentation/source_file.rb +++ b/lib/support_table_data/documentation/source_file.rb @@ -32,7 +32,7 @@ def source # # @return [String] def source_without_yard_docs - "#{source.sub(YARD_COMMENT_REGEX, "").rstrip}#{trailing_newline}" + "#{source.gsub(YARD_COMMENT_REGEX, "").rstrip}#{trailing_newline}" end # Return the source code with the generated YARD documentation added. @@ -63,8 +63,10 @@ def source_with_yard_docs updated_source << "\n#{indent}end" if has_class_def updated_source << "\n#{indent}# rubocop:enable all" updated_source << "\n#{indent}#{END_YARD_COMMENT}" - updated_source << source[existing_yard_docs.end(0)..-1] - updated_source + # Strip out any duplicate generated blocks (i.e. left over from a bad merge) + # so that the file is left with exactly one block. + updated_source << source[existing_yard_docs.end(0)..].gsub(YARD_COMMENT_REGEX, "") + "#{updated_source.rstrip}#{trailing_newline}" else yard_comments = <<~SOURCE.chomp("\n") #{BEGIN_YARD_COMMENT} diff --git a/spec/support_table_data/documentation/source_file_spec.rb b/spec/support_table_data/documentation/source_file_spec.rb index 01c9b01..064f5c8 100644 --- a/spec/support_table_data/documentation/source_file_spec.rb +++ b/spec/support_table_data/documentation/source_file_spec.rb @@ -188,6 +188,36 @@ def some_method expect(result).to match(/class Color < ActiveRecord::Base.*# Begin YARD docs.*# @!method self\.black.*# End YARD docs.*def some_method/m) end + it "removes duplicated documentation blocks while preserving user code" do + source_with_duplicate_blocks = <<~RUBY + class Color < ActiveRecord::Base + include SupportTableData + + # Begin YARD docs for support_table_data + # Old docs + # End YARD docs for support_table_data + + def user_method + end + + # Begin YARD docs for support_table_data + # Old duplicated docs + # End YARD docs for support_table_data + end + RUBY + + source_file = SupportTableData::Documentation::SourceFile.new(Color, color_path) + allow(source_file).to receive(:source).and_return(source_with_duplicate_blocks) + + result = source_file.source_with_yard_docs + + expect(result).to include("def user_method") + expect(result).not_to include("# Old docs") + expect(result).not_to include("# Old duplicated docs") + expect(result.scan(SupportTableData::Documentation::SourceFile::BEGIN_YARD_COMMENT).size).to eq 1 + expect(result.scan(SupportTableData::Documentation::SourceFile::END_YARD_COMMENT).size).to eq 1 + end + it "preserves indentation when replacing inline YARD docs" do source_with_indented_docs = <<~RUBY class Color < ActiveRecord::Base diff --git a/spec/support_table_data_spec.rb b/spec/support_table_data_spec.rb index 88d6e5b..9233627 100644 --- a/spec/support_table_data_spec.rb +++ b/spec/support_table_data_spec.rb @@ -261,6 +261,13 @@ expect(Size.small_label).to eq "Small" expect(Size.large_label).to eq "Large" end + + it "syncs overridden attributes to the existing record instead of creating a new one" do + Size.sync_table_data! + expect(Size.count).to eq 3 + expect(Size.find(3).label).to eq "Large" + expect(Size.large_label).to eq Size.find(3).label + end end describe "single table inheritance" do @@ -364,6 +371,18 @@ }) end + it "merges overrides for a named instance that do not repeat the key attribute" do + data = Size.support_table_data + expect(data.size).to eq 3 + expect(data).to include({ + "id" => 3, + "name" => "large", + "label" => "Large", + "active" => false, + "introduced_on" => Date.new(2021, 6, 30) + }) + end + it "returns a fresh copy every call" do data_1 = Color.support_table_data data_2 = Color.support_table_data From ab35b0f5f74e6672a224fa3b5a2c8b378142be20 Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Thu, 6 Aug 2026 09:14:42 -0700 Subject: [PATCH 05/12] Update actions --- .github/dependabot.yml | 8 +++++--- .github/workflows/continuous_integration.yml | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index d0f2e7a..1ec0b9b 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -1,4 +1,3 @@ -# Dependabot update strategy version: 2 updates: - package-ecosystem: bundler @@ -6,7 +5,10 @@ updates: schedule: interval: weekly allow: - # Automatically keep all runtime dependencies updated - dependency-name: "*" dependency-type: "production" - versioning-strategy: lockfile-only + versioning-strategy: increase-if-necessary + - package-ecosystem: github-actions + directory: "/" + schedule: + interval: weekly diff --git a/.github/workflows/continuous_integration.yml b/.github/workflows/continuous_integration.yml index 91a961c..653dadf 100644 --- a/.github/workflows/continuous_integration.yml +++ b/.github/workflows/continuous_integration.yml @@ -36,7 +36,7 @@ jobs: appraisal: "activerecord_6.1" steps: - name: Checkout - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false - name: Ruby CI From f7c21229f4eef2d3f6b25f002b5a81bd64a8a993 Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Sun, 9 Aug 2026 09:41:45 -0700 Subject: [PATCH 06/12] update ci gemfiles --- gemfiles/activerecord_6.1.gemfile | 2 +- gemfiles/activerecord_7.0.gemfile | 2 +- gemfiles/activerecord_7.1.gemfile | 2 +- gemfiles/activerecord_7.2.gemfile | 2 +- gemfiles/activerecord_8.0.gemfile | 2 +- gemfiles/activerecord_8.1.gemfile | 2 +- lib/support_table_data.rb | 10 +++------- spec/spec_helper.rb | 20 +++++++++++++++++++- spec/support_table_data_spec.rb | 2 +- 9 files changed, 29 insertions(+), 15 deletions(-) diff --git a/gemfiles/activerecord_6.1.gemfile b/gemfiles/activerecord_6.1.gemfile index 0d454b9..e3fea66 100644 --- a/gemfiles/activerecord_6.1.gemfile +++ b/gemfiles/activerecord_6.1.gemfile @@ -3,9 +3,9 @@ source "https://rubygems.org" gem "sqlite3", "~> 1.4.0" +gem "csv" gem "rake" gem "rspec", "~> 3.11" -gem "csv" gem "activerecord", "~> 6.1.0" gem "concurrent-ruby", "1.3.4" diff --git a/gemfiles/activerecord_7.0.gemfile b/gemfiles/activerecord_7.0.gemfile index a4692d1..8da0b1f 100644 --- a/gemfiles/activerecord_7.0.gemfile +++ b/gemfiles/activerecord_7.0.gemfile @@ -3,9 +3,9 @@ source "https://rubygems.org" gem "sqlite3", "~> 1.4.0" +gem "csv" gem "rake" gem "rspec", "~> 3.11" -gem "csv" gem "activerecord", "~> 7.0.0" gem "concurrent-ruby", "1.3.4" diff --git a/gemfiles/activerecord_7.1.gemfile b/gemfiles/activerecord_7.1.gemfile index a4692d1..8da0b1f 100644 --- a/gemfiles/activerecord_7.1.gemfile +++ b/gemfiles/activerecord_7.1.gemfile @@ -3,9 +3,9 @@ source "https://rubygems.org" gem "sqlite3", "~> 1.4.0" +gem "csv" gem "rake" gem "rspec", "~> 3.11" -gem "csv" gem "activerecord", "~> 7.0.0" gem "concurrent-ruby", "1.3.4" diff --git a/gemfiles/activerecord_7.2.gemfile b/gemfiles/activerecord_7.2.gemfile index a4692d1..8da0b1f 100644 --- a/gemfiles/activerecord_7.2.gemfile +++ b/gemfiles/activerecord_7.2.gemfile @@ -3,9 +3,9 @@ source "https://rubygems.org" gem "sqlite3", "~> 1.4.0" +gem "csv" gem "rake" gem "rspec", "~> 3.11" -gem "csv" gem "activerecord", "~> 7.0.0" gem "concurrent-ruby", "1.3.4" diff --git a/gemfiles/activerecord_8.0.gemfile b/gemfiles/activerecord_8.0.gemfile index 5b2f385..b3558f7 100644 --- a/gemfiles/activerecord_8.0.gemfile +++ b/gemfiles/activerecord_8.0.gemfile @@ -3,9 +3,9 @@ source "https://rubygems.org" gem "sqlite3", "~> 2.5.0" +gem "csv" gem "rake" gem "rspec", "~> 3.11" -gem "csv" gem "activerecord", "~> 8.0.0" gemspec path: "../" diff --git a/gemfiles/activerecord_8.1.gemfile b/gemfiles/activerecord_8.1.gemfile index db2737e..396f24a 100644 --- a/gemfiles/activerecord_8.1.gemfile +++ b/gemfiles/activerecord_8.1.gemfile @@ -3,9 +3,9 @@ source "https://rubygems.org" gem "sqlite3", "~> 2.9.0" +gem "csv" gem "rake" gem "rspec", "~> 3.11" -gem "csv" gem "activerecord", "~> 8.1.0" gemspec path: "../" diff --git a/lib/support_table_data.rb b/lib/support_table_data.rb index f86d385..6527950 100644 --- a/lib/support_table_data.rb +++ b/lib/support_table_data.rb @@ -464,9 +464,10 @@ def define_support_table_predicates_helper(method_name, attribute_name, attribut raise ArgumentError.new("Could not define support table helper method #{name}##{method_name} because it is already a defined method") end + cast_value = type_for_attribute(attribute_name).cast(attribute_value) class_eval <<~RUBY, __FILE__, __LINE__ + 1 def #{method_name} - #{attribute_name} == self.class.type_for_attribute(#{attribute_name.inspect}).cast(#{attribute_value.inspect}) + #{attribute_name} == #{cast_value.inspect} end RUBY end @@ -489,12 +490,7 @@ def support_table_parse_data_file(file_path) else require "yaml" unless defined?(YAML) require "date" unless defined?(Date) - data = if Gem::Version.new(Psych::VERSION) >= Gem::Version.new("3.1.0.pre1") - YAML.safe_load(file_data, permitted_classes: [Date, Time], aliases: true) - else - # Positional arguments for Psych < 3.1 (Ruby 2.5). - YAML.safe_load(file_data, [Date, Time], [], true) - end + data = YAML.safe_load(file_data, permitted_classes: [Date, Time], aliases: true) end data diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index fe9b1cd..cdc87af 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,7 +1,21 @@ -require "bundler/setup" +# frozen_string_literal: true + +ENV["BUNDLE_GEMFILE"] ||= File.expand_path("../Gemfile", __dir__) + +require "bundler/setup" if File.exist?(ENV["BUNDLE_GEMFILE"]) require "active_record" +begin + require "simplecov" + SimpleCov.start do + skip ["/spec/"] + end +rescue LoadError +end + +Bundler.require(:default, :test) + ActiveRecord::Base.establish_connection("adapter" => "sqlite3", "database" => ":memory:") require_relative "../lib/support_table_data" @@ -11,7 +25,11 @@ require_relative "models" RSpec.configure do |config| + config.warnings = true + config.disable_monkey_patching! + config.default_formatter = "doc" if config.files_to_run.one? config.order = :random + Kernel.srand config.seed config.before do Thing.delete_all diff --git a/spec/support_table_data_spec.rb b/spec/support_table_data_spec.rb index 9233627..73ba3a9 100644 --- a/spec/support_table_data_spec.rb +++ b/spec/support_table_data_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -describe SupportTableData do +RSpec.describe SupportTableData do let(:red) { Color.find_by(name: "Red") } let(:green) { Color.find_by(name: "Green") } let(:blue) { Color.find_by(name: "Blue") } From 06acc645b04a88610b42e806e97908e3b8c42d8f Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Sun, 9 Aug 2026 09:55:04 -0700 Subject: [PATCH 07/12] fix tests --- .github/workflows/continuous_integration.yml | 2 +- lib/support_table_data.rb | 3 +-- lib/support_table_data/tasks/utils.rb | 2 ++ 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/continuous_integration.yml b/.github/workflows/continuous_integration.yml index 653dadf..2bf6d15 100644 --- a/.github/workflows/continuous_integration.yml +++ b/.github/workflows/continuous_integration.yml @@ -32,7 +32,7 @@ jobs: appraisal: "activerecord_7.1" - ruby: "2.7" appraisal: "activerecord_7.0" - - ruby: "2.5" + - ruby: "2.6" appraisal: "activerecord_6.1" steps: - name: Checkout diff --git a/lib/support_table_data.rb b/lib/support_table_data.rb index 6527950..b1e21ac 100644 --- a/lib/support_table_data.rb +++ b/lib/support_table_data.rb @@ -464,10 +464,9 @@ def define_support_table_predicates_helper(method_name, attribute_name, attribut raise ArgumentError.new("Could not define support table helper method #{name}##{method_name} because it is already a defined method") end - cast_value = type_for_attribute(attribute_name).cast(attribute_value) class_eval <<~RUBY, __FILE__, __LINE__ + 1 def #{method_name} - #{attribute_name} == #{cast_value.inspect} + #{attribute_name} == self.class.type_for_attribute(#{attribute_name.inspect}).cast(#{attribute_value.inspect}) end RUBY end diff --git a/lib/support_table_data/tasks/utils.rb b/lib/support_table_data/tasks/utils.rb index ce5328d..4a21f88 100644 --- a/lib/support_table_data/tasks/utils.rb +++ b/lib/support_table_data/tasks/utils.rb @@ -59,6 +59,8 @@ def support_table_rbs_files(file_path = nil) end def model_file_path(klass) + return nil unless klass.name + file_path = "#{klass.name.underscore}.rb" model_path = nil From 1fcb35206b030aeaa1383770b3a86d835584030c Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Mon, 10 Aug 2026 09:03:22 -0700 Subject: [PATCH 08/12] handle sti protected records --- CHANGELOG.md | 4 +- Rakefile | 2 +- lib/support_table_data.rb | 116 +++++++++++++++++++++++--------- spec/support_table_data_spec.rb | 70 +++++++++++++++++++ 4 files changed, 159 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a9e4a0..0c67597 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `named_instance_attribute_helpers` can now be called again with an attribute that was already registered without raising an `ArgumentError`. - Data files that override attributes on a named instance are now merged by the instance name rather than only by the key attribute. Previously, an override that did not repeat the key attribute value (for example, a file containing only `large:` with a `label`) was treated as a brand new record; the override was never applied to the real row and a row with only the overridden attributes was inserted on every sync. - YAML data files can now use anchors/aliases and date/time values. Previously these raised `Psych::AliasesNotEnabled` or `Psych::DisallowedClass` errors. -- `protected_instance?` no longer returns stale results when data files are added after the protected keys were first computed. +- `protected_instance?` and `instance_keys` no longer return stale results when data files are added after their values were first computed. This includes single table inheritance subclasses that computed their values before a data file was added to their base class. +- `protected_instance?` and `instance_keys` now include data files added to single table inheritance subclasses when called on a base class. Previously rows managed by a subclass' data file were reported as unprotected by the base class even though they live in the same table. +- `sync_table_data!` with `delete_missing: true` no longer deletes rows that are managed by data files added to single table inheritance subclasses. Previously syncing the base class deleted rows that the subclass was responsible for syncing. - Fixed broken cycle detection in the autosave association check during syncs that could cause infinite recursion on cyclic autosave associations. - `sync_table_data!` now retries once on `ActiveRecord::RecordNotUnique` errors caused by concurrent syncs inserting the same rows from another process. - `sync_table_data!` now returns an empty array instead of `nil` when the table does not exist. diff --git a/Rakefile b/Rakefile index 48f3eb5..c12e547 100644 --- a/Rakefile +++ b/Rakefile @@ -30,7 +30,7 @@ namespace :appraisal do File.delete(file) if File.file?(file) end - system "bundle exec appraisal generate" || abort("appraisal generate failed") + system("bundle exec appraisal generate") || abort("appraisal generate failed") Dir.glob("gemfiles/*.gemfile") do |file| puts "Locking #{file}" diff --git a/lib/support_table_data.rb b/lib/support_table_data.rb index b1e21ac..74507b1 100644 --- a/lib/support_table_data.rb +++ b/lib/support_table_data.rb @@ -146,7 +146,15 @@ def sync_table_data!(delete_missing: false) end if delete_missing - where.not(primary_key => synced_ids).destroy_all + missing_records = where.not(primary_key => synced_ids) + + # Rows managed by data files added to single table inheritance subclasses live in + # this table, but they are synced by the subclass, so they are not missing rows. + # Keys from this class' own data files are already excluded by the synced ids. + managed_keys = instance_keys.compact + missing_records = missing_records.where.not(support_table_key_attribute => managed_keys) unless managed_keys.empty? + + missing_records.destroy_all end end end @@ -174,8 +182,6 @@ def add_support_table_data(data_file_path) root_dir = support_table_data_directory || SupportTableData.data_directory || Dir.pwd support_table_mutex.synchronize do @support_table_data_files = support_table_data_files + [File.expand_path(data_file_path, root_dir)] - @support_table_instance_keys = nil - @protected_keys = nil end define_support_table_named_instances end @@ -215,10 +221,19 @@ def support_table_attribute_helpers # # @return [Array] List of attributes for all records in the data files. def support_table_data + support_table_data_for_files(support_table_data_files) + end + + # Get the data for the support table from a specific list of data files. + # + # @param data_files [Array] The paths of the data files to read. + # @return [Array] List of attributes for all records in the data files. + # @api private + def support_table_data_for_files(data_files) records = [] named_records = {} - support_table_data_files.each do |data_file_path| + data_files.each do |data_file_path| file_data = support_table_parse_data_file(data_file_path) if file_data.is_a?(Hash) @@ -306,43 +321,33 @@ def named_instance(instance_name) find_by!(support_table_key_attribute => instances[instance_name]) end - # Get the key values for all instances loaded from the data files. + # Get the key values for all instances loaded from the data files. Data files added to + # single table inheritance subclasses are included since those rows live in this table too. # # @return [Array] List of all the key attribute values. def instance_keys - keys = @support_table_instance_keys - if keys.nil? - support_table_mutex.synchronize do - keys = @support_table_instance_keys - if keys.nil? - values = [] - support_table_data.each do |attributes| - key_value = attributes[support_table_key_attribute] - instance = new - instance.send(:"#{support_table_key_attribute}=", key_value) - values << instance.send(support_table_key_attribute) - end - keys = values.uniq - @support_table_instance_keys = keys - end + support_table_cached_data_value(:@support_table_instance_keys, support_table_data_files_with_descendants) do |data_files| + values = [] + support_table_data_for_files(data_files).each do |attributes| + key_value = attributes[support_table_key_attribute] + instance = new + instance.send(:"#{support_table_key_attribute}=", key_value) + values << instance.send(support_table_key_attribute) end + values.uniq end - keys end - # Return true if the instance has data being managed from a data file. + # Return true if the instance has data being managed from a data file. Instances are matched + # on the key attribute only. Single table inheritance types are intentionally not considered + # since syncing also matches existing rows on just the key attribute and will overwrite a row + # regardless of the type it currently has. Data files added to single table inheritance + # subclasses are included since those rows live in this table too. # # @return [Boolean] def protected_instance?(instance) - keys = @protected_keys - if keys.nil? - support_table_mutex.synchronize do - keys = @protected_keys - if keys.nil? - keys = support_table_data.collect { |attributes| attributes[support_table_key_attribute].to_s } - @protected_keys = keys - end - end + keys = support_table_cached_data_value(:@support_table_protected_keys, support_table_data_files_with_descendants) do |data_files| + support_table_data_for_files(data_files).collect { |attributes| attributes[support_table_key_attribute].to_s } end keys.include?(instance[support_table_key_attribute].to_s) @@ -509,6 +514,55 @@ def support_table_record_changed?(record, seen = Set.new) end end + # Memoize a value calculated from the data files in an instance variable on this class. + # The list of data files used to calculate the value is cached along with it so that the + # value is recalculated whenever the list changes. This keeps the value from going stale + # when a data file is added after it was first calculated, including when the file is added + # to a base class after a single table inheritance subclass has cached its own copy or when + # a subclass that adds its own data files is loaded lazily. + # + # @param variable_name [Symbol] The name of the instance variable to memoize the value in. + # @param data_files [Array] The data files the value is calculated from. These are + # yielded to the block and cached with the value so it can be invalidated. + # @return [Object] The cached value. + def support_table_cached_data_value(variable_name, data_files) + cached = instance_variable_get(variable_name) + return cached.last if cached && cached.first == data_files + + support_table_mutex.synchronize do + cached = instance_variable_get(variable_name) + unless cached && cached.first == data_files + cached = [data_files.dup.freeze, yield(data_files)].freeze + instance_variable_set(variable_name, cached) + end + end + + cached.last + end + + # Get the list of data files for this class along with any added to single table inheritance + # subclasses. Rows from a subclass' data files live in the same table, so they need to be + # included when determining which rows in the table are managed from data files. + # + # Note that this can only detect subclasses that have already been loaded by the application. + # + # @return [Array] List of data file paths. + def support_table_data_files_with_descendants + files = support_table_data_files + + descendants.each do |subclass| + next unless subclass.include?(SupportTableData) + + subclass_files = subclass.send(:support_table_data_files) + # Subclasses without their own data files inherit the exact same array from this class. + next if subclass_files.equal?(files) + + files += (subclass_files - files) + end + + files + end + # The class level state used by the concern is stored in instance variables on the # class where the concern was included. These readers fall back to the superclass # so that single table inheritance subclasses share the state defined on their diff --git a/spec/support_table_data_spec.rb b/spec/support_table_data_spec.rb index 73ba3a9..49cf03f 100644 --- a/spec/support_table_data_spec.rb +++ b/spec/support_table_data_spec.rb @@ -281,6 +281,76 @@ expect(Polygon.rectangle.protected_instance?).to eq true expect(Triangle.new(name: "Scalene").protected_instance?).to eq false end + + it "protects subclass records regardless of the type in the data files" do + # Syncing matches existing rows on just the key attribute, so a record with a key value + # from the data files will be overwritten no matter what type it currently has. + expect(Triangle.new(name: "Rectangle").protected_instance?).to eq true + end + + it "does not delete rows managed by a subclass' data files when delete_missing is true" do + base_class = Class.new(ActiveRecord::Base) do + include SupportTableData + + self.table_name = "colors" + end + subclass = Class.new(base_class) + base_class.add_support_table_data("colors/named_colors.yml") + subclass.add_support_table_data("colors/colors.json") + + subclass.sync_table_data! + expect(base_class.where(id: [1, 3, 8, 9]).count).to eq 4 + + unmanaged = base_class.new + unmanaged.id = 99 + unmanaged.save! + + base_class.sync_table_data!(delete_missing: true) + + # Rows 8 and 9 are only in the subclass' data file, but they still live in this table. + expect(base_class.where(id: [8, 9]).count).to eq 2 + expect(base_class.exists?(99)).to eq false + end + + it "includes data files added to subclasses when called on the base class" do + base_class = Class.new(ActiveRecord::Base) do + include SupportTableData + + self.table_name = "colors" + end + subclass = Class.new(base_class) + base_class.add_support_table_data("colors/named_colors.yml") + + light_gray = base_class.new + light_gray.id = 8 + expect(base_class.protected_instance?(light_gray)).to eq false + expect(base_class.instance_keys).to_not include 8 + + # Rows from a subclass' data files live in the same table, so the base class needs to + # know about them as well. + subclass.add_support_table_data("colors/colors.json") + expect(base_class.protected_instance?(light_gray)).to eq true + expect(base_class.instance_keys).to include 8 + end + + it "picks up data files added to the base class after a subclass memoized its values" do + base_class = Class.new(ActiveRecord::Base) do + include SupportTableData + + self.table_name = "colors" + end + subclass = Class.new(base_class) + base_class.add_support_table_data("colors/named_colors.yml") + + light_gray = subclass.new + light_gray.id = 8 + expect(subclass.protected_instance?(light_gray)).to eq false + expect(subclass.instance_keys).to_not include 8 + + base_class.add_support_table_data("colors/colors.json") + expect(subclass.protected_instance?(light_gray)).to eq true + expect(subclass.instance_keys).to include 8 + end end describe "instance_names" do From 970aea783c070da0227e6e6842049b03e1dc254f Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Tue, 11 Aug 2026 08:06:13 -0700 Subject: [PATCH 09/12] Enhance support table data handling and documentation; add support for single table inheritance, improve error handling, and update YARD documentation format --- CHANGELOG.md | 4 + README.md | 4 +- lib/support_table_data.rb | 159 +++++++++++------- .../documentation/yard_doc.rb | 80 ++++----- lib/support_table_data/tasks/utils.rb | 27 ++- lib/tasks/support_table_data.rake | 2 +- spec/data/shapes.yml | 2 + spec/data/squares.yml | 2 + spec/models.rb | 7 + spec/models/shape.rb | 9 + spec/models/size.rb | 2 +- spec/models/square.rb | 5 + spec/spec_helper.rb | 1 + .../documentation/yard_doc_spec.rb | 75 +++++++-- spec/support_table_data_spec.rb | 76 ++++++++- 15 files changed, 324 insertions(+), 131 deletions(-) create mode 100644 spec/data/shapes.yml create mode 100644 spec/data/squares.yml create mode 100644 spec/models/shape.rb create mode 100644 spec/models/square.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c67597..99aa215 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `sync_table_data!` now emits a single `support_table_data.sync` notification per call. A sync that was retried after a uniqueness violation emitted two. - `sync_table_data!` with `delete_missing: true` now raises an `ArgumentError` instead of deleting every row in the table when the data files contain no rows (for example, when a data file was accidentally emptied or truncated). - Generated predicate methods (e.g. `record.active?`) now cast the data file value to the attribute type before comparing. Previously the raw file value was compared to the cast database attribute, so predicates silently returned `false` whenever the types differed (guaranteed for CSV data files, where all values are strings). - Single table inheritance subclasses no longer raise `NoMethodError` from `instance_names`, `instance_keys`, `protected_instance?`, and other class methods. Subclasses now share the support table state defined on their base class. @@ -27,6 +28,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The documentation tasks no longer corrupt model source files that contain duplicated generated YARD doc blocks (e.g. from a bad merge); the regex that finds the generated block is no longer greedy and all duplicated blocks are now removed so the file is left with exactly one block. - Data file names containing extra dots no longer break the class name detection used by `SupportTableData.sync_all!` to eager load models. - Error messages for invalid named instance definitions now include the model class name instead of repeating the instance name. +- Single table inheritance subclasses now properly inherit named instance helpers regardless of load order. +- The `:compact` YARD format now emits one comment block per method instead of `@!macro` invocations. YARD only expands a macro when it is attached to a method definition, so the macro form resolved to methods with no description, no `@return`, and no `@raise` tags, and it documented the predicate methods as class methods rather than instance methods. +- The documentation tasks no longer report success when a model raises an `ArgumentError` for an invalid named instance definition. The rescue that produced an empty list of source files covered the whole lookup rather than just the file path expansion it was meant to guard. ## 1.6.1 diff --git a/README.md b/README.md index 319852a..c6b775d 100644 --- a/README.md +++ b/README.md @@ -205,7 +205,7 @@ A good practice is to add a check to your CI pipeline to ensure the documentatio Each model can choose how its YARD docs are generated by setting `support_table_yard_docs` to one of three values: - `:full` — verbose comment block per generated method (the default) -- `:compact` — shared `@!macro` definitions at the top of the documentation block plus a short `@!method` / `@!macro` pair per generated method. IDEs and `yard doc` resolve the macros into the same per-method documentation as `:full`. Useful when a model has many named instances and the verbose comment block is too long to be useful inline. +- `:compact` — the same `@!method` and `@!return` tags per generated method as `:full`, but without the prose descriptions. YARD resolves it to exactly the same methods and return types as `:full`. Useful when a model has many named instances and the verbose comment block is too long to be useful inline. - `:none` — generate no YARD docs for this model. The rake task will also strip any previously generated YARD docs from the source file. ```ruby @@ -283,6 +283,8 @@ SupportTableData.sync_all!(delete_missing: true) As a safeguard, `sync_table_data!` will raise an `ArgumentError` rather than delete anything when `delete_missing` is enabled but the data files contain no rows (for instance, when a data file was accidentally emptied or truncated). +Rows managed by data files added to a single table inheritance subclass are never deleted by a sync on the base class, even if the subclass has not been loaded yet. Each candidate row is resolved to its own class through the inheritance column before it is deleted, so the subclass is loaded on demand and gets to declare which rows it owns. + It is recommended to add a unique database index on the key attribute column. Concurrent syncs from multiple processes (for example, parallel deployment jobs) could otherwise insert duplicate rows. If a sync hits a uniqueness violation from a concurrent insert, it will automatically retry once to pick up the other process' changes. The number of records contained in data files should be fairly small (ideally fewer than 100). It is possible to load just a subset of rows in a large table because only the rows listed in the data files will be synced. You can use this feature if your table allows user-entered data, but has a few rows that must exist for the code to work. diff --git a/lib/support_table_data.rb b/lib/support_table_data.rb index 74507b1..6a2c035 100644 --- a/lib/support_table_data.rb +++ b/lib/support_table_data.rb @@ -103,7 +103,9 @@ def sync_table_data!(delete_missing: false) retried = false - begin + # The instrumentation wraps the retry so that one call emits one event even if the + # sync has to be attempted twice. + ActiveSupport::Notifications.instrument("support_table_data.sync", class: self) do canonical_data = support_table_data.each_with_object({}) do |attributes, hash| hash[attributes[support_table_key_attribute].to_s] = attributes end @@ -114,50 +116,38 @@ def sync_table_data!(delete_missing: false) records = where(support_table_key_attribute => canonical_data.keys) changes = [] - - ActiveSupport::Notifications.instrument("support_table_data.sync", class: self) do - synced_ids = [] - - transaction do - records.each do |record| - key = record[support_table_key_attribute].to_s - attributes = canonical_data.delete(key) - attributes&.each do |name, value| - record.send(:"#{name}=", value) if record.respond_to?(:"#{name}=", true) - end - if support_table_record_changed?(record) - changes << record.changes - record.save! - end - - synced_ids << record.id if attributes + synced_ids = [] + + transaction do + records.each do |record| + key = record[support_table_key_attribute].to_s + attributes = canonical_data.delete(key) + attributes&.each do |name, value| + record.send(:"#{name}=", value) if record.respond_to?(:"#{name}=", true) end - - canonical_data.each_value do |attributes| - class_name = attributes[inheritance_column] - klass = class_name ? sti_class_for(class_name) : self - record = klass.new - attributes.each do |name, value| - record.send(:"#{name}=", value) if record.respond_to?(:"#{name}=", true) - end + if support_table_record_changed?(record) changes << record.changes record.save! - synced_ids << record.id end - if delete_missing - missing_records = where.not(primary_key => synced_ids) - - # Rows managed by data files added to single table inheritance subclasses live in - # this table, but they are synced by the subclass, so they are not missing rows. - # Keys from this class' own data files are already excluded by the synced ids. - managed_keys = instance_keys.compact - missing_records = missing_records.where.not(support_table_key_attribute => managed_keys) unless managed_keys.empty? + synced_ids << record.id if attributes + end - missing_records.destroy_all + canonical_data.each_value do |attributes| + class_name = attributes[inheritance_column] + klass = class_name ? sti_class_for(class_name) : self + record = klass.new + attributes.each do |name, value| + record.send(:"#{name}=", value) if record.respond_to?(:"#{name}=", true) end + changes << record.changes + record.save! + synced_ids << record.id end + + delete_missing_records(where.not(primary_key => synced_ids)) if delete_missing end + changes rescue ActiveRecord::RecordInvalid => e raise SupportTableData::ValidationError.new(e.record) @@ -340,17 +330,23 @@ def instance_keys # Return true if the instance has data being managed from a data file. Instances are matched # on the key attribute only. Single table inheritance types are intentionally not considered - # since syncing also matches existing rows on just the key attribute and will overwrite a row - # regardless of the type it currently has. Data files added to single table inheritance - # subclasses are included since those rows live in this table too. + # when matching since syncing also matches existing rows on just the key attribute and will + # overwrite a row regardless of the type it currently has. Data files added to single table + # inheritance subclasses are included since those rows live in this table too. # # @return [Boolean] def protected_instance?(instance) - keys = support_table_cached_data_value(:@support_table_protected_keys, support_table_data_files_with_descendants) do |data_files| - support_table_data_for_files(data_files).collect { |attributes| attributes[support_table_key_attribute].to_s } - end + key = instance[support_table_key_attribute].to_s + return true if support_table_protected_keys.include?(key) + + # The instance may be a single table inheritance subclass that had not been loaded yet + # when the keys for this class were calculated. Loading a row instantiates it as its own + # subclass, so the subclass can be asked directly rather than relying on it having been + # eager loaded before this point. + instance_class = instance.class + return false if instance_class == self || !instance_class.include?(SupportTableData) - keys.include?(instance[support_table_key_attribute].to_s) + instance_class.send(:support_table_protected_keys).include?(key) end # Explicitly define other support tables that this model depends on. A support table depends @@ -371,6 +367,34 @@ def support_table_dependency(*class_names) private + # Destroy the rows in a relation that are not managed from a data file. + # + # Rows managed by data files added to single table inheritance subclasses live in this + # table, but they are synced by the subclass, so they must not be deleted here. Loading a + # row instantiates it as its own subclass, which loads that class if it hasn't been loaded + # yet, so `protected_instance?` can consult the subclass directly without having to eager + # load the entire application first. + # + # @param relation [ActiveRecord::Relation] The rows that are candidates for deletion. + # @return [void] + def delete_missing_records(relation) + relation.find_each do |record| + next if protected_instance?(record) + + record.destroy + end + end + + # The key attribute values of every row managed from this class' data files, including + # data files added to single table inheritance subclasses that have already been loaded. + # + # @return [Array] + def support_table_protected_keys + support_table_cached_data_value(:@support_table_protected_keys, support_table_data_files_with_descendants) do |data_files| + support_table_data_for_files(data_files).collect { |attributes| attributes[support_table_key_attribute].to_s } + end + end + def define_support_table_named_instances merged_data = {} @@ -434,6 +458,11 @@ def define_support_table_named_instance_methods(name, attributes) end end + # Values from the data files are captured in the method closures rather than being + # interpolated into the method body as literals. Not every value that can appear in a + # data file has an `inspect` representation that is valid Ruby source (`Date` and `Time` + # are the notable ones), so interpolating them would raise a SyntaxError when the method + # is defined. def define_support_table_instance_helper(method_name, attribute_name, attribute_value, redefine: false) if redefine singleton_class.silence_redefinition_of_method(method_name) @@ -441,11 +470,10 @@ def define_support_table_instance_helper(method_name, attribute_name, attribute_ raise ArgumentError.new("Could not define support table helper method #{name}.#{method_name} because it is already a defined method") end - class_eval <<~RUBY, __FILE__, __LINE__ + 1 - def self.#{method_name} - find_by!(#{attribute_name}: #{attribute_value.inspect}) - end - RUBY + attribute_name = attribute_name.to_s + singleton_class.send(:define_method, method_name) do + find_by!(attribute_name => attribute_value) + end end def define_support_table_instance_attribute_helper(method_name, attribute_value, redefine: false) @@ -455,11 +483,10 @@ def define_support_table_instance_attribute_helper(method_name, attribute_value, raise ArgumentError.new("Could not define support table helper method #{name}.#{method_name} because it is already a defined method") end - class_eval <<~RUBY, __FILE__, __LINE__ + 1 - def self.#{method_name} - #{attribute_value.inspect}.freeze - end - RUBY + # The value is returned directly on every call, so it is copied and frozen to keep + # callers from mutating the data shared by all of them. + value = support_table_deep_freeze(attribute_value.dup) + singleton_class.send(:define_method, method_name) { value } end def define_support_table_predicates_helper(method_name, attribute_name, attribute_value, redefine: false) @@ -469,11 +496,29 @@ def define_support_table_predicates_helper(method_name, attribute_name, attribut raise ArgumentError.new("Could not define support table helper method #{name}##{method_name} because it is already a defined method") end - class_eval <<~RUBY, __FILE__, __LINE__ + 1 - def #{method_name} - #{attribute_name} == self.class.type_for_attribute(#{attribute_name.inspect}).cast(#{attribute_value.inspect}) + attribute_name = attribute_name.to_s + # The value has to be cast to the attribute type before it can be compared to the value + # read off the record. The cast is done lazily and memoized per class because the type + # requires a database connection to resolve, which is not necessarily available when the + # model class is being loaded. + cast_values = {} + define_method(method_name) do + klass = self.class + cast_value = cast_values.fetch(klass) do + cast_values[klass] = klass.type_for_attribute(attribute_name).cast(attribute_value) end - RUBY + send(attribute_name) == cast_value + end + end + + def support_table_deep_freeze(value) + case value + when Hash + value.each_value { |element| support_table_deep_freeze(element) } + when Array + value.each { |element| support_table_deep_freeze(element) } + end + value.freeze end def support_table_parse_data_file(file_path) diff --git a/lib/support_table_data/documentation/yard_doc.rb b/lib/support_table_data/documentation/yard_doc.rb index 0908bbd..ee0a06b 100644 --- a/lib/support_table_data/documentation/yard_doc.rb +++ b/lib/support_table_data/documentation/yard_doc.rb @@ -3,10 +3,6 @@ module SupportTableData module Documentation class YardDoc - MACRO_FINDER = "support_table_data_finder" - MACRO_PREDICATE = "support_table_data_predicate" - MACRO_ATTRIBUTE = "support_table_data_attribute" - # @param klass [Class] The model class to generate documentation for def initialize(klass) @klass = klass @@ -16,8 +12,7 @@ def initialize(klass) # is controlled by the model's `support_table_yard_docs` setting: # # * `:full` - verbose comment block per method (default) - # * `:compact` - shared @!macro definitions plus a short @!method/@!macro - # pair per generated method + # * `:compact` - the same tags per method without the prose descriptions # * `:none` - generate no docs at all # # @return [String, nil] The YARD documentation, or nil if no docs should @@ -107,16 +102,23 @@ def generate_verbose_yard_docs(instance_names) yard_lines.join("\n") end + # The compact format drops the prose description from each method and keeps only the + # tags. Every method still needs its own comment block separated by a blank line: + # YARD builds one docstring per block, so combining methods into a single block would + # both drop the tags for all but the last method and leak the `self.` scope of a + # singleton method onto the instance methods that follow it. def generate_compact_yard_docs(instance_names) yard_lines = ["# @!group Named Instances"] - yard_lines << "" - yard_lines << compact_preamble - yard_lines << "" - yard_lines << compact_macro_definitions instance_names.sort.each do |name| yard_lines << "" - yard_lines << compact_instance_block(name) + yard_lines << compact_instance_helper_yard_doc(name) + yard_lines << "" + yard_lines << compact_predicate_helper_yard_doc(name) + klass.support_table_attribute_helpers.each do |attribute_name| + yard_lines << "" + yard_lines << compact_attribute_helper_yard_doc(name, attribute_name) + end end yard_lines << "" @@ -125,53 +127,29 @@ def generate_compact_yard_docs(instance_names) yard_lines.join("\n") end - def compact_preamble + def compact_instance_helper_yard_doc(name) <<~YARD.chomp("\n") - # The methods in this group are dynamically defined by support_table_data - # for each named instance in the data file. The macros below are the - # documentation templates; the per-instance @!method lines that follow - # invoke them with the instance name (and attribute name, where applicable). + # @!method self.#{name} + # @return [#{klass.name}] + # @raise [ActiveRecord::RecordNotFound] if the record does not exist + # @!visibility public YARD end - def compact_macro_definitions - attribute_macro = <<~YARD.chomp("\n") - # @!macro [new] #{MACRO_ATTRIBUTE} - # Get the +$2+ attribute from the data file for the named instance +$1+. - # @return [$3] - # @!visibility public - YARD - - finder_macro = <<~YARD.chomp("\n") - # @!macro [new] #{MACRO_FINDER} - # Find the named instance +$1+ from the database. - # @return [#{klass.name}] - # @raise [ActiveRecord::RecordNotFound] if the record does not exist - # @!visibility public - YARD - - predicate_macro = <<~YARD.chomp("\n") - # @!macro [new] #{MACRO_PREDICATE} - # Check if this record is the named instance +$1+. - # @return [Boolean] - # @!visibility public + def compact_predicate_helper_yard_doc(name) + <<~YARD.chomp("\n") + # @!method #{name}? + # @return [Boolean] + # @!visibility public YARD - - [finder_macro, "", predicate_macro, "", attribute_macro].join("\n") end - def compact_instance_block(name) - lines = [] - lines << "# @!method self.#{name}" - lines << "# @!macro #{MACRO_FINDER} #{name}" - lines << "# @!method #{name}?" - lines << "# @!macro #{MACRO_PREDICATE} #{name}" - klass.support_table_attribute_helpers.each do |attribute_name| - return_type = attribute_yard_return_type(name, attribute_name) - lines << "# @!method self.#{name}_#{attribute_name}" - lines << "# @!macro #{MACRO_ATTRIBUTE} #{name} #{attribute_name} #{return_type}" - end - lines.join("\n") + def compact_attribute_helper_yard_doc(name, attribute_name) + <<~YARD.chomp("\n") + # @!method self.#{name}_#{attribute_name} + # @return [#{attribute_yard_return_type(name, attribute_name)}] + # @!visibility public + YARD end def attribute_yard_return_type(name, attribute_name) diff --git a/lib/support_table_data/tasks/utils.rb b/lib/support_table_data/tasks/utils.rb index 4a21f88..174eec5 100644 --- a/lib/support_table_data/tasks/utils.rb +++ b/lib/support_table_data/tasks/utils.rb @@ -23,14 +23,18 @@ def eager_load! # @param file_path [String, Pathname, nil] Optional file path to filter by. # @return [Array] def support_table_sources(file_path = nil) - file_path = Pathname.new(file_path) if file_path.is_a?(String) - require file_path.expand_path if file_path + resolved_path = expand_file_path(file_path) + return [] if file_path && resolved_path.nil? + + require resolved_path.to_s if resolved_path sources = [] ActiveRecord::Base.descendants.each do |klass| next unless klass.include?(SupportTableData) - # Skip STI subclasses; only the class that owns the data files gets documented. + # Only the class that added the data files is documented. Single table inheritance + # subclasses that don't add their own data files inherit the helper methods and + # would otherwise duplicate the base class documentation. next unless klass.instance_variable_defined?(:@support_table_data_files) next if klass.instance_names.empty? @@ -40,12 +44,9 @@ def support_table_sources(file_path = nil) sources << Documentation::SourceFile.new(klass, model_file_path) end - return sources if file_path.nil? + return sources if resolved_path.nil? - resolved_path = Pathname.new(file_path.to_s).expand_path sources.select { |source| source.path.expand_path == resolved_path } - rescue ArgumentError - [] end # Return RBS file handlers for all support table models. @@ -58,6 +59,18 @@ def support_table_rbs_files(file_path = nil) end end + # Expand a file path argument to an absolute path. + # + # @param file_path [String, Pathname, nil] + # @return [Pathname, nil] nil if no path was given or it cannot be expanded. + def expand_file_path(file_path) + return nil if file_path.nil? + + Pathname.new(file_path.to_s).expand_path + rescue ArgumentError + nil + end + def model_file_path(klass) return nil unless klass.name diff --git a/lib/tasks/support_table_data.rake b/lib/tasks/support_table_data.rake index 7e4031e..67eba82 100644 --- a/lib/tasks/support_table_data.rake +++ b/lib/tasks/support_table_data.rake @@ -80,8 +80,8 @@ namespace :support_table_data do SupportTableData::Tasks::Utils.eager_load! SupportTableData::Tasks::Utils.support_table_rbs_files(args[:file_path]).each do |rbs_file| next if rbs_file.up_to_date? + next unless rbs_file.write! - rbs_file.write! puts "Wrote RBS signatures for #{rbs_file.klass.name} to #{rbs_file.path}." end end diff --git a/spec/data/shapes.yml b/spec/data/shapes.yml new file mode 100644 index 0000000..3e7be03 --- /dev/null +++ b/spec/data/shapes.yml @@ -0,0 +1,2 @@ +circle: + name: Circle diff --git a/spec/data/squares.yml b/spec/data/squares.yml new file mode 100644 index 0000000..90692a2 --- /dev/null +++ b/spec/data/squares.yml @@ -0,0 +1,2 @@ +square: + name: Square diff --git a/spec/models.rb b/spec/models.rb index a398752..e5f265f 100644 --- a/spec/models.rb +++ b/spec/models.rb @@ -47,6 +47,11 @@ t.integer :side_count end + connection.create_table(:shapes) do |t| + t.string :name + t.string :type + end + connection.create_table(:sizes) do |t| t.string :name t.string :label @@ -64,6 +69,8 @@ autoload :Polygon, File.expand_path("models/polygon.rb", __dir__) autoload :Rectangle, File.expand_path("models/rectangle.rb", __dir__) autoload :Shade, File.expand_path("models/shade.rb", __dir__) +autoload :Shape, File.expand_path("models/shape.rb", __dir__) +autoload :Square, File.expand_path("models/square.rb", __dir__) autoload :Size, File.expand_path("models/size.rb", __dir__) autoload :ShadeHue, File.expand_path("models/shade_hue.rb", __dir__) autoload :Thing, File.expand_path("models/thing.rb", __dir__) diff --git a/spec/models/shape.rb b/spec/models/shape.rb new file mode 100644 index 0000000..80a7a6b --- /dev/null +++ b/spec/models/shape.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class Shape < ActiveRecord::Base + include SupportTableData + + self.support_table_key_attribute = :name + + add_support_table_data "shapes.yml" +end diff --git a/spec/models/size.rb b/spec/models/size.rb index 9be621d..1b3b205 100644 --- a/spec/models/size.rb +++ b/spec/models/size.rb @@ -3,7 +3,7 @@ class Size < ActiveRecord::Base include SupportTableData - named_instance_attribute_helpers :label + named_instance_attribute_helpers :label, :introduced_on add_support_table_data "sizes.yml" add_support_table_data "sizes_override.yml" diff --git a/spec/models/square.rb b/spec/models/square.rb new file mode 100644 index 0000000..8bbefbc --- /dev/null +++ b/spec/models/square.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class Square < Shape + add_support_table_data "squares.yml" +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index cdc87af..b05b020 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -36,5 +36,6 @@ Hue.delete_all Group.delete_all Color.delete_all + Shape.delete_all end end diff --git a/spec/support_table_data/documentation/yard_doc_spec.rb b/spec/support_table_data/documentation/yard_doc_spec.rb index a815e8b..5a1a986 100644 --- a/spec/support_table_data/documentation/yard_doc_spec.rb +++ b/spec/support_table_data/documentation/yard_doc_spec.rb @@ -3,6 +3,35 @@ require "spec_helper" RSpec.describe SupportTableData::Documentation::YardDoc do + # Run the generated documentation through YARD and return the methods it resolved as a hash + # of method path (i.e. "Color.red" or "Color#red?") to its sorted list of tags. Generated + # docs are only useful if YARD actually attaches the tags to the right method, which cannot + # be verified by matching against the generated text. + def yard_methods_for(klass) + begin + require "yard" + require "tmpdir" + rescue LoadError + skip "yard is not available" + end + + docs = SupportTableData::Documentation::YardDoc.new(klass).named_instance_yard_docs + body = docs.split("\n").collect { |line| line.empty? ? line : " #{line}" }.join("\n") + + Dir.mktmpdir do |dir| + path = File.join(dir, "#{klass.name.underscore}.rb") + File.write(path, "class #{klass.name}\n#{body}\nend\n") + + YARD::Registry.clear + YARD::Registry.load([path], true) + YARD::Registry.all(:method).each_with_object({}) do |method, hash| + hash[method.path] = method.tags.collect { |tag| [tag.tag_name, tag.types] }.sort + end + end + ensure + YARD::Registry.clear if defined?(YARD::Registry) + end + describe "#instance_helper_yard_doc" do it "generates YARD documentation for a named instance class method" do doc = SupportTableData::Documentation::YardDoc.new(Color) @@ -122,7 +151,7 @@ end end - it "emits the compact macro form" do + it "emits the tags for each method without the prose descriptions" do doc = SupportTableData::Documentation::YardDoc.new(Color) result = doc.named_instance_yard_docs @@ -130,16 +159,33 @@ expect(result).to include("# @!group Named Instances") expect(result).to include("# @!endgroup") - expect(result).to include("# @!macro [new] support_table_data_finder") - expect(result).to include("# @!macro [new] support_table_data_predicate") - expect(result).to include("# @!macro [new] support_table_data_attribute") - expect(result.scan("# @!macro [new] support_table_data_finder").size).to eq(1) - expect(result).to include("# @!method self.red") - expect(result).to include("# @!macro support_table_data_finder red") expect(result).to include("# @!method red?") - expect(result).to include("# @!macro support_table_data_predicate red") + expect(result).to include("# @return [Color]") + expect(result).to include("# @return [Boolean]") expect(result).not_to include("# Find the named instance +red+ from the database.") + expect(result).not_to include("@!macro") + end + + it "generates docs that YARD resolves to the same methods and tags as the full format" do + compact = yard_methods_for(Color) + + Color.support_table_yard_docs = :full + full = yard_methods_for(Color) + + expect(compact.keys).to match_array(full.keys) + expect(compact.keys).to include("Color.red", "Color#red?") + + compact.each do |path, tags| + expect(tags).to eq(full.fetch(path)), "expected #{path} to have the same tags in both formats" + end + end + + it "documents the predicate as an instance method and the finder as a class method" do + methods = yard_methods_for(Color) + + expect(methods["Color.red"]).to eq([["return", ["Color"]], ["raise", ["ActiveRecord::RecordNotFound"]]].sort) + expect(methods["Color#red?"]).to eq([["return", ["Boolean"]]]) end end @@ -154,13 +200,20 @@ end end - it "emits attribute macro invocations with column-derived types" do + it "emits attribute helper methods with data-derived types" do doc = SupportTableData::Documentation::YardDoc.new(Group) result = doc.named_instance_yard_docs # Group has attribute helpers for group_id (integer) and name (string). - expect(result).to include("# @!macro support_table_data_attribute primary group_id Integer") - expect(result).to include("# @!macro support_table_data_attribute primary name String") + expect(result).to include("# @!method self.primary_group_id\n# @return [Integer]") + expect(result).to include("# @!method self.primary_name\n# @return [String]") + end + + it "generates attribute helper docs that YARD resolves with the right return types" do + methods = yard_methods_for(Group) + + expect(methods["Group.primary_group_id"]).to eq([["return", ["Integer"]]]) + expect(methods["Group.primary_name"]).to eq([["return", ["String"]]]) end end diff --git a/spec/support_table_data_spec.rb b/spec/support_table_data_spec.rb index 49cf03f..891a0ce 100644 --- a/spec/support_table_data_spec.rb +++ b/spec/support_table_data_spec.rb @@ -114,8 +114,17 @@ raise ActiveRecord::RecordNotUnique.new("duplicate key") if calls == 1 original.call(*args, &block) end - expect(Group.sync_table_data!.size).to eq 3 + events = [] + subscriber = ActiveSupport::Notifications.subscribe("support_table_data.sync") { |*args| events << args } + begin + expect(Group.sync_table_data!.size).to eq 3 + ensure + ActiveSupport::Notifications.unsubscribe(subscriber) + end + expect(calls).to eq 2 + # The retry is an implementation detail; one call emits one event. + expect(events.size).to eq 1 end it "reraises the error if the retried sync also hits a uniqueness violation" do @@ -262,6 +271,16 @@ expect(Size.large_label).to eq "Large" end + it "defines attribute helpers for values that have no literal representation in ruby source" do + expect(Size.small_introduced_on).to eq Date.new(2020, 1, 15) + expect(Size.large_introduced_on).to eq Date.new(2021, 6, 30) + end + + it "returns frozen values from attribute helpers" do + expect(Size.small_label).to be_frozen + expect(Size.small_introduced_on).to be_frozen + end + it "syncs overridden attributes to the existing record instead of creating a new one" do Size.sync_table_data! expect(Size.count).to eq 3 @@ -351,6 +370,59 @@ expect(subclass.protected_instance?(light_gray)).to eq true expect(subclass.instance_keys).to include 8 end + + context "when a subclass has not been loaded yet" do + # A subclass that has not been loaded yet is not in `descendants`, so the base class has + # no way to know about its data files up front. Ruby cannot unload the subclass once the + # spec suite has referenced it, so the base class is instead limited to its own data files + # to reproduce what it can see in that situation. Reading a row resolves its class from + # the type column, and that is what makes the row identifiable as one the subclass owns. + # + # The override has to be defined on the singleton class rather than stubbed: the subclass + # inherits the singleton method and must still get the real implementation. + before do + Square.sync_table_data! + Shape.sync_table_data! + + shape_files = Shape.send(:support_table_data_files) + Shape.singleton_class.send(:define_method, :support_table_data_files_with_descendants) do + (self == Shape) ? shape_files : super() + end + end + + after do + Shape.singleton_class.send(:remove_method, :support_table_data_files_with_descendants) + end + + it "keeps rows managed by the subclass' data files when delete_missing is true" do + unmanaged = Shape.new + unmanaged.name = "Blob" + unmanaged.save! + + Shape.sync_table_data!(delete_missing: true) + + expect(Shape.where(name: "Square").count).to eq 1 + expect(Shape.where(name: "Circle").count).to eq 1 + expect(Shape.where(name: "Blob").count).to eq 0 + end + + it "deletes subclass rows that are not in any data file" do + orphan = Square.new + orphan.name = "Rhombus" + orphan.save! + + Shape.sync_table_data!(delete_missing: true) + + expect(Shape.where(name: "Rhombus").count).to eq 0 + expect(Shape.where(name: "Square").count).to eq 1 + end + + it "reports subclass managed rows as protected from the base class" do + square = Shape.find_by!(name: "Square") + + expect(Shape.protected_instance?(square)).to eq true + end + end end describe "instance_names" do @@ -424,7 +496,7 @@ describe "support_table_classes" do it "gets a list of all loaded support table classes with dependencies listed first" do - expect(SupportTableData.support_table_classes).to eq [Shade, Group, Hue, Color, Invalid, Polygon, Size] + expect(SupportTableData.support_table_classes).to eq [Shade, Group, Hue, Color, Invalid, Polygon, Shape, Size, Square] end end From 51810c24a0cfdd63d2ad8949b5bed80ceafd2ae3 Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Tue, 11 Aug 2026 09:04:27 -0700 Subject: [PATCH 10/12] code cleanup --- CHANGELOG.md | 8 +- README.md | 2 +- lib/support_table_data.rb | 218 ++++++++++-------- .../documentation/source_file.rb | 15 +- .../documentation/yard_doc.rb | 65 ++++-- .../documentation/yard_doc_spec.rb | 16 +- 6 files changed, 203 insertions(+), 121 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99aa215..a120bef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,12 +4,16 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## 1.6.2 +## 1.7.0 ### Fixed - `sync_table_data!` now emits a single `support_table_data.sync` notification per call. A sync that was retried after a uniqueness violation emitted two. - `sync_table_data!` with `delete_missing: true` now raises an `ArgumentError` instead of deleting every row in the table when the data files contain no rows (for example, when a data file was accidentally emptied or truncated). +- `sync_table_data!` now raises an `ArgumentError` when a data file row has no value for the key attribute (for example, when a key column was renamed or dropped). Previously such rows were collapsed into a single record with a blank key, and with `delete_missing: true` the sync deleted every real row in the table. +- Syncing a single table inheritance subclass no longer inserts duplicate or wrongly typed rows for records defined in the base class' data files. Existing rows are matched without the inheritance type condition, and new rows default to the type of the class whose data file defines them. +- Records are now merged in strict data file order, so later files take precedence even when named instance files are mixed with list format files. `named_instance_data` and the named instance helpers use the same merged records that are synced to the database. +- Entries under a name beginning with an underscore are treated as anonymous records even when the value is a single hash. Previously two files reusing the same underscore name each with a single hash were merged into one record, silently dropping rows. - Generated predicate methods (e.g. `record.active?`) now cast the data file value to the attribute type before comparing. Previously the raw file value was compared to the cast database attribute, so predicates silently returned `false` whenever the types differed (guaranteed for CSV data files, where all values are strings). - Single table inheritance subclasses no longer raise `NoMethodError` from `instance_names`, `instance_keys`, `protected_instance?`, and other class methods. Subclasses now share the support table state defined on their base class. - Named instance helper methods are now redefined when a later data file overrides an attribute or key value, so the helpers always return the merged values that are synced to the database. Previously they permanently returned the values from the first file that defined the named instance. @@ -29,7 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Data file names containing extra dots no longer break the class name detection used by `SupportTableData.sync_all!` to eager load models. - Error messages for invalid named instance definitions now include the model class name instead of repeating the instance name. - Single table inheritance subclasses now properly inherit named instance helpers regardless of load order. -- The `:compact` YARD format now emits one comment block per method instead of `@!macro` invocations. YARD only expands a macro when it is attached to a method definition, so the macro form resolved to methods with no description, no `@return`, and no `@raise` tags, and it documented the predicate methods as class methods rather than instance methods. +- The `:compact` YARD format now emits macro invocations that YARD can actually expand. Each generated method gets its own comment block with the `@!macro` invocation indented under its `@!method` line, and the macros no longer use positional parameters (YARD only fills those in from real method calls in the source). The previous form put all directives in one comment block and passed arguments on the invocation line, which YARD does not support; it resolved to methods with no description, no `@return`, and no `@raise` tags, and it documented the predicate methods as class methods rather than instance methods. Macro names are also namespaced per class so models do not overwrite each other's macros. - The documentation tasks no longer report success when a model raises an `ArgumentError` for an invalid named instance definition. The rescue that produced an empty list of source files covered the whole lookup rather than just the file path expansion it was meant to guard. ## 1.6.1 diff --git a/README.md b/README.md index c6b775d..837087a 100644 --- a/README.md +++ b/README.md @@ -205,7 +205,7 @@ A good practice is to add a check to your CI pipeline to ensure the documentatio Each model can choose how its YARD docs are generated by setting `support_table_yard_docs` to one of three values: - `:full` — verbose comment block per generated method (the default) -- `:compact` — the same `@!method` and `@!return` tags per generated method as `:full`, but without the prose descriptions. YARD resolves it to exactly the same methods and return types as `:full`. Useful when a model has many named instances and the verbose comment block is too long to be useful inline. +- `:compact` — shared `@!macro` definitions plus a short comment block per generated method that expands them. YARD resolves it to exactly the same methods and return types as `:full`. Useful when a model has many named instances and the verbose comment block is too long to be useful inline. - `:none` — generate no YARD docs for this model. The rake task will also strip any previously generated YARD docs from the source file. ```ruby diff --git a/lib/support_table_data.rb b/lib/support_table_data.rb index 6a2c035..e42e736 100644 --- a/lib/support_table_data.rb +++ b/lib/support_table_data.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "active_support/core_ext/module/redefine_method" +require "concurrent/map" # This concern can be mixed into models that represent static support tables. These are small tables # that have a limited number of rows, and have values that are often tied to the logic in the code. @@ -69,8 +70,8 @@ def support_table_key_attribute # Get the YARD documentation mode for this model. One of: # # * `:full` - emit a verbose comment block per generated method (default) - # * `:compact` - emit shared @!macro definitions plus a short - # @!method/@!macro pair per generated method + # * `:compact` - emit shared @!macro definitions plus a short comment + # block per generated method that expands them # * `:none` - generate no YARD docs for this model; the rake task will # strip any existing generated YARD docs # @@ -110,11 +111,25 @@ def sync_table_data!(delete_missing: false) hash[attributes[support_table_key_attribute].to_s] = attributes end + if canonical_data.include?("") + raise ArgumentError.new("Cannot sync #{name} because the data files contain a row with no value for the key attribute #{support_table_key_attribute}") + end + if delete_missing && canonical_data.empty? && exists? raise ArgumentError.new("Refusing to sync #{name} with delete_missing enabled because the data files contain no rows; this would delete every row in the table") end - records = where(support_table_key_attribute => canonical_data.keys) + # Rows are matched on the key attribute alone, so the lookup must not be scoped + # to this class's single table inheritance type. A subclass syncing rows from + # inherited data files has to find them no matter what type they currently have + # or it would insert duplicates. + scope = finder_needs_type_condition? ? base_class : self + records = scope.where(support_table_key_attribute => canonical_data.keys) + + # New rows default to the type of the class whose data files define them so that + # a subclass syncing inherited files does not create them with its own type. + record_classes = support_table_record_classes + changes = [] synced_ids = [] @@ -135,7 +150,11 @@ def sync_table_data!(delete_missing: false) canonical_data.each_value do |attributes| class_name = attributes[inheritance_column] - klass = class_name ? sti_class_for(class_name) : self + klass = if class_name + sti_class_for(class_name) + else + record_classes[attributes[support_table_key_attribute].to_s] || self + end record = klass.new attributes.each do |name, value| record.send(:"#{name}=", value) if record.respond_to?(:"#{name}=", true) @@ -171,7 +190,10 @@ def sync_table_data!(delete_missing: false) def add_support_table_data(data_file_path) root_dir = support_table_data_directory || SupportTableData.data_directory || Dir.pwd support_table_mutex.synchronize do - @support_table_data_files = support_table_data_files + [File.expand_path(data_file_path, root_dir)] + # Only the files added directly to this class are stored on it. Files added to a + # base class are composed in at read time so a single table inheritance subclass + # sees files the base class adds later, no matter the order the classes set up in. + @support_table_data_files = (@support_table_data_files || []) + [File.expand_path(data_file_path, root_dir)] end define_support_table_named_instances end @@ -220,73 +242,14 @@ def support_table_data # @return [Array] List of attributes for all records in the data files. # @api private def support_table_data_for_files(data_files) - records = [] - named_records = {} - - data_files.each do |data_file_path| - file_data = support_table_parse_data_file(data_file_path) - - if file_data.is_a?(Hash) - file_data.each do |instance_name, attributes| - unless attributes.is_a?(Hash) - # A name mapped to a list of records (i.e. a name beginning with an underscore) - # holds anonymous records that are only identified by the key attribute. - Array(attributes).flatten.each { |record| records << record.dup } - next - end - - # Records are merged by their name so that a later data file can override - # attributes on a named record without having to repeat the key attribute. - instance_name = instance_name.to_s - existing = named_records[instance_name] - if existing - existing.merge!(attributes) - else - record = attributes.dup - named_records[instance_name] = record - records << record - end - end - else - Array(file_data).flatten.each { |record| records << record.dup } - end - end - - # Records that resolve to the same key attribute value are merged together. - data = {} - records.each do |attributes| - key_value = attributes[support_table_key_attribute].to_s - existing = data[key_value] - if existing - existing.merge!(attributes) - else - data[key_value] = attributes - end - end - - data.values + support_table_merged_records(data_files).first end # Get the data for a named instances from the data files. # # @return [Hasn] Hash of named instance attributes. def named_instance_data(name) - data = {} - name = name.to_s - - support_table_data_files.each do |data_file_path| - file_data = support_table_parse_data_file(data_file_path) - next unless file_data.is_a?(Hash) - - file_data.each do |instance_name, attributes| - next unless name == instance_name.to_s - next unless attributes.is_a?(Hash) - - data.merge!(attributes) - end - end - - data + support_table_merged_records(support_table_data_files).last[name.to_s] || {} end # Get the names of all named instances. @@ -395,25 +358,91 @@ def support_table_protected_keys end end - def define_support_table_named_instances - merged_data = {} + # Parse the data files and merge the records they define. Files are processed in + # order so that later files take precedence no matter how each file is structured. + # Records are matched both by their instance name and by their key attribute value. + # Entry names that begin with an underscore do not define named instances; their + # values hold anonymous records identified only by the key attribute. + # + # @param data_files [Array] The paths of the data files to read. + # @return [Array(Array, Hash)] The ordered list of merged records + # and the named instance records by name. + def support_table_merged_records(data_files) + records = [] + named_records = {} + keyed_records = {} + + merge_record = lambda do |attributes, instance_name| + record = named_records[instance_name] if instance_name + if record.nil? && (instance_name.nil? || attributes.include?(support_table_key_attribute)) + record = keyed_records[attributes[support_table_key_attribute].to_s] + end + + if record + record.merge!(attributes) + else + record = attributes.dup + records << record + end - support_table_data_files.each do |file_path| - data = support_table_parse_data_file(file_path) - next unless data.is_a?(Hash) + named_records[instance_name] = record if instance_name + keyed_records[record[support_table_key_attribute].to_s] = record + end + + data_files.each do |data_file_path| + file_data = support_table_parse_data_file(data_file_path) - data.each do |name, attributes| - name = name.to_s - existing = merged_data[name] - merged_data[name] = if existing.is_a?(Hash) && attributes.is_a?(Hash) - existing.merge(attributes) + unless file_data.is_a?(Hash) + Array(file_data).flatten.each { |attributes| merge_record.call(attributes, nil) } + next + end + + file_data.each do |instance_name, attributes| + instance_name = instance_name.to_s + if instance_name.start_with?("_") + anonymous_records = attributes.is_a?(Hash) ? [attributes] : Array(attributes).flatten + anonymous_records.each { |record_attributes| merge_record.call(record_attributes, nil) } + elsif attributes.is_a?(Hash) + merge_record.call(attributes, instance_name) else - attributes + raise ArgumentError.new("Cannot define named instance #{instance_name} on #{name}; value must be a Hash") end end end - merged_data.each do |name, attributes| + [records, named_records] + end + + # The classes in the hierarchy that added their own data files, listed base class + # first, along with the file paths each one added. + # + # @return [Array)>] + def support_table_data_file_owners + owners = superclass.include?(SupportTableData) ? superclass.send(:support_table_data_file_owners) : [] + own_files = @support_table_data_files + owners += [[self, own_files]] if own_files && !own_files.empty? + owners + end + + # Map each record's key attribute value to the class in the hierarchy whose own data + # files define it. Used to determine the single table inheritance type for new rows + # that do not specify one in the data files. + # + # @return [Hash] + def support_table_record_classes + record_classes = {} + support_table_data_file_owners.each do |owner, files| + next if owner == self + + support_table_data_for_files(files).each do |attributes| + record_classes[attributes[support_table_key_attribute].to_s] ||= owner + end + end + record_classes + end + + def define_support_table_named_instances + support_table_merged_records(support_table_data_files).last.each do |name, attributes| support_table_mutex.synchronize do define_support_table_named_instance_methods(name, attributes) end @@ -439,12 +468,12 @@ def define_support_table_named_instance_methods(name, attributes) if instance_names_map[method_name] != key_value define_support_table_instance_helper(method_name, support_table_key_attribute, key_value, redefine: true) define_support_table_predicates_helper("#{method_name}?", support_table_key_attribute, key_value, redefine: true) - @support_table_instance_names = instance_names_map.merge(method_name => key_value) + @support_table_instance_names = (@support_table_instance_names || {}).merge(method_name => key_value) end else define_support_table_instance_helper(method_name, support_table_key_attribute, key_value) define_support_table_predicates_helper("#{method_name}?", support_table_key_attribute, key_value) - @support_table_instance_names = instance_names_map.merge(method_name => key_value) + @support_table_instance_names = (@support_table_instance_names || {}).merge(method_name => key_value) end support_table_attribute_helpers_map.each do |attribute_name, defined_methods| @@ -500,12 +529,13 @@ def define_support_table_predicates_helper(method_name, attribute_name, attribut # The value has to be cast to the attribute type before it can be compared to the value # read off the record. The cast is done lazily and memoized per class because the type # requires a database connection to resolve, which is not necessarily available when the - # model class is being loaded. - cast_values = {} + # model class is being loaded. The map is shared by every instance, so it needs to be + # thread safe on Ruby implementations without a global interpreter lock. + cast_values = Concurrent::Map.new define_method(method_name) do klass = self.class - cast_value = cast_values.fetch(klass) do - cast_values[klass] = klass.type_for_attribute(attribute_name).cast(attribute_value) + cast_value = cast_values.fetch_or_store(klass) do + klass.type_for_attribute(attribute_name).cast(attribute_value) end send(attribute_name) == cast_value end @@ -598,31 +628,31 @@ def support_table_data_files_with_descendants descendants.each do |subclass| next unless subclass.include?(SupportTableData) - subclass_files = subclass.send(:support_table_data_files) - # Subclasses without their own data files inherit the exact same array from this class. - next if subclass_files.equal?(files) - - files += (subclass_files - files) + files |= subclass.send(:support_table_data_files) end files end # The class level state used by the concern is stored in instance variables on the - # class where the concern was included. These readers fall back to the superclass - # so that single table inheritance subclasses share the state defined on their - # base class rather than crashing on uninitialized instance variables. + # class where the concern was included. These readers compose in or fall back to the + # superclass state so that single table inheritance subclasses share the state defined + # on their base class rather than crashing on uninitialized instance variables. def support_table_mutex @support_table_mutex || (superclass.include?(SupportTableData) ? superclass.send(:support_table_mutex) : nil) end def support_table_data_files - @support_table_data_files || (superclass.include?(SupportTableData) ? superclass.send(:support_table_data_files) : []) + inherited = superclass.include?(SupportTableData) ? superclass.send(:support_table_data_files) : [] + own = @support_table_data_files || [] + inherited.empty? ? own : inherited + own end def support_table_instance_names_map - @support_table_instance_names || (superclass.include?(SupportTableData) ? superclass.send(:support_table_instance_names_map) : {}) + inherited = superclass.include?(SupportTableData) ? superclass.send(:support_table_instance_names_map) : {} + own = @support_table_instance_names || {} + inherited.empty? ? own : inherited.merge(own) end def support_table_attribute_helpers_map diff --git a/lib/support_table_data/documentation/source_file.rb b/lib/support_table_data/documentation/source_file.rb index 8b0c7bb..a3d8c90 100644 --- a/lib/support_table_data/documentation/source_file.rb +++ b/lib/support_table_data/documentation/source_file.rb @@ -64,9 +64,18 @@ def source_with_yard_docs updated_source << "\n#{indent}# rubocop:enable all" updated_source << "\n#{indent}#{END_YARD_COMMENT}" # Strip out any duplicate generated blocks (i.e. left over from a bad merge) - # so that the file is left with exactly one block. - updated_source << source[existing_yard_docs.end(0)..].gsub(YARD_COMMENT_REGEX, "") - "#{updated_source.rstrip}#{trailing_newline}" + # so that the file is left with exactly one block. Otherwise the tail of the + # file is preserved verbatim so that a file whose docs are already up to date + # compares byte for byte with its own source. + tail = source[existing_yard_docs.end(0)..] + deduped_tail = tail.gsub(YARD_COMMENT_REGEX, "") + if deduped_tail == tail + updated_source << tail + updated_source + else + updated_source << deduped_tail + "#{updated_source.rstrip}#{trailing_newline}" + end else yard_comments = <<~SOURCE.chomp("\n") #{BEGIN_YARD_COMMENT} diff --git a/lib/support_table_data/documentation/yard_doc.rb b/lib/support_table_data/documentation/yard_doc.rb index ee0a06b..c79c6b7 100644 --- a/lib/support_table_data/documentation/yard_doc.rb +++ b/lib/support_table_data/documentation/yard_doc.rb @@ -12,7 +12,8 @@ def initialize(klass) # is controlled by the model's `support_table_yard_docs` setting: # # * `:full` - verbose comment block per method (default) - # * `:compact` - the same tags per method without the prose descriptions + # * `:compact` - shared @!macro definitions plus a short comment block + # per method that expands them # * `:none` - generate no docs at all # # @return [String, nil] The YARD documentation, or nil if no docs should @@ -102,13 +103,17 @@ def generate_verbose_yard_docs(instance_names) yard_lines.join("\n") end - # The compact format drops the prose description from each method and keeps only the - # tags. Every method still needs its own comment block separated by a blank line: - # YARD builds one docstring per block, so combining methods into a single block would - # both drop the tags for all but the last method and leak the `self.` scope of a - # singleton method onto the instance methods that follow it. + # The compact format defines the shared documentation once as macros and expands + # them under each generated method. YARD imposes three rules on this layout: each + # method needs its own comment block (tags and the `self.` scope apply to the whole + # block), a macro invocation must be indented under its @!method line to attach to + # that method, and macro parameters ($1, etc.) cannot be used because YARD only + # fills them in from real method calls in the source. Macro names are global to the + # YARD registry, so they are namespaced with the class name. def generate_compact_yard_docs(instance_names) yard_lines = ["# @!group Named Instances"] + yard_lines << "" + yard_lines << compact_macro_definitions instance_names.sort.each do |name| yard_lines << "" @@ -127,31 +132,65 @@ def generate_compact_yard_docs(instance_names) yard_lines.join("\n") end + def compact_macro_definitions + finder_macro = <<~YARD.chomp("\n") + # @!macro [new] #{compact_macro_name("finder")} + # Find this named instance from the database. + # @return [#{klass.name}] + # @raise [ActiveRecord::RecordNotFound] if the record does not exist + # @!visibility public + YARD + + predicate_macro = <<~YARD.chomp("\n") + # @!macro [new] #{compact_macro_name("predicate")} + # Check if this record is this named instance. + # @return [Boolean] + # @!visibility public + YARD + + macros = [finder_macro, "", predicate_macro] + + if klass.support_table_attribute_helpers.any? + attribute_macro = <<~YARD.chomp("\n") + # @!macro [new] #{compact_macro_name("attribute")} + # Get this attribute from the data file for this named instance. + # @!visibility public + YARD + macros << "" + macros << attribute_macro + end + + macros.join("\n") + end + def compact_instance_helper_yard_doc(name) <<~YARD.chomp("\n") # @!method self.#{name} - # @return [#{klass.name}] - # @raise [ActiveRecord::RecordNotFound] if the record does not exist - # @!visibility public + # @!macro #{compact_macro_name("finder")} YARD end def compact_predicate_helper_yard_doc(name) <<~YARD.chomp("\n") # @!method #{name}? - # @return [Boolean] - # @!visibility public + # @!macro #{compact_macro_name("predicate")} YARD end + # The attribute macro cannot carry the @return tag because the return type differs + # per method, so each attribute method adds its own. def compact_attribute_helper_yard_doc(name, attribute_name) <<~YARD.chomp("\n") # @!method self.#{name}_#{attribute_name} - # @return [#{attribute_yard_return_type(name, attribute_name)}] - # @!visibility public + # @!macro #{compact_macro_name("attribute")} + # @return [#{attribute_yard_return_type(name, attribute_name)}] YARD end + def compact_macro_name(suffix) + "support_table_#{klass.name.underscore.tr("/", "_")}_#{suffix}" + end + def attribute_yard_return_type(name, attribute_name) TypeInference.yard_type(TypeInference.value_type(klass, name, attribute_name)) end diff --git a/spec/support_table_data/documentation/yard_doc_spec.rb b/spec/support_table_data/documentation/yard_doc_spec.rb index 5a1a986..2dbd708 100644 --- a/spec/support_table_data/documentation/yard_doc_spec.rb +++ b/spec/support_table_data/documentation/yard_doc_spec.rb @@ -151,7 +151,7 @@ def yard_methods_for(klass) end end - it "emits the tags for each method without the prose descriptions" do + it "emits shared macro definitions and a short block per method that expands them" do doc = SupportTableData::Documentation::YardDoc.new(Color) result = doc.named_instance_yard_docs @@ -159,12 +159,12 @@ def yard_methods_for(klass) expect(result).to include("# @!group Named Instances") expect(result).to include("# @!endgroup") - expect(result).to include("# @!method self.red") - expect(result).to include("# @!method red?") - expect(result).to include("# @return [Color]") - expect(result).to include("# @return [Boolean]") + expect(result).to include("# @!macro [new] support_table_color_finder\n# Find this named instance from the database.\n# @return [Color]") + expect(result).to include("# @!macro [new] support_table_color_predicate\n# Check if this record is this named instance.\n# @return [Boolean]") + + expect(result).to include("# @!method self.red\n# @!macro support_table_color_finder") + expect(result).to include("# @!method red?\n# @!macro support_table_color_predicate") expect(result).not_to include("# Find the named instance +red+ from the database.") - expect(result).not_to include("@!macro") end it "generates docs that YARD resolves to the same methods and tags as the full format" do @@ -205,8 +205,8 @@ def yard_methods_for(klass) result = doc.named_instance_yard_docs # Group has attribute helpers for group_id (integer) and name (string). - expect(result).to include("# @!method self.primary_group_id\n# @return [Integer]") - expect(result).to include("# @!method self.primary_name\n# @return [String]") + expect(result).to include("# @!method self.primary_group_id\n# @!macro support_table_group_attribute\n# @return [Integer]") + expect(result).to include("# @!method self.primary_name\n# @!macro support_table_group_attribute\n# @return [String]") end it "generates attribute helper docs that YARD resolves with the right return types" do From b724359dc02a2abfbc2db5cdb827d7c3fc464d61 Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Tue, 11 Aug 2026 09:04:55 -0700 Subject: [PATCH 11/12] bump version --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index fdd3be6..bd8bf88 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.6.2 +1.7.0 From 48031580df33b48e29dbef946be988d0cd39bb89 Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Tue, 11 Aug 2026 09:11:51 -0700 Subject: [PATCH 12/12] cleanup changelog --- CHANGELOG.md | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a120bef..c81d8c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,33 +8,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- `sync_table_data!` now emits a single `support_table_data.sync` notification per call. A sync that was retried after a uniqueness violation emitted two. -- `sync_table_data!` with `delete_missing: true` now raises an `ArgumentError` instead of deleting every row in the table when the data files contain no rows (for example, when a data file was accidentally emptied or truncated). -- `sync_table_data!` now raises an `ArgumentError` when a data file row has no value for the key attribute (for example, when a key column was renamed or dropped). Previously such rows were collapsed into a single record with a blank key, and with `delete_missing: true` the sync deleted every real row in the table. +- `sync_table_data!` now retries once on `ActiveRecord::RecordNotUnique` errors caused by a concurrent sync in another process inserting the same rows. +- `sync_table_data!` with `delete_missing: true` now raises an `ArgumentError` instead of deleting every row in the table when the data files contain no rows. +- `sync_table_data!` now raises an `ArgumentError` when a data file row has no value for the key attribute. Previously such rows were collapsed into a single record with a blank key. +- `sync_table_data!` now returns an empty array instead of `nil` when the table does not exist. +- Fixed broken cycle detection in the autosave association check during syncs that could cause infinite recursion on cyclic autosave associations. - Syncing a single table inheritance subclass no longer inserts duplicate or wrongly typed rows for records defined in the base class' data files. Existing rows are matched without the inheritance type condition, and new rows default to the type of the class whose data file defines them. -- Records are now merged in strict data file order, so later files take precedence even when named instance files are mixed with list format files. `named_instance_data` and the named instance helpers use the same merged records that are synced to the database. +- Single table inheritance subclasses now share the support table state defined on their base class regardless of load order. Previously class methods like `instance_names` and `protected_instance?` raised `NoMethodError` and named instance helpers could be missing on subclasses. +- `protected_instance?` and `instance_keys` now include data files added to single table inheritance subclasses and no longer return stale results when data files are added after their values were first computed. +- `sync_table_data!` with `delete_missing: true` no longer deletes rows that are managed by data files added to single table inheritance subclasses. +- Records are now merged in strict data file order, so later files take precedence even when named instance files are mixed with list format files. +- Data files that override attributes on a named instance are now merged by the instance name rather than only by the key attribute. Previously an override that did not repeat the key attribute value was treated as a new record and inserted as an extra row on every sync. +- Named instance helper methods and `named_instance_data` now return the merged values that are synced to the database. Previously the helpers permanently returned the values from the first file that defined the named instance. - Entries under a name beginning with an underscore are treated as anonymous records even when the value is a single hash. Previously two files reusing the same underscore name each with a single hash were merged into one record, silently dropping rows. -- Generated predicate methods (e.g. `record.active?`) now cast the data file value to the attribute type before comparing. Previously the raw file value was compared to the cast database attribute, so predicates silently returned `false` whenever the types differed (guaranteed for CSV data files, where all values are strings). -- Single table inheritance subclasses no longer raise `NoMethodError` from `instance_names`, `instance_keys`, `protected_instance?`, and other class methods. Subclasses now share the support table state defined on their base class. -- Named instance helper methods are now redefined when a later data file overrides an attribute or key value, so the helpers always return the merged values that are synced to the database. Previously they permanently returned the values from the first file that defined the named instance. -- `named_instance_attribute_helpers` can now be called again with an attribute that was already registered without raising an `ArgumentError`. -- Data files that override attributes on a named instance are now merged by the instance name rather than only by the key attribute. Previously, an override that did not repeat the key attribute value (for example, a file containing only `large:` with a `label`) was treated as a brand new record; the override was never applied to the real row and a row with only the overridden attributes was inserted on every sync. - YAML data files can now use anchors/aliases and date/time values. Previously these raised `Psych::AliasesNotEnabled` or `Psych::DisallowedClass` errors. -- `protected_instance?` and `instance_keys` no longer return stale results when data files are added after their values were first computed. This includes single table inheritance subclasses that computed their values before a data file was added to their base class. -- `protected_instance?` and `instance_keys` now include data files added to single table inheritance subclasses when called on a base class. Previously rows managed by a subclass' data file were reported as unprotected by the base class even though they live in the same table. -- `sync_table_data!` with `delete_missing: true` no longer deletes rows that are managed by data files added to single table inheritance subclasses. Previously syncing the base class deleted rows that the subclass was responsible for syncing. -- Fixed broken cycle detection in the autosave association check during syncs that could cause infinite recursion on cyclic autosave associations. -- `sync_table_data!` now retries once on `ActiveRecord::RecordNotUnique` errors caused by concurrent syncs inserting the same rows from another process. -- `sync_table_data!` now returns an empty array instead of `nil` when the table does not exist. -- `named_instance` now raises a clear `ActiveRecord::RecordNotFound` error for undefined named instances instead of querying the database for a `nil` key (which could silently return a row with a `NULL` key value). -- Memoized class-level state is now consistently synchronized with the class mutex to avoid races on non-MRI Ruby implementations. +- Generated predicate methods (e.g. `record.active?`) now cast the data file value to the attribute type before comparing, so they no longer silently return `false` when the types differ (always the case for CSV data files, where all values are strings). +- `named_instance` now raises an `ActiveRecord::RecordNotFound` error for undefined named instances instead of querying the database for a `nil` key. +- `named_instance_attribute_helpers` can now be called again with an attribute that was already registered without raising an `ArgumentError`. +- Modifications to memoized class-level state are now synchronized to avoid races on Ruby implementations without a global interpreter lock. - Setting `config.support_table.auto_sync = false` before the gem is loaded is no longer overwritten back to `true` by the Railtie. -- The documentation tasks no longer corrupt model source files that contain duplicated generated YARD doc blocks (e.g. from a bad merge); the regex that finds the generated block is no longer greedy and all duplicated blocks are now removed so the file is left with exactly one block. - Data file names containing extra dots no longer break the class name detection used by `SupportTableData.sync_all!` to eager load models. - Error messages for invalid named instance definitions now include the model class name instead of repeating the instance name. -- Single table inheritance subclasses now properly inherit named instance helpers regardless of load order. -- The `:compact` YARD format now emits macro invocations that YARD can actually expand. Each generated method gets its own comment block with the `@!macro` invocation indented under its `@!method` line, and the macros no longer use positional parameters (YARD only fills those in from real method calls in the source). The previous form put all directives in one comment block and passed arguments on the invocation line, which YARD does not support; it resolved to methods with no description, no `@return`, and no `@raise` tags, and it documented the predicate methods as class methods rather than instance methods. Macro names are also namespaced per class so models do not overwrite each other's macros. -- The documentation tasks no longer report success when a model raises an `ArgumentError` for an invalid named instance definition. The rescue that produced an empty list of source files covered the whole lookup rather than just the file path expansion it was meant to guard. +- The `:compact` YARD format now emits macros that YARD can actually expand. Previously the generated docs resolved to methods with no description, `@return`, or `@raise` tags and documented the predicate methods as class methods. Macro names are also namespaced per class so models do not overwrite each other's macros. +- The documentation tasks now remove duplicated generated YARD doc blocks (e.g. left over from a bad merge) instead of corrupting the model source file. +- The documentation tasks no longer report success when a model raises an `ArgumentError` for an invalid named instance definition. ## 1.6.1