Made change to allow models to update primary key values#15
Made change to allow models to update primary key values#15dhumphreys wants to merge 1 commit intowheels-dev:new-masterfrom
Conversation
This was a problem in some places where numeric or character IDs needed to be changed, but Wheels was not putting the properties in the SET for the UPDATE to run.
|
Say you have a model, users, with a primary key of 'username' and an additional 'password' field. user = model("user").findByKey("dhumphreys");
user.update(username="dhumphreys2");The update will go through without error, but the 'username' primary key is not actually updated. The changed piece of code was keeping any properties that are part of the primary key from ever being updated. In some situations, such as when an auto incremented numeric id is used, this is appropriate. However, there are cases where some applications may need to update primary key values, especially in tables with composite keys. My change allows any property to be updated, as long as it has actually been changed in the model object. |
|
don, you're part of core now so you can commit this directly. I still think there are some areas with this patch that we need to consider such as security. rails doesn't allow you to update primary keys through direct access or mass assignment. personally i would like to have some sort of setting this feature to be turned on both globally and at the model level. |
|
Yes, I know I can commit directly. I just knew that this issue required some discussion, so I have not merged it yet. I also already had the changes in my own branch and saw no reason to put an extra feature branch in the main wheels repository. We could enable this with a setting pretty easily. It only really comes into play when not using an auto increment integer as primary key. |
|
looking at this again, i'm still thinking that this can cause quite a bit of havoc and security problems. including this patch would allow any primary key to be updated through mass assignment. the only way around that would be to use protectedProperties() and state the primary keys as protected properties on each model, which would be a pain. this still leaves the problem though of when you actually need to update a primary key. for that a work around is to use the updateAll() method which doesn't have the restrictions of using update(). |
TestBox silently skips bundles it cannot instantiate (e.g. CFML parse errors) — its JSON response shows totalPass: 0 with no failures and no errors, indistinguishable from "all clear" or "no specs found." The fresh-VM tutorial run lost ~10 minutes when an unescaped `#` inside a CSS selector crashed Lucee's parser silently. The user only discovered the broken spec by loading /wheels/app/tests in a browser. Add countSpecsOnDisk() and listSpecsOnDisk() helpers to TestRunner that walk the project's filesystem under variables.projectRoot and return *Spec.cfc counts/names as dotted bundle names. Wire them into Module.cfc::displayTestResults: if disk count exceeds TestBox's loaded bundle count, emit a "WARN N spec file(s) failed to compile and were silently skipped:" block listing the unloaded paths, and append "X failed to load" to the summary line. The exit code is unchanged — strict-loading mode is intentionally out of scope. The new code path is best-effort: any probe error is logged in verbose mode and never crashes the test report. Closes finding #2 in docs/superpowers/plans/2026-04-29-fresh-vm-onboarding-findings.md (subsumes April 19 #15). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Records the four CLI fixes landed in batch B (gitkeep, migrate output, test runner compile-error surfacing, reload hint) and adds a Batch D shipped entry now that PR #2361 is merged. Marks #7 closed via the out-of-band PR #2360 (Rails-style argument order). Crosses out April 19's #15 ("test runner output format needs verification") which is subsumed by 2026-04-29 finding #2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(docs): add batch B plan for CLI output polish Plan covers findings #2, #3, #8, and a new sub-finding (.gitkeep files not copied by scaffolder) from the 2026-04-29 fresh-VM onboarding triage. Implementation lands in subsequent commits on this branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli): copy .gitkeep files so empty test dirs survive git commit copyTemplateDir() previously skipped any file named .gitkeep with a comment that they "exist only to keep empty dirs in git" — but skipping them meant the scaffolded app's tests/specs/{controllers,functional, models}/ directories vanished on first git commit, contradicting the tutorial's chapter 1 file tree. Same problem hits app/lib, app/jobs, app/mailers, public/{files,images,javascripts,stylesheets}, and other intentionally-empty directories that ship with .gitkeep markers in the template tree (14 in total). Copy .gitkeep files byte-for-byte (no placeholder processing — they're empty by design). Extend NewCommandTemplateSpec to assert the .gitkeep files exist on disk for three representative paths. Note on test coverage: a deeper test that scaffolds via the production copyTemplateDir code path would require reflective invocation of a private method on Module.cfc; the existing template-existence check plus CI's full integration run cover the regression sufficiently. Closes the new sub-finding from docs/superpowers/plans/2026-04-29-fresh-vm-onboarding-findings.md (top of the "Shipped" section, surfaced during batch A's Task 0 reconnaissance). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(migration): emit CRLF (not bare CR) so migrator output renders correctly Migrator.cfc and migrator/Base.cfc::announce() built their output using bare Chr(13). On macOS, Linux, and the LuCLI out() pipe, bare CR moves the cursor to column 0 without advancing the line, so subsequent text overwrites. On a fresh `wheels migrate latest`, the section header, divider, and per-table summary all collapsed onto a single line where the tutorial promised three. Switch every CR to CRLF (Chr(13) & Chr(10)) — 29 occurrences in Migrator.cfc plus the single line in migrator/Base.cfc::announce(). Add MigratorOutputSpec to pin the announce() contract so future migrator hacks can't drop the LF again. Update the existing "is appending announcements" assertion in migrationSpec.cfc that was previously asserting the buggy bare-CR concatenation. Closes finding #3 in docs/superpowers/plans/2026-04-29-fresh-vm-onboarding-findings.md Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli): surface specs that fail to compile in wheels test output TestBox silently skips bundles it cannot instantiate (e.g. CFML parse errors) — its JSON response shows totalPass: 0 with no failures and no errors, indistinguishable from "all clear" or "no specs found." The fresh-VM tutorial run lost ~10 minutes when an unescaped `#` inside a CSS selector crashed Lucee's parser silently. The user only discovered the broken spec by loading /wheels/app/tests in a browser. Add countSpecsOnDisk() and listSpecsOnDisk() helpers to TestRunner that walk the project's filesystem under variables.projectRoot and return *Spec.cfc counts/names as dotted bundle names. Wire them into Module.cfc::displayTestResults: if disk count exceeds TestBox's loaded bundle count, emit a "WARN N spec file(s) failed to compile and were silently skipped:" block listing the unloaded paths, and append "X failed to load" to the summary line. The exit code is unchanged — strict-loading mode is intentionally out of scope. The new code path is best-effort: any probe error is logged in verbose mode and never crashes the test report. Closes finding #2 in docs/superpowers/plans/2026-04-29-fresh-vm-onboarding-findings.md (subsumes April 19 #15). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli): hint at cold reload from wheels reload output `wheels reload` re-fires the framework reload path (?reload=true) but does NOT re-run onApplicationStart — surprising to users coming from Rails or Django where restart is the default. Append a one-line cyan note pointing readers at `wheels stop && wheels start` whenever they need init code to re-execute. Pairs with the chapter 6 doc fix in batch A: the contract is now visible at both surfaces a fresh-VM user encounters (the auth tutorial and the CLI itself). Closes finding #8 in docs/superpowers/plans/2026-04-29-fresh-vm-onboarding-findings.md Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(docs): mark batch B + D items shipped, close April 19 #15 Records the four CLI fixes landed in batch B (gitkeep, migrate output, test runner compile-error surfacing, reload hint) and adds a Batch D shipped entry now that PR #2361 is merged. Marks #7 closed via the out-of-band PR #2360 (Rails-style argument order). Crosses out April 19's #15 ("test runner output format needs verification") which is subsumed by 2026-04-29 finding #2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was a problem in some places where numeric or character IDs needed to be changed, but Wheels was not putting the properties in the SET for the UPDATE to run.
Based on my findings, the hasChanged() call should be a sufficient check to decide whether or not to update any property value. The $addKeyWhereClause() function called from within $update() even considers that primary key values may have been changed. It doesn't make sense to not allow the model to send these changed values to the SQL server if the application required it.
This is reopening pull request #9, which I accidentally closed.