Skip to content

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