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

How to apply default value if empty value provided by client during model binding? #4042

Closed
takanuva15 opened this issue Aug 29, 2024 · 8 comments

Comments

@takanuva15
Copy link
Contributor

takanuva15 commented Aug 29, 2024

Description

(Note I am not reporting a bug here, but rather asking a question)

I have a struct with a form tag specifying a default value. If the user provides an empty value for the query parameter during binding, I would like Gin to ignore it and use the default value provided in the form tag instead. Is there a combination of tags that would make this possible with Gin? (I looked at a lot of issues and couldn't seem to find anything that enabled this behavior)

(I am aware I can add a manual if-statement in the handler body to check-or-set my default fields. However, that adds duplicate code which I would prefer to simply resolve using the default= I already specified in the form tag. I also cannot simply force the client to stop sending the empty tag by using something like binding:"required", because that would break existing clients using our APIs which I don't have the ability to repair at the moment)

How to reproduce

package main

import (
	"net/http"

	"github.com/gin-gonic/gin"
)

type ReqStruct struct {
	Name string `form:"name,default=myDefault" binding:"omitempty"`
}

func main() {
	g := gin.Default()
	g.GET("/hello", func(c *gin.Context) {
		var req ReqStruct
		if err := c.ShouldBindQuery(&req); err != nil {
			c.JSON(http.StatusBadRequest, err)
		} else {
			c.JSON(http.StatusOK, req)
		}
	})
	_ = g.Run("localhost:9000")
}

Expectations

curl 'http://localhost:9000/hello?name='
{"Name":"myDefault"}

Actual result

curl 'http://localhost:9000/hello?name='
{"Name":""}

Environment

  • go version: go version go1.23.0 darwin/arm64
  • gin version (or commit ref): github.com/gin-gonic/gin v1.9.1
  • operating system: macOS Sonoma Version 14.6.1 (23G93)
@ahmadSaeedGoda
Copy link
Contributor

ahmadSaeedGoda commented Aug 29, 2024

But I think when you pass it like: curl 'http://localhost:9000/hello?name=' it means an empty string for the param value "the zero value for type string".

When not passed at all, its value will be nil, means value is absent. Then, and only then, the validation should take place and put "myDefault" instead.

Anyway, I am debugging it to find out some helpful/useful info to share once available. It should be something relevant to the zero value validation case. I do not think it is a bug so far. And I see it as intuitive and idiomatic behavior TBH.
@takanuva15

@ahmadSaeedGoda
Copy link
Contributor

Here are the debugging steps in order "neglecting few non-important ones":

2024-08-30_01-45
image
image
image
image
image
image
image
image
image

Turns out that something I could catch for the first time which should be taken into account to enhance the performance & resource utilization further. As calling functions that do nothing except calling other functions without doing real job should be really avoidable specially in Go. "Due to its initial small stack size per routine, and to simplify the stack trace minimizing the transmitted meta data!"

@takanuva15
Copy link
Contributor Author

takanuva15 commented Sep 1, 2024

@ahmadSaeedGoda Hi, thanks for your reply and the debugging screenshots. That gives some nice information on how the Bind functionality works.

However, I did not say in my question that the omitempty tag was misbehaving; actually, I am not reporting a bug at all in this issue. Rather, I am simply asking "Is there a combination of tags that would make this possible with Gin?". If the omitempty tag will still bind empty user-provided values to struct fields instead of using the default form value, then my question is simply whether there is some other tag I could use so that whether the user provides no value or an empty value, the default will still kick in and override the user's input.

If that's not possible, then could we add this functionality somehow, such as adding a form tag defaultIfEmpty= so that the default kicks in whether the user provides no input or empty input?

EDIT: It turns out someone seems to have mentioned this same issue in 2019 if I'm not mistaken

@RedCrazyGhost
Copy link
Contributor

@takanuva15 Currently you can only handle the assignment problem manually, which is the least costly for you

The question you raised, and I thought about it a little bit, is that the logical tree will converge to the point where the incoming value is null, and the default value, the default value is enabled.

The scope of the impact of modifying gin's underlying code is difficult to estimate.

gin/binding/form_mapping.go

Lines 136 to 166 in 3cb3067

type setOptions struct {
isDefaultExists bool
defaultValue string
}
func tryToSetValue(value reflect.Value, field reflect.StructField, setter setter, tag string) (bool, error) {
var tagValue string
var setOpt setOptions
tagValue = field.Tag.Get(tag)
tagValue, opts := head(tagValue, ",")
if tagValue == "" { // default value is FieldName
tagValue = field.Name
}
if tagValue == "" { // when field is "emptyField" variable
return false, nil
}
var opt string
for len(opts) > 0 {
opt, opts = head(opts, ",")
if k, v := head(opt, "="); k == "default" {
setOpt.isDefaultExists = true
setOpt.defaultValue = v
}
}
return setter.TrySet(value, field, tagValue, setOpt)
}

The legacy code and github.com/go-playground/validator validation framework don't mix well.

func TestMappingDefault(t *testing.T) {
var s struct {
Int int `form:",default=9"`
Slice []int `form:",default=9"`
Array [1]int `form:",default=9"`
}
err := mappingByPtr(&s, formSource{}, "form")
require.NoError(t, err)
assert.Equal(t, 9, s.Int)
assert.Equal(t, []int{9}, s.Slice)
assert.Equal(t, [1]int{9}, s.Array)
}

If you have a strong idea, you are welcome to submit a PR here

ahmadSaeedGoda added a commit to ahmadSaeedGoda/gin that referenced this issue Sep 3, 2024
- Use provided default value in struct tags when binding a request input to struct for validation.

Fixes: How to apply default value if empty value provided by client during model binding? gin-gonic#4042, #13042df, #a41721a
@ahmadSaeedGoda
Copy link
Contributor

Kindly check the PR to lemme know if any!

Cheers,

@ahmadSaeedGoda
Copy link
Contributor

@ahmadSaeedGoda Hi, thanks for your reply and the debugging screenshots. That gives some nice information on how the Bind functionality works.

However, I did not say in my question that the omitempty tag was misbehaving; actually, I am not reporting a bug at all in this issue. Rather, I am simply asking "Is there a combination of tags that would make this possible with Gin?". If the omitempty tag will still bind empty user-provided values to struct fields instead of using the default form value, then my question is simply whether there is some other tag I could use so that whether the user provides no value or an empty value, the default will still kick in and override the user's input.

If that's not possible, then could we add this functionality somehow, such as adding a form tag defaultIfEmpty= so that the default kicks in whether the user provides no input or empty input?

EDIT: It turns out someone seems to have mentioned this same issue in 2019 if I'm not mistaken

@takanuva15 with the proposed PR You can have the default to kick in whether the user provides no value or not sent the field at all. I've decided that it should be done with no need for a new form tag such as defaultIfEmpty= as it's a basic assumption that the default value specified via code should kick in for any empty value no matter provided as empty or not sent in the first place.

Hopefully that makes sense,
Awaiting approval from maintainers.

ahmadSaeedGoda added a commit to ahmadSaeedGoda/gin that referenced this issue Sep 4, 2024
- Use specified default value in struct tags when binding a request input to struct for validation, even if sent empty, not only when not sent at all.
- Add string field to `TestMappingDefault` test case.
- Add test case for not sent form field to default to the value specified via code.
- Add test case for form field sent empty to default to the value specified via code.

Fixes: How to apply default value if empty value provided by client during model binding? gin-gonic#4042, #13042df, #a41721a
appleboy pushed a commit that referenced this issue Sep 6, 2024
- Use specified default value in struct tags when binding a request input to struct for validation, even if sent empty, not only when not sent at all.
- Add string field to `TestMappingDefault` test case.
- Add test case for not sent form field to default to the value specified via code.
- Add test case for form field sent empty to default to the value specified via code.

Fixes: How to apply default value if empty value provided by client during model binding? #4042, #13042df, #a41721a
@takanuva15
Copy link
Contributor Author

I was gonna say that the way your PR is implemented, it's a breaking change to existing servers that assume the default value is not applied if the client sends an empty value. Looks like your PR got merged anyways though and it's set for v1.11, so whatever lol

@ahmadSaeedGoda
Copy link
Contributor

ahmadSaeedGoda commented Sep 9, 2024

@takanuva15 That's a good point. But shall we agree that "default value" "as the name suggests" is meant to be applied whenever the value passed is null or empty?
Meaning absent or provided but zero value..
Isn't this the meaning of default?

So talking about existing servers that assume when client provide empty value, the default value should not be applied. Why in the first place they specify a default value?
Someone might say, they specify the default value to be applied only when value is not sent/passed around (== nil/null). Okay and what if empty? doesn't empty means that the client wants it to be just sent for some cases like mandatory fields required by the server? Then server can decide how to populate it, either as empty or specify a default value. Both cases are handled here with my PR proposed change. Because if the server developer or maintainer decide to set default value within struct tags, it makes sense to be applied on empty and null values, here the framework usage is clear. Otherwise, if the server coder decides that there should be no default value applied, then empty is gonna be handled as empty.

Framework is to take care of providing user(developer) some way to determine how the value should be validated.
Validation must be conducted as per user/developer instructions via code.

When user/developer require a field to be provided/passed/sent by client request, only 2 options/scenarios here are possible. Either client provide value to be set for this field, or empty value. When dev state default, it has to be set for empty because the field is required in the first place! Means that default is only meant for empty values because there can be only values or empty values in this scenario of a required field. Not possible to have nil or absent field value. It's either empty or has value. Then default value has only one purpose, which is to replace empty values provided by the client.

So your suggestion can't be relevant for the above scenario I think, can it?

Second scenario is when field is optional (not required by the server), in such a case client can either provide value, empty value, or not send it within the request at all. Here 3 options available. When developer say default value is blabla. They must mean that this value is to be set for the null & empty cases altogether. Otherwise, the framework must provide functionality or option for developers to state a default value for null and another for empty (does not make sense for me). I just assumed that the framework will treat both as no value, then check if default stated and set it as is (whether stated or not stated it's gonna be set).
If default has been stated by dev, it's gonna replace empty or null values. If not stated, the default value is now an empty value, that will replace the empty value provided by client. So nothing happens, and the empty value provided will remain empty as it's replaced by empty which is equal.

I must admit I like your way of thinking, but just wanted to clarify for you my intention & reasoning behind appending that peace of code. So let's distill all the cases as follows in bullet points:

1. Required field:
- Non-empty Value provided by client: Value will be set no matter default value specified or not.
- Empty Value provided by client: If default value specified it's gonna be set. If no default value specified, means the default value is empty or zero value for string, which is the data type the framework uses already for the default upfront before setting the appropriate type later on. So my proposed change will fill the field with default whether specified with val or empty. Empty default val that will replace empty provided val means no impact.

2. Optional field:
- Not provided at all by client: Here framework checks for default value and set it. No issue.
- Provided as empty by client: Here framework checks for the field if passed, then it finds it available "but carries empty val", so framework will set field's value as provided "which is empty". After that it will check it if empty, it gonna check the default value, if default exists will replace the value of the field. If not exists, means default is empty, so no problem of replacing empty value provided with empty value for default. Has no impact but still achieving the goal.
- Provided with non-empty value by client: Here my proposed snippet has no impact also.

Hopefully I could make it a bit clearer, and kindly take a look at the function that does all what I've tried to explain above:

gin/binding/form_mapping.go

Lines 217 to 270 in 3cb3067

func setByForm(value reflect.Value, field reflect.StructField, form map[string][]string, tagValue string, opt setOptions) (isSet bool, err error) {
vs, ok := form[tagValue]
if !ok && !opt.isDefaultExists {
return false, nil
}
switch value.Kind() {
case reflect.Slice:
if !ok {
vs = []string{opt.defaultValue}
}
if ok, err = trySetCustom(vs[0], value); ok {
return ok, err
}
if vs, err = trySplit(vs, field); err != nil {
return false, err
}
return true, setSlice(vs, value, field)
case reflect.Array:
if !ok {
vs = []string{opt.defaultValue}
}
if ok, err = trySetCustom(vs[0], value); ok {
return ok, err
}
if vs, err = trySplit(vs, field); err != nil {
return false, err
}
if len(vs) != value.Len() {
return false, fmt.Errorf("%q is not valid value for %s", vs, value.Type().String())
}
return true, setArray(vs, value, field)
default:
var val string
if !ok {
val = opt.defaultValue
}
if len(vs) > 0 {
val = vs[0]
}
if ok, err := trySetCustom(val, value); ok {
return ok, err
}
return true, setWithProperType(val, value, field)
}
}

It explains everything I listed here in a very concise manner. First checks if field is passed by client "set as key in the form map" and default value exists, if one of them isn't true the function stops execution and returns 🤷‍♂️
then falls down to default case in the switch statement (as in our case the field isn't slice nor array), checks the previous condition separately. If not ok (means field isn't passed at all as not set into the form map parameter/argument) it sets default as-is, since it doesn't matter there exists default value or not, the field is to be filled with it whether has default or empty string (""). After that function checks the availability of provided value, again it fills the field with it as-is (no matter empty or non-empty). Here my addendum/supplement comes into play to check once again if val of the field is still empty (which means the value is provided but empty) so the default will be set again no matter it has value specified as default or not specified so it is empty.

All of this has no contradiction with current implementation and all the responsibility falls entirely on the developers' shoulder to know what they are doing logically. Additionally, all the test cases pass BTW, which means my change shouldn't conflict with any other cases already available and provided by the current version of the framework. Moreover, I've appended some tests to already available case, along with 2 of mine to cover my introduced tiny change.

It's confusing a bit by nature, and I highly appreciate your excellent point & your feedback. Just wanted to share with you my thoughts about what's already the case provided by the current implementation that doesn't allow for empty fields sent, to be saved empty with the existence of default value.
No breaking change IMHO.

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

No branches or pull requests

3 participants