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

bind: Maintain backwards compatibility for map[string]interface{} binding #2656

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

thesaltree
Copy link
Contributor

This PR modifies the bindData function to preserve the pre v4.12.0 behavior for map[string]interface{} while supporting the new functionality:

  • Single values are stored as strings
  • Multiple values are stored as []string

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:

  • Updated existing tests to reflect the new behavior

Please review and let me know if any further changes are needed.

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 {
Copy link
Contributor

@aldas aldas Jul 11, 2024

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

Copy link
Contributor Author

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.

Copy link
Contributor

@aldas aldas left a 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

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aldas aldas merged commit f13e264 into labstack:master Jul 22, 2024
14 checks passed
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

Successfully merging this pull request may close these issues.

2 participants