Closed ccapurso closed 2 years ago
I believe that prior to this change non-strings were valid inputs: https://go.dev/play/p/oy2jtFp-k7z
Do we instead want to special case json.Number?
Thanks for pointing this out @briankassouf. The ParseCommaStringSlice
function is a lot more permissive than I expected. I imagine that is due to setting WeaklyTypedInput
to true
in the mapstructure.DecoderConfig
. For example, passing []bool{true, false, true}
results in []string{"1", "0", "1"}
. It's kind of interesting that this function has somewhat undefined/magical behavior with the weak typing. Based on this, it definitely makes sense to just special case json.Number
. I can expand the unit tests but do we actually intend to make this function this permissive? The name (and associated use of StringToSliceHookFunc
) doesn't necessarily imply that. Just want to ensure that our test cases cover what we do want to support.
@ccapurso Since we use it in our API parameter parsing code, i think it's hard to say if making it less permissive would break existing APIs usage or not. I'm also not sure if boundary is using it in any meaningful way. Since it takes an interface{}
as input it's probably best to not break the existing contract and just special case json.Number
@briankassouf, definitely agree. I didn't mean to imply that we should be less permissive. I am trying to get a feel for whether there might be other cases that might panic so that we can handle accordingly and ensure that there are appropriate tests.
The
ParseCommaStringSlice
function inparseutil
performs a type assertion (i.e.in.(string)
) but does not return an error if the assertion fails. Without an early return, a panic might occur in themapstructure.NewDecoder
call.StringToSliceHookFunc
inmapstructure
returns a hook func that performs a similar string type assertion but does not verify that it was successful. This can occur if the input is ajson.Number
given that itsreflect.Kind
isstring
. The type assertion inmapstructure
will panic withinvalid type assertion: data.(string) (non-interface type json.Number on left)
.Unit tests for
ParseCommaStringSlice
were also added as there weren't any prior to this PR.