fix(model): quote column identifiers in SELECT clause builder (#2787)

* fix(model): quote column identifiers in SELECT clause builder

The WHERE and ORDER BY clause builders already routed column names
through the adapter's $quoteIdentifier, but $createSQLFieldList — the
SELECT/GROUP BY engine — appended the column part raw. Models backed
by tables with reserved-word column names (e.g. `key`, `order`,
`group`) blew up on `findAll`/`findOne`/dynamic finders with cryptic
SQL syntax errors as soon as the SELECT list mentioned the column.

Also strips quote chars from the property extracted by the
duplicate-column rename loop so the alias replacement still matches
the unquoted ` AS <alias>` form, and updates the empty-pagination
columnList extraction in read.cfc to strip identifier quotes before
stripping the table prefix.

Fixes #2784

Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>

* docs(web/guides): note reserved-word column support via property alias in models guide

Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>

* fix(model): address Reviewer A/B consensus findings (round 1)

- Condense 4-line block comments at vendor/wheels/model/read.cfc:217
  and vendor/wheels/model/sql.cfc:634 to single-line comments
  (CLAUDE.md: "Never write multi-paragraph docstrings or multi-line
  comment blocks — one short line max").
- Stop using $quoteColumn() for the table-name argument in
  vendor/wheels/tests/specs/model/reservedColumnQuotingSpec.cfc;
  switch to the model's public $quotedTableName() helper so the spec
  names match what each helper actually quotes.
- Add a zero-row paginated findAll spec to
  reservedColumnQuotingSpec.cfc that exercises the QueryNew branch in
  vendor/wheels/model/read.cfc:225 with an aliased column, covering
  the path the original spec did not reach.
- Mention ORDER BY alongside SELECT and GROUP BY in
  web/sites/guides/src/content/docs/v4-0-1-snapshot/basics/models-and-the-orm.mdx
  so readers do not infer ORDER BY is unsafe with reserved-word
  columns (ORDER BY already routes through $quoteIdentifier).

Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>

* fix(model): address Reviewer A/B consensus findings (round 2)

Condense the remaining multi-line comment blocks in reservedColumnQuotingSpec.cfc
to single lines per CLAUDE.md ("Never write multi-paragraph docstrings or
multi-line comment blocks — one short line max"):

- vendor/wheels/tests/specs/model/reservedColumnQuotingSpec.cfc:10 — 3-line
  block about City's id -> countyid alias condensed.
- vendor/wheels/tests/specs/model/reservedColumnQuotingSpec.cfc:18 — 3-line
  block about Author.firstName (property == column) condensed.
- vendor/wheels/tests/specs/model/reservedColumnQuotingSpec.cfc:40 — 6-line
  block added in round 1 inside the zero-row pagination it() condensed to
  the single-line form Reviewer A supplied.

The line-30 GROUP BY comment was already single-line; A's "30-32" citation
was off-by-one for that one. No production code changed; pure comment-style
fix. Test totals unchanged at 4 pass / 0 fail in the spec; full model suite
remains 839 pass / 0 fail / 0 error / 11 skipped across 35 bundles.

Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>

---------

Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: Peter Amiri <peter@alurium.com>

Author: 3 people

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 <name>` is now a transparent alias for `wheels packages add <name>` 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 <name>` 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)

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);

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;
 							}

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", () => {

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);
+			});
+
+		});
+	}
+}

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" {