-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
bind: Maintain backwards compatibility for map[string]interface{} binding #2656
Conversation
bind.go
Outdated
@@ -159,6 +159,12 @@ func (b *DefaultBinder) bindData(destination interface{}, data map[string][]stri | |||
for k, v := range data { | |||
if isElemString { | |||
val.SetMapIndex(reflect.ValueOf(k), reflect.ValueOf(v[0])) | |||
} else if isElemInterface { | |||
if len(v) == 1 { |
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 am not certain that this behavior is not creating more problems. Number of query params does not need to be fixed. You could have situations where user send request with multiple values and some time one. now at backend you are forced to real with situation that you could be dealing with slice of strings or element of string type. This functionality could be some kind of search form with checkboxes etc.
I think if we want have backwards compatibility we need a check for map[string]interface{}{}
that is produces always strings and not slices of strings
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.
@aldas thanks for pointing this out. I agree having to deal with slice of strings and single string would be inconvenient.
I made changes as per your suggestions. Now for interface type, it will produce always first string. Please review again.
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.
please remove that if len(v) == 1 {
check and add remark to } else if isElemInterface {
that we are doing this for backwards compatibility
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.
LGTM
This PR modifies the bindData function to preserve the pre v4.12.0 behavior for map[string]interface{} while supporting the new functionality:
This approach maintains compatibility with existing code that expects single string values, while allowing new code to take advantage of multiple values when present.
The change addresses the issue reported in #2652, where the binding behavior for map[string]interface{} changed in v4.12.0, potentially breaking existing implementations.
Testing:
Please review and let me know if any further changes are needed.