oschwald / maxminddb-golang

MaxMind DB Reader for Go
ISC License
576 stars 99 forks source link

Add a facility allowing manual control of the decoding #91

Open damz opened 1 year ago

damz commented 1 year ago

This PR adds a facility allowing full manual control of the decoding process.

While we are aware that the experimental deserializer interface exists, it is quite painful to use as it doesn't let the caller control the sequence of operations.

The implementation is centered around a Decoder object, which allows decoding ONE value at a particular offset.

The API for scalar values is straightforward:

DecodeBool() (bool, error)
DecodeString() (string, error)
DecodeBytes() ([]byte, error)
DecodeFloat32() (float32, error)
DecodeFloat64() (float64, error)
DecodeInt32() (int32, error)
DecodeUInt16() (uint16, error)
DecodeUInt32() (uint32, error)
DecodeUInt64() (uint64, error)
DecodeUInt128() (hi, lo uint64, err error)

For maps and slices, the API iterates over the items and provides a specific decoder for the value:

DecodeMap(cb func(key string, value *Decoder) error) error
DecodeSlice(cb func(value *Decoder) error) error

The implementation follows pointers automatically, and keeps track of the offset of the "next" value as soon as it is known, which facilitate the iteration of maps and slices. (For pointers, the "next" value is the value right after the pointer, for maps and slices it is the value right after the next item.)

damz commented 1 year ago

@oschwald Any feedback on this after the discussion in #92?

oschwald commented 1 year ago

Sorry for taking so long on this. I still need to take a closer look. One thought I have is possibly moving the decoder to another package, but I have not looked into the implications of that.

damz commented 1 year ago

I removed the patterns of using an error sentinel to stop the iteration and shadowing require in the tests, despite them being totally bog standard patterns, because your linter configuration is quite aggressive. (Even silencing these false positives is quite painful.)

It should pass the linter stage now.

damz commented 1 year ago

@oschwald Do you have any feedback on this? We have this implementation in production now.

andrei-dascalu commented 1 year ago

This would be quite good to have on my side - any feedback ?

oschwald commented 1 year ago

Before considering merging this, I'd like to see the decoder moved to a separate package. Most users of this library are not likely to use this interface, and having it in the same package as the reader is likely to cause confusion. Once that is done, I think I would be willing to accept a PR as long as the documentation made it clear that the API was experimental and subject to change. I haven't had a need for this API myself and haven't yet had the opportunity to use it.

damz commented 1 year ago

@oschwald The decoder relies on the internal decoder type right now, so it cannot be moved to a different package without significant refactoring.