Skip to content

fix(model): resolve association foreign key defaults against either reference convention - #3353

Merged
bpamiri merged 1 commit into
developfrom
fix/3337-underscore-reference-foreignkey
Aug 4, 2026
Merged

fix(model): resolve association foreign key defaults against either reference convention#3353
bpamiri merged 1 commit into
developfrom
fix/3337-underscore-reference-foreignkey

Conversation

@bpamiri

@bpamiri bpamiri commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Closes #3337. Implements option 4 + option 3 from the issue discussion — schema-driven resolution plus a dev-mode assertion — rather than the issue's preferred option 1. Rationale below; it is a deliberate departure and easy to reverse.

Reproduced first

Same flag state throughout, lucee7 + sqlite, before touching anything:

migrator_emits        = author_id
model_derives_flagON  = authorid
model_derives_flagOFF = authorid     <- identical, so the model never reads the flag
memoized              = yes

useUnderscoreReferenceColumns appears in migrator/TableDefinition.cfc, migrator/Migration.cfc, events/init/orm.cfm and the CLI advisories — and nowhere under vendor/wheels/model/.

Why not option 1 (make the default read the flag)

Two reasons, both found while verifying:

  1. It breaks a population the project already knows about. An app that flipped the flag on mid-life has legacy xxxid columns and bare associations that resolve correctly today. Option 1 breaks exactly those — and cli/lucli/Module.cfc:5267 ships a mixed-convention advisory whose whole purpose is warning those users.
  2. The two layers have different flag lifetimes. migrator/CLAUDE.md:53 promises a runtime flip works, and references() re-reads $get() per call. But the model-side default is memoized for the application lifetime (expandedMetadataFilled, under a lock). A flag-driven default would take effect for migrations and not for models — one inconsistency swapped for another.

Resolving against the columns that actually exist avoids both, and is strictly error-reducing: the underscore form is only consulted when the legacy form is absent, which is a case that threw before. It cannot turn a working app into a broken one.

The change

$deriveAssociationForeignKey() tries the legacy <modelName><key> shape, falls back to <modelName>_<key>, and returns whichever resolves against the model that owns the column. Which side that is depends on the association type, and I verified it rather than assuming:

association FK column lives on verified
belongsTo the declaring model (post.authorid) onCurrent=Y onAssociated=N
hasMany / hasOne the associated model (comment.postid) onCurrent=N onAssociated=Y

Plus Wheels.AssociationForeignKeyNotFound (option 3): when a derived default matches no column, throw at association-resolution time naming the association, both candidate shapes, and foreignKey= as the fix — instead of key [userid] doesn't exist from deep inside the join builder. It sits inside the existing memo, so the success path costs one check per application lifetime; it is gated on showErrorInformation; and it never second-guesses an explicit foreignKey=.

Scope: polymorphic is NOT covered

belongsTo(polymorphic=true) and hasMany/hasOne with as= pin their foreign key at registration time (associations.cfc:30, :81, :134), before the schema is available, so the join-time resolution never sees a blank to fill. Those still need an explicit foreignKey="<name>_id" under the underscore convention.

I have documented that rather than leaving it implicit — the root CLAUDE.md previously claimed the _type half worked too.

Three documents were asserting an alignment no code implemented

  • vendor/wheels/migrator/CLAUDE.md:50
  • root CLAUDE.md:269
  • cli/lucli/templates/app/config/settings.cfm:29 — the worst one, because wheels new writes that comment into every generated app

All three now describe what the code does, including the polymorphic gap and why the model side deliberately does not read the flag.

Red-first

With sql.cfc reverted to develop and the fixtures + spec in place, 7 of 8 specs fail, the two central ones with the reported symptom:

key [refParentid] doesn't exist

Fixtures RefParent / RefChild use the underscore shape. A separate spec pins that post belongsTo author still derives authorid, since the framework's own fixtures are all legacy-shaped — so the change cannot have quietly shifted existing behaviour.

Verification

lucee7, full core suite, one container:

leg result
sqlite 4721 pass / 7 fail / 4 error / 4750 specs
mysql 4729 pass / 7 fail / 4 error / 4746 specs

