diff --git a/bundle/config/mutator/run_as_test.go b/bundle/config/mutator/run_as_test.go index c57de847..67bf7bcc 100644 --- a/bundle/config/mutator/run_as_test.go +++ b/bundle/config/mutator/run_as_test.go @@ -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) { diff --git a/bundle/deploy/terraform/convert_test.go b/bundle/deploy/terraform/convert_test.go index e1f73be2..7ea44853 100644 --- a/bundle/deploy/terraform/convert_test.go +++ b/bundle/deploy/terraform/convert_test.go @@ -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{ { diff --git a/bundle/permissions/filter.go b/bundle/permissions/filter.go index 6d39630c..60264f6e 100644 --- a/bundle/permissions/filter.go +++ b/bundle/permissions/filter.go @@ -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 } diff --git a/libs/dyn/convert/from_typed.go b/libs/dyn/convert/from_typed.go index e5fb0de6..af49a07a 100644 --- a/libs/dyn/convert/from_typed.go +++ b/libs/dyn/convert/from_typed.go @@ -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,12 +106,22 @@ 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 dyn.NewValue(out, ref.Location()), nil + // 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) { @@ -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 diff --git a/libs/dyn/convert/from_typed_test.go b/libs/dyn/convert/from_typed_test.go index 7a0dad84..e5447fe8 100644 --- a/libs/dyn/convert/from_typed_test.go +++ b/libs/dyn/convert/from_typed_test.go @@ -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) {