fix(seed): make wheels seed --generate create rows and report honest success - #3107
Conversation
…success The CLI seed bridge's generate loop iterated $classData().properties (a struct keyed by property name) as if it were an array of property structs, so prop.name threw 'there is no property with name [NAME] found in [string]'. Every model errored, zero rows were created, yet the run still returned success=true and the CLI printed 'Seeding completed.' with exit 0 (the #2987 honesty fix had only covered convention mode). Move the generate path into a dedicated, unit-tested wheels.Seeder.generateSeeds(models, count) method that iterates the property struct correctly and forces overall success=false when any model fails or no rows are created, so the CLI surfaces a non-zero exit. The cli.cfm generate branch now delegates to it and the duplicated page-level generateTestData() helper is removed. Fixes #3082 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — No doc updatesReviewed this PR's diff and found no docs that need updating (bug fix restoring already-documented behavior — |
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR: This PR fixes the #3082 defect for real — generateSeeds() iterates $classData().properties as the struct it is (verified against prior art at vendor/wheels/model/sql.cfc:1363, which accesses .properties[name].column), and the honest-failure contract plus the three new specs are solid. However, the new result envelope introduces a CLI-side regression: it carries totalCreated without totalSkipped, and the CLI's success printer interpolates result.totalSkipped whenever totalCreated exists — so a successful wheels seed --generate now throws a key-not-exist error and exits non-zero, failing this PR's own acceptance criterion. One-line fix. Verdict: request changes — submitted as a comment-state review only because GitHub forbids a REQUEST_CHANGES event on the review author's own PR (this PR was opened by wheels-bot); treat finding 1 as blocking.
Correctness
-
[Blocking] Successful generate runs now crash the CLI printer — missing
totalSkippedkey (vendor/wheels/Seeder.cfc:249-256).- The
generateSeeds()result struct issuccess, mode, seeded, totalCreated, totalFailed, message— nototalSkipped. vendor/wheels/public/views/cli.cfm:923—StructAppend(result, generateResult, true)merges it verbatim into the bridge JSON, andparseCliResponse()(cli/lucli/Module.cfc:6451) returns the struct verbatim on success.cli/lucli/Module.cfc:4030-4031:if (structKeyExists(result, "totalCreated")) { out("Seeded: #result.totalCreated# created, #result.totalSkipped# skipped", "green"); }- Before this PR, the generate path never emitted
totalCreated(the removed inline loop incli.cfmonly setseeded+message), so generate mode always took theelsebranch. After this PR,structKeyExists(result, "totalCreated")is true, the interpolation ofresult.totalSkippedthrows on the bundled Lucee, and — becauseparseCliResponsethrows onsuccess:falsebefore this line — the crash fires only on successful runs. The command seeds the database, then errors out. - Affected commands:
wheels seed --generate,wheels seedwhen auto mode resolves to generate (no seed files), andwheels db reset --force(callsrunSeed("auto", "")atModule.cfc:4070) on any app without seed files — the fresh-app default. - Fix: add
totalSkipped = 0to the result struct ingenerateSeeds()(Seeder.cfc:249-256), which also keeps the generate envelope congruent with the convention envelope (runSeeds()always returnstotalSkipped). A defensiveresult.totalSkipped ?: 0inModule.cfc:4031would be a sensible companion, but that file isn't in this diff. - Why the test run didn't catch it: the new specs unit-test
Seeder.generateSeeds()directly; nothing exercises the bridge →parseCliResponse()→ printer path (the checklist ran the core suite only, nottools/test-cli-local.shor a livewheels seed --generate). Worth either an end-to-end CLI check or a spec asserting the generate result contains every keyrunSeed()prints (totalCreated,totalSkipped).
- The
-
[Nit, non-blocking]
count <= 0produces a self-contradictory failure message.val(sp.count)(cli.cfm:917) turns a non-numericcountinto0;generateSeeds(count=0)then yieldsentrySuccess = (0 == 0)→ true (Seeder.cfc:287),totalFailed = 0,totalCreated = 0→success = false(Seeder.cfc:308), and the failure message renders as "Created 0 records; 0 of 1 models failed ()" (Seeder.cfc:312) — reporting failure while saying zero models failed, with an empty parenthetical from$failedGenerateSummary(). Suggest an explicit guard:count < 1→ early return with a clear message.
Tests
- [Nit, non-blocking] Happy-path spec cleanup can leak rows into later specs. In
vendor/wheels/tests/specs/seederSpec.cfc:248-253the Author-row cleanup runs after the expectations, so a failing expectation leaves the generated rows behind for subsequent specs that touchc_o_r_e_authors. The established pattern in this file is a defensive pre-clean before acting (see the rollback spec atseederSpec.cfc:73-77). Otherwise the three specs are well-targeted: real-row verification, error-entry honesty, and the delimiter-only-list edge are all meaningful regressions to pin.
What checks out
- Struct-iteration fix verified:
$classData().propertiesis keyed by property name; calculated properties live separately in.calculatedProperties(vendor/wheels/model/properties.cfc:805-811), so the Author test model's SQL-backednumberofitemsis correctly excluded from generated records. - Cross-engine: internal helpers follow the
public+$-prefix convention; ternary-inside-string-interpolation has shipped prior art in this same file (Seeder.cfc:97);Left(file, 1)is safe (DirectoryList names are non-empty, so no Lucee 7Left(str, 0)trap); no reserved-scope names, bracket-notation calls, orfinallyloops introduced. - Behavior cleanup is a net win: the old loop marked a model
success = trueeven when everysave()failed validation;entrySuccess = (seededCount == arguments.count)closes that hole too. - Docs/metadata: changelog fragment present and correctly named (
changelog.d/3082-cli-seed-generate.fixed.md); guides/AI-docs deferred tobot-update-docs.ymlper the established flow. - Commits: single commit
fix(seed): make wheels seed --generate create rows and report honest successconforms to commitlint (valid type, scope, subject length/case).
…pped to the bridge result
Two review criticals on the generateSeeds() extraction:
1. $resolveGenerateModels("") included app/models/Model.cfc — the
framework base class every scaffolded app ships. model("Model")
throws Wheels.TableNotFound, and under the new honesty rule
(success requires zero failures) a blank-models
'wheels seed --generate' run could never exit 0 on a conventional
app. Auto-scan now skips Model.cfc, matching the CLI's own model
enumeration (Analysis.cfc / Module.cfc); explicit lists pass
through verbatim.
2. The generate result struct carried totalCreated but not
totalSkipped, while Module.cfc::runSeed() prints
'#result.totalSkipped# skipped' whenever totalCreated exists — so
a SUCCESSFUL generate run threw 'element TOTALSKIPPED is
undefined' in the CLI. The result now always includes
totalSkipped = 0.
Specs cover both: the success-path spec asserts totalSkipped exists
and is 0, and new $resolveGenerateModels specs assert the Model.cfc
exclusion and verbatim explicit lists.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR: Re-review at head 973e9e6b4. The previous review's blocking finding is resolved: generateSeeds() now carries totalSkipped = 0 (vendor/wheels/Seeder.cfc:258), satisfying the CLI printer contract at cli/lucli/Module.cfc:4030-4031, and the head commit additionally excludes the base Model.cfc from auto-scan — both spec-pinned. One new cross-engine finding in the test spec: the chained ValueList(model("Author").findAll(select = "id").id) form has no prior art anywhere in this repo and is at risk on Adobe CF, and the PR-gated CI only exercises this spec on Lucee. Effective verdict: request changes on that one trivial fix (hoist the query into a variable). Submitted as a comment-state review only because GitHub forbids a REQUEST_CHANGES event on the review author's own PR (this PR was opened by wheels-bot) — treat finding 1 as the blocking item.
Resolved from the previous review (on c6ebef1a6)
- Missing
totalSkipped(was blocking) — fixed.generateSeeds()now initializestotalSkipped = 0in its result struct (vendor/wheels/Seeder.cfc:254-258, with a comment naming the printer contract), soModule.cfc:4031'sout("Seeded: #result.totalCreated# created, #result.totalSkipped# skipped", ...)no longer throws on a successful generate run. The success-path spec pins the key's presence and value (vendor/wheels/tests/specs/seederSpec.cfc:239-240). - New in this head: base
Model.cfcexcluded from auto-scan — correct. Without it,model("Model")throwsWheels.TableNotFoundon every scaffolded app and the honesty rule would force every blank-models run to fail. The exclusion (Seeder.cfc:347) matches the CLI's own enumeration — verified prior art atcli/lucli/services/Analysis.cfc:100andcli/lucli/Module.cfc:1345— and CFML's case-insensitive!=also coversmodel.cfcon case-sensitive filesystems. Both behaviors are spec-pinned: auto-scan exclusion (seederSpec.cfc:292-303) and verbatim explicit lists (seederSpec.cfc:306-312).
Cross-engine
- [Blocking] Chained
ValueList()on afindAll()return is unproven on Adobe CF — hoist the query into a variable (vendor/wheels/tests/specs/seederSpec.cfc:227and:251).local.beforeIds = ValueList(model("Author").findAll(select = "id").id);- These two lines are the only chained-call
ValueList()usages in the entire repo (grep over all CFC/spec sources). The established idiom assigns the query first:var sample = g.model("author").findAll(...); var idList = ValueList(sample.id);(vendor/wheels/tests/specs/model/adapterBaseQueryPathSpec.cfc:14-16). - On Adobe CF,
qry.colin a general expression position evaluates to the current row's scalar value —ValueList()works through the engine's special handling of the dottedquery.columnreference form, which a chained method-call argument does not fit. Lucee is proven green (theLucee 7 + SQLiteleg passed at this head), but the PR-gated Adobe legs are smoke-only (boot + HTTP) and never executeseederSpec— an Adobe incompatibility here would surface as a post-merge compat-matrix failure, the failure class #3029/#3051 exists to keep out ofdevelop. I could not run an Adobe engine in this session, so this is flagged as unverified-on-Adobe risk rather than an observed crash. - Fix (two lines, matches prior art):
(same for
local.beforeQuery = model("Author").findAll(select = "id"); local.beforeIds = ValueList(local.beforeQuery.id);afterIdsat line 251). Alternatively, verify green on a live adobe2023 engine per the CLAUDE.md curl check and say so on the PR — either resolution is acceptable.
- These two lines are the only chained-call
Conventions
- [Nit, carried over, non-blocking]
count <= 0still yields a self-contradictory failure message.val(sp.count)(cli.cfm:917) turns a non-numeric count into0; thenentrySuccess = (0 == 0)is true (Seeder.cfc:292),totalFailed = 0,totalCreated = 0→success = false(Seeder.cfc:313), and the message renders "Database seeding failed. Created 0 records; 0 of 1 models failed ()" (Seeder.cfc:317) — failure with zero failed models and an empty parenthetical. An explicitcount < 1early return would clean this up.
Tests
- [Nit, carried over, non-blocking] Happy-path cleanup runs after the expectations (
seederSpec.cfc:254-259), so a failing expectation leaks the two generated Author rows into later specs. The established pattern in this file is a defensive pre-clean before acting (seederSpec.cfc:73-77). Otherwise the five specs are well-targeted: real-row verification,totalSkippedcontract, error-entry honesty, the delimiter-only-list edge, and both$resolveGenerateModelsbehaviors.
What checks out
- Envelope merge is sound:
runDbSeed()initializes{success = true, mode = "auto", message = ""}(cli.cfm:878) andStructAppend(result, generateResult, true)(cli.cfm:924) overwritessuccess/message/modewith the honest values;parseCliResponse()then throwsWheels.Cli.CommandFailed→ non-zero exit onsuccess:false. - Cross-engine elsewhere: internal helpers follow
public+$-prefix (Invariant 7); catch blocks only mutate the outerresultstruct (the Invariant-11-safe pattern); ternary-inside-interpolation has in-file prior art (Seeder.cfc:97);ArrayFindNoCaseandtoBe([...])array equality both have shipped prior art (parallelRunnerSpec.cfc:56); no closures, bracket-notation calls, orfinallyloops introduced. - Commits: both conform to commitlint (
fix(seed): …, headers 76 and 95 chars, sentence case), and DCO sign-offs match the git authors (c6ebef1a6→ claude[bot];973e9e6b4→ Peter Amiri). The second commit body documents both review fixes with the why. - Docs/metadata: changelog fragment present and correctly named (
changelog.d/3082-cli-seed-generate.fixed.md); guides/AI-docs deferred tobot-update-docs.ymlper the established flow; PR checklist honestly reports the local core-suite run (4424 pass) including the failing-first capture.
…tions metadata lock (#3152) * fix(controller): memoize $getStatusCodes, hoist column tokenization, log $includeFile blanked columns Closes the three rendering residuals of the #2961 roll-up (C13/C16/C17): - C17: $getStatusCodes() rebuilt a 63-entry constant struct on every render path. It is now built once per application lifetime and memoized in the application scope together with a deterministic reverse (text-to-code) lookup; $returnStatusCode() reads that lookup instead of running StructFindValue over the rebuilt struct, and duplicated status texts (Unassigned at 427/430/509) deterministically resolve to the lowest code. The numeric branch of $setRequestStatusCode() keeps its validation call but no longer assigns the unused text. - C16: $includeFile() re-ran ListToArray(query.columnList) inside both per-row loops; the column list is constant per query so it is tokenized once above the loops. - C13: the blanket catch that blanked a column $includeFile() could not read now logs a warning (once per column per render) to the wheels log naming the column, partial, first failing row, and underlying error before defaulting to an empty string. Refs #2961 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com> * fix(model): lock the context-independent metadata fill-ins in $expandedAssociations The metadata fill-ins above the JOIN-variant memo (foreign/join keys, table name, column/property lists and structs) wrote the shared application-scoped association struct on every call without a lock — the same unlocked-shared-struct pattern #2910 fixed for the JOIN string itself, flagged in the #2952 coverage-audit comment. They are now filled once under the same double-checked named lock (wheelsJoinMemo), with a fill-once marker written last so lock-skipping readers only ever observe a fully populated metadata set. The values are derived solely from class data, so fill-once is equivalent to the previous per-call rewrite. Refs #2952 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com> * test: fix Adobe-only compile crashes in sendFile and seeder specs Adobe CF validates built-in argument counts and ValueList() operands at COMPILE time, and the core runner compiles every spec in the directory, so each of these crashed the entire Adobe 2023 suite (0 specs run): - miscellaneousSpec.cfc (#3101): DirectoryCreate(path, true) — the createPath boolean is Lucee-only; Adobe's DirectoryCreate takes exactly one parameter. Three call sites, all with existing parents, switched to the single-argument form. - seederSpec.cfc (#3107): ValueList(model(...).findAll(...).id) — Adobe only accepts a plain query.column reference inside ValueList(). The query is assigned to a variable first. Verified: full core suite on Adobe 2023 + SQLite goes from a whole-suite onRequest compile error to 4462 pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com> --------- Signed-off-by: Peter Amiri <peter@alurium.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What changed
wheels seed --generatewas non-functional: the CLI seed bridge's generate loop invendor/wheels/public/views/cli.cfmiterated$classData().properties— a struct keyed by property name — as if it were an array of property structs, soprop.namethrewthere is no property with name [NAME] found in [string]. Every model errored, zero rows were created, yet the run still returnedsuccess=trueand the CLI printed "Seeding completed." with exit 0 (the #2987 honesty fix had only covered convention mode).This moves the generate path into a dedicated, unit-tested
wheels.Seeder.generateSeeds(models, count)method that:$classData().propertiesstruct correctly (keys → metadata), so rows are actually created; andsuccess=falsewhen any model throws, fewer than the requested rows save, or no models resolve — so the bridge JSON'ssuccess:falsemakesparseCliResponse()throwWheels.Cli.CommandFailed→ non-zero exit instead of silently lying.The
cli.cfmgenerate branch now delegates togenerateSeeds(), and the duplicated page-levelgenerateTestData()helper is moved intoSeeder.cfcas$generateTestData(the old copy is removed). Acceptance criterion met:wheels seed --generatenow either creates rows and exits 0, or errors loudly with a non-zero exit.Fixes #3082
Type of change
Checklist
vendor/wheels/tests/specs/seederSpec.cfcgains agenerateSeeds()describe block: a failing-then-passing spec proving (a) real rows are created and honest success is reported, (b) a model that can't be seeded forces overallsuccess=falsewith a recorded error, and (c) an explicit list that resolves to no models reports failure rather than silent success.bot-update-docs.yml)bot-update-docs.yml)bot-update-docs.yml)changelog.d/3082-cli-seed-generate.fixed.mdseederSpecbundle 20 passed, 0 failed. Captured the failing state first (3 errors:No matching function [generateSeeds]), then green after implementation.