Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Route Overlapping #9

Merged
merged 15 commits into from
Apr 9, 2023
Merged

Conversation

tigerwill90
Copy link
Owner

@tigerwill90 tigerwill90 commented Mar 27, 2023

Introduction

Fox uses a Radix tree for route matching, ensuring fast and efficient routing. Currently, this router only supports explicit matches and does not allow overlapping routes. However, there are valid use cases where allowing a certain level of route overlap would be beneficial without significantly impacting performance.

This RFC proposes a change that allows limited route overlapping while maintaining high performance and enhancing flexibility. The aim is to find a balance between the performance of the router and the flexibility of route definition.

Background

Currently, routes such as:

/*{filepath}
/static

and

/content/{id}
/content/articles/{id}
/content/news/{id}
/content/events/{id}

are not supported, as they overlap.

Proposal

Allow limited route overlapping by prioritizing more specific routes and permitting only one named or catch-all route to overlap with a static route segment.

Rules

  1. Prioritize the more specific route:
    • If a catch-all or named route overlaps with a static route segment, the static segment should be prioritized.
  2. Allow only one named or catch-all route to overlap with a static route segment.
  3. Overlapping named parameters and catch-all parameters in the same segment is not allowed.
  4. Each HTTP method has its own tree, so overlapping routes with different methods are allowed.

Not Allowed

  1. Overlapping catch-all routes and named parameter route (e.g. /*{filepath} and /{user}/track).
  2. Overlapping named routes (e.g. /users/{id} and /users/{name}).
  3. Overlapping catch-all routes

Example

Thoses routes should be allowed with the proposed enhancements.

GET /*{filepath}
GET /users/{id}
GET /users/{id}/emails
GET /users/{id}/*{actions}
POST /users/{name}/emails

These routes can coexist because the more specific routes take precedence over the less specific routes. The catch-all route /*{filepath} would not match any requests that match the more specific routes for users. Additionally, the /users/{id}/*{actions} route would only match requests that have /users/{id}/ as a prefix and would not conflict with the /users/{id}/emails route since it is more specific.

Adding the route /users/{name} would break the rules because there are two named parameters ({id} and {name}) overlapping in the same path segment /user/ for the same request method. The router would not allow this configuration, as it would introduce ambiguity in route matching.

Benefits

  • More flexibility in route definition
  • Support for common use cases that require route overlapping

Drawbacks

  • Potential increase in complexity when implementing the router
  • Slightly reduced performance when handling overlapping routes (this may have more impact on performance than I hope)

Conclusion

By allowing limited route overlapping and prioritizing more specific routes, Fox could handle a broader range of use cases without significantly impacting performance.

@tigerwill90 tigerwill90 requested a review from 4nth0 March 27, 2023 22:08
@tigerwill90 tigerwill90 self-assigned this Mar 27, 2023
@tigerwill90 tigerwill90 requested a review from rewiko March 28, 2023 08:15
@tigerwill90
Copy link
Owner Author

tigerwill90 commented Apr 2, 2023

Summary of changes and their impacts

  1. Algorithm changes:
  • Introducing the childParamIndex (in favor of childParam): With the new implementation allowing overlapping routes, this addition helps in quickly identifying the index of the child node that is a dynamic segment (named or catch-all parameter). It is essential for handling scenarios where a single named or catch-all parameter overlaps with any number of static routes.
  • Route evaluation order: The modified algorithm now evaluates routes based on specificity, giving higher priority to more specific routes over less specific ones.
  • Backtracking capability: The algorithm has been adapted to include backtracking functionality. This feature ensures that the algorithm can explore alternative routes when the initial evaluated route does not match exactly.
  1. Performance impact:

    Although the modifications to the algorithm come with some performance trade-offs compared to the current implementation, the overall performance remains competitive, often outperforming Gin in various cases. The preliminary benchmark results show an increase in the execution time for some scenarios, but Fox still provides better performance than Gin, especially for overlapping routes (84.0% faster). Further performance profiling should be conducted before releasing this change.

goos: linux
goarch: amd64
pkg: github.com/tigerwill90/fox
cpu: Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz
                    │   old.txt   │                 new.txt                 │
                    │   sec/op    │    sec/op      vs base                  │
StaticAll-16          7.548µ ± 4%   11.179µ ±  2%  +48.11% (p=0.000 n=10)
Lookup-16             6.171µ ± 2%
GithubParamsAll-16    76.87n ± 3%    83.64n ±  3%   +8.81% (p=0.000 n=10)
StaticParallel-16     6.348n ± 4%    9.056n ± 11%  +42.66% (p=0.000 n=10)
CatchAll-16           31.42n ± 3%    34.12n ±  4%   +8.59% (p=0.000 n=10)
CatchAllParallel-16   5.383n ± 4%    4.893n ± 23%        ~ (p=0.280 n=10)
geomean               125.2n         385.7n        +17.81%           

@tigerwill90 tigerwill90 force-pushed the route-overlapping-enhancement branch from 3832f51 to 7b86e97 Compare April 4, 2023 19:48
@tigerwill90 tigerwill90 marked this pull request as ready for review April 4, 2023 19:59
Repository owner deleted a comment from codecov bot Apr 4, 2023
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #9 (7eb1435) into master (92ba374) will decrease coverage by 3.19%.
The diff coverage is 74.06%.

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
- Coverage   87.65%   84.47%   -3.19%     
==========================================
  Files           8       12       +4     
  Lines        1231     1552     +321     
==========================================
+ Hits         1079     1311     +232     
- Misses        125      216      +91     
+ Partials       27       25       -2     
Flag Coverage Δ
coverage.txt 84.47% <74.06%> (-3.19%) ⬇️

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

Impacted Files Coverage Δ
response_writer.go 31.32% <31.32%> (ø)
recovery.go 45.83% <45.83%> (ø)
error.go 63.88% <50.00%> (-16.12%) ⬇️
options.go 62.16% <53.84%> (-7.54%) ⬇️
context.go 68.90% <68.90%> (ø)
node.go 74.04% <81.25%> (+0.09%) ⬆️
fox.go 89.66% <88.77%> (ø)
tree.go 93.05% <97.84%> (+3.77%) ⬆️
iter.go 98.59% <100.00%> (-0.04%) ⬇️
params.go 90.00% <100.00%> (+2.12%) ⬆️
... and 1 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tigerwill90 tigerwill90 force-pushed the route-overlapping-enhancement branch from 7b1cb7a to 2cfe7fc Compare April 8, 2023 13:06
@tigerwill90 tigerwill90 force-pushed the route-overlapping-enhancement branch from 0e54261 to fa16505 Compare April 8, 2023 22:00
@tigerwill90 tigerwill90 force-pushed the route-overlapping-enhancement branch from 4842be5 to 5c83d40 Compare April 9, 2023 19:03
@tigerwill90 tigerwill90 merged commit eb63fff into master Apr 9, 2023
@tigerwill90 tigerwill90 deleted the route-overlapping-enhancement branch April 9, 2023 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant