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

Fix the HttpRouter problem with overlapping in trail and the middle of routes #382

Merged
merged 1 commit into from
Apr 10, 2019

Conversation

Vkt0r
Copy link
Member

@Vkt0r Vkt0r commented Apr 7, 2019

This PR should fix #378, #381 and #376 and can be resumed in the following steps:

  • Remove the set of the empty string for not matched parameters, by default should be nil
  • Add unit tests to verify the trail dynamic identifier as the end of the route and with more dynamic path after the end of the route and the overlapping during the middle of the route

The findHandler method was refactored to fix two main issues occurring during the search of a route.

  1. Nodes ending in dynamic trails were not matched correctly if they have more routes after the trail. Examples of routes are defined in the new test case testHttpRouterShouldHandleOverlappingRoutesInTrail added.
  2. After the search traverse an incorrect path in a dynamic route the recursion was not handling the rest of the case properly regarding the IndexingIterator<[String] is mutated after call next().

The method was refactored to use a default array with indexes instead of the IndexingIterator<[String] to be able to traverse the routes and search correctly and removed the part of the code causing the routes with a dynamic trail were not matched.

@Vkt0r Vkt0r added the bug label Apr 7, 2019
@Vkt0r Vkt0r changed the title Fix the HttpRouter problem with overlapping in trail and the middle of routes [WIP] Fix the HttpRouter problem with overlapping in trail and the middle of routes Apr 7, 2019
@Vkt0r Vkt0r force-pushed the dynamic-overlapping-trail branch from cbcabb5 to d111301 Compare April 7, 2019 03:48
@Vkt0r
Copy link
Member Author

Vkt0r commented Apr 10, 2019

I tried this fix in a big suite of UI Tests with a lot of overlapping in the routes as it's working as expected. Also, we have positive answers about the fix of the issue in #381 and #376. So this PR is ready to be reviewed 🚀.

@Vkt0r Vkt0r changed the title [WIP] Fix the HttpRouter problem with overlapping in trail and the middle of routes Fix the HttpRouter problem with overlapping in trail and the middle of routes Apr 10, 2019
Sources/HttpRouter.swift Show resolved Hide resolved
@adamkaplan
Copy link
Collaborator

adamkaplan commented Apr 10, 2019

Merge if you like, just one minor comment about the docs, otherwise looks good

* Remove the set of the empty string for not matched parameters, by default should be `nil`
* Add unit tests to verify the trail dynamic identifier as end of route and with more dynamic path after the end of the route and the overlapping during the middle of the route
* Update the `XCTestManifest` with the new tests for Linux
@Vkt0r Vkt0r force-pushed the dynamic-overlapping-trail branch from d111301 to d3806a1 Compare April 10, 2019 14:27
@Vkt0r
Copy link
Member Author

Vkt0r commented Apr 10, 2019

Thanks, @adamkaplan and welcome back 😀.

@Vkt0r Vkt0r merged commit c7c95af into httpswift:stable Apr 10, 2019
@Vkt0r Vkt0r deleted the dynamic-overlapping-trail branch April 10, 2019 15:30
@Vkt0r Vkt0r mentioned this pull request May 1, 2019
tomieq pushed a commit to tomieq/swifterfork that referenced this pull request Apr 1, 2021
Fix the HttpRouter problem with overlapping in trail and the middle of routes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

router urls with "?"
2 participants