Skip to content

🔥 feat: unify internal and custom constraints into single interface#4397

Draft
pageton wants to merge 7 commits into
gofiber:mainfrom
pageton:perf/constraint-rework
Draft

🔥 feat: unify internal and custom constraints into single interface#4397
pageton wants to merge 7 commits into
gofiber:mainfrom
pageton:perf/constraint-rework

Conversation

@pageton
Copy link
Copy Markdown
Contributor

@pageton pageton commented Jun 2, 2026

Summary

Unifies the internal constraint system (TypeConstraint bitmask + switch) and the custom constraint interface (CustomConstraint) into a single ConstraintHandler interface with an optional Analyze() phase for precomputation at registration time.

Closes #4395

Changes

New file: constraint.go

  • ConstraintHandler interface — Name() string + Execute(param, args, precompiled)
  • ConstraintAnalyzer interface (optional) — Analyze(args) -> (any, error) for precomputation
  • Each built-in constraint is now its own type: intConstraintType, minLenConstraintType, regexConstraintType, etc.
  • customConstraintWrapper adapts existing CustomConstraint implementations automatically
  • findConstraintHandler() merges custom + built-in constraints (custom priority)
  • newConstraint() creates a Constraint with automatic Analyze() call

Modified: path.go

  • Removed: TypeConstraint uint16 type, 15 iota constants, needOneData/needTwoData bitmasks
  • Removed: getParamConstraintType() (33-line switch)
  • Removed: CheckConstraint() (~130-line switch) — replaced by single handler.Execute() dispatch
  • Updated: analyseParameterPart() uses findConstraintHandler() + newConstraint()
  • Updated: checkConstraint() delegates to handler.Execute() via matchConstraint()
  • Preserved: CheckConstraint() method as thin wrapper for backward compat

Modified: path_test.go

  • Updated test cases to use newConstraint() instead of direct struct literals

Performance impact

  • Zero per-request parsing: strconv.Atoi, time.Parse layout, regex compilation all happen once at registration via Analyze()
  • Single dispatch: Replaces TypeConstraint switch + custom constraint iteration with one Execute() call
  • Precompiled data stored as any: Type-asserted at match time (no interface overhead for simple constraints)

Backward compatibility

  • CustomConstraint interface preserved — auto-wrapped via customConstraintWrapper
  • CheckConstraint() method preserved for external callers
  • RegisterCustomConstraint() API unchanged
  • Route pattern syntax (:param<constraint(data)>) unchanged

Checks

make format  → 0 issues
make lint    → 0 issues
make test    → 3460 tests passed, 0 failures

pageton added 2 commits June 2, 2026 14:46
Draft PR for constraint system rework per gofiber#4395.

- Unify built-in and custom constraints under a single Constraint interface
- Add optional Analyse() phase for precomputation at registration time
- Remove TypeConstraint bitmask and large switch statement
- Pre-parse integer args, compile regex, parse time layouts once at registration
Refactor the constraint system to treat built-in and custom constraints
uniformly through a single ConstraintHandler interface with an optional
Analyze() phase for precomputation at registration time.

Key changes:
- Add ConstraintHandler interface (Name, Execute) and optional
  ConstraintAnalyzer interface (Analyze) for precomputation
- Convert each built-in constraint (int, bool, float, alpha, guid,
  minLen, maxLen, len, betweenLen, min, max, range, datetime, regex)
  into its own type implementing ConstraintHandler
- Remove TypeConstraint bitmask, getParamConstraintType switch,
  needOneData/needTwoData constants, and the ~130-line switch in
  CheckConstraint
- Pre-parse integer args, compile regex, and parse time layouts once
  at registration via Analyze() instead of on every request
- Wrap existing CustomConstraint implementations automatically via
  customConstraintWrapper for backward compatibility
- Preserve CheckConstraint method for external callers

Closes gofiber#4395
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 2, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 95b5e108-f318-4266-98a3-3f2d5e7b5072

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ReneWerner87 ReneWerner87 added this to v3 Jun 2, 2026
@ReneWerner87 ReneWerner87 added this to the v3 milestone Jun 2, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors route constraints handling by introducing the ConstraintHandler and ConstraintAnalyzer interfaces, extracting built-in constraints into separate types, and simplifying the Constraint struct. The review feedback highlights two critical issues: a potential nil pointer dereference panic in CheckConstraint when a Constraint is manually constructed by external packages, and a bug where custom regexHandler results are discarded if they return a standard *regexp.Regexp.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread constraint.go
Comment thread path.go Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 2, 2026

