Skip to content

Commit

Permalink
types/basetypes: Fix Float64Attribute precision comparisons (#817)
Browse files Browse the repository at this point in the history
* implement without semantic equality initially

* add initial semantic equals

* remove todos + add doc string

* add semantic equal tests

* add comment

* added changelog

* Update .changes/unreleased/BUG FIXES-20230803-120411.yaml

Co-authored-by: Brian Flad <bflad417@gmail.com>

* add panic prevent

* removed copy

* space

* Update types/basetypes/float64_value.go

Co-authored-by: Brian Flad <bflad417@gmail.com>

* add docs for nan

* fix cmp nil error

* add another test

---------

Co-authored-by: Brian Flad <bflad417@gmail.com>
  • Loading branch information
austinvalle and bflad authored Aug 3, 2023
1 parent f937d74 commit a4b8bc6
Show file tree
Hide file tree
Showing 5 changed files with 245 additions and 21 deletions.
6 changes: 6 additions & 0 deletions .changes/unreleased/BUG FIXES-20230803-120411.yaml
Original file line number Diff line number Diff line change
@@ -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"
6 changes: 5 additions & 1 deletion types/basetypes/float64_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
31 changes: 25 additions & 6 deletions types/basetypes/float64_type_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
58 changes: 50 additions & 8 deletions types/basetypes/float64_value.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package basetypes
import (
"context"
"fmt"
"math/big"

"github.com/hashicorp/terraform-plugin-go/tftypes"

Expand All @@ -14,7 +15,8 @@ import (
)

var (
_ Float64Valuable = Float64Value{}
_ Float64Valuable = Float64Value{}
_ Float64ValuableWithSemanticEquals = Float64Value{}
)

// Float64Valuable extends attr.Value for float64 value types.
Expand Down Expand Up @@ -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()
Expand All @@ -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`.
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down
165 changes: 159 additions & 6 deletions types/basetypes/float64_value_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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)
}
})
}
}

0 comments on commit a4b8bc6

Please sign in to comment.