types: prefer TextUnmarshaler over BinaryUnmarshaler for str format#389
Open
alliasgher wants to merge 3 commits intovmihailenco:v5from
Open
types: prefer TextUnmarshaler over BinaryUnmarshaler for str format#389alliasgher wants to merge 3 commits intovmihailenco:v5from
alliasgher wants to merge 3 commits intovmihailenco:v5from
Conversation
The float64 fallback error path said "decoding float32" instead of "decoding float64". Fix the message to match the function. Fixes vmihailenco#352 Signed-off-by: alliasgher <alliasgher123@gmail.com>
The isEmptyValue method used structs.Fields to inspect only exported fields when deciding whether a struct value tagged omitempty should be omitted. If a struct had zero-valued exported fields but non-zero unexported fields, it was incorrectly treated as empty and omitted. Replace the exported-field scan with reflect.Value.IsZero, which checks all fields (exported and unexported) recursively. A struct is now omitted only when it equals the zero value of its type, which is the semantically correct behaviour for an "omit if empty" tag. Fixes vmihailenco#378 Signed-off-by: alliasgher <alliasgher123@gmail.com>
When a type implements both encoding.BinaryUnmarshaler and encoding.TextUnmarshaler, the decoder previously always picked BinaryUnmarshaler regardless of the wire format. This is wrong for types like uuid.UUID that are stored as msgpack str (e.g. after a JSON round-trip): the BinaryUnmarshaler expects raw bytes while a TextUnmarshaler expects the text representation. Introduce unmarshalBinaryOrTextValue that peeks at the next code: - msgpack str (any variant) → use TextUnmarshaler - everything else (bin, nil, ...) → use BinaryUnmarshaler Fixes vmihailenco#370 Signed-off-by: alliasgher <alliasgher123@gmail.com>
|
Hi @alliasgher, nice fix. Heads up that this repo has been unmaintained for ~3 years and PRs have been piling up. We maintain an active fork at github.com/Basekick-Labs/msgpack (v6) where we already handle this case — decode_value.go:269-281 peeks at the wire format and routes to func unmarshalBinaryOrTextValue(d *Decoder, v reflect.Value) error {
c, err := d.PeekCode()
if err != nil {
return err
}
if msgpcode.IsString(c) {
return unmarshalTextValue(d, v)
}
return unmarshalBinaryValue(d, v)
}If upstream doesn't pick this up, feel free to port it to the fork — contributions welcome. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #370. For types implementing both BinaryUnmarshaler and TextUnmarshaler, peek at the wire format and use TextUnmarshaler for msgpack str and BinaryUnmarshaler for everything else.