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

Made nested regexp pattern route properly. #506

Merged
merged 2 commits into from
Apr 16, 2020
Merged

Conversation

Jahaja
Copy link
Contributor

@Jahaja Jahaja commented Apr 15, 2020

Fixes #411

@pkieltyka
Copy link
Member

@Jahaja very nice work on this. I think this is very close.

I've tried the code against the test case below and found it to fail, but looks extremely close!

func TestRegexpRecursive(t *testing.T) {
	hStub1 := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})
	hStub2 := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})

	tr := &node{}
	tr.InsertRoute(mGET, "/one/{firstId:[a-z0-9-]+}/{secondId:[a-z0-9-]+}/first", hStub1)
	tr.InsertRoute(mGET, "/one/{firstId:[a-z0-9-_]+}/{secondId:[a-z0-9-_]+}/second", hStub2)

	// log.Println("~~~~~~~~~")
	// log.Println("~~~~~~~~~")
	// debugPrintTree(0, 0, tr, 0)
	// log.Println("~~~~~~~~~")
	// log.Println("~~~~~~~~~")

	tests := []struct {
		r string       // input request path
		h http.Handler // output matched handler
		k []string     // output param keys
		v []string     // output param values
	}{
		{r: "/one/hello/world/first", h: hStub1, k: []string{"firstId", "secondId"}, v: []string{"hello", "world"}},
		{r: "/one/hi_there/ok/second", h: hStub2, k: []string{"firstId", "secondId"}, v: []string{"hi_there", "ok"}},
		{r: "/one///first", h: nil, k: []string{}, v: []string{}},
		{r: "/one/hi/123/second", h: hStub2, k: []string{"firstId", "secondId"}, v: []string{"hi", "123"}},
	}

	for i, tt := range tests {
		rctx := NewRouteContext()

		_, handlers, _ := tr.FindRoute(rctx, mGET, tt.r)

		var handler http.Handler
		if methodHandler, ok := handlers[mGET]; ok {
			handler = methodHandler.handler
		}

		paramKeys := rctx.routeParams.Keys
		paramValues := rctx.routeParams.Values

		if fmt.Sprintf("%v", tt.h) != fmt.Sprintf("%v", handler) {
			t.Errorf("input [%d]: find '%s' expecting handler:%v , got:%v", i, tt.r, tt.h, handler)
		}
		if !stringSliceEqual(tt.k, paramKeys) {
			t.Errorf("input [%d]: find '%s' expecting paramKeys:(%d)%v , got:(%d)%v", i, tt.r, len(tt.k), tt.k, len(paramKeys), paramKeys)
		}
		if !stringSliceEqual(tt.v, paramValues) {
			t.Errorf("input [%d]: find '%s' expecting paramValues:(%d)%v , got:(%d)%v", i, tt.r, len(tt.v), tt.v, len(paramValues), paramValues)
		}
	}
}

results from running:

➜  chi git:(master) ✗ go test -v -run=RegexpRecursive .
=== RUN   TestRegexpRecursive
    TestRegexpRecursive: tree_test.go:56: input [3]: find '/one/hi/123/second' expecting paramValues:(2)[hi 123] , got:(2)[ 123]
--- FAIL: TestRegexpRecursive (0.00s)
FAIL
FAIL	github.com/go-chi/chi	0.002s
FAIL

@pkieltyka
Copy link
Member

the other bit of testing here is to ensure the algorithm doesn't create any new allocations and performance is the same as previously (for conditions that aren't regexp type params). See https://gist.github.com/pkieltyka/123032f12052520aaccab752bd3e78cc

@Jahaja
Copy link
Contributor Author

Jahaja commented Apr 16, 2020

Thanks! I found the issue and committed a fix for it.

The run from the benchmarks looks the same to me:

   Chi: 94920 Bytes

#GPlusAPI Routes: 13
   Chi: 8024 Bytes

#ParseAPI Routes: 26
   Chi: 9744 Bytes

#Static Routes: 157
   Chi: 82432 Bytes

=== RUN   TestRouters
--- PASS: TestRouters (0.04s)
goos: linux
goarch: amd64
pkg: github.com/pkieltyka/go-http-routing-benchmark
BenchmarkChi_Param        	 2513401	       464 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_Param5       	 1791766	       662 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_Param20      	  921858	      1355 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_ParamWrite   	 2321802	       520 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_GithubStatic 	 2479116	       489 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_GithubParam  	 1783575	       664 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_GithubAll    	    8445	    143108 ns/op	   87699 B/op	     609 allocs/op
BenchmarkChi_GPlusStatic  	 2699571	       441 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_GPlusParam   	 2401137	       503 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_GPlus2Params 	 2099559	       574 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_GPlusAll     	  155424	      7321 ns/op	    5616 B/op	      39 allocs/op
BenchmarkChi_ParseStatic  	 2729254	       440 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_ParseParam   	 2458795	       485 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_Parse2Params 	 2245444	       534 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_ParseAll     	   86856	     13870 ns/op	   11232 B/op	      78 allocs/op
BenchmarkChi_StaticAll    	   13815	     88988 ns/op	   67826 B/op	     471 allocs/op
PASS
ok  	github.com/pkieltyka/go-http-routing-benchmark	26.271s

When I changed routes.go to use my modified fork:

   Chi: 94680 Bytes

#GPlusAPI Routes: 13
   Chi: 8024 Bytes

#ParseAPI Routes: 26
   Chi: 9744 Bytes

#Static Routes: 157
   Chi: 82432 Bytes

=== RUN   TestRouters
--- PASS: TestRouters (0.04s)
goos: linux
goarch: amd64
pkg: github.com/pkieltyka/go-http-routing-benchmark
BenchmarkChi_Param        	 2562949	       466 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_Param5       	 1770303	       664 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_Param20      	  923127	      1383 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_ParamWrite   	 2333044	       522 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_GithubStatic 	 2445535	       493 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_GithubParam  	 1820755	       665 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_GithubAll    	    7936	    143187 ns/op	   87699 B/op	     609 allocs/op
BenchmarkChi_GPlusStatic  	 2718314	       442 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_GPlusParam   	 2347321	       507 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_GPlus2Params 	 2074357	       581 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_GPlusAll     	  163339	      7333 ns/op	    5616 B/op	      39 allocs/op
BenchmarkChi_ParseStatic  	 2693289	       448 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_ParseParam   	 2449399	       496 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_Parse2Params 	 2216649	       541 ns/op	     432 B/op	       3 allocs/op
BenchmarkChi_ParseAll     	   87310	     14202 ns/op	   11232 B/op	      78 allocs/op
BenchmarkChi_StaticAll    	   13256	     88655 ns/op	   67826 B/op	     471 allocs/op
PASS
ok  	github.com/pkieltyka/go-http-routing-benchmark	26.388s

@pkieltyka pkieltyka merged commit 99ad97f into go-chi:master Apr 16, 2020
@pkieltyka
Copy link
Member

@Jahaja thank you again! merged :)

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.

regexp route nested wrongly -> no routing
2 participants