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

case insensitivity #2064

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ffhelicopter
Copy link
Contributor

@ffhelicopter ffhelicopter commented Sep 19, 2019

The Gin framework is case sensitive for URLs, but the URL is not case sensitive in the browser. For a better user experience, it is recommended to optimize the URL case sensitivity, and use the lowercase path uniformly when registering the handler. Considering the case of URI parameters, you can't directly convert the URL to lowercase or uppercase in the program. You can convert it to lowercase in the node.getValue() method and compare it to achieve case insensitivity.

For example, the more complex URI below:

Router := gin.Default()

router.GET("/user/:firstname/:lastname/go42", func(c *gin.Context) {
Fname := c.Param("firstname")
Lname := c.Param("lastname")

c.String(http.StatusOK, "Hello %s %s ", fname, lname)
})

router.Run(":8080")

When this feature is implemented. Although the user and go42 in the above program are lowercase when registering, when accessing the following two URLs, they can be accessed normally and the page is rendered.

Localhost:8080/user/Rob/Pike/Go42
Localhost:8080/uSer/Rob/Pike/Go42

@appleboy
Copy link
Member

@ffhelicopter Can you help to resolve the conflicts?

@appleboy appleboy modified the milestones: 1.6, 1.7 Mar 16, 2020
@appleboy
Copy link
Member

appleboy commented Mar 16, 2020

move to 1.x milestones.

@appleboy appleboy modified the milestones: 1.7, 1.x Mar 16, 2020
@wangcheng0509
Copy link

When to release this version

@codecov
Copy link

codecov bot commented May 15, 2020

Codecov Report

Merging #2064 into master will increase coverage by 0.19%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2064      +/-   ##
==========================================
+ Coverage   98.63%   98.83%   +0.19%     
==========================================
  Files          41       40       -1     
  Lines        2349     2225     -124     
==========================================
- Hits         2317     2199     -118     
+ Misses         18       14       -4     
+ Partials       14       12       -2     
Impacted Files Coverage Δ
tree.go 100.00% <100.00%> (ø)
mode.go 91.66% <0.00%> (-8.34%) ⬇️
render/json.go 87.34% <0.00%> (-0.47%) ⬇️
gin.go 99.11% <0.00%> (-0.04%) ⬇️
auth.go 100.00% <0.00%> (ø)
path.go 100.00% <0.00%> (ø)
render/text.go 100.00% <0.00%> (ø)
routergroup.go 100.00% <0.00%> (ø)
render/reader.go 100.00% <0.00%> (ø)
... and 9 more

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 1d5b9ba...8da51b6. Read the comment docs.

@appleboy
Copy link
Member

appleboy commented May 21, 2020

I think we can't merge this feature since of another framework like Echo or Fasthttp don't have the same rule and strings.ToLower has too lower performance.

result:

BenchmarkLowercaseString-4        242542              5108 ns/op            1101 B/op         27 allocs/op
BenchmarkLowercaseBytes-4         416874              3381 ns/op            1024 B/op         18 allocs/op

source code

func BenchmarkLowercaseString(B *testing.B) {
	B.ReportAllocs()
	B.ResetTimer()
	s := strings.Repeat("foo_Bar", 64)
	f := strings.Repeat("Foo_Bar", 64)
	for i := 0; i < B.N; i++ {
		_ = strings.ToLower(s) == strings.ToLower(f)
	}
}

func BenchmarkLowercaseBytes(B *testing.B) {
	B.ReportAllocs()
	B.ResetTimer()
	s := strings.Repeat("foo_Bar", 64)
	f := strings.Repeat("Foo_Bar", 64)
	for i := 0; i < B.N; i++ {
		_ = bytes.EqualFold([]byte(s), []byte(f))
	}
}

@tslling
Copy link

tslling commented Jun 18, 2020

Maybe we can use case-insensitive comparison as a backup, something like this:

if s == f || strings.EqualFold(s, f) {
    ...
}

In this way, we can support case-insensitive feature without costing too much.

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.

None yet

5 participants