From dac5f09556875003986832f74829bdbc326e725f Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Tue, 25 Jun 2024 19:10:21 +0530 Subject: [PATCH] Retain location metadata for values in `convert.FromTyped` (#1523) ## Changes There are four different treatments location metadata can receive in the `convert.FromTyped` method. 1. Location metadata is **retained** for maps, structs and slices if the value is **not nil** 2. Location metadata is **lost** for maps, structs and slices if the value is **is nil** 3. Location metadata is **retained** if a scalar type (eg. bool, string etc) does not change. 4. Location metadata is **lost** if the value for a scalar type changes. This PR ensures that location metadata is not lost in any case; that is, it's always preserved. For (2), this serves as a bug fix so that location information is not lost on conversion to and from typed for nil values of complex types (struct, slices, and maps). For (4) this is a change in semantics. For primitive values modified in a `typed` mutator, any references to `.Location()` for computed primitive fields will now return associated YAML location metadata (if any) instead of an empty location. While arguable, these semantics are OK since: 1. Situations like these will be rare. 2. Knowing the YAML location (if any) is better than not knowing the location at all. These locations are typically visible to the user in errors and warnings. ## Tests Unit tests --- libs/dyn/convert/from_typed.go | 34 +++++---- libs/dyn/convert/from_typed_test.go | 109 +++++++++++++++++++++------- 2 files changed, 105 insertions(+), 38 deletions(-) diff --git a/libs/dyn/convert/from_typed.go b/libs/dyn/convert/from_typed.go index af49a07a..258ade4e 100644 --- a/libs/dyn/convert/from_typed.go +++ b/libs/dyn/convert/from_typed.go @@ -42,7 +42,7 @@ func fromTyped(src any, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, // Dereference pointer if necessary for srcv.Kind() == reflect.Pointer { if srcv.IsNil() { - return dyn.NilValue, nil + return dyn.NilValue.WithLocation(ref.Location()), nil } srcv = srcv.Elem() @@ -55,27 +55,35 @@ func fromTyped(src any, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, } } + var v dyn.Value + var err error switch srcv.Kind() { case reflect.Struct: - return fromTypedStruct(srcv, ref, options...) + v, err = fromTypedStruct(srcv, ref, options...) case reflect.Map: - return fromTypedMap(srcv, ref) + v, err = fromTypedMap(srcv, ref) case reflect.Slice: - return fromTypedSlice(srcv, ref) + v, err = fromTypedSlice(srcv, ref) case reflect.String: - return fromTypedString(srcv, ref, options...) + v, err = fromTypedString(srcv, ref, options...) case reflect.Bool: - return fromTypedBool(srcv, ref, options...) + v, err = fromTypedBool(srcv, ref, options...) case reflect.Int, reflect.Int32, reflect.Int64: - return fromTypedInt(srcv, ref, options...) + v, err = fromTypedInt(srcv, ref, options...) case reflect.Float32, reflect.Float64: - return fromTypedFloat(srcv, ref, options...) + v, err = fromTypedFloat(srcv, ref, options...) case reflect.Invalid: // If the value is untyped and not set (e.g. any type with nil value), we return nil. - return dyn.NilValue, nil + v, err = dyn.NilValue, nil + default: + return dyn.InvalidValue, fmt.Errorf("unsupported type: %s", srcv.Kind()) } - return dyn.InvalidValue, fmt.Errorf("unsupported type: %s", srcv.Kind()) + // Ensure the location metadata is retained. + if err != nil { + return dyn.InvalidValue, err + } + return v.WithLocation(ref.Location()), err } func fromTypedStruct(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) { @@ -117,7 +125,7 @@ func fromTypedStruct(src reflect.Value, ref dyn.Value, options ...fromTypedOptio // 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 + return dyn.V(out), nil } // Otherwise, return nil. @@ -164,7 +172,7 @@ func fromTypedMap(src reflect.Value, ref dyn.Value) (dyn.Value, error) { out.Set(refk, nv) } - return dyn.NewValue(out, ref.Location()), nil + return dyn.V(out), nil } func fromTypedSlice(src reflect.Value, ref dyn.Value) (dyn.Value, error) { @@ -199,7 +207,7 @@ func fromTypedSlice(src reflect.Value, ref dyn.Value) (dyn.Value, error) { out[i] = nv } - return dyn.NewValue(out, ref.Location()), nil + return dyn.V(out), nil } func fromTypedString(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) { diff --git a/libs/dyn/convert/from_typed_test.go b/libs/dyn/convert/from_typed_test.go index e5447fe8..c2c17a57 100644 --- a/libs/dyn/convert/from_typed_test.go +++ b/libs/dyn/convert/from_typed_test.go @@ -49,7 +49,7 @@ func TestFromTypedStructPointerZeroFields(t *testing.T) { require.NoError(t, err) assert.Equal(t, dyn.NilValue, nv) - // For an initialized pointer with a nil reference we expect a nil. + // For an initialized pointer with a nil reference we expect an empty map. src = &Tmp{} nv, err = FromTyped(src, dyn.NilValue) require.NoError(t, err) @@ -103,7 +103,7 @@ func TestFromTypedStructSetFields(t *testing.T) { }), nv) } -func TestFromTypedStructSetFieldsRetainLocationIfUnchanged(t *testing.T) { +func TestFromTypedStructSetFieldsRetainLocation(t *testing.T) { type Tmp struct { Foo string `json:"foo"` Bar string `json:"bar"` @@ -122,11 +122,9 @@ func TestFromTypedStructSetFieldsRetainLocationIfUnchanged(t *testing.T) { nv, err := FromTyped(src, ref) require.NoError(t, err) - // Assert foo has retained its location. + // Assert foo and bar have retained their location. assert.Equal(t, dyn.NewValue("bar", dyn.Location{File: "foo"}), nv.Get("foo")) - - // Assert bar lost its location (because it was overwritten). - assert.Equal(t, dyn.NewValue("qux", dyn.Location{}), nv.Get("bar")) + assert.Equal(t, dyn.NewValue("qux", dyn.Location{File: "bar"}), nv.Get("bar")) } func TestFromTypedStringMapWithZeroValue(t *testing.T) { @@ -354,7 +352,7 @@ func TestFromTypedMapNonEmpty(t *testing.T) { }), nv) } -func TestFromTypedMapNonEmptyRetainLocationIfUnchanged(t *testing.T) { +func TestFromTypedMapNonEmptyRetainLocation(t *testing.T) { var src = map[string]string{ "foo": "bar", "bar": "qux", @@ -368,11 +366,9 @@ func TestFromTypedMapNonEmptyRetainLocationIfUnchanged(t *testing.T) { nv, err := FromTyped(src, ref) require.NoError(t, err) - // Assert foo has retained its location. + // Assert foo and bar have retained their locations. assert.Equal(t, dyn.NewValue("bar", dyn.Location{File: "foo"}), nv.Get("foo")) - - // Assert bar lost its location (because it was overwritten). - assert.Equal(t, dyn.NewValue("qux", dyn.Location{}), nv.Get("bar")) + assert.Equal(t, dyn.NewValue("qux", dyn.Location{File: "bar"}), nv.Get("bar")) } func TestFromTypedMapFieldWithZeroValue(t *testing.T) { @@ -429,7 +425,7 @@ func TestFromTypedSliceNonEmpty(t *testing.T) { }), nv) } -func TestFromTypedSliceNonEmptyRetainLocationIfUnchanged(t *testing.T) { +func TestFromTypedSliceNonEmptyRetainLocation(t *testing.T) { var src = []string{ "foo", "bar", @@ -437,17 +433,15 @@ func TestFromTypedSliceNonEmptyRetainLocationIfUnchanged(t *testing.T) { ref := dyn.V([]dyn.Value{ dyn.NewValue("foo", dyn.Location{File: "foo"}), - dyn.NewValue("baz", dyn.Location{File: "baz"}), + dyn.NewValue("bar", dyn.Location{File: "bar"}), }) nv, err := FromTyped(src, ref) require.NoError(t, err) - // Assert foo has retained its location. + // Assert foo and bar have retained their locations. assert.Equal(t, dyn.NewValue("foo", dyn.Location{File: "foo"}), nv.Index(0)) - - // Assert bar lost its location (because it was overwritten). - assert.Equal(t, dyn.NewValue("bar", dyn.Location{}), nv.Index(1)) + assert.Equal(t, dyn.NewValue("bar", dyn.Location{File: "bar"}), nv.Index(1)) } func TestFromTypedStringEmpty(t *testing.T) { @@ -482,12 +476,20 @@ func TestFromTypedStringNonEmptyOverwrite(t *testing.T) { assert.Equal(t, dyn.V("new"), nv) } -func TestFromTypedStringRetainsLocationsIfUnchanged(t *testing.T) { - var src string = "foo" +func TestFromTypedStringRetainsLocations(t *testing.T) { var ref = dyn.NewValue("foo", dyn.Location{File: "foo"}) + + // case: value has not been changed + var src string = "foo" nv, err := FromTyped(src, ref) require.NoError(t, err) assert.Equal(t, dyn.NewValue("foo", dyn.Location{File: "foo"}), nv) + + // case: value has been changed + src = "bar" + nv, err = FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.NewValue("bar", dyn.Location{File: "foo"}), nv) } func TestFromTypedStringTypeError(t *testing.T) { @@ -529,12 +531,20 @@ func TestFromTypedBoolNonEmptyOverwrite(t *testing.T) { assert.Equal(t, dyn.V(true), nv) } -func TestFromTypedBoolRetainsLocationsIfUnchanged(t *testing.T) { - var src bool = true +func TestFromTypedBoolRetainsLocations(t *testing.T) { var ref = dyn.NewValue(true, dyn.Location{File: "foo"}) + + // case: value has not been changed + var src bool = true nv, err := FromTyped(src, ref) require.NoError(t, err) assert.Equal(t, dyn.NewValue(true, dyn.Location{File: "foo"}), nv) + + // case: value has been changed + src = false + nv, err = FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.NewValue(false, dyn.Location{File: "foo"}), nv) } func TestFromTypedBoolVariableReference(t *testing.T) { @@ -584,12 +594,20 @@ func TestFromTypedIntNonEmptyOverwrite(t *testing.T) { assert.Equal(t, dyn.V(int64(1234)), nv) } -func TestFromTypedIntRetainsLocationsIfUnchanged(t *testing.T) { - var src int = 1234 +func TestFromTypedIntRetainsLocations(t *testing.T) { var ref = dyn.NewValue(1234, dyn.Location{File: "foo"}) + + // case: value has not been changed + var src int = 1234 nv, err := FromTyped(src, ref) require.NoError(t, err) assert.Equal(t, dyn.NewValue(1234, dyn.Location{File: "foo"}), nv) + + // case: value has been changed + src = 1235 + nv, err = FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.NewValue(int64(1235), dyn.Location{File: "foo"}), nv) } func TestFromTypedIntVariableReference(t *testing.T) { @@ -639,12 +657,21 @@ func TestFromTypedFloatNonEmptyOverwrite(t *testing.T) { assert.Equal(t, dyn.V(1.23), nv) } -func TestFromTypedFloatRetainsLocationsIfUnchanged(t *testing.T) { - var src float64 = 1.23 +func TestFromTypedFloatRetainsLocations(t *testing.T) { + var src float64 var ref = dyn.NewValue(1.23, dyn.Location{File: "foo"}) + + // case: value has not been changed + src = 1.23 nv, err := FromTyped(src, ref) require.NoError(t, err) assert.Equal(t, dyn.NewValue(1.23, dyn.Location{File: "foo"}), nv) + + // case: value has been changed + src = 1.24 + nv, err = FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.NewValue(1.24, dyn.Location{File: "foo"}), nv) } func TestFromTypedFloatVariableReference(t *testing.T) { @@ -669,3 +696,35 @@ func TestFromTypedAnyNil(t *testing.T) { require.NoError(t, err) assert.Equal(t, dyn.NilValue, nv) } + +func TestFromTypedNilPointerRetainsLocations(t *testing.T) { + type Tmp struct { + Foo string `json:"foo"` + Bar string `json:"bar"` + } + + var src *Tmp + ref := dyn.NewValue(nil, dyn.Location{File: "foobar"}) + + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.NewValue(nil, dyn.Location{File: "foobar"}), nv) +} + +func TestFromTypedNilMapRetainsLocation(t *testing.T) { + var src map[string]string + ref := dyn.NewValue(nil, dyn.Location{File: "foobar"}) + + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.NewValue(nil, dyn.Location{File: "foobar"}), nv) +} + +func TestFromTypedNilSliceRetainsLocation(t *testing.T) { + var src []string + ref := dyn.NewValue(nil, dyn.Location{File: "foobar"}) + + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.NewValue(nil, dyn.Location{File: "foobar"}), nv) +}