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

add support for regexp routes #86

Closed
wants to merge 4 commits into from
Closed

Conversation

jxskiss
Copy link

@jxskiss jxskiss commented Oct 31, 2021

This pr adds support for regexp routes.
The discussion is here: #85

Change-Id: I811b19d094c0e42a795aa09bc0f6edb3063e5eec
Change-Id: I4a89cfce9b54740be2087aa19090aaac26b62954
Change-Id: I1ba32700c73f4e2c4582fdffeeda2d80837995cd
@jxskiss
Copy link
Author

jxskiss commented Oct 31, 2021

The PR code implements regexp support as following:

  1. regexp routes can be registered after static, wildcard prefix
  2. multiple regexp routes can be registered after a same prefix, they will be checked in their registering order
  3. regexp routes with longer static or wildcard prefix takes higher priority than those with shorter prefix
  4. named capturing groups will be passped to handler as params map
  5. if a route is matched, but no handler is registered for the request method, for simplicity, the returned status code is 404 instead of 405 (not sure if we should support 405 for regexp routes)

@jxskiss
Copy link
Author

jxskiss commented Nov 5, 2021

@dmitris Hi, wish you have time to take a look at this, and feedback what do you think about it.

@dmitris
Copy link
Contributor

dmitris commented Nov 5, 2021

@jxskiss honored by your request but to make sure - you didn't mean someone else (maybe @dimfeld), did you? 😄

@dimfeld
Copy link
Owner

dimfeld commented Nov 5, 2021

Been very busy lately, hopefully will have time soon

@jxskiss
Copy link
Author

jxskiss commented Nov 6, 2021

@jxskiss honored by your request but to make sure - you didn't mean someone else (maybe @dimfeld), did you? 😄

Oh yes it's a typo, sorry to bother 😳

Change-Id: Iaf1e4f5dac83567d41cdbfb9cffe9c3e02dc0b85
@dimfeld
Copy link
Owner

dimfeld commented Nov 30, 2021

Apologies for the delay. A couple things seemed notable to me:

  1. Right now the regexp matches on the entire remaining path, but it also seems desirable to have a regexp that only matches on a single path segment, such as /images3/~^(?P<timestamp>\d+)/:imageName. It feels like the single-path match should actually be the normal case here. What do you think?
  2. It would be good to have this support the MethodNotFound error. This is basically the case where found is not nil, but handler is nil, and the code that calls search uses that condition to return MethodNotFound. You can see where this happens in the wildcard child code.

@jxskiss
Copy link
Author

jxskiss commented Dec 2, 2021

  1. Right now the regexp matches on the entire remaining path, but it also seems desirable to have a regexp that only matches on a single path segment, such as /images3/~^(?P<timestamp>\d+)/:imageName. It feels like the single-path match should actually be the normal case here. What do you think?
  2. It would be good to have this support the MethodNotFound error. This is basically the case where found is not nil, but handler is nil, and the code that calls search uses that condition to return MethodNotFound. You can see where this happens in the wildcard child code.
  1. Though the pattern /images3/~^(?P<timestamp>\d+)/:imageName can also be easily written using regular expression and gives more flexibility, I agree with that single-path match should be a more normal case in url matching.
  2. I'm ok to return the MethodNotFound error.

I will update the code.

@jxskiss jxskiss closed this Dec 11, 2022
@jxskiss jxskiss deleted the regexp_routes branch December 11, 2022 04:37
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.

3 participants