From 30e46eaa275188b5e79597b0b464b7e09b2f1d44 Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Mon, 3 Aug 2026 19:26:29 -0700 Subject: [PATCH] fix(model): resolve association foreign key defaults against either reference convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- CLAUDE.md | 6 +- ...association-foreignkey-underscore.fixed.md | 2 + vendor/wheels/migrator/CLAUDE.md | 6 +- vendor/wheels/model/sql.cfc | 144 +++++++++++++++++- .../wheels/tests/_assets/models/RefChild.cfc | 16 ++ .../wheels/tests/_assets/models/RefParent.cfc | 15 ++ vendor/wheels/tests/populate.cfm | 26 +++- .../AssociationForeignKeyConventionSpec.cfc | 141 +++++++++++++++++ 8 files changed, 349 insertions(+), 7 deletions(-) create mode 100644 changelog.d/3337-association-foreignkey-underscore.fixed.md create mode 100644 vendor/wheels/tests/_assets/models/RefChild.cfc create mode 100644 vendor/wheels/tests/_assets/models/RefParent.cfc create mode 100644 vendor/wheels/tests/specs/model/AssociationForeignKeyConventionSpec.cfc diff --git a/CLAUDE.md b/CLAUDE.md index 647c6ea149..c6f7beaa3b 100755 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -266,7 +266,11 @@ t.primaryKey(name="userId", autoIncrement=true); For new migrator helpers or anywhere you accept a column-name argument: declare `string columnNames` (NOT `required`), and call `$combineArguments(args=arguments, combine="columnNames,columnName", required=true)` at the top of the body. The pattern is documented in [vendor/wheels/migrator/CLAUDE.md](vendor/wheels/migrator/CLAUDE.md). Boolean nullable flag is `allowNull` everywhere — never `null`. -`t.references()` also respects `useUnderscoreReferenceColumns` (boolean, framework default `false`, `wheels new` template default `true`) — when true it produces `_id` / `_type` columns matching Wheels model `belongsTo` defaults. +`t.references()` also respects `useUnderscoreReferenceColumns` (boolean, framework default `false`, `wheels new` template default `true`) — when true it produces `_id` / `_type` columns instead of `id` / `type`. + +Association foreign-key defaults resolve **either** convention: the default derivation checks which column actually exists on whichever side owns the foreign key, rather than reading the setting ([#3337](https://github.com/wheels-dev/wheels/issues/3337) — before that fix the model layer derived `` unconditionally and a stock `wheels new` app threw `key [id] doesn't exist` on any `include=`). It is schema-driven on purpose: the migrator reads the flag per call, but the model-side default is memoized for the application lifetime, so honouring the flag there would let a runtime flip change migrations without changing models. Apps holding a mix of both shapes work for the same reason. + +**Polymorphic associations are not covered.** `belongsTo(polymorphic=true)` and `hasMany`/`hasOne` with `as=` fix their foreign key to `id` at *registration* time (`vendor/wheels/model/associations.cfc:30`, `:81`, `:134`), before the schema is available, so the join-time resolution never sees a blank to fill. Against an underscore-shaped schema those still need an explicit `foreignKey="_id"`. ## Wheels Conventions diff --git a/changelog.d/3337-association-foreignkey-underscore.fixed.md b/changelog.d/3337-association-foreignkey-underscore.fixed.md new file mode 100644 index 0000000000..747c022bcb --- /dev/null +++ b/changelog.d/3337-association-foreignkey-underscore.fixed.md @@ -0,0 +1,2 @@ +- Association foreign-key defaults now resolve either reference-column convention instead of only the legacy `` one. `useUnderscoreReferenceColumns` (framework default `false`, `wheels new` template default `true`) makes the migrator emit `user_id`, but the model layer derived `userid` unconditionally — so a stock new app that declared `belongsTo("user")` without an explicit `foreignKey` threw `key [userid] doesn't exist` the first time any `include=` traversed the association. 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), so both conventions work — including apps that enabled the flag mid-life and hold a mix of both shapes. This is deliberately schema-driven rather than reading the setting: `references()` re-reads the flag on every call while the model-side default is memoized for the application lifetime, so a flag-driven default would let a runtime flip change migrations without changing models. It is also strictly error-reducing — the underscore form is only consulted when the legacy form is absent, which is a case that used to throw. Polymorphic associations are not covered; they pin their foreign key at registration time, before the schema is available, and still need an explicit `foreignKey=` under the underscore convention (#3337) +- An association whose *derived* default foreign key matches no column on the model that owns it now throws `Wheels.AssociationForeignKeyNotFound` at association-resolution time, naming the association, both candidate column shapes, and `foreignKey=` as the fix. Previously this surfaced as `key [userid] doesn't exist` from deep inside the join builder, which named neither the association nor the argument that resolves it. Development and testing only, and only for defaults Wheels derived itself — an explicit `foreignKey=` is left alone (#3337) diff --git a/vendor/wheels/migrator/CLAUDE.md b/vendor/wheels/migrator/CLAUDE.md index becbcb60dd..bcbfc080a7 100644 --- a/vendor/wheels/migrator/CLAUDE.md +++ b/vendor/wheels/migrator/CLAUDE.md @@ -48,7 +48,11 @@ New code should pass `columnNames`. Both keep working. | `false` (framework default) | `userid` | `userid`, `usertype` | | `true` (new-app template default) | `user_id` | `user_id`, `user_type` | -The framework default is `false` so existing apps with applied migrations keep matching their database schemas. The `wheels new` template at `cli/lucli/templates/app/config/settings.cfm` opts new apps into `true` so they match Wheels model `belongsTo` defaults out of the box. +The framework default is `false` so existing apps with applied migrations keep matching their database schemas. The `wheels new` template at `cli/lucli/templates/app/config/settings.cfm` opts new apps into `true`. + +**The model side does not read this flag, and must not.** Association foreign-key defaults resolve against the columns that actually exist — `vendor/wheels/model/sql.cfc::$deriveAssociationForeignKey()` tries the legacy `` shape first and falls back to `_` — so both conventions work, including a schema holding a mix of the two. Making it flag-driven instead would break: this function's result is memoized for the application lifetime (`expandedMetadataFilled`), whereas `references()` re-reads `$get()` on every call, so a runtime flip would change migrations without changing models. Before [#3337](https://github.com/wheels-dev/wheels/issues/3337) the model layer derived `` unconditionally, which meant a stock `wheels new` app had a migrator and a model layer that could never agree. + +The exception is **polymorphic** associations, which pin their foreign key to `id` at registration time — see the note in the root `CLAUDE.md`. Those still need an explicit `foreignKey=` under the underscore convention. The flag is read via `$get("useUnderscoreReferenceColumns")` inside `references()` at runtime — apps can flip the setting in `config/settings.cfm` without reloading the framework. Migrations already applied to a real database are unaffected; only the column name the *next* migration produces changes. diff --git a/vendor/wheels/model/sql.cfc b/vendor/wheels/model/sql.cfc index 39fe54b657..0d6b945ef5 100644 --- a/vendor/wheels/model/sql.cfc +++ b/vendor/wheels/model/sql.cfc @@ -1388,13 +1388,39 @@ component { lock name="wheelsJoinMemo#application.applicationName#" type="exclusive" timeout="10" { if (!StructKeyExists(local.classAssociations[local.name], "expandedMetadataFilled")) { if (!Len(local.classAssociations[local.name].foreignKey)) { - // cfformat-ignore-start + // The foreign key column lives on a different side depending on the association + // type: for `belongsTo` it is a column on THIS model's table, for `hasMany` / + // `hasOne` it is a column on the ASSOCIATED model's table. Resolve the default + // against whichever side actually owns it so both the legacy `` + // form and the `_` form that `useUnderscoreReferenceColumns` + // makes the migrator emit are honoured (#3337). if (local.classAssociations[local.name].type == "belongsTo") { - local.classAssociations[local.name].foreignKey = local.associatedClass.$classData().modelName & Replace(local.associatedClass.$classData().keys, ",", ",#local.associatedClass.$classData().modelName#", "all"); + local.fkNameSource = local.associatedClass; + local.fkColumnOwner = local.class; } else { - local.classAssociations[local.name].foreignKey = local.class.$classData().modelName & Replace(local.class.$classData().keys, ",", ",#local.class.$classData().modelName#", "all"); + local.fkNameSource = local.class; + local.fkColumnOwner = local.associatedClass; + } + local.classAssociations[local.name].foreignKey = $deriveAssociationForeignKey( + columnOwner = local.fkColumnOwner, + modelName = local.fkNameSource.$classData().modelName, + keys = local.fkNameSource.$classData().keys + ); + // A derived default matching no column on the owning side can only fail later, + // deep inside the join builder, as `key [xxx] doesn't exist` — a message naming + // neither the association nor the `foreignKey=` argument that fixes it. Report it + // here instead, while both candidate shapes are still in hand (#3337). Runs inside + // the memo so the success path costs one check per application lifetime, and only + // for defaults derived here — an explicit `foreignKey=` is the developer's call. + if (application.wheels.showErrorInformation) { + $assertDerivedForeignKeyResolves( + associationName = local.name, + foreignKey = local.classAssociations[local.name].foreignKey, + columnOwner = local.fkColumnOwner, + modelName = local.fkNameSource.$classData().modelName, + keys = local.fkNameSource.$classData().keys + ); } - // cfformat-ignore-end } if (!Len(local.classAssociations[local.name].joinKey)) { if (local.classAssociations[local.name].type == "belongsTo") { @@ -1551,6 +1577,116 @@ component { return local.rv; } + /** + * Internal function. + * Builds the conventional foreign key list for an association default: the model name + * prefixed onto each of the target primary keys, joined by `separator`. + * + * `keys` may be a comma list for composite primary keys, so every element gets the + * prefix — `user` + `a,b` yields `usera,userb`, or `user_a,user_b` with an underscore. + */ + public string function $buildForeignKeyList( + required string modelName, + required string keys, + string separator = "" + ) { + local.rv = ""; + local.keysArray = ListToArray(arguments.keys); + local.iEnd = ArrayLen(local.keysArray); + for (local.i = 1; local.i <= local.iEnd; local.i++) { + local.rv = ListAppend(local.rv, arguments.modelName & arguments.separator & Trim(local.keysArray[local.i])); + } + return local.rv; + } + + /** + * Internal function. + * True when every element of a foreign key list is a property on the supplied class. + * Checks property names rather than column names because that is the lookup the join + * builder performs (`properties[foreignKey].column`). + */ + public boolean function $foreignKeyListResolves(required any columnOwner, required string foreignKey) { + local.properties = arguments.columnOwner.$classData().properties; + local.keysArray = ListToArray(arguments.foreignKey); + local.iEnd = ArrayLen(local.keysArray); + for (local.i = 1; local.i <= local.iEnd; local.i++) { + if (!StructKeyExists(local.properties, Trim(local.keysArray[local.i]))) { + return false; + } + } + return local.iEnd > 0; + } + + /** + * Internal function. + * Derives the default foreign key for an association, preferring whichever conventional + * shape actually exists on the model that owns the column. + * + * Wheels has two conventions in play. The legacy `` form is what this + * function has always produced, and `useUnderscoreReferenceColumns` (the `wheels new` + * default) makes the migrator emit `_` instead — leaving stock new apps + * with a schema the association default could never match (#3337). + * + * Resolving against the real columns rather than reading the setting fixes both + * conventions at once, including apps that flipped the flag mid-life and therefore hold + * a mix of both shapes. It is also strictly error-reducing: the underscore form is only + * consulted when the legacy form is absent, which is a case that throws today. The + * setting is deliberately NOT consulted — it is read per call by the migrator, whereas + * this result is memoized for the application lifetime, so honouring it here would make + * a runtime flip take effect for migrations but not for models. + * + * Falls back to the legacy shape when neither resolves, leaving the existing error path + * (and `$assertDerivedForeignKeyResolves`) to report it. + */ + public string function $deriveAssociationForeignKey( + required any columnOwner, + required string modelName, + required string keys + ) { + local.legacy = $buildForeignKeyList(modelName = arguments.modelName, keys = arguments.keys); + if ($foreignKeyListResolves(columnOwner = arguments.columnOwner, foreignKey = local.legacy)) { + return local.legacy; + } + local.underscored = $buildForeignKeyList(modelName = arguments.modelName, keys = arguments.keys, separator = "_"); + if ($foreignKeyListResolves(columnOwner = arguments.columnOwner, foreignKey = local.underscored)) { + return local.underscored; + } + return local.legacy; + } + + /** + * Internal function. + * Throws a descriptive error when an association's DERIVED default foreign key matches no + * property on the model that owns the column. Without this the failure surfaces much later + * as `key [xxx] doesn't exist` from inside the join builder, which names neither the + * association nor the argument that fixes it (#3337). + * + * Skipped when the owner has no properties at all — an un-migrated or missing table would + * otherwise produce this error instead of the clearer one the query itself raises. + */ + public void function $assertDerivedForeignKeyResolves( + required string associationName, + required string foreignKey, + required any columnOwner, + required string modelName, + required string keys + ) { + if (!StructCount(arguments.columnOwner.$classData().properties)) { + return; + } + if ($foreignKeyListResolves(columnOwner = arguments.columnOwner, foreignKey = arguments.foreignKey)) { + return; + } + local.ownerName = arguments.columnOwner.$classData().modelName; + local.legacy = $buildForeignKeyList(modelName = arguments.modelName, keys = arguments.keys); + local.underscored = $buildForeignKeyList(modelName = arguments.modelName, keys = arguments.keys, separator = "_"); + Throw( + type = "Wheels.AssociationForeignKeyNotFound", + message = "The `#arguments.associationName#` association derives a default foreign key of `#arguments.foreignKey#`, which is not a property on the `#local.ownerName#` model.", + extendedInfo = "Wheels looks for the conventional `#local.legacy#` and, for schemas built with `useUnderscoreReferenceColumns` enabled, `#local.underscored#`. Neither exists on `#local.ownerName#`. Either pass `foreignKey=""""` explicitly when setting up the `#arguments.associationName#` association, or rename the column on `#local.ownerName#` to one of those two forms." + ); + } + /** * Internal function. */ diff --git a/vendor/wheels/tests/_assets/models/RefChild.cfc b/vendor/wheels/tests/_assets/models/RefChild.cfc new file mode 100644 index 0000000000..af3eb9211f --- /dev/null +++ b/vendor/wheels/tests/_assets/models/RefChild.cfc @@ -0,0 +1,16 @@ +/** + * Fixture for #3337: the child side of an association whose foreign key column uses the + * `_id` convention that `useUnderscoreReferenceColumns` makes the migrator emit. + * + * The association passes no `foreignKey`, so the default derivation has to resolve + * `refparent_id` on this model. Before #3337 it derived `refparentid` unconditionally and + * any `include=` threw `key [refparentid] doesn't exist`. + */ +component extends="Model" { + + function config() { + table("c_o_r_e_refchildren"); + belongsTo("refParent"); + } + +} diff --git a/vendor/wheels/tests/_assets/models/RefParent.cfc b/vendor/wheels/tests/_assets/models/RefParent.cfc new file mode 100644 index 0000000000..a94058ba78 --- /dev/null +++ b/vendor/wheels/tests/_assets/models/RefParent.cfc @@ -0,0 +1,15 @@ +/** + * Fixture for #3337: the parent side of an association whose foreign key column uses the + * `_id` convention that `useUnderscoreReferenceColumns` makes the migrator emit. + * + * Exercises the hasMany branch of the association foreign-key default, where the column + * lives on the ASSOCIATED model (`c_o_r_e_refchildren.refparent_id`). + */ +component extends="Model" { + + function config() { + table("c_o_r_e_refparents"); + hasMany("refChildren"); + } + +} diff --git a/vendor/wheels/tests/populate.cfm b/vendor/wheels/tests/populate.cfm index 5193355665..2f0a5e9692 100644 --- a/vendor/wheels/tests/populate.cfm +++ b/vendor/wheels/tests/populate.cfm @@ -96,7 +96,7 @@ - + + +CREATE TABLE c_o_r_e_refparents +( + id #local.identityColumnType# + ,name varchar(50) + ,PRIMARY KEY(id) +) #local.storageEngine# + + + +CREATE TABLE c_o_r_e_refchildren +( + id #local.identityColumnType# + ,refparent_id #local.intColumnType# + ,PRIMARY KEY(id) +) #local.storageEngine# + + CREATE TABLE c_o_r_e_combikeys ( diff --git a/vendor/wheels/tests/specs/model/AssociationForeignKeyConventionSpec.cfc b/vendor/wheels/tests/specs/model/AssociationForeignKeyConventionSpec.cfc new file mode 100644 index 0000000000..dc5184e81d --- /dev/null +++ b/vendor/wheels/tests/specs/model/AssociationForeignKeyConventionSpec.cfc @@ -0,0 +1,141 @@ +/** + * Regression coverage for #3337. + * + * `useUnderscoreReferenceColumns` (framework default `false`, `wheels new` template default + * `true`) makes the migrator emit `_id` columns, but the association foreign-key + * default was unconditional `` concatenation. A stock new app therefore had + * a migrator and a model layer that could never agree, and any `include=` threw + * `key [id] doesn't exist` from deep inside the join builder. + * + * The default now resolves against the columns that actually exist on whichever side owns + * the foreign key, so both conventions work — including apps holding a mix of the two. + * Deliberately schema-driven rather than reading the setting: the migrator reads the flag + * per call, while this result is memoized for the application lifetime, so honouring the + * flag here would let a runtime flip change migrations without changing models. + * + * Fixtures: `RefParent` / `RefChild` in `tests/_assets/models`, tables in `tests/populate.cfm`. + */ +component extends="wheels.WheelsTest" { + + function run() { + g = application.wo; + + describe("association foreign key default — underscore convention (##3337)", () => { + + it("resolves the underscore form for belongsTo, where the column is on this model", () => { + // RefChild.refparent_id — legacy `refparentid` does not exist + var assoc = g.model("refChild").$expandedAssociations(include = "refParent")[1]; + + expect(assoc.foreignKey).toBe("refparent_id"); + }); + + it("resolves the underscore form for hasMany, where the column is on the associated model", () => { + // the same column, reached from the parent side + var assoc = g.model("refParent").$expandedAssociations(include = "refChildren")[1]; + + expect(assoc.foreignKey).toBe("refparent_id"); + }); + + it("still derives the legacy form when that is what the schema uses", () => { + // c_o_r_e_posts.authorid — the framework's own fixtures are all legacy-shaped, + // so this pins that the underscore support did not shift existing behaviour + var assoc = g.model("post").$expandedAssociations(include = "author")[1]; + + expect(assoc.foreignKey).toBe("authorid"); + }); + + it("traverses the association without throwing — the reported symptom", () => { + // Pre-fix this threw `key [refparentid] doesn't exist`. Empty tables are fine; + // the point is that building and running the join succeeds. + var state = {threw = false, message = ""}; + try { + g.model("refChild").findAll(include = "refParent"); + } catch (any e) { + state.threw = true; + state.message = e.message; + } + + expect(state.threw).toBeFalse(); + expect(state.message).toBe(""); + }); + + it("builds both candidate shapes for composite keys", () => { + var m = g.model("refChild"); + + expect(m.$buildForeignKeyList(modelName = "user", keys = "id")).toBe("userid"); + expect(m.$buildForeignKeyList(modelName = "user", keys = "id", separator = "_")).toBe("user_id"); + expect(m.$buildForeignKeyList(modelName = "user", keys = "a,b")).toBe("usera,userb"); + expect(m.$buildForeignKeyList(modelName = "user", keys = "a,b", separator = "_")).toBe("user_a,user_b"); + }); + + }); + + describe("unresolvable derived foreign key is reported at the association (##3337)", () => { + + it("throws Wheels.AssociationForeignKeyNotFound naming the association and both shapes", () => { + var m = g.model("refChild"); + var state = {type = "", message = "", extended = ""}; + try { + m.$assertDerivedForeignKeyResolves( + associationName = "someAssociation", + foreignKey = "nosuchmodelid", + columnOwner = g.model("refChild"), + modelName = "nosuchmodel", + keys = "id" + ); + } catch (any e) { + state.type = e.type; + state.message = e.message; + state.extended = e.extendedInfo; + } + + expect(state.type).toBe("Wheels.AssociationForeignKeyNotFound"); + expect(state.message).toInclude("someAssociation"); + expect(state.message).toInclude("nosuchmodelid"); + // the extended info has to name the escape hatch, which the old + // `key [xxx] doesn't exist` message never did + expect(state.extended).toInclude("foreignKey"); + expect(state.extended).toInclude("nosuchmodel_id"); + }); + + it("stays silent when the derived key does resolve", () => { + var m = g.model("refChild"); + var state = {threw = false}; + try { + m.$assertDerivedForeignKeyResolves( + associationName = "refParent", + foreignKey = "refparent_id", + columnOwner = g.model("refChild"), + modelName = "refparent", + keys = "id" + ); + } catch (any e) { + state.threw = true; + } + + expect(state.threw).toBeFalse(); + }); + + it("stays silent when the owner has no properties at all", () => { + // an un-migrated or missing table must surface the query's own error, not this one + var m = g.model("refChild"); + var state = {threw = false}; + try { + m.$assertDerivedForeignKeyResolves( + associationName = "whatever", + foreignKey = "anythingid", + columnOwner = g.model("userTableless"), + modelName = "anything", + keys = "id" + ); + } catch (any e) { + state.threw = true; + } + + expect(state.threw).toBeFalse(); + }); + + }); + } + +}