From f76b16100f89dfe0e2aa099c8a44b56eb966eb04 Mon Sep 17 00:00:00 2001 From: Hengfeng Li Date: Wed, 14 Apr 2021 10:02:08 +1000 Subject: [PATCH 1/5] fix(spanner): invalid numeric should throw an error --- spanner/value.go | 35 +++++++++++++++++++++++++++++++++++ spanner/value_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/spanner/value.go b/spanner/value.go index eb2a873a715..b8e64c3a201 100644 --- a/spanner/value.go +++ b/spanner/value.go @@ -24,6 +24,7 @@ import ( "math/big" "reflect" "strconv" + "strings" "time" "cloud.google.com/go/civil" @@ -56,6 +57,32 @@ func NumericString(r *big.Rat) string { return r.FloatString(NumericScaleDigits) } +// validateNumeric returns nil if there are no errors. It will return an error +// when the numeric number is not valid. +func validateNumeric(r *big.Rat) error { + if r == nil { + return nil + } + // Add one more digit to the scale component to find out if there are more + // digits than required. + strRep := r.FloatString(NumericScaleDigits + 1) + strRep = strings.TrimRight(strRep, "0") + strRep = strings.TrimLeft(strRep, "-") + s := strings.Split(strRep, ".") + if len(s) != 2 { + return fmt.Errorf("invalid numeric float string: %s", strRep) + } + whole := s[0] + scale := s[1] + if len(scale) > NumericScaleDigits { + return fmt.Errorf("max scale for a numeric is %d. The requested numeric has more", NumericScaleDigits) + } + if len(whole) > NumericPrecisionDigits-NumericScaleDigits { + return fmt.Errorf("max precision for the whole component of a numeric is %d. The requested numeric has a whole component with precision %d", NumericPrecisionDigits-NumericScaleDigits, len(whole)) + } + return nil +} + var ( // CommitTimestamp is a special value used to tell Cloud Spanner to insert // the commit timestamp of the transaction into a column. It can be used in @@ -2654,6 +2681,10 @@ func encodeValue(v interface{}) (*proto3.Value, *sppb.Type, error) { pt = listType(floatType()) case big.Rat: pb.Kind = stringKind(NumericString(&v)) + err = validateNumeric(&v) + if err != nil { + return nil, nil, err + } pt = numericType() case []big.Rat: if v != nil { @@ -2680,6 +2711,10 @@ func encodeValue(v interface{}) (*proto3.Value, *sppb.Type, error) { if v != nil { pb.Kind = stringKind(NumericString(v)) } + err = validateNumeric(v) + if err != nil { + return nil, nil, err + } pt = numericType() case []*big.Rat: if v != nil { diff --git a/spanner/value_test.go b/spanner/value_test.go index 1d40fe43f89..60925578c07 100644 --- a/spanner/value_test.go +++ b/spanner/value_test.go @@ -213,6 +213,8 @@ func TestEncodeValue(t *testing.T) { numValuePtr := big.NewRat(12345, 1e3) var numNilPtr *big.Rat num2ValuePtr := big.NewRat(12345, 1e4) + maxNumValuePtr, _ := (&big.Rat{}).SetString("99999999999999999999999999999.999999999") + minNumValuePtr, _ := (&big.Rat{}).SetString("-99999999999999999999999999999.999999999") var ( tString = stringType() @@ -281,6 +283,8 @@ func TestEncodeValue(t *testing.T) { // NUMERIC / NUMERIC ARRAY {*numValuePtr, numericProto(numValuePtr), tNumeric, "big.Rat"}, {numValuePtr, numericProto(numValuePtr), tNumeric, "*big.Rat"}, + {maxNumValuePtr, numericProto(maxNumValuePtr), tNumeric, "max numeric"}, + {minNumValuePtr, numericProto(minNumValuePtr), tNumeric, "min numeric"}, {numNilPtr, nullProto(), tNumeric, "*big.Rat with null"}, {NullNumeric{*numValuePtr, true}, numericProto(numValuePtr), tNumeric, "NullNumeric with value"}, {NullNumeric{*numValuePtr, false}, nullProto(), tNumeric, "NullNumeric with null"}, @@ -406,6 +410,37 @@ func TestEncodeValue(t *testing.T) { } } +// Test encoding invalid values. +func TestEncodeInvalidValues(t *testing.T) { + type CustomNumeric big.Rat + + invalidNumPtr1 := big.NewRat(11234567891, 1e10) + invalidNumPtr2, _ := (&big.Rat{}).SetString("199999999999999999999999999999.999999999") + + for i, test := range []struct { + desc string + in interface{} + errMsg string + }{ + // NUMERIC + {desc: "numeric pointer with invalid scale component", in: invalidNumPtr1, errMsg: "max scale for a numeric is 9. The requested numeric has more"}, + {desc: "numeric pointer with invalid whole component", in: invalidNumPtr2, errMsg: "max precision for the whole component of a numeric is 29. The requested numeric has a whole component with precision 30"}, + {desc: "numeric with invalid scale component", in: *invalidNumPtr1, errMsg: "max scale for a numeric is 9. The requested numeric has more"}, + {desc: "numeric with invalid whole component", in: *invalidNumPtr2, errMsg: "max precision for the whole component of a numeric is 29. The requested numeric has a whole component with precision 30"}, + // CUSTOM NUMERIC + {desc: "custom numeric type with invalid scale component", in: CustomNumeric(*invalidNumPtr1), errMsg: "max scale for a numeric is 9. The requested numeric has more"}, + {desc: "custom numeric type with invalid whole component", in: CustomNumeric(*invalidNumPtr2), errMsg: "max precision for the whole component of a numeric is 29. The requested numeric has a whole component with precision 30"}, + } { + _, _, err := encodeValue(test.in) + if err == nil { + t.Fatalf("#%d (%s): want error during encoding, but got nil", i, test.desc) + } + if err.Error() != test.errMsg { + t.Errorf("#%d (%s): incorrect error message, got %v, want %v", i, test.desc, err, test.errMsg) + } + } +} + type encodeTest struct { desc string in interface{} From 2448acdad94e20b26413e787901bae2841e44ac5 Mon Sep 17 00:00:00 2001 From: Hengfeng Li Date: Sat, 1 May 2021 23:47:19 +1000 Subject: [PATCH 2/5] Add round and error modes. --- spanner/value.go | 39 +++++++++++++++++++++++++++++++-------- spanner/value_test.go | 3 +++ 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/spanner/value.go b/spanner/value.go index b8e64c3a201..e4b3875398c 100644 --- a/spanner/value.go +++ b/spanner/value.go @@ -50,6 +50,19 @@ const ( NumericScaleDigits = 9 ) +// NumericLossHandlingMode describes the way how to deal with invalid numeric values. +type NumericLossHandlingMode int + +const ( + // NumericRound rounds an invalid numeric value, e.g., 0.1234567895 rounds + // to 0.123456790. + NumericRound NumericLossHandlingMode = iota + // NumericError throws an error when meeting invalid numeric value. + NumericError +) + +var NumericPrecisionLossHandling NumericLossHandlingMode + // NumericString returns a string representing a *big.Rat in a format compatible // with Spanner SQL. It returns a floating-point literal with 9 digits after the // decimal point. @@ -2680,11 +2693,16 @@ func encodeValue(v interface{}) (*proto3.Value, *sppb.Type, error) { } pt = listType(floatType()) case big.Rat: - pb.Kind = stringKind(NumericString(&v)) - err = validateNumeric(&v) - if err != nil { - return nil, nil, err + switch NumericPrecisionLossHandling { + case NumericError: + err = validateNumeric(&v) + if err != nil { + return nil, nil, err + } + case NumericRound: + // pass } + pb.Kind = stringKind(NumericString(&v)) pt = numericType() case []big.Rat: if v != nil { @@ -2708,13 +2726,18 @@ func encodeValue(v interface{}) (*proto3.Value, *sppb.Type, error) { } pt = listType(numericType()) case *big.Rat: + switch NumericPrecisionLossHandling { + case NumericError: + err = validateNumeric(v) + if err != nil { + return nil, nil, err + } + case NumericRound: + // pass + } if v != nil { pb.Kind = stringKind(NumericString(v)) } - err = validateNumeric(v) - if err != nil { - return nil, nil, err - } pt = numericType() case []*big.Rat: if v != nil { diff --git a/spanner/value_test.go b/spanner/value_test.go index 60925578c07..99b0d64e5f4 100644 --- a/spanner/value_test.go +++ b/spanner/value_test.go @@ -417,6 +417,9 @@ func TestEncodeInvalidValues(t *testing.T) { invalidNumPtr1 := big.NewRat(11234567891, 1e10) invalidNumPtr2, _ := (&big.Rat{}).SetString("199999999999999999999999999999.999999999") + // Enable error mode. + NumericPrecisionLossHandling = NumericError + for i, test := range []struct { desc string in interface{} From 2fef62254e379b0118da7267ac809b48457bb2fe Mon Sep 17 00:00:00 2001 From: Hengfeng Li Date: Sat, 1 May 2021 23:51:18 +1000 Subject: [PATCH 3/5] Update comments. --- spanner/value.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/spanner/value.go b/spanner/value.go index e4b3875398c..00687264e21 100644 --- a/spanner/value.go +++ b/spanner/value.go @@ -50,7 +50,8 @@ const ( NumericScaleDigits = 9 ) -// NumericLossHandlingMode describes the way how to deal with invalid numeric values. +// NumericLossHandlingMode describes the mode of how to deal with loss of +// precision on numeric values. type NumericLossHandlingMode int const ( @@ -61,6 +62,8 @@ const ( NumericError ) +// NumericPrecisionLossHandling is the configuration for hanlding loss of +// precission on numeric values. var NumericPrecisionLossHandling NumericLossHandlingMode // NumericString returns a string representing a *big.Rat in a format compatible From e4169bd76a3572552825fdbe4f10244e16dc5de9 Mon Sep 17 00:00:00 2001 From: Hengfeng Li Date: Tue, 4 May 2021 17:43:23 +1000 Subject: [PATCH 4/5] Fix comments. --- spanner/value.go | 26 +++++++++++++++----------- spanner/value_test.go | 2 +- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/spanner/value.go b/spanner/value.go index 00687264e21..2f6e068b0aa 100644 --- a/spanner/value.go +++ b/spanner/value.go @@ -50,21 +50,25 @@ const ( NumericScaleDigits = 9 ) -// NumericLossHandlingMode describes the mode of how to deal with loss of +// LossOfPrecisionHandlingOption describes the option to deal with loss of // precision on numeric values. -type NumericLossHandlingMode int +type LossOfPrecisionHandlingOption int const ( - // NumericRound rounds an invalid numeric value, e.g., 0.1234567895 rounds - // to 0.123456790. - NumericRound NumericLossHandlingMode = iota - // NumericError throws an error when meeting invalid numeric value. + // NumericRound automatically rounds a numeric value that has a higher + // than what is supported by Spanner, e.g., 0.1234567895 rounds to + // 0.123456790. + NumericRound LossOfPrecisionHandlingOption = iota + // NumericError returns an error for numeric values that have a higher + // precision than what is supported by Spanner. E.g. the client returns an + // error if the application tries to insert the value 0.1234567895. NumericError ) -// NumericPrecisionLossHandling is the configuration for hanlding loss of -// precission on numeric values. -var NumericPrecisionLossHandling NumericLossHandlingMode +// LossOfPrecisionHandling configures how to deal with loss of precision on +// numeric values. The value of this configuration is global and will be used +// for all Spanner clients. +var LossOfPrecisionHandling LossOfPrecisionHandlingOption // NumericString returns a string representing a *big.Rat in a format compatible // with Spanner SQL. It returns a floating-point literal with 9 digits after the @@ -2696,7 +2700,7 @@ func encodeValue(v interface{}) (*proto3.Value, *sppb.Type, error) { } pt = listType(floatType()) case big.Rat: - switch NumericPrecisionLossHandling { + switch LossOfPrecisionHandling { case NumericError: err = validateNumeric(&v) if err != nil { @@ -2729,7 +2733,7 @@ func encodeValue(v interface{}) (*proto3.Value, *sppb.Type, error) { } pt = listType(numericType()) case *big.Rat: - switch NumericPrecisionLossHandling { + switch LossOfPrecisionHandling { case NumericError: err = validateNumeric(v) if err != nil { diff --git a/spanner/value_test.go b/spanner/value_test.go index 99b0d64e5f4..17312a496fd 100644 --- a/spanner/value_test.go +++ b/spanner/value_test.go @@ -418,7 +418,7 @@ func TestEncodeInvalidValues(t *testing.T) { invalidNumPtr2, _ := (&big.Rat{}).SetString("199999999999999999999999999999.999999999") // Enable error mode. - NumericPrecisionLossHandling = NumericError + LossOfPrecisionHandling = NumericError for i, test := range []struct { desc string From 4468ceb77d8a1cd0bb0b8f3d8b06776f9a3847bb Mon Sep 17 00:00:00 2001 From: Hengfeng Li Date: Tue, 4 May 2021 18:16:13 +1000 Subject: [PATCH 5/5] Fix comments. --- spanner/value.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/spanner/value.go b/spanner/value.go index 2f6e068b0aa..c5ec9ab1451 100644 --- a/spanner/value.go +++ b/spanner/value.go @@ -56,8 +56,8 @@ type LossOfPrecisionHandlingOption int const ( // NumericRound automatically rounds a numeric value that has a higher - // than what is supported by Spanner, e.g., 0.1234567895 rounds to - // 0.123456790. + // precision than what is supported by Spanner, e.g., 0.1234567895 rounds + // to 0.123456790. NumericRound LossOfPrecisionHandlingOption = iota // NumericError returns an error for numeric values that have a higher // precision than what is supported by Spanner. E.g. the client returns an @@ -89,9 +89,6 @@ func validateNumeric(r *big.Rat) error { strRep = strings.TrimRight(strRep, "0") strRep = strings.TrimLeft(strRep, "-") s := strings.Split(strRep, ".") - if len(s) != 2 { - return fmt.Errorf("invalid numeric float string: %s", strRep) - } whole := s[0] scale := s[1] if len(scale) > NumericScaleDigits {