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

Fix #216: Enable to call binding multiple times in some formats #1341

Merged
merged 10 commits into from
May 11, 2018

Conversation

delphinus
Copy link

close #216

Reported problems

#216 says some formats in requests' body cannot be bound into different structs by calling multiple times.

type modelA struct {
  Foo string `form:"foo" json:"foo" binding:"required"`
  Bar int64  `form:"bar" json:"bar" binding:"required"`
}

type modelB struct {
  Hoge string `form:"hoge" json:"hoge" xml:"hoge" binding:"required"`
  Fuga int64  `form:"fuga" json:"fuga" xml:"fuga" binding:"required"`
}

func main() {
  r := gin.New()
  r.POST("/json", func(c *gin.Context) {
    fa, fb := modelA{}, modelB{}
    if aerr := c.ShouldBindWith(&fa, binding.JSON); aerr == nil {
      c.JSON(http.StatusOK, gin.H{"form": fa})
    } else if berr := c.ShouldBindWith(&fb, binding.JSON); berr == nil {
      c.JSON(http.StatusOK, gin.H{"form": fb})
    } else {
      c.AbortWithStatusJSON(
        http.StatusBadRequest,
        gin.H{
          "errors": gin.H{
            "modelA": aerr.Error(),
            "modelB": berr.Error(),
          },
        },
      )
    }
  })
  r.Run("0:8080")
}
# modelA can be bound successfully
$ curl -X POST -d '{"foo":"AAA","bar":123}' 0:8080/json | jq
{
  "form": {
    "foo": "AAA",
    "bar": 123
  }
}

# but modelB cannot
$ curl -X POST -d '{"hoge":"BBB","fuga":123}' 0:8080/json | jq
{
  "errors": {
    "modelA": "Key: 'modelA.Bar' Error:Field validation for 'Bar' failed on the 'req
ired' tag\nKey: 'modelA.Foo' Error:Field validation for 'Foo' failed on the 'require
' tag",
    "modelB": "EOF"
  }
}

Proposal to solve this

This PR solves this by adding a new interface binding.BindingBody & a new method c.ShouldBindBodyWith.

