diff --git a/CHANGELOG.md b/CHANGELOG.md index f25dd1a..bb57cf0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,10 @@ ### Fixed +- Resolve loaded Active Record models by their configured table names, preserving + model-backed attribute serialization, and raise deterministically when unrelated + models ambiguously own one table + ([#109](https://github.com/rdy/fixture_builder/issues/109)). - Omit database-generated columns from generated fixtures so Rails can load snapshots from tables that have them ([#100](https://github.com/rdy/fixture_builder/issues/100)). diff --git a/lib/fixture_builder.rb b/lib/fixture_builder.rb index 474eb66..74c6d11 100644 --- a/lib/fixture_builder.rb +++ b/lib/fixture_builder.rb @@ -3,6 +3,7 @@ require "fixture_builder/delegations" require "fixture_builder/configuration" require "fixture_builder/namer" +require "fixture_builder/ambiguous_model_error" require "fixture_builder/builder" require "fixture_builder/fixtures_path" diff --git a/lib/fixture_builder/ambiguous_model_error.rb b/lib/fixture_builder/ambiguous_model_error.rb new file mode 100644 index 0000000..0dd55dc --- /dev/null +++ b/lib/fixture_builder/ambiguous_model_error.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module FixtureBuilder + class AmbiguousModelError < StandardError + attr_reader :table_name, :models + + def initialize(table_name, models) + @table_name = table_name + @models = models.sort_by(&:name) + super("Multiple models match table #{table_name}: #{@models.map(&:name).join(", ")}") + end + end +end diff --git a/lib/fixture_builder/builder.rb b/lib/fixture_builder/builder.rb index 63ea951..5efb748 100644 --- a/lib/fixture_builder/builder.rb +++ b/lib/fixture_builder/builder.rb @@ -16,6 +16,7 @@ def generate! clean_out_old_data create_fixture_objects names_from_ivars! + @models_by_table = resolve_models_by_table write_data_to_files after_build&.call end @@ -64,7 +65,6 @@ def write_data_to_files def clean_out_old_data delete_tables - delete_yml_files end def delete_tables @@ -86,6 +86,14 @@ def say(*messages) end # standard:enable Rails/Output + def write_fixture_file(fixture_data, table_name) + File.write(fixture_file(table_name), fixture_data.to_yaml) + end + + def fixture_file(table_name) + fixtures_dir("#{table_name}.yml") + end + def dump_empty_fixtures_for_all_tables tables.each do |table_name| write_fixture_file({}, table_name) @@ -94,15 +102,11 @@ def dump_empty_fixtures_for_all_tables def dump_tables fixtures = tables.inject([]) do |files, table_name| - table_klass = begin - table_name.classify.constantize - rescue - nil - end - rows = if table_klass && table_klass < ActiveRecord::Base - generated_names = generated_column_names(table_klass.table_name) + table_klass = @models_by_table.fetch(table_name) + generated_names = generated_column_names(table_name) + rows = if table_klass table_klass.unscoped do - table_klass.order(:id).all.collect do |obj| + table_klass.order(Array(table_klass.primary_key)).all.collect do |obj| attrs = obj.attributes_before_type_cast.slice(*table_klass.column_names) attrs.each do |attr_name, value| column_type = table_klass.columns_hash.fetch(attr_name).type @@ -114,7 +118,6 @@ def dump_tables end end else - generated_names = generated_column_names(table_name) ActiveRecord::Base.connection.select_all(format(select_sql, table: ActiveRecord::Base.connection.quote_table_name(table_name))) .map { |row| row.except(*generated_names) } @@ -132,24 +135,51 @@ def dump_tables say "Built #{fixtures.to_sentence}" end + private + + def resolve_models_by_table + tables.each_with_object({}) do |table_name, models_by_table| + models_by_table[table_name] = resolve_model(table_name) + end + end + + def resolve_model(table_name) + table_name.classify.safe_constantize + candidates = ActiveRecord::Base.descendants.select do |model| + eligible_model?(model, table_name) + end + root_models = candidates.reject do |model| + candidates.any? { |candidate| candidate != model && model < candidate } + end + + return if root_models.empty? + return root_models.first if root_models.one? + + raise AmbiguousModelError.new(table_name, root_models) + end + + def eligible_model?(model, table_name) + return false if model.abstract_class? + + model_name = model.name + return false unless model_name && model_name.safe_constantize.equal?(model) + return false unless model.table_name == table_name + return false unless model.connection_pool.equal?(ActiveRecord::Base.connection_pool) + + primary_keys = Array(model.primary_key).compact + primary_keys.any? && primary_keys.all? { |key| model.columns_hash.key?(key) } + end + # A database-generated (virtual/stored generated) column cannot be # inserted, so Rails rejects a fixture file containing it. Only those # column names are removed from the extracted rows; everything else a row # carries - including an expression a custom `select_sql` selects - is # left as it was produced. - private def generated_column_names(table_name) + def generated_column_names(table_name) connection = ActiveRecord::Base.connection return [] unless connection.supports_virtual_columns? connection.columns(table_name).select(&:virtual?).map(&:name) end - - def write_fixture_file(fixture_data, table_name) - File.write(fixture_file(table_name), fixture_data.to_yaml) - end - - def fixture_file(table_name) - fixtures_dir("#{table_name}.yml") - end end end diff --git a/test/fixture_builder_test.rb b/test/fixture_builder_test.rb index 957f02e..575ba8b 100644 --- a/test/fixture_builder_test.rb +++ b/test/fixture_builder_test.rb @@ -188,21 +188,32 @@ def test_generated_columns_come_from_the_model_table_name create_and_blow_away_old_db force_fixture_generation - table_name = RELOCATED_CREATURES_TABLE + table_names = [CREATURE_ARCHIVE_TABLE, RELOCATED_CREATURES_TABLE] + wizard_data = WizardData.new(level: 99, title: "Lady of the Lake", allies: ["Arthur"]) FixtureBuilder.configure do |fbuilder| fbuilder.files_to_check = [] - fbuilder.skip_tables = ActiveRecord::Base.connection.tables - [table_name] - fbuilder.factory { RelocatedCreature.create!(name: "Nimue") } + fbuilder.skip_tables = ActiveRecord::Base.connection.tables - table_names + fbuilder.factory do + RelocatedCreature.create!(name: "Nimue", wizard_data: wizard_data) + ActiveRecord::Base.connection.execute( + "INSERT INTO #{RELOCATED_CREATURES_TABLE} (unrelated) VALUES ('Morgana')" + ) + end end - generated_fixture = YAML.safe_load_file(test_path("fixtures/#{table_name}.yml")) - # `name` is a plain column on the model's own table, so it must survive even - # though the iterated table of the same inferred name generates it. - assert_include generated_fixture, "nimue" - record = generated_fixture.fetch("nimue") - assert_include record, "name" - assert_equal "Nimue", record["name"] - assert_not_include record, "unrelated" + archive_fixture = YAML.safe_load_file(test_path("fixtures/#{CREATURE_ARCHIVE_TABLE}.yml")) + assert_equal( + {"level" => 99, "title" => "Lady of the Lake", "allies" => ["Arthur"]}, + archive_fixture.dig("nimue", "wizard_data") + ) + + # `RelocatedCreature` maps to `creature_archive`, not the conventionally + # inferred `relocated_creatures` table. The latter remains on the raw SQL + # path, where its database-generated `name` is excluded. + relocated_fixture = YAML.safe_load_file(test_path("fixtures/#{RELOCATED_CREATURES_TABLE}.yml")) + record = relocated_fixture.fetch("relocated_creatures_001") + assert_equal "Morgana", record["unrelated"] + assert_not_include record, "name" end def test_custom_json_attribute_type_round_trips_through_fixtures @@ -253,6 +264,15 @@ def test_deprecator_has_fixture_builder_metadata assert_equal "FixtureBuilder", FixtureBuilder.deprecator.gem_name end + def test_ambiguous_model_error_exposes_its_table_name_and_models + models = [MagicalCreature, GeneratedCreature] + error = FixtureBuilder::AmbiguousModelError.new("creatures", models) + + assert_equal "creatures", error.table_name + assert_equal [GeneratedCreature, MagicalCreature], error.models + assert_equal "Multiple models match table creatures: GeneratedCreature, MagicalCreature", error.message + end + def test_sql_setters_reject_positional_table_format_without_warning {select_sql: "SELECT * FROM %s", delete_sql: "DELETE FROM %s"}.each do |attribute, sql| configuration = FixtureBuilder::Configuration.new diff --git a/test/resolve_configured_table_name_test.rb b/test/resolve_configured_table_name_test.rb new file mode 100644 index 0000000..167ec81 --- /dev/null +++ b/test/resolve_configured_table_name_test.rb @@ -0,0 +1,393 @@ +# frozen_string_literal: true + +require_relative "test_helper" + +# Regression tests for model resolution by configured table name (#109). +class ResolveConfiguredTableNameErrorTest < Test::Unit::TestCase + def test_ambiguous_model_error_exposes_its_table_name_and_models + models = [MagicalCreature, GeneratedCreature] + error = FixtureBuilder::AmbiguousModelError.new("creatures", models) + + assert_equal "creatures", error.table_name + assert_equal [GeneratedCreature, MagicalCreature], error.models + assert_equal( + "Multiple models match table creatures: GeneratedCreature, MagicalCreature", + error.message + ) + end +end + +# standard:disable Rails/ApplicationRecord +class ResolveConfiguredTableNameSerializationTest < Test::Unit::TestCase + with_model :FixtureBuilderScopedRelocatedCreature do + table do |table| + table.string :name, null: false + table.json :wizard_data + end + + model do + attribute :wizard_data, WizardDataType.new + end + end + + with_model :FixtureBuilderScopedRawCreature do + table(id: false) do |table| + table.string :unrelated + table.virtual :name, type: :string, as: "upper(unrelated)", stored: true + end + end + + def test_configured_table_name_uses_model_backed_custom_serialization + archive_table = FixtureBuilderScopedRelocatedCreature.table_name + raw_table = FixtureBuilderScopedRawCreature.table_name + assert_equal FixtureBuilderScopedRelocatedCreature, resolve_model(archive_table) + assert_nil resolve_model(raw_table) + + force_fixture_generation + build_fixtures_for(archive_table, raw_table) do + FixtureBuilderScopedRelocatedCreature.create!( + name: "Nimue", + wizard_data: WizardData.new(level: 99, title: "Lady of the Lake", allies: ["Arthur"]) + ) + ActiveRecord::Base.connection.execute( + "INSERT INTO #{raw_table} (unrelated) VALUES ('Morgana')" + ) + end + + archive_fixture = YAML.safe_load_file(test_path("fixtures/#{archive_table}.yml")) + assert_equal( + {"level" => 99, "title" => "Lady of the Lake", "allies" => ["Arthur"]}, + archive_fixture.dig("nimue", "wizard_data") + ) + + relocated_fixture = YAML.safe_load_file(test_path("fixtures/#{raw_table}.yml")) + record = relocated_fixture.fetch("#{raw_table}_001") + assert_equal "Morgana", record["unrelated"] + assert_not_include record, "name" + end + + def teardown + FileUtils.rm_f(test_path("fixtures/#{FixtureBuilderScopedRelocatedCreature.table_name}.yml")) + FileUtils.rm_f(test_path("fixtures/#{FixtureBuilderScopedRawCreature.table_name}.yml")) + end + + private + + def build_fixtures_for(*table_names, &factory) + FixtureBuilder.configure do |fbuilder| + fbuilder.files_to_check = [] + fbuilder.skip_tables = ActiveRecord::Base.connection.tables - table_names + fbuilder.factory(&factory) + end + end + + def resolve_model(table_name) + FixtureBuilder::Builder.allocate.send(:resolve_model, table_name) + end +end + +module ResolveConfiguredTableNameAmbiguityBehavior + def test_unrelated_models_raise_before_replacing_the_fixture + table_name = FixtureBuilderAmbiguousAlpha.table_name + FixtureBuilderAmbiguousZulu.table_name = table_name + FixtureBuilderAmbiguousZulu.reset_column_information + fixture_path = test_path("fixtures/#{table_name}.yml") + original_fixture = "existing fixture bytes\n" + File.binwrite(fixture_path, original_fixture) + force_fixture_generation + + error = assert_raise(FixtureBuilder::AmbiguousModelError) do + build_fixtures_for(table_name) do + ActiveRecord::Base.connection.execute("INSERT INTO #{table_name} (name) VALUES ('Merlin')") + end + end + + assert_equal table_name, error.table_name + assert_equal %w[FixtureBuilderAmbiguousAlpha FixtureBuilderAmbiguousZulu], error.models.map(&:name) + assert_equal( + "Multiple models match table #{table_name}: FixtureBuilderAmbiguousAlpha, FixtureBuilderAmbiguousZulu", + error.message + ) + assert_equal original_fixture, File.binread(fixture_path) + end + + def teardown + FileUtils.rm_f(test_path("fixtures/#{FixtureBuilderAmbiguousAlpha.table_name}.yml")) + end + + private + + def build_fixtures_for(*table_names, &factory) + FixtureBuilder.configure do |fbuilder| + fbuilder.files_to_check = [] + fbuilder.skip_tables = ActiveRecord::Base.connection.tables - table_names + fbuilder.factory(&factory) + end + end +end + +class ResolveConfiguredTableNameAmbiguityAlphaFirstTest < Test::Unit::TestCase + include ResolveConfiguredTableNameAmbiguityBehavior + + with_model :FixtureBuilderAmbiguousAlpha do + table { |table| table.string :name } + end + + with_model :FixtureBuilderAmbiguousZulu do + table { |table| table.string :name } + end +end + +class ResolveConfiguredTableNameAmbiguityZuluFirstTest < Test::Unit::TestCase + include ResolveConfiguredTableNameAmbiguityBehavior + + with_model :FixtureBuilderAmbiguousZulu do + table { |table| table.string :name } + end + + with_model :FixtureBuilderAmbiguousAlpha do + table { |table| table.string :name } + end +end + +class ResolveConfiguredTableNameStiTest < Test::Unit::TestCase + with_model :FixtureBuilderStiBase do + table do |table| + table.string :name + table.string :type + end + end + + with_model :FixtureBuilderStiSubclass, superclass: :FixtureBuilderStiBase do + table(false) + end + + def test_sti_models_dump_all_subtype_rows_through_the_base_model + table_name = FixtureBuilderStiBase.table_name + assert_equal FixtureBuilderStiBase, resolve_model(table_name) + force_fixture_generation + + build_fixtures_for(table_name) do + FixtureBuilderStiBase.create!(name: "Base creature") + FixtureBuilderStiSubclass.create!(name: "Subclass creature") + end + + fixture = YAML.safe_load_file(test_path("fixtures/#{table_name}.yml")) + assert_equal %w[Base\ creature Subclass\ creature], fixture.values.pluck("name") + assert_nil fixture.values.first["type"] + assert_equal FixtureBuilderStiSubclass.name, fixture.values.last["type"] + end + + def teardown + FileUtils.rm_f(test_path("fixtures/#{FixtureBuilderStiBase.table_name}.yml")) + end + + private + + def build_fixtures_for(*table_names, &factory) + FixtureBuilder.configure do |fbuilder| + fbuilder.files_to_check = [] + fbuilder.skip_tables = ActiveRecord::Base.connection.tables - table_names + fbuilder.factory(&factory) + end + end + + def resolve_model(table_name) + FixtureBuilder::Builder.allocate.send(:resolve_model, table_name) + end +end + +class ResolveConfiguredTableNameManualBoundaryTest < Test::Unit::TestCase + include TestDatabase + + def test_conventionally_named_autoloaded_model_uses_model_backed_serialization + create_and_blow_away_old_db + table_name = "fixture_builder_autoloaded_models" + + with_temporary_table(table_name, columns: {wizard_data: :json}) do + with_autoloaded_model("FixtureBuilderAutoloadedModel") do + force_fixture_generation + build_fixtures_for(table_name) do + value = ActiveRecord::Base.connection.quote({"level" => 99}.to_json) + ActiveRecord::Base.connection.execute( + "INSERT INTO #{table_name} (wizard_data) VALUES (#{value})" + ) + end + + assert_equal :json, + Object.const_get(:FixtureBuilderAutoloadedModel).columns_hash.fetch("wizard_data").type + + fixture = YAML.safe_load_file(test_path("fixtures/#{table_name}.yml")) + assert_equal({"level" => 99}, fixture.values.first["wizard_data"]) + end + end + end + + def test_concrete_siblings_under_an_abstract_ancestor_remain_ambiguous + create_and_blow_away_old_db + table_name = "fixture_builder_abstract_sibling_models" + + with_temporary_table(table_name, columns: {name: :string}) do + with_abstract_sibling_models( + "FixtureBuilderAbstractAncestor", + ["FixtureBuilderAbstractAlpha", "FixtureBuilderAbstractZulu"], + table_name: table_name + ) do + force_fixture_generation + + error = assert_raise(FixtureBuilder::AmbiguousModelError) do + build_fixtures_for(table_name) do + ActiveRecord::Base.connection.execute( + "INSERT INTO #{table_name} (name) VALUES ('Merlin')" + ) + end + end + + assert_equal %w[FixtureBuilderAbstractAlpha FixtureBuilderAbstractZulu], + error.models.map(&:name) + end + end + end + + def test_separate_pool_model_with_the_same_table_name_is_ignored + create_and_blow_away_old_db + table_name = "fixture_builder_separate_pool_models" + + with_temporary_table(table_name, columns: {name: :string}) do + with_separate_pool_model("FixtureBuilderSeparatePool", table_name: table_name) do + force_fixture_generation + build_fixtures_for(table_name) do + ActiveRecord::Base.connection.execute( + "INSERT INTO #{table_name} (name) VALUES ('Base pool row')" + ) + end + + fixture = YAML.safe_load_file(test_path("fixtures/#{table_name}.yml")) + assert_equal "Base pool row", fixture.values.first["name"] + end + end + end + + def test_candidate_schema_errors_propagate + create_and_blow_away_old_db + table_name = "fixture_builder_schema_errors" + error_class = Class.new(StandardError) + + with_temporary_table(table_name, columns: {name: :string}) do + with_named_models(["FixtureBuilderSchemaError"], table_name: table_name) do |model| + model.define_singleton_method(:columns_hash) { raise error_class } + force_fixture_generation + + assert_raise(error_class) do + build_fixtures_for(table_name) do + ActiveRecord::Base.connection.execute( + "INSERT INTO #{table_name} (name) VALUES ('Merlin')" + ) + end + end + end + end + end + + def test_id_less_model_table_uses_raw_sql_and_preserves_select_aliases + create_and_blow_away_old_db + table_name = "fixture_builder_id_less_models" + + with_temporary_table(table_name, columns: {name: :string}, id: false) do + force_fixture_generation + FixtureBuilder.configure do |fbuilder| + fbuilder.files_to_check = [] + fbuilder.skip_tables = ActiveRecord::Base.connection.tables - [table_name] + fbuilder.select_sql = "SELECT *, upper(name) AS shouted_name FROM %s" + fbuilder.factory do + ActiveRecord::Base.connection.execute( + "INSERT INTO #{table_name} (name) VALUES ('Merlin')" + ) + end + end + + fixture = YAML.safe_load_file(test_path("fixtures/#{table_name}.yml")) + assert_equal "Merlin", fixture.values.first["name"] + assert_equal "MERLIN", fixture.values.first["shouted_name"] + end + end + + private + + def build_fixtures_for(*table_names, &factory) + FixtureBuilder.configure do |fbuilder| + fbuilder.files_to_check = [] + fbuilder.skip_tables = ActiveRecord::Base.connection.tables - table_names + fbuilder.factory(&factory) + end + end + + def with_temporary_table(table_name, columns:, id: true) + connection = ActiveRecord::Base.connection + options = {force: true} + options[:id] = false unless id + connection.create_table(table_name, **options) do |table| + columns.each { |name, type| table.column(name, type) } + end + yield connection + ensure + connection.drop_table(table_name) if connection&.data_source_exists?(table_name) + FileUtils.rm_f(test_path("fixtures/#{table_name}.yml")) + end + + def with_autoloaded_model(class_name) + path = test_path("#{class_name.underscore}.rb") + File.write(path, <<~RUBY) + Object.const_set(:#{class_name}, Class.new(ActiveRecord::Base) do + attribute :wizard_data, WizardDataType.new + end) + RUBY + Object.autoload(class_name.to_sym, path) + yield + ensure + Object.send(:remove_const, class_name) if Object.const_defined?(class_name, false) + FileUtils.rm_f(path) if path + end + + def with_named_models(class_names, table_name:) + models = class_names.map do |class_name| + Object.const_set(class_name, Class.new(ActiveRecord::Base)) + end + models.each { |model| model.table_name = table_name } + yield(*models) + ensure + remove_model_constants(models.reverse) + end + + def with_abstract_sibling_models(ancestor_name, class_names, table_name:) + models = [] + ancestor = Object.const_set(ancestor_name, Class.new(ActiveRecord::Base)) + ancestor.abstract_class = true + class_names.each do |class_name| + models << Object.const_set(class_name, Class.new(ancestor)) + end + models.each { |model| model.table_name = table_name } + yield(*models) + ensure + remove_model_constants(models.reverse + [ancestor]) + end + + def with_separate_pool_model(class_name, table_name:) + model = Object.const_set(class_name, Class.new(ActiveRecord::Base)) + model.establish_connection(adapter: "sqlite3", database: ":memory:") + model.table_name = table_name + yield model + ensure + model&.connection_pool&.disconnect! + remove_model_constants([model]) + end + + def remove_model_constants(models) + models&.each do |model| + next unless model&.name && Object.const_defined?(model.name, false) + + Object.send(:remove_const, model.name) + end + end +end +# standard:enable Rails/ApplicationRecord diff --git a/test/support/test_database.rb b/test/support/test_database.rb index 70f247e..bc32ecc 100644 --- a/test/support/test_database.rb +++ b/test/support/test_database.rb @@ -57,20 +57,17 @@ def create_and_blow_away_old_db # database-generated column. create_generated_column_table(GENERATED_COLUMN_RECORDS_TABLE) - # The table FixtureBuilder iterates (`relocated_creatures`) alongside the - # differently named table `RelocatedCreature` actually reads. - # - # The two tables expose deliberately incompatible schemas: the iterated - # table's only writable column is `unrelated` and its `name` is - # database-generated, while the model's table has a writable `name`. Reading - # generated columns from the iterated table instead of the model's table - # therefore strips `name` from the fixture. + # `RelocatedCreature` is configured for `creature_archive`, even though its + # class name conventionally maps to the distinct `relocated_creatures` table. + # These intentionally incompatible tables prove resolution follows the + # configured table name rather than inferred constant naming. connection.create_table(RELOCATED_CREATURES_TABLE, force: true) do |t| t.string :unrelated t.virtual :name, type: :string, as: "upper(unrelated)", stored: true end connection.create_table(CREATURE_ARCHIVE_TABLE, force: true) do |t| t.string :name, null: false + t.json :wizard_data end GeneratedCreature.reset_column_information diff --git a/test/test_helper.rb b/test/test_helper.rb index ccb97e0..1375ab6 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -104,11 +104,11 @@ def wizard_data(attributes) class GeneratedCreature < ActiveRecord::Base end -# Inferable from the `relocated_creatures` table name, but backed by a -# differently named table, so writable column names must come from -# `table_name` rather than the table FixtureBuilder is iterating. +# Its configured table name is intentionally not inferable from this class +# name, so FixtureBuilder must resolve it from loaded model metadata. class RelocatedCreature < ActiveRecord::Base self.table_name = "creature_archive" + attribute :wizard_data, WizardDataType.new end class MagicalCreature < ActiveRecord::Base