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

support bind http header param #1956 #1957

Merged

Conversation

guonaihong
Copy link
Contributor

update #1956

package main

import (
	"fmt"
	"github.com/gin-gonic/gin"
)

type testHeader struct {
	Rate   int    `header:"Rate"`
	Domain string `header:"Domain"`
}

func main() {
	r := gin.Default()
	r.GET("/", func(c *gin.Context) {
		h := testHeader{}

		if err := c.ShouldBindHeader(&h); err != nil {
			c.JSON(200, err)
		}

		fmt.Printf("%#v\n", h)
		c.JSON(200, gin.H{"Rate": h.Rate, "Domain": h.Domain})
	})

	r.Run()

// client
// curl -H "rate:300" -H "domain:music" 127.0.0.1:8080/
// output
// {"Domain":"music","Rate":300}
}

update gin-gonic#1956
```
package main

import (
	"fmt"
	"github.com/gin-gonic/gin"
)

type testHeader struct {
	Rate   int    `header:"Rate"`
	Domain string `header:"Domain"`
}

func main() {
	r := gin.Default()
	r.GET("/", func(c *gin.Context) {
		h := testHeader{}

		if err := c.ShouldBindHeader(&h); err != nil {
			c.JSON(200, err)
		}

		fmt.Printf("%#v\n", h)
		c.JSON(200, gin.H{"Rate": h.Rate, "Domain": h.Domain})
	})

	r.Run()

// client
// curl -H "rate:300" -H "domain:music" 127.0.0.1:8080/
// output
// {"Domain":"music","Rate":300}
}
```
@codecov
Copy link

codecov bot commented Jun 18, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1957      +/-   ##
==========================================
+ Coverage   98.77%   98.77%   +<.01%     
==========================================
  Files          39       40       +1     
  Lines        2198     2212      +14     
==========================================
+ Hits         2171     2185      +14     
  Misses         15       15              
  Partials       12       12
Impacted Files Coverage Δ
binding/binding.go 100% <ø> (ø) ⬆️
binding/header.go 100% <100%> (ø)
context.go 98.44% <100%> (+0.01%) ⬆️

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 09a3650...adef0a8. Read the comment docs.

@appleboy
Copy link
Member

@guonaihong Please write the unit test.

@appleboy appleboy added this to the 1.5 milestone Jun 20, 2019
@guonaihong
Copy link
Contributor Author

@appleboy ok

When the http header is obtained in the standard library,
the key value will be modified by the CanonicalMIMEHeaderKey function,
and finally the value of the http header will be obtained from the map.
As follows.
```go
func (h MIMEHeader) Get(key string) string {
        // ...
         v := h[CanonicalMIMEHeaderKey(key)]
        // ...
}
```

This pr also follows this modification
)

