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.

Sign up for GitHub

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

Jump to bottom

Feat: allow "/*catch-all" and "/normal" routes coexist #3270

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Feat: allow "/*catch-all" and "/normal" routes coexist #3270

wants to merge 2 commits into from

Conversation

StephanoGeorge
Copy link

@StephanoGeorge StephanoGeorge commented Aug 6, 2022 •
edited

Allow /*catch-all and /normal routes coexist, like /*filepath, /users/me, /users/*user.

/users will match /users/*user, and the param will be [{"user": "/"}], unless route /users/ added.

Param and catch-all parts are still in conflict: /*filepath, /:action, but this is quite reasonable, after all, wo can't say that a path with only one slash should correspond to param.

Reason for removing empty node before /*catch-all node:

  • Now empty node can't guarantee that path must start with a slash when entering catch-all.
  • n.wildChild will be false when n has a catch-all empty node as child, so we have to check n.children[len(n.children)-1].wildChild, there was no problem before because there would be no coexistence of /normal routes and /*catch-all routes before.

fixes #2102
fixes #2846
fixes #2920
fixes #2930

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 7, 2022
View reviewed changes
tree.go
default:
panic("invalid node type")
case catchAll:
if path[0] != '/' {

Check notice

Code scanning

Bad redirect check

This is a check that [this value](1), which flows into a [redirect](2), has a leading slash, but not that it does not have '/' or '\' in its second position. This is a check that [this value](3), which flows into a [redirect](2), has a leading slash, but not that it does not have '/' or '\' in its second position. This is a check that [this value](4), which flows into a [redirect](2), has a leading slash, but not that it does not have '/' or '\' in its second position. This is a check that [this value](5), which flows into a [redirect](2), has a leading slash, but not that it does not have '/' or '\' in its second position. This is a check that [this value](6), which flows into a [redirect](2), has a leading slash, but not that it does not have '/' or '\' in its second position. This is a check that [this value](7), which flows into a [redirect](2), has a leading slash, but not that it does not have '/' or '\' in its second position. This is a check that [this value](8), which flows into a [redirect](2), has a leading slash, but not that it does not have '/' or '\' in its second position.
Copy link
Author

Choose a reason for hiding this comment

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

if path[0] != '/' and redirecting are not related

@codecov
Copy link

codecov bot commented Aug 7, 2022 •
edited

Codecov Report

Merging #3270 (8f2fd7a) into master (ad66d9d) will decrease coverage by 0.26%.
The diff coverage is 99.42%.

❗ Current head 8f2fd7a differs from pull request most recent head 90a6a2c. Consider uploading reports for the commit 90a6a2c to get more accurate results

@@            Coverage Diff             @@
##           master    #3270      +/-   ##
==========================================
- Coverage   98.36%   98.10%   -0.27%     
==========================================
  Files          42       42              
  Lines        3124     3107      -17     
==========================================
- Hits         3073     3048      -25     
- Misses         38       45       +7     
- Partials       13       14       +1
Flag Coverage Δ
98.10% <99.42%> (-0.27%) ⬇️
go-1.15 98.10% <99.42%> (-0.27%) ⬇️
go-1.16 ∅ <ø> (?)
go-1.17 98.00% <99.42%> (-0.27%) ⬇️
go-1.18 98.00% <99.42%> (-0.27%) ⬇️
macos-latest 98.10% <99.42%> (-0.27%) ⬇️
ubuntu-latest 98.10% <99.42%> (-0.27%) ⬇️

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

Impacted Files Coverage Δ
tree.go 98.57% <99.42%> (-1.43%) ⬇️

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

@StephanoGeorge StephanoGeorge force-pushed the fix/catchall branch 2 times, most recently from 9902cdb to 90a6a2c Compare August 8, 2022 10:05
@StephanoGeorge
Copy link
Author

Please approval workflows. I force pushed because I fixed code coverage, and squashed commits down to just 2.

@aix3
Copy link

aix3 commented Aug 15, 2023

Is there any progress on this feature? We need this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@StephanoGeorge @aix3