yousuf64/shift

High-performance HTTP router for Go.

shift's Issues

Matches the registered param route when having empty parameter values in the URL

Issue Summary

I've observed unexpected behavior in route matching when having empty parameter values in the URL.

Scenario 1:

Registered Routes:

• /products:id
• /products/:id/tags

Current Behavior:

The following routes match with /products:id when they shouldn't match any.

1. /products/
2. /products/1
3. /products//
4. /products//tags

Scenario 2:

Registered Routes:

• /posts/:id
• /:abc
• /:abc/:xyz/comments

Current Behavior:

The following routes match with /posts/:id when they shouldn't match any.

1. /posts//
2. /posts//comments

Scenario 3:

Registered Routes:

• /:aaa

Current Behavior:

The following routes match with /:aaa when they shouldn't match any.

1. https://example.com//
2. https://example.com///
3. https://example.com///hello

Additional Information

Configuration details (All defaults):

• TrailingSlashMatch: Skip
• PathCorrectionMatch: Skip

Copy is not safe to use with concurrent requests

Hello @yousuf64

Awesome library, many thanks for working on it! Please take a look at this test:

func TestParams_Copy(t *testing.T) {
	t.Run("Copy should not be affected when source is reset and reused", func(t *testing.T) {
		p := newParams(1)
		p.setKeys(&[]string{"foo"})
		p.appendValue("bar")

		cp := p.Copy()

		p.reset()
		p.setKeys(&[]string{"foo"})
		p.appendValue("bar2")

		val := cp.Get("foo")
		assert(t, val == "bar", fmt.Sprintf("expected: bar, got: %s", val))
	})
}

Outputs expected: bar, got: bar2

Slices in copy still reference to the original ones. If I am not missing sth here – pooling params and re-using them may result in copy corruption at the moment.

Test case failure

Hello again @yousuf64

Consider this test case:

func TestRouter_ServeHTTP_ParamRoutes_New(t *testing.T) {
	r := newTestShift()

	paths := map[string]string{
		"/:file":      http.MethodGet,
		"/hero-:name": http.MethodGet,
	}

	rec := &recorder{}

	for path, meth := range paths {
		r.Map([]string{meth}, path, rec.Handler(path))
	}

	tt := routerScenario{
		{method: http.MethodGet, path: "/hero-", valid: false, pathTemplate: "", params: nil},
	}

	testRouter_ServeHTTP(t, r.Serve(), rec, tt)
}

It passes. Now add one more route /hero/:name (which I suppose should not affect test case):

func TestRouter_ServeHTTP_ParamRoutes_New(t *testing.T) {
	r := newTestShift()

	paths := map[string]string{
		"/:file":      http.MethodGet,
		"/hero-:name": http.MethodGet,
		"/hero/:name": http.MethodGet,
	}

	rec := &recorder{}

	for path, meth := range paths {
		r.Map([]string{meth}, path, rec.Handler(path))
	}

	tt := routerScenario{
		{method: http.MethodGet, path: "/hero-", valid: false, pathTemplate: "", params: nil},
	}

	testRouter_ServeHTTP(t, r.Serve(), rec, tt)
}

As we can see now /hero- starting to match /:file route:

router_test.go:2106: /hero- > didn't expect a handler, but found a handler with template '/:file'

Which actually seems correct, but in the first test it did not match for some reason.

global middleware not executed on missing routes

No middleware is ran if the route does not exist.

This also means the heartbeat middleware endpoint never works.

func main() {
	router := shift.New()
	router.UseTrailingSlashMatch(shift.WithExecute())
	router.Use(authMiddleware)
	router.Use(heartbeat("/ping"))
	router.Use(shift.Recover())

	router.GET("/", shift.HTTPHandlerFunc(HelloHandler))

	router.GET("/other", func(w http.ResponseWriter, r *http.Request, route shift.Route) error {
		_, err := fmt.Fprint(w, "Hello, world!")
		return err
	})

	http.ListenAndServe(":8080", router.Serve())
}

func HelloHandler(w http.ResponseWriter, r *http.Request) {
	_, _ = fmt.Fprintf(w, "👋👋👋")
}

func authMiddleware(next shift.HandlerFunc) shift.HandlerFunc {
	return func(w http.ResponseWriter, r *http.Request, route shift.Route) error {
		if v := r.Header.Get("Authorization"); v != "" {
			return next(w, r, route)
		}
		w.WriteHeader(http.StatusBadRequest)
		_, _ = w.Write([]byte("'Authorization' header required"))
		return errors.New("'Authorization' header required")
	}
}

func heartbeat(endpoint string) shift.MiddlewareFunc {
	log.Println("inside hb")
	return func(next shift.HandlerFunc) shift.HandlerFunc {
		log.Println("inside next")
		return func(w http.ResponseWriter, r *http.Request, route shift.Route) error {
			log.Println("urlpath", r.URL.Path)
			if (r.Method == "GET" || r.Method == "HEAD") && strings.EqualFold(r.URL.Path, endpoint) {
				w.Header().Set("Content-Type", "text/plain")
				w.WriteHeader(http.StatusOK)
				w.Write([]byte("."))
			}

			return next(w, r, route)
		}
	}
}

add benchmark against chi

I think this would be good since chi and shift are similar being router & middleware