fix(model): guard the scope dereference in validatesUniquenessOf - #3355
fix(model): guard the scope dereference in validatesUniquenessOf#3355bpamiri wants to merge 1 commit into
Conversation
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — This PR fixes #3350: validatesUniquenessOf(property="x", scope="y") threw Component [Model] has no accessible Member with name [y] when the scope property was never assigned, because $buildWhereClausePart() dereferenced this[property] with no existence guard. The fix — treating an absent scope property as blank via StructKeyExists — is correct, idiomatic, and well-covered by red-first specs. Verdict: comment. The framework change is clean; however the PR also commits 7 unrelated generated TestBox MockBox stub artifacts that should be dropped before merge. No blocking correctness / cross-engine / security issue, so this does not block, but a maintainer should strip the stray files.
Conventions
Stray generated artifacts — 7 TestBox MockBox stub files unrelated to the fix. The diff adds:
public/testbox/system/stubs/139184C0543CCDB338AEFB643110CB89.cfm (tmp_closeSSEStream_…)
public/testbox/system/stubs/16CA394252B074478BEBD2B3A9C8EA82.cfm (tmp_sendSSEComment_…)
public/testbox/system/stubs/30942F4D0BCB6139072EF27C66218715.cfm (tmp_checkError_…)
public/testbox/system/stubs/754BEF48E30B63BC11E518FA73C07785.cfm (tmp_sendSSEEvent_…)
public/testbox/system/stubs/B7681C49C5125395612F24D97559A4C2.cfm (tmp_initSSEStream_…)
public/testbox/system/stubs/C383511C024245809F1F73E0CA52E220.cfm (tmp_poll_…)
public/testbox/system/stubs/CDCB73D6FD17909D6519A49548FD6769.cfm (tmp_closeSSEStream_…)
These are MockBox runtime-generated method stubs (hash-named, containing _mockResults / _mockMethodCallCounters machinery) for SSE mocking — they have nothing to do with the validatesUniquenessOf scope guard and leaked in from a local test run with mocking enabled. They are build artifacts and should not be tracked. Recommendation:
git rmthe 7 files from this PR (474 of the 475 added lines are these stubs), and- add
public/testbox/system/stubs/to.gitignoreso they can't leak again.
Note there is already one such file on develop (F952D54F1096E25C030C8E3149ABD8C4.cfm) — that is a pre-existing accidental commit, not a precedent to follow; the ignore rule would let a follow-up clean that one up too.
Correctness
No issues. $buildWhereClausePart() (vendor/wheels/model/validations.cfc:795) now reads:
local.value = StructKeyExists(this, arguments.property) ? this[arguments.property] : "";Treating absent as "" is semantically right: the blank value flows through $quoteValue() and, for numeric types, the existing ='' → IS NULL conversion at line 800 — so an absent numeric scope collapses to IS NULL exactly as a present-but-empty one always did. The reasoning that only scope= reaches this code absent (the validated property is short-circuited by $shouldInvokeValidation()) checks out against key() / validationTypeForProperty() in the same file, which already guard optional-property reads with StructKeyExists(this, …) (lines 545, 615, 908, 918). The guard mirrors established prior art in the same layer.
Tests
Well done — genuinely red-first. vendor/wheels/tests/specs/model/validationsSpec.cfc adds two specs against the existing CombiKey fixture (no new fixture/table needed):
validatesUniquenessOf_with_absent_scope_property—StructDelete(combiKey, "id2")then assertsvalid()returns rather than throwing (the crash path).validatesUniquenessOf_absent_scope_matches_blank_scope— assertsabsent.$buildWhereClausePart("id2") == blank.$buildWhereClausePart("id2"), pinning the equivalence of absent and blank rather than merely the absence of a crash. This is the stronger assertion and correctly captures the actual defect (the split between the two states).
Commits
fix(model): guard the scope dereference in validatesUniquenessOf — valid conventional-commit header (64 chars, allowed fix type, optional model scope), DCO Signed-off-by trailer present, message explains the "why" thoroughly. No violations.
Docs
Changelog fragment changelog.d/3350-uniqueness-scope-guard.fixed.md present with a correct fixed-type bullet — the fragment-file convention is followed (no direct CHANGELOG.md edit). Good.
18af4fb to
b0e2e90
Compare
`validatesUniquenessOf(property="x", scope="y")` threw Component [MyModel] has no accessible Member with name [y] whenever `y` had never been assigned — out of a method whose entire job is to return a validation result. A caller that correctly wraps `save()` in an `if` still got an exception, and it surfaced as a 500. `$buildWhereClausePart()` read `this[arguments.property]` with no existence guard. That is easy to hit because absent and empty are different states here: `$setDefaultValues()` only seeds properties that have an explicit `property()` mapping, so a column with a database-level default but no mapping is missing from a `new()`-ed object rather than present-and-blank. The `scope=` in the model source looks fine; nothing in it hints that the property will be absent at validation time. Only scopes were exposed. The validated property itself cannot reach this code absent — `$shouldInvokeValidation()` returns false when it is missing from the object — which is why the split went unnoticed: "unset" and "set to empty" produced an exception and a validation result respectively, for the same logical state. An absent scope property is now treated as blank, so both states take the identical path, including the existing conversion of an empty numeric scope to `IS NULL`. The guard mirrors `validationTypeForProperty()` and `key()` in the same layer, which already read optional properties this way. Red-first: 945 pass / 0 fail / 2 errors, both errors being the reported message verbatim against the existing `CombiKey` fixture (`validatesUniquenessOf(property="id1", scope="id2")`). Verification, lucee7 + sqlite, full core suite: develop ab901cf 4732 pass / 0 fail / 0 error this branch 4734 pass / 0 fail / 0 error Exactly +2, the new specs. Closes #3350 Signed-off-by: Peter Amiri <peter@alurium.com>
b0e2e90 to
6952e7b
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — This PR fixes #3350: validatesUniquenessOf(property="x", scope="y") threw Component [Model] has no accessible Member with name [y] out of a validation method whenever the scope property was absent (not merely blank), surfacing as a 500. The fix adds a StructKeyExists existence guard in $buildWhereClausePart() so an absent scope property is treated as blank, and two red-first specs pin the behavior. Correct, well-scoped, cross-engine-safe, and properly tested — approve.
Correctness
The core change is sound. vendor/wheels/model/validations.cfc:795:
local.value = StructKeyExists(this, arguments.property) ? this[arguments.property] : "";This preserves the original this[arguments.property] read verbatim when the key exists and only substitutes "" when absent — so an unset scope property now flows into the existing empty-numeric → IS NULL branch (:800-802) instead of throwing. The comment's claim that the validated property can never reach this code absent checks out: $shouldInvokeValidation() (:538) gates that path. No null deref, no off-by-one, and property is not a reserved scope name, so no shadowing.
Cross-engine
Clean. StructKeyExists(this, ...) plus a ternary are universally supported across Lucee 5/6/7, Adobe 2018–2025, and BoxLang — no .map() member collision, no application-scope function member, no reserved-scope parameter, no bracket-call-in-closure. The one-line guard has no adapter or SQL-dialect surface, so skipping the matrix dispatch is defensible here.
Tests
Both new specs (vendor/wheels/tests/specs/model/validationsSpec.cfc:1027, :1042) extend wheels.WheelsTest (BDD, not RocketUnit) and reuse the existing CombiKey fixture (validatesUniquenessOf(property="id1", scope="id2")), so no new table or fixture was needed. StructDelete(combiKey, "id2") faithfully reproduces the absent-property state. The second spec is the stronger one — asserting absent.$buildWhereClausePart("id2") == blank.$buildWhereClausePart("id2") pins the equivalence the bug violated, not just the absence of a crash. Both were red before the fix (the +2 pass delta lines up with the PR body).
Docs
Changelog fragment changelog.d/3350-uniqueness-scope-guard.fixed.md is present and correctly named (<slug>.fixed.md, not a direct CHANGELOG.md edit). This is framework-internal behavior with no user-facing guide surface, so no guides update is warranted.
Commits
fix(model): guard the scope dereference in validatesUniquenessOf conforms to commitlint (valid type, header well under 100 chars, not ALL-CAPS) and is DCO signed off.
Minor observations (non-blocking)
- The PR deletes a stale TestBox MockBox stub (
public/testbox/system/stubs/F952D54F...cfm). Nothing references these hash-named generated artifacts by name, so removing a committed one is harmless cleanup. Worth a passing note that these stub files are build artifacts and ideally would not be tracked at all, but that is out of scope for this fix.
Nice work — the red-first evidence and the equivalence-pinning spec make this easy to trust.
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — This PR fixes issue 3350: validatesUniquenessOf(property="x", scope="y") threw "Component [Model] has no accessible Member with name [y]" out of a validation method whenever the scope property was absent (not present-and-empty). The fix is a single existence guard in $buildWhereClausePart() that treats an absent scope property as blank, unifying it with the present-but-empty path. Minimal, idiomatic, red-first tested, and changelogged — approve.
Correctness
The core change is sound. vendor/wheels/model/validations.cfc:795 replaces the unguarded read with: local.value = StructKeyExists(this, arguments.property) ? this[arguments.property] : "". The old str = this[arguments.property] had no existence guard, so an absent scope column dereferenced to a member-not-found exception. Treating absent as "" is correct: a numeric scope flows into the existing empty-to-IS-NULL conversion at lines 800-802, and a string scope matches present-but-empty behavior, so the two states are now equivalent — the right semantics for a uniqueness scope. The bounding claim checks out: the validated property cannot reach this code absent because $shouldInvokeValidation() short-circuits first, so the guard only ever fires for scope= entries. Both callers (validations.cfc:743, :749) pass through unchanged.
Conventions
The guard is the established idiom in this layer — StructKeyExists(this, ...) guards appear throughout vendor/wheels/model/ (create.cfc:238,263; nestedproperties.cfc:57,96,169,192; miscellaneous.cfc:440). No mixed positional/named args, no reserved-scope shadowing. The comment at :786-794 accurately documents the $setDefaultValues() absent-vs-empty distinction.
Cross-engine
No concern. Engine-independent CFML: a StructKeyExists guard and a ternary, no adapter/SQL-dialect surface, no .map() member calls, no closures-as-constructor-args, no reserved-scope hazards. StructDelete(obj, "key") in the new specs operates on a model instance this scope, which Lucee/Adobe/BoxLang all support. Skipping the matrix dispatch is defensible.
Tests
Red-first and precise. validationsSpec.cfc:1027 (validatesUniquenessOf_with_absent_scope_property) exercises the crash path — new(id1=1, id2=1) then StructDelete(combiKey, "id2") makes id2 genuinely absent, asserting valid() returns rather than throws. The spec at :1037 pins the equivalence directly via $buildWhereClausePart("id2"), stronger than a bare no-crash assertion. Both reuse the existing CombiKey fixture.
Docs
Changelog fragment present and correctly named: changelog.d/3350-uniqueness-scope-guard.fixed.md (.fixed type, no direct CHANGELOG.md edit). Accurately describes the behavior change and retires the property(name=..., defaultValue="") workaround.
Commits
fix(model): guard the scope dereference in validatesUniquenessOf — conforms to commitlint (valid type fix, valid scope model, subject under 100 chars, not ALL-CAPS). Single commit, message reflects the why.
Security
No new surface. Values still route through the adapter $quoteValue(); the guard only substitutes a literal empty string for an absent key, introducing no injection vector.
1 participant
Closes #3350.
The bug
validatesUniquenessOf(property="x", scope="y")threwwhenever
yhad never been assigned — out of a method whose entire job is to return a validation result. A caller that correctly wrapssave()in anifstill got an exception, and it surfaced as a 500.$buildWhereClausePart()readthis[arguments.property]with no existence guard.Why it is easy to hit
Absent and empty are different states here.
$setDefaultValues()only seeds properties that have an explicitproperty()mapping, so a column with a database-level default but no mapping is missing from anew()-ed object rather than present-and-blank. Nothing in the model source hints at it — thescope=looks fine.Only scopes were exposed
Worth stating, because it bounds the fix: the validated property cannot reach this code absent.
$shouldInvokeValidation()returnsfalsewhen the property is missing from the object, so the validation never runs. That is why the split went unnoticed — "unset" and "set to empty" produced an exception and a validation result respectively, for the same logical state.The fix
An absent scope property is treated as blank, so both states take the identical path — including the existing conversion of an empty numeric scope to
IS NULL. This is the fix suggested in the issue.The guard mirrors
validationTypeForProperty()andkey()in the same layer, which already read optional properties this way. Theproperty(name="<scopeProperty>", defaultValue="")workaround is no longer needed.Red-first
945 pass / 0 fail / 2 errors— both errors are the reported message verbatim, against the existingCombiKeyfixture (validatesUniquenessOf(property="id1", scope="id2")), so no new fixture or table was needed:Two specs: one that the validation returns a result rather than throwing, and one that an absent scope and a blank scope produce the same WHERE fragment — pinning the equivalence rather than just the absence of a crash.
Verification
lucee7 + sqlite, full core suite:
ab901cff7Exactly +2 — the new specs, nothing else moved.
No matrix dispatch on this one: the change is a
StructKeyExistsguard in engine-independent CFML with no adapter or SQL-dialect surface, and it is covered by the standard PR checks. Say the word if you would rather I run it anyway.🤖 Generated with Claude Code