Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for squashing embedded struct pointers #205

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 15 additions & 7 deletions mapstructure.go
Expand Up @@ -862,6 +862,10 @@ func (d *Decoder) decodeMapFromStruct(name string, dataVal reflect.Value, val re
// Next get the actual value of this field and verify it is assignable
// to the map value.
v := dataVal.Field(i)
if v.Kind() == reflect.Ptr && v.Elem().Kind() == reflect.Struct {
// Handle embedded struct pointers as embedded structs.
v = v.Elem()
}
Comment on lines +865 to +868
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is what is causing the bug in #231

If I remove this line my tests pass, but of course the new tested added in this PR fail.

I attempted to move this check to lower down, but I have not got it working yet. It seems like we should not be changing v, but we may need to deref the ptr in a few places to make this work.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it to pass, one sec.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if !v.Type().AssignableTo(valMap.Type().Elem()) {
return fmt.Errorf("cannot assign type '%s' to map value field of type '%s'", v.Type(), valMap.Type().Elem())
}
Expand Down Expand Up @@ -1232,10 +1236,14 @@ func (d *Decoder) decodeStructFromMap(name string, dataVal, val reflect.Value) e

for i := 0; i < structType.NumField(); i++ {
fieldType := structType.Field(i)
fieldKind := fieldType.Type.Kind()
fieldVal := structVal.Field(i)
if fieldVal.Kind() == reflect.Ptr && fieldVal.Elem().Kind() == reflect.Struct {
// Handle embedded struct pointers as embedded structs.
fieldVal = fieldVal.Elem()
}

// If "squash" is specified in the tag, we squash the field down.
squash := d.config.Squash && fieldKind == reflect.Struct && fieldType.Anonymous
squash := d.config.Squash && fieldVal.Kind() == reflect.Struct && fieldType.Anonymous
remain := false

// We always parse the tags cause we're looking for other tags too
Expand All @@ -1253,21 +1261,21 @@ func (d *Decoder) decodeStructFromMap(name string, dataVal, val reflect.Value) e
}

if squash {
if fieldKind != reflect.Struct {
if fieldVal.Kind() != reflect.Struct {
errors = appendErrors(errors,
fmt.Errorf("%s: unsupported type for squash: %s", fieldType.Name, fieldKind))
fmt.Errorf("%s: unsupported type for squash: %s", fieldType.Name, fieldVal.Kind()))
} else {
structs = append(structs, structVal.FieldByName(fieldType.Name))
structs = append(structs, fieldVal)
}
continue
}

// Build our field
if remain {
remainField = &field{fieldType, structVal.Field(i)}
remainField = &field{fieldType, fieldVal}
} else {
// Normal struct field, store it away
fields = append(fields, field{fieldType, structVal.Field(i)})
fields = append(fields, field{fieldType, fieldVal})
}
}
}
Expand Down
76 changes: 75 additions & 1 deletion mapstructure_test.go
Expand Up @@ -61,6 +61,11 @@ type EmbeddedSquash struct {
Vunique string
}

type EmbeddedPointerSquash struct {
*Basic `mapstructure:",squash"`
Vunique string
}

type EmbeddedAndNamed struct {
Basic
Named Basic
Expand Down Expand Up @@ -655,6 +660,56 @@ func TestDecodeFrom_EmbeddedSquash(t *testing.T) {
}
}

func TestDecode_EmbeddedPointerSquash_FromStructToMap(t *testing.T) {
t.Parallel()

input := EmbeddedPointerSquash{
Basic: &Basic{
Vstring: "foo",
},
Vunique: "bar",
}

var result map[string]interface{}
err := Decode(input, &result)
if err != nil {
t.Fatalf("got an err: %s", err.Error())
}

if result["Vstring"] != "foo" {
t.Errorf("vstring value should be 'foo': %#v", result["Vstring"])
}

if result["Vunique"] != "bar" {
t.Errorf("vunique value should be 'bar': %#v", result["Vunique"])
}
}

func TestDecode_EmbeddedPointerSquash_FromMapToStruct(t *testing.T) {
t.Parallel()

input := map[string]interface{}{
"Vstring": "foo",
"Vunique": "bar",
}

result := EmbeddedPointerSquash{
Basic: &Basic{},
}
err := Decode(input, &result)
if err != nil {
t.Fatalf("got an err: %s", err.Error())
}

if result.Vstring != "foo" {
t.Errorf("vstring value should be 'foo': %#v", result.Vstring)
}

if result.Vunique != "bar" {
t.Errorf("vunique value should be 'bar': %#v", result.Vunique)
}
}

func TestDecode_EmbeddedSquashConfig(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -2334,7 +2389,26 @@ func TestDecode_StructTaggedWithOmitempty_KeepNonEmptyValues(t *testing.T) {
"visible-map": emptyMap,
"omittable-map": map[string]interface{}{"k": "v"},
"visible-nested": emptyNested,
"omittable-nested": &Nested{},
"omittable-nested": map[string]interface{}{
"Vbar": map[string]interface{}{
"Vbool": false,
"Vdata": interface{}(nil),
"Vextra": "",
"Vfloat": float64(0),
"Vint": 0,
"Vint16": int16(0),
"Vint32": int32(0),
"Vint64": int64(0),
"Vint8": int8(0),
"VjsonFloat": float64(0),
"VjsonInt": 0,
"VjsonNumber": json.Number(""),
"VjsonUint": uint(0),
"Vstring": "",
"Vuint": uint(0),
},
"Vfoo": "",
},
Comment on lines +2392 to +2411
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test change was masking the bug described in #231

This PR should not have changed any tests with omitempty, right?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're probably right. I thought this would be safe because this only added tests but I see this is a change.

}

actual := &map[string]interface{}{}
Expand Down