diff --git a/changelog.d/strict-arguments.added.md b/changelog.d/strict-arguments.added.md new file mode 100644 index 0000000000..36bce460f2 --- /dev/null +++ b/changelog.d/strict-arguments.added.md @@ -0,0 +1 @@ +- New `strictArguments` setting reports an argument that a framework declaration doesn't know, instead of ignoring it: a typo or another framework's name such as `foreign_key`, `orderby` or a migration column's `nullable`. It checks associations, validations, callback registration, `property()`, `table()`, the finders (including dynamic finders), controller `caches()` / `protectsFromForgery()` / `provides()` / `usesLayout()` and migration column functions. `"warn"` (the development default) logs one `wheels.log` line per model or controller, function and argument, with a suggested name; `"throw"` raises `Wheels.UnknownArgument`; `"off"` (the default elsewhere) keeps the old behaviour diff --git a/cli/lucli/templates/app/.ai/migrations.md b/cli/lucli/templates/app/.ai/migrations.md index 65c7a92af2..4864146608 100644 --- a/cli/lucli/templates/app/.ai/migrations.md +++ b/cli/lucli/templates/app/.ai/migrations.md @@ -2,6 +2,8 @@ Part of the Wheels application guide; start with `../CLAUDE.md`. +Column options are `allowNull`, `default`, `limit`, `precision`, `scale` (`null` is the deprecated pre-3.0 name for `allowNull`; write `allowNull`); with `strictArguments` on (development default), an unknown option such as `nullable=true` is logged instead of ignored. + ## Shared Dev DB Reconciliation `wheels_migrator_versions` can drift from on-disk files when several developers share a single dev database (peer applied a migration whose file isn't yet in your branch). Detected and surfaced automatically; reconciliation is explicit: diff --git a/cli/lucli/templates/app/.ai/models.md b/cli/lucli/templates/app/.ai/models.md index 58378e7e59..c34c697921 100644 --- a/cli/lucli/templates/app/.ai/models.md +++ b/cli/lucli/templates/app/.ai/models.md @@ -2,6 +2,8 @@ Part of the Wheels application guide; start with `../CLAUDE.md`. +In development, `strictArguments` (default `"warn"`) logs any argument a declaration or finder doesn't know (`foreign_key`, `orderby`) to `wheels.log`; `set(strictArguments="throw")` makes it an error. A logged argument was ignored: fix the call. + ```cfm component extends="Model" { function config() { diff --git a/docs/consumer-ai/.ai/migrations.md b/docs/consumer-ai/.ai/migrations.md index 65c7a92af2..4864146608 100644 --- a/docs/consumer-ai/.ai/migrations.md +++ b/docs/consumer-ai/.ai/migrations.md @@ -2,6 +2,8 @@ Part of the Wheels application guide; start with `../CLAUDE.md`. +Column options are `allowNull`, `default`, `limit`, `precision`, `scale` (`null` is the deprecated pre-3.0 name for `allowNull`; write `allowNull`); with `strictArguments` on (development default), an unknown option such as `nullable=true` is logged instead of ignored. + ## Shared Dev DB Reconciliation `wheels_migrator_versions` can drift from on-disk files when several developers share a single dev database (peer applied a migration whose file isn't yet in your branch). Detected and surfaced automatically; reconciliation is explicit: diff --git a/docs/consumer-ai/.ai/models.md b/docs/consumer-ai/.ai/models.md index 58378e7e59..c34c697921 100644 --- a/docs/consumer-ai/.ai/models.md +++ b/docs/consumer-ai/.ai/models.md @@ -2,6 +2,8 @@ Part of the Wheels application guide; start with `../CLAUDE.md`. +In development, `strictArguments` (default `"warn"`) logs any argument a declaration or finder doesn't know (`foreign_key`, `orderby`) to `wheels.log`; `set(strictArguments="throw")` makes it an error. A logged argument was ignored: fix the call. + ```cfm component extends="Model" { function config() { diff --git a/vendor/wheels/controller/layouts.cfc b/vendor/wheels/controller/layouts.cfc index 0bf4941bcf..7d3c887427 100644 --- a/vendor/wheels/controller/layouts.cfc +++ b/vendor/wheels/controller/layouts.cfc @@ -18,6 +18,7 @@ component { string only, boolean useDefault = true ) { + $checkArguments(args = arguments, name = "usesLayout"); // Exit the function if we're on an internal Wheels page (we don't want to allow overriding the layout there). if (variables.$class.name == "Wheels") { return; diff --git a/vendor/wheels/controller/provides.cfc b/vendor/wheels/controller/provides.cfc index 631d2a5861..7b696054a4 100644 --- a/vendor/wheels/controller/provides.cfc +++ b/vendor/wheels/controller/provides.cfc @@ -10,6 +10,7 @@ component { */ public void function provides(string formats = "") { $combineArguments(args = arguments, combine = "formats,format", required = true); + $checkArguments(args = arguments, name = "provides"); arguments.formats = $listClean(arguments.formats); local.possibleFormats = StructKeyList($get("formats")); local.formatsArray = ListToArray(arguments.formats); diff --git a/vendor/wheels/events/init/debugging.cfm b/vendor/wheels/events/init/debugging.cfm index 9de1334a00..7083c8d020 100644 --- a/vendor/wheels/events/init/debugging.cfm +++ b/vendor/wheels/events/init/debugging.cfm @@ -5,6 +5,9 @@ // it must never be briefly true in production (issue ##3671). application.$wheels.showDebugInformation = application.$wheels.environment == "development"; application.$wheels.showErrorInformation = application.$wheels.environment != "production"; + // Unknown arguments to framework declarations (associations, validations, callbacks, ...): + // "warn" logs them to wheels.log, "throw" raises Wheels.UnknownArgument, "off" ignores them. + application.$wheels.strictArguments = application.$wheels.environment == "development" ? "warn" : "off"; application.$wheels.sendEmailOnError = false; application.$wheels.errorEmailSubject = "Error"; application.$wheels.excludeFromErrorEmail = ""; diff --git a/vendor/wheels/global/util.cfm b/vendor/wheels/global/util.cfm index 38b784eb7a..cf600f1dec 100644 --- a/vendor/wheels/global/util.cfm +++ b/vendor/wheels/global/util.cfm @@ -187,6 +187,9 @@ $combineArguments(args = arguments.args, combine = "#local.first#,#local.second#", required = local.required); } } + if ($strictArgumentsMode() != "off" && Len($strictArgumentsSource(arguments.name))) { + $checkArguments(args = arguments.args, name = arguments.name, accepted = $strictArgumentsAccepted(arguments.name)); + } if ($get("showErrorInformation")) { if (ListLen(arguments.reserved)) { local.iEnd = ListLen(arguments.reserved); @@ -222,6 +225,280 @@ } + /** + * Internal function. The `strictArguments` setting: "warn", "throw" or "off" (also for any + * other value, or before the setting exists). + */ + public string function $strictArgumentsMode() { + if ( + !StructKeyExists(application, "wheels") + || !StructKeyExists(application.wheels, "strictArguments") + || !IsSimpleValue(application.wheels.strictArguments) + ) { + return "off"; + } + local.rv = LCase(application.wheels.strictArguments); + return ListFind("warn,throw", local.rv) ? local.rv : "off"; + } + + /** + * Internal function. The framework functions whose arguments `strictArguments` checks, and + * where each is declared: "model" (the `wheels.Model` surface), a component path, or "" for a + * function that isn't checked. These take a fixed set of arguments. Functions that turn unknown + * arguments into something (property values, route params, HTML attributes, view variables, + * filter arguments) aren't checked. + */ + public string function $strictArgumentsSource(required string name) { + local.modelFunctions = "belongsTo,hasMany,hasOne,nestedProperties,property,table," + & "validate,validateOnCreate,validateOnUpdate,validatesConfirmationOf,validatesExclusionOf," + & "validatesFormatOf,validatesInclusionOf,validatesLengthOf,validatesNumericalityOf," + & "validatesPresenceOf,validatesUniquenessOf,afterNew,afterFind,afterInitialization," + & "beforeValidation,beforeValidationOnCreate,beforeValidationOnUpdate,afterValidation," + & "afterValidationOnCreate,afterValidationOnUpdate,beforeSave,afterSave,beforeCreate,afterCreate," + & "beforeUpdate,afterUpdate,beforeDelete,afterDelete,afterCommit,afterRollback," + & "findAll,findOne,findByKey,findFirst,findLastOne"; + if (ListFindNoCase(local.modelFunctions, arguments.name)) { + return "model"; + } + local.controllerFunctions = { + caches = "wheels.controller.caching", + protectsFromForgery = "wheels.controller.csrf", + provides = "wheels.controller.provides", + usesLayout = "wheels.controller.layouts" + }; + if (StructKeyExists(local.controllerFunctions, arguments.name)) { + return local.controllerFunctions[arguments.name]; + } + return ""; + } + + /** + * Internal function. Arguments a checked function takes without declaring them. findAll() reads + * `returnType` and `keyColumn` (native query options), the other finders pass everything on to + * findAll(), and aliases resolved later (`property` for `properties`) count too. + */ + public string function $strictArgumentsAccepted(required string name) { + switch (arguments.name) { + case "findAll": + return "returnType,keyColumn"; + case "findOne": case "findByKey": case "findFirst": + return ListAppend($declaredArgumentNames("findAll"), "returnType,keyColumn"); + case "findLastOne": + return ListAppend($declaredArgumentNames("findAll"), "returnType,keyColumn,properties"); + } + // the validations also take `property` for `properties` + if (Left(arguments.name, 9) == "validates") { + return "property"; + } + return ""; + } + + /** + * Internal function. The declared parameter names of a checked framework function, as a list, + * read from the framework's own definition (so an app override doesn't change it) and cached + * for the application's lifetime. Empty when the metadata can't be read: then nothing is + * checked, rather than guessed. + */ + public string function $declaredArgumentNames(required string name) { + if (!StructKeyExists(application.wheels, "$strictArgumentsDeclared")) { + application.wheels.$strictArgumentsDeclared = {}; + } + if (!StructKeyExists(application.wheels.$strictArgumentsDeclared, arguments.name)) { + try { + application.wheels.$strictArgumentsDeclared[arguments.name] = $readDeclaredArgumentNames(arguments.name); + } catch (any e) { + application.wheels.$strictArgumentsDeclared[arguments.name] = ""; + } + } + return application.wheels.$strictArgumentsDeclared[arguments.name]; + } + + /** + * Internal function. Reads [see:$declaredArgumentNames] from the component that declares the + * function: a bare `wheels.Model` for model functions, the controller component otherwise. + */ + public string function $readDeclaredArgumentNames(required string name) { + local.source = $strictArgumentsSource(arguments.name); + if (local.source == "model") { + if (!StructKeyExists(application.wheels, "modelSuperPrototype")) { + application.wheels.modelSuperPrototype = CreateObject("component", "wheels.Model"); + } + local.owner = application.wheels.modelSuperPrototype; + } else if (Len(local.source)) { + local.owner = CreateObject("component", local.source); + } else { + return ""; + } + if (!StructKeyExists(local.owner, arguments.name) || !IsCustomFunction(local.owner[arguments.name])) { + return ""; + } + local.fn = local.owner[arguments.name]; + local.rv = ""; + for (local.parameter in GetMetadata(local.fn).parameters) { + local.rv = ListAppend(local.rv, local.parameter.name); + } + return local.rv; + } + + /** + * Internal function. With `strictArguments` set to "warn" or "throw", reports each argument in + * `args` that `name` doesn't declare: positional and `$`-prefixed (internal) keys and the names + * in `accepted` are allowed. "warn" writes one `wheels.log` warning per class, function and + * argument for the application's lifetime; "throw" raises `Wheels.UnknownArgument`. + * `declared` replaces the function's metadata when given. + */ + public void function $checkArguments( + required struct args, + required string name, + string accepted = "", + string declared = "", + string label = "" + ) { + local.mode = $strictArgumentsMode(); + if (local.mode == "off") { + return; + } + local.known = Len(arguments.declared) ? arguments.declared : $declaredArgumentNames(arguments.name); + if (!Len(local.known)) { + return; + } + local.known = ListAppend(local.known, arguments.accepted); + for (local.key in arguments.args) { + // A declared parameter that wasn't passed can still be iterated (Lucee), as a null. + if (!StructKeyExists(arguments.args, local.key)) { + continue; + } + if (IsNumeric(local.key) || Left(local.key, 1) == "$" || ListFindNoCase(local.known, local.key)) { + continue; + } + $reportUnknownArgument( + name = Len(arguments.label) ? arguments.label : arguments.name, + argumentName = local.key, + known = local.known, + mode = local.mode, + family = arguments.name + ); + } + } + + /** + * Internal function. Logs or throws one unknown argument for [see:$checkArguments]. + */ + public void function $reportUnknownArgument( + required string name, + required string argumentName, + required string known, + required string mode, + string family = "" + ) { + local.owner = $strictArgumentsOwner(); + local.message = "`#arguments.name#()`" & (Len(local.owner) ? " on `#local.owner#`" : "") + & " got an argument it doesn't know: `#arguments.argumentName#`. Unknown arguments are ignored."; + local.suggestion = $suggestArgumentName(arguments.argumentName, arguments.known); + if (Len(local.suggestion)) { + local.message &= " Did you mean `#local.suggestion#`?"; + } + if (ListFindNoCase("findAll,findOne,findByKey,findFirst,findLastOne", Len(arguments.family) ? arguments.family : arguments.name)) { + local.message &= " If a model method overriding a finder passes its own arguments on (argumentCollection = arguments), remove them first with StructDelete()."; + } + if (arguments.mode == "throw") { + Throw( + type = "Wheels.UnknownArgument", + message = local.message, + extendedInfo = "Arguments it knows: #Replace(arguments.known, ",", ", ", "all")#. Set `strictArguments` to ""warn"" or ""off"" to allow it." + ); + } + if (!StructKeyExists(application.wheels, "$strictArgumentsSeen")) { + application.wheels.$strictArgumentsSeen = {}; + } + // The finders pass their arguments on to each other, so an unknown one is reported once, by + // the finder the app called (it checks first). + local.family = Len(arguments.family) ? arguments.family : arguments.name; + if (ListFindNoCase("findAll,findOne,findByKey,findFirst,findLastOne", local.family)) { + local.family = "finder"; + } + local.seenKey = local.owner & "|" & local.family & "|" & arguments.argumentName; + if (StructKeyExists(application.wheels.$strictArgumentsSeen, local.seenKey)) { + return; + } + // Argument names can come from a request (argumentCollection = params), so the record of + // what was logged is capped: past the cap, one last line says logging stopped. + if (StructCount(application.wheels.$strictArgumentsSeen) >= $strictArgumentsSeenCap()) { + if (!StructKeyExists(application.wheels, "$strictArgumentsCapped")) { + application.wheels.$strictArgumentsCapped = true; + writeLog( + file = "wheels", + type = "warning", + text = "Wheels: strictArguments has logged #$strictArgumentsSeenCap()# unknown arguments and stops logging new ones until the application reloads. Check for argumentCollection = params passed to a framework function." + ); + } + return; + } + application.wheels.$strictArgumentsSeen[local.seenKey] = true; + writeLog(file = "wheels", type = "warning", text = "Wheels: " & local.message); + } + + /** + * Internal function. How many distinct unknown arguments "warn" logs before it stops. + */ + public numeric function $strictArgumentsSeenCap() { + return 500; + } + + /** + * Internal function. The model or controller the checked call configures, for the message. + */ + public string function $strictArgumentsOwner() { + if (StructKeyExists(variables, "wheels") && IsStruct(variables.wheels) && StructKeyExists(variables.wheels, "class")) { + if (StructKeyExists(variables.wheels.class, "modelName")) { + return variables.wheels.class.modelName; + } + } + if (StructKeyExists(variables, "$class") && IsStruct(variables.$class) && StructKeyExists(variables.$class, "name")) { + return variables.$class.name; + } + return ListLast(GetMetadata(this).name, "."); + } + + /** + * Internal function. A known argument name that `argumentName` probably meant: the same name + * ignoring case, underscores and dashes, or a common name from other frameworks. Empty when + * there is no likely match. + */ + public string function $suggestArgumentName(required string argumentName, required string known) { + local.aliases = { + "null" = "allowNull", + "nullable" = "allowNull", + "classname" = "modelName", + "class" = "modelName", + "orderby" = "order", + "sort" = "order", + "limit" = "maxRows", + "includes" = "include", + "joins" = "include" + }; + local.normalized = LCase(ReReplace(arguments.argumentName, "[_\-]", "", "all")); + for (local.candidate in ListToArray(arguments.known)) { + if (LCase(ReReplace(local.candidate, "[_\-]", "", "all")) == local.normalized) { + return local.candidate; + } + } + // singular for plural and the other way round (method / methods) + for (local.variant in [arguments.argumentName & "s", ReReplaceNoCase(arguments.argumentName, "s$", "")]) { + local.position = ListFindNoCase(arguments.known, local.variant); + if (local.position) { + return ListGetAt(arguments.known, local.position); + } + } + if (StructKeyExists(local.aliases, local.normalized)) { + local.position = ListFindNoCase(arguments.known, local.aliases[local.normalized]); + if (local.position) { + return ListGetAt(arguments.known, local.position); + } + } + return ""; + } + // ====================================================================== // MISC FUNCTIONS // ====================================================================== diff --git a/vendor/wheels/migrator/Migration.cfc b/vendor/wheels/migrator/Migration.cfc index d7262f9772..b82a5cfc16 100755 --- a/vendor/wheels/migrator/Migration.cfc +++ b/vendor/wheels/migrator/Migration.cfc @@ -248,7 +248,14 @@ component extends="Base" { } else { arguments.columnNames = arguments.columnName ?: ""; } - invoke(t, arguments.columnType, arguments); + // The column helper gets the column's options, not this function's own arguments. + local.columnArgs = StructCopy(arguments); + StructDelete(local.columnArgs, "table"); + StructDelete(local.columnArgs, "addColumns"); + if (arguments.columnType != "references") { + StructDelete(local.columnArgs, "referenceName"); + } + invoke(t, arguments.columnType, local.columnArgs); t.change(addColumns = arguments.addColumns); } diff --git a/vendor/wheels/migrator/TableDefinition.cfc b/vendor/wheels/migrator/TableDefinition.cfc index 3bd4c76e3d..423754e280 100644 --- a/vendor/wheels/migrator/TableDefinition.cfc +++ b/vendor/wheels/migrator/TableDefinition.cfc @@ -168,7 +168,15 @@ component extends="Base" { */ private any function $addTypedColumns(required string columnType, required struct args) { $combineArguments(args = arguments.args, combine = "columnNames,columnName", required = true); + // `null` (the option's pre-3.0 name) becomes allowNull first, so the check below doesn't report it. $applyNullAlias(arguments.args); + // The options column() and ColumnDefinition read; anything else (`limits`, `nullable`) is ignored. + $checkArguments( + args = arguments.args, + name = arguments.columnType, + declared = "columnNames,columnName,columnType,default,allowNull,limit,precision,scale,unsigned,size,autoIncrement,afterColumn", + label = arguments.columnType == "biginteger" ? "bigInteger" : arguments.columnType + ); arguments.args.columnType = arguments.columnType; local.columnNamesArray = ListToArray(arguments.args.columnNames); local.iEnd = ArrayLen(local.columnNamesArray); diff --git a/vendor/wheels/model/calculations.cfm b/vendor/wheels/model/calculations.cfm index b031272026..04a000610c 100644 --- a/vendor/wheels/model/calculations.cfm +++ b/vendor/wheels/model/calculations.cfm @@ -267,6 +267,8 @@ StructDelete(arguments, "type"); StructDelete(arguments, "property"); StructDelete(arguments, "distinct"); + local.ifNull = StructKeyExists(arguments, "ifNull") ? arguments.ifNull : ""; + StructDelete(arguments, "ifNull"); // Since we don't return any records for calculation methods we want to skip the callbacks. arguments.callbacks = false; @@ -276,8 +278,8 @@ // If not grouping by something we just return the value itself. if (!StructKeyExists(arguments, "group")) { local.rv = local.rv[local.alias]; - if (!Len(local.rv) && Len(arguments.ifNull)) { - local.rv = arguments.ifNull; + if (!Len(local.rv) && Len(local.ifNull)) { + local.rv = local.ifNull; } } diff --git a/vendor/wheels/model/callbacks.cfm b/vendor/wheels/model/callbacks.cfm index a4f607cca2..ace3257f4c 100644 --- a/vendor/wheels/model/callbacks.cfm +++ b/vendor/wheels/model/callbacks.cfm @@ -207,6 +207,7 @@ * Internal function. */ public void function $registerCallback(required string type, required string methods) { + $checkArguments(args = arguments, name = arguments.type, accepted = "type,method"); // Create this type in the array if it doesn't already exist. if (!StructKeyExists(variables.wheels.class.callbacks, arguments.type)) { variables.wheels.class.callbacks[arguments.type] = []; @@ -374,6 +375,7 @@ * @on Restrict to one or more operations: create, update, delete (comma-delimited; blank = all). */ public void function afterCommit(string methods = "", string on = "") { + $checkArguments(args = arguments, name = "afterCommit"); $registerTransactionCallback(type = "afterCommit", methods = arguments.methods, on = arguments.on); } @@ -387,6 +389,7 @@ * @on Restrict to one or more operations: create, update, delete (comma-delimited; blank = all). */ public void function afterRollback(string methods = "", string on = "") { + $checkArguments(args = arguments, name = "afterRollback"); $registerTransactionCallback(type = "afterRollback", methods = arguments.methods, on = arguments.on); } diff --git a/vendor/wheels/model/miscellaneous.cfm b/vendor/wheels/model/miscellaneous.cfm index a2672df787..066236983b 100644 --- a/vendor/wheels/model/miscellaneous.cfm +++ b/vendor/wheels/model/miscellaneous.cfm @@ -75,6 +75,7 @@ * @name Name of the table to map this model to. */ public void function table(required any name) { + $checkArguments(args = arguments, name = "table"); variables.wheels.class.tableName = arguments.name; } diff --git a/vendor/wheels/model/onmissingmethod.cfm b/vendor/wheels/model/onmissingmethod.cfm index 381b1d4a2f..c4ed12a25f 100644 --- a/vendor/wheels/model/onmissingmethod.cfm +++ b/vendor/wheels/model/onmissingmethod.cfm @@ -265,6 +265,22 @@ StructDelete(arguments.missingMethodArguments, "1"); StructDelete(arguments.missingMethodArguments, "value"); StructDelete(arguments.missingMethodArguments, "values"); + // A property passed by name was read into the where clause above. findAll() ignores it, so + // it's dropped before the strictArguments check (kept if it's also a finder option). + if ($strictArgumentsMode() != "off") { + local.finderOptions = $strictArgumentsAccepted("findOne"); + for (local.property in local.finderProperties) { + if (!ListFindNoCase(local.finderOptions, local.property)) { + StructDelete(arguments.missingMethodArguments, local.property); + } + } + $checkArguments( + args = arguments.missingMethodArguments, + name = local.finderPrefix == "findOneBy" ? "findOne" : "findAll", + accepted = local.finderOptions, + label = arguments.missingMethodName + ); + } // call finder method if (Left(arguments.missingMethodName, 9) == "findOneBy") { @@ -315,7 +331,18 @@ // add where argument for findOne and remove afterwards arguments.where = $keyWhereString(local.property, local.value); - local.object = findOne(argumentCollection = arguments); + local.findArgs = arguments; + if ($strictArgumentsMode() != "off") { + // findOne() ignores the property values meant for create(), so don't pass them to it. + local.findArgs = {}; + local.finderOptions = $strictArgumentsAccepted("findOne"); + for (local.key in arguments) { + if (StructKeyExists(arguments, local.key) && (Left(local.key, 1) == "$" || ListFindNoCase(local.finderOptions, local.key))) { + local.findArgs[local.key] = arguments[local.key]; + } + } + } + local.object = findOne(argumentCollection = local.findArgs); StructDelete(arguments, "where"); if (IsObject(local.object)) { diff --git a/vendor/wheels/model/properties.cfm b/vendor/wheels/model/properties.cfm index ab3e49afd6..e77a77cf17 100644 --- a/vendor/wheels/model/properties.cfm +++ b/vendor/wheels/model/properties.cfm @@ -78,6 +78,7 @@ string dataType = "char", boolean automaticValidations ) { + $checkArguments(args = arguments, name = "property"); // validate setup if (Len(arguments.column) && Len(arguments.sql)) { Throw( diff --git a/vendor/wheels/model/read.cfm b/vendor/wheels/model/read.cfm index 038b4ad62b..ff4f755057 100644 --- a/vendor/wheels/model/read.cfm +++ b/vendor/wheels/model/read.cfm @@ -563,6 +563,9 @@ * @property [see:findFirst]. */ public any function findLastOne(string property) { + if ($strictArgumentsMode() != "off") { + $checkArguments(args = arguments, name = "findLastOne", accepted = $strictArgumentsAccepted("findLastOne")); + } arguments.$sort = "DESC"; return findFirst(argumentCollection = arguments); } diff --git a/vendor/wheels/model/validations.cfm b/vendor/wheels/model/validations.cfm index c60ade3354..b4acf82d9b 100644 --- a/vendor/wheels/model/validations.cfm +++ b/vendor/wheels/model/validations.cfm @@ -23,6 +23,7 @@ * @when [see:validatesConfirmationOf]. */ public void function validate(string methods = "", string condition = "", string unless = "", string when = "onSave") { + $checkArguments(args = arguments, name = "validate", accepted = "method"); $registerValidation(argumentCollection = arguments); } @@ -37,6 +38,7 @@ * @unless [see:validatesConfirmationOf]. */ public void function validateOnCreate(string methods = "", string condition = "", string unless = "") { + $checkArguments(args = arguments, name = "validateOnCreate", accepted = "method"); $registerValidation(when = "onCreate", argumentCollection = arguments); } @@ -51,6 +53,7 @@ * @unless [see:validatesConfirmationOf]. */ public void function validateOnUpdate(string methods = "", string condition = "", string unless = "") { + $checkArguments(args = arguments, name = "validateOnUpdate", accepted = "method"); $registerValidation(when = "onUpdate", argumentCollection = arguments); } diff --git a/vendor/wheels/tests/specs/global/strictArgumentsSpec.cfc b/vendor/wheels/tests/specs/global/strictArgumentsSpec.cfc new file mode 100644 index 0000000000..737a831814 --- /dev/null +++ b/vendor/wheels/tests/specs/global/strictArgumentsSpec.cfc @@ -0,0 +1,210 @@ +/** + * The strictArguments setting reports arguments a framework declaration doesn't know (a typo, a + * name from another framework) instead of ignoring them silently: "warn" logs one wheels.log line + * per class, function and argument, "throw" raises Wheels.UnknownArgument, "off" does nothing. + * Only functions with a fixed set of arguments are checked. The declaration specs use "throw", + * which fails before anything is registered, so no shared model class is changed. + */ +component extends="wheels.WheelsTest" { + + function beforeAll() { + variables.g = application.wo; + variables.migration = CreateObject("component", "wheels.migrator.Migration").init(); + } + + function saveStrictState() { + variables.saved = { + mode = StructKeyExists(application.wheels, "strictArguments") ? application.wheels.strictArguments : "off", + seen = StructKeyExists(application.wheels, "$strictArgumentsSeen") ? application.wheels.$strictArgumentsSeen : {} + }; + application.wheels.$strictArgumentsSeen = {}; + } + + function restoreStrictState() { + application.wheels.strictArguments = variables.saved.mode; + application.wheels.$strictArgumentsSeen = variables.saved.seen; + } + + function useMode(required string mode) { + application.wheels.strictArguments = arguments.mode; + } + + // The type and message of what `callback` throws, or empty strings. + 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("strictArguments", () => { + + beforeEach(() => { + saveStrictState(); + }); + + afterEach(() => { + restoreStrictState(); + }); + + describe("strictArguments = throw", () => { + + it("rejects an unknown association argument and suggests the right name", () => { + useMode("throw"); + var author = variables.g.model("author"); + var failure = failureOf(() => author.hasMany(name = "posts", foreign_key = "authorid")); + expect(failure.type).toBe("Wheels.UnknownArgument"); + expect(failure.message).toInclude("hasMany()"); + expect(failure.message).toInclude("foreign_key"); + expect(failure.message).toInclude("Did you mean `foreignKey`"); + }); + + it("rejects unknown arguments to validations and callback registration", () => { + useMode("throw"); + var author = variables.g.model("author"); + expect(failureOf(() => author.validatesPresenceOf(properties = "firstName", mesage = "x")).type).toBe("Wheels.UnknownArgument"); + expect(failureOf(() => author.beforeSave(methods = "callbackThatReturnsTrue", on = "create")).type).toBe("Wheels.UnknownArgument"); + expect(failureOf(() => author.afterCommit(method = "callbackThatReturnsTrue")).message).toInclude("Did you mean `methods`"); + }); + + it("accepts the documented aliases", () => { + useMode("throw"); + var author = variables.g.model("author"); + var accepted = author.$strictArgumentsAccepted("validatesLengthOf"); + expect(failureOf(() => author.$checkArguments(args = {property = "firstName", minimum = 1}, name = "validatesLengthOf", accepted = accepted)).type).toBe(""); + expect(failureOf(() => author.$args(args = {associations = "profile"}, name = "nestedProperties", combine = "association/associations")).type).toBe(""); + }); + + it("names the finder the app called", () => { + useMode("throw"); + var author = variables.g.model("author"); + expect(failureOf(() => author.findAll(orderby = "lastName")).message).toInclude("Did you mean `order`"); + expect(failureOf(() => author.findOne(orderby = "lastName")).message).toInclude("findOne()"); + expect(failureOf(() => author.findOneByFirstName(value = "Per", orderby = "lastName")).message).toInclude("findOneByFirstName()"); + }); + + it("lets the finders' own options and internal forwarding through", () => { + useMode("throw"); + var author = variables.g.model("author"); + expect(failureOf(() => author.findAll(order = "lastName", returnType = "query")).type).toBe(""); + expect(failureOf(() => author.findOneByFirstName(firstName = "Per")).type).toBe(""); + expect(failureOf(() => author.count()).type).toBe(""); + expect(failureOf(() => author.exists(where = "firstName = 'Per'")).type).toBe(""); + expect(failureOf(() => author.findFirst(property = "lastName")).type).toBe(""); + expect(failureOf(() => author.findLastOne(properties = "lastName")).type).toBe(""); + }); + + it("rejects an unknown column option in a migration", () => { + useMode("throw"); + var t = variables.migration.createTable(name = "c_o_r_e_strictargs", force = true); + var failure = failureOf(() => t.string(columnNames = "title", nullable = true)); + expect(failure.type).toBe("Wheels.UnknownArgument"); + expect(failure.message).toInclude("Did you mean `allowNull`"); + expect(failureOf(() => t.string(columnNames = "title", allowNull = true, limit = 50, default = "")).type).toBe(""); + expect(failureOf(() => t.bigInteger(columnName = "views", unsigned = true)).type).toBe(""); + }); + + it("accepts null, the deprecated column option, without warning", () => { + useMode("throw"); + var t = variables.migration.createTable(name = "c_o_r_e_strictargs", force = true); + // `null` is a keyword on some engines, so it goes in through argumentCollection. + var options = {columnNames = "title"}; + options["null"] = false; + expect(failureOf(() => t.string(argumentCollection = options)).type).toBe(""); + expect(t.columns[ArrayLen(t.columns)].allowNull).toBeFalse(); + useMode("warn"); + var more = {columnNames = "summary"}; + more["null"] = false; + t.string(argumentCollection = more); + expect(StructCount(application.wheels.$strictArgumentsSeen)).toBe(0); + }); + + it("doesn't check functions that turn unknown arguments into something", () => { + useMode("throw"); + expect(variables.g.$strictArgumentsSource("linkTo")).toBe(""); + expect(variables.g.$strictArgumentsSource("create")).toBe(""); + expect(variables.g.$strictArgumentsSource("filters")).toBe(""); + expect(variables.g.$strictArgumentsSource("hasMany")).toBe("model"); + }); + + it("ignores positional and internal arguments", () => { + useMode("throw"); + var author = variables.g.model("author"); + var args = {name = "posts"}; + args["1"] = "posts"; + args["$internal"] = true; + expect(failureOf(() => author.$checkArguments(args = args, name = "hasMany")).type).toBe(""); + }); + + }); + + describe("strictArguments = warn", () => { + + it("records an unknown argument once per class, function and argument, without throwing", () => { + useMode("warn"); + var author = variables.g.model("author"); + var first = failureOf(() => author.$checkArguments(args = {name = "posts", foreign_key = "x"}, name = "hasMany")); + var second = failureOf(() => author.$checkArguments(args = {name = "posts", foreign_key = "x"}, name = "hasMany")); + expect(first.type).toBe(""); + expect(second.type).toBe(""); + expect(StructCount(application.wheels.$strictArgumentsSeen)).toBe(1); + expect(StructKeyList(application.wheels.$strictArgumentsSeen)).toInclude("foreign_key"); + }); + + it("stops recording once the cap is reached", () => { + useMode("warn"); + var author = variables.g.model("author"); + var full = {}; + for (var i = 1; i <= author.$strictArgumentsSeenCap(); i++) { + full["filler|hasMany|arg#i#"] = true; + } + application.wheels.$strictArgumentsSeen = full; + StructDelete(application.wheels, "$strictArgumentsCapped"); + var failure = failureOf(() => author.$checkArguments(args = {name = "posts", foreign_key = "x"}, name = "hasMany")); + expect(failure.type).toBe(""); + expect(StructCount(application.wheels.$strictArgumentsSeen)).toBe(author.$strictArgumentsSeenCap()); + expect(StructKeyExists(application.wheels, "$strictArgumentsCapped")).toBeTrue(); + StructDelete(application.wheels, "$strictArgumentsCapped"); + }); + + it("reports a finder argument once, under the finder the app called", () => { + useMode("warn"); + var author = variables.g.model("author"); + author.findOne(orderby = "lastName"); + expect(StructCount(application.wheels.$strictArgumentsSeen)).toBe(1); + }); + + }); + + describe("strictArguments = off", () => { + + it("ignores unknown arguments", () => { + useMode("off"); + var author = variables.g.model("author"); + expect(failureOf(() => author.$checkArguments(args = {name = "posts", foreign_key = "x"}, name = "hasMany")).type).toBe(""); + expect(failureOf(() => author.findAll(orderby = "lastName")).type).toBe(""); + expect(StructCount(application.wheels.$strictArgumentsSeen)).toBe(0); + }); + + }); + + describe("The strictArguments default", () => { + + it("is off outside development", () => { + expect(variables.saved.mode).toBe(variables.g.get("environment") == "development" ? "warn" : "off"); + }); + + }); + + }); + + } + +} diff --git a/vendor/wheels/tests/specs/model/callbacksSpec.cfc b/vendor/wheels/tests/specs/model/callbacksSpec.cfc index 92d5edb6b0..03071e20aa 100644 --- a/vendor/wheels/tests/specs/model/callbacksSpec.cfc +++ b/vendor/wheels/tests/specs/model/callbacksSpec.cfc @@ -153,10 +153,10 @@ component extends="wheels.WheelsTest" { }) it("is setting properties on multiple objects", () => { - postsOrg = g.model("post").findAll(returnAs = "objects", callbacks = "false", orderby = "views DESC") + postsOrg = g.model("post").findAll(returnAs = "objects", callbacks = "false", order = "views DESC, id") views1 = postsOrg[1].views + 100 views2 = postsOrg[2].views + 100 - posts = g.model("post").findAll(returnAs = "objects", orderby = "views DESC") + posts = g.model("post").findAll(returnAs = "objects", order = "views DESC, id") expect(posts[1].title).toBe("setTitle") expect(posts[2].title).toBe("setTitle") diff --git a/vendor/wheels/tests/specs/model/crudSpec.cfc b/vendor/wheels/tests/specs/model/crudSpec.cfc index be71dbd079..bcb73c7d34 100644 --- a/vendor/wheels/tests/specs/model/crudSpec.cfc +++ b/vendor/wheels/tests/specs/model/crudSpec.cfc @@ -624,7 +624,7 @@ component extends="wheels.WheelsTest" { }) it("is selecting users groupby addresses", () => { - q = user.findAll(select = "address", group = "address", order = "address", result = "result") + q = user.findAll(select = "address", group = "address", order = "address") expect(q.recordcount).toBe(4) diff --git a/web/sites/guides/src/content/docs/v4-2-0/core-concepts/environments-and-configuration.mdx b/web/sites/guides/src/content/docs/v4-2-0/core-concepts/environments-and-configuration.mdx index f8684d11b6..c963581e9c 100644 --- a/web/sites/guides/src/content/docs/v4-2-0/core-concepts/environments-and-configuration.mdx +++ b/web/sites/guides/src/content/docs/v4-2-0/core-concepts/environments-and-configuration.mdx @@ -63,6 +63,8 @@ Settings cascade: framework defaults load first, then `config/settings.cfm`, the One development-only default worth noting: `reloadOnGlobalChange` (defaults `true` in development) makes bare `?reload=true` re-include `app/global/*.cfm` when any tracked file's mtime changes, so new helpers added to `app/global/functions.cfm` are picked up immediately without the password-gated full reload. Opt out with `set(reloadOnGlobalChange=false)` in `config/settings.cfm`. The setting defaults to `false` in all other environments, so there is no `DirectoryList` overhead in production. +Another: `strictArguments` (defaults `"warn"` in development, `"off"` elsewhere) logs an argument that an association, validation, finder or migration column doesn't know, such as `foreign_key` or `orderby`, instead of ignoring it. `set(strictArguments="throw")` makes it an error. See [Strict Arguments](/v4-2-0/digging-deeper/strict-arguments/). + ## Environment detection The active environment is whatever `config/environment.cfm` sets. A generated app reads it from `WHEELS_ENV`: diff --git a/web/sites/guides/src/content/docs/v4-2-0/digging-deeper/overriding-core-methods.mdx b/web/sites/guides/src/content/docs/v4-2-0/digging-deeper/overriding-core-methods.mdx index 8685fb7a7c..9a2b1db1f2 100644 --- a/web/sites/guides/src/content/docs/v4-2-0/digging-deeper/overriding-core-methods.mdx +++ b/web/sites/guides/src/content/docs/v4-2-0/digging-deeper/overriding-core-methods.mdx @@ -53,7 +53,7 @@ component extends="Model" { } ``` -This works for any public framework method mixed into models — `findOne()`, `columnNames()`, `save()`, and so on. +This works for any public framework method mixed into models — `findOne()`, `columnNames()`, `save()`, and so on. If your override takes arguments of its own, remove them from `arguments` before delegating: the original ignores them, and with [`strictArguments`](/v4-2-0/digging-deeper/strict-arguments/) on it reports them. ## Overriding a controller or view helper diff --git a/web/sites/guides/src/content/docs/v4-2-0/digging-deeper/strict-arguments.mdx b/web/sites/guides/src/content/docs/v4-2-0/digging-deeper/strict-arguments.mdx new file mode 100644 index 0000000000..9cc98e4047 --- /dev/null +++ b/web/sites/guides/src/content/docs/v4-2-0/digging-deeper/strict-arguments.mdx @@ -0,0 +1,126 @@ +--- +title: Strict Arguments +description: Catch misspelled or unsupported arguments to associations, validations, callbacks, finders and migration columns. +type: howto +--- + +import { Aside, CardGrid, LinkCard } from '@astrojs/starlight/components'; + +The `strictArguments` setting reports an argument a framework function doesn't know, instead of ignoring it. + +You'll learn: + +- what the setting checks, and its three values +- how to read a warning and fix the call +- how to opt into errors, and when an override needs a change + +## Why it exists + +Wheels functions take named arguments, and an unknown name is ignored. A typo such as `orderby` for `order`, or a name from another framework such as `foreign_key` or `nullable`, doesn't fail. The call runs without the option, and the mistake shows up later as unordered results, a missing foreign key or a column that allows NULL when you didn't mean it to. + +With `strictArguments` on, those calls are reported where they're made: + +```text title="illustrative — do not type" +Wheels: `hasMany()` on `Post` got an argument it doesn't know: `foreign_key`. Unknown arguments are ignored. Did you mean `foreignKey`? +``` + +## The three values + +| Value | What happens | Default in | +|---|---|---| +| `"warn"` | One line in `wheels.log` per model or controller, function and argument, for the life of the application. | `development` | +| `"throw"` | The call raises `Wheels.UnknownArgument`, with the same text. | none (opt-in) | +| `"off"` | Unknown arguments are ignored, as before 4.2. | `testing`, `production`, `maintenance` | + +Set it in `config/settings.cfm`, or per environment in `config//settings.cfm`: + +```cfm title="config/development/settings.cfm" {test:compile} + + set(strictArguments = "throw"); + +``` + +Use `"throw"` in development once your app's warnings are fixed, so a new mistake fails at once. Keep `"off"` in production: a warning there means the code was never run in development. + +## What it checks + +Only functions that take a fixed set of arguments are checked: + +- **Associations:** `belongsTo()`, `hasMany()`, `hasOne()`, `nestedProperties()`. +- **Validations:** `validate()`, `validateOnCreate()`, `validateOnUpdate()` and every `validates*()` function. +- **Callback registration:** `beforeSave()`, `afterCommit()` and the other callback functions. +- **Model configuration:** `property()`, `table()`. +- **Finders:** `findAll()`, `findOne()`, `findByKey()`, `findFirst()`, `findLastOne()` and the dynamic finders (`findOneByEmail()`, `findAllByStatus()`). +- **Controller configuration:** `caches()`, `protectsFromForgery()`, `provides()`, `usesLayout()`. +- **Migration columns:** `t.string()`, `t.integer()` and the other column functions of a table definition. + +Functions that turn unknown arguments into something aren't checked: `create()`, `new()` and `update()` (property values), `linkTo()` and `URLFor()` (route parameters), the form helpers (HTML attributes), `includePartial()` and `sendEmail()` (view variables), and `filters()` (filter arguments). + +Documented aliases are accepted: `property` for `properties`, `method` for `methods`, `associations` for `association`. So are the finders' `returnType` and `keyColumn`, and a property passed by name to a dynamic finder (`findOneByEmail(email = params.email)`). + +## Fix the common warnings + +| Argument | Use instead | +|---|---| +| `foreign_key`, `class_name` | `foreignKey`, `modelName` | +| `nullable` on a migration column | `allowNull` (`null`, its name before Wheels 3.0, is still read, with a deprecation warning) | +| `orderby`, `sort` | `order` | +| `limit` on a finder | `maxRows` | +| `on` on `beforeSave()` | `beforeCreate()` or `beforeUpdate()` (`afterCommit()` and `afterRollback()` do take `on`) | +| `method` on `afterCommit()` | `methods` | + +```cfm title="app/models/Post.cfc" {test:compile} +component extends="Model" { + function config() { + hasMany(name = "comments", foreignKey = "postId"); + validatesPresenceOf(properties = "title", message = "Every post needs a title"); + beforeCreate("setSlug"); + } +} +``` + +## Overriding a finder + +A model method that overrides a finder and passes its own arguments on with `argumentCollection = arguments` is reported too, because the extra arguments reach the framework's finder. Remove them before the call: + +```cfm title="app/models/Post.cfc" {test:compile} +component extends="Model" { + function findAll(boolean published = false) { + if (arguments.published) { + arguments.where = "publishedAt IS NOT NULL"; + } + StructDelete(arguments, "published"); + return superFindAll(argumentCollection = arguments); + } +} +``` + +## Don't pass `params` straight through + +`findAll(argumentCollection = params)` turns every request parameter into an argument name, so a visitor decides what reaches the finder. With `strictArguments` on, every unknown parameter is reported, and with `"throw"` an extra query-string key fails the request. Pick the keys you mean: + +```cfm title="app/controllers/Posts.cfc" {test:compile} +component extends="Controller" { + function index() { + posts = model("post").findAll( + where = "status = 'published'", + order = "publishedAt DESC", + page = Val(params.page ?: 1), + perPage = 20 + ); + } +} +``` + +`"warn"` logs at most 500 distinct unknown arguments per application lifetime. After that it logs one line saying it stopped, so arbitrary request keys can't grow the log or the record of what was logged. + + + +## Related guides + + + + + diff --git a/web/sites/guides/src/sidebars/v4-2-0.json b/web/sites/guides/src/sidebars/v4-2-0.json index 30203ddfd3..701babb18a 100644 --- a/web/sites/guides/src/sidebars/v4-2-0.json +++ b/web/sites/guides/src/sidebars/v4-2-0.json @@ -101,6 +101,7 @@ { "label": "Rate Limiting", "link": "/v4-2-0/digging-deeper/rate-limiting/" }, { "label": "Dependency Injection Usage", "link": "/v4-2-0/digging-deeper/dependency-injection-usage/" }, { "label": "Overriding Core Methods", "link": "/v4-2-0/digging-deeper/overriding-core-methods/" }, + { "label": "Strict Arguments", "link": "/v4-2-0/digging-deeper/strict-arguments/" }, { "label": "Debug Panel", "link": "/v4-2-0/digging-deeper/debug-panel/" } ] },