diff --git a/acceptance/bundle/resource_deps/bad_ref_float_to_int/databricks.yml b/acceptance/bundle/resource_deps/bad_ref_float_to_int/databricks.yml new file mode 100644 index 00000000000..ded0667ebe4 --- /dev/null +++ b/acceptance/bundle/resource_deps/bad_ref_float_to_int/databricks.yml @@ -0,0 +1,15 @@ +bundle: + name: test-bundle + +resources: + jobs: + bar: + name: job bar + job_clusters: + - job_cluster_key: main + new_cluster: + azure_attributes: + spot_bid_max_price: 1.9 + foo: + name: job foo + max_concurrent_runs: ${resources.jobs.bar.job_clusters[0].new_cluster.azure_attributes.spot_bid_max_price} diff --git a/acceptance/bundle/resource_deps/bad_ref_float_to_int/out.plan.txt b/acceptance/bundle/resource_deps/bad_ref_float_to_int/out.plan.txt new file mode 100644 index 00000000000..73bc1960b6b --- /dev/null +++ b/acceptance/bundle/resource_deps/bad_ref_float_to_int/out.plan.txt @@ -0,0 +1,6 @@ + +>>> musterr [CLI] bundle plan +Error: cannot plan resources.jobs.foo: cannot update max_concurrent_runs with value of "${resources.jobs.bar.job_clusters[0].new_cluster.azure_attributes.spot_bid_max_price}": cannot set (*jobs.JobSettings).max_concurrent_runs to float64 (1.9): cannot set 1.9 to int: precision loss + +Error: planning failed + diff --git a/acceptance/bundle/resource_deps/bad_ref_float_to_int/out.test.toml b/acceptance/bundle/resource_deps/bad_ref_float_to_int/out.test.toml new file mode 100644 index 00000000000..a927a5fbc06 --- /dev/null +++ b/acceptance/bundle/resource_deps/bad_ref_float_to_int/out.test.toml @@ -0,0 +1,2 @@ +Cloud = false +EnvMatrix.DMS = ["", "true"] diff --git a/acceptance/bundle/resource_deps/bad_ref_float_to_int/output.txt b/acceptance/bundle/resource_deps/bad_ref_float_to_int/output.txt new file mode 100644 index 00000000000..e69de29bb2d diff --git a/acceptance/bundle/resource_deps/bad_ref_float_to_int/script b/acceptance/bundle/resource_deps/bad_ref_float_to_int/script new file mode 100644 index 00000000000..f486f04ba0b --- /dev/null +++ b/acceptance/bundle/resource_deps/bad_ref_float_to_int/script @@ -0,0 +1 @@ +trace musterr $CLI bundle plan &> out.plan.txt diff --git a/acceptance/bundle/resource_deps/bad_ref_float_to_int/test.toml b/acceptance/bundle/resource_deps/bad_ref_float_to_int/test.toml new file mode 100644 index 00000000000..e66a9af3b05 --- /dev/null +++ b/acceptance/bundle/resource_deps/bad_ref_float_to_int/test.toml @@ -0,0 +1,7 @@ +RecordRequests = false + +Env.DATABRICKS_CACHE_ENABLED = 'false' + +[[Repls]] +Old = 'terraform.tfstate' +New = 'resources.json' diff --git a/acceptance/bundle/variables/float_to_int_field/databricks.yml b/acceptance/bundle/variables/float_to_int_field/databricks.yml new file mode 100644 index 00000000000..d356d4e787f --- /dev/null +++ b/acceptance/bundle/variables/float_to_int_field/databricks.yml @@ -0,0 +1,13 @@ +bundle: + name: test-bundle + +variables: + x: + default: 5.9 + +resources: + jobs: + foo: + name: job foo + # Records current behaviour: a warning, the value is dropped and the default (1) applies. + max_concurrent_runs: ${var.x} diff --git a/acceptance/bundle/variables/float_to_int_field/out.test.toml b/acceptance/bundle/variables/float_to_int_field/out.test.toml new file mode 100644 index 00000000000..a927a5fbc06 --- /dev/null +++ b/acceptance/bundle/variables/float_to_int_field/out.test.toml @@ -0,0 +1,2 @@ +Cloud = false +EnvMatrix.DMS = ["", "true"] diff --git a/acceptance/bundle/variables/float_to_int_field/output.txt b/acceptance/bundle/variables/float_to_int_field/output.txt new file mode 100644 index 00000000000..2adb343ab98 --- /dev/null +++ b/acceptance/bundle/variables/float_to_int_field/output.txt @@ -0,0 +1,28 @@ + +>>> [CLI] bundle validate +Warning: cannot accurately represent "5.9" as integer due to precision loss + at resources.jobs.foo.max_concurrent_runs + in databricks.yml:13:28 + +Name: test-bundle +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default + +Found 1 warning + +>>> [CLI] bundle validate -o json +Warning: cannot accurately represent "5.9" as integer due to precision loss + at resources.jobs.foo.max_concurrent_runs + in databricks.yml:13:28 + +{ + "variables": { + "x": { + "default": 5.9, + "value": 5.9 + } + }, + "max_concurrent_runs": 1 +} diff --git a/acceptance/bundle/variables/float_to_int_field/script b/acceptance/bundle/variables/float_to_int_field/script new file mode 100644 index 00000000000..dee8a94c629 --- /dev/null +++ b/acceptance/bundle/variables/float_to_int_field/script @@ -0,0 +1,2 @@ +trace $CLI bundle validate +trace $CLI bundle validate -o json | jq '{variables, max_concurrent_runs: .resources.jobs.foo.max_concurrent_runs}' diff --git a/libs/structs/structaccess/set.go b/libs/structs/structaccess/set.go index 02752e456ea..ef1d055077d 100644 --- a/libs/structs/structaccess/set.go +++ b/libs/structs/structaccess/set.go @@ -3,6 +3,7 @@ package structaccess import ( "errors" "fmt" + "math" "reflect" "slices" "strconv" @@ -280,6 +281,11 @@ func convertValue(valueVal reflect.Value, targetType reflect.Type) (reflect.Valu return valueVal, nil } + // Numeric conversions must not silently truncate, wrap or round. + if isNumericKind(valueType.Kind()) && isNumericKind(targetType.Kind()) { + return convertNumeric(valueVal, targetType) + } + // Convertibility check (handles typedefed types) if valueType.ConvertibleTo(targetType) { return valueVal.Convert(targetType), nil @@ -294,6 +300,15 @@ func convertValue(valueVal reflect.Value, targetType reflect.Type) (reflect.Valu ptr.Elem().Set(valueVal) return ptr, nil } + if isNumericKind(valueType.Kind()) && isNumericKind(elemType.Kind()) { + converted, err := convertNumeric(valueVal, elemType) + if err != nil { + return reflect.Value{}, err + } + ptr := reflect.New(elemType) + ptr.Elem().Set(converted) + return ptr, nil + } if valueType.ConvertibleTo(elemType) { ptr := reflect.New(elemType) ptr.Elem().Set(valueVal.Convert(elemType)) @@ -310,6 +325,78 @@ func convertValue(valueVal reflect.Value, targetType reflect.Type) (reflect.Valu return reflect.Value{}, fmt.Errorf("cannot convert %s to %s", valueType, targetType) } +func isNumericKind(k reflect.Kind) bool { + switch k { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, + reflect.Float32, reflect.Float64: + return true + default: + return false + } +} + +// convertNumeric converts between numeric kinds and fails instead of losing data, +// following the rules of libs/dyn/convert (normalizeInt). Unlike normalizeFloat, +// any conversion to a float kind is allowed, even when it rounds or overflows. +func convertNumeric(valueVal reflect.Value, targetType reflect.Type) (reflect.Value, error) { + dstKind := targetType.Kind() + zero := reflect.New(targetType).Elem() + + switch { + case valueVal.CanInt(): + v := valueVal.Int() + switch { + case zero.CanInt(): + if zero.OverflowInt(v) { + return reflect.Value{}, fmt.Errorf("value %d overflows %s", v, dstKind) + } + case zero.CanUint(): + if v < 0 || zero.OverflowUint(uint64(v)) { + return reflect.Value{}, fmt.Errorf("value %d overflows %s", v, dstKind) + } + default: + // Int to float: allowed even with a loss. + } + + case valueVal.CanUint(): + v := valueVal.Uint() + switch { + case zero.CanInt(): + if v > math.MaxInt64 || zero.OverflowInt(int64(v)) { + return reflect.Value{}, fmt.Errorf("value %d overflows %s", v, dstKind) + } + case zero.CanUint(): + if zero.OverflowUint(v) { + return reflect.Value{}, fmt.Errorf("value %d overflows %s", v, dstKind) + } + default: + // Uint to float: allowed even with a loss. + } + + default: + f := valueVal.Float() + if !zero.CanInt() && !zero.CanUint() { + // Float to float: allowed even with a loss. + break + } + if math.IsNaN(f) || f != math.Trunc(f) { + return reflect.Value{}, fmt.Errorf("cannot set %v to %s: precision loss", f, dstKind) + } + // Bounds are powers of two, so they are exact in float64. + bits := float64(targetType.Bits()) + lo, hi := 0.0, math.Exp2(bits) + if zero.CanInt() { + lo, hi = -math.Exp2(bits-1), math.Exp2(bits-1) + } + if f < lo || f >= hi { + return reflect.Value{}, fmt.Errorf("value %v overflows %s", f, dstKind) + } + } + + return valueVal.Convert(targetType), nil +} + // updateForceSendFields handles ForceSendFields when setting values: // - If setting nil: remove field from ForceSendFields // - If setting empty value: add field to ForceSendFields (if not already present) diff --git a/libs/structs/structaccess/set_numeric_test.go b/libs/structs/structaccess/set_numeric_test.go new file mode 100644 index 00000000000..48d3905a86a --- /dev/null +++ b/libs/structs/structaccess/set_numeric_test.go @@ -0,0 +1,312 @@ +package structaccess_test + +import ( + "math" + "testing" + + "github.com/databricks/cli/libs/structs/structaccess" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type numericTarget struct { + I int `json:"i,omitempty"` + I8 int8 `json:"i8,omitempty"` + I32 int32 `json:"i32,omitempty"` + I64 int64 `json:"i64,omitempty"` + U uint `json:"u,omitempty"` + U8 uint8 `json:"u8,omitempty"` + U64 uint64 `json:"u64,omitempty"` + F32 float32 `json:"f32,omitempty"` + F64 float64 `json:"f64,omitempty"` + PI *int `json:"pi,omitempty"` + PI32 *int32 `json:"pi32,omitempty"` + PU *uint `json:"pu,omitempty"` + PF32 *float32 `json:"pf32,omitempty"` + PF64 *float64 `json:"pf64,omitempty"` +} + +func TestSetNumericConversion(t *testing.T) { + tests := []struct { + name string + path string + value any + // Exactly one of expected and err is set; expected has the type of the target field. + expected any + err string + }{ + // int <- float + { + name: "int from integral float", + path: "i", + value: 2.0, + expected: 2, + }, + { + name: "int from negative integral float", + path: "i", + value: -3.0, + expected: -3, + }, + { + name: "int from fractional float", + path: "i", + value: 1.9, + err: "cannot set 1.9 to int: precision loss", + }, + { + name: "int from NaN", + path: "i", + value: math.NaN(), + err: "cannot set NaN to int: precision loss", + }, + { + name: "int from +Inf", + path: "i", + value: math.Inf(1), + err: "value +Inf overflows int", + }, + { + name: "int8 from out of range float", + path: "i8", + value: 200.0, + err: "value 200 overflows int8", + }, + { + name: "int64 from float 2^63", + path: "i64", + value: math.Exp2(63), + err: "overflows int64", + }, + { + name: "int64 from float -2^63", + path: "i64", + value: -math.Exp2(63), + expected: int64(math.MinInt64), + }, + { + name: "uint from negative float", + path: "u", + value: -1.0, + err: "value -1 overflows uint", + }, + { + name: "uint8 from out of range float", + path: "u8", + value: 256.0, + err: "value 256 overflows uint8", + }, + { + name: "uint64 from float 2^64", + path: "u64", + value: math.Exp2(64), + err: "overflows uint64", + }, + { + name: "uint from float32 integral", + path: "u", + value: float32(7), + expected: uint(7), + }, + + // int <- int/uint + { + name: "int32 from fitting int64", + path: "i32", + value: int64(5), + expected: int32(5), + }, + { + name: "int32 from overflowing int64", + path: "i32", + value: int64(1 << 40), + err: "value 1099511627776 overflows int32", + }, + { + name: "int8 from negative overflowing int", + path: "i8", + value: -129, + err: "value -129 overflows int8", + }, + { + name: "int64 from uint64 above MaxInt64", + path: "i64", + value: uint64(math.MaxUint64), + err: "value 18446744073709551615 overflows int64", + }, + { + name: "int64 from uint64 MaxInt64", + path: "i64", + value: uint64(math.MaxInt64), + expected: int64(math.MaxInt64), + }, + { + name: "int8 from overflowing uint", + path: "i8", + value: uint(128), + err: "value 128 overflows int8", + }, + + // uint <- int/uint + { + name: "uint from negative int", + path: "u", + value: -1, + err: "value -1 overflows uint", + }, + { + name: "uint8 from overflowing int", + path: "u8", + value: 256, + err: "value 256 overflows uint8", + }, + { + name: "uint8 from fitting int", + path: "u8", + value: 255, + expected: uint8(255), + }, + { + name: "uint8 from overflowing uint64", + path: "u8", + value: uint64(300), + err: "value 300 overflows uint8", + }, + { + name: "uint64 from large uint64", + path: "u64", + value: uint64(math.MaxUint64), + expected: uint64(math.MaxUint64), + }, + + // float <- int/uint + { + name: "float64 from small int", + path: "f64", + value: 1 << 53, + expected: float64(1 << 53), + }, + { + name: "float64 from int64 above 2^53 rounds", + path: "f64", + value: int64(1<<53 + 1), + expected: float64(1 << 53), + }, + { + name: "float64 from MaxInt64 rounds", + path: "f64", + value: int64(math.MaxInt64), + expected: float64(math.MaxInt64), + }, + { + name: "float64 from uint64 above 2^53 rounds", + path: "f64", + value: uint64(1<<53 + 1), + expected: float64(1 << 53), + }, + { + name: "float64 from MaxUint64 rounds", + path: "f64", + value: uint64(math.MaxUint64), + expected: float64(math.MaxUint64), + }, + { + name: "float32 from int above 2^24 rounds", + path: "f32", + value: 1<<24 + 1, + expected: float32(1 << 24), + }, + { + name: "float32 from int 2^24", + path: "f32", + value: 1 << 24, + expected: float32(1 << 24), + }, + + // float <- float + { + name: "float32 from float64", + path: "f32", + value: 1.5, + expected: float32(1.5), + }, + { + name: "float32 from rounding float64", + path: "f32", + value: 1.1, + expected: float32(1.1), + }, + { + name: "float32 from overflowing float64", + path: "f32", + value: 1e300, + expected: float32(math.Inf(1)), + }, + { + name: "float64 from float32", + path: "f64", + value: float32(0.5), + expected: 0.5, + }, + + // pointer targets + { + name: "ptr int from integral float", + path: "pi", + value: 2.0, + expected: 2, + }, + { + name: "ptr int from fractional float", + path: "pi", + value: 1.9, + err: "cannot set 1.9 to int: precision loss", + }, + { + name: "ptr int32 from overflowing int64", + path: "pi32", + value: int64(1 << 40), + err: "value 1099511627776 overflows int32", + }, + { + name: "ptr uint from negative int", + path: "pu", + value: -1, + err: "value -1 overflows uint", + }, + { + name: "ptr float32 from overflowing float64", + path: "pf32", + value: 1e300, + expected: float32(math.Inf(1)), + }, + { + name: "ptr float64 from large int64 rounds", + path: "pf64", + value: int64(1<<53 + 1), + expected: float64(1 << 53), + }, + { + name: "ptr float64 from small int", + path: "pf64", + value: 3, + expected: 3.0, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + v := &numericTarget{} + err := structaccess.SetByString(v, tc.path, tc.value) + if tc.err != "" { + require.ErrorContains(t, err, tc.err) + assert.Equal(t, &numericTarget{}, v, "failed set must leave the struct untouched") + return + } + require.NoError(t, err) + got, err := structaccess.GetByString(v, tc.path) + require.NoError(t, err) + assert.Equal(t, tc.expected, got) + }) + } +} diff --git a/libs/structs/structvar/structvar_test.go b/libs/structs/structvar/structvar_test.go index 9d4c2532ad6..81009829c70 100644 --- a/libs/structs/structvar/structvar_test.go +++ b/libs/structs/structvar/structvar_test.go @@ -290,3 +290,12 @@ func TestToStructVarRequiresPointerType(t *testing.T) { require.NoError(t, err) assert.Equal(t, "test", sv.Value.(*TestObj).Name) } + +func TestResolveRefLossyNumericConversion(t *testing.T) { + sv := structvar.NewStructVar(&TestObj{Age: 25}, map[string]string{"age": "${var.age}"}) + + err := sv.ResolveRef("${var.age}", 1.9) + require.ErrorContains(t, err, "cannot set 1.9 to int: precision loss") + assert.Equal(t, 25, sv.Value.(*TestObj).Age) + assert.Contains(t, sv.Refs, "age") +}