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

Define interface for HTTP router; remove path canonicalization #521

Merged
merged 1 commit into from Nov 14, 2018

Conversation

jacobgreenleaf
Copy link
Contributor

@jacobgreenleaf jacobgreenleaf commented Nov 14, 2018

Refactor some of the interfaces around HTTP router to match better with the standard library in anticipation of having to fork or replace julienschmidt/httprouter.

Fixed

  • HTTPRouter.Handle (previously Register) will now return an error in all cases where the router detects that the registered route has a conflict, rather than sometimes error and sometimes panic.

Changed

  • URL parameters decoded from the path by the HTTP router (HTTPRequest.Params) switched from httprouter.Params to url.Values from the standard library. The url.Values type is explicitly designed for this purpose and also provides a faster lookup by key than httprouter.Params due to being a map rather than list.
  • HTTPRouter concrete type unexported (now httpRouter) and replaced with an interface:
type HTTPRouter interface {
	http.Handler
	Handle(method, pattern string, handler http.Handler) error
}

This will be the HTTP router interface we can swap out the implementation of from julienschmidt/httprouter to something like gorilla/mux or even standard library http.ServeMux.

  • RouterEndpoint.HandleRequest function signature removed params parameter in lieu of getting the parameters from the context on the http.Request. This is to make HandleRequest transformable to http.Handler via http.HandlerFunc by making it match func(w http.ResponseWriter, r *http.Request).

Removed

  • Path canonicalization removed and enabled the RedirectTrailingSlash flag of julienschmidt/httprouter which issues a redirect. For example, when registering /foo/bar, if you request /foo/bar/ it will issue a 301 to /foo/bar. The existing logic to canonicalize the paths was incomplete (it didn't detect that /a/:b and /a/:c are the same for purposes of the router) and it is unclear if this is a requirement.

@coveralls
Copy link

coveralls commented Nov 14, 2018

Coverage Status

Coverage decreased (-5.05%) to 63.04% when pulling ce58391 on jg-router-iface into 207cc52 on master.

@jacobgreenleaf jacobgreenleaf merged commit 0c4ce2d into master Nov 14, 2018
@ChuntaoLu ChuntaoLu deleted the jg-router-iface branch January 30, 2019 21:31
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.

None yet

3 participants