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

Decode does not zero the destination variable before decoding the value into it. #74

Open
curioustolearn opened this issue Feb 14, 2017 · 8 comments

Comments

@curioustolearn
Copy link

curioustolearn commented Feb 14, 2017

I realized today that Decode does not zero the destination variable before decoding the value into it. Is this expected; or is this a bug?

Example:


	var rec r.ChangeResponse
	var updatedDec UserDecision

      	go func() {
		for cur.Next(&rec) {
			newValInterface := rec.NewValue
			fmt.Printf("\nNewvalue: %+v\n", newValInterface)
			fmt.Printf("\nupdatedDec: %+v\n", updatedDec) 
			mapstructure.Decode(newValInterface, &updatedDec) 
			fmt.Printf("\nDecision update: %+v\n", updatedDec)
			decUpdateCh <- updatedDec
		}
		log.Println("Exiting goroutine listening on decUpdate changes.")
	}()


In the above example updatedDec is not zeroed across iterations. updatedDec contains an array. If that array has more elements than the same array in the new value, then some of the earlier values remain in there after mapstrucure.Decode.

@mitchellh
Copy link
Owner

Yes this is done on purpose so that you can call Decode multiple times to "build up" a result.

Arrays/slices in particular this may not make sense though.

@curioustolearn
Copy link
Author

Thanks. I will remember to zero them before the new iteration.

@mitchellh
Copy link
Owner

I do think the slice/array should be zeroed though (hence me not closing this issue). I'm thinking about it more though.

@curioustolearn
Copy link
Author

Yes, I agree. In the case some input is helpful: I think currently it replaces the existing elements in the slice with new elements. This works as long as the new slice is the same length or bigger than the existing slice. This, however, results in a problem when the new slice is smaller than the existing slice, since some of (but not all) previous elements are left in the slice.

@nd2s
Copy link

nd2s commented Sep 1, 2017

Is there a solution/workaround yet? I'd need slices to be zeroed before values are written into (but only if this field should be updated/values are set).

@nd2s
Copy link

nd2s commented Feb 27, 2018

Any more thoughts on this? In my opinion probably everyone expects an array/slice to be zeroed before update... Partial update of an array seems to be a very rare edge case.

@yashtewari
Copy link

I agree that arrays/slices should be fully replaced when decoding... I think this would be expected behavior for most users, which could make them unknowingly introduce bugs into their systems (as I did). What are some arguments for keeping things the way they are?

I'm willing to put up a PR for this.

mcdoker18 added a commit to mcdoker18/opentelemetry-collector that referenced this issue Sep 27, 2022
mapstructure library doesn't override full slice during unmarshalling.
Origin issue: mitchellh/mapstructure#74 (comment)
To address this we zeroes every slice before unmarshalling unless user provided slice is nil.
mcdoker18 added a commit to mcdoker18/opentelemetry-collector that referenced this issue Sep 29, 2022
mapstructure library doesn't override full slice during unmarshalling.
Origin issue: mitchellh/mapstructure#74 (comment)
To address this we zeroes every slice before unmarshalling unless user provided slice is nil.
mcdoker18 added a commit to mcdoker18/opentelemetry-collector that referenced this issue Sep 30, 2022
mapstructure library doesn't override full slice during unmarshalling.
Origin issue: mitchellh/mapstructure#74 (comment)
To address this we zeroes every slice before unmarshalling unless user provided slice is nil.
mcdoker18 added a commit to mcdoker18/opentelemetry-collector that referenced this issue Oct 15, 2022
mapstructure library doesn't override full slice during unmarshalling.
Origin issue: mitchellh/mapstructure#74 (comment)
To address this we zeroes every slice before unmarshalling unless user provided slice is nil.
mcdoker18 added a commit to mcdoker18/opentelemetry-collector that referenced this issue Oct 17, 2022
mapstructure library doesn't override full slice during unmarshalling.
Origin issue: mitchellh/mapstructure#74 (comment)
To address this we empty every slice before unmarshalling unless user provided slice is nil.
@klesh
Copy link

klesh commented Jul 4, 2024

😂 I find myself falling into the same hole as well. We rely on the feature to implement a bunch of PATCH APIs and they worked nicely for years until I tried to reduce one of the fields with the list type a week ago.

I wonder what is the use case of "Merging List"? How about strings because they could be perceived as a list of characters?
If it has to be the default behavior, can we have an option to disable it?

I agree that arrays/slices should be fully replaced when decoding... I think this would be expected behavior for most users, which could make them unknowingly introduce bugs into their systems (as I did). What are some arguments for keeping things the way they are?

I'm willing to put up a PR for this.

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

No branches or pull requests

5 participants