search

ugorji / go

idiomatic codec and rpc lib for msgpack, cbor, json, etc. msgpack.org[Go]
MIT License
1.86k stars 295 forks source link

Decode array/list types to array instead of slice (optionally?) #170

Closed cvermilion closed 8 years ago

cvermilion commented 8 years ago

Msgpack, for example, allows arbitrary values as map keys, but slices can't be map keys since they're not hashable. Arrays can be, and for formats like msgpack where the length of the array is specified at the beginning of the encoded byte stream, using a fixed-length array seems sensible.

I only use this library for Msgpack encoding/decoding, so I'm not familiar enough with the other use cases to know which this would be inappropriate for. Also, it seems like the only way to construct arbitrary type and size arrays at run time is to use reflect heavily, which presumably has a performance cost. So I would think this feature would have to be optional.

cvermilion commented 8 years ago

I worked out how to do this for the Msgpack case, at least, in a branch for my own use -- here's a PoC PR, which maybe could be extended to handle all cases correctly and make the array decoding optional: https://github.com/ugorji/go/pull/171.

ugorji commented 8 years ago

It's not clear to me how to solve this.

To read a value from an interface, you must know the exact underlying type. In theory, a map[interface{}]X cannot have some keys being arrays unless the user knows the full list of array types which may exist here. I don't think we can solve this issue here.

The better solution, is to have a map[[7]int]string, etc where you say this is a map type, and the keys are arrays of 7 values each. For this, go-codec has a great solution.

In addition, your PR just assumes that valueTypeArray is for map keys. however, valueTypeArray from kInterfaceNaked is for any value, which may exist anywhere.

So, in summary, the way to solve this is to use an explicit array, or expect that the value will be one of the known types (primitive or slice or map of basic values).

ugorji commented 8 years ago

No response from OP, so closing.

cvermilion commented 8 years ago

Hi, sorry for the delayed response; I'm on vacation this week.

I agree it's a tricky problem, and the best course of action may be to not support arbitrary keys. But I do think the current implementation means that certain valid msgpack messages can't be decoded.

It's true that you need to know what the keys might be, but that's also true for map[string]interface{} or map[interface{}]interface{} -- you can't really do anything interesting with the keys or values without casting them to the appropriate value via introspection. So I don't think this is a unique problem for array keys. In general I would assume (our code does, eg), that maps and slices can contain any of the standard msgpack scalar types as well as nested maps and slices.

Maybe there's a solution using a "boxed" []interface{} type that defines a hash function; I'll think about it some more.

As for valueTypeArray being used for just map keys or generally, I wasn't trying to restrict the array behavior to just map keys. I'd think it would make sense for arrays to decode always to arrays or always to slices, but I don't really have strong feelings there. In any case that was a detail I hadn't tried to address yet in this PR, I just wanted to explore if this was a useful approach.

ugorji commented 8 years ago

Good points.

Thought about it more, and fixed some days back. I don't have all my notes, but just realized that I didn't update yet.