diff --git a/changelog.d/where-literal-predicates.fixed.md b/changelog.d/where-literal-predicates.fixed.md new file mode 100644 index 0000000000..8db4a86dea --- /dev/null +++ b/changelog.d/where-literal-predicates.fixed.md @@ -0,0 +1 @@ +- `where=`: a literal compared with another table's column inside a subquery (`id IN (SELECT book_id FROM accesses WHERE user_id = 1)`) now throws `Wheels.UnbindableWhereValue`, naming the condition (bound values shown as `?`, quoted values hidden) and the alternatives: `whereIn()` with a value array, or running the subquery first. Before, it failed with `Array index [1] out of range`. `Wheels.ColumnNotFound` from a `where=` now explains the rule too. `where=` accepts model columns compared with values, so a condition with no column, such as `where="1 = 0"`, isn't supported: for no rows use `whereIn()` with an empty array, and for every row leave the condition out. diff --git a/cli/lucli/templates/app/.ai/models.md b/cli/lucli/templates/app/.ai/models.md index 9e5c3efacd..213f74876c 100644 --- a/cli/lucli/templates/app/.ai/models.md +++ b/cli/lucli/templates/app/.ai/models.md @@ -60,6 +60,10 @@ component extends="Model" { ``` Finders: `model("User").findAll()`, `findOne(where="...")`, `findByKey(params.key)`. + +`where=` is parsed, not passed through: every `column literal` is bound as a parameter, and the column must be the model's or an `include`d model's. +- A condition with no column (`where="1 = 0"`, `"1 = 1"`) isn't supported; it throws `Wheels.ColumnNotFound`. To match nothing use `whereIn("id", [])` (no rows); to match everything leave the condition out. +- A literal inside a subquery (`id IN (SELECT ... WHERE user_id = 1)`) throws `Wheels.UnbindableWhereValue`. A subquery comparing columns only works. Otherwise run the subquery first and pass its values with `whereIn("id", ValueArray(q, "bookId"))`. Create: `model("User").new(params.user).save()`, or `model("User").create(params.user)`. Include associations: `findAll(include="role,orders")`. Pagination: `findAll(page=params.page, perPage=25)`. diff --git a/docs/consumer-ai/.ai/models.md b/docs/consumer-ai/.ai/models.md index 9e5c3efacd..213f74876c 100644 --- a/docs/consumer-ai/.ai/models.md +++ b/docs/consumer-ai/.ai/models.md @@ -60,6 +60,10 @@ component extends="Model" { ``` Finders: `model("User").findAll()`, `findOne(where="...")`, `findByKey(params.key)`. + +`where=` is parsed, not passed through: every `column literal` is bound as a parameter, and the column must be the model's or an `include`d model's. +- A condition with no column (`where="1 = 0"`, `"1 = 1"`) isn't supported; it throws `Wheels.ColumnNotFound`. To match nothing use `whereIn("id", [])` (no rows); to match everything leave the condition out. +- A literal inside a subquery (`id IN (SELECT ... WHERE user_id = 1)`) throws `Wheels.UnbindableWhereValue`. A subquery comparing columns only works. Otherwise run the subquery first and pass its values with `whereIn("id", ValueArray(q, "bookId"))`. Create: `model("User").new(params.user).save()`, or `model("User").create(params.user)`. Include associations: `findAll(include="role,orders")`. Pagination: `findAll(page=params.page, perPage=25)`. diff --git a/vendor/wheels/model/sql.cfm b/vendor/wheels/model/sql.cfm index 9004c9c392..84a64fdbba 100644 --- a/vendor/wheels/model/sql.cfm +++ b/vendor/wheels/model/sql.cfm @@ -1575,7 +1575,7 @@ Throw( type = "Wheels.ColumnNotFound", message = "Wheels looked for the column mapped to the `#local.param.property#` property but couldn't find it in the database table.", - extendedInfo = "Verify the `where` argument and/or your property to column mappings done with the `property` method inside the model's `config` method to make sure everything is correct." + extendedInfo = $columnNotFoundWhereHint(property = local.param.property, where = arguments.where) ); } else { writeLog( @@ -1608,6 +1608,10 @@ if (!StructIsEmpty(local.leftExpression)) { local.where = Replace(local.where, local.element, Replace(local.element, local.leftExpression.dataPart, "?", "one")); ArrayAppend(local.params, local.leftExpression.param); + } else if (Find("?", local.element)) { + // A value compared with something that isn't this model's (or an included + // model's) column, typically inside a subquery: there is no column to bind it to. + $throwUnbindableWhereValue(element = local.element); } else if (ArrayLen(local.classes) > 1) { local.where = $qualifyUnboundConditionColumn(where = local.where, element = local.element, classes = local.classes, useTableAlias = local.useTableAlias); } @@ -1679,6 +1683,39 @@ return local.rv; } + /** + * Internal function. The detail for Wheels.ColumnNotFound raised by a `where` condition: what + * to check, and, for a number in the column's place (`1 = 0`) or a clause with a subquery, + * what `where` accepts instead. + */ + public string function $columnNotFoundWhereHint(required string property, required string where) { + local.rv = "Verify the `where` argument and/or your property to column mappings done with the `property` method inside the model's `config` method to make sure everything is correct."; + if (ReFind("^[+-]?[0-9.]+$", arguments.property)) { + local.rv &= " `where` accepts this model's and included models' columns compared with values; a condition with no column, such as `1 = 0` or `1 = 1`, isn't supported. For no rows, use whereIn() with an empty array; for every row, leave the condition out."; + } else if (FindNoCase("SELECT", arguments.where)) { + local.rv &= " If `#arguments.property#` is a column inside a subquery: `where` binds every `column value` and accepts only this model's and included models' columns, so a value compared with a subquery's column can't be bound. Use whereIn() with a value array, or run the subquery as its own query first."; + } + return local.rv; + } + + /** + * Internal function. Throws Wheels.UnbindableWhereValue for a `where` condition whose value + * has no column of this model (or an included model) to be bound against. The condition is + * echoed with its quoted values hidden: a bound one is already `?`, and any other quoted + * literal (still in its masked, reversible form) is shown as '...'. Unquoted numbers in the + * developer's own where text are shown as written. + */ + public void function $throwUnbindableWhereValue(required string element) { + local.condition = Trim(ReReplaceNoCase(Trim(arguments.element), "^(AND|OR)([^a-zA-Z0-9_$])", "\2")); + local.condition = ReReplace(local.condition, "'" & $whereLiteralSentinel() & "[^']*'", "'...'", "all"); + local.condition = Replace(local.condition, Chr(2), "", "all"); + Throw( + type = "Wheels.UnbindableWhereValue", + message = "Wheels can't bind the value in the `where` condition `#local.condition#` (bound values are shown as ? and quoted values as '...'): it isn't compared with a column of this model or an included model, such as a value inside a subquery.", + extendedInfo = "`where` binds every `column value` as a query parameter, and the column must belong to the model or an `include`d model. Compare subquery columns with each other only, pass the values with whereIn() (a value array), or run the subquery as its own query first and pass its results." + ); + } + /** * Internal function. For a WHERE condition whose value is a function call (so not a * bound parameter), qualifies the property it starts with by its diff --git a/vendor/wheels/tests/specs/model/whereUnboundLiteralSpec.cfc b/vendor/wheels/tests/specs/model/whereUnboundLiteralSpec.cfc new file mode 100644 index 0000000000..5a60ba5c8f --- /dev/null +++ b/vendor/wheels/tests/specs/model/whereUnboundLiteralSpec.cfc @@ -0,0 +1,98 @@ +component extends="wheels.WheelsTest" { + + function run() { + + g = application.wo + + // `where` binds every `column value`, and the column must belong to the model or + // an included model. A condition with no column (1 = 0) isn't supported, and a value with no + // column to bind against, such as one inside a subquery, throws an error naming the condition. + + describe("where with a condition that has no column", () => { + + it("says 1 = 0 isn't supported, and what to use instead", () => { + var state = {type = "", detail = ""}; + try { + g.model("post").findAll(where = "1 = 0", returnAs = "query", reload = true); + } catch (any e) { + state.type = e.type; + state.detail = e.extendedInfo; + } + expect(state.type).toBe("Wheels.ColumnNotFound"); + expect(state.detail).toInclude("isn't supported"); + expect(state.detail).toInclude("whereIn() with an empty array"); + }) + + it("returns no rows for whereIn() with an empty array, the suggested alternative", () => { + expect(g.model("post").whereIn("id", []).count()).toBe(0); + }) + + }); + + describe("where with a value it can't bind", () => { + + it("names the condition when a subquery compares a column with a value", () => { + var state = {type = "", message = ""}; + try { + g.model("author").findAll(where = "id IN (SELECT authorid FROM c_o_r_e_posts WHERE views = 5)", returnAs = "query", reload = true); + } catch (any e) { + state.type = e.type; + state.message = e.message; + } + expect(state.type).toBe("Wheels.UnbindableWhereValue"); + // The parser drops the spaces around an operator it masks. + expect(state.message).toInclude("WHERE views=?"); + }) + + it("hides quoted values in the error, including a quoted literal elsewhere in the subquery", () => { + var state = {type = "", message = ""}; + try { + g.model("author").findAll( + where = "id IN (SELECT COALESCE(authorid, LENGTH('private_marker_x')) FROM c_o_r_e_posts WHERE views = 5 AND title = 'private_title_y')", + returnAs = "query", + reload = true + ); + } catch (any e) { + state.type = e.type; + state.message = e.message & " " & e.extendedInfo; + } + expect(state.type).toBe("Wheels.UnbindableWhereValue"); + expect(state.message).toInclude("'...'"); + for (var secret in ["private_marker_x", "private_title_y"]) { + expect(state.message).notToInclude(secret); + expect(state.message).notToInclude(LCase(BinaryEncode(CharsetDecode(secret, "utf-8"), "hex"))); + } + expect(state.message).notToInclude("wmask"); + expect(Find(Chr(2), state.message)).toBe(0); + }) + + it("still runs a subquery that compares columns only", () => { + var authors = g.model("author").findAll( + where = "id IN (SELECT authorid FROM c_o_r_e_posts WHERE c_o_r_e_posts.authorid = c_o_r_e_authors.id)", + returnAs = "query", + reload = true + ); + expect(authors.recordCount).toBeGT(0); + }) + + it("points at subqueries when a later condition names a column the model doesn't have", () => { + var state = {type = "", detail = ""}; + try { + g.model("author").findAll( + where = "id IN (SELECT authorid FROM c_o_r_e_posts WHERE c_o_r_e_posts.authorid = c_o_r_e_authors.id AND views = 5)", + returnAs = "query", + reload = true + ); + } catch (any e) { + state.type = e.type; + state.detail = e.extendedInfo; + } + expect(state.type).toBe("Wheels.ColumnNotFound"); + expect(state.detail).toInclude("subquery"); + expect(state.detail).toInclude("whereIn()"); + }) + + }); + } + +} diff --git a/web/sites/guides/src/content/docs/v4-2-0/basics/query-builder-and-scopes.mdx b/web/sites/guides/src/content/docs/v4-2-0/basics/query-builder-and-scopes.mdx index fcea2e1e62..363152495a 100644 --- a/web/sites/guides/src/content/docs/v4-2-0/basics/query-builder-and-scopes.mdx +++ b/web/sites/guides/src/content/docs/v4-2-0/basics/query-builder-and-scopes.mdx @@ -127,6 +127,16 @@ A literal anywhere else stays in the SQL text, escaped for your database: Escaped is not the same as bound. Don't build those positions from request input: compare with an operator instead, or use the builder. +**`where` accepts model columns compared with values.** A `where` string is parsed, not passed through. Each `column literal` is bound, and the column has to belong to the model you're querying or to one you `include`. Two shapes follow from that. + +- A condition with no column, such as `where="1 = 0"` or `where="1 = 1"`, isn't supported. It throws `Wheels.ColumnNotFound`. For a query that should match nothing, use `whereIn()` with an empty array: `model("Post").whereIn("id", []).get()` returns no rows. For one that should match everything, leave the condition out, or skip the call. +- A literal compared with another table's column inside a subquery can't be bound: `id IN (SELECT book_id FROM accesses WHERE user_id = 1)` throws `Wheels.UnbindableWhereValue`, naming the condition. A subquery that compares columns only works: `id IN (SELECT book_id FROM accesses WHERE accesses.book_id = books.id)`. For a subquery that needs a value, run it as its own query first and pass its results with `whereIn()`, which binds each value: + + ```cfm + bookIds = model("Access").findAll(where = "userId = #userId#", select = "bookId", returnAs = "query"); + books = model("Book").whereIn("id", ValueArray(bookIds, "bookId")).get(); + ``` +