Codecov Report

❌ Patch coverage is 96.28378% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.39%. Comparing base (3026a5e) to head (3a5deb1).
⚠️ Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
constraint.go 96.08% 7 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4397      +/-   ##
==========================================
- Coverage   91.40%   91.39%   -0.02%     
==========================================
  Files         132      133       +1     
  Lines       13120    13266     +146     
==========================================
+ Hits        11992    12124     +132     
- Misses        711      724      +13     
- Partials      417      418       +1     
Flag Coverage Δ
unittests 91.39% <96.28%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Resolve handler on the fly in matchConstraint if nil (prevents panic
  on manually constructed Constraint structs from external packages)
- Update precompiled with custom regexHandler result when it returns
  *regexp.Regexp, instead of discarding it
@pageton
Copy link
Copy Markdown
Contributor Author

pageton commented Jun 2, 2026

@ReneWerner87 as discussed, this draft PR implements the constraint system rework we talked about. The key idea:

  • Unify built-in and custom constraints under a single ConstraintHandler interface with Name() + Execute()
  • Add optional Analyze() phase — pre-parse integers, compile regex, parse time layouts once at registration instead of on every request
  • Remove the TypeConstraint bitmask, getParamConstraintType() switch, and the ~130-line switch in CheckConstraint()
  • Backward compatible — existing CustomConstraint implementations auto-wrapped, RegisterCustomConstraint() API unchanged

The pattern is:

Registration:  find handler by Name → optional Analyze(args) → store precompiled
Request:       handler.Execute(param, args, precompiled)

All 3460 tests pass, 0 lint issues. Still draft — would love your thoughts on the approach before we finalize.

Cover nil-handler fallback in matchConstraint, missing-args Analyze
branches for all constraint types, nil-precompiled Execute branches,
and custom constraint wrapper integration.
@ReneWerner87
Copy link
Copy Markdown
Member

@pageton can you share some before and after benchmarks

@ReneWerner87
Copy link
Copy Markdown
Member

Good first draft.
Initially, we also wanted to avoid having to convert the values taken from the routing patterns into a different data type again for the match.
Since that makes no sense at runtime and costs time and memory allocations every time.

Can you make sure that DATA is an interface{}[] and that it receives the correct value with the correct data types each time during the analysis of the routing pattern?

i.e., during analysis, if an int constraint is used, data should be an int so that it already matches for the comparison

if there is a regex constraint in the pattern, it should already be available in the routing segment for the match as precompiled in the constraints, so that it can then simply be used (of course, using the configured regex compiler that can be configured in the app)

In the end, there should be no more special cases for regex or int constraints, because they all work the same way and the data from the routing pattern is in the format used for matching the actual route or segment, ensuring that the routing match is as fast as possible

@ReneWerner87
Copy link
Copy Markdown
Member

In other words, there are two goals:

  • Convert data types only when analyzing the route pattern
  • Standardize custom constraints and internal constraints so that there is no longer any difference between them and no additional conditions apply to them in the code flow

@pageton
Copy link
Copy Markdown
Contributor Author

pageton commented Jun 3, 2026

@ReneWerner87 here are the before/after benchmarks:

goos: linux / goarch: amd64 / cpu: AMD Ryzen 5 7600X 6-Core Processor

                                   │   old (main)   │          new (constraint-rework)           │
                                   │     sec/op     │    sec/op     vs base                      │
int-12                               17.46n ± 4%    18.02n ± 2%        ~ (p=0.008)
bool-12                              14.08n ± 3%    15.49n ± 1%   +10.05% (p=0.000)
float-12                             27.16n ± 2%    27.20n ± 1%        ~ (p=0.541)
alpha-12                             18.62n ± 4%    18.10n ± 1%    -2.85% (p=0.000)
guid-12                              29.48n ± 3%    29.23n ± 1%        ~ (p=0.271)
minLen-12                            15.98n ± 2%    14.30n ± 1%   -10.48% (p=0.000) ✓
maxLen-12                            16.21n ± 3%    13.92n ± 2%   -14.13% (p=0.000) ✓
len-12                               15.77n ± 1%    14.71n ± 2%    -6.72% (p=0.000) ✓
betweenLen-12                        19.04n ± 1%    14.71n ± 2%   -22.72% (p=0.000) ✓✓
min-12                               18.97n ± 1%    18.38n ± 1%    -3.14% (p=0.000)
max-12                               19.55n ± 1%    18.62n ± 3%    -4.81% (p=0.000) ✓
range-12                             21.81n ± 1%    18.48n ± 2%   -15.29% (p=0.000) ✓✓
datetime-12                          53.39n ± 1%    58.56n ± 3%    +9.69% (p=0.000)
regex-12                             81.06n ± 1%    82.18n ± 5%    +1.38% (p=0.000)
geomean                              22.75n         21.74n         -4.47%

