Skip to content

Commit 1c9915e

Browse files
bpamiriclaudegithub-actions[bot]
authored
fix(router): normalize Java named capture groups in route constraints (#2989)
* fix(router): normalize Java named capture groups in route constraints A constraint containing (?<name>...) still counted as a capturing group in $mergeRoutePattern's match-position arithmetic, silently shifting every subsequent route variable's value on java-regex engines — and legacy-regex engines rejected the syntax outright at match time. Extend $nonCapturingConstraint's char-class-aware scanner to rewrite the whole (?<name> opener to (?:, leaving lookbehinds ((?<=, (?<!) and literals inside character classes untouched. Fixes #2976 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com> * fix(router): address Reviewer A/B consensus findings (round 1) - Add spec asserting Wheels.InvalidRegex is thrown at draw time when a named-capture constraint also backreferences the name (e.g. `(?<n>\d+)\k<n>`). After normalization the group disappears but the `\k<n>` backref remains, so Pattern.compile() rejects it and $compileRegex rethrows. Closes the loop on issue #2976's second acceptance criterion from the PR description, which was previously asserted only in the changelog. - Replace the unnecessary `##` escape in the existing `(issue ##2976)` comment with a single `#`. CFML's `#` sigil is special only in double-quoted strings and output contexts; in `//` comments inside a script-mode cfc it is a literal character, so `##2976` rendered as two hash signs in IDEs, git output, and GitHub diffs. Cosmetic and test-only — no production code change. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> * fix(router): address Reviewer A/B consensus findings (round 2) Remove the `local.` prefix from `constraintMapper` in the backref fail-fast spec (`vendor/wheels/tests/specs/mapperModernSpec.cfc` lines 407 and 417). Each anonymous closure has its own `local` scope on Adobe CF, so `local.constraintMapper` inside the `expect(function() { ... })` closure was unbound and would throw an undefined-variable error before `whereMatch` could fail with `Wheels.InvalidRegex`, defeating the spec. The unscoped assignment goes to the CFC-level `variables` scope, which inner closures inherit — the same pattern used by the sister "invalid regex throws at draw time" spec just above (line 341). Reference: CLAUDE.md Anti-Pattern 10. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> * chore(docs): move changelog entry to changelog.d fragment Eliminates the [Unreleased]-anchor merge conflicts across campaign PRs; fragments are assembled into CHANGELOG.md at release promotion. 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> Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
1 parent d654586 commit 1c9915e

3 files changed

Lines changed: 76 additions & 8 deletions

File tree

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
- Route constraints containing a Java named capture group (`whereMatch("year", "(?<yr>20\d{2})")`) no longer corrupt route matching. `$nonCapturingConstraint` now normalizes the whole `(?<name>` opener to `(?:`, the same treatment anonymous capturing groups already receive — previously a named group both shifted every subsequent route variable's value in `$mergeRoutePattern`'s positional extraction (java-regex engines) and threw `Sequence (?<...) not recognized` at match time (legacy-regex engines). Lookbehinds (`(?<=`, `(?<!`) and parenthesis runs inside character classes are left untouched; a constraint that also backreferences the name (`\k<name>`) fails fast at draw time via `$compileRegex` (#2976)

‎vendor/wheels/Mapper.cfc‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -163,14 +163,32 @@ component output="false" {
163163
local.rv &= local.char;
164164
continue;
165165
}
166-
if (
167-
!local.escaped
168-
&& local.char == "("
169-
&& local.charClassDepth == 0
170-
&& (local.i == local.length || Mid(arguments.pattern, local.i + 1, 1) != "?")
171-
) {
172-
// Unescaped capturing group outside any character class: make it non-capturing.
173-
local.rv &= "(?:";
166+
if (!local.escaped && local.char == "(" && local.charClassDepth == 0) {
167+
if (local.i == local.length || Mid(arguments.pattern, local.i + 1, 1) != "?") {
168+
// Unescaped capturing group outside any character class: make it non-capturing.
169+
local.rv &= "(?:";
170+
} else {
171+
// `(?` opens a non-capturing construct ((?:, (?=, (?!, (?<=, (?<!)
172+
// — EXCEPT a Java named capturing group `(?<name>`, which still
173+
// counts in the positional group arithmetic $mergeRoutePattern
174+
// relies on (and which legacy CFML regex engines reject outright).
175+
// Normalize the whole `(?<name>` opener to `(?:`; lookbehinds have
176+
// `=` or `!` after `(?<` and are left untouched (issue #2976).
177+
// A constraint that also backreferences the name (`\k<name>`)
178+
// fails fast at draw time via $compileRegex.
179+
local.namedGroup = ReFind(
180+
"^\(\?<[A-Za-z][A-Za-z0-9]*>",
181+
Mid(arguments.pattern, local.i, local.length - local.i + 1),
182+
1,
183+
true
184+
);
185+
if (local.namedGroup.pos[1] == 1) {
186+
local.rv &= "(?:";
187+
local.i += local.namedGroup.len[1] - 1;
188+
} else {
189+
local.rv &= local.char;
190+
}
191+
}
174192
} else {
175193
local.rv &= local.char;
176194
}

‎vendor/wheels/tests/specs/mapperModernSpec.cfc‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -369,6 +369,55 @@ component extends="wheels.WheelsTest" {
369369
expect(ArrayLen(local.matches.pos)).toBe(3)
370370
})
371371

372+
it("rewrites Java named capture groups in constraints to non-capturing groups", function() {
373+
local.mapper = $mapper()
374+
.$draw()
375+
.get(name="namedArchive", pattern="archive/[year]/[month]", to="archive##show")
376+
.whereMatch("year", "(?<yr>20\d{2})")
377+
.end()
378+
379+
local.routes = local.mapper.getRoutes()
380+
local.route = local.routes[1]
381+
expect("archive/2024/05").toMatch(local.route.regex)
382+
383+
// Java named groups still count in the positional group arithmetic
384+
// $mergeRoutePattern relies on, so they must be normalized away
385+
// exactly like anonymous capturing groups — otherwise [month]
386+
// would silently receive the year's value (issue #2976).
387+
local.matches = ReFind(local.route.regex, "archive/2024/05", 1, true)
388+
expect(ArrayLen(local.matches.pos)).toBe(3)
389+
})
390+
391+
it("normalizes named groups but preserves lookbehinds in $nonCapturingConstraint", function() {
392+
local.mapper = $mapper()
393+
394+
// Named capturing group opener is replaced wholesale with `(?:`.
395+
expect(local.mapper.$nonCapturingConstraint("(?<yr>20\d{2})")).toBe("(?:20\d{2})")
396+
397+
// Lookbehinds start with `(?<` too but are NOT capturing groups —
398+
// the rewrite must leave them untouched.
399+
expect(local.mapper.$nonCapturingConstraint("\w+(?<!tmp)")).toBe("\w+(?<!tmp)")
400+
expect(local.mapper.$nonCapturingConstraint("\w+(?<=pdf)")).toBe("\w+(?<=pdf)")
401+
402+
// Inside a character class, `(?<x>` is a run of literal characters.
403+
expect(local.mapper.$nonCapturingConstraint("[(?<x>)]")).toBe("[(?<x>)]")
404+
})
405+
406+
it("throws Wheels.InvalidRegex when a normalized named group leaves a dangling backref", function() {
407+
constraintMapper = $mapper()
408+
.$draw()
409+
.get(name="backrefs", pattern="items/[id]", to="items##show")
410+
411+
// After normalization, `(?<n>\d+)` becomes `(?:\d+)`, so the
412+
// trailing `\k<n>` backref points at a group name that no longer
413+
// exists. java.util.regex.Pattern.compile() rejects this, and
414+
// $compileRegex rethrows it as Wheels.InvalidRegex at draw time
415+
// — closing the loop on issue #2976's second acceptance criterion.
416+
expect(function() {
417+
constraintMapper.whereMatch("id", "(?<n>\d+)\k<n>")
418+
}).toThrow("Wheels.InvalidRegex")
419+
})
420+
372421
it("leaves parentheses inside character classes untouched", function() {
373422
local.mapper = $mapper()
374423
.$draw()

0 commit comments

Comments
 (0)