diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f025e122..f9dff16f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,7 @@ Versioning](http://semver.org/spec/v2.0.0.html) except to the first release. - Flaky decimal/TestSelect (#300) - Race condition at roundRobinStrategy.GetNextConnection() (#309) +- Incorrect decoding of an MP_DECIMAL when the `scale` value is negative (#314) ## [1.12.0] - 2023-06-07 diff --git a/decimal/bcd.go b/decimal/bcd.go index d6222b861..61a437276 100644 --- a/decimal/bcd.go +++ b/decimal/bcd.go @@ -46,8 +46,11 @@ package decimal // https://github.com/tarantool/decNumber/blob/master/decPacked.c import ( + "bytes" "fmt" "strings" + + "github.com/vmihailenco/msgpack/v5" ) const ( @@ -185,13 +188,17 @@ func encodeStringToBCD(buf string) ([]byte, error) { // ended by a 4-bit sign nibble in the least significant four bits of the final // byte. The scale is used (negated) as the exponent of the decimal number. // Note that zeroes may have a sign and/or a scale. -func decodeStringFromBCD(bcdBuf []byte) (string, error) { - // Index of a byte with scale. - const scaleIdx = 0 - scale := int(bcdBuf[scaleIdx]) +func decodeStringFromBCD(bcdBuf []byte) (string, int, error) { + // Read scale. + buf := bytes.NewBuffer(bcdBuf) + dec := msgpack.NewDecoder(buf) + scale, err := dec.DecodeInt() + if err != nil { + return "", 0, fmt.Errorf("unable to decode the decimal scale: %w", err) + } - // Get a BCD buffer without scale. - bcdBuf = bcdBuf[scaleIdx+1:] + // Get the data without the scale. + bcdBuf = buf.Bytes() bufLen := len(bcdBuf) // Every nibble contains a digit, and the last low nibble contains a @@ -206,10 +213,6 @@ func decodeStringFromBCD(bcdBuf []byte) (string, error) { // Reserve bytes for dot and sign. numLen := ndigits + 2 - // Reserve bytes for zeroes. - if scale >= ndigits { - numLen += scale - ndigits - } var bld strings.Builder bld.Grow(numLen) @@ -221,26 +224,10 @@ func decodeStringFromBCD(bcdBuf []byte) (string, error) { bld.WriteByte('-') } - // Add missing zeroes to the left side when scale is bigger than a - // number of digits and a single missed zero to the right side when - // equal. - if scale > ndigits { - bld.WriteByte('0') - bld.WriteByte('.') - for diff := scale - ndigits; diff > 0; diff-- { - bld.WriteByte('0') - } - } else if scale == ndigits { - bld.WriteByte('0') - } - const MaxDigit = 0x09 // Builds a buffer with symbols of decimal number (digits, dot and sign). processNibble := func(nibble byte) { if nibble <= MaxDigit { - if ndigits == scale { - bld.WriteByte('.') - } bld.WriteByte(nibble + '0') ndigits-- } @@ -256,5 +243,8 @@ func decodeStringFromBCD(bcdBuf []byte) (string, error) { processNibble(lowNibble) } - return bld.String(), nil + if bld.Len() == 0 || isNegative[sign] && bld.Len() == 1 { + bld.WriteByte('0') + } + return bld.String(), -1 * scale, nil } diff --git a/decimal/decimal.go b/decimal/decimal.go index dd7b30433..d9eae23ed 100644 --- a/decimal/decimal.go +++ b/decimal/decimal.go @@ -98,16 +98,20 @@ func (decNum *Decimal) UnmarshalMsgpack(b []byte) error { // +--------+-------------------+------------+===============+ // | MP_EXT | length (optional) | MP_DECIMAL | PackedDecimal | // +--------+-------------------+------------+===============+ - digits, err := decodeStringFromBCD(b) + digits, exp, err := decodeStringFromBCD(b) if err != nil { return fmt.Errorf("msgpack: can't decode string from BCD buffer (%x): %w", b, err) } + dec, err := decimal.NewFromString(digits) - *decNum = *NewDecimal(dec) if err != nil { return fmt.Errorf("msgpack: can't encode string (%s) to a decimal number: %w", digits, err) } + if exp != 0 { + dec = dec.Shift(int32(exp)) + } + *decNum = *NewDecimal(dec) return nil } diff --git a/decimal/decimal_test.go b/decimal/decimal_test.go index 92616c5f2..ae98dc386 100644 --- a/decimal/decimal_test.go +++ b/decimal/decimal_test.go @@ -121,6 +121,18 @@ var correctnessSamples = []struct { "c7150113012345678912345678900987654321987654321d", false}, } +var correctnessDecodeSamples = []struct { + numString string + mpBuf string + fixExt bool +}{ + {"1e2", "d501fe1c", true}, + {"1e33", "c70301d0df1c", false}, + {"1.1e31", "c70301e2011c", false}, + {"13e-2", "c7030102013c", false}, + {"-1e3", "d501fd1d", true}, +} + // There is a difference between encoding result from a raw string and from // decimal.Decimal. It's expected because decimal.Decimal simplifies decimals: // 0.00010000 -> 0.0001 @@ -397,18 +409,22 @@ func TestEncodeStringToBCD(t *testing.T) { func TestDecodeStringFromBCD(t *testing.T) { samples := correctnessSamples + samples = append(samples, correctnessDecodeSamples...) samples = append(samples, rawSamples...) samples = append(samples, benchmarkSamples...) for _, testcase := range samples { t.Run(testcase.numString, func(t *testing.T) { b, _ := hex.DecodeString(testcase.mpBuf) bcdBuf := trimMPHeader(b, testcase.fixExt) - s, err := DecodeStringFromBCD(bcdBuf) + s, exp, err := DecodeStringFromBCD(bcdBuf) if err != nil { t.Fatalf("Failed to decode BCD '%x' to decimal: %s", bcdBuf, err) } decActual, err := decimal.NewFromString(s) + if exp != 0 { + decActual = decActual.Shift(int32(exp)) + } if err != nil { t.Fatalf("Failed to msgpack.Encoder string ('%s') to decimal", s) } @@ -551,6 +567,37 @@ func TestSelect(t *testing.T) { tupleValueIsDecimal(t, resp.Data, number) } +func TestUnmarshal_from_decimal_new(t *testing.T) { + skipIfDecimalUnsupported(t) + + conn := test_helpers.ConnectWithValidation(t, server, opts) + defer conn.Close() + + samples := correctnessSamples + samples = append(samples, correctnessDecodeSamples...) + samples = append(samples, benchmarkSamples...) + for _, testcase := range samples { + str := testcase.numString + t.Run(str, func(t *testing.T) { + number, err := decimal.NewFromString(str) + if err != nil { + t.Fatalf("Failed to prepare test decimal: %s", err) + } + + call := NewEvalRequest("return require('decimal').new(...)"). + Args([]interface{}{str}) + resp, err := conn.Do(call).Get() + if err != nil { + t.Fatalf("Decimal create failed: %s", err) + } + if resp == nil { + t.Fatalf("Response is nil after Call") + } + tupleValueIsDecimal(t, []interface{}{resp.Data}, number) + }) + } +} + func assertInsert(t *testing.T, conn *Connection, numString string) { number, err := decimal.NewFromString(numString) if err != nil { diff --git a/decimal/export_test.go b/decimal/export_test.go index c43a812c6..2c8fda1c7 100644 --- a/decimal/export_test.go +++ b/decimal/export_test.go @@ -4,7 +4,7 @@ func EncodeStringToBCD(buf string) ([]byte, error) { return encodeStringToBCD(buf) } -func DecodeStringFromBCD(bcdBuf []byte) (string, error) { +func DecodeStringFromBCD(bcdBuf []byte) (string, int, error) { return decodeStringFromBCD(bcdBuf) } diff --git a/decimal/fuzzing_test.go b/decimal/fuzzing_test.go index c7df2e080..5ec2a2b8f 100644 --- a/decimal/fuzzing_test.go +++ b/decimal/fuzzing_test.go @@ -10,11 +10,14 @@ import ( . "github.com/tarantool/go-tarantool/v2/decimal" ) -func strToDecimal(t *testing.T, buf string) decimal.Decimal { +func strToDecimal(t *testing.T, buf string, exp int) decimal.Decimal { decNum, err := decimal.NewFromString(buf) if err != nil { t.Fatal(err) } + if exp != 0 { + decNum = decNum.Shift(int32(exp)) + } return decNum } @@ -33,13 +36,13 @@ func FuzzEncodeDecodeBCD(f *testing.F) { if err != nil { t.Skip("Only correct requests are interesting: %w", err) } - var dec string - dec, err = DecodeStringFromBCD(bcdBuf) + + dec, exp, err := DecodeStringFromBCD(bcdBuf) if err != nil { t.Fatalf("Failed to decode encoded value ('%s')", orig) } - if !strToDecimal(t, dec).Equal(strToDecimal(t, orig)) { + if !strToDecimal(t, dec, exp).Equal(strToDecimal(t, orig, 0)) { t.Fatal("Decimal numbers are not equal") } })