From 00eb03fa3b7b8c67d3ed27d952e64f186cb6ffac Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Mon, 5 Oct 2026 10:02:10 -0700 Subject: [PATCH 1/2] fix(model): dependent deletes follow callbacks = false; check dependent when declared delete(callbacks = false) passed nothing on to the dependent records it deleted or removed, so their callbacks still ran. An invalid dependent value was accepted by hasMany() / hasOne() and only threw at the first delete; it now throws Wheels.InvalidArgument when the association is declared. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Peter Amiri --- changelog.d/dependent-callbacks.fixed.md | 1 + vendor/wheels/model/associations.cfm | 31 +++++++- vendor/wheels/model/delete.cfm | 3 +- .../tests/_assets/models/DepAuthorDelete.cfc | 8 ++ .../tests/_assets/models/DepAuthorInvalid.cfc | 8 ++ .../tests/_assets/models/DepAuthorRemove.cfc | 8 ++ .../wheels/tests/_assets/models/DepPost.cfc | 29 +++++++ .../specs/model/dependentCallbacksSpec.cfc | 78 +++++++++++++++++++ 8 files changed, 163 insertions(+), 3 deletions(-) create mode 100644 changelog.d/dependent-callbacks.fixed.md create mode 100644 vendor/wheels/tests/_assets/models/DepAuthorDelete.cfc create mode 100644 vendor/wheels/tests/_assets/models/DepAuthorInvalid.cfc create mode 100644 vendor/wheels/tests/_assets/models/DepAuthorRemove.cfc create mode 100644 vendor/wheels/tests/_assets/models/DepPost.cfc create mode 100644 vendor/wheels/tests/specs/model/dependentCallbacksSpec.cfc diff --git a/changelog.d/dependent-callbacks.fixed.md b/changelog.d/dependent-callbacks.fixed.md new file mode 100644 index 0000000000..59c19ce8e5 --- /dev/null +++ b/changelog.d/dependent-callbacks.fixed.md @@ -0,0 +1 @@ +- `delete(callbacks=false)` now also skips the callbacks of the dependent records it deletes or removes through `dependent="delete"` or `dependent="remove"`. An invalid `dependent` value on `hasMany()` or `hasOne()` now throws `Wheels.InvalidArgument` when the association is declared, instead of at the first delete diff --git a/vendor/wheels/model/associations.cfm b/vendor/wheels/model/associations.cfm index 54b9702efd..8d2572d51b 100644 --- a/vendor/wheels/model/associations.cfm +++ b/vendor/wheels/model/associations.cfm @@ -71,6 +71,9 @@ string as = "" ) { $args(name = "hasMany", args = arguments); + if (StructKeyExists(arguments, "dependent")) { + $validateDependent(arguments.dependent); + } local.singularizedName = capitalize(singularize(arguments.name)); local.capitalizedName = capitalize(arguments.name); arguments.type = "hasMany"; @@ -125,6 +128,9 @@ string as = "" ) { $args(name = "hasOne", args = arguments); + if (StructKeyExists(arguments, "dependent")) { + $validateDependent(arguments.dependent); + } local.capitalizedName = capitalize(arguments.name); arguments.type = "hasOne"; @@ -240,11 +246,30 @@ variables.wheels.class.associations[local.associationName] = arguments; } + /** + * Internal function. Throws Wheels.InvalidArgument for a `dependent` value hasMany() and hasOne() + * don't support, so the mistake surfaces when the association is declared, not at the first delete. + */ + public void function $validateDependent(required any dependent) { + if (IsBoolean(arguments.dependent) && !arguments.dependent) { + return; + } + if (IsSimpleValue(arguments.dependent) && ListFindNoCase("delete,deleteAll,remove,removeAll", arguments.dependent)) { + return; + } + Throw( + type = "Wheels.InvalidArgument", + message = "'#IsSimpleValue(arguments.dependent) ? arguments.dependent : "(complex value)"#' is not a valid dependency.", + extendedInfo = "Use `delete`, `deleteAll`, `remove`, `removeAll` or `false`." + ); + } + /* * Called when a model object is deleted (e.g. post.delete()). - * Deletes all associated records (or sets their foreign key values to NULL). + * Deletes all associated records (or sets their foreign key values to NULL). With + * `callbacks = false` the instantiated dependents skip their callbacks too. */ - public void function $deleteDependents(boolean softDelete = true, boolean includeSoftDeletes = false) { + public void function $deleteDependents(boolean softDelete = true, boolean includeSoftDeletes = false, boolean callbacks = true) { for (local.key in variables.wheels.class.associations) { local.association = variables.wheels.class.associations[local.key]; if (ListFindNoCase("hasMany,hasOne", local.association.type) && local.association.dependent != false) { @@ -256,6 +281,7 @@ case "delete": local.invokeArgs = {}; local.invokeArgs.instantiate = true; + local.invokeArgs.callbacks = arguments.callbacks; local.invokeArgs.softDelete = arguments.softDelete; local.invokeArgs.includeSoftDeletes = arguments.includeSoftDeletes; $invoke(componentReference = this, method = "delete#local.all##local.key#", invokeArgs = local.invokeArgs); @@ -263,6 +289,7 @@ case "remove": local.invokeArgs = {}; local.invokeArgs.instantiate = true; + local.invokeArgs.callbacks = arguments.callbacks; $invoke(componentReference = this, method = "remove#local.all##local.key#", invokeArgs = local.invokeArgs); break; case "deleteAll": diff --git a/vendor/wheels/model/delete.cfm b/vendor/wheels/model/delete.cfm index b7557a11f8..f4232c64d5 100644 --- a/vendor/wheels/model/delete.cfm +++ b/vendor/wheels/model/delete.cfm @@ -213,7 +213,8 @@ // Done before the main record is deleted to make sure eventual foreign key constraints does not prevent deletion. $deleteDependents( softDelete = arguments.softDelete, - includeSoftDeletes = arguments.includeSoftDeletes + includeSoftDeletes = arguments.includeSoftDeletes, + callbacks = arguments.callbacks ); local.deleted = variables.wheels.class.adapter.$querySetup( diff --git a/vendor/wheels/tests/_assets/models/DepAuthorDelete.cfc b/vendor/wheels/tests/_assets/models/DepAuthorDelete.cfc new file mode 100644 index 0000000000..72f81137f2 --- /dev/null +++ b/vendor/wheels/tests/_assets/models/DepAuthorDelete.cfc @@ -0,0 +1,8 @@ +component extends="Model" { + + function config() { + table("c_o_r_e_authors"); + hasMany(name = "depPosts", modelName = "depPost", foreignKey = "authorId", dependent = "delete"); + } + +} diff --git a/vendor/wheels/tests/_assets/models/DepAuthorInvalid.cfc b/vendor/wheels/tests/_assets/models/DepAuthorInvalid.cfc new file mode 100644 index 0000000000..750dd21bc5 --- /dev/null +++ b/vendor/wheels/tests/_assets/models/DepAuthorInvalid.cfc @@ -0,0 +1,8 @@ +component extends="Model" { + + function config() { + table("c_o_r_e_authors"); + hasMany(name = "depPosts", modelName = "depPost", foreignKey = "authorId", dependent = "destroy"); + } + +} diff --git a/vendor/wheels/tests/_assets/models/DepAuthorRemove.cfc b/vendor/wheels/tests/_assets/models/DepAuthorRemove.cfc new file mode 100644 index 0000000000..69ca2f97ff --- /dev/null +++ b/vendor/wheels/tests/_assets/models/DepAuthorRemove.cfc @@ -0,0 +1,8 @@ +component extends="Model" { + + function config() { + table("c_o_r_e_authors"); + hasMany(name = "depPosts", modelName = "depPost", foreignKey = "authorId", dependent = "remove"); + } + +} diff --git a/vendor/wheels/tests/_assets/models/DepPost.cfc b/vendor/wheels/tests/_assets/models/DepPost.cfc new file mode 100644 index 0000000000..227822def0 --- /dev/null +++ b/vendor/wheels/tests/_assets/models/DepPost.cfc @@ -0,0 +1,29 @@ +/** + * A post whose delete and save callbacks log to request.$depLog, for dependentCallbacksSpec. + */ +component extends="Model" { + + function config() { + table("c_o_r_e_posts"); + beforeDelete("logDelete"); + beforeSave("logSave"); + } + + function logDelete() { + $logDependentCallback("delete"); + return true; + } + + function logSave() { + $logDependentCallback("save"); + return true; + } + + function $logDependentCallback(required string event) { + if (!StructKeyExists(request, "$depLog")) { + request.$depLog = []; + } + ArrayAppend(request.$depLog, arguments.event); + } + +} diff --git a/vendor/wheels/tests/specs/model/dependentCallbacksSpec.cfc b/vendor/wheels/tests/specs/model/dependentCallbacksSpec.cfc new file mode 100644 index 0000000000..e7c2ac175b --- /dev/null +++ b/vendor/wheels/tests/specs/model/dependentCallbacksSpec.cfc @@ -0,0 +1,78 @@ +/** + * delete(callbacks = false) covers the dependent deletes and removes it triggers, and an invalid + * `dependent` value fails when the association is declared rather than at the first delete. Each + * delete runs with transaction = "rollback", so the seeded rows are kept. + */ +component extends="wheels.WheelsTest" { + + function beforeAll() { + variables.g = application.wo; + variables.authorId = variables.g.model("post").findOne(order = "id").authorId; + } + + function failureOf(required any callback) { + var state = {type = "", message = ""}; + var target = arguments.callback; + try { + target(); + } catch (any e) { + state.type = e.type; + state.message = e.message; + } + return state; + } + + function run() { + + describe("Dependent records and callbacks = false", () => { + + beforeEach(() => { + request.$depLog = []; + }); + + it("deletes dependents without their callbacks when callbacks = false", () => { + var author = variables.g.model("depAuthorDelete").findByKey(variables.authorId); + author.delete(callbacks = false, transaction = "rollback"); + expect(ArrayLen(request.$depLog)).toBe(0); + }); + + it("removes dependents without their callbacks when callbacks = false", () => { + var author = variables.g.model("depAuthorRemove").findByKey(variables.authorId); + author.delete(callbacks = false, transaction = "rollback"); + expect(ArrayLen(request.$depLog)).toBe(0); + }); + + it("still runs the dependents' callbacks with callbacks = true", () => { + var deleting = variables.g.model("depAuthorDelete").findByKey(variables.authorId); + deleting.delete(transaction = "rollback"); + expect(ArrayToList(request.$depLog)).toInclude("delete"); + request.$depLog = []; + var removing = variables.g.model("depAuthorRemove").findByKey(variables.authorId); + removing.delete(transaction = "rollback"); + expect(ArrayToList(request.$depLog)).toInclude("save"); + }); + + }); + + describe("An invalid dependent value", () => { + + it("fails when the association is declared", () => { + var failure = failureOf(() => variables.g.model("depAuthorInvalid")); + expect(failure.type).toBe("Wheels.InvalidArgument"); + expect(failure.message).toInclude("destroy"); + }); + + it("accepts every documented value", () => { + var post = variables.g.model("post"); + expect(failureOf(() => post.$validateDependent("delete")).type).toBe(""); + expect(failureOf(() => post.$validateDependent("deleteAll")).type).toBe(""); + expect(failureOf(() => post.$validateDependent("remove")).type).toBe(""); + expect(failureOf(() => post.$validateDependent("removeAll")).type).toBe(""); + expect(failureOf(() => post.$validateDependent(false)).type).toBe(""); + }); + + }); + + } + +} From ab06ed73e3a7f462af18aeb7bd0251ad445ae90a Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Mon, 5 Oct 2026 12:47:23 -0700 Subject: [PATCH 2/2] fix(model): callbacks = false reaches the records dependent deletes load deleteAll(instantiate = true), deleteOne() and updateOne() loaded their records without passing callbacks on, so afterFind / afterInitialization still ran with callbacks = false. A hasOne with dependent = "deleteAll" / "removeAll" loads its record through deleteOne() / updateOne() and got no callbacks argument at all. Both now follow the parent's callbacks; specs cover hasOne for every dependent value and the finder and transaction callbacks. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Peter Amiri --- changelog.d/dependent-callbacks.fixed.md | 2 +- vendor/wheels/model/associations.cfm | 6 +++- vendor/wheels/model/delete.cfm | 2 ++ vendor/wheels/model/update.cfm | 1 + .../_assets/models/DepAuthorOneDelete.cfc | 8 +++++ .../_assets/models/DepAuthorOneDeleteAll.cfc | 8 +++++ .../_assets/models/DepAuthorOneRemove.cfc | 8 +++++ .../_assets/models/DepAuthorOneRemoveAll.cfc | 8 +++++ .../wheels/tests/_assets/models/DepPost.cfc | 12 +++++++ .../specs/model/dependentCallbacksSpec.cfc | 34 +++++++++++++++++-- 10 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 vendor/wheels/tests/_assets/models/DepAuthorOneDelete.cfc create mode 100644 vendor/wheels/tests/_assets/models/DepAuthorOneDeleteAll.cfc create mode 100644 vendor/wheels/tests/_assets/models/DepAuthorOneRemove.cfc create mode 100644 vendor/wheels/tests/_assets/models/DepAuthorOneRemoveAll.cfc diff --git a/changelog.d/dependent-callbacks.fixed.md b/changelog.d/dependent-callbacks.fixed.md index 59c19ce8e5..af54914548 100644 --- a/changelog.d/dependent-callbacks.fixed.md +++ b/changelog.d/dependent-callbacks.fixed.md @@ -1 +1 @@ -- `delete(callbacks=false)` now also skips the callbacks of the dependent records it deletes or removes through `dependent="delete"` or `dependent="remove"`. An invalid `dependent` value on `hasMany()` or `hasOne()` now throws `Wheels.InvalidArgument` when the association is declared, instead of at the first delete +- `delete(callbacks=false)` now also skips the callbacks of the dependent records it deletes or removes through `hasMany()` / `hasOne()` `dependent=`, including their `afterFind` / `afterInitialization` and transaction callbacks; a `hasOne` with `dependent="deleteAll"` or `"removeAll"` (which loads the record) is covered too. `deleteAll(instantiate=true)`, `deleteOne()` and `updateOne()` with `callbacks=false` no longer run `afterFind` / `afterInitialization` on the records they load, like `updateAll(instantiate=true)`. An invalid `dependent` value on `hasMany()` or `hasOne()` now throws `Wheels.InvalidArgument` when the association is declared, instead of at the first delete diff --git a/vendor/wheels/model/associations.cfm b/vendor/wheels/model/associations.cfm index 8d2572d51b..f260e93259 100644 --- a/vendor/wheels/model/associations.cfm +++ b/vendor/wheels/model/associations.cfm @@ -293,13 +293,17 @@ $invoke(componentReference = this, method = "remove#local.all##local.key#", invokeArgs = local.invokeArgs); break; case "deleteAll": + // a hasOne's deleteAll still instantiates (deleteOne), so it needs callbacks too local.invokeArgs = {}; + local.invokeArgs.callbacks = arguments.callbacks; local.invokeArgs.softDelete = arguments.softDelete; local.invokeArgs.includeSoftDeletes = arguments.includeSoftDeletes; $invoke(componentReference = this, method = "delete#local.all##local.key#", invokeArgs = local.invokeArgs); break; case "removeAll": - $invoke(componentReference = this, method = "remove#local.all##local.key#"); + local.invokeArgs = {}; + local.invokeArgs.callbacks = arguments.callbacks; + $invoke(componentReference = this, method = "remove#local.all##local.key#", invokeArgs = local.invokeArgs); break; default: Throw( diff --git a/vendor/wheels/model/delete.cfm b/vendor/wheels/model/delete.cfm index a3d61be371..7fc1142828 100644 --- a/vendor/wheels/model/delete.cfm +++ b/vendor/wheels/model/delete.cfm @@ -37,6 +37,7 @@ if (arguments.instantiate) { local.rv = 0; local.objects = findAll( + callbacks = arguments.callbacks, include = arguments.include, includeSoftDeletes = arguments.includeSoftDeletes, parameterize = arguments.parameterize, @@ -145,6 +146,7 @@ ) { $args(name = "deleteOne", args = arguments); local.object = findOne( + callbacks = arguments.callbacks, includeSoftDeletes = arguments.includeSoftDeletes, order = arguments.order, reload = arguments.reload, diff --git a/vendor/wheels/model/update.cfm b/vendor/wheels/model/update.cfm index 0736ae68c9..38563d6d94 100644 --- a/vendor/wheels/model/update.cfm +++ b/vendor/wheels/model/update.cfm @@ -204,6 +204,7 @@ ) { $args(name = "updateOne", args = arguments); local.object = findOne( + callbacks = arguments.callbacks, includeSoftDeletes = arguments.includeSoftDeletes, order = arguments.order, reload = arguments.reload, diff --git a/vendor/wheels/tests/_assets/models/DepAuthorOneDelete.cfc b/vendor/wheels/tests/_assets/models/DepAuthorOneDelete.cfc new file mode 100644 index 0000000000..587082b95b --- /dev/null +++ b/vendor/wheels/tests/_assets/models/DepAuthorOneDelete.cfc @@ -0,0 +1,8 @@ +component extends="Model" { + + function config() { + table("c_o_r_e_authors"); + hasOne(name = "depPost", modelName = "depPost", foreignKey = "authorId", dependent = "delete"); + } + +} diff --git a/vendor/wheels/tests/_assets/models/DepAuthorOneDeleteAll.cfc b/vendor/wheels/tests/_assets/models/DepAuthorOneDeleteAll.cfc new file mode 100644 index 0000000000..3523e40637 --- /dev/null +++ b/vendor/wheels/tests/_assets/models/DepAuthorOneDeleteAll.cfc @@ -0,0 +1,8 @@ +component extends="Model" { + + function config() { + table("c_o_r_e_authors"); + hasOne(name = "depPost", modelName = "depPost", foreignKey = "authorId", dependent = "deleteAll"); + } + +} diff --git a/vendor/wheels/tests/_assets/models/DepAuthorOneRemove.cfc b/vendor/wheels/tests/_assets/models/DepAuthorOneRemove.cfc new file mode 100644 index 0000000000..5c67352758 --- /dev/null +++ b/vendor/wheels/tests/_assets/models/DepAuthorOneRemove.cfc @@ -0,0 +1,8 @@ +component extends="Model" { + + function config() { + table("c_o_r_e_authors"); + hasOne(name = "depPost", modelName = "depPost", foreignKey = "authorId", dependent = "remove"); + } + +} diff --git a/vendor/wheels/tests/_assets/models/DepAuthorOneRemoveAll.cfc b/vendor/wheels/tests/_assets/models/DepAuthorOneRemoveAll.cfc new file mode 100644 index 0000000000..19f4e06a2c --- /dev/null +++ b/vendor/wheels/tests/_assets/models/DepAuthorOneRemoveAll.cfc @@ -0,0 +1,8 @@ +component extends="Model" { + + function config() { + table("c_o_r_e_authors"); + hasOne(name = "depPost", modelName = "depPost", foreignKey = "authorId", dependent = "removeAll"); + } + +} diff --git a/vendor/wheels/tests/_assets/models/DepPost.cfc b/vendor/wheels/tests/_assets/models/DepPost.cfc index 227822def0..f6163e3aa5 100644 --- a/vendor/wheels/tests/_assets/models/DepPost.cfc +++ b/vendor/wheels/tests/_assets/models/DepPost.cfc @@ -7,6 +7,18 @@ component extends="Model" { table("c_o_r_e_posts"); beforeDelete("logDelete"); beforeSave("logSave"); + afterFind("logFind"); + afterRollback("logRollback"); + } + + function logFind() { + $logDependentCallback("find"); + return true; + } + + function logRollback() { + $logDependentCallback("rollback"); + return true; } function logDelete() { diff --git a/vendor/wheels/tests/specs/model/dependentCallbacksSpec.cfc b/vendor/wheels/tests/specs/model/dependentCallbacksSpec.cfc index e7c2ac175b..7b6dc1a4b6 100644 --- a/vendor/wheels/tests/specs/model/dependentCallbacksSpec.cfc +++ b/vendor/wheels/tests/specs/model/dependentCallbacksSpec.cfc @@ -1,5 +1,6 @@ /** - * delete(callbacks = false) covers the dependent deletes and removes it triggers, and an invalid + * delete(callbacks = false) covers the dependent deletes and removes it triggers (the children's + * find, write and transaction callbacks, for hasMany and hasOne), and an invalid * `dependent` value fails when the association is declared rather than at the first delete. Each * delete runs with transaction = "rollback", so the seeded rows are kept. */ @@ -32,24 +33,51 @@ component extends="wheels.WheelsTest" { it("deletes dependents without their callbacks when callbacks = false", () => { var author = variables.g.model("depAuthorDelete").findByKey(variables.authorId); + request.$depLog = []; author.delete(callbacks = false, transaction = "rollback"); expect(ArrayLen(request.$depLog)).toBe(0); }); it("removes dependents without their callbacks when callbacks = false", () => { var author = variables.g.model("depAuthorRemove").findByKey(variables.authorId); + request.$depLog = []; author.delete(callbacks = false, transaction = "rollback"); expect(ArrayLen(request.$depLog)).toBe(0); }); + it("skips a hasOne dependent's callbacks with callbacks = false, for every dependent value", () => { + var parents = ["depAuthorOneDelete", "depAuthorOneDeleteAll", "depAuthorOneRemove", "depAuthorOneRemoveAll"]; + var logged = {}; + for (var parent in parents) { + var author = variables.g.model(parent).findByKey(variables.authorId); + request.$depLog = []; + author.delete(callbacks = false, transaction = "rollback"); + logged[parent] = ArrayToList(request.$depLog); + } + for (var parent in parents) { + expect(logged[parent]).toBe("", parent); + } + }); + it("still runs the dependents' callbacks with callbacks = true", () => { var deleting = variables.g.model("depAuthorDelete").findByKey(variables.authorId); - deleting.delete(transaction = "rollback"); - expect(ArrayToList(request.$depLog)).toInclude("delete"); request.$depLog = []; + deleting.delete(transaction = "rollback"); + var deleted = ArrayToList(request.$depLog); + expect(deleted).toInclude("find"); + expect(deleted).toInclude("delete"); + expect(deleted).toInclude("rollback"); var removing = variables.g.model("depAuthorRemove").findByKey(variables.authorId); + request.$depLog = []; removing.delete(transaction = "rollback"); expect(ArrayToList(request.$depLog)).toInclude("save"); + var one = variables.g.model("depAuthorOneDeleteAll").findByKey(variables.authorId); + request.$depLog = []; + one.delete(transaction = "rollback"); + var oneLogged = ArrayToList(request.$depLog); + expect(oneLogged).toInclude("find"); + expect(oneLogged).toInclude("delete"); + expect(oneLogged).toInclude("rollback"); }); });