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 to handle overlapping of the routes #359

Merged
merged 1 commit into from
Jan 22, 2019

Conversation

Vkt0r
Copy link
Member

@Vkt0r Vkt0r commented Dec 31, 2018

This PR should fix #355 and can be resumed in the following steps:

  • Fix the HttpRouter to handle overlapping between routes with wildcard correctly.
  • Include the SwifterTestHttpRouter in the SwifteriOSTests.
  • Add a unit tests for overlapped routes.

The current findHandler(_:, ..) function was taking only one path once a match was found and sometimes incorrectly, let's see the two routes proposed in the issue:

  • a/b
  • a/:id/c

If we see the following tree generated by the two routes above it should look like the following:

                                   GET
                                    |
                                    a
                                  /   \
                                :id    b
                               /
                              c

The previous findHandler(_:, ..) algorithm was traversing the path recursively and returning if a match was found but the way it was implemented makes really hard to take advantage of the recursive search to continue searching in case of went through a path incorrectly.

In the refactored algorithm I didn't return only a match, nevertheless, all the possible matches in the Tree kept in an array and handling the incorrect path without return anything directly.

There are several updates to the previous algorithms in terms of refactoring but two are very important:

  1. The necessity of keeping the current index inside the generator.
  2. Handle the case of when the generator doesn't have more nodes and the current node neither and that means it's a match in the tree.

Happy to discuss more or any doubt in the refactoring. Happy review!

@Vkt0r
Copy link
Member Author

Vkt0r commented Jan 16, 2019

@adamkaplan @glock45 Can someone of you guys take a look in this PR when you have time 😀 ?

@adamkaplan
Copy link
Collaborator

adamkaplan commented Jan 21, 2019

@Vkt0r Just back from vacation! It looks good. The change is actually fairly small if the whitespace is ignored.

I think it should have a more precise unit tests. The test that was added does check for path match, but does not verify which path was matched. So, the intention (priority) is not very clear. You can probably set different route handlers and toggle a flag in each one.

Something more like this:

var foundStaticRoute = false
router.register("GET", path: "a/b") { _ in
	foundStaticRoute = true
	return HttpResponse.accepted
}

var foundVariableRoute = false
router.register("GET", path: "a/:id/c") { _ in
	foundVariableRoute = true
	return HttpResponse.accepted
}

XCTAssertNotNil(router.route("GET", path: "a/b"))
XCTAssertTrue(foundStaticRoute)

XCTAssertNotNil(router.route("GET", path: "a/b/c"))
XCTAssertTrue(foundVariableRoute)

@Vkt0r
Copy link
Member Author

Vkt0r commented Jan 22, 2019

@adamkaplan I hope you enjoyed your vacations 😀 ! Totally agree with you, it makes more sense to have more precise tests as you mentioned. Thanks for the suggestion, I'll be adding more tests soon!

• Include the `SwifterTestHttpRouter` in the `SwifteriOSTests`
• Add a unit tests for overlapped routes
@Vkt0r
Copy link
Member Author

Vkt0r commented Jan 22, 2019

@adamkaplan Test updated!

@adamkaplan adamkaplan merged commit c870c97 into httpswift:stable Jan 22, 2019
@Vkt0r Vkt0r deleted the fix-http-router branch January 22, 2019 16:11
tomieq pushed a commit to tomieq/swifterfork that referenced this pull request Apr 1, 2021
Fix the HttpRouter to handle overlapping of the routes
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.

HttpRouter takes incorrect route
2 participants