Fix Bindings for empty fields #1578
Conversation
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed within a month if no further activity occurs. Thank you for your contributions. |
Codecov Report
@@ Coverage Diff @@
## master #1578 +/- ##
==========================================
+ Coverage 84.96% 91.98% +7.02%
==========================================
Files 28 34 +6
Lines 2168 3793 +1625
==========================================
+ Hits 1842 3489 +1647
+ Misses 211 201 -10
+ Partials 115 103 -12
Continue to review full report at Codecov.
|
|
@lammel I think we should merge this. Bool having true/false is one thing but overwriting values with da type default value makes chaining different binding functions impossible - as it will overwrite previously successfully bound values. See #1521 (comment) |
|
As this change existing behavior I'd prefer merge for v5. There might be some implementations that rely on the current (mis)behaviour and we would suddenly break those with a new tag. |
|
I have to admit that I am wrong here. I did some additional tests and debugged bind. Part where I state that each Bind overwrites previous value is half true. Field is overwritten only if that |
Dear @aldas, I didn't quite catch why this is OK. Could you please kindly help me understand? In the issue #1521 I have stated that it would be great to validate if the field was present or not and "defaulting" missing fields makes it impossible. |
|
This is because you should end up in Example: func main() {
e := echo.New()
e.GET("/", func(c echo.Context) error {
var params struct {
ID int64 `query:"id" form:"id"`
}
params.ID = 100
if err := c.Bind(¶ms); err != nil {
return err
}
return c.String(http.StatusOK, fmt.Sprintf("%+v\n", params))
})
log.Fatal(e.Start(":8080"))
}Output for some test cases: when no query param is provided - field is not bound x@x:~/code/$ curl http://localhost:8080/
{ID:100}when value is provide - field is bound x@x:~/code/$ curl http://localhost:8080/?id=1
{ID:1}when empty value is provided - field is bound to default value x@x:~/code/$ curl http://localhost:8080/?id=
{ID:0}There is a quite significant difference between providing value (even if empty) and not providing (first means that you have taken time to provide that field in query/form values - if even if empty it means that empty value has been sent intentionally). If we would ignore "empty" values then you would not be able to send explicit empty values |
|
Same with bool func main() {
e := echo.New()
e.GET("/", func(c echo.Context) error {
var params struct {
ID int64 `query:"id" form:"id"`
Bool bool `query:"b" form:"b"`
}
params.ID = 100
params.Bool = true
if err := c.Bind(¶ms); err != nil {
return err
}
return c.String(http.StatusOK, fmt.Sprintf("%+v\n", params))
})
log.Fatal(e.Start(":8080"))
}Output: x@x:~/code/$ curl http://localhost:8080/
{ID:100 Bool:true}
x@x:~/code/$ curl http://localhost:8080/?b=
{ID:100 Bool:false}
x@x:~/code/$ curl http://localhost:8080/?b=true
{ID:100 Bool:true}
x@x:~/code/$ curl http://localhost:8080/?b=false
{ID:100 Bool:false} |
|
and after scrolling up and checking what was the original problem
now I have to agree with you. Probably treating "empty" as default value is not appropriate when json unmarshaller does not allow it. I personally considered it from the use-case when you chain multiple binding methods and if it overwrites or not and not in perspective if "empty" should be allowed to be fail at conversion from string to type x@x:~/code/$ curl http://localhost:8080/?b=
{ID:100 Bool:false}@lammel should this be error when empty bool fails with json? |
|
In that case I would expect x@x:~/code/$ curl http://localhost:8080/?b= And if validating further with c.Validate(req) (where c is echo.Context) it would fail with the error and that was exactly what was needed) there is no other way to properly validate the request on missing fields if we "defaulting them". This PR was fixing the issue. |
|
func main() {
e := echo.New()
e.GET("/", func(c echo.Context) error {
var params struct {
ID int64 `query:"id" form:"id"`
Bool *bool `query:"b" form:"b"`
}
params.ID = 100
if err := c.Bind(¶ms); err != nil {
return err
}
return c.String(http.StatusOK, fmt.Sprintf("%+v\n", params))
})
log.Fatal(e.Start(":8080"))
}Output x@x:~/code/$ curl http://localhost:8080/
{ID:100 Bool:<nil>} |
|
This is example how empty bool fails with json func main() {
e := echo.New()
e.POST("/", func(c echo.Context) error {
var params struct {
ID int64 `query:"id" form:"id" json:"id"`
Bool bool `query:"b" form:"b" json:"b"`
}
params.ID = 100
params.Bool = true
if err := c.Bind(¶ms); err != nil {
return err
}
return c.String(http.StatusOK, fmt.Sprintf("%+v\n", params))
})
log.Fatal(e.Start(":8080"))
}x@x:~/code/$ curl -H 'Content-Type: application/json' http://localhost:8080/?b=true -d '{}'
{ID:100 Bool:true}
x@x:~/code/$ curl -H 'Content-Type: application/json' http://localhost:8080/ -d '{}'
{ID:100 Bool:true}
x@x:~/code/$ curl -H 'Content-Type: application/json' http://localhost:8080/?b=false -d '{}'
{ID:100 Bool:true}
x@x:~/code/$ curl -H 'Content-Type: application/json' http://localhost:8080/?b=false -d ''
{ID:100 Bool:true}
x@x:~/code/$ curl -H 'Content-Type: application/json' http://localhost:8080/ -d '{"b":false}'
{ID:100 Bool:false}
x@x:~/code/$ curl -H 'Content-Type: application/json' http://localhost:8080/ -d '{"b":""}'
{"message":"Unmarshal type error: expected=bool, got=string, field=b, offset=7"} |
|
Yes, I am sorry, I think I lost some context over the time. You are right, what I meant was if we remove the defaulting in the code, binding will fail if the value is not present. *bool is what I ended up using for a proper validation. I think I started having second thoughts about defaulting. If we remove the defaulting, all the fields will be automatically "required", right? and this is not what we want either. |
|
If we would remove defaulting empty values we would only affect use cases like This is because for first URL Binder gets empty string |
|
Oh, yes, thank you for pointing that out! Hence, I think we should remove the defaulting. What do you think? |
|
I think for for ints/floats/bool empty value should have same result as it would have with JSON. I think it is expected behavior to have an error when you try to convert |
|
@lammel could you check and validate. I have had bad takes in this topic. |
This PR is related to the already discussed issue (#1521).
Without that fix empty fields have default values during binding that does not fail. This situation makes it difficult to validate requests later.
For instance, I had an empty required bool field in the request. It turns into
false, binding works without error and the filed passes subsequent validation. This is not a valid behaviour. We would like bindings to fail in such cases like json.Umsarshal. If you try to unmarshal empty field into a bool, it will fail with error.PR should be tagged as v5.