All 8 new specs pass on both. The 11 remaining failures are an identical pre-existing cluster on both databases (app.controllers.Controller missing its mixed-in helpers) — local to my container, absent from CI's legs, not chased.

Compat matrix dispatched separately; this touches the association hot path on every engine, so I am not shipping a cross-engine claim on inference. Expect the red Wheels Test Results check on this head as a result — the #3302 misattribution artifact, not a regression.

…eference convention

`useUnderscoreReferenceColumns` (framework default `false`, `wheels new` template default
`true`) makes the migrator emit `user_id`, but the model layer derived `userid` by
unconditional concatenation. A stock new app that declared `belongsTo("user")` without an
explicit `foreignKey` therefore threw `key [userid] doesn't exist` the first time any
`include=` traversed the association — a migrator and a model layer that could never agree.

Reproduced before changing anything, same flag state throughout:

  migrator_emits        = author_id
  model_derives_flagON  = authorid
  model_derives_flagOFF = authorid   <- identical, so the model never read the flag

The default is now resolved against the columns that actually exist on whichever side owns
the foreign key — `belongsTo` looks at the declaring model, `hasMany`/`hasOne` at the
associated one (verified empirically, not assumed). Both conventions work, including apps
that enabled the flag mid-life and hold a mix of both shapes.

Schema-driven rather than flag-driven, deliberately. `references()` re-reads `$get()` on
every call, while this result is memoized for the application lifetime
(`expandedMetadataFilled`), so a flag-driven default would let a runtime flip change
migrations without changing models — replacing one inconsistency with another. It is also
strictly error-reducing: the underscore form is only consulted when the legacy form is
absent, which is a case that threw before, so it cannot break a working app. That matters
because the CLI ships a mixed-convention advisory (`Module.cfc:5267`) precisely because
apps with the flag on and legacy columns exist — an unconditional flag-driven change would
have broken exactly those.

Also adds `Wheels.AssociationForeignKeyNotFound`: when a DERIVED default matches no column
on the owning model, throw at association-resolution time naming the association, both
candidate shapes, and `foreignKey=` as the fix. Runs inside the existing memo so the
success path costs one check per application lifetime, gated on `showErrorInformation`, and
never second-guesses an explicit `foreignKey=`.

Scope: polymorphic associations are NOT covered. `belongsTo(polymorphic=true)` and
`hasMany`/`hasOne` with `as=` pin their foreign key at registration time
(`associations.cfc:30`, `:81`, `:134`), before the schema is available, so the join-time
resolution never sees a blank to fill. Documented rather than silently left — the root
CLAUDE.md previously claimed the `_type` half worked too.

Corrects three documents that asserted an alignment no code implemented: root CLAUDE.md,
`vendor/wheels/migrator/CLAUDE.md`, and — worst — the comment `wheels new` writes into
every generated app's `config/settings.cfm`.

7 regression specs plus a legacy-shape guard, red-first: with `sql.cfc` reverted, 7 of the
8 fail and the two central ones fail with the reported symptom, `key [refParentid] doesn't
exist`. Fixtures `RefParent` / `RefChild` and their tables use the underscore shape; the
framework's own fixtures are all legacy-shaped, which the guard spec pins.

Verification, lucee7, full core suite in one container:

  sqlite  4721 pass / 7 fail / 4 error / 4750 specs
  mysql   4729 pass / 7 fail / 4 error / 4746 specs

All 8 new specs pass on both. The 11 remaining failures are an identical pre-existing
cluster on both databases (`app.controllers.Controller` missing its mixed-in helpers),
local to this container and absent from CI's legs.

Closes #3337

Signed-off-by: Peter Amiri <peter@alurium.com>

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer

TL;DR — This PR fixes issue 3337: the model-layer association foreign-key default derived <modelName><key> unconditionally, so a stock wheels new app (which sets useUnderscoreReferenceColumns=true, making the migrator emit <name>_id) threw key [<name>id] does not exist on any include=. The fix resolves the default against the columns that actually exist on whichever side owns the FK, adds a clear dev/testing-only Wheels.AssociationForeignKeyNotFound error, and documents that polymorphic associations are out of scope. The change is schema-driven, well-reasoned, cross-engine clean, and backed by focused regression coverage. Verdict: approve.

