🐛 bug: let empty optional params yield to later required segments - #4722

Walkthrough

The route matcher retries non-greedy optional parameters as empty when later matching fails. Tests cover optional parameter captures and confirm that non-empty values that violate constraints do not match.

Changes

Optional Parameter Matching

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 89752

The matcher now handles successive empty optional parameters while preserving existing constraint-failure behavior. No actionable merge-blocking regression remains established; merge after normal checks pass.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 89752

The change expands acceptance for affected optional-parameter routes while preserving rejection of invalid non-empty constrained values. Retry work is bounded and retry state is local to each invocation. No security bypass was established, but application-specific route combinations and custom-validator behavior remain uncertain.

Retained concerns
No architecture-level concerns identified.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{A rabbit checks the paths at night,
And hops where optional words take flight.
If later steps cannot align,
It clears one choice and tries the line.
But broken bounds still end the run,
Then curls beneath the moonlit sun.}

_{Comment @coderabbitai help to get the list of available commands.}

@00200200 Remove optional_empty_param_test.go and merge the test into existing files.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.03%. Comparing base (28f2aa7) to head (a6d7e62).
⚠️ Report is 66 commits behind head on main.

@@            Coverage Diff             @@
##             main    #4722      +/-   ##
==========================================
+ Coverage   94.98%   95.03%   +0.05%     
==========================================
  Files         141      141              
  Lines       17580    17816     +236     
==========================================
+ Hits        16698    16932     +234     
- Misses        554      556       +2     
  Partials      328      328              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@ReneWerner87 @gaby Thank you for the thorough review and pointers! Addressed in 959a2e3:

1. Fast-path loop preserved: getMatch now runs the original loop first. For standard routes and routes without optional segments, performance is identical to main (0 extra overhead or allocations).
2. Fallback only for optionals: If a route contains an optional parameter and fails its initial greedy match, it falls back to matchBacktrack.
3. No constraint bypass: Constraint failures on non-empty values return false immediately in both passes without retrying empty.
4. Bounded backtracking: Backtracking state is bounded to a single retry point so consecutive optionals do not cause exponential retry growth.
5. Test consolidation: Removed optional_empty_param_test.go and moved the constraint regression test into router_test.go using http.NoBody.

Extracted the repeated backtrack stack restore block into a local helper closure with explicit bounds guards and fixed the linter warnings in 897525a.

@ReneWerner87 Thank you for the insightful benchmark analysis and pointers! Addressed in 3306218:

1. canRetry fast path: getMatch now sets canRetry only when segment.IsOptional && !segment.IsGreedy && i > 0. If no optional parameter consumed input, route misses return false immediately without entering matchBacktrack.
2. Greedy wildcards preserved: Added !segment.IsGreedy to the backtrack stack recording so greedy wildcards (*) do not yield to subsequent segments. Added Test_RouteParser_GreedyWildcardDoesNotYield.
3. Bound documented and tested: Named maxBacktrackRetries = 16, added documentation, and added Test_RouteParser_BacktrackMaxRetriesBound.
4. Description updated: Updated the PR summary to reflect the current test files, canRetry fast path, and retry bound.

@00200200 I pushed two commits on top of your branch.

16c1b22: a miss on a route with an optional parameter before a constant still walked the route twice (GET /api/health against four /api/:version?/... routes: 498 ns vs 229 ns on main). That retry cannot succeed, because findParamLen ends the parameter at the first occurrence of the constant. Only an optional parameter directly before another parameter can change the result by yielding, so those routes now go to matchBacktrack up front and getMatch is main's loop again. The case above is at 236 ns. Retries that cannot succeed no longer count against the bound, so /pre:a?:b?:c?:d?:e?-end now matches /pre-end; exceeds_limit got a sixth optional.

a6d7e62: the Copilot finding, see the thread.

Checked against main and the previous head over 41,370 generated patterns x 19,608 paths, with and without partial matching: no match lost or changed.

_{a6d7e62 vs main@6bd5c5f · 1915/1936 results compared · 21 new, 0 gone · retest: 0/4 regressions, 1/2 improvements reproduced · noise-aware thresholds · full results · github.com/gofiber/fiber/v3}

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord