steve/databricks-cli
steve
/
databricks-cli
mirror of https://github.com/databricks/cli.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Properly deal with nil values in `convert.FromTyped` (#1511) 

## Changes

When a configuration defines:
```yaml
run_as:
```

It first showed up as `run_as -> nil` in the dynamic configuration only
to later be converted to `run_as -> {}` while going through typed
conversion. We were using the presence of a key to initialize an empty
value. This is incorrect and it should have remained a nil value.

This conversion was happening in `convert.FromTyped` where any struct
always returned a map value. Instead, it should only return a map value
in any one of these cases: 1) the struct has elements, 2) the struct was
originally a map in the dynamic configuration, or 3) the struct was
initialized to a non-empty pointer value.

Stacked on top of #1516 and #1518.

## Tests

* Unit tests pass.
* Integration tests pass.
* Manually ran through bundle CRUD with a bundle without resources.
This commit is contained in:
Pieter Noordhuis 2024-06-21 15:43:21 +02:00 committed by GitHub
parent 87bc583819
commit 446a9d0c52
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 115 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
bundle/config/mutator/run_as_test.go
View File

@ -18,7 +18,7 @@ import (
func allResourceTypes(t *testing.T) []string {
// Compute supported resource types based on the `Resources{}` struct.
r := config.Resources{}
r := &config.Resources{}
rv, err := convert.FromTyped(r, dyn.NilValue)
require.NoError(t, err)
normalized, _ := convert.Normalize(r, rv, convert.IncludeMissingFields)
@ -154,6 +154,11 @@ func TestRunAsErrorForUnsupportedResources(t *testing.T) {
v, err := convert.FromTyped(base, dyn.NilValue)
require.NoError(t, err)
// Define top level resources key in the bundle configuration.
// This is not part of the typed configuration, so we need to add it manually.
v, err = dyn.Set(v, "resources", dyn.V(map[string]dyn.Value{}))
require.NoError(t, err)
for _, rt := range allResourceTypes(t) {
// Skip allowed resources
if slices.Contains(allowList, rt) {

18
bundle/deploy/terraform/convert_test.go
View File

@ -455,6 +455,24 @@ func TestBundleToTerraformModelServingPermissions(t *testing.T) {
var src = resources.ModelServingEndpoint{
CreateServingEndpoint: &serving.CreateServingEndpoint{
Name: "name",
// Need to specify this to satisfy the equivalence test:
// The previous method of generation includes the "create" field
// because it is required (not marked as `omitempty`).
// The previous method used [json.Marshal] from the standard library
// and as such observed the `omitempty` tag.
// The new method leverages [dyn.Value] where any field that is not
// explicitly set is not part of the value.
Config: serving.EndpointCoreConfigInput{
ServedModels: []serving.ServedModelInput{
{
ModelName: "model_name",
ModelVersion: "1",
ScaleToZeroEnabled: true,
WorkloadSize: "Small",
},
},
},
},
Permissions: []resources.Permission{
{

5
bundle/permissions/filter.go
View File

@ -66,6 +66,11 @@ func (m *filterCurrentUser) Apply(ctx context.Context, b *bundle.Bundle) diag.Di
err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) {
rv, err := dyn.Get(v, "resources")
if err != nil {
// If the resources key is not found, we can skip this mutator.
if dyn.IsNoSuchKeyError(err) {
return v, nil
}
return dyn.InvalidValue, err
}

62
libs/dyn/convert/from_typed.go
View File

@ -12,26 +12,24 @@ import (
type fromTypedOptions int
const (
// If this flag is set, zero values for scalars (strings, bools, ints, floats)
// would resolve to corresponding zero values in the dynamic representation.
// Otherwise, zero values for scalars resolve to dyn.NilValue.
// If this flag is set, zero values in the typed representation are resolved to
// the equivalent zero value in the dynamic representation.
// If it is not set, zero values resolve to [dyn.NilValue].
//
// This flag exists to reconcile the default values for scalars in a Go struct
// being zero values with zero values in a dynamic representation. In a Go struct,
// zero values are the same as the values not being set at all. This is not the case
// in the dynamic representation.
//
// If a scalar value in a typed Go struct is zero, in the dynamic representation
// we would set it to dyn.NilValue, i.e. equivalent to the value not being set at all.
//
// If a scalar value in a Go map, slice or pointer is set to zero, we will set it
// to the zero value in the dynamic representation, and not dyn.NilValue. This is
// equivalent to the value being intentionally set to zero.
includeZeroValuedScalars fromTypedOptions = 1 << iota
// This flag exists to reconcile default values in Go being zero values with values
// being intentionally set to their zero value. We capture zero values in the dynamic
// configuration if they are 1) behind a pointer, 2) a map value, 3) a slice element,
// in the typed configuration.
includeZeroValues fromTypedOptions = 1 << iota
)
// FromTyped converts changes made in the typed structure w.r.t. the configuration value
// back to the configuration value, retaining existing location information where possible.
//
// It uses the reference value both for location information and to determine if the typed
// value was changed or not. For example, if a struct-by-value field is nil in the reference
// it will be zero-valued in the typed configuration. If it remains zero-valued, this
// this function will still emit a nil value in the dynamic representation.
func FromTyped(src any, ref dyn.Value) (dyn.Value, error) {
return fromTyped(src, ref)
}
@ -48,18 +46,18 @@ func fromTyped(src any, ref dyn.Value, options ...fromTypedOptions) (dyn.Value,
}
srcv = srcv.Elem()
// If a pointer to a scalar type points to a zero value, we should include
// If a pointer to a type points to a zero value, we should include
// that zero value in the dynamic representation.
// This is because by default a pointer is nil in Go, and it not being nil
// indicates its value was intentionally set to zero.
if !slices.Contains(options, includeZeroValuedScalars) {
options = append(options, includeZeroValuedScalars)
if !slices.Contains(options, includeZeroValues) {
options = append(options, includeZeroValues)
}
}
switch srcv.Kind() {
case reflect.Struct:
return fromTypedStruct(srcv, ref)
return fromTypedStruct(srcv, ref, options...)
case reflect.Map:
return fromTypedMap(srcv, ref)
case reflect.Slice:
@ -80,7 +78,7 @@ func fromTyped(src any, ref dyn.Value, options ...fromTypedOptions) (dyn.Value,
return dyn.InvalidValue, fmt.Errorf("unsupported type: %s", srcv.Kind())
}
func fromTypedStruct(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
func fromTypedStruct(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) {
// Check that the reference value is compatible or nil.
switch ref.Kind() {
case dyn.KindMap, dyn.KindNil:
@ -108,14 +106,24 @@ func fromTypedStruct(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
return dyn.InvalidValue, err
}
if nv != dyn.NilValue {
// Either if the key was set in the reference or the field is not zero-valued, we include it.
if ok || nv != dyn.NilValue {
out.Set(refk, nv)
}
}
// Return the new mapping if:
// 1. The mapping has entries (i.e. the struct was not empty).
// 2. The reference is a map (i.e. the struct was and still is empty).
// 3. The "includeZeroValues" option is set (i.e. the struct is a non-nil pointer).
if out.Len() > 0 || ref.Kind() == dyn.KindMap || slices.Contains(options, includeZeroValues) {
return dyn.NewValue(out, ref.Location()), nil
}
// Otherwise, return nil.
return dyn.NilValue, nil
}
func fromTypedMap(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
// Check that the reference value is compatible or nil.
switch ref.Kind() {
@ -146,7 +154,7 @@ func fromTypedMap(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
}
// Convert entry taking into account the reference value (may be equal to dyn.NilValue).
nv, err := fromTyped(v.Interface(), refv, includeZeroValuedScalars)
nv, err := fromTyped(v.Interface(), refv, includeZeroValues)
if err != nil {
return dyn.InvalidValue, err
}
@ -183,7 +191,7 @@ func fromTypedSlice(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
}
// Convert entry taking into account the reference value (may be equal to dyn.NilValue).
nv, err := fromTyped(v.Interface(), refv, includeZeroValuedScalars)
nv, err := fromTyped(v.Interface(), refv, includeZeroValues)
if err != nil {
return dyn.InvalidValue, err
}
@ -206,7 +214,7 @@ func fromTypedString(src reflect.Value, ref dyn.Value, options ...fromTypedOptio
case dyn.KindNil:
// This field is not set in the reference. We set it to nil if it's zero
// valued in the typed representation and the includeZeroValues option is not set.
if src.IsZero() && !slices.Contains(options, includeZeroValuedScalars) {
if src.IsZero() && !slices.Contains(options, includeZeroValues) {
return dyn.NilValue, nil
}
return dyn.V(src.String()), nil
@ -226,7 +234,7 @@ func fromTypedBool(src reflect.Value, ref dyn.Value, options ...fromTypedOptions
case dyn.KindNil:
// This field is not set in the reference. We set it to nil if it's zero
// valued in the typed representation and the includeZeroValues option is not set.
if src.IsZero() && !slices.Contains(options, includeZeroValuedScalars) {
if src.IsZero() && !slices.Contains(options, includeZeroValues) {
return dyn.NilValue, nil
}
return dyn.V(src.Bool()), nil
@ -251,7 +259,7 @@ func fromTypedInt(src reflect.Value, ref dyn.Value, options ...fromTypedOptions)
case dyn.KindNil:
// This field is not set in the reference. We set it to nil if it's zero
// valued in the typed representation and the includeZeroValues option is not set.
if src.IsZero() && !slices.Contains(options, includeZeroValuedScalars) {
if src.IsZero() && !slices.Contains(options, includeZeroValues) {
return dyn.NilValue, nil
}
return dyn.V(src.Int()), nil
@ -276,7 +284,7 @@ func fromTypedFloat(src reflect.Value, ref dyn.Value, options ...fromTypedOption
case dyn.KindNil:
// This field is not set in the reference. We set it to nil if it's zero
// valued in the typed representation and the includeZeroValues option is not set.
if src.IsZero() && !slices.Contains(options, includeZeroValuedScalars) {
if src.IsZero() && !slices.Contains(options, includeZeroValues) {
return dyn.NilValue, nil
}
return dyn.V(src.Float()), nil

58
libs/dyn/convert/from_typed_test.go
View File

@ -15,9 +15,14 @@ func TestFromTypedStructZeroFields(t *testing.T) {
}
src := Tmp{}
ref := dyn.NilValue
nv, err := FromTyped(src, ref)
// For an empty struct with a nil reference we expect a nil.
nv, err := FromTyped(src, dyn.NilValue)
require.NoError(t, err)
assert.Equal(t, dyn.NilValue, nv)
// For an empty struct with a non-nil reference we expect an empty map.
nv, err = FromTyped(src, dyn.V(map[string]dyn.Value{}))
require.NoError(t, err)
assert.Equal(t, dyn.V(map[string]dyn.Value{}), nv)
}
@ -28,17 +33,54 @@ func TestFromTypedStructPointerZeroFields(t *testing.T) {
Bar string `json:"bar"`
}
// For an initialized pointer we expect an empty map.
src := &Tmp{}
nv, err := FromTyped(src, dyn.NilValue)
require.NoError(t, err)
assert.Equal(t, dyn.V(map[string]dyn.Value{}), nv)
var src *Tmp
var nv dyn.Value
var err error
// For a nil pointer we expect nil.
// For a nil pointer with a nil reference we expect a nil.
src = nil
nv, err = FromTyped(src, dyn.NilValue)
require.NoError(t, err)
assert.Equal(t, dyn.NilValue, nv)
// For a nil pointer with a non-nil reference we expect a nil.
src = nil
nv, err = FromTyped(src, dyn.V(map[string]dyn.Value{}))
require.NoError(t, err)
assert.Equal(t, dyn.NilValue, nv)
// For an initialized pointer with a nil reference we expect a nil.
src = &Tmp{}
nv, err = FromTyped(src, dyn.NilValue)
require.NoError(t, err)
assert.Equal(t, dyn.V(map[string]dyn.Value{}), nv)
// For an initialized pointer with a non-nil reference we expect an empty map.
src = &Tmp{}
nv, err = FromTyped(src, dyn.V(map[string]dyn.Value{}))
require.NoError(t, err)
assert.Equal(t, dyn.V(map[string]dyn.Value{}), nv)
}
func TestFromTypedStructNilFields(t *testing.T) {
type Tmp struct {
Foo string `json:"foo"`
Bar string `json:"bar"`
}
// For a zero value struct with a reference containing nil fields we expect the nils to be retained.
src := Tmp{}
ref := dyn.V(map[string]dyn.Value{
"foo": dyn.NilValue,
"bar": dyn.NilValue,
})
nv, err := FromTyped(src, ref)
require.NoError(t, err)
assert.Equal(t, dyn.V(map[string]dyn.Value{
"foo": dyn.NilValue,
"bar": dyn.NilValue,
}), nv)
}
func TestFromTypedStructSetFields(t *testing.T) {