Correctness
The refactor preserves legacy behaviour exactly and is strictly error-reducing, which I verified against the surviving branches:

  • $deriveAssociationForeignKey (vendor/wheels/model/sql.cfc:1621) tries the legacy shape first and returns it when it resolves, consults the underscore shape only when legacy is absent, and falls back to legacy when neither resolves. So an app whose legacy FK resolves gets an identical result; an app that used to throw is the only one whose result changes.
  • The side-selection is equivalent to the old inline logic: belongsTo selects fkNameSource=associatedClass, fkColumnOwner=class (sql.cfc:1397-1399); hasMany/hasOne the reverse. $buildForeignKeyList("user","a,b","_") yields user_a,user_b, matching the old Replace-based composite handling.
  • $foreignKeyListResolves (sql.cfc:1600) correctly checks property names, not column names — matching the join builder lookup properties[foreignKey].column — and guards the empty list with return local.iEnd > 0.
  • The assertion at sql.cfc:1415 is gated on application.wheels.showErrorInformation (skipped in production) and only fires for defaults that were already going to throw key [xxx] does not exist downstream in the same $expandedAssociations call, so no working app is newly broken. The !StructCount(...properties) early-return at sql.cfc:1672 correctly lets un-migrated/tableless models surface the query error instead.

Cross-engine
Clean. The new helpers are public with $ prefix (invariant 7), use only ListToArray/ArrayLen/ListAppend/Trim/StructKeyExists/StructCount, have no .map()/bracket-notation calls/closures, and their for loops are not inside finally. The spec try/catch blocks accumulate into a shared state struct (var state = {threw = false}) rather than local.X — correct per invariant 11 (BoxLang catch-scope discard).

Tests
AssociationForeignKeyConventionSpec.cfc is a proper wheels.WheelsTest BDD spec and covers both branches (belongsTo where the column lives on this model, hasMany where it lives on the associated model), the legacy-shape preservation via the existing post/author fixtures, the traverses-without-throwing reported symptom, composite-key list building, and all three assertion paths (throws-and-names-both-shapes, resolves-silently, tableless-silent). Fixtures RefParent/RefChild plus populate.cfm tables are wired correctly and added to the teardown list.

Docs
Root CLAUDE.md, vendor/wheels/migrator/CLAUDE.md, and the changelog fragment changelog.d/3337-association-foreignkey-underscore.fixed.md (correct .fixed.md type, not a direct CHANGELOG.md edit) are all updated and mutually consistent, including the honest polymorphic-scope caveat. Nice catch correcting the three documents that previously asserted an alignment no code implemented.

Commits
fix(model): resolve association foreign key defaults against either reference convention — valid type/scope, lowercase subject, under 100 chars, and the body explains the why (schema-driven vs flag-driven, memoization lifetime). Conforms to commitlint.config.js.

Minor (non-blocking): the underscore fallback for composite primary keys emits <name>_<keyA>,<name>_<keyB>. t.references() only ever produces a single <name>_id/<name>_type pair, so the composite-underscore shape is speculative — but it is harmless (only a fallback candidate that degrades to the legacy shape if it does not resolve), so no change needed.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Wheels Test Results

     31 files    9 772 suites   21m 21s ⏱️
131 061 tests 130 591 ✅ 397 💤 38 ❌ 35 🔥
132 993 runs  132 523 ✅ 397 💤 38 ❌ 35 🔥

For more details on these failures and errors, see this check.

Results for commit 30e46ea.

@bpamiri
bpamiri merged commit ab901cf into develop Aug 4, 2026
13 of 19 checks passed
@bpamiri
bpamiri deleted the fix/3337-underscore-reference-foreignkey branch August 4, 2026 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

useUnderscoreReferenceColumns=true (the wheels new default) produces columns the belongsTo/hasMany foreignKey default never matches

1 participant