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

delphinus · 2018-04-29T07:09:33Z

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 · 2018-04-29T07:16:16Z

Codecov Report

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

@@            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 · 2018-05-01T06:25:35Z

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

thinkerou · 2018-05-01T06:47:50Z

@appleboy OK~

thinkerou · 2018-05-01T07:08:29Z

context.go

@@ -31,6 +31,7 @@ const (
 	MIMEPlain             = binding.MIMEPlain
 	MIMEPOSTForm          = binding.MIMEPOSTForm
 	MIMEMultipartPOSTForm = binding.MIMEMultipartPOSTForm
+	BodyBytesKey          = "github.com/gin-gonic/gin/bodyBytes"


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

Sure. fixed.

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

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?

Good, agreed!

thinkerou · 2018-05-01T07:44:30Z

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

thinkerou · 2018-05-01T07:45:30Z

context.go

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


Maybe not need named return.

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.

OK, got it! Thanks~

thinkerou · 2018-05-02T01:56:23Z

context.go

+// 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


Please use two spaces, thanks.

thinkerou · 2018-05-02T01:56:45Z

binding/binding.go

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

+// BindingBody adds BindBody method to Binding.  BindBody is similar with Bind,


Please use two spaces, thanks.

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

Oh....so sorry, one space!

thinkerou · 2018-05-02T01:59:23Z

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

delphinus · 2018-05-03T08:22:00Z

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.

This reverts commit 2c82901.

appleboy · 2018-05-03T09:12:51Z

Please add some testing and example in readme.

delphinus · 2018-05-05T08:58:37Z

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

gin-gonic/gin#1341 Signed-off-by: Toby Yan <me@tobyan.com>

wulianglongrd · 2023-02-24T06:17:33Z

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.

delphinus added 2 commits April 29, 2018 15:15

Add interface to read body bytes in binding

3ae520b

Add BindingBody implementation for some binding

e825c37

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

Merge branch 'master' into feature/multiple-binding

1e0e4a8

thinkerou reviewed May 1, 2018

View reviewed changes

thinkerou reviewed May 2, 2018

View reviewed changes

appleboy added the feature label May 2, 2018

appleboy added this to the 1.3 milestone May 2, 2018

Fix to use BindBodyBytesKey for key

2c82901

delphinus added 2 commits May 3, 2018 17:47

Revert "Fix to use BindBodyBytesKey for key"

edc5f44

This reverts commit 2c82901.

Use private-like key for body bytes

e68e6d2

delphinus added 2 commits May 5, 2018 17:05

Add tests for BindingBody & ShouldBindBodyWith

09a2ba4

Add note for README

1cecbd3

delphinus force-pushed the feature/multiple-binding branch from fd12128 to 1cecbd3 Compare May 5, 2018 08:42

Remove redundant space between sentences

4a60eed

appleboy approved these changes May 11, 2018

View reviewed changes

Merge branch 'master' into feature/multiple-binding

bfb9aab

appleboy merged commit 995fa8e into gin-gonic:master May 11, 2018

appleboy mentioned this pull request May 11, 2018

Calling bind multiple times #216

Closed

delphinus deleted the feature/multiple-binding branch May 11, 2018 02:35

delphinus mentioned this pull request May 31, 2018

How to access the Request.Body after ShouldBind*() returns an error ? #1377

Closed

thinkerou mentioned this pull request Jun 28, 2018

Use BindJSON multiple times fails #1078

Closed

jeffxf mentioned this pull request Jul 3, 2018

get request body #1295

Closed

thinkerou mentioned this pull request Jul 26, 2018

c *gin.Context get parameter #1439

Closed

thinkerou mentioned this pull request Aug 21, 2018

BindJSON returns EOF #439

Closed

toby1991 added a commit to totoval/framework that referenced this pull request Mar 6, 2019

let basecontroller binding could bind multiple tim

0ebd99a

gin-gonic/gin#1341 Signed-off-by: Toby Yan <me@tobyan.com>

toby1991 added a commit to totoval/framework that referenced this pull request Mar 6, 2019

let basecontroller binding could bind multiple tim

d7c23f8

gin-gonic/gin#1341 Signed-off-by: Toby Yan <me@tobyan.com>

toby1991 added a commit to totoval/framework that referenced this pull request May 10, 2019

let basecontroller binding could bind multiple tim

f7a8bb2

gin-gonic/gin#1341 Signed-off-by: Toby Yan <me@tobyan.com>

toby1991 added a commit to totoval/framework that referenced this pull request May 10, 2019

let basecontroller binding could bind multiple tim

8573607

gin-gonic/gin#1341 Signed-off-by: Toby Yan <me@tobyan.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

delphinus commented Apr 29, 2018

codecov bot commented Apr 29, 2018 •

edited

appleboy commented May 1, 2018

thinkerou commented May 1, 2018

thinkerou May 1, 2018 •

edited

delphinus May 3, 2018

thinkerou May 3, 2018

delphinus May 3, 2018

thinkerou May 3, 2018

thinkerou commented May 1, 2018

thinkerou May 1, 2018

delphinus May 3, 2018

thinkerou May 3, 2018

thinkerou May 2, 2018

thinkerou May 2, 2018

delphinus May 3, 2018

thinkerou May 3, 2018

thinkerou commented May 2, 2018

delphinus commented May 3, 2018

appleboy commented May 3, 2018 •

edited

delphinus commented May 5, 2018

wulianglongrd commented Feb 24, 2023

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

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

Conversation

delphinus commented Apr 29, 2018

Reported problems

Proposal to solve this

Question

codecov bot commented Apr 29, 2018 • edited

Codecov Report

appleboy commented May 1, 2018

thinkerou commented May 1, 2018

thinkerou May 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thinkerou commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thinkerou commented May 2, 2018

delphinus commented May 3, 2018

appleboy commented May 3, 2018 • edited

delphinus commented May 5, 2018

wulianglongrd commented Feb 24, 2023

codecov bot commented Apr 29, 2018 •

edited

thinkerou May 1, 2018 •

edited

appleboy commented May 3, 2018 •

edited