diff --git a/CHANGELOG.md b/CHANGELOG.md index 768090a6b1..735110f7f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ All historical references to "CFWheels" in this changelog have been preserved fo ### Fixed +- Model layer SELECT clause builder now routes column identifiers through the adapter's `$quoteIdentifier`, so reserved-word column names (e.g. `key`, `order`, `group`) survive on every supported dialect instead of breaking `findAll` / `findOne` / dynamic finders with cryptic SQL syntax errors. The WHERE / ORDER BY paths already quoted columns; `$createSQLFieldList` and the empty-pagination column-list extraction in `read.cfc` now match. - `wheels packages install ` is now a transparent alias for `wheels packages add ` on every caller path that actually reaches `Module.cfc` (stdio MCP server, scripted in-process clients, spec suite). Previously the dispatch layer's `case "install":` branch printed a yellow warning to stdout and returned `""` without installing anything — so even though `PackagesMainCli.install()` itself had been a true alias for `add()` since #2729, any caller routing through the CLI module's verb dispatch silently no-op'd. The shell-facing `wheels packages install ` form is still intercepted by LuCLI's built-in extension installer upstream of module dispatch and remains broken on that path (and is documented as such in the module-owned help text), but MCP tool calls and programmatic callers now behave identically to `add`. Both branches now share a single fall-through body so the validation, error shape, and install behavior cannot drift apart again. - `wheels.wheelstest.BrowserTest` now throws a clear `Wheels.BrowserTest.NotWired` error — naming `browserDescribe()` as the fix — when a spec calls a DSL method on `this.browser` from a plain `describe()` block. Previously the uninitialized `this.browser` was an empty string, producing the misleading `function [visitUrl] does not exist in the String` on every newcomer's first BrowserTest spec. A sentinel `UnwiredBrowserGuard` is now installed at `this.browser` before `browserDescribe()` wires the real `BrowserClient` and after `$endBrowserContext()` tears it down - `BrowserTest`'s default base URL is no longer hardcoded to `http://localhost:8080`. `$resolveBaseUrl()` now consults a layered lookup at instance time: `this.baseUrl` (per-spec override, set in the component pseudo-constructor) → `get("browserTestBaseUrl")` (Wheels setting) → `-Dwheels.browserTest.baseUrl=...` (JVM system property) → `WHEELS_BROWSER_TEST_BASE_URL` env var → `cgi.server_name`/`cgi.server_port` auto-detect → `http://localhost:8080` default. Specs running against a non-8080 server (Titan on 60050, `wheels new` scaffolds on 60080) can set `this.baseUrl` in the pseudo-constructor or rely on the CGI auto-detect instead of comparing `getBaseUrl()` against a sentinel string. The bare-env-var approach still works for CI but is no longer the only escape hatch (the JVM caches env vars at process start, so post-launch `export` had no effect). Regression spec at `vendor/wheels/tests/specs/wheelstest/BrowserTestBaseUrlResolutionSpec.cfc` (#2779) diff --git a/vendor/wheels/model/read.cfc b/vendor/wheels/model/read.cfc index 787427986d..8b0b1d53c6 100644 --- a/vendor/wheels/model/read.cfc +++ b/vendor/wheels/model/read.cfc @@ -214,7 +214,9 @@ component { list = arguments.select, returnAs = arguments.returnAs ); - local.columns = ReReplace(local.columns, "[`""\[\]\w]*?\.([\w\s]*?)(,|$)", "\1\2", "all"); + // Strip dialect quotes: $createSQLFieldList now quotes identifiers; the bare-identifier regex below requires unquoted input. + local.columns = variables.wheels.class.adapter.$stripIdentifierQuotes(local.columns); + local.columns = ReReplace(local.columns, "[\w]*?\.([\w\s]*?)(,|$)", "\1\2", "all"); local.columns = ReReplace(local.columns, "\(.*?\)\sAS\s([\w\s]*?)(,|$)", "\1\2", "all"); local.columns = ReReplace(local.columns, "\w*?\sAS\s([\w\s]*?)(,|$)", "\1\2", "all"); local.rv = QueryNew(local.columns); diff --git a/vendor/wheels/model/sql.cfc b/vendor/wheels/model/sql.cfc index c7040bf2aa..f62ec78958 100644 --- a/vendor/wheels/model/sql.cfc +++ b/vendor/wheels/model/sql.cfc @@ -567,9 +567,9 @@ component { if (StructKeyExists(local.classData.propertyStruct, local.iItem)) { local.toAppend &= variables.wheels.class.adapter.$quoteIdentifier(local.classData.tableName) & "."; if (StructKeyExists(local.classData.columnStruct, local.iItem)) { - local.toAppend &= local.iItem; + local.toAppend &= variables.wheels.class.adapter.$quoteIdentifier(local.iItem); } else { - local.toAppend &= local.classData.properties[local.iItem].column; + local.toAppend &= variables.wheels.class.adapter.$quoteIdentifier(local.classData.properties[local.iItem].column); if (arguments.clause == "select") { local.toAppend &= " AS " & local.iItem; } @@ -631,6 +631,9 @@ component { // get the property part, done by taking everything from the end of the string to a . or a space (which would be found when using " AS ") local.property = Reverse(SpanExcluding(Reverse(local.iItem), ". ")); + // Strip dialect quotes added above so alias matching and downstream concatenation work on bare identifiers. + local.property = variables.wheels.class.adapter.$stripIdentifierQuotes(local.property); + // check if this one has been flagged as a duplicate, we get the number of classes to skip and also remove the flagged info from the item local.duplicateCount = 0; local.matches = ReFind("^\[\[duplicate\]\](\d+)(.+)$", local.iItem, 1, true); @@ -668,7 +671,7 @@ component { local.newItem = ReplaceNoCase(local.iItem, " AS " & local.property, " AS " & local.newProperty); } else { if (local.aliasFound) { - local.newItem = local.alias & "." & local.property & " AS " & local.newProperty; + local.newItem = local.alias & "." & variables.wheels.class.adapter.$quoteIdentifier(local.property) & " AS " & local.newProperty; } else { local.newItem = local.iItem & " AS " & local.newProperty; } diff --git a/vendor/wheels/tests/specs/model/crudSpec.cfc b/vendor/wheels/tests/specs/model/crudSpec.cfc index e58fe597a0..a151074d85 100644 --- a/vendor/wheels/tests/specs/model/crudSpec.cfc +++ b/vendor/wheels/tests/specs/model/crudSpec.cfc @@ -573,7 +573,7 @@ component extends="wheels.WheelsTest" { // trim extra whitespace actual = Trim(actual) - expected = "SELECT #qi('c_o_r_e_authors')#.id FROM #qi('c_o_r_e_authors')#" + expected = "SELECT #qi('c_o_r_e_authors')#.#qi('id')# FROM #qi('c_o_r_e_authors')#" expect(actual).toBe(expected) }) @@ -1671,7 +1671,7 @@ component extends="wheels.WheelsTest" { local.a = qi("c_o_r_e_authors"); local.p = qi("c_o_r_e_posts"); - expect(columnList).toBe("#local.a#.firstname,#local.a#.id,#local.a#.id AS Authorid,#local.a#.lastname,#local.p#.averagerating AS postaveragerating,#local.p#.body AS postbody,#local.p#.createdat AS postcreatedat,#local.p#.deletedat AS postdeletedat,#local.p#.id AS postid,#local.p#.status AS poststatus,#local.p#.title AS posttitle,#local.p#.updatedat AS postupdatedat,#local.p#.views AS postviews") + expect(columnList).toBe("#local.a#.#qi('firstname')#,#local.a#.#qi('id')#,#local.a#.#qi('id')# AS Authorid,#local.a#.#qi('lastname')#,#local.p#.#qi('averagerating')# AS postaveragerating,#local.p#.#qi('body')# AS postbody,#local.p#.#qi('createdat')# AS postcreatedat,#local.p#.#qi('deletedat')# AS postdeletedat,#local.p#.#qi('id')# AS postid,#local.p#.#qi('status')# AS poststatus,#local.p#.#qi('title')# AS posttitle,#local.p#.#qi('updatedat')# AS postupdatedat,#local.p#.#qi('views')# AS postviews") }) it("works with association with expanded aliases disabled", () => { @@ -1688,7 +1688,7 @@ component extends="wheels.WheelsTest" { local.a = qi("c_o_r_e_authors"); local.p = qi("c_o_r_e_posts"); - expect(columnList).toBe("#local.a#.firstname,#local.a#.id,#local.a#.id AS Authorid,#local.a#.lastname,#local.p#.averagerating,#local.p#.body,#local.p#.createdat,#local.p#.deletedat,#local.p#.id AS postid,#local.p#.status,#local.p#.title,#local.p#.updatedat,#local.p#.views") + expect(columnList).toBe("#local.a#.#qi('firstname')#,#local.a#.#qi('id')#,#local.a#.#qi('id')# AS Authorid,#local.a#.#qi('lastname')#,#local.p#.#qi('averagerating')#,#local.p#.#qi('body')#,#local.p#.#qi('createdat')#,#local.p#.#qi('deletedat')#,#local.p#.#qi('id')# AS postid,#local.p#.#qi('status')#,#local.p#.#qi('title')#,#local.p#.#qi('updatedat')#,#local.p#.#qi('views')#") }) it("works on calculated property", () => { diff --git a/vendor/wheels/tests/specs/model/reservedColumnQuotingSpec.cfc b/vendor/wheels/tests/specs/model/reservedColumnQuotingSpec.cfc new file mode 100644 index 0000000000..f9eef9aa04 --- /dev/null +++ b/vendor/wheels/tests/specs/model/reservedColumnQuotingSpec.cfc @@ -0,0 +1,51 @@ +component extends="wheels.WheelsTest" { + + function run() { + + g = application.wo; + + describe("$createSQLFieldList column identifier quoting", () => { + + it("quotes the underlying column when a property aliases a different column name", () => { + // City aliases id -> countyid, so the SELECT column must be quoted in case it is a reserved word. + var m = g.model("city"); + var actual = m.$selectClause(select = "id", include = "", returnAs = "objects"); + + expect(actual).toInclude(m.$quotedTableName() & "." & m.$quoteColumn("countyid")); + }); + + it("quotes the column when the property name matches the column name", () => { + // Author.firstName has no column mapping (property == column); SELECT must still quote the identifier. + var m = g.model("author"); + var actual = m.$selectClause(select = "firstName", include = "", returnAs = "objects"); + + expect(actual).toInclude(m.$quotedTableName() & "." & m.$quoteColumn("firstName")); + }); + + it("quotes column identifiers in the GROUP BY clause too", () => { + // $groupByClause routes through the same $createSQLFieldList builder. + var m = g.model("city"); + var actual = m.$groupByClause( + select = "id", + include = "", + group = "id", + distinct = false, + returnAs = "objects" + ); + + expect(actual).toInclude(m.$quotedTableName() & "." & m.$quoteColumn("countyid")); + }); + + it("returns a well-formed empty query when paginated findAll matches zero rows on an aliased column", () => { + // Without $stripIdentifierQuotes, dialect quote chars from $createSQLFieldList leak into QueryNew column names. + var m = g.model("city"); + var rv = m.findAll(select = "id", where = "id = -1", page = 1, perPage = 25); + + expect(IsQuery(rv)).toBeTrue(); + expect(rv.recordCount).toBe(0); + expect(ListFindNoCase(rv.columnList, "id")).toBeGT(0); + }); + + }); + } +} diff --git a/web/sites/guides/src/content/docs/v4-0-1-snapshot/basics/models-and-the-orm.mdx b/web/sites/guides/src/content/docs/v4-0-1-snapshot/basics/models-and-the-orm.mdx index 68967c41f9..a4cbcaee1f 100644 --- a/web/sites/guides/src/content/docs/v4-0-1-snapshot/basics/models-and-the-orm.mdx +++ b/web/sites/guides/src/content/docs/v4-0-1-snapshot/basics/models-and-the-orm.mdx @@ -51,7 +51,7 @@ When the table, key, or column names don't match the defaults, override them in - `tableName("old_blog_entries")` — non-convention table name. - `setPrimaryKey("entryId")` — non-`id` primary key. - `dataSource("legacy_db")` — non-default datasource. See [Database and Multiple Datasources](/v4-0-1-snapshot/basics/database-and-multiple-datasources/) when this lands for the wider multi-database story. -- `property(name="publishedAt", column="pub_date")` — the property on the model is `publishedAt`, the column in the table is `pub_date`. The view and controller never see the column name. +- `property(name="publishedAt", column="pub_date")` — the property on the model is `publishedAt`, the column in the table is `pub_date`. The view and controller never see the column name. This is also the correct pattern when a column name is a SQL reserved word (`key`, `order`, `group`, etc.) — Wheels quotes all column identifiers in SELECT, GROUP BY, and ORDER BY clauses, so the alias stays clean while the generated SQL brackets the underlying column name for the target dialect. ```cfm {test:compile} component extends="Model" {