diff --git a/.changes/unreleased/BUG FIXES-20230803-120411.yaml b/.changes/unreleased/BUG FIXES-20230803-120411.yaml new file mode 100644 index 000000000..1d15765a4 --- /dev/null +++ b/.changes/unreleased/BUG FIXES-20230803-120411.yaml @@ -0,0 +1,6 @@ +kind: BUG FIXES +body: 'types/basetypes: Prevented Float64Value Terraform data consistency errors for + numbers with high precision floating point rounding errors' +time: 2023-08-03T12:04:11.996955-04:00 +custom: + Issue: "817" diff --git a/types/basetypes/float64_type.go b/types/basetypes/float64_type.go index b72df48d3..6c690690b 100644 --- a/types/basetypes/float64_type.go +++ b/types/basetypes/float64_type.go @@ -155,7 +155,11 @@ func (t Float64Type) ValueFromTerraform(ctx context.Context, in tftypes.Value) ( return nil, fmt.Errorf("Value %s cannot be represented as a 64-bit floating point.", bigF) } - return NewFloat64Value(f), nil + // Underlying *big.Float values are not exposed with helper functions, so creating Float64Value via struct literal + return Float64Value{ + state: attr.ValueStateKnown, + value: bigF, + }, nil } // ValueType returns the Value type. diff --git a/types/basetypes/float64_type_test.go b/types/basetypes/float64_type_test.go index 25aa505d1..4f1035254 100644 --- a/types/basetypes/float64_type_test.go +++ b/types/basetypes/float64_type_test.go @@ -136,17 +136,36 @@ func TestFloat64TypeValueFromTerraform(t *testing.T) { expectedErr: "can't unmarshal tftypes.String into *big.Float, expected *big.Float", }, // Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/647 + // To ensure underlying *big.Float precision matches, create `expectation` via struct literal "zero-string-float": { - input: tftypes.NewValue(tftypes.Number, testMustParseFloat("0.0")), - expectation: NewFloat64Value(0.0), + input: tftypes.NewValue(tftypes.Number, testMustParseFloat("0.0")), + expectation: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("0.0"), + }, }, "positive-string-float": { - input: tftypes.NewValue(tftypes.Number, testMustParseFloat("123.2")), - expectation: NewFloat64Value(123.2), + input: tftypes.NewValue(tftypes.Number, testMustParseFloat("123.2")), + expectation: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("123.2"), + }, }, "negative-string-float": { - input: tftypes.NewValue(tftypes.Number, testMustParseFloat("-123.2")), - expectation: NewFloat64Value(-123.2), + input: tftypes.NewValue(tftypes.Number, testMustParseFloat("-123.2")), + expectation: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("-123.2"), + }, + }, + // Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/815 + // To ensure underlying *big.Float precision matches, create `expectation` via struct literal + "retain-string-float-512-precision": { + input: tftypes.NewValue(tftypes.Number, testMustParseFloat("0.010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000003")), + expectation: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("0.010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000003"), + }, }, // Reference: https://pkg.go.dev/math/big#Float.Float64 // Reference: https://pkg.go.dev/math#pkg-constants diff --git a/types/basetypes/float64_value.go b/types/basetypes/float64_value.go index 6d0873a12..fb9c19a5e 100644 --- a/types/basetypes/float64_value.go +++ b/types/basetypes/float64_value.go @@ -6,6 +6,7 @@ package basetypes import ( "context" "fmt" + "math/big" "github.com/hashicorp/terraform-plugin-go/tftypes" @@ -14,7 +15,8 @@ import ( ) var ( - _ Float64Valuable = Float64Value{} + _ Float64Valuable = Float64Value{} + _ Float64ValuableWithSemanticEquals = Float64Value{} ) // Float64Valuable extends attr.Value for float64 value types. @@ -61,19 +63,20 @@ func NewFloat64Unknown() Float64Value { } // Float64Value creates a Float64 with a known value. Access the value via the Float64 -// type ValueFloat64 method. +// type ValueFloat64 method. Passing a value of `NaN` will result in a panic. // // Setting the deprecated Float64 type Null, Unknown, or Value fields after // creating a Float64 with this function has no effect. func NewFloat64Value(value float64) Float64Value { return Float64Value{ state: attr.ValueStateKnown, - value: value, + value: big.NewFloat(value), } } // NewFloat64PointerValue creates a Float64 with a null value if nil or a known // value. Access the value via the Float64 type ValueFloat64Pointer method. +// Passing a value of `NaN` will result in a panic. func NewFloat64PointerValue(value *float64) Float64Value { if value == nil { return NewFloat64Null() @@ -89,7 +92,29 @@ type Float64Value struct { state attr.ValueState // value contains the known value, if not null or unknown. - value float64 + value *big.Float +} + +// Float64SemanticEquals returns true if the given Float64Value is semantically equal to the current Float64Value. +// The underlying value *big.Float can store more precise float values then the Go built-in float64 type. This only +// compares the precision of the value that can be represented as the Go built-in float64, which is 53 bits of precision. +func (f Float64Value) Float64SemanticEquals(ctx context.Context, newValuable Float64Valuable) (bool, diag.Diagnostics) { + var diags diag.Diagnostics + + newValue, ok := newValuable.(Float64Value) + if !ok { + diags.AddError( + "Semantic Equality Check Error", + "An unexpected value type was received while performing semantic equality checks. "+ + "Please report this to the provider developers.\n\n"+ + "Expected Value Type: "+fmt.Sprintf("%T", f)+"\n"+ + "Got Value Type: "+fmt.Sprintf("%T", newValuable), + ) + + return false, diags + } + + return f.ValueFloat64() == newValue.ValueFloat64(), diags } // Equal returns true if `other` is a Float64 and has the same value as `f`. @@ -108,7 +133,12 @@ func (f Float64Value) Equal(other attr.Value) bool { return true } - return f.value == o.value + // Not possible to create a known Float64Value with a nil value, but check anyways + if f.value == nil || o.value == nil { + return f.value == o.value + } + + return f.value.Cmp(o.value) == 0 } // ToTerraformValue returns the data contained in the Float64 as a tftypes.Value. @@ -156,13 +186,19 @@ func (f Float64Value) String() string { return attr.NullValueString } - return fmt.Sprintf("%f", f.value) + f64 := f.ValueFloat64() + return fmt.Sprintf("%f", f64) } // ValueFloat64 returns the known float64 value. If Float64 is null or unknown, returns // 0.0. func (f Float64Value) ValueFloat64() float64 { - return f.value + if f.IsNull() || f.IsUnknown() { + return float64(0.0) + } + + f64, _ := f.value.Float64() + return f64 } // ValueFloat64Pointer returns a pointer to the known float64 value, nil for a @@ -172,7 +208,13 @@ func (f Float64Value) ValueFloat64Pointer() *float64 { return nil } - return &f.value + if f.IsUnknown() { + f64 := float64(0.0) + return &f64 + } + + f64, _ := f.value.Float64() + return &f64 } // ToFloat64Value returns Float64. diff --git a/types/basetypes/float64_value_test.go b/types/basetypes/float64_value_test.go index 9c8e54874..ca9dca6b9 100644 --- a/types/basetypes/float64_value_test.go +++ b/types/basetypes/float64_value_test.go @@ -11,6 +11,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/hashicorp/terraform-plugin-framework/attr" + "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-go/tftypes" ) @@ -97,16 +98,92 @@ func TestFloat64ValueEqual(t *testing.T) { expectation bool } tests := map[string]testCase{ - "known-known-same": { - input: NewFloat64Value(123), - candidate: NewFloat64Value(123), + "known-known-53-precison-same": { + input: NewFloat64Value(123.123), + candidate: NewFloat64Value(123.123), expectation: true, }, - "known-known-diff": { - input: NewFloat64Value(123), - candidate: NewFloat64Value(456), + "known-known-53-precison-diff": { + input: NewFloat64Value(123.123), + candidate: NewFloat64Value(456.456), + expectation: false, + }, + "known-known-512-precision-same": { + input: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("0.010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001"), + }, + candidate: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("0.010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001"), + }, + expectation: true, + }, + "known-known-512-precision-diff": { + input: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("0.010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001"), + }, + candidate: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("0.010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000009"), + }, expectation: false, }, + "known-known-512-precision-mantissa-diff": { + input: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("0.010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001"), + }, + candidate: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("0.01"), + }, + expectation: false, + }, + "known-known-precisiondiff-mantissa-same": { + input: NewFloat64Value(123), + candidate: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("123"), + }, + expectation: true, + }, + "known-known-precisiondiff-mantissa-diff": { + input: NewFloat64Value(0.1), + candidate: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("0.1"), + }, + expectation: false, + }, + "knownnil-known": { + input: Float64Value{ + state: attr.ValueStateKnown, + value: nil, + }, + candidate: NewFloat64Value(0.1), + expectation: false, + }, + "known-knownnil": { + input: NewFloat64Value(0.1), + candidate: Float64Value{ + state: attr.ValueStateKnown, + value: nil, + }, + expectation: false, + }, + "knownnil-knownnil": { + input: Float64Value{ + state: attr.ValueStateKnown, + value: nil, + }, + candidate: Float64Value{ + state: attr.ValueStateKnown, + value: nil, + }, + expectation: true, + }, "known-unknown": { input: NewFloat64Value(123), candidate: NewFloat64Unknown(), @@ -395,3 +472,79 @@ func TestNewFloat64PointerValue(t *testing.T) { }) } } + +func TestFloat64ValueFloat64SemanticEquals(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + currentFloat64 Float64Value + givenFloat64 Float64Value + expectedMatch bool + expectedDiags diag.Diagnostics + }{ + "not equal - whole number": { + currentFloat64: NewFloat64Value(1), + givenFloat64: NewFloat64Value(2), + expectedMatch: false, + }, + "not equal - float": { + currentFloat64: NewFloat64Value(1.1), + givenFloat64: NewFloat64Value(1.2), + expectedMatch: false, + }, + "not equal - float differing precision": { + currentFloat64: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("0.01"), + }, + givenFloat64: NewFloat64Value(0.02), + expectedMatch: false, + }, + "semantically equal - whole number": { + currentFloat64: NewFloat64Value(1), + givenFloat64: NewFloat64Value(1), + expectedMatch: true, + }, + "semantically equal - float": { + currentFloat64: NewFloat64Value(1.1), + givenFloat64: NewFloat64Value(1.1), + expectedMatch: true, + }, + "semantically equal - float differing precision": { + currentFloat64: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("0.01"), + }, + givenFloat64: NewFloat64Value(0.01), + expectedMatch: true, + }, + // Only 53 bits of precision are compared, Go built-in float64 + "semantically equal - float 512 precision, different value not significant": { + currentFloat64: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("0.010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001"), + }, + givenFloat64: Float64Value{ + state: attr.ValueStateKnown, + value: testMustParseFloat("0.010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000009"), + }, + expectedMatch: true, + }, + } + for name, testCase := range testCases { + name, testCase := name, testCase + t.Run(name, func(t *testing.T) { + t.Parallel() + + match, diags := testCase.currentFloat64.Float64SemanticEquals(context.Background(), testCase.givenFloat64) + + if testCase.expectedMatch != match { + t.Errorf("Expected Float64SemanticEquals to return: %t, but got: %t", testCase.expectedMatch, match) + } + + if diff := cmp.Diff(diags, testCase.expectedDiags); diff != "" { + t.Errorf("Unexpected diagnostics (-got, +expected): %s", diff) + } + }) + } +}