Repository navigation
Let applications choose the model for an ambiguous table - #136
Merged
Merged
Conversation
Scripts, rake tasks, and one-off backfills often define their own model for an existing table. Once the application eager-loads, that model ties with the application model and the build raises AmbiguousModelError with no way to proceed. Add model_classes, which maps table names to models and is consulted before discovery, and an opt-in prefer_conventional_models, which picks the conventionally named model when exactly one ambiguous candidate has that name. Ambiguity still raises by default, and the error names both settings.
There was a problem hiding this comment.
🟡 Changes recommended
Missing mappings raise the wrong exception, STI mappings can omit rows, and validation occurs after destructive cleanup.
3 open findings
What changed in this PR
Adds configurable model selection for ambiguous fixture tables.
Changes:
- Adds explicit model mappings and conventional-model preference.
- Improves ambiguity guidance and documentation.
- Adds resolver and integration tests.
| File | Description |
|---|---|
lib/fixture_builder/model_resolver.rb |
Implements model-selection settings. |
lib/fixture_builder/configuration.rb |
Exposes configuration options. |
lib/fixture_builder/builder.rb |
Passes settings to the resolver. |
lib/fixture_builder/ambiguous_model_error.rb |
Expands resolution guidance. |
test/model_resolver/model_classes_test.rb |
Tests explicit mappings. |
test/model_resolver/conventional_preference_test.rb |
Tests conventional preference. |
test/model_resolver/ambiguous_model_error_test.rb |
Updates error assertions. |
test/support/model_resolver_ambiguity_behavior.rb |
Updates shared ambiguity assertions. |
README.md |
Documents both settings. |
CHANGELOG.md |
Records the new functionality. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+131
to
+134
| resolver = ModelResolver.new( | ||
| connection_pool: ActiveRecord::Base.connection_pool, | ||
| model_classes: model_classes, | ||
| prefer_conventional_models: prefer_conventional_models |
| def mapped_model(table_name) | ||
| mapping = @model_classes.fetch(table_name) | ||
| model = mapping.is_a?(Module) ? mapping : mapping.to_s.constantize | ||
| return model if model < ActiveRecord::Base && eligible_model?(model, table_name) |
Comment on lines
+36
to
+40
| model = mapping.is_a?(Module) ? mapping : mapping.to_s.constantize | ||
| return model if model < ActiveRecord::Base && eligible_model?(model, table_name) | ||
|
|
||
| raise ArgumentError, "model_classes maps #{table_name} to #{model.name}, which is not an eligible model " \ | ||
| "for that table: it must be concrete, named, use the table on the same connection, and have a primary key" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Model-aware fixtures pick a model for each table from the loaded models and raise
AmbiguousModelErrorwhen unrelated models share a table. Scripts, rake tasks, and one-off backfills often define a small row model for an existing table, so once an application eager-loads (typically in CI), the build stops and offers no way forward short of changing application code. Migration-local models are already ignored (#129), but this is the same pattern outside migrations.This adds two settings:
model_classesmaps table names to a model class or class name and is consulted before discovery. A mapped model must pass the same eligibility checks as a discovered one; otherwise the build raisesArgumentErrornaming the table and class.prefer_conventional_modelsis off by default. When enabled and several unrelated models match a table, the one namedtable_name.classifyis chosen if exactly one has that name; otherwise ambiguity still raises.AmbiguousModelErrornow names both settings. The README documents them next to model discovery, and the changelog has an Added entry.Tests cover a row model beside its application model with and without each setting, a mapping to a model for another table, a preference with no conventionally named candidate, and both settings applied through
FixtureBuilder.configure.Closes #135