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

Question regarding unmarshalReflect #124

Closed
alexkohler opened this issue Apr 27, 2020 · 4 comments
Closed

Question regarding unmarshalReflect #124

alexkohler opened this issue Apr 27, 2020 · 4 comments

Comments

@alexkohler
Copy link

alexkohler commented Apr 27, 2020

Hi there, I'm currently running into an issue that I think may potentially be a bug, but wanted to get some background on the guarantees of unmarshalReflect (and the general premise of how this library performs unmarshaling). Inside of of the unmarshalReflect's reflect.Array case, we have the following:

	case reflect.Array:
		arr := reflect.New(rv.Type()).Elem()
		elemtype := arr.Type().Elem()
		switch {
		case av.B != nil:
			for i, b := range av.B {
				arr.Index(i).Set(reflect.ValueOf(b))
			}
			rv.Set(arr)
			return nil

How are you guaranteeing that len(av.B) is equal to (or less than) arr.Len()?

@guregu
Copy link
Owner

guregu commented Apr 29, 2020

Hello
Looks like a bug to me 😅
After messing around with encoding/json a bit, it seems to discard values that won’t fit into the array
We should probably match that behavior
Although I think an error message might be more useful 🤔

@alexkohler
Copy link
Author

Either approach seems better than a panic 😄 It might be more idiomatic to match encoding/json's behavior, but the behavior should be explicitly documented either way.

@guregu guregu closed this as completed in 1e91f10 May 1, 2020
@guregu
Copy link
Owner

guregu commented May 1, 2020

I released v1.7.1 which fixes this. I decided to return an error when the array is too small. I imagine most of the time unmarshaling into a too-small array is a mistake instead of intended behavior, and silently truncating data is scary. I can always change this later if there's demand for it.

I haven't updated the docs yet but I'd like to have more explicit info about how each type is handled, I added this to #45.

Thanks for the bug report!

@guregu
Copy link
Owner

guregu commented May 1, 2020

Whoops, forgot to include the fix for lists -> arrays in v1.7.1.
Released v1.7.2 which takes care of that.

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

2 participants