...
    // use ShouldBindBodyWith
    if aerr := c.ShouldBindBodyWith(&fa, binding.JSON); aerr == nil {
      c.JSON(http.StatusOK, gin.H{"form": fa})
    } else if berr := c.ShouldBindBodyWith(&fb, binding.JSON); berr == nil {
      c.JSON(http.StatusOK, gin.H{"form": fb})
    } else {
...
# successfully modelB bound
$ curl -X POST -d '{"hoge":"BBB","fuga":123}' 0:8080/json | jq
{
  "form": {
    "hoge": "BBB",
    "fuga": 123
  }
}

And you can mix different formats.

...
    if aerr := c.ShouldBindBodyWith(&fa, binding.JSON); aerr == nil {
      c.JSON(http.StatusOK, gin.H{"form": fa})
    } else if berr := c.ShouldBindBodyWith(&fb, binding.XML); berr == nil {
      c.JSON(http.StatusOK, gin.H{"form": fb})
    } else {
...
$ curl -X POST -d '<?xml version="1.0" encoding="UTF-8"?>\n<root>\n<fuga>123</fuga>\n<hoge>BBB</hoge>\n</root>' 0:8080/json | jq
{
  "form": {
    "hoge": "BBB",
    "fuga": 123
  }
}

I added implementations for JSON, XML, msgpack and protobuf. Other formats, query & forms, are already available to be called multiple times, because they uses parsed req.Form (url.Values) instead of req.Body.

Question

  • If you prefer to this feature, I will add unit tests.
  • I'm worrying about cool naming for bind.BindingBody, c.ShouldBindBodyWith. I'm happy if you give better names.

@codecov
Copy link

codecov bot commented Apr 29, 2018

Codecov Report

Merging #1341 into master will decrease coverage by 0.08%.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1341      +/-   ##
==========================================
- Coverage   98.27%   98.19%   -0.09%     
==========================================
  Files          34       34              
  Lines        1800     1826      +26     
==========================================
+ Hits         1769     1793      +24     
- Misses         25       26       +1     
- Partials        6        7       +1
Impacted Files Coverage Δ
binding/binding.go 100% <ø> (ø) ⬆️
binding/msgpack.go 100% <100%> (ø) ⬆️
binding/json.go 100% <100%> (ø) ⬆️
binding/protobuf.go 100% <100%> (ø) ⬆️
binding/xml.go 100% <100%> (ø) ⬆️
context.go 96.05% <81.81%> (-0.46%) ⬇️

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 6e09ef0...bfb9aab. Read the comment docs.

@appleboy appleboy requested review from appleboy, javierprovecho, nazwa and a team April 30, 2018 05:51
@appleboy
Copy link
Member

appleboy commented May 1, 2018

@thinkerou Can you also help to review this PR? Thanks.

@thinkerou
Copy link
Member

@appleboy OK~

context.go Outdated
@@ -31,6 +31,7 @@ const (
MIMEPlain = binding.MIMEPlain
MIMEPOSTForm = binding.MIMEPOSTForm
MIMEMultipartPOSTForm = binding.MIMEMultipartPOSTForm
BodyBytesKey = "github.com/gin-gonic/gin/bodyBytes"
Copy link
Member

@thinkerou thinkerou May 1, 2018

Choose a reason for hiding this comment

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

Sorry, I don't understand it's mean, a little surprising.
Ohh, understand, but the value maybe BindBodyBytesKey better?

Copy link
Author

Choose a reason for hiding this comment

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

Sure. fixed.

Copy link
Member

Choose a reason for hiding this comment

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

I mean: BodyBytesKey = "BindBodyBytesKey", actually I see github.com/gin-gonic/gin/bodyBytes at first feel like import, do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, yes, but it is a bit dangerous. c.Get(key)'s key is only a string and this is easy to conflict with user-defined keys. I meant the full import-pathish strings as a private key.

I grepped sources and found BindKey = "_gin-gonic/gin/bindkey" (here). Following this, I want to use BodyBytesKey = "_gin-gonic/gin/bodybyteskey", how about this?

Copy link
Member

Choose a reason for hiding this comment

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

Good, agreed!

@thinkerou
Copy link
Member

Hi, @delphinus , the PR not need to add BindBody to query and form? Thanks!

context.go Outdated
// ShouldBindWith for better performance if you need to call only once.
func (c *Context) ShouldBindBodyWith(
obj interface{}, bb binding.BindingBody,
) (err error) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe not need named return.

Copy link
Author

Choose a reason for hiding this comment

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

Without this, I should add an explicit declaration var err error because this is needed for reusing body in L527.

// if you use unnamed return value...
func (...) ShouldBindBodyWith(...) error {

  ...

  if body == nil {
    // you need explicit declaration here.
    var err error
    body, err = ioutil.ReadAll(...)
    if err != nil {
      return err
    }
    ...
  }
  return ...
}

I want to simplify the code and use this named return variable.

Copy link
Member

Choose a reason for hiding this comment

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

OK, got it! Thanks~

context.go Outdated
// ShouldBindBodyWith is similar with ShouldBindWith, but it stores the request
// body into the context, and reuse when it is called again.
//
// NOTE: This method reads the body before binding. So you should use
Copy link
Member

Choose a reason for hiding this comment

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

Please use two spaces, thanks.

@@ -29,6 +29,13 @@ type Binding interface {
Bind(*http.Request, interface{}) error
}

// BindingBody adds BindBody method to Binding. BindBody is similar with Bind,
Copy link
Member

Choose a reason for hiding this comment

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

Please use two spaces, thanks.

Copy link
Author

Choose a reason for hiding this comment

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

You mean “one space”, right? mean to replace ...˽Binding.˽˽BindBody˽is... into ...˽Binding.˽BindBody˽is... ?

Copy link
Member

Choose a reason for hiding this comment

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

Oh....so sorry, one space!

@thinkerou
Copy link
Member

Hi, @delphinus @appleboy Except for comments, need to add unit test and README, it's OK then LGTM to me. Thanks!

@appleboy appleboy added this to the 1.3 milestone May 2, 2018
@delphinus
Copy link
Author

Hi, @delphinus , the PR not need to add BindBody to query and form? Thanks!

No, it is not needed.

Query binding parses req.URL.Query() every times when it is called.

Form, FormPost and FormMultipart parses req.URL.Query() and/or req.Body and store them into req.Form, req.PostForm when it is called at the first time. And it will reuse at the second time and after.

I will add the tests and try to confirm this.

@appleboy
Copy link
Member

appleboy commented May 3, 2018

Please add some testing and example in readme.

@delphinus
Copy link
Author

Added tests & README. Review again @appleboy 👍

And I wrote gist for Query and FormPost. They can be called by the original c.ShouldBind multiple times and do not need this feature.

https://gist.github.com/delphinus/9db93c9ece3dc0bf093986a8e11c3fb7

@appleboy appleboy merged commit 995fa8e into gin-gonic:master May 11, 2018
@delphinus delphinus deleted the feature/multiple-binding branch May 11, 2018 02:35
@jeffxf jeffxf mentioned this pull request Jul 3, 2018
@thinkerou thinkerou mentioned this pull request Aug 21, 2018
toby1991 added a commit to totoval/framework that referenced this pull request Mar 6, 2019
toby1991 added a commit to totoval/framework that referenced this pull request Mar 6, 2019
toby1991 added a commit to totoval/framework that referenced this pull request May 10, 2019
toby1991 added a commit to totoval/framework that referenced this pull request May 10, 2019
@wulianglongrd
Copy link

out of curiosity, what is the benefit of adding the ShouldBindBodyWith method instead of supporting multiple reads of the body on the ShouldBindWith method? thanks.

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.

Calling bind multiple times
4 participants