Fixes path order dependent response when a nil handler is defined for a route #517
Conversation
mux_test.go
Outdated
| t.Fatalf("Expected status code 200 (got %d)", w.Code) | ||
| } | ||
|
|
||
| reversedPathRouter := NewRouter() |
There was a problem hiding this comment.
Break these into subtests - e.g.
t.Run("Test handler ordering", func(t *testing.T) {
// test
})
t.Run("Test reversed handler ordering", func(t *testing.T) {
// test
})
I would also add additional test cases:
- Test ordering where handlers have the same method & path, but a different handler (expected: the first handler added is the one executed)
- Same path, different methods (no path variables)
- Same as Host() with a port fails to match when the host is supplied in the HTTP Host: header #2, but with middleware
mux_test.go
Outdated
| reversedPathRouter := NewRouter() | ||
| reversedPathRouter.Path("/a/{a}").Handler(http.HandlerFunc(handler)).Methods(http.MethodGet) | ||
| reversedPathRouter.Path("/a/b").Handler(nil).Methods(http.MethodOptions) | ||
| reversedPathRouter.Use(MiddlewareFunc(testOptionsMiddleWare)) |
There was a problem hiding this comment.
This should be before the paths are added.
mux_test.go
Outdated
| reversedPathRouter.Path("/a/b").Handler(nil).Methods(http.MethodOptions) | ||
| reversedPathRouter.Use(MiddlewareFunc(testOptionsMiddleWare)) | ||
|
|
||
| w = NewRecorder() |
There was a problem hiding this comment.
Make it a subtest (as above) and call newRequest again to separate it.
|
@rkilingr - did you still want to complete this? |
|
@rkilingr - just following up again if you're still keen on completing this? |
Apologies for the huge delay. Was not active last year, will be updating the PR |
|
Hi @rkilingr - quick follow-up - are you interested in getting this merged? Will do a review again if existing comments are addressed. |
Codecov Report
@@ Coverage Diff @@
## main #517 +/- ##
==========================================
+ Coverage 78.01% 78.19% +0.17%
==========================================
Files 5 5
Lines 887 885 -2
==========================================
Hits 692 692
+ Misses 140 138 -2
Partials 55 55
|
Fixes #515
Summary of Changes
handler != nilcondition only for setting the matched handler, not for clearing Method mismatch errornil