binding:fix empty value error by guonaihong · Pull Request #2169 · gin-gonic/gin

guonaihong · 2019-12-06T07:55:45Z

Here is the code that can report an error

package main

import (
	"fmt"
	"github.com/gin-gonic/gin"
	"io"
	"net/http"
	"os"
	"time"
)

type header struct {
	Duration   time.Duration `header:"duration"`
	CreateTime time.Time     `header:"createTime" time_format:"unix"`
}

func needFix1() {
	g := gin.Default()
	g.GET("/", func(c *gin.Context) {
		h := header{}
		err := c.ShouldBindHeader(&h)
		if err != nil {
			c.JSON(500, fmt.Sprintf("fail:%s\n", err))
			return
		}

		c.JSON(200, h)
	})

	g.Run(":8081")
}

func needFix2() {
	g := gin.Default()
	g.GET("/", func(c *gin.Context) {
		h := header{}
		err := c.ShouldBindHeader(&h)
		if err != nil {
			c.JSON(500, fmt.Sprintf("fail:%s\n", err))
			return
		}

		c.JSON(200, h)
	})

	g.Run(":8082")
}

func sendNeedFix1() {
	// send to needFix1
	sendBadData("http://127.0.0.1:8081", "duration")
}

func sendNeedFix2() {
	// send to needFix2
	sendBadData("http://127.0.0.1:8082", "createTime")
}

func sendBadData(url, key string) {
	req, err := http.NewRequest("GET", "http://127.0.0.1:8081", nil)
	if err != nil {
		fmt.Printf("err:%s\n", err)
		return
	}

	// Only the key and no value can cause an error
	req.Header.Add(key, "")
	rsp, err := http.DefaultClient.Do(req)
	if err != nil {
		return
	}
	io.Copy(os.Stdout, rsp.Body)
	rsp.Body.Close()
}

func main() {
	go needFix1()
	go needFix2()

	time.Sleep(time.Second / 1000 * 200) // 200ms
	sendNeedFix1()
	sendNeedFix2()
}

codecov · 2019-12-06T08:00:21Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.68%. Comparing base (3dc1cd6) to head (e0ee47d).
⚠️ Report is 222 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2169      +/-   ##
==========================================
- Coverage   99.21%   98.68%   -0.53%     
==========================================
  Files          42       44       +2     
  Lines        3182     2976     -206     
==========================================
- Hits         3157     2937     -220     
- Misses         17       26       +9     
- Partials        8       13       +5

Flag	Coverage Δ
	`?`
