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

Fixed parameters parsing #237

Merged
merged 5 commits into from
Apr 6, 2022

Conversation

ivan-tymoshenko
Copy link
Collaborator

@ivan-tymoshenko ivan-tymoshenko commented Jan 17, 2022

Fixed #235 #238

The main problem with multi-parametrical nodes is that we can't easily describe the rules of how should it be parsed. Thanks for @mcollina, he said that if I have an unclear case it should work as the path-to-regexp library does #238 (comment). The path-to-regexp converts the hole path to the regexp, so a multi-parametric path is also parsed as a regex. The main idea of this solution is to convert the multi-parametrical part of the path to the regexp. Seems like it's the only way to make it work properly.

Important point: It doesn't convert the whole path to the regexp (as path-to-regexp does), only the part of the path between two '/' if it contains multiple params or param and static part.

It replaces parameters with (.*?), regex parameters with themselves, the static part is also inserted into the regex.

Examples:

Description Multi-parametrical part of path Regexp
Param with static part /:param1-static/ /^(.*?)-static$/
Multiple params /:param1-:param2/ /^(.*?)-(.*?)$/
Regexp param with param /:param1(^\w{3}).param2/ /^(\w{3})\.(.*?)$/

Depends on #243

@ivan-tymoshenko ivan-tymoshenko force-pushed the fix-parameters-parsing branch 3 times, most recently from 8c15bc1 to d8acc99 Compare March 30, 2022 16:40
@ivan-tymoshenko ivan-tymoshenko marked this pull request as ready for review March 30, 2022 17:55
Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Could you run the benchmarks before and after?

@ivan-tymoshenko
Copy link
Collaborator Author

Main branch

Снимок экрана 2022-03-31 в 14 00 51

Current branch

Current

@mcollina
Copy link
Collaborator

It seems there is a regression with the "multi-parametric route" benchmark and an improvement for the "multi-parametric route with regexp", could you take a look?

@ivan-tymoshenko
Copy link
Collaborator Author

Yep, I can explain it. There is a regression for a multi-parameter route because previously two parametric nodes were used to parse paths like that /:param1-:param2/, which did not contain any regular expressions. It just parsed everything before and after the "-" as parameters. But there wasn't the right behavior. It worked only with simple paths. Now when it uses regex for this it obviously takes more time, but it works correctly in general.

About "multi-parametric route with regexp". It created two different nodes to parse paths like that /:param1(^\w{3}).param2/. The first is a param node with regex and the second is just a parametric node. Now it's just one regex parametric node. I guess that is the reason why it's working faster.

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

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.

Issue in parameter resolution
3 participants