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

Additional tests for prefix paths and path params #2396

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

synth3
Copy link
Contributor

@synth3 synth3 commented Mar 23, 2023

Motivation:

Additional router tests and reproducer for #2395.

@synth3 synth3 changed the title Additional tests for prefix paths and path params #2395 Additional tests for prefix paths and path params Mar 23, 2023
Comment on lines +3636 to +3647
// should call "/:id/*"
testRequest(HttpMethod.GET, "/123", 200, "OK", "r1");
// should call "/a/:id/*"
testRequest(HttpMethod.GET, "/a/123", 200, "OK", "r2");
testRequest(HttpMethod.GET, "/a//123", 200, "OK", "r2");
// should call "/a/:id/b1/*"
testRequest(HttpMethod.GET, "/a/123/b1", 200, "OK", "r3");
testRequest(HttpMethod.GET, "/a//123//b1", 200, "OK", "r3");
// should call "/a/:id/b/:id2/c/*"
testRequest(HttpMethod.GET, "/a/123/b/1234/c", 200, "OK", "r4");
testRequest(HttpMethod.GET, "/a//123//b//1234//c", 200, "OK", "r4");
}
Copy link
Member

Choose a reason for hiding this comment

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

These assumptions are incorrect according to the rules the matching engine works.

A wildcard route is a route that ends with: * however it is important to note that the final slash is significant. At the HTTP spec level: http://server/a and http://server/a/ are considered 2 different resources. In vertx-web we claim that for simplicity, a static route that doesn't include the final / will match both requests to the same route, however, the routes in this example end with /* which means that the final / is significative. This means the request must include it to be matched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks alot for the explanation! There might still be some inconsistencies in documentation and/or behavior - see also #2395 (comment)

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

Successfully merging this pull request may close these issues.

None yet

2 participants