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

Use encoding.TextUnmarshaler when MessagePack format is a str #370

Open
WGH- opened this issue Feb 26, 2024 · 0 comments
Open

Use encoding.TextUnmarshaler when MessagePack format is a str #370

WGH- opened this issue Feb 26, 2024 · 0 comments

Comments

@WGH-
Copy link

WGH- commented Feb 26, 2024

Expected Behavior

msgpack successfully unmarshals a type implementing both encoding.TextUnmarshaler and encoding.BinaryUnmashaler using the former interface when the data is of a str format, and is stored in text representation.

Current Behavior

msgpack fails to unmarshal the data in aforementioned conditions, because it's using the latter interface which does not expect text data.

This is because the library prefers BinaryUnmarshaler unconditionally:

msgpack/decode_value.go

Lines 73 to 78 in 19c91df

if typ.Implements(binaryUnmarshalerType) {
return nilAwareDecoder(typ, unmarshalBinaryValue)
}
if typ.Implements(textUnmarshalerType) {
return nilAwareDecoder(typ, unmarshalTextValue)
}

Possible Solution

Choose the preferred unmarshalling interface based on MessagePack format (MessagePack has distinct str and bin formats), preferring TextUnmarshaler when it's an str.

Steps to Reproduce

This example might look arbitrary out of context, but this situation easily arises when round-tripping the data through JSON and/or other libraries (especially in dynamically typed programming languages).

https://play.golang.com/p/3WFWbPVKDkp

package main

import (
	"log"

	"github.com/gofrs/uuid/v5"
	"github.com/vmihailenco/msgpack/v5"
)

type T struct {
	ID uuid.UUID
}

func main() {
	obj := map[string]any{
		"ID": "29247b08-ead7-46b3-b263-6944c2096673",
	}
	b, err := msgpack.Marshal(obj)
	if err != nil {
		panic(err)
	}

	var t T

	err = msgpack.Unmarshal(b, &t)
	if err != nil {
		panic(err)
	}

	log.Printf("%#v", t)
}
WGH- added a commit to WGH-/msgpack that referenced this issue Feb 27, 2024
This library currently always prefers to use encoding.BinaryUnmashaler
when it's implemented by the target type.

This might lead to problems when the type also has a text
representation implemented as encoding.TextUnmarshaler.

Consider netip.Addr from stdlib as example, which implements both.
This library won't be able decode MessagePack containing a string
"192.0.2.1" into *netip.Addr because it will attempt to use
encoding.BinaryUnmashaler which doesn't expect text representation.

Fortunately, MessagePack has distinct string and binary types, so we can
check the source data type before choosing the interface to use.

This commit changes the behaviour of decoder as follows. When

1) target Go data type implements both BinaryUnmashaler and
   TextUnmarshaler
2) source MessagePack data type is a string

TextUnmarshaler will be preferred over BinaryUnmashaler.

This feature is gated behind a Decoder option, because it is potentially
backward-incompatible change.

See vmihailenco#370
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

1 participant