Repository navigation
fix(mapper): cross-engine null-safety for BoxLang strict null handling - #3212
Conversation
BoxLang throws a NullPointerException on a null string/struct subject where
Lucee and Adobe coerce null to ""/empty. Surfaced benchmarking the framework on
BoxLang via the perf bench app.
- mapper/matching.cfc: normalize a null derived `pattern` to "" after the
pattern/name guard. A named route given via `to=` with no explicit pattern
(or some resource forms) left `pattern` null at the Find/ReFindNoCase/concat
sites, failing to load on BoxLang at onApplicationStart.
- Dispatch.cfc $buildMiddlewareCgiScope(): default null request headers to {}
before iterating, so a null from the servlet host doesn't NPE the CGI build.
- events/EventMethods.cfc $runOnError(): guard a null exception `cause` before
StructKeyExists so the error handler itself doesn't NPE.
Adds mapperNullPatternSpec covering the named-`to=` and resources() route shapes
the compat-matrix demo app (.wildcard().root() only) left unexercised on
BoxLang. Behavior is unchanged on Lucee/Adobe (null already coerced); full core
suite green (4539). Note: this fixes route loading + these request-path sites;
BoxLang's live request path has further null-handling spots tracked separately.
Signed-off-by: Peter Amiri <petera@pai.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — This PR hardens three null-deref sites that NPE on BoxLang's strict null semantics where Lucee/Adobe coerce null to empty. The Dispatch.cfc and EventMethods.cfc guards are correct and clean. But the mapper/matching.cfc fix normalizes a null pattern to empty-string after the name-to-pattern derivation step, which means a named route gets an empty pattern on BoxLang instead of the hyphenized name every other engine produces — trading an NPE for a silent cross-engine behavioral divergence. The new spec's assertions are too weak to catch it. Verdict: request changes.
Cross-engine
vendor/wheels/mapper/matching.cfc:377 — null pattern is normalized to empty-string instead of the name-derived pattern, diverging from Lucee/Adobe.
The new guard sits after the name-to-pattern derivation block (line 359-361, if (!StructKeyExists(arguments, "pattern")) arguments.pattern = hyphenize(arguments.name);).
On BoxLang the unpassed optional pattern argument (declared on get()/post()/... and on $match() itself, forwarded via argumentCollection) surfaces as a present-but-null key — that is precisely why it reaches the downstream string ops as null per the PR description. Because the key exists, StructKeyExists(arguments, "pattern") is true, so the hyphenize(arguments.name) derivation at line 360 is skipped, and the new guard then sets the pattern to empty-string.
Result: .get(name="ping", to="main##ping") builds a route whose pattern is empty on BoxLang but /ping on Lucee/Adobe. The framework's own tests pin the correct value — MatchingSpec.cfc:49 asserts that the name=..., to=... shape yields expect(r[1].pattern).toBe("/sign-in") (i.e. / + hyphenize(name)). An empty pattern breaks both inbound URL matching (/ping no longer matches) and link generation (linkTo(route="ping") produces / instead of /ping) on BoxLang. This is the same class of cross-engine divergence the PR sets out to eliminate — it just converts a hard NPE into a silent wrong value.
For a named route the correct normalization is the hyphenized name, not empty-string. Strip the present-but-null key before the derivation block (before line 355) so the existing hyphenize path runs identically on every engine — e.g. if (StructKeyExists(arguments, "pattern") && IsNull(arguments.pattern)) StructDelete(arguments, "pattern");. This also lets the throw-guard at line 368 fire correctly when neither name nor a real pattern is supplied. (It likely also un-breaks the existing MatchingSpec assertions on the BoxLang matrix, which the current approach would convert from NPE to assertion-failure rather than to green.)
Tests
vendor/wheels/tests/specs/mapper/mapperNullPatternSpec.cfc:38-62 — assertions are too weak to catch the divergence above.
Both cases assert only expect(state.thrown).toBeFalse() and expect(ArrayLen(m.getRoutes())).toBeGT(0). With the current empty-string fix these pass green on BoxLang even though the derived route is wrong (empty pattern). Assert the actual pattern so the spec guards the real contract, mirroring MatchingSpec.cfc:49: expect(m.getRoutes()[1].pattern).toBe("/ping"). A pattern-value assertion would have surfaced the empty-pattern regression on BoxLang rather than masking it. (Minor: the file is named mapperNullPatternSpec.cfc; every sibling spec uses PascalCase — MapperNullPatternSpec.cfc — worth aligning.)
Correctness
The other two guards are correct:
Dispatch.cfc:484-486— defaulting a null/non-struct header source to an empty struct before iterating is sound; the!IsStructarm also covers any non-struct return defensively.EventMethods.cfc:54,65—&& !IsNull(arguments.exception.cause) && IsStruct(arguments.exception.cause)is inserted with correct short-circuit ordering (after theStructKeyExistscheck, before the nested.cause.rootCauseaccess), so a present-but-nullcauseno longer NPEs the error handler.
Commits / Docs
- Commit
fix(mapper): cross-engine null-safety for BoxLang strict null handlingis a valid conventional-commit header (typefix, scopemapper, well under 100 chars). - Changelog fragment
changelog.d/boxlang-null-safety.fixed.mduses the correct<slug>.<type>.mdfragment form (fixedis an allowed type) — no directCHANGELOG.mdedit. Good.
Once the matching.cfc normalization derives the hyphenized name (and the spec asserts it), this is a solid cross-engine hardening PR.
…g it Addresses the bot review on #3212. The previous normalization set a null `pattern` to "" AFTER the name->pattern derivation, so on BoxLang a named route given via to= (e.g. .get(name="ping", to="main##ping")) got an EMPTY pattern instead of the hyphenized name every other engine produces — trading the NPE for a silent cross-engine divergence that breaks URL matching and linkTo(). Root cause: on BoxLang an unpassed optional `pattern` arg is a present-but-null key, so StructKeyExists() is true and the hyphenize() derivation is skipped. Fix: strip the present-null `pattern` key BEFORE the derivation block, so the existing hyphenize() path runs identically on every engine and the "pattern or name required" guard still fires when neither is supplied. Strengthen the spec (now PascalCase MapperNullPatternSpec) to assert the derived pattern value (r[1].pattern == "/ping") mirroring MatchingSpec, so an empty-pattern regression fails on the BoxLang matrix rather than passing. Full core suite green (4539). Signed-off-by: Peter Amiri <petera@pai.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — This PR hardens three null-deref sites that NPE on BoxLang's strict null semantics where Lucee/Adobe coerce null to empty. The new commit c2570f8a8 addresses my prior CHANGES_REQUESTED review in full: the mapper/matching.cfc normalization now runs before the name→pattern derivation (so a named route derives hyphenize(name) on BoxLang exactly as on Lucee/Adobe, instead of being left empty), and the new spec asserts the concrete derived pattern rather than just "didn't throw." All blocking findings are resolved and no new ones fire. Verdict: approve.
Resolved findings (supersede audit, issue #3048)
My earlier review (marker ...:3212:09ef67441..., state CHANGES_REQUESTED) raised two blocking items. Both are now fixed:
-
mapper/matching.cfc— nullpatternwas normalized after derivation, leaving a named route empty on BoxLang. Resolved atvendor/wheels/mapper/matching.cfc:359-361:if (StructKeyExists(arguments, "pattern") && IsNull(arguments.pattern)) { StructDelete(arguments, "pattern"); }This now sits before the derivation block at
:369-371(if (!StructKeyExists(arguments, "pattern")) arguments.pattern = hyphenize(arguments.name);). Because the present-but-null key is deleted first,StructKeyExistsis false at:369, sohyphenize(arguments.name)runs and the route gets/pingon BoxLang — matching Lucee/Adobe. It also lets the:378"either pattern or name must be defined" throw fire correctly when neither is supplied. This is exactly the fix requested. -
Spec assertions were too weak to catch an empty-pattern regression. Resolved in
vendor/wheels/tests/specs/mapper/MapperNullPatternSpec.cfc. The named-to=case now asserts the concrete value —expect(r[1].pattern).toBe("/ping")plusname/controller/action— mirroringMatchingSpec.cfc:49(expect(r[1].pattern).toBe("/sign-in")for thename="signIn", to=...shape). Theresources()case asserts every route pattern is non-empty (expect(Len(route.pattern)).toBeGT(0)). An empty-pattern regression now fails rather than passing green. The file is also renamed to PascalCase (MapperNullPatternSpec.cfc), aligning with its siblings — my earlier minor nit.
Cross-engine
The remaining two guards were already correct in the prior review and are unchanged:
vendor/wheels/Dispatch.cfc:484-486— defaulting a null/non-struct header source to{}before iterating is sound; the!IsStructarm covers any non-struct servlet return defensively.vendor/wheels/events/EventMethods.cfc:54,65—&& !IsNull(arguments.exception.cause) && IsStruct(arguments.exception.cause)is ordered correctly (afterStructKeyExists, before the nested.cause.rootCauseaccess), so a present-but-nullcauseno longer NPEs the error handler.
StructDelete / StructKeyExists / IsNull / IsStruct are all cross-engine safe, and the new code adds no Lucee- or Adobe-only idioms.
Commits
fix(mapper): derive named-route pattern on BoxLang instead of emptying it — valid conventional-commit header (type fix, scope mapper, ≤100 chars, not ALL-CAPS), and the subject states the "why."
Docs
Changelog fragment changelog.d/boxlang-null-safety.fixed.md uses the correct <slug>.<type>.md form (fixed is allowed) with no direct CHANGELOG.md [Unreleased] edit. Good.
Solid cross-engine hardening. Approving.
…ngelog Address wheels-bot review on #3212: - EventMethods.cfc $runOnError dereferenced arguments.exception.cause.rootCause.type after only guarding `cause`, so a present-but-null `rootCause` (BoxLang strict null semantics) still NPE'd — the same null-deref class this PR hardens (reproduced on real BoxLang 1.11.0). Add symmetric !IsNull + IsStruct + StructKeyExists(...,"type") guards before the deref in both branches. - The changelog fragment described the OLD, superseded approach ("normalizes a null derived pattern to ''"). Rewrite it to match the shipped code: matching.cfc removes the present-null `pattern` key before derivation so a named route derives its pattern from its name. Also reflect the new rootCause guard and the PascalCase MapperNullPatternSpec filename. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Amiri <petera@pai.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — This PR hardens cross-engine null-deref sites that NPE on BoxLang's strict null semantics where Lucee/Adobe coerce null to ""/empty. The new head commit fe4cb64e1 (a) extends the EventMethods.cfc $runOnError() guard to also cover a present-but-null rootCause (not just cause), and (b) rewrites the changelog fragment to describe the shipped matching.cfc approach. Both my prior blocking findings (mapper/matching.cfc derivation order + weak spec assertions) remain resolved at head, and the new commit introduces no new blocking issues. Verdict: approve — this also supersedes my earlier still-active CHANGES_REQUESTED review (auto-dismissed by the new push), clearing the merge block (issue #3048).
Resolved findings (supersede audit, issue #3048)
My first review (marker ...:3212:09ef67441..., state CHANGES_REQUESTED, still active because the new push auto-dismissed my interim approval) raised two blocking items. Both are fixed and remain fixed at head fe4cb64e1:
-
mapper/matching.cfc— nullpatternwas normalized after derivation, leaving a named route empty on BoxLang. Resolved atvendor/wheels/mapper/matching.cfc:359-361:if (StructKeyExists(arguments, "pattern") && IsNull(arguments.pattern)) { StructDelete(arguments, "pattern"); }This sits before the derivation block at
:368-370(if (!StructKeyExists(arguments, "pattern")) arguments.pattern = hyphenize(arguments.name);). The present-null key is deleted first, soStructKeyExistsis false at:368,hyphenize(arguments.name)runs, and a named route gets/pingon BoxLang — matching Lucee/Adobe. It also lets the:377"Either 'pattern' or 'name' must be defined" throw fire when neither is supplied. -
Spec assertions were too weak to catch an empty-pattern regression. Resolved in
vendor/wheels/tests/specs/mapper/MapperNullPatternSpec.cfc:45-48— the named-to=case now asserts the concrete value (expect(r[1].pattern).toBe("/ping")plusname/controller/action), mirroringMatchingSpec.cfc:49. Theresources()case asserts every route pattern is non-empty (:56,expect(Len(route.pattern)).toBeGT(0)). The file is PascalCase, aligned with siblings — my earlier minor nit.
Correctness
The new commit's additional guard is correct:
vendor/wheels/events/EventMethods.cfc:56-58(BoxLang branch) and:70-72(non-BoxLangcause.rootCausebranch) — the deref ofarguments.exception.cause.rootCause.typeis now guarded symmetrically with!IsNull(...) && IsStruct(...) && StructKeyExists(..., "type"), in correct short-circuit order (after the existingcauseguards, before theLeft(...)read). A present-but-nullrootCause(BoxLang strict null) no longer NPEs the error handler — the same null-deref class the rest of the PR hardens.Left(..., 6)uses a constant length (not0), so it is safe on Lucee 7 (Cross-Engine Invariant #8).
The two earlier guards remain correct:
vendor/wheels/Dispatch.cfc:486-488— defaulting a null/non-struct$requestHttpHeaders()return to{}before iterating is sound; the!IsStructarm covers any non-struct servlet return defensively.
Cross-engine
StructDelete / StructKeyExists / IsNull / IsStruct are all cross-engine safe; the new code adds no Lucee- or Adobe-only idioms. The StructKeyExists(arguments, "pattern") && IsNull(...) shape correctly handles BoxLang's present-but-null key (where StructKeyExists returns true) while remaining a no-op on Lucee/Adobe (where the optional arg is simply absent).
Commits
fix(events): guard present-null rootCause in $runOnError; correct changelog — valid conventional-commit header (type fix, scope events, ≤100 chars, not ALL-CAPS), and the subject states the "why." Sign-off trailer present.
Docs
Changelog fragment changelog.d/boxlang-null-safety.fixed.md uses the correct <slug>.<type>.md form (fixed is allowed) with no direct CHANGELOG.md [Unreleased] edit. The rewrite now accurately describes the shipped behavior (removes the present-null pattern key before derivation; guards null/non-struct cause and rootCause), correcting the prior fragment that still described the superseded empty-string approach. Good.
Solid cross-engine hardening. Approving and clearing the prior block.
Summary
BoxLang throws
NullPointerExceptionon a null string/struct subject where Lucee/Adobe coerce null to""/empty. Surfaced while benchmarking the framework on BoxLang (via the cross-engine perf app). These are real cross-engine robustness fixes; behavior is unchanged on Lucee/Adobe.Changes
mapper/matching.cfc— normalize a null derivedpatternto""after the pattern/name guard. A named route given viato=with no explicit pattern (e.g..get(name="ping", to="main##ping")), or someresources()forms, leftarguments.patternnull at theFind/ReFindNoCase/concat sites →onApplicationStartfailed to load routes on BoxLang. Validated:onApplicationStartnow completes on BoxLang.Dispatch.cfc$buildMiddlewareCgiScope()— default null request headers to{}before iterating (a null from the servlet host NPE'd the CGI-scope build).events/EventMethods.cfc$runOnError()— guard a null exceptioncausebeforeStructKeyExistsso the error handler doesn't itself NPE.Tests
vendor/wheels/tests/specs/mapper/mapperNullPatternSpec.cfcexercises the named-to=andresources()route shapes. The compat-matrix demo app uses only.wildcard().root(), so these shapes were unexercised on BoxLang — this closes that gap (the spec runs in the matrix).Scope note
This fixes route loading on BoxLang plus the dispatch/error-handler sites above. BoxLang's live request path (middleware pipeline + downstream) has further null-handling spots — the framework broadly assumes Lucee/Adobe null→
""coercion. Fully hardening it is a follow-up best driven by extending the compat-matrix to exercise the live request path on BoxLang (today it spec-injects request context, bypassing the live-servlet null path).Type
Changelog
changelog.d/boxlang-null-safety.fixed.md