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

mapstr.M.Clone() should clone []interface{} values #75

Open
andrewkroh opened this issue Aug 19, 2022 · 1 comment
Open

mapstr.M.Clone() should clone []interface{} values #75

andrewkroh opened this issue Aug 19, 2022 · 1 comment
Labels
enhancement New feature or request

Comments

@andrewkroh
Copy link
Member

andrewkroh commented Aug 19, 2022

Clone() does a deep clone of inner map[string]interface{} and mapstr.M values. But if a map is contained in an array then the value is not cloned. For example, if an object like this were Clone()ed then map within array would not be cloned because Clone() does not look inside []interface{}.

mapstr.M{
  "array": []interface{}{
    map[string]interface{}{
      "key": "value",
    },
  },
} 

One use case is Filebeat's httpjson input. It uses heavily uses Clone() and as a result arrays object's do not behave as expected. It depends on the result of Clone() only containing mapstr.M (and not map[string]interface{}). It needs mapstr.M's implementation of fmt.Stringer that returns json.

As a possible workaround, httpjson has been considering its own deepClone(mapstr.M) mapstr.M (workaround.go.txt) implementation. But I think it would be better if mapstr.M Clone's implementation automatically handled arrays.

Relates: elastic/beats#32472

@cmacknz
Copy link
Member

cmacknz commented Aug 19, 2022

I don't think we'd have any opposition to this, feel free to just make this change here.

The only real note for implementation is that much of the test coverage for usage of mapstr.M still lives in Beats (as a consequence of system tests for example). I would recommend creating a Beats branch pointed at the development branch for this change to see if there are any unexpected test failures before merging the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants