WEB-8347: support non-default primary key types for belongs_to and HABTM foreign keys#183
Conversation
15219cb to
15c7189
Compare
Replace eager `reflection.klass` lookup at `belongs_to` time with a
deferred `resolver:` callback installed on the placeholder FieldSpec.
The migration generator invokes the resolver after `eager_load!` (so
all parent classes are loaded) and swaps in a fully-mirrored FieldSpec
that matches the parent's primary key type, limit, and other options.
This avoids the dependency-cycle hazard where a child model declares
`belongs_to :parent` before the parent class is loaded, and removes the
need for the post-hoc `fk_field_options` DB-column lookup that was
silently overriding the spec's :limit at change_table time.
Three corrections required to make this work end-to-end:
1. `_primary_key_field_spec_from_table_options` now handles the
bare-Symbol form `declare_schema id: :integer` in addition to the
Hash form. Previously the Symbol form silently fell through to
:bigint default; the old fk_field_options DB lookup was masking
the bug.
2. The resolver reconciles its declared-PK-derived spec against the
live DB column when the type matches but :limit differs (e.g. a
legacy table where `id` is INT(4) but the model now declares the
default :bigint). This preserves the behavior of the removed
fk_field_options without overriding intentional type changes.
3. The HABTM shim falls back to a :bigint FieldSpec when the parent
class is not a declare_schema model (no `_foreign_key_field_spec`),
instead of raising NoMethodError.
Co-authored-by: Cursor <cursoragent@cursor.com>
HabtmModelShim.new was changed earlier on this branch from (join_table, foreign_keys, parent_table_names, connection:) to (join_table, parents, connection:) where parents is an array of [foreign_key, parent_class] pairs. The instance-methods spec subject was still using the old 3-positional-arg signature and crashing on load. Update to the new shape and provide named Advertiser/Campaign constants for the long-name context (anonymous classes have nil .name, which crashes the descendant-nuking loop in prepare_testapp.rb). Co-authored-by: Cursor <cursoragent@cursor.com>
Three specs covering the behavior enabled by the deferred resolver:
1. belongs_to defers parent class lookup to migration time, so a
child can reference an as-yet-undefined parent without raising
NameError.
2. belongs_to mirrors a parent's :string, limit: 36 primary key
(declared via declare_schema id: { type: :string, limit: 36 }),
producing a matching string foreign key.
3. HABTM mirrors both parents independently when each declares a
different non-default primary key type (:string, limit: 36 and
:integer, limit: 4).
Co-authored-by: Cursor <cursoragent@cursor.com>
Pull the Hash-vs-Symbol parsing of `declare_schema id: ...` out of _primary_key_field_spec_from_table_options into a small private helper. Slims the caller down to its essential logic (resolve raw value, fall back to Rails generator default, build FieldSpec) and makes the supported forms self-documenting in one place. No behavior change. 253 / 0 / 0 on MySQL, 253 / 0 / 8 on sqlite3. Co-authored-by: Cursor <cursoragent@cursor.com>
The resolver: callback now has exactly one shape: take the placeholder FieldSpec, return the final FieldSpec. The migrator unconditionally swaps in whatever the resolver returns. No nil case, no mutate-in-place backward-compat branch. When the resolver has no opinion (parent class isn't a declare_schema model and we can't ask for its PK spec), it returns the placeholder unchanged -- semantically a no-op, but expressed in the same shape. Also extract the parent-PK mirroring + live-DB reconciliation into a focused _mirror_parent_primary_key helper. Co-authored-by: Cursor <cursoragent@cursor.com>
The workflow pinned bundler to 2.2.29 for all Ruby versions. That bundler vendors an old Thor that references DidYouMean::SPELL_CHECKERS, which Ruby 3.4 removed -- so every Ruby 3.4 job now dies during `bundle install` before any spec runs: bundler-2.2.29/.../thor/error.rb:105: uninitialized constant DidYouMean::SPELL_CHECKERS (NameError) Letting setup-ruby pick `bundler: latest` restores Ruby 3.4 CI without affecting the older Ruby versions (which already passed and continue to pass on newer bundlers). Unrelated to WEB-8347 in scope but folded in to clear the PR's check failures. Co-authored-by: Cursor <cursoragent@cursor.com>
CI has only tested Rails 7.0–8.0 for some time and the Appraisals file already gates MIN_RAILS_VERSION at 7.0.0. Make the floor explicit: - Bump gemspec required Rails to >= 7.0. - Bump root Gemfile to ~> 7.0 (matches the CI floor). - Remove stale gemfiles/rails_6_*.gemfile.lock leftovers. - Note the drop in CHANGELOG. Removed dead branches in: lib/declare_schema.rb#current_adapter (the Rails::VERSION::MAJOR >= 6.1 check is now always true under Rails 7+, so the connection_config fallback is gone). Co-authored-by: Cursor <cursoragent@cursor.com>
The two existing Command Injection ignores for command.rb had stale fingerprints — the mysql2 rescue block was removed and line numbers shifted. Warnings are unchanged in intent and still accepted. Co-authored-by: Cursor <cursoragent@cursor.com>
The root Gemfile.lock pinned rexml 3.3.8, which has two open ruby-advisory-db entries (GHSA-2rxp-v6pw-ch6m High, GHSA-c2f4-jgmc-q2r5 Unknown). Bump to 3.4.4 to clear them. This is the only remaining bundler-audit finding after the Rails 6.x drop in 6d125de moved the root lockfile off the EOL Rails 6.1.7.8 chain (which had been dragging in vulnerable transitive deps — actionpack, activerecord, activestorage, rack, nokogiri, etc.). The appraisal gemfiles/*.gemfile.lock files are gitignored and not scanned by SSaaS. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
2e929f6 to
533632c
Compare
position: and null: are passed explicitly by every caller (_mirror_parent_primary_key and _foreign_key_field_spec). The position: 0 default would silently put an FK at column 0 if a future caller forgot it, and null: nil's inherit-from-source sentinel is meaningful as a value but not as a callsite default. Make both required. Co-authored-by: Cursor <cursoragent@cursor.com>
…mary_key_type Three FK fallbacks were hardcoded to :bigint and a fourth path (_primary_key_field_spec_from_table_options) was reading the Rails generators config directly. Extract a single memoized helper so apps that set config.generators.primary_key_type get consistent behavior across: - Polymorphic FK fallback in _infer_foreign_key_field_spec - Non-polymorphic FK placeholder before the resolver runs - _primary_key_field_spec_from_table_options when no id: type was given - HabtmModelShim#field_specs for non-declare_schema parents Co-authored-by: Cursor <cursoragent@cursor.com>
The shim historically returned `false` because ActiveRecord pre-7.1 had no notion of composite primary keys. We now require Rails >= 7.0 and the join table's PK is conceptually the composite of its two foreign keys (already exposed via _declared_primary_key and used by the migrator's create_table primary_key: [...] branch. Make #primary_key return the same composite array so the duck-type matches AR's modern composite-PK contract. Nothing in declare_schema's hot paths called #primary_key on a shim; the only consumer was the spec assertion, which is updated. EOF ) Co-authored-by: Cursor <cursoragent@cursor.com>
The arg is the FieldSpec built at belongs_to time from DeclareSchema.default_generated_primary_key_type. The resolver may mirror the parent's PK (when the parent is a declare_schema model) or accept the default as the final answer. 'default_spec' names what the value is and composes with the new helper. Co-authored-by: Cursor <cursoragent@cursor.com>
The migrator was reaching into field_spec.options.delete(:resolver), mutating the spec internal options hash to extract the callback. Pull resolver out of **options into its own keyword arg with an attr_reader so the migrator just reads field_spec.resolver. No mutation, no risk of :resolver leaking into @sql_options or the mirrored FK spec. Co-authored-by: Cursor <cursoragent@cursor.com>
…ary_key_type When the declared primary key isn't in the DB yet, Migrator#change_table appends it to `to_add` without a FieldSpec. The fallback type was hardcoded to `:integer`, which silently overrode `config.generators.primary_key_type` for apps that configured it. Route the fallback through DeclareSchema.default_generated_primary_key_type for consistency with the rest of the gem, and add a spec that stubs the helper to prove the call site delegates to it. Co-authored-by: Cursor <cursoragent@cursor.com>
Mirror the production file's path: lib/generators/declare_schema/migration/migrator.rb already lives in spec/lib/generators/declare_schema/migration/migrator_spec.rb, so the new spec belongs alongside it rather than under spec/lib/declare_schema/. Co-authored-by: Cursor <cursoragent@cursor.com>
Ticket numbers are fleeting; spec names should describe behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
The migrator was reaching into FieldSpec to fish out the optional
resolver lambda and call it. Hide that mechanic behind a single method:
m.field_specs.transform_values!(&:resolve)
#resolve returns self when no resolver was supplied, otherwise calls
the resolver with self and memoizes the result. Drop the public
:resolver attr_reader since callers now go through #resolve. Add
focused unit specs covering both branches plus memoization.
Co-authored-by: Cursor <cursoragent@cursor.com>
…neration 1. STI subclass `_table_options` nil guard `_primary_key_field_spec_from_table_options` indexed `_table_options` directly, which raised `NoMethodError: undefined method '[]' for nil` when a `belongs_to` pointed at an STI subclass. STI subclasses inherit `field_specs` via `inheriting_cattr_reader` but never call `declare_schema` themselves, so their own `@_table_options` is nil. Use safe navigation and let the existing default-PK fallback handle it. 2. HabtmModelShim missing `table_name=` and `columns_hash` The migrator's `with_previous_model_table_name` helper (used by `change_column_back` / `add_column_back`) assigns to `model.table_name=` and reads `model.columns_hash`. AR classes have these; the shim did not, so any HABTM join-table column change crashed with `NoMethodError: undefined method 'table_name=' for an instance of DeclareSchema::Model::HabtmModelShim`. Add a `table_name=` that rewrites `@join_table` (the only state `table_name` reads) and a `columns_hash` backed by `connection.columns(table_name).index_by(&:name)`. Co-authored-by: Cursor <cursoragent@cursor.com>
Consolidate three near-duplicate helpers (_mirror_parent_primary_key, _mirror_parent_primary_key_from_db, _infer_primary_key_from_db) into: - _parent_pk_field_spec(klass) - the spec to mirror, or nil - _reconcile_fk_with_live_pk(klass) - pin live :limit when drift detected - _pk_column_for(klass) - safe live PK column lookup - _field_spec_options_from_pk_column - column -> [type, options] translator Preserve child belongs_to column_options (notably :default) through PK mirroring -- they were silently dropped, causing phantom change_column runs. Add brief YARD comments to all touched methods documenting the deferred- resolver flow, the nil-as-fallback signaling, and the int(4)/bigint(8) distinction MySQL hides behind ActiveRecord's :integer type. 44 examples, 0 failures. Co-authored-by: Cursor <cursoragent@cursor.com>
The deferred-resolution callback added in c0eb4ed is conceptually a block -- it's "this is the work that runs later". Replace the `resolver:` kwarg on FieldSpec.new with a `&resolver` parameter so callers use the natural Ruby idiom: FieldSpec.new(...) do |default_spec| _resolve_belongs_to_foreign_key_field_spec(reflection, default_spec) end instead of constructing a lambda first and passing it via kwarg. Trims one item from FieldSpec#initialize's already-busy keyword list and removes the lambda-as-kwarg dance at the (sole) call site. No external callers exist; safe pure refactor. Co-authored-by: Cursor <cursoragent@cursor.com>
Move the deferred-resolution behavior off FieldSpec into a dedicated DeferredFieldSpec class. FieldSpec returns to being a pure concrete spec; DeferredFieldSpec wraps a default FieldSpec plus a resolver block and exposes #resolve to swap itself for the produced FieldSpec. The migrator's transform_values!(&:resolve) loop dispatches uniformly: FieldSpec#resolve returns self (duck-type), DeferredFieldSpec#resolve runs the block. Used at exactly one site (belongs_to between two declare_schema models, where mirroring the parent's PK at field-declaration time would risk model-load cycles). Composing with default_spec rather than mirroring FieldSpec's constructor keeps DeferredFieldSpec tiny. Two test assertions that read field_specs[fk_name].options before the migrator runs now call .resolve.options first; no production code reads deferred FK specs pre-resolve. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…K spec The default-PK-type integration spec used `integer PRIMARY KEY AUTOINCREMENT NOT NULL` for non-mysql adapters. AUTOINCREMENT is sqlite-only syntax and postgres rejects it with `PG::SyntaxError: syntax error at or near "AUTOINCREMENT"`, failing every postgres CI build. The migrator only inspects column type and null-ness, not auto-increment metadata, so unify all three adapters (sqlite/mysql/postgres) on `integer PRIMARY KEY NOT NULL`. Co-authored-by: Cursor <cursoragent@cursor.com>
- Replace `respond_to? ? : ` ternary with `try(:_declared_primary_key)`. - Fold the `if pk_name` guard into the assignment. - Drop `.to_s` on `pk_name`: both `_declared_primary_key` and AR's `primary_key` already normalize to String for single-column PKs. - Document that compound PKs (Array PK name) intentionally return nil; callers already nil-guard and gracefully fall back to the gem default. Co-authored-by: Cursor <cursoragent@cursor.com>
…t `_parse_pk_table_options` - Expand the inner ternary in `_field_spec_options_from_pk_column` into an explicit `if`/`else` for symmetry with the outer guard and easier reading. - Flesh out the YARD on `_parse_pk_table_options` to document the input shapes accepted by `declare_schema id: ...`, the normalized return shape, and the `type.nil?` signaling contract used by the caller to decide between live-PK inspection and the gem default. Co-authored-by: Cursor <cursoragent@cursor.com>
Expand the one-line comment on `habtm_tables` to spell out: - why we iterate every AR descendant and key by `join_table` rather than by parent model (each join table is shared by both sides of the association, so the naive iteration yields two reflections per table); - why `||=` is correct (we want exactly one shim per join table, so silently dropping the second-seen side is the desired behavior); - the return shape, for callers building HabtmModelShims. Co-authored-by: Cursor <cursoragent@cursor.com>
| require 'active_record' | ||
| require 'declare_schema/dsl' | ||
| require 'declare_schema/model' | ||
| require 'declare_schema/field_declaration_dsl' |
There was a problem hiding this comment.
Turns out this was dead code! There's a newer Dsl class.
| _declared_primary_key = model._declared_primary_key | ||
|
|
||
| name.to_s == _declared_primary_key and raise ArgumentError, "you may not provide a field spec for the primary key #{name.inspect}" | ||
|
|
There was a problem hiding this comment.
This assertion moved up a layer.
| pre_migration.call(field_spec) | ||
| end | ||
| end | ||
| m.try(:field_specs)&.transform_values!(&:resolve) |
There was a problem hiding this comment.
This is the main change of the PR. Every model gets resolved (if not already) just before generating the migration. Resolution happens just in time here so we can see the shape of the type and options of the primary key we're pointing to. For example, it could be a type: :binary, { limit: 16 }. Previously we'd only adapt the integer limit between 4 and 8.
| habtm_tables.each do |name, reflections| | ||
| models_by_table_name[name] = ::DeclareSchema::Model::HabtmModelShim.from_reflection(reflections.first) | ||
| habtm_tables.each do |name, reflection| | ||
| models_by_table_name[name] = ::DeclareSchema::Model::HabtmModelShim.from_reflection(reflection) |
There was a problem hiding this comment.
We weren't looking beyond the first, so why hold the list at all?
| end | ||
| end | ||
|
|
||
| def fk_field_options(model, field_name) |
There was a problem hiding this comment.
This is the old code that fixed up the limit but didn't know how to have an entirely different foreign key type, like binary(16).
| else | ||
| model_class.connection_config[:adapter] | ||
| end | ||
| model_class.connection_db_config.adapter |
There was a problem hiding this comment.
Dropped support of Rails 6.
There was a problem hiding this comment.
Pull request overview
This PR updates declare_schema’s migration generation to correctly mirror non-default primary key types/attributes into belongs_to and HABTM foreign keys by deferring FK type resolution until migration-generation time (after eager-loading), and adds specs to cover the new behavior.
Changes:
- Introduces deferred FK FieldSpecs (resolver-based) so
belongs_tocan mirror parent PK types without triggering autoload dependency cycles. - Updates the migration generator to resolve deferred specs up-front and removes the older FK limit inference logic.
- Improves HABTM join-table shim behavior/API surface and expands spec coverage (including default PK type integration).
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/declare_schema/model.rb | Defers FK spec resolution; adds PK spec helpers and live-DB reconciliation. |
| lib/declare_schema/model/field_spec.rb | Adds resolve no-op and foreign_key_field_spec mirroring helper. |
| lib/declare_schema/model/deferred_field_spec.rb | New placeholder type for deferred FieldSpec resolution. |
| lib/generators/declare_schema/migration/migrator.rb | Resolves deferred specs during generation; removes legacy FK limit override. |
| lib/declare_schema/model/habtm_model_shim.rb | Refactors shim initialization; attempts PK mirroring for declare_schema parents; adds model-like APIs. |
| lib/declare_schema.rb | Adds default_generated_primary_key_type; simplifies adapter lookup for Rails 7+. |
| lib/declare_schema/version.rb | Bumps gem version string. |
| lib/declare_schema/extensions/active_record/fields_declaration.rb | Removes require for deleted FieldDeclarationDsl. |
| lib/declare_schema/field_declaration_dsl.rb | Deletes unused DSL implementation. |
| spec/lib/declare_schema/migration_generator_spec.rb | Adds extensive FK mirroring specs; updates null/optional expectations to resolve deferred specs. |
| spec/lib/declare_schema/deferred_field_spec_spec.rb | Adds unit tests for DeferredFieldSpec behavior. |
| spec/lib/declare_schema/model/habtm_model_shim_spec.rb | Updates spec for new shim constructor and composite PK behavior. |
| spec/lib/generators/declare_schema/migration/migrator_default_pk_type_spec.rb | Adds integration coverage for default PK type when adding declared PK columns. |
| spec/lib/declare_schema/field_declaration_dsl_spec.rb | Removes tests for deleted DSL. |
| Gemfile / Gemfile.lock / declare_schema.gemspec | Updates Rails dependency baseline to 7.x. |
| CHANGELOG.md | Documents new version and Rails 6 support removal. |
| .github/workflows/pipeline.yml | Changes CI Bundler selection to latest. |
| config/brakeman.ignore | Updates ignore fingerprints/metadata after code movement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Non-declare_schema parent: fall back to the configured default PK type. | ||
| ::DeclareSchema::Model::FieldSpec.new(self, foreign_key, ::DeclareSchema.default_generated_primary_key_type, position: i, null: false) | ||
| end | ||
| end | ||
| end | ||
|
|
| if column&.type == :integer | ||
| if column.limit == 8 | ||
| [:bigint, {}] | ||
| else | ||
| [:integer, { limit: column.limit || 4 }] | ||
| end |
| # Default primary key type for generated foreign keys when we have no parent | ||
| # model to mirror (e.g. polymorphic FKs, non-declare_schema parents). Mirrors | ||
| # `config.generators.primary_key_type` so apps that override the Rails default | ||
| # get consistent behavior. Memoized — Rails config is set at boot. | ||
| def default_generated_primary_key_type | ||
| @default_generated_primary_key_type ||= | ||
| Rails.application.config.generators.options.dig(:active_record, :primary_key_type) || :bigint | ||
| end |
| Note: this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | ||
|
|
||
| ## [3.0.0] - Unreleased | ||
| ## [3.1.0] - Unreleased | ||
| ### Added | ||
| - Add HABTM support for arbitrary primary key in the referenced table (rather than just :bigint). | ||
|
|
||
| ### Removed | ||
| - Drop support for Rails 6.x. Minimum supported Rails is now 7.0. | ||
|
|
96c9c14 to
a595646
Compare
Summary
WEB-8347: support non-
:bigintprimary key types forbelongs_toandhas_and_belongs_to_manyforeign keys.declare_schemapreviously assumed every primary key was:bigint(or whatever the live DB column happened to be after the fact). When a model used a non-default PK type — e.g.declare_schema id: { type: :string, limit: 36 }— the foreign keys generated forbelongs_toand HABTM associations did not mirror that type, producing migrations that MySQL would reject as incompatible.The fix is to defer the FK's type decision from
belongs_totime (when the parent class might not be loaded yet — dependency-cycle hazard) to migration-generation time (aftereager_load!), via a&resolvercallback installed on aDeferredFieldSpec. The migration generator invokes the resolver, which mirrors the parent's_primary_key_field_spec(type, limit, charset, collation, etc.) and reconciles against the live DB column when it differs only in:limit.What changed
Production
lib/declare_schema/model.rb_infer_foreign_key_field_spec(non-polymorphic path) installs aresolver:callback on a:bigintplaceholder instead of touchingreflection.klasseagerly._resolve_belongs_to_foreign_key_field_specruns at migration generation time. Mirrors the parent's PK spec viaklass._primary_key_field_spec.foreign_key_field_spec(...)and reconciles:limitagainst the live DB column (handles legacy tables where declared bigint defaults disagree with on-disk INT(4))._primary_key_field_spec_from_table_optionsnow handles the bare-Symbol form (declare_schema id: :integer) in addition to the Hash form. The Hash/Symbol parsing is encapsulated in a small private helper_parse_pk_table_options.lib/generators/declare_schema/migration/migrator.rb:resolverand swaps in the FieldSpec it returns. The contract is intentionally minimal: take the placeholder, return the final FieldSpec.fk_field_options, the post-hoc DB-column lookup that was redundant after the resolver lands and was actively wrong for non-default PK types (it would overwrite a correctlimit:with whatever the DB happened to have).lib/declare_schema/model/habtm_model_shim.rb:bigintFieldSpecwhen a HABTM parent isn't adeclare_schemamodel (instead ofNoMethodErroron_foreign_key_field_spec).Specs
spec/lib/declare_schema/migration_generator_spec.rbNameErrorwhen child references parent before it's loaded),belongs_tomirroring:string, limit: 36PK, HABTM mirroring two parents with different non-default PK types.spec/lib/declare_schema/model/habtm_model_shim_spec.rbHabtmModelShim.newwas changed earlier on this branch from(join_table, foreign_keys, parent_table_names, connection:)to(join_table, parents, connection:)but the spec subject hadn't been updated).Test plan
bundle exec rake test:prepare_testapp[mysql,true] && bundle exec rake test:all→ 253 examples, 0 failures, 0 pendingbundle exec rake test:prepare_testapp[sqlite3,true] && bundle exec rake test:all→ 253 examples, 0 failures, 8 pending (8 are MySQL-only)_resolve_belongs_to_foreign_key_field_spec— we override:limitonly when type matches and limit differs; intentional type changes (e.g. bigint → string) are preserved.:resolvercallback name; we deliberately avoided "pre_migration:" since it's not a callback that runs before a migration is applied, but a resolver invoked during migration generation.