gorilla / schema

Package gorilla/schema fills a struct with form values.
https://gorilla.github.io
BSD 3-Clause "New" or "Revised" License
1.39k stars 231 forks source link

[Question] Is ignoring mismatched slice element types intended behavior? #210

Closed lll-lll-lll-lll closed 6 months ago

lll-lll-lll-lll commented 8 months ago

Is there an existing feature request for this?

Is your feature request related to a problem? Please describe.

Question

when a struct field is a slice with two different element types, the current implementation seems to ignore element of the wrong type. Is this intentional behavior? Is there no error handling for this scenario?

Implementation

https://github.com/gorilla/schema/blob/main/decoder.go#L130-L137

for _, val := range vals {
    //this check is to handle if the wrong value is provided
    if convertedVal := builtinConverters[f.typ.Elem().Kind()](val); convertedVal.IsValid() {
        defaultSlice = reflect.Append(defaultSlice, convertedVal)
    }
}

example


var decoder = schema.NewDecoder()

type D struct {
    S string `schema:"s,default:test1"`
    A []int  `schema:"a,default:test|1"`
}

func main() {
    data := map[string][]string{}
    d := D{}
    if err := decoder.Decode(&d, data); err != nil {
        log.Fatal("Error while decoding:", err)
    }
    fmt.Println(d)
}

// output
// {test1 [1]}

Describe the solution that you would like.

for _, val := range vals {
    //this check is to handle if the wrong value is provided
    if convertedVal := builtinConverters[f.typ.Elem().Kind()](val); !convertedVal.IsValid() {
            // error handling
    }
        defaultSlice = reflect.Append(defaultSlice, convertedVal)
}

Describe alternatives you have considered.

No response

Anything else?

No response

zak905 commented 8 months ago

Hi @lll-lll-lll-lll, thanks. It seems like this is intentional, but I honestly do not recall the reasons behind the choice because it has been a while since I submitted the PR. Maybe it was because default was considered as something optional and therefore should not break the decoding, no matter what. There is also a test case for this behavior: https://github.com/gorilla/schema/blob/main/decoder_test.go#L2217

I like the idea of failing when a default value that do not match the type is provided. This would enforce correctness and notify when some unexpected value is used. It's certainly a plus.

@AlexVulaj, @jaitaiwan what do you think ?

AlexVulaj commented 7 months ago

I'm in agreement with @zak905 here - I think it's better to handle cases with an unexpected value. Please feel free to submit a PR for this change and tag me in it, thanks!

lll-lll-lll-lll commented 7 months ago

Thank you, I will try to make a PR!