if opt.isHeader {
vs, ok = form[textproto.CanonicalMIMEHeaderKey(tagValue)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to implement a header mapping.

In my mind, you complicate a little the current implementation of the form mapping.

This is my suggestion of an implementation (add to your new binding/header.go file):

func mapHeader(ptr interface{}, h map[string][]string) error {
	return mappingByPtr(ptr, headerSource(h), "header")
}

type headerSource map[string][]string

var _ setter = headerSource(nil)

func (hs headerSource) TrySet(value reflect.Value, field reflect.StructField, tagValue string, opt setOptions) (isSetted bool, err error) {
	return setByForm(value, field, hs, textproto.CanonicalMIMEHeaderKey(tagValue), opt)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good idea, I am going to modify it at night

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, thanks!

You can even uncomment the //var _ setter = headerSource(nil) line, it is legal =)

return "header"
}

func (headerBinding) Bind(req *http.Request, obj interface{}) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, is a good idea to add unit tests inside of binding package to cover it by 100% inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

Choose a reason for hiding this comment

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

Please make a coverage of binding package to 100%. (binding/header.go)

This can help you:

cd binding
go test -coverprofile=cover.prof
go tool cover -html=cover.prof

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good way,have submitted changes

env GOPATH=`pwd` go test github.com/gin-gonic/gin/binding -coverprofile=cover.prof
ok  	github.com/gin-gonic/gin/binding	0.015s	coverage: 100.0% of statements
@@ -38,8 +38,6 @@ type setter interface {

type formSource map[string][]string

var _ setter = formSource(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, return back this line.
And it will be perfect, in my mind, if you add var _ setter = headerSource(nil) after this line. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't quite understand the meaning of this line of code.
Var _ setter = formSource(nil)
Can you tell me why?
Doesn't it work?

Copy link
Member

Choose a reason for hiding this comment

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

check formSource implement setter interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is just checking in compile time, what this type is implemented that interface.

This line is optional, but it is for a good self-documented code. It simply shows why this new type was added. This is a good practice when you add a new type only for a specific interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

demo

import (
        "reflect"
)

type setOptions struct {
        isDefaultExists bool
        defaultValue    string
}

type setter interface {
        TrySet(value reflect.Value, field reflect.StructField, key string, opt setOptions) (isSetted bool, err error)
}

func mappingByPtr(ptr interface{}, setter setter, tag string) error {
        //_, err := mapping(reflect.ValueOf(ptr), emptyField, setter, tag)
        return nil
}

type formSource map[string][]string

func (f formSource) TrySet(value reflect.Value, field reflect.StructField, key string, opt setOptions) (isSetted bool, err error) {
        return true, nil
}

type headerSource map[string][]string

func main() {

        mappingByPtr(reflect.ValueOf(nil), formSource(map[string][]string{}), "test")
        mappingByPtr(reflect.ValueOf(nil), headerSource(map[string][]string{}), "test")

}

output

./interface.go:32:49: cannot use headerSource(map[string][]string literal) (type headerSource) as type setter in argument to mappingByPtr:
headerSource does not implement setter (missing TrySet method)

idea

The compiler has checked when the parameter is assigned. It doesn't seem to be necessary code.
I will check the code tomorrow morning.

@thinkerou
Copy link
Member

@guonaihong please add use case to README.md, thanks!

Copy link
Contributor

@vkd vkd left a comment

Choose a reason for hiding this comment

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

Excellent, thanks!

@thinkerou thinkerou merged commit f98b339 into gin-gonic:master Jun 27, 2019
ThomasObenaus pushed a commit to ThomasObenaus/gin that referenced this pull request Feb 19, 2020
* support bind http header param gin-gonic#1956

update gin-gonic#1956
```
package main

import (
	"fmt"
	"github.com/gin-gonic/gin"
)

type testHeader struct {
	Rate   int    `header:"Rate"`
	Domain string `header:"Domain"`
}

func main() {
	r := gin.Default()
	r.GET("/", func(c *gin.Context) {
		h := testHeader{}

		if err := c.ShouldBindHeader(&h); err != nil {
			c.JSON(200, err)
		}

		fmt.Printf("%#v\n", h)
		c.JSON(200, gin.H{"Rate": h.Rate, "Domain": h.Domain})
	})

	r.Run()

// client
// curl -H "rate:300" -H "domain:music" 127.0.0.1:8080/
// output
// {"Domain":"music","Rate":300}
}
```

* add unit test

* Modify the code to get the http header

When the http header is obtained in the standard library,
the key value will be modified by the CanonicalMIMEHeaderKey function,
and finally the value of the http header will be obtained from the map.
As follows.
```go
func (h MIMEHeader) Get(key string) string {
        // ...
         v := h[CanonicalMIMEHeaderKey(key)]
        // ...
}
```

This pr also follows this modification

* Thanks to vkd for suggestions, modifying code

* Increase test coverage

env GOPATH=`pwd` go test github.com/gin-gonic/gin/binding -coverprofile=cover.prof
ok  	github.com/gin-gonic/gin/binding	0.015s	coverage: 100.0% of statements

* Rollback check code

* add use case to README.md
var _ setter = headerSource(nil)

func (hs headerSource) TrySet(value reflect.Value, field reflect.StructField, tagValue string, opt setOptions) (isSetted bool, err error) {
return setByForm(value, field, hs, textproto.CanonicalMIMEHeaderKey(tagValue), opt)

Choose a reason for hiding this comment

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

the tag value of CanonicalMIMEHeaderKey is used instead of the actual tag value. This should be mentioned in the docs.

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

Successfully merging this pull request may close these issues.

5 participants