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

Hold matched route full path in the Context #1826

Merged
merged 4 commits into from
May 26, 2019

Conversation

zaynetro
Copy link
Contributor

This is a proposed change to return handler full path from the Context.

Where it can be useful?

When using lambda functions as a handler names returned from (*Context).HandlerName become not lean. With this change it becomes easy to analyse which route was called exactly. This can be useful when trying to find out which routes are not being called in the app.

Alternatives

It is possible to create a wrapper around route definition:

func logGET(router *gin.Engine, path string, handler func (*gin.Context)) {
    router.GET(path, func (c *gin.Context) {
        // Do something with path
        handler(c)
    })
}

...

logGET(router, "/user/:id", func (c *gin.Context) {
    c.Status(200)
})

This approach is doable but considering that Gin already holds full path in its structures I though that it might be better to just expose it.

P.S. README no longer contains any docs so I am not certain where I should document this feature.

@thinkerou
Copy link
Member

@zaynetro thanks! ref: #1390 #1447

@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

Merging #1826 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1826      +/-   ##
==========================================
+ Coverage   98.74%   98.74%   +<.01%     
==========================================
  Files          38       38              
  Lines        2143     2157      +14     
==========================================
+ Hits         2116     2130      +14     
  Misses         15       15              
  Partials       12       12
Impacted Files Coverage Δ
context.go 98.37% <100%> (+0.01%) ⬆️
tree.go 100% <100%> (ø) ⬆️
gin.go 99.11% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78a8b5c...9bd2505. Read the comment docs.

@zaynetro
Copy link
Contributor Author

Oh, I wasn't aware of multiple previous attempts...

@thinkerou
Copy link
Member

The pull request benchmark data:

➜  go-http-routing-benchmark git:(master) ✗ go test -bench=Gin
#GithubAPI Routes: 203
   Gin: 58480 Bytes

#GPlusAPI Routes: 13
   Gin: 4368 Bytes

#ParseAPI Routes: 26
   Gin: 7744 Bytes

#Static Routes: 157
   Gin: 34064 Bytes

goos: darwin
goarch: amd64
pkg: github.com/julienschmidt/go-http-routing-benchmark
BenchmarkGin_Param        	20000000	        83.5 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Param5       	10000000	       136 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Param20      	 5000000	       333 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParamWrite   	10000000	       158 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubStatic 	20000000	       104 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubParam  	10000000	       163 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubAll    	   50000	     32729 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusStatic  	20000000	        82.9 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusParam   	20000000	       103 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlus2Params 	10000000	       131 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusAll     	 1000000	      1476 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseStatic  	20000000	        88.6 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseParam   	20000000	        97.2 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Parse2Params 	20000000	       103 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseAll     	  500000	      2528 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_StaticAll    	  100000	     22313 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/julienschmidt/go-http-routing-benchmark	30.647s

The master branch benchmark data:

➜  go-http-routing-benchmark git:(master) ✗ go test -bench=Gin
#GithubAPI Routes: 203
   Gin: 52064 Bytes

#GPlusAPI Routes: 13
   Gin: 3936 Bytes

#ParseAPI Routes: 26
   Gin: 6912 Bytes

#Static Routes: 157
   Gin: 30480 Bytes

goos: darwin
goarch: amd64
pkg: github.com/julienschmidt/go-http-routing-benchmark
BenchmarkGin_Param        	20000000	        97.6 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Param5       	10000000	       180 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Param20      	 3000000	       544 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParamWrite   	10000000	       178 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubStatic 	10000000	       124 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubParam  	10000000	       220 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubAll    	   30000	     48147 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusStatic  	20000000	        89.7 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusParam   	10000000	       149 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlus2Params 	10000000	       213 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusAll     	 1000000	      2136 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseStatic  	20000000	        94.0 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseParam   	20000000	       111 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Parse2Params 	10000000	       149 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseAll     	  500000	      3791 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_StaticAll    	   50000	     29414 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/julienschmidt/go-http-routing-benchmark	32.626s

@thinkerou
Copy link
Member

thinkerou commented May 22, 2019

About the proposal, #1390 committed by @4396 (thanks!) on 12 Jun 2018 first, and #1447 committed by @youngbloood (thanks!) on 28 Jul 2018, but they have a bit performance issue, please see #1390 (comment) and #1447 (comment).
For respecting everybody's contribution and positivity, we should handle and merge the first commit pull request, but sorry the first and second have some performance issue, maybe we should merge the pull request that the goal is the same. thanks!

@zaynetro
Copy link
Contributor Author

I am fine with anything as long as the changes land on master.

@appleboy
Copy link
Member

@zaynetro Could you also update the documentation?

@zaynetro
Copy link
Contributor Author

I wasn't sure where to update the documentation so decided to extend an existing example.

Copy link
Member

@appleboy appleboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks @zaynetro

@thinkerou thinkerou merged commit 35e33d3 into gin-gonic:master May 26, 2019
@zaynetro zaynetro deleted the absolute-path-context branch May 26, 2019 11:06
@wpjunior
Copy link

wpjunior commented Oct 7, 2019

Nice feature!, could you release a new version with this feature?, I wanna use it without lock with the master branch.

@Dominik-K
Copy link

Dominik-K commented Oct 18, 2019

@thinkerou @appleboy @javierprovecho Can you release this useful feature that we can properly version this module, please?

It fixes #748 & #1759.

@thinkerou
Copy link
Member

@Dominik-K yes

ThomasObenaus pushed a commit to ThomasObenaus/gin that referenced this pull request Feb 19, 2020
* Return nodeValue from getValue method

* Hold route full path in the Context

* Add small example
@ltpquang
Copy link

@thinkerou nice feature, but why is it labeled break-backward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants