From aa0c715930a78ea6b5fcc71c722ae8e426360a0f Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Tue, 13 Feb 2024 15:12:19 +0100 Subject: [PATCH] Retain partially valid structs in `convert.Normalize` (#1203) ## Changes Before this change, any error in a subtree would cause the entire subtree to be dropped from the output. This is not ideal when debugging, so instead we drop only the values that cannot be normalized. Note that this doesn't change behavior if the caller is properly checking the returned diagnostics for errors. Note: this includes a change to use `dyn.InvalidValue` as opposed to `dyn.NilValue` when returning errors. ## Tests Added unit tests for the case where nested struct, map, or slice elements contain an error. --- libs/dyn/convert/from_typed.go | 22 +++--- libs/dyn/convert/normalize.go | 28 ++++---- libs/dyn/convert/normalize_test.go | 106 +++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 25 deletions(-) diff --git a/libs/dyn/convert/from_typed.go b/libs/dyn/convert/from_typed.go index bd6b6367..6dcca2b8 100644 --- a/libs/dyn/convert/from_typed.go +++ b/libs/dyn/convert/from_typed.go @@ -59,7 +59,7 @@ func fromTyped(src any, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, return fromTypedFloat(srcv, ref, options...) } - return dyn.NilValue, fmt.Errorf("unsupported type: %s", srcv.Kind()) + return dyn.InvalidValue, fmt.Errorf("unsupported type: %s", srcv.Kind()) } func fromTypedStruct(src reflect.Value, ref dyn.Value) (dyn.Value, error) { @@ -67,7 +67,7 @@ func fromTypedStruct(src reflect.Value, ref dyn.Value) (dyn.Value, error) { switch ref.Kind() { case dyn.KindMap, dyn.KindNil: default: - return dyn.Value{}, fmt.Errorf("unhandled type: %s", ref.Kind()) + return dyn.InvalidValue, fmt.Errorf("unhandled type: %s", ref.Kind()) } out := make(map[string]dyn.Value) @@ -76,7 +76,7 @@ func fromTypedStruct(src reflect.Value, ref dyn.Value) (dyn.Value, error) { // Convert the field taking into account the reference value (may be equal to config.NilValue). nv, err := fromTyped(v.Interface(), ref.Get(k)) if err != nil { - return dyn.Value{}, err + return dyn.InvalidValue, err } if nv != dyn.NilValue { @@ -92,7 +92,7 @@ func fromTypedMap(src reflect.Value, ref dyn.Value) (dyn.Value, error) { switch ref.Kind() { case dyn.KindMap, dyn.KindNil: default: - return dyn.Value{}, fmt.Errorf("unhandled type: %s", ref.Kind()) + return dyn.InvalidValue, fmt.Errorf("unhandled type: %s", ref.Kind()) } // Return nil if the map is nil. @@ -109,7 +109,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(), ref.Get(k), includeZeroValues) if err != nil { - return dyn.Value{}, err + return dyn.InvalidValue, err } // Every entry is represented, even if it is a nil. @@ -125,7 +125,7 @@ func fromTypedSlice(src reflect.Value, ref dyn.Value) (dyn.Value, error) { switch ref.Kind() { case dyn.KindSequence, dyn.KindNil: default: - return dyn.Value{}, fmt.Errorf("unhandled type: %s", ref.Kind()) + return dyn.InvalidValue, fmt.Errorf("unhandled type: %s", ref.Kind()) } // Return nil if the slice is nil. @@ -140,7 +140,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(), ref.Index(i), includeZeroValues) if err != nil { - return dyn.Value{}, err + return dyn.InvalidValue, err } out[i] = nv @@ -167,7 +167,7 @@ func fromTypedString(src reflect.Value, ref dyn.Value, options ...fromTypedOptio return dyn.V(src.String()), nil } - return dyn.Value{}, fmt.Errorf("unhandled type: %s", ref.Kind()) + return dyn.InvalidValue, fmt.Errorf("unhandled type: %s", ref.Kind()) } func fromTypedBool(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) { @@ -187,7 +187,7 @@ func fromTypedBool(src reflect.Value, ref dyn.Value, options ...fromTypedOptions return dyn.V(src.Bool()), nil } - return dyn.Value{}, fmt.Errorf("unhandled type: %s", ref.Kind()) + return dyn.InvalidValue, fmt.Errorf("unhandled type: %s", ref.Kind()) } func fromTypedInt(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) { @@ -207,7 +207,7 @@ func fromTypedInt(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) return dyn.V(src.Int()), nil } - return dyn.Value{}, fmt.Errorf("unhandled type: %s", ref.Kind()) + return dyn.InvalidValue, fmt.Errorf("unhandled type: %s", ref.Kind()) } func fromTypedFloat(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) { @@ -227,5 +227,5 @@ func fromTypedFloat(src reflect.Value, ref dyn.Value, options ...fromTypedOption return dyn.V(src.Float()), nil } - return dyn.Value{}, fmt.Errorf("unhandled type: %s", ref.Kind()) + return dyn.InvalidValue, fmt.Errorf("unhandled type: %s", ref.Kind()) } diff --git a/libs/dyn/convert/normalize.go b/libs/dyn/convert/normalize.go index 7a652cbc..5595aae1 100644 --- a/libs/dyn/convert/normalize.go +++ b/libs/dyn/convert/normalize.go @@ -35,7 +35,7 @@ func normalizeType(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnostics return normalizeFloat(typ, src) } - return dyn.NilValue, diag.Errorf("unsupported type: %s", typ.Kind()) + return dyn.InvalidValue, diag.Errorf("unsupported type: %s", typ.Kind()) } func typeMismatch(expected dyn.Kind, src dyn.Value) diag.Diagnostic { @@ -69,7 +69,7 @@ func normalizeStruct(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnosti if err != nil { diags = diags.Extend(err) // Skip the element if it cannot be normalized. - if err.HasError() { + if !v.IsValid() { continue } } @@ -82,7 +82,7 @@ func normalizeStruct(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnosti return src, diags } - return dyn.NilValue, diags.Append(typeMismatch(dyn.KindMap, src)) + return dyn.InvalidValue, diags.Append(typeMismatch(dyn.KindMap, src)) } func normalizeMap(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnostics) { @@ -97,7 +97,7 @@ func normalizeMap(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnostics) if err != nil { diags = diags.Extend(err) // Skip the element if it cannot be normalized. - if err.HasError() { + if !v.IsValid() { continue } } @@ -110,7 +110,7 @@ func normalizeMap(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnostics) return src, diags } - return dyn.NilValue, diags.Append(typeMismatch(dyn.KindMap, src)) + return dyn.InvalidValue, diags.Append(typeMismatch(dyn.KindMap, src)) } func normalizeSlice(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnostics) { @@ -125,7 +125,7 @@ func normalizeSlice(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnostic if err != nil { diags = diags.Extend(err) // Skip the element if it cannot be normalized. - if err.HasError() { + if !v.IsValid() { continue } } @@ -138,7 +138,7 @@ func normalizeSlice(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnostic return src, diags } - return dyn.NilValue, diags.Append(typeMismatch(dyn.KindSequence, src)) + return dyn.InvalidValue, diags.Append(typeMismatch(dyn.KindSequence, src)) } func normalizeString(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnostics) { @@ -155,7 +155,7 @@ func normalizeString(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnosti case dyn.KindFloat: out = strconv.FormatFloat(src.MustFloat(), 'f', -1, 64) default: - return dyn.NilValue, diags.Append(typeMismatch(dyn.KindString, src)) + return dyn.InvalidValue, diags.Append(typeMismatch(dyn.KindString, src)) } return dyn.NewValue(out, src.Location()), diags @@ -177,10 +177,10 @@ func normalizeBool(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnostics out = false default: // Cannot interpret as a boolean. - return dyn.NilValue, diags.Append(typeMismatch(dyn.KindBool, src)) + return dyn.InvalidValue, diags.Append(typeMismatch(dyn.KindBool, src)) } default: - return dyn.NilValue, diags.Append(typeMismatch(dyn.KindBool, src)) + return dyn.InvalidValue, diags.Append(typeMismatch(dyn.KindBool, src)) } return dyn.NewValue(out, src.Location()), diags @@ -197,14 +197,14 @@ func normalizeInt(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnostics) var err error out, err = strconv.ParseInt(src.MustString(), 10, 64) if err != nil { - return dyn.NilValue, diags.Append(diag.Diagnostic{ + return dyn.InvalidValue, diags.Append(diag.Diagnostic{ Severity: diag.Error, Summary: fmt.Sprintf("cannot parse %q as an integer", src.MustString()), Location: src.Location(), }) } default: - return dyn.NilValue, diags.Append(typeMismatch(dyn.KindInt, src)) + return dyn.InvalidValue, diags.Append(typeMismatch(dyn.KindInt, src)) } return dyn.NewValue(out, src.Location()), diags @@ -221,14 +221,14 @@ func normalizeFloat(typ reflect.Type, src dyn.Value) (dyn.Value, diag.Diagnostic var err error out, err = strconv.ParseFloat(src.MustString(), 64) if err != nil { - return dyn.NilValue, diags.Append(diag.Diagnostic{ + return dyn.InvalidValue, diags.Append(diag.Diagnostic{ Severity: diag.Error, Summary: fmt.Sprintf("cannot parse %q as a floating point number", src.MustString()), Location: src.Location(), }) } default: - return dyn.NilValue, diags.Append(typeMismatch(dyn.KindFloat, src)) + return dyn.InvalidValue, diags.Append(typeMismatch(dyn.KindFloat, src)) } return dyn.NewValue(out, src.Location()), diags diff --git a/libs/dyn/convert/normalize_test.go b/libs/dyn/convert/normalize_test.go index 13b1ed52..70281615 100644 --- a/libs/dyn/convert/normalize_test.go +++ b/libs/dyn/convert/normalize_test.go @@ -104,6 +104,44 @@ func TestNormalizeStructError(t *testing.T) { }, err[0]) } +func TestNormalizeStructNestedError(t *testing.T) { + type Nested struct { + F1 int `json:"f1"` + F2 int `json:"f2"` + } + type Tmp struct { + Foo Nested `json:"foo"` + Bar Nested `json:"bar"` + } + + var typ Tmp + vin := dyn.V(map[string]dyn.Value{ + "foo": dyn.V(map[string]dyn.Value{ + "f1": dyn.V("error"), + "f2": dyn.V(1), + }), + "bar": dyn.V(map[string]dyn.Value{ + "f1": dyn.V(1), + "f2": dyn.V("error"), + }), + }) + vout, err := Normalize(typ, vin) + assert.Len(t, err, 2) + + // Verify that valid fields are retained. + assert.Equal(t, + dyn.V(map[string]dyn.Value{ + "foo": dyn.V(map[string]dyn.Value{ + "f2": dyn.V(int64(1)), + }), + "bar": dyn.V(map[string]dyn.Value{ + "f1": dyn.V(int64(1)), + }), + }), + vout, + ) +} + func TestNormalizeMap(t *testing.T) { var typ map[string]string vin := dyn.V(map[string]dyn.Value{ @@ -157,6 +195,40 @@ func TestNormalizeMapError(t *testing.T) { }, err[0]) } +func TestNormalizeMapNestedError(t *testing.T) { + type Nested struct { + F1 int `json:"f1"` + F2 int `json:"f2"` + } + + var typ map[string]Nested + vin := dyn.V(map[string]dyn.Value{ + "foo": dyn.V(map[string]dyn.Value{ + "f1": dyn.V("error"), + "f2": dyn.V(1), + }), + "bar": dyn.V(map[string]dyn.Value{ + "f1": dyn.V(1), + "f2": dyn.V("error"), + }), + }) + vout, err := Normalize(typ, vin) + assert.Len(t, err, 2) + + // Verify that valid fields are retained. + assert.Equal(t, + dyn.V(map[string]dyn.Value{ + "foo": dyn.V(map[string]dyn.Value{ + "f2": dyn.V(int64(1)), + }), + "bar": dyn.V(map[string]dyn.Value{ + "f1": dyn.V(int64(1)), + }), + }), + vout, + ) +} + func TestNormalizeSlice(t *testing.T) { var typ []string vin := dyn.V([]dyn.Value{ @@ -209,6 +281,40 @@ func TestNormalizeSliceError(t *testing.T) { }, err[0]) } +func TestNormalizeSliceNestedError(t *testing.T) { + type Nested struct { + F1 int `json:"f1"` + F2 int `json:"f2"` + } + + var typ []Nested + vin := dyn.V([]dyn.Value{ + dyn.V(map[string]dyn.Value{ + "f1": dyn.V("error"), + "f2": dyn.V(1), + }), + dyn.V(map[string]dyn.Value{ + "f1": dyn.V(1), + "f2": dyn.V("error"), + }), + }) + vout, err := Normalize(typ, vin) + assert.Len(t, err, 2) + + // Verify that valid fields are retained. + assert.Equal(t, + dyn.V([]dyn.Value{ + dyn.V(map[string]dyn.Value{ + "f2": dyn.V(int64(1)), + }), + dyn.V(map[string]dyn.Value{ + "f1": dyn.V(int64(1)), + }), + }), + vout, + ) +} + func TestNormalizeString(t *testing.T) { var typ string vin := dyn.V("string")