Skip to content

Commit

Permalink
msgpack: Never marshal integers as MessagePack float
Browse files Browse the repository at this point in the history
Previously we were using MessagePack's float family for anything that
could convert losslessly to float64.

Unfortunately, cty's interpretation of numbers uses binary floating point
internally but decimal strings as the presentation, including when numbers
are converted to strings, and so we need to make some assumptions about
precision when we're choosing a string representation of a number, to
avoid confusing situations where slight inaccuracy in the binary
interpretation of a number presented in decimal would appear as unwanted
extra digits in the string representation.

Slight differences caused by precision handling are acceptable for the
fractional parts of numbers, but callers expect integers to be presented
exactly as originally given. Round-tripping a large integer through a
float64, even if the binary representation is exact, causes us to end up
with a rounded string representation after decoding, because the decoded
number has 52-bit precision instead of the 512 we use when dealing with
string representations of large integers.

Therefore we'll now use MessagePack float encodings only for numbers that
have a fractional component. If an integer cannot fit in any of
MessagePack's integer encodings then we'll encode it as a string containing
decimal digits instead, so that it'll decode back to a number with the
same precision and thus the same decimal string representation.
  • Loading branch information
liamcervante authored and apparentlymart committed Mar 20, 2024
1 parent 0e3c880 commit f41ae52
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# 1.14.4 (Unreleased)

* `msgpack`: Now uses string encoding instead of float encoding for a whole number that is too large to fit in any of MessagePack's integer types.

# 1.14.3 (February 29, 2024)

Expand Down
2 changes: 1 addition & 1 deletion cty/msgpack/marshal.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func marshal(val cty.Value, ty cty.Type, path cty.Path, enc *msgpack.Encoder) er
bf := val.AsBigFloat()
if iv, acc := bf.Int64(); acc == big.Exact {
err = enc.EncodeInt(iv)
} else if fv, acc := bf.Float64(); acc == big.Exact {
} else if fv, acc := bf.Float64(); acc == big.Exact && !bf.IsInt() {
err = enc.EncodeFloat64(fv)
} else {
err = enc.EncodeString(bf.Text('f', -1))
Expand Down
118 changes: 118 additions & 0 deletions cty/msgpack/roundtrip_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"testing"

"github.com/zclconf/go-cty/cty"
"github.com/zclconf/go-cty/cty/convert"
)

func TestRoundTrip(t *testing.T) {
Expand Down Expand Up @@ -96,6 +97,10 @@ func TestRoundTrip(t *testing.T) {
cty.MustParseNumberVal("9223372036854775809"),
cty.Number,
},
{
cty.MustParseNumberVal("18446744073709551616"),
cty.Number,
},
{
awkwardFractionVal,
cty.Number,
Expand Down Expand Up @@ -363,6 +368,119 @@ func TestRoundTrip(t *testing.T) {
}
}

func TestRoundTrip_fromString(t *testing.T) {
tests := []struct {
Value string
Type cty.Type
}{
{
"0",
cty.Number,
},
{
"1",
cty.Number,
},
{
"-1",
cty.Number,
},
{
"9223372036854775807",
cty.Number,
},
{
"9223372036854775808",
cty.Number,
},
{
"9223372036854775809",
cty.Number,
},
{
"18446744073709551616",
cty.Number,
},
{
"-9223372036854775807",
cty.Number,
},
{
"-9223372036854775808",
cty.Number,
},
{
"-9223372036854775809",
cty.Number,
},
{
"-18446744073709551616",
cty.Number,
},
{
"true",
cty.Bool,
},
{
"false",
cty.Bool,
},
}
for _, test := range tests {
t.Run(fmt.Sprintf("%#v as %#v", test.Value, test.Type), func(t *testing.T) {
stringVal := cty.StringVal(test.Value)

original, err := convert.Convert(stringVal, test.Type)
if err != nil {
t.Fatalf("input type must be convertible from string: %s", err)
}

{
// We'll first make sure that the conversion works even without
// MessagePack involved, since otherwise we might falsely blame
// the MessagePack encoding for bugs in package convert.
stringGot, err := convert.Convert(original, cty.String)
if err != nil {
t.Fatalf("result must be convertible to string: %s", err)
}

if !stringGot.RawEquals(stringVal) {
t.Fatalf("value did not round-trip to string even without msgpack\ninput: %#v\nresult: %#v", test.Value, stringGot)
}
}

b, err := Marshal(original, test.Type)
if err != nil {
t.Fatal(err)
}

t.Logf("encoded as %x", b)

got, err := Unmarshal(b, test.Type)
if err != nil {
t.Fatal(err)
}

if !got.RawEquals(original) {
t.Errorf(
"value did not round-trip\ninput: %#v\nresult: %#v",
test.Value, got,
)
}

stringGot, err := convert.Convert(got, cty.String)
if err != nil {
t.Fatalf("result must be convertible to string: %s", err)
}

if !stringGot.RawEquals(stringVal) {
t.Errorf("value did not round-trip to string\ninput: %#v\nresult: %#v", test.Value, stringGot)
}

})
}
}

// Unknown values with very long string prefix refinements do not round-trip
// losslessly. If the prefix is longer than 256 bytes it will be truncated to
// a maximum of 256 bytes.
Expand Down

0 comments on commit f41ae52

Please sign in to comment.