-
Notifications
You must be signed in to change notification settings - Fork 24
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
Handle JSON Number in ParseCommaStringSlice #29
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
@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 |
@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.