From 2e7b27c7a22335068848b3d80fc7cf3dccc10de5 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 16 Oct 2023 13:54:39 +0200 Subject: [PATCH 1/8] Store kind of value in the Value struct --- libs/config/kind.go | 41 +++++++++++++++++++++++++++++++ libs/config/value.go | 58 +++++++++++++++++++++++++------------------- 2 files changed, 74 insertions(+), 25 deletions(-) create mode 100644 libs/config/kind.go diff --git a/libs/config/kind.go b/libs/config/kind.go new file mode 100644 index 0000000000..f3017a1dbe --- /dev/null +++ b/libs/config/kind.go @@ -0,0 +1,41 @@ +package config + +import "time" + +type Kind int + +const ( + // Invalid is the zero value of Kind. + KindInvalid Kind = iota + KindMap + KindSequence + KindNil + KindString + KindBool + KindInt + KindFloat + KindTime +) + +func kindOf(v any) Kind { + switch v.(type) { + case map[string]Value: + return KindMap + case []Value: + return KindSequence + case nil: + return KindNil + case string: + return KindString + case bool: + return KindBool + case int, int32, int64: + return KindInt + case float32, float64: + return KindFloat + case time.Time: + return KindTime + default: + panic("not handled") + } +} diff --git a/libs/config/value.go b/libs/config/value.go index 994aec3895..00282e1ee3 100644 --- a/libs/config/value.go +++ b/libs/config/value.go @@ -1,9 +1,11 @@ package config -import "time" +import "fmt" type Value struct { v any + + k Kind l Location // Whether or not this value is an anchor. @@ -12,12 +14,15 @@ type Value struct { } // NilValue is equal to the zero-value of Value. -var NilValue = Value{} +var NilValue = Value{ + k: KindNil, +} // NewValue constructs a new Value with the given value and location. func NewValue(v any, loc Location) Value { return Value{ v: v, + k: kindOf(v), l: loc, } } @@ -27,45 +32,47 @@ func (v Value) AsMap() (map[string]Value, bool) { return m, ok } +func (v Value) Kind() Kind { + return v.k +} + func (v Value) Location() Location { return v.l } func (v Value) AsAny() any { - switch vv := v.v.(type) { - case map[string]Value: - m := make(map[string]any) + switch v.k { + case KindInvalid: + panic("invoked AsAny on invalid value") + case KindMap: + vv := v.v.(map[string]Value) + m := make(map[string]any, len(vv)) for k, v := range vv { m[k] = v.AsAny() } return m - case []Value: + case KindSequence: + vv := v.v.([]Value) a := make([]any, len(vv)) for i, v := range vv { a[i] = v.AsAny() } return a - case string: - return vv - case bool: - return vv - case int: - return vv - case int32: - return vv - case int64: - return vv - case float32: - return vv - case float64: - return vv - case time.Time: - return vv - case nil: - return nil + case KindNil: + return v.v + case KindString: + return v.v + case KindBool: + return v.v + case KindInt: + return v.v + case KindFloat: + return v.v + case KindTime: + return v.v default: // Panic because we only want to deal with known types. - panic("not handled") + panic(fmt.Sprintf("invalid kind: %d", v.k)) } } @@ -99,6 +106,7 @@ func (v Value) Index(i int) Value { func (v Value) MarkAnchor() Value { return Value{ v: v.v, + k: v.k, l: v.l, anchor: true, From c548f06998c42b4eb6e737754194e29e01d14909 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 18 Oct 2023 08:57:02 +0200 Subject: [PATCH 2/8] Convert config.Value to Go struct --- bundle/config/convert/error.go | 8 + bundle/config/convert/struct_info.go | 86 ++++++ bundle/config/convert/struct_info_test.go | 89 ++++++ bundle/config/convert/to_typed.go | 178 ++++++++++++ bundle/config/convert/to_typed_test.go | 336 ++++++++++++++++++++++ libs/config/value.go | 57 +++- 6 files changed, 753 insertions(+), 1 deletion(-) create mode 100644 bundle/config/convert/error.go create mode 100644 bundle/config/convert/struct_info.go create mode 100644 bundle/config/convert/struct_info_test.go create mode 100644 bundle/config/convert/to_typed.go create mode 100644 bundle/config/convert/to_typed_test.go diff --git a/bundle/config/convert/error.go b/bundle/config/convert/error.go new file mode 100644 index 0000000000..e651da1309 --- /dev/null +++ b/bundle/config/convert/error.go @@ -0,0 +1,8 @@ +package convert + +import "github.com/databricks/cli/libs/config" + +type TypeError struct { + value config.Value + msg string +} diff --git a/bundle/config/convert/struct_info.go b/bundle/config/convert/struct_info.go new file mode 100644 index 0000000000..82cf0001f1 --- /dev/null +++ b/bundle/config/convert/struct_info.go @@ -0,0 +1,86 @@ +package convert + +import ( + "reflect" + "strings" + "sync" +) + +// structInfo holds the type information we need to efficiently +// convert data from a [config.Value] to a Go struct. +type structInfo struct { + // Fields maps the JSON-name of the field to the field's index for use with [FieldByIndex]. + Fields map[string][]int +} + +// structInfoCache caches type information. +var structInfoCache = make(map[reflect.Type]structInfo) + +// structInfoCacheLock guards concurrent access to structInfoCache. +var structInfoCacheLock sync.Mutex + +// getStructInfo returns the [structInfo] for the given type. +// It lazily populates a cache, so the first call for a given +// type is slower than subsequent calls for that same type. +func getStructInfo(typ reflect.Type) structInfo { + structInfoCacheLock.Lock() + defer structInfoCacheLock.Unlock() + + si, ok := structInfoCache[typ] + if !ok { + si = buildStructInfo(typ) + structInfoCache[typ] = si + } + + return si +} + +// buildStructInfo populates a new [structInfo] for the given type. +func buildStructInfo(typ reflect.Type) structInfo { + var out = structInfo{ + Fields: make(map[string][]int), + } + + // Queue holds the indexes of the structs to visit. + // It starts out empty to visit the struct in the type argument. + var queue [][]int = [][]int{{}} + for i := 0; i < len(queue); i++ { + prefix := queue[i] + + // Traverse embedded anonymous types + styp := typ + if len(prefix) > 0 { + styp = styp.FieldByIndex(prefix).Type + } + + // Dereference pointer type. + if styp.Kind() == reflect.Pointer { + styp = styp.Elem() + } + + nf := styp.NumField() + for j := 0; j < nf; j++ { + sf := styp.Field(j) + + // Recurse into anonymous fields. + if sf.Anonymous { + queue = append(queue, append(prefix, sf.Index...)) + continue + } + + name, _, _ := strings.Cut(sf.Tag.Get("json"), ",") + if name == "" || name == "-" { + continue + } + + // Top level fields always take precedence + if _, ok := out.Fields[name]; ok { + continue + } + + out.Fields[name] = append(prefix, sf.Index...) + } + } + + return out +} diff --git a/bundle/config/convert/struct_info_test.go b/bundle/config/convert/struct_info_test.go new file mode 100644 index 0000000000..3079958b2b --- /dev/null +++ b/bundle/config/convert/struct_info_test.go @@ -0,0 +1,89 @@ +package convert + +import ( + "reflect" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestStructInfoPlain(t *testing.T) { + type Tmp struct { + Foo string `json:"foo"` + Bar string `json:"bar,omitempty"` + + // Baz must be skipped. + Baz string `json:""` + + // Qux must be skipped. + Qux string `json:"-"` + } + + si := getStructInfo(reflect.TypeOf(Tmp{})) + assert.Len(t, si.Fields, 2) + assert.Equal(t, []int{0}, si.Fields["foo"]) + assert.Equal(t, []int{1}, si.Fields["bar"]) +} + +func TestStructInfoAnonymousByValue(t *testing.T) { + type Bar struct { + Bar string `json:"bar"` + } + + type Foo struct { + Foo string `json:"foo"` + Bar + } + + type Tmp struct { + Foo + } + + si := getStructInfo(reflect.TypeOf(Tmp{})) + assert.Len(t, si.Fields, 2) + assert.Equal(t, []int{0, 0}, si.Fields["foo"]) + assert.Equal(t, []int{0, 1, 0}, si.Fields["bar"]) +} + +func TestStructInfoAnonymousByValuePrecedence(t *testing.T) { + type Bar struct { + Bar string `json:"bar"` + } + + type Foo struct { + Foo string `json:"foo"` + Bar + } + + type Tmp struct { + // "foo" comes from [Foo]. + Foo + // "bar" comes from [Bar] directly, not through [Foo]. + Bar + } + + si := getStructInfo(reflect.TypeOf(Tmp{})) + assert.Len(t, si.Fields, 2) + assert.Equal(t, []int{0, 0}, si.Fields["foo"]) + assert.Equal(t, []int{1, 0}, si.Fields["bar"]) +} + +func TestStructInfoAnonymousByPointer(t *testing.T) { + type Bar struct { + Bar string `json:"bar"` + } + + type Foo struct { + Foo string `json:"foo"` + *Bar + } + + type Tmp struct { + *Foo + } + + si := getStructInfo(reflect.TypeOf(Tmp{})) + assert.Len(t, si.Fields, 2) + assert.Equal(t, []int{0, 0}, si.Fields["foo"]) + assert.Equal(t, []int{0, 1, 0}, si.Fields["bar"]) +} diff --git a/bundle/config/convert/to_typed.go b/bundle/config/convert/to_typed.go new file mode 100644 index 0000000000..ba975baf75 --- /dev/null +++ b/bundle/config/convert/to_typed.go @@ -0,0 +1,178 @@ +package convert + +import ( + "fmt" + "reflect" + + "github.com/databricks/cli/libs/config" +) + +func ToTyped(dst any, src config.Value) error { + dstv := reflect.ValueOf(dst) + + // Dereference pointer if necessary + for dstv.Kind() == reflect.Pointer { + if dstv.IsNil() { + dstv.Set(reflect.New(dstv.Type().Elem())) + } + dstv = dstv.Elem() + } + + // Verify that vv is settable. + if !dstv.CanSet() { + panic("cannot set destination value") + } + + switch dstv.Kind() { + case reflect.Struct: + return toTypedStruct(dstv, src) + case reflect.Map: + return toTypedMap(dstv, src) + case reflect.Slice: + return toTypedSlice(dstv, src) + case reflect.String: + return toTypedString(dstv, src) + case reflect.Bool: + return toTypedBool(dstv, src) + case reflect.Int, reflect.Int32, reflect.Int64: + return toTypedInt(dstv, src) + case reflect.Float32, reflect.Float64: + return toTypedFloat(dstv, src) + default: + return fmt.Errorf("unsupported type: %s", dstv.Kind()) + } +} + +func toTypedStruct(dst reflect.Value, src config.Value) error { + switch src.Kind() { + case config.KindMap: + info := getStructInfo(dst.Type()) + for k, v := range src.MustMap() { + index, ok := info.Fields[k] + if !ok { + // unknown field + continue + } + + // Create intermediate structs embedded as pointer types. + // Code inspired by [reflect.FieldByIndex] implementation. + f := dst + for i, x := range index { + if i > 0 { + if f.Kind() == reflect.Pointer { + if f.IsNil() { + f.Set(reflect.New(f.Type().Elem())) + } + f = f.Elem() + } + } + f = f.Field(x) + } + + err := ToTyped(f.Addr().Interface(), v) + if err != nil { + return err + } + } + + return nil + default: + return fmt.Errorf("todo") + } +} + +func toTypedMap(dst reflect.Value, src config.Value) error { + switch src.Kind() { + case config.KindMap: + m := src.MustMap() + + // Always overwrite. + dst.Set(reflect.MakeMapWithSize(dst.Type(), len(m))) + for k, v := range m { + kv := reflect.ValueOf(k) + vv := reflect.New(dst.Type().Elem()) + err := ToTyped(vv.Interface(), v) + if err != nil { + return err + } + dst.SetMapIndex(kv, vv.Elem()) + } + return nil + default: + return fmt.Errorf("todo") + } +} + +func toTypedSlice(dst reflect.Value, src config.Value) error { + switch src.Kind() { + case config.KindSequence: + seq := src.MustSequence() + + // Always overwrite. + dst.Set(reflect.MakeSlice(dst.Type(), len(seq), len(seq))) + for i := range seq { + err := ToTyped(dst.Index(i).Addr().Interface(), seq[i]) + if err != nil { + return err + } + } + return nil + default: + return fmt.Errorf("todo") + } +} + +func toTypedString(dst reflect.Value, src config.Value) error { + switch src.Kind() { + case config.KindString: + dst.SetString(src.MustString()) + return nil + default: + return fmt.Errorf("todo") + } +} + +func toTypedBool(dst reflect.Value, src config.Value) error { + switch src.Kind() { + case config.KindBool: + dst.SetBool(src.MustBool()) + return nil + case config.KindString: + // See https://github.com/go-yaml/yaml/blob/f6f7691b1fdeb513f56608cd2c32c51f8194bf51/decode.go#L684-L693. + switch src.MustString() { + case "y", "Y", "yes", "Yes", "YES", "on", "On", "ON": + dst.SetBool(true) + return nil + case "n", "N", "no", "No", "NO", "off", "Off", "OFF": + dst.SetBool(false) + return nil + } + return fmt.Errorf("todo: work with variables") + default: + return fmt.Errorf("todo") + } +} + +func toTypedInt(dst reflect.Value, src config.Value) error { + switch src.Kind() { + case config.KindInt: + dst.SetInt(src.MustInt()) + return nil + case config.KindString: + return fmt.Errorf("todo: work with variables") + default: + return fmt.Errorf("todo") + } +} + +func toTypedFloat(dst reflect.Value, src config.Value) error { + switch src.Kind() { + case config.KindFloat: + dst.SetFloat(src.MustFloat()) + return nil + case config.KindString: + return fmt.Errorf("todo: work with variables") + default: + return fmt.Errorf("todo") + } +} diff --git a/bundle/config/convert/to_typed_test.go b/bundle/config/convert/to_typed_test.go new file mode 100644 index 0000000000..3de0f310de --- /dev/null +++ b/bundle/config/convert/to_typed_test.go @@ -0,0 +1,336 @@ +package convert + +import ( + "testing" + + "github.com/databricks/cli/libs/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestToTypedStruct(t *testing.T) { + type Tmp struct { + Foo string `json:"foo"` + Bar string `json:"bar,omitempty"` + + // Baz must be skipped. + Baz string `json:""` + + // Qux must be skipped. + Qux string `json:"-"` + } + + var out Tmp + v := config.V(map[string]config.Value{ + "foo": config.V("bar"), + "bar": config.V("baz"), + }) + + err := ToTyped(&out, v) + require.NoError(t, err) + assert.Equal(t, "bar", out.Foo) + assert.Equal(t, "baz", out.Bar) +} + +func TestToTypedStructOverwrite(t *testing.T) { + type Tmp struct { + Foo string `json:"foo"` + Bar string `json:"bar,omitempty"` + + // Baz must be skipped. + Baz string `json:""` + + // Qux must be skipped. + Qux string `json:"-"` + } + + var out = Tmp{ + Foo: "baz", + Bar: "qux", + } + v := config.V(map[string]config.Value{ + "foo": config.V("bar"), + "bar": config.V("baz"), + }) + + err := ToTyped(&out, v) + require.NoError(t, err) + assert.Equal(t, "bar", out.Foo) + assert.Equal(t, "baz", out.Bar) +} + +func TestToTypedStructAnonymousByValue(t *testing.T) { + type Bar struct { + Bar string `json:"bar"` + } + + type Foo struct { + Foo string `json:"foo"` + Bar + } + + type Tmp struct { + Foo + } + + var out Tmp + v := config.V(map[string]config.Value{ + "foo": config.V("bar"), + "bar": config.V("baz"), + }) + + err := ToTyped(&out, v) + require.NoError(t, err) + assert.Equal(t, "bar", out.Foo.Foo) + assert.Equal(t, "baz", out.Foo.Bar.Bar) +} + +func TestToTypedStructAnonymousByPointer(t *testing.T) { + type Bar struct { + Bar string `json:"bar"` + } + + type Foo struct { + Foo string `json:"foo"` + *Bar + } + + type Tmp struct { + *Foo + } + + var out Tmp + v := config.V(map[string]config.Value{ + "foo": config.V("bar"), + "bar": config.V("baz"), + }) + + err := ToTyped(&out, v) + require.NoError(t, err) + assert.Equal(t, "bar", out.Foo.Foo) + assert.Equal(t, "baz", out.Foo.Bar.Bar) +} + +func TestToTypedMap(t *testing.T) { + var out = map[string]string{} + + v := config.V(map[string]config.Value{ + "key": config.V("value"), + }) + + err := ToTyped(&out, v) + require.NoError(t, err) + assert.Len(t, out, 1) + assert.Equal(t, "value", out["key"]) +} + +func TestToTypedMapOverwrite(t *testing.T) { + var out = map[string]string{ + "foo": "bar", + } + + v := config.V(map[string]config.Value{ + "bar": config.V("qux"), + }) + + err := ToTyped(&out, v) + require.NoError(t, err) + assert.Len(t, out, 1) + assert.Equal(t, "qux", out["bar"]) +} + +func TestToTypedMapWithPointerElement(t *testing.T) { + var out map[string]*string + + v := config.V(map[string]config.Value{ + "key": config.V("value"), + }) + + err := ToTyped(&out, v) + require.NoError(t, err) + assert.Len(t, out, 1) + assert.Equal(t, "value", *out["key"]) +} + +func TestToTypedSlice(t *testing.T) { + var out []string + + v := config.V([]config.Value{ + config.V("foo"), + config.V("bar"), + }) + + err := ToTyped(&out, v) + require.NoError(t, err) + assert.Len(t, out, 2) + assert.Equal(t, "foo", out[0]) + assert.Equal(t, "bar", out[1]) +} + +func TestToTypedSliceOverwrite(t *testing.T) { + var out = []string{"qux"} + + v := config.V([]config.Value{ + config.V("foo"), + config.V("bar"), + }) + + err := ToTyped(&out, v) + require.NoError(t, err) + assert.Len(t, out, 2) + assert.Equal(t, "foo", out[0]) + assert.Equal(t, "bar", out[1]) +} + +func TestToTypedSliceWithPointerElement(t *testing.T) { + var out []*string + + v := config.V([]config.Value{ + config.V("foo"), + config.V("bar"), + }) + + err := ToTyped(&out, v) + require.NoError(t, err) + assert.Len(t, out, 2) + assert.Equal(t, "foo", *out[0]) + assert.Equal(t, "bar", *out[1]) +} + +func TestToTypedString(t *testing.T) { + var out string + err := ToTyped(&out, config.V("foo")) + require.NoError(t, err) + assert.Equal(t, "foo", out) +} + +func TestToTypedStringOverwrite(t *testing.T) { + var out string = "bar" + err := ToTyped(&out, config.V("foo")) + require.NoError(t, err) + assert.Equal(t, "foo", out) +} + +func TestToTypedBool(t *testing.T) { + var out bool + err := ToTyped(&out, config.V(true)) + require.NoError(t, err) + assert.Equal(t, true, out) +} + +func TestToTypedBoolOverwrite(t *testing.T) { + var out bool = true + err := ToTyped(&out, config.V(false)) + require.NoError(t, err) + assert.Equal(t, false, out) +} + +func TestToTypedBoolFromString(t *testing.T) { + var out bool + + // True-ish + for _, v := range []string{"y", "yes", "on"} { + err := ToTyped(&out, config.V(v)) + require.NoError(t, err) + assert.Equal(t, true, out) + } + + // False-ish + for _, v := range []string{"n", "no", "off"} { + err := ToTyped(&out, config.V(v)) + require.NoError(t, err) + assert.Equal(t, false, out) + } + + // Other + err := ToTyped(&out, config.V("${var.foo}")) + require.Error(t, err) +} + +func TestToTypedInt(t *testing.T) { + var out int + err := ToTyped(&out, config.V(1234)) + require.NoError(t, err) + assert.Equal(t, int(1234), out) +} + +func TestToTypedInt32(t *testing.T) { + var out32 int32 + err := ToTyped(&out32, config.V(1235)) + require.NoError(t, err) + assert.Equal(t, int32(1235), out32) +} + +func TestToTypedInt64(t *testing.T) { + var out64 int64 + err := ToTyped(&out64, config.V(1236)) + require.NoError(t, err) + assert.Equal(t, int64(1236), out64) +} + +func TestToTypedIntOverwrite(t *testing.T) { + var out int = 123 + err := ToTyped(&out, config.V(1234)) + require.NoError(t, err) + assert.Equal(t, int(1234), out) +} + +func TestToTypedInt32Overwrite(t *testing.T) { + var out32 int32 = 123 + err := ToTyped(&out32, config.V(1234)) + require.NoError(t, err) + assert.Equal(t, int32(1234), out32) +} + +func TestToTypedInt64Overwrite(t *testing.T) { + var out64 int64 = 123 + err := ToTyped(&out64, config.V(1234)) + require.NoError(t, err) + assert.Equal(t, int64(1234), out64) +} + +func TestToTypedIntFromString(t *testing.T) { + var out int + err := ToTyped(&out, config.V("abc")) + require.Error(t, err) +} + +func TestToTypedFloat32(t *testing.T) { + var out float32 + err := ToTyped(&out, config.V(float32(1.0))) + require.NoError(t, err) + assert.Equal(t, float32(1.0), out) +} + +func TestToTypedFloat64(t *testing.T) { + var out float64 + err := ToTyped(&out, config.V(float64(1.0))) + require.NoError(t, err) + assert.Equal(t, float64(1.0), out) +} + +func TestToTypedFloat32Overwrite(t *testing.T) { + var out float32 = 1.0 + err := ToTyped(&out, config.V(float32(2.0))) + require.NoError(t, err) + assert.Equal(t, float32(2.0), out) +} + +func TestToTypedFloat64Overwrite(t *testing.T) { + var out float64 = 1.0 + err := ToTyped(&out, config.V(float64(2.0))) + require.NoError(t, err) + assert.Equal(t, float64(2.0), out) +} + +func TestToTypedFloat32FromString(t *testing.T) { + var out float32 + err := ToTyped(&out, config.V("abc")) + require.Error(t, err) +} + +func TestToTypedFloat64FromString(t *testing.T) { + var out float64 + err := ToTyped(&out, config.V("abc")) + require.Error(t, err) +} diff --git a/libs/config/value.go b/libs/config/value.go index 00282e1ee3..c77f8147dc 100644 --- a/libs/config/value.go +++ b/libs/config/value.go @@ -1,6 +1,9 @@ package config -import "fmt" +import ( + "fmt" + "time" +) type Value struct { v any @@ -18,6 +21,14 @@ var NilValue = Value{ k: KindNil, } +// V constructs a new Value with the given value. +func V(v any) Value { + return Value{ + v: v, + k: kindOf(v), + } +} + // NewValue constructs a new Value with the given value and location. func NewValue(v any, loc Location) Value { return Value{ @@ -116,3 +127,47 @@ func (v Value) MarkAnchor() Value { func (v Value) IsAnchor() bool { return v.anchor } + +func (v Value) MustMap() map[string]Value { + return v.v.(map[string]Value) +} + +func (v Value) MustSequence() []Value { + return v.v.([]Value) +} + +func (v Value) MustString() string { + return v.v.(string) +} + +func (v Value) MustBool() bool { + return v.v.(bool) +} + +func (v Value) MustInt() int64 { + switch vv := v.v.(type) { + case int: + return int64(vv) + case int32: + return int64(vv) + case int64: + return int64(vv) + default: + panic("not an int") + } +} + +func (v Value) MustFloat() float64 { + switch vv := v.v.(type) { + case float32: + return float64(vv) + case float64: + return float64(vv) + default: + panic("not a float") + } +} + +func (v Value) MustTime() time.Time { + return v.v.(time.Time) +} From 5ea19daa5a10549e4788aacb11361bb88821bd9b Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 20 Oct 2023 15:59:02 +0200 Subject: [PATCH 3/8] Comments --- bundle/config/convert/struct_info.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/bundle/config/convert/struct_info.go b/bundle/config/convert/struct_info.go index 82cf0001f1..367b9ecdc4 100644 --- a/bundle/config/convert/struct_info.go +++ b/bundle/config/convert/struct_info.go @@ -42,12 +42,12 @@ func buildStructInfo(typ reflect.Type) structInfo { } // Queue holds the indexes of the structs to visit. - // It starts out empty to visit the struct in the type argument. + // It is initialized with a single empty slice to visit the top level struct. var queue [][]int = [][]int{{}} for i := 0; i < len(queue); i++ { prefix := queue[i] - // Traverse embedded anonymous types + // Traverse embedded anonymous types (if prefix is non-empty). styp := typ if len(prefix) > 0 { styp = styp.FieldByIndex(prefix).Type @@ -73,7 +73,8 @@ func buildStructInfo(typ reflect.Type) structInfo { continue } - // Top level fields always take precedence + // Top level fields always take precedence. + // Therefore, if it is already set, we ignore it. if _, ok := out.Fields[name]; ok { continue } From 8ea37fbe614fb4be80ddc87b9f15042e5419052a Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 23 Oct 2023 10:37:38 +0200 Subject: [PATCH 4/8] Move convert package under config --- {bundle => libs}/config/convert/error.go | 0 {bundle => libs}/config/convert/struct_info.go | 0 {bundle => libs}/config/convert/struct_info_test.go | 0 {bundle => libs}/config/convert/to_typed.go | 0 {bundle => libs}/config/convert/to_typed_test.go | 0 5 files changed, 0 insertions(+), 0 deletions(-) rename {bundle => libs}/config/convert/error.go (100%) rename {bundle => libs}/config/convert/struct_info.go (100%) rename {bundle => libs}/config/convert/struct_info_test.go (100%) rename {bundle => libs}/config/convert/to_typed.go (100%) rename {bundle => libs}/config/convert/to_typed_test.go (100%) diff --git a/bundle/config/convert/error.go b/libs/config/convert/error.go similarity index 100% rename from bundle/config/convert/error.go rename to libs/config/convert/error.go diff --git a/bundle/config/convert/struct_info.go b/libs/config/convert/struct_info.go similarity index 100% rename from bundle/config/convert/struct_info.go rename to libs/config/convert/struct_info.go diff --git a/bundle/config/convert/struct_info_test.go b/libs/config/convert/struct_info_test.go similarity index 100% rename from bundle/config/convert/struct_info_test.go rename to libs/config/convert/struct_info_test.go diff --git a/bundle/config/convert/to_typed.go b/libs/config/convert/to_typed.go similarity index 100% rename from bundle/config/convert/to_typed.go rename to libs/config/convert/to_typed.go diff --git a/bundle/config/convert/to_typed_test.go b/libs/config/convert/to_typed_test.go similarity index 100% rename from bundle/config/convert/to_typed_test.go rename to libs/config/convert/to_typed_test.go From ba8c99c8fa7e26e6296288bfff4cc54143803c7a Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 23 Oct 2023 13:24:42 +0200 Subject: [PATCH 5/8] Add conversions and errors --- libs/config/convert/error.go | 10 ++++- libs/config/convert/to_typed.go | 62 ++++++++++++++++++++++------ libs/config/convert/to_typed_test.go | 48 +++++++++++++++++++-- libs/config/kind.go | 23 +++++++++++ 4 files changed, 126 insertions(+), 17 deletions(-) diff --git a/libs/config/convert/error.go b/libs/config/convert/error.go index e651da1309..b55668d67f 100644 --- a/libs/config/convert/error.go +++ b/libs/config/convert/error.go @@ -1,8 +1,16 @@ package convert -import "github.com/databricks/cli/libs/config" +import ( + "fmt" + + "github.com/databricks/cli/libs/config" +) type TypeError struct { value config.Value msg string } + +func (e TypeError) Error() string { + return fmt.Sprintf("%s: %s", e.value.Location(), e.msg) +} diff --git a/libs/config/convert/to_typed.go b/libs/config/convert/to_typed.go index ba975baf75..73a31e0772 100644 --- a/libs/config/convert/to_typed.go +++ b/libs/config/convert/to_typed.go @@ -3,6 +3,7 @@ package convert import ( "fmt" "reflect" + "strconv" "github.com/databricks/cli/libs/config" ) @@ -77,7 +78,10 @@ func toTypedStruct(dst reflect.Value, src config.Value) error { return nil default: - return fmt.Errorf("todo") + return TypeError{ + value: src, + msg: fmt.Sprintf("expected a map, found a %s", src.Kind()), + } } } @@ -99,7 +103,10 @@ func toTypedMap(dst reflect.Value, src config.Value) error { } return nil default: - return fmt.Errorf("todo") + return TypeError{ + value: src, + msg: fmt.Sprintf("expected a map, found a %s", src.Kind()), + } } } @@ -118,7 +125,10 @@ func toTypedSlice(dst reflect.Value, src config.Value) error { } return nil default: - return fmt.Errorf("todo") + return TypeError{ + value: src, + msg: fmt.Sprintf("expected a sequence, found a %s", src.Kind()), + } } } @@ -127,8 +137,20 @@ func toTypedString(dst reflect.Value, src config.Value) error { case config.KindString: dst.SetString(src.MustString()) return nil + case config.KindBool: + dst.SetString(strconv.FormatBool(src.MustBool())) + return nil + case config.KindInt: + dst.SetString(strconv.FormatInt(src.MustInt(), 10)) + return nil + case config.KindFloat: + dst.SetString(strconv.FormatFloat(src.MustFloat(), 'f', -1, 64)) + return nil default: - return fmt.Errorf("todo") + return TypeError{ + value: src, + msg: fmt.Sprintf("expected a string, found a %s", src.Kind()), + } } } @@ -147,9 +169,11 @@ func toTypedBool(dst reflect.Value, src config.Value) error { dst.SetBool(false) return nil } - return fmt.Errorf("todo: work with variables") - default: - return fmt.Errorf("todo") + } + + return TypeError{ + value: src, + msg: fmt.Sprintf("expected a boolean, found a %s", src.Kind()), } } @@ -159,9 +183,15 @@ func toTypedInt(dst reflect.Value, src config.Value) error { dst.SetInt(src.MustInt()) return nil case config.KindString: - return fmt.Errorf("todo: work with variables") - default: - return fmt.Errorf("todo") + if i64, err := strconv.ParseInt(src.MustString(), 10, 64); err == nil { + dst.SetInt(i64) + return nil + } + } + + return TypeError{ + value: src, + msg: fmt.Sprintf("expected an int, found a %s", src.Kind()), } } @@ -171,8 +201,14 @@ func toTypedFloat(dst reflect.Value, src config.Value) error { dst.SetFloat(src.MustFloat()) return nil case config.KindString: - return fmt.Errorf("todo: work with variables") - default: - return fmt.Errorf("todo") + if f64, err := strconv.ParseFloat(src.MustString(), 64); err == nil { + dst.SetFloat(f64) + return nil + } + } + + return TypeError{ + value: src, + msg: fmt.Sprintf("expected a float, found a %s", src.Kind()), } } diff --git a/libs/config/convert/to_typed_test.go b/libs/config/convert/to_typed_test.go index 3de0f310de..162718872b 100644 --- a/libs/config/convert/to_typed_test.go +++ b/libs/config/convert/to_typed_test.go @@ -211,6 +211,27 @@ func TestToTypedStringOverwrite(t *testing.T) { assert.Equal(t, "foo", out) } +func TestToTypedStringFromBool(t *testing.T) { + var out string + err := ToTyped(&out, config.V(true)) + require.NoError(t, err) + assert.Equal(t, "true", out) +} + +func TestToTypedStringFromInt(t *testing.T) { + var out string + err := ToTyped(&out, config.V(123)) + require.NoError(t, err) + assert.Equal(t, "123", out) +} + +func TestToTypedStringFromFloat(t *testing.T) { + var out string + err := ToTyped(&out, config.V(1.2)) + require.NoError(t, err) + assert.Equal(t, "1.2", out) +} + func TestToTypedBool(t *testing.T) { var out bool err := ToTyped(&out, config.V(true)) @@ -289,12 +310,19 @@ func TestToTypedInt64Overwrite(t *testing.T) { assert.Equal(t, int64(1234), out64) } -func TestToTypedIntFromString(t *testing.T) { +func TestToTypedIntFromStringError(t *testing.T) { var out int err := ToTyped(&out, config.V("abc")) require.Error(t, err) } +func TestToTypedIntFromStringInt(t *testing.T) { + var out int + err := ToTyped(&out, config.V("123")) + require.NoError(t, err) + assert.Equal(t, int(123), out) +} + func TestToTypedFloat32(t *testing.T) { var out float32 err := ToTyped(&out, config.V(float32(1.0))) @@ -323,14 +351,28 @@ func TestToTypedFloat64Overwrite(t *testing.T) { assert.Equal(t, float64(2.0), out) } -func TestToTypedFloat32FromString(t *testing.T) { +func TestToTypedFloat32FromStringError(t *testing.T) { var out float32 err := ToTyped(&out, config.V("abc")) require.Error(t, err) } -func TestToTypedFloat64FromString(t *testing.T) { +func TestToTypedFloat64FromStringError(t *testing.T) { var out float64 err := ToTyped(&out, config.V("abc")) require.Error(t, err) } + +func TestToTypedFloat32FromString(t *testing.T) { + var out float32 + err := ToTyped(&out, config.V("1.2")) + require.NoError(t, err) + assert.Equal(t, float32(1.2), out) +} + +func TestToTypedFloat64FromString(t *testing.T) { + var out float64 + err := ToTyped(&out, config.V("1.2")) + require.NoError(t, err) + assert.Equal(t, float64(1.2), out) +} diff --git a/libs/config/kind.go b/libs/config/kind.go index f3017a1dbe..5ed1a6650b 100644 --- a/libs/config/kind.go +++ b/libs/config/kind.go @@ -39,3 +39,26 @@ func kindOf(v any) Kind { panic("not handled") } } + +func (k Kind) String() string { + switch k { + case KindMap: + return "map" + case KindSequence: + return "sequence" + case KindNil: + return "nil" + case KindString: + return "string" + case KindBool: + return "bool" + case KindInt: + return "int" + case KindFloat: + return "float" + case KindTime: + return "time" + default: + return "invalid" + } +} From 6e34e509e81b4d2b4c53735f4cdf2d1929232f21 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 23 Oct 2023 13:37:46 +0200 Subject: [PATCH 6/8] Move default case out of switch scope --- libs/config/convert/to_typed.go | 44 ++++++++++++++++----------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/libs/config/convert/to_typed.go b/libs/config/convert/to_typed.go index 73a31e0772..14f2591b34 100644 --- a/libs/config/convert/to_typed.go +++ b/libs/config/convert/to_typed.go @@ -39,9 +39,9 @@ func ToTyped(dst any, src config.Value) error { return toTypedInt(dstv, src) case reflect.Float32, reflect.Float64: return toTypedFloat(dstv, src) - default: - return fmt.Errorf("unsupported type: %s", dstv.Kind()) } + + return fmt.Errorf("unsupported type: %s", dstv.Kind()) } func toTypedStruct(dst reflect.Value, src config.Value) error { @@ -77,11 +77,11 @@ func toTypedStruct(dst reflect.Value, src config.Value) error { } return nil - default: - return TypeError{ - value: src, - msg: fmt.Sprintf("expected a map, found a %s", src.Kind()), - } + } + + return TypeError{ + value: src, + msg: fmt.Sprintf("expected a map, found a %s", src.Kind()), } } @@ -102,11 +102,11 @@ func toTypedMap(dst reflect.Value, src config.Value) error { dst.SetMapIndex(kv, vv.Elem()) } return nil - default: - return TypeError{ - value: src, - msg: fmt.Sprintf("expected a map, found a %s", src.Kind()), - } + } + + return TypeError{ + value: src, + msg: fmt.Sprintf("expected a map, found a %s", src.Kind()), } } @@ -124,11 +124,11 @@ func toTypedSlice(dst reflect.Value, src config.Value) error { } } return nil - default: - return TypeError{ - value: src, - msg: fmt.Sprintf("expected a sequence, found a %s", src.Kind()), - } + } + + return TypeError{ + value: src, + msg: fmt.Sprintf("expected a sequence, found a %s", src.Kind()), } } @@ -146,11 +146,11 @@ func toTypedString(dst reflect.Value, src config.Value) error { case config.KindFloat: dst.SetString(strconv.FormatFloat(src.MustFloat(), 'f', -1, 64)) return nil - default: - return TypeError{ - value: src, - msg: fmt.Sprintf("expected a string, found a %s", src.Kind()), - } + } + + return TypeError{ + value: src, + msg: fmt.Sprintf("expected a string, found a %s", src.Kind()), } } From f0bac95bc5f8ea5ba7da6130661bec0c455ad883 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 23 Oct 2023 13:51:43 +0200 Subject: [PATCH 7/8] Convert nil to struct/map/slice --- libs/config/convert/to_typed.go | 9 +++++ libs/config/convert/to_typed_test.go | 52 ++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/libs/config/convert/to_typed.go b/libs/config/convert/to_typed.go index 14f2591b34..55602a69e1 100644 --- a/libs/config/convert/to_typed.go +++ b/libs/config/convert/to_typed.go @@ -76,6 +76,9 @@ func toTypedStruct(dst reflect.Value, src config.Value) error { } } + return nil + case config.KindNil: + dst.SetZero() return nil } @@ -102,6 +105,9 @@ func toTypedMap(dst reflect.Value, src config.Value) error { dst.SetMapIndex(kv, vv.Elem()) } return nil + case config.KindNil: + dst.SetZero() + return nil } return TypeError{ @@ -124,6 +130,9 @@ func toTypedSlice(dst reflect.Value, src config.Value) error { } } return nil + case config.KindNil: + dst.SetZero() + return nil } return TypeError{ diff --git a/libs/config/convert/to_typed_test.go b/libs/config/convert/to_typed_test.go index 162718872b..26e17dcce5 100644 --- a/libs/config/convert/to_typed_test.go +++ b/libs/config/convert/to_typed_test.go @@ -111,6 +111,28 @@ func TestToTypedStructAnonymousByPointer(t *testing.T) { assert.Equal(t, "baz", out.Foo.Bar.Bar) } +func TestToTypedStructNil(t *testing.T) { + type Tmp struct { + Foo string `json:"foo"` + } + + var out = Tmp{} + err := ToTyped(&out, config.NilValue) + require.NoError(t, err) + assert.Equal(t, Tmp{}, out) +} + +func TestToTypedStructNilOverwrite(t *testing.T) { + type Tmp struct { + Foo string `json:"foo"` + } + + var out = Tmp{"bar"} + err := ToTyped(&out, config.NilValue) + require.NoError(t, err) + assert.Equal(t, Tmp{}, out) +} + func TestToTypedMap(t *testing.T) { var out = map[string]string{} @@ -152,6 +174,22 @@ func TestToTypedMapWithPointerElement(t *testing.T) { assert.Equal(t, "value", *out["key"]) } +func TestToTypedMapNil(t *testing.T) { + var out = map[string]string{} + err := ToTyped(&out, config.NilValue) + require.NoError(t, err) + assert.Nil(t, out) +} + +func TestToTypedMapNilOverwrite(t *testing.T) { + var out = map[string]string{ + "foo": "bar", + } + err := ToTyped(&out, config.NilValue) + require.NoError(t, err) + assert.Nil(t, out) +} + func TestToTypedSlice(t *testing.T) { var out []string @@ -197,6 +235,20 @@ func TestToTypedSliceWithPointerElement(t *testing.T) { assert.Equal(t, "bar", *out[1]) } +func TestToTypedSliceNil(t *testing.T) { + var out []string + err := ToTyped(&out, config.NilValue) + require.NoError(t, err) + assert.Nil(t, out) +} + +func TestToTypedSliceNilOverwrite(t *testing.T) { + var out = []string{"foo"} + err := ToTyped(&out, config.NilValue) + require.NoError(t, err) + assert.Nil(t, out) +} + func TestToTypedString(t *testing.T) { var out string err := ToTyped(&out, config.V("foo")) From 3dab19319fbd2a62f6be89b918288eb77c96b531 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Tue, 24 Oct 2023 13:06:58 +0200 Subject: [PATCH 8/8] Update comment --- libs/config/convert/to_typed.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/config/convert/to_typed.go b/libs/config/convert/to_typed.go index 55602a69e1..9915d30a6e 100644 --- a/libs/config/convert/to_typed.go +++ b/libs/config/convert/to_typed.go @@ -51,7 +51,8 @@ func toTypedStruct(dst reflect.Value, src config.Value) error { for k, v := range src.MustMap() { index, ok := info.Fields[k] if !ok { - // unknown field + // Ignore unknown fields. + // A warning will be printed later. See PR #904. continue }