From f3af6611ed303bbeee451879048a19b63712691e Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Sat, 12 Sep 2026 20:01:43 -0700 Subject: [PATCH] fix(cli): scaffolded controllers 404 on a missing or soft-deleted record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit show(), edit(), update() and delete() called findByKey bare. It returns a non-object for BOTH a missing key and a soft-deleted row (`deletedAt` set), so the action operated on an empty value and the view threw — a 500 where a 404 belongs. Reproduced on a soft-deleted post. The `crud` and `api-resource` generators already guard this; the scaffold was the outlier. A `requireRecord` BEFORE filter now throws Wheels.RecordNotFound, which the framework maps to 404. It deliberately does not assign the record: the actions keep their conventional single-finder body, which the `--belongsTo` parent wiring depends on. ScaffoldSource.showInclude() only rewrites show() when the finder is its entire body, so an inline guard there silently disables the include= wiring. Loading twice is cheap and preserves that contract. Why three earlier attempts failed, so nobody repeats them: ScaffoldSource.scan() fails CLOSED on any interpolated string — a single `#params.key#` anywhere in the file returns valid=false, method() then cannot find show(), and the parent wiring reports "skipped". The guard's message was interpolating the key. It is now a plain string, and the generated file must scan; a new assertion pins that at the source so this cannot regress quietly. The filter's finder also uses the all-named `findByKey(key=params.key)`: a spec forbids the bare positional form anywhere in the controller, since the mixed positional+named shape is rejected at runtime. Verified on a generated app: soft-deleted /posts/12 and nonexistent /posts/99999 -> 404 (both were 500); /posts/12/edit -> 404; real /posts/1 -> 200 with its eager-loaded comments still listed, proving the wiring survives. CLI suite: 1356 pass, the 4 pre-existing DbCommandSpec failures, 0 errors. Complexity gate: PASS. Signed-off-by: Peter Amiri --- changelog.d/scaffold-404-guard.fixed.md | 1 + cli/lucli/templates/codegen/CRUDContent.txt | 28 +++++++++++++++ .../tests/specs/services/ScaffoldSpec.cfc | 34 +++++++++++++++++++ 3 files changed, 63 insertions(+) create mode 100644 changelog.d/scaffold-404-guard.fixed.md diff --git a/changelog.d/scaffold-404-guard.fixed.md b/changelog.d/scaffold-404-guard.fixed.md new file mode 100644 index 000000000..541bc7617 --- /dev/null +++ b/changelog.d/scaffold-404-guard.fixed.md @@ -0,0 +1 @@ +- Scaffolded controllers now return **404** for a key that matches no record, including a soft-deleted one. Previously `show`, `edit`, `update` and `delete` called `findByKey` bare, and a missing row made the action operate on an empty value — the view threw and the user saw a 500. A `requireRecord` before-filter now throws `Wheels.RecordNotFound`, which the framework maps to 404 diff --git a/cli/lucli/templates/codegen/CRUDContent.txt b/cli/lucli/templates/codegen/CRUDContent.txt index e034e6e03..fa83f15d3 100644 --- a/cli/lucli/templates/codegen/CRUDContent.txt +++ b/cli/lucli/templates/codegen/CRUDContent.txt @@ -8,6 +8,17 @@ */ function config() { super.config(); + // 404 rather than a 500 when the key matches nothing. findByKey returns a + // non-object for BOTH a missing key and a soft-deleted row, and the + // actions below then operate on that empty value — `post.title` in the + // view, or `.update()` here, throws. + // + // A BEFORE filter that deliberately does NOT assign the record: the + // actions keep their conventional single-finder body, which the + // `--belongsTo` parent wiring depends on — it only rewrites `show()` + // when the finder is the whole body, and skips a customized one. + // Loading twice is cheap and keeps that contract intact. + filters(through="requireRecord", only="show,edit,update,delete"); } /** @@ -81,4 +92,21 @@ redirectTo(route="|ObjectNamePlural|"); } + /** + * Guard: abort with a 404 when the key matches no record. + * + * The message is a plain string on purpose. The scaffold's parent wiring + * scans this file with ScaffoldSource, which refuses interpolated strings + * (`##...##`) wholesale — a single `##params.key##` here would silently turn + * off `include=` wiring for every `--belongsTo` child of this model. + **/ + private function requireRecord() { + if (!IsObject(model("|ObjectNameSingularC|").findByKey(key=params.key))) { + Throw( + type = "Wheels.RecordNotFound", + message = "|ObjectNameSingularC| not found for the requested key." + ); + } + } + } diff --git a/cli/lucli/tests/specs/services/ScaffoldSpec.cfc b/cli/lucli/tests/specs/services/ScaffoldSpec.cfc index 6fd26f898..286f1bedd 100644 --- a/cli/lucli/tests/specs/services/ScaffoldSpec.cfc +++ b/cli/lucli/tests/specs/services/ScaffoldSpec.cfc @@ -77,6 +77,40 @@ component extends="wheels.wheelstest.system.BaseSpec" { expect(content).toInclude("function delete()"); }); + it("guards every key-loading action against a missing record", () => { + // findByKey returns a non-object for BOTH a missing key and a + // soft-deleted row. Without a guard the action renders against + // that empty value and the view throws a 500 where a 404 + // belongs — reproduced on a soft-deleted post before the fix. + var result = scaffold.generateScaffold( + name = "Notefile", + properties = [{name = "title", type = "string"}], + force = true + ); + expect(result.success).toBeTrue(); + var content = fileRead(tempRoot & "/app/controllers/Notefiles.cfc"); + + // A BEFORE filter covering all four key-loading actions. + expect(content).toInclude('filters(through="requireRecord", only="show,edit,update,delete")'); + expect(content).toInclude("private function requireRecord()"); + expect(content).toInclude('if (!IsObject(model("Notefile").findByKey(key=params.key))) {'); + expect(content).toInclude('type = "Wheels.RecordNotFound"'); + + // The guard must NOT live inside show(): ScaffoldSource only + // rewrites show() when its finder is the whole body, so an + // inline guard there disables `--belongsTo` include= wiring. + var showBody = ReMatchNoCase("function show\(\) \{[^}]+\}", content); + expect(ArrayLen(showBody)).toBe(1); + expect(showBody[1]).notToInclude("IsObject"); + + // And the generated file must still SCAN. ScaffoldSource fails + // closed on any interpolated string, so a single `##...##` in + // the guard's message would silently switch off the parent + // wiring for every child of this model. Pin it at the source. + var scanned = new cli.lucli.services.ScaffoldSource().scan(content); + expect(scanned.valid).toBeTrue(scanned.reason); + }); + it("emits a full CRUD controller spec with model().create() test data", () => { var result = scaffold.generateScaffold( name = "Chronicle",