diff --git a/changelog.d/dependent-callbacks.fixed.md b/changelog.d/dependent-callbacks.fixed.md new file mode 100644 index 0000000000..af54914548 --- /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 `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 fc9405f12d..0ced8304c6 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,13 +246,37 @@ 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). When the object itself * was soft-deleted (`parentSoftDeleted`), nothing a restore would need is destroyed or unlinked: - * dependents that can be soft-deleted are, and every other dependent is left alone. + * dependents that can be soft-deleted are, and every other dependent is left alone. With + * `callbacks = false` the instantiated dependents skip their callbacks too. */ - public void function $deleteDependents(boolean softDelete = true, boolean includeSoftDeletes = false, boolean parentSoftDeleted = false) { + public void function $deleteDependents( + boolean softDelete = true, + boolean includeSoftDeletes = false, + boolean callbacks = true, + boolean parentSoftDeleted = false + ) { for (local.key in variables.wheels.class.associations) { local.association = variables.wheels.class.associations[local.key]; if ( @@ -262,6 +292,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); @@ -269,16 +300,21 @@ 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": + // 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 6c4b04c307..95222b82e6 100644 --- a/vendor/wheels/model/delete.cfm +++ b/vendor/wheels/model/delete.cfm @@ -41,6 +41,7 @@ if (arguments.instantiate) { local.rv = 0; local.objects = findAll( + callbacks = arguments.callbacks, include = arguments.include, includeSoftDeletes = arguments.includeSoftDeletes, parameterize = arguments.parameterize, @@ -157,6 +158,7 @@ arguments.includeSoftDeletes = true; } local.object = findOne( + callbacks = arguments.callbacks, includeSoftDeletes = arguments.includeSoftDeletes, order = arguments.order, reload = arguments.reload, @@ -264,12 +266,21 @@ local.recordCount = $runDeleteStatement(sql = arguments.sql, parameterize = arguments.parameterize); if (local.recordCount == 1) { $markSoftDeleted(arguments.$softDeletedAt); - $deleteDependents(softDelete = true, includeSoftDeletes = arguments.includeSoftDeletes, parentSoftDeleted = true); + $deleteDependents( + softDelete = true, + includeSoftDeletes = arguments.includeSoftDeletes, + callbacks = arguments.callbacks, + parentSoftDeleted = true + ); } } else { // Delete dependent record(s) before the record itself so foreign key constraints don't // prevent the deletion. - $deleteDependents(softDelete = arguments.softDelete, includeSoftDeletes = arguments.includeSoftDeletes); + $deleteDependents( + softDelete = arguments.softDelete, + includeSoftDeletes = arguments.includeSoftDeletes, + callbacks = arguments.callbacks + ); local.recordCount = $runDeleteStatement(sql = arguments.sql, parameterize = arguments.parameterize); } if (local.recordCount == 1 && $callback("afterDelete", arguments.callbacks)) { 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/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/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/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..f6163e3aa5 --- /dev/null +++ b/vendor/wheels/tests/_assets/models/DepPost.cfc @@ -0,0 +1,41 @@ +/** + * 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"); + afterFind("logFind"); + afterRollback("logRollback"); + } + + function logFind() { + $logDependentCallback("find"); + return true; + } + + function logRollback() { + $logDependentCallback("rollback"); + return true; + } + + 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..7b6dc1a4b6 --- /dev/null +++ b/vendor/wheels/tests/specs/model/dependentCallbacksSpec.cfc @@ -0,0 +1,106 @@ +/** + * 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. + */ +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); + 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); + 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"); + }); + + }); + + 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(""); + }); + + }); + + } + +}