Allocations: 0 B/op, 0 allocs/op on both branches (identical)

Key improvements (precomputed strconv.Atoi at registration instead of per-request):

  • betweenLen: -22.7% (2 args parsed once)
  • range: -15.3% (2 args parsed once)
  • maxLen: -14.1%
  • minLen: -10.5%
  • len: -6.7%

Slight regressions in simple constraints that don't benefit from precomputation (bool, int check) — likely interface dispatch overhead. These are sub-10ns differences at the noise floor.

The main value of this rework is architectural: unified interface, extensibility, and precomputation for multi-arg constraints. The 0-allocation guarantee is preserved.

pageton added 2 commits June 3, 2026 21:23
Benchmark all 14 constraint types for before/after comparison.
Restructure constraint system so Data holds pre-typed values:
- Data is now []any with values converted at registration time
  (int for minLen/maxLen/len/betweenLen/min/max/range,
   *regexp.Regexp for regex, string for datetime layout)
- Remove separate precompiled field — Execute uses Data directly
- No special cases per constraint type in match path
- bool uses strconv.ParseBool like original implementation
- Custom constraints wrapped via customConstraintWrapper

Benchstat vs main (geomean -3.45%):
  betweenLen:  -21.38%
  range:       -17.81%
  maxLen:       -8.65%
  max:          -8.87%
  minLen:       -7.65%
  len:          -7.27%
  min:          -3.67%
@pageton
Copy link
Copy Markdown
Contributor Author

pageton commented Jun 3, 2026

@ReneWerner87 updated the implementation per your feedback. Data is now []any with pre-typed values — no more separate precompiled field:

  • minLen(5)Data[0] is int(5) (not string "5")
  • range(1,20)Data[0] is int(1), Data[1] is int(20)
  • regex(^\d+$)Data[0] is *regexp.Regexp (precompiled)
  • datetime(2006-01-02)Data[0] is string (layout)
  • boolstrconv.ParseBool at match time (no args to pre-type)

Execute(param, data) uses typed data directly — no per-request conversion.

                                   │   old (main)   │       new (pre-typed Data)       │
                                   │     sec/op     │    sec/op     vs base            │
minLen-12                            15.42n         14.24n         -7.65%
maxLen-12                            15.73n         14.37n         -8.65%
len-12                               15.40n         14.28n         -7.27%
betweenLen-12                        18.57n         14.60n        -21.38% ✓
min-12                               18.28n         17.61n         -3.67%
max-12                               19.06n         17.37n         -8.87% ✓
range-12                             21.34n         17.54n        -17.81% ✓
geomean                              22.14n         21.38n         -3.45%

Allocations: 0 B/op, 0 allocs/op (identical)

All 3499 tests pass, 0 lint issues.

@ReneWerner87
Copy link
Copy Markdown
Member

@pageton why is this regex related extra code still needed ?
image
image

image image

@pageton
Copy link
Copy Markdown
Contributor Author

pageton commented Jun 4, 2026

@ReneWerner87 you're right, that regex-specific code was no longer needed and conflicted with the goal of keeping the parser/match flow uniform.

I removed the regex-only handling from path.go:

  • no constraintName == ConstraintRegex branch in argument parsing
  • no handler.Name() == ConstraintRegex branch after newConstraint
  • no routeSegment.regexMatchers side storage
  • no separate checkConstraint path for regex

The parser now only resolves the handler, creates the constraint, and appends it like every other constraint. Regex-specific behavior is handled inside regexConstraintType.Analyze, where the configured RegexHandler compiles the pattern and stores the matcher in Constraint.Data.

Pushed in 3a5deb1e.

@ReneWerner87
Copy link
Copy Markdown
Member

@efectn can you check
Do you see any issues with the approach chosen here and the code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

🔥 feat: unify internal and custom constraints into a single interface

2 participants