--ldflags="-checklinkname=0" -tags sonic	`98.67% <100.00%> (?)`
-tags go_json	`98.61% <100.00%> (?)`
-tags nomsgpack	`98.67% <100.00%> (?)`
go-1.18	`?`
go-1.19	`?`
go-1.20	`?`
go-1.21	`?`
go-1.24	`98.68% <100.00%> (?)`
go-1.25	`98.68% <100.00%> (?)`
macos-latest	`98.68% <100.00%> (-0.53%)`	⬇️
ubuntu-latest	`98.68% <100.00%> (-0.53%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

appleboy · 2020-01-16T06:48:29Z

@vkd @thinkerou Any feedbacks?

vkd · 2020-01-16T09:26:33Z

+
+	// ok
+	tests := []bindTestData{
+		{need: &needFixUnixNanoEmpty{}, got: &needFixUnixNanoEmpty{}, in: formSource{"createTime": []string{"   "}}},


The non-empty string does not look to me as a default empty value. For me it looks like a wrong value.

The strconv.ParseInt function in Go will report an error when it encounters data beginning with a space. The function of removing spaces is added so that the function does not report an error.

package main import ( "fmt" "strconv" ) func main() { fmt.Println(strconv.ParseInt(" 5", 10, 0)) fmt.Println(strconv.ParseInt("5", 10, 0)) } /* 0 strconv.ParseInt: parsing " 5": invalid syntax 5 <nil> */

Yes, and from my point of view the binding shouldn't do this.

vkd · 2020-01-16T11:27:06Z

 }

 func setWithProperType(val string, value reflect.Value, field reflect.StructField) error {
+	if value.Kind() != reflect.String {


This change is not explained by the PR description.
Please update PR description with explanation what cases have you fixed and why.

If it is a string type, no spaces are removed, and the user data is not modified here.

Please document your changes in the PR's description. Just add information about what is changed and why.

guonaihong · 2020-01-18T06:31:37Z

Vote for this pr dispute. @appleboy @vkd @thinkerou
When dealing with int, float and other types, I recommend adding the string.TrimSpace function. The reason is that go's strconv.ParseUint and other functions will report an error when encountering data beginning with a space.

package main

import (
	"fmt"
	"strconv"
)

func main() {
	fmt.Println(strconv.ParseInt("  5", 10, 0))
	fmt.Println(strconv.ParseInt("5", 10, 0))
}

/*
0 strconv.ParseInt: parsing "  5": invalid syntax
5 <nil>

*/

strings.TrimSpace + 1

vkd · 2020-01-19T13:04:45Z

By my perspective, adding strings.TrimSpace will make the gin bindings is not unified.
It looks not usual for gin - because, as example, json binding doesn't do that kind of conversion.
I think keeping the same behavior for all kind of bindings is better.
@guonaihong thanks for your PR, but I vote for the not using the strings.TrimSpace in headers/form bindings.

strings.TrimSpace - 1

thinkerou · 2020-02-28T13:12:52Z

 func setWithProperType(val string, value reflect.Value, field reflect.StructField) error {
+	// If it is a string type, no spaces are removed, and the user data is not modified here
+	if value.Kind() != reflect.String {
+		val = strings.TrimSpace(val)


If val is not string type, can call func TrimSpace(s string) string?

The type of val is string, as explained below.
If the client command is the following code

curl -H 'intkey: v1" -H "strkey: v2" url
We define the following structure to parse the http header

type needHeader struct { IntKey int `header:"intkey" StrKey string `header:"strkey" }

Such a data structure.
Before setting into the structure, regardless of the int or string required by the structure, it is of type string before being parsed.

appleboy · 2020-03-20T15:42:06Z

@thinkerou move to 1.7 milestones?

thinkerou · 2020-03-21T01:19:45Z

@thinkerou move to 1.7 milestones?

OK, thanks!

appleboy · 2025-11-28T02:25:30Z

@guonaihong, please help resolve conflicts

guonaihong · 2025-11-28T02:57:15Z

No problem,tonight.

…

---Original--- From: "Bo-Yi ***@***.***> Date: Fri, Nov 28, 2025 10:25 AM To: ***@***.***>; Cc: ***@***.******@***.***>; Subject: Re: [gin-gonic/gin] binding:fix empty value error (#2169) appleboy left a comment (gin-gonic/gin#2169) @guonaihong, please help resolve conflicts — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

Here is the code that can report an error ```go package main import ( "fmt" "github.com/gin-gonic/gin" "io" "net/http" "os" "time" ) type header struct { Duration time.Duration `header:"duration"` CreateTime time.Time `header:"createTime" time_format:"unix"` } func needFix1() { g := gin.Default() g.GET("/", func(c *gin.Context) { h := header{} err := c.ShouldBindHeader(&h) if err != nil { c.JSON(500, fmt.Sprintf("fail:%s\n", err)) return } c.JSON(200, h) }) g.Run(":8081") } func needFix2() { g := gin.Default() g.GET("/", func(c *gin.Context) { h := header{} err := c.ShouldBindHeader(&h) if err != nil { c.JSON(500, fmt.Sprintf("fail:%s\n", err)) return } c.JSON(200, h) }) g.Run(":8082") } func sendNeedFix1() { // send to needFix1 sendBadData("http://127.0.0.1:8081", "duration") } func sendNeedFix2() { // send to needFix2 sendBadData("http://127.0.0.1:8082", "createTime") } func sendBadData(url, key string) { req, err := http.NewRequest("GET", "http://127.0.0.1:8081", nil) if err != nil { fmt.Printf("err:%s\n", err) return } // Only the key and no value can cause an error req.Header.Add(key, "") rsp, err := http.DefaultClient.Do(req) if err != nil { return } io.Copy(os.Stdout, rsp.Body) rsp.Body.Close() } func main() { go needFix1() go needFix2() time.Sleep(time.Second / 1000 * 200) // 200ms sendNeedFix1() sendNeedFix2() } ```

- Replace 'interface{}' with 'any' alias in bindTestData struct - Change assert.NoError to require.NoError in TestMappingTimeUnixNano and TestMappingTimeDuration to fail fast on mapping errors

appleboy · 2025-11-30T13:31:06Z

@thinkerou @vkd Please help review the PR.

guonaihong · 2025-11-30T13:31:36Z

Conflicts have been resolved @appleboy

vkd

The changes look good to me!

appleboy · 2025-12-03T11:18:20Z

@guonaihong Thanks.

guonaihong changed the title ~~fix empty value error~~ binding:fix empty value error Dec 6, 2019

vkd reviewed Dec 7, 2019

View reviewed changes

Comment thread binding/binding_test.go Outdated

Comment thread binding/binding_test.go Outdated

appleboy previously approved these changes Jan 16, 2020

View reviewed changes

appleboy requested review from appleboy and thinkerou January 16, 2020 06:47

appleboy added the type/bug Found something you weren't expecting? Report it here! label Jan 16, 2020

appleboy added this to the 1.6 milestone Jan 16, 2020

vkd reviewed Jan 16, 2020

View reviewed changes

guonaihong dismissed appleboy’s stale review via 3623763 January 16, 2020 14:53

thinkerou reviewed Feb 28, 2020

View reviewed changes

thinkerou modified the milestones: 1.6, 1.7 Mar 21, 2020

thinkerou modified the milestones: 1.7, v1.8 Nov 8, 2020

daheige approved these changes May 1, 2021

View reviewed changes

thinkerou modified the milestones: v1.8, v1.9 May 28, 2022

thinkerou modified the milestones: v1.9, v1.10 Jan 20, 2023

appleboy modified the milestones: v1.10, v1.11 Mar 21, 2024

appleboy modified the milestones: v1.11, v1.12 Jun 21, 2025

guonaihong added 3 commits November 30, 2025 20:59

modify code

6341a25

add comment

2891214

guonaihong force-pushed the fix-binding-empty-value branch from 6655232 to 2891214 Compare November 30, 2025 13:05

test(binding): use 'any' alias and require.NoError in form mapping tests

af5561c

- Replace 'interface{}' with 'any' alias in bindTestData struct - Change assert.NoError to require.NoError in TestMappingTimeUnixNano and TestMappingTimeDuration to fail fast on mapping errors

appleboy requested review from thinkerou and vkd November 30, 2025 13:30

vkd approved these changes Dec 1, 2025

View reviewed changes

Merge branch 'master' into fix-binding-empty-value

e0ee47d

appleboy approved these changes Dec 3, 2025

View reviewed changes

appleboy merged commit b917b14 into gin-gonic:master Dec 3, 2025
26 of 27 checks passed

campaigner-prod Bot mentioned this pull request Apr 23, 2026

fix(deps): vuln minor upgrades — 15 packages (minor: 3 · patch: 12) [sticker-award] DataDog/stickerlandia#257

Closed

4 tasks

Conversation

guonaihong commented Dec 6, 2019

Uh oh!

codecov Bot commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

appleboy commented Jan 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guonaihong commented Jan 18, 2020

Uh oh!

vkd commented Jan 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

appleboy commented Mar 20, 2020

Uh oh!

thinkerou commented Mar 21, 2020

Uh oh!

appleboy commented Nov 28, 2025

Uh oh!

guonaihong commented Nov 28, 2025 via email

Uh oh!

appleboy commented Nov 30, 2025

Uh oh!

guonaihong commented Nov 30, 2025

Uh oh!

vkd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

appleboy commented Dec 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Dec 6, 2019 •

edited

Loading