Repository: mitchellh/mapstructure (go)

Issue:
Panic with a decode hook to convert a `time.Time` in a struct into a `*github.com/golang/protobuf/types.Timestamp` in other struct
I was trying to use mapstructure to convert between a protobuf type and a corresponding internal type. The two times had different forms of timestamp where the protobuf type used the well known Timestamp type and the internal type used a `time.Time`.

My decode hook looked like the following:

```
var (
	tsType      = reflect.TypeOf((*types.Timestamp)(nil))
	timePtrType = reflect.TypeOf((*time.Time)(nil))
	timeType    = timePtrType.Elem()
	mapStrInf   = reflect.TypeOf((map[string]interface{})(nil))
)

// HookTimeToPBtimestamp is a mapstructure decode hook to translate a time.Time value to
// a protobuf Timestamp value.
func HookTimeToPBTimestamp(from, to reflect.Type, data interface{}) (interface{}, error) {
	// Note that mapstructure doesn't do direct struct to struct conversion in this case. I
	// still don't completely understand why converting the PB TS to time.Time does but
	// I suspect it has something to do with the struct containing a concrete time.Time
	// as opposed to a pointer to a time.Time. Regardless this path through mapstructure
	// first will decode the concrete time.Time into a map[string]interface{} before
	// eventually decoding that map[string]interface{} into the *types.Timestamp. One
	// other note is that mapstructure ends up creating a new Value and sets it it to
	// the time.Time value and thats what gets passed to us. That is why we end up
	// seeing a *time.Time instead of a time.Time.
	if from == timePtrType && to == mapStrInf {
		ts := data.(*time.Time)
		nanos := ts.UnixNano()

		seconds := nanos / 1000000000
		nanos = nanos % 1000000000

		return &map[string]interface{}{
			"Seconds": seconds,
			"Nanos":   int32(nanos),
		}, nil
	}
	return data, nil
}
```

I had a test to ensure my decode hook was working but it was panicing like so:

```panic: reflect: reflect.flag.mustBeAssignable using unaddressable value [recovered]
        panic: reflect: reflect.flag.mustBeAssignable using unaddressable value

goroutine 4 [running]:
testing.tRunner.func1.1(0x14b6d00, 0xc000061410)
        /usr/local/go/src/testing/testing.go:941 +0x3d0
testing.tRunner.func1(0xc00012f440)
        /usr/local/go/src/testing/testing.go:944 +0x3f9
panic(0x14b6d00, 0xc000061410)
        /usr/local/go/src/runtime/panic.go:967 +0x166
reflect.flag.mustBeAssignableSlow(0x15)
        /usr/local/go/src/reflect/value.go:247 +0x138
reflect.flag.mustBeAssignable(...)
        /usr/local/go/src/reflect/value.go:234
reflect.Value.Set(0x14d9fe0, 0xc00007bd10, 0x15, 0x14d9fe0, 0xc00007bd70, 0x15)
        /usr/local/go/src/reflect/value.go:1526 +0x3b
github.com/mitchellh/mapstructure.(*Decoder).decodeMapFromMap(0xc000010070, 0x1481f3b, 0x9, 0x14d9fe0, 0xc000010098, 0x195, 0x14d9fe0, 0xc00007bd10, 0x15, 0x14d9fe0, ...)
        /Users/mkeeler/Code/repos/mapstructure/mapstructure.go:843 +0x49c
github.com/mitchellh/mapstructure.(*Decoder).decodeMap(0xc000010070, 0x1481f3b, 0x9, 0x14a7320, 0xc000010098, 0x14d9fe0, 0xc00007bd10, 0x15, 0x14a7320, 0xc000010098)
        /Users/mkeeler/Code/repos/mapstructure/mapstructure.go:763 +0x56a
github.com/mitchellh/mapstructure.(*Decoder).decode(0xc000010070, 0x1481f3b, 0x9, 0x1552700, 0xc00000e600, 0x14d9fe0, 0xc00007bd10, 0x15, 0x0, 0x0)
        /Users/mkeeler/Code/repos/mapstructure/mapstructure.go:439 +0x625
github.com/mitchellh/mapstructure.(*Decoder).decodeMapFromStruct(0xc000010070, 0x0, 0x0, 0x14eddc0, 0xc00000e580, 0x99, 0x14d9fe0, 0xc000010078, 0x195, 0x14d9fe0, ...)
        /Users/mkeeler/Code/repos/mapstructure/mapstructure.go:912 +0x978
github.com/mitchellh/mapstructure.(*Decoder).decodeStruct(0xc000010070, 0x0, 0x0, 0x14eddc0, 0xc00000e580, 0x14edd40, 0xc000010068, 0x199, 0x14eddc0, 0xc00000e580)
        /Users/mkeeler/Code/repos/mapstructure/mapstructure.go:1172 +0x329
github.com/mitchellh/mapstructure.(*Decoder).decode(0xc000010070, 0x0, 0x0, 0x14eddc0, 0xc00000e580, 0x14edd40, 0xc000010068, 0x199, 0xc00000e580, 0xc00005df58)
        /Users/mkeeler/Code/repos/mapstructure/mapstructure.go:437 +0x75f
github.com/mitchellh/mapstructure.(*Decoder).Decode(0xc000010070, 0x14eddc0, 0xc00000e580, 0xc00000e580, 0x0)
        /Users/mkeeler/Code/repos/mapstructure/mapstructure.go:370 +0xf0
github.com/hashicorp/consul/agent/agentpb.TestHookTimeToPBTimestamp(0xc00012f440)
        /Users/mkeeler/Code/repos/consul/auto-config/cert-generation/agent/agentpb/translate_test.go:63 +0x16c
```

I managed to find the cause which exists in a couple places. In both the intermediate map being created for the intermediate steps in a struct -> struct decode are not being created in a way that they are settable:

https://github.com/mitchellh/mapstructure/blob/d16e9488127408e67948eb43b6d3fbb9f222da10/mapstructure.go#L907

https://github.com/mitchellh/mapstructure/blob/d16e9488127408e67948eb43b6d3fbb9f222da10/mapstructure.go#L1157-L1158

I have a PR almost ready that fixes the issues by creating these maps in a way that they are settable. I just wanted to have an issue open to track when adding my test to mapstructure_bug_test.go.

Pull request:
Ensure that intermediate maps during struct to struct decoding are settable
Fixes #202 

Patch:
diff --git a/mapstructure.go b/mapstructure.go
index b384d9d..f41bcc5 100644
--- a/mapstructure.go
+++ b/mapstructure.go
@@ -906,11 +906,22 @@ func (d *Decoder) decodeMapFromStruct(name string, dataVal reflect.Value, val re
 			mType := reflect.MapOf(vKeyType, vElemType)
 			vMap := reflect.MakeMap(mType)
 
-			err := d.decode(keyName, x.Interface(), vMap)
+			// Creating a pointer to a map so that other methods can completely
+			// overwrite the map if need be (looking at you decodeMapFromMap). The
+			// indirection allows the underlying map to be settable (CanSet() == true)
+			// where as reflect.MakeMap returns an unsettable map.
+			addrVal := reflect.New(vMap.Type())
+			reflect.Indirect(addrVal).Set(vMap)
+
+			err := d.decode(keyName, x.Interface(), reflect.Indirect(addrVal))
 			if err != nil {
 				return err
 			}
 
+			// the underlying map may have been completely overwritten so pull
+			// it indirectly out of the enclosing value.
+			vMap = reflect.Indirect(addrVal)
+
 			if squash {
 				for _, k := range vMap.MapKeys() {
 					valMap.SetMapIndex(k, vMap.MapIndex(k))
@@ -1154,13 +1165,23 @@ func (d *Decoder) decodeStruct(name string, data interface{}, val reflect.Value)
 		// Not the most efficient way to do this but we can optimize later if
 		// we want to. To convert from struct to struct we go to map first
 		// as an intermediary.
-		m := make(map[string]interface{})
-		mval := reflect.Indirect(reflect.ValueOf(&m))
-		if err := d.decodeMapFromStruct(name, dataVal, mval, mval); err != nil {
+
+		// Make a new map to hold our result
+		mapType := reflect.TypeOf((map[string]interface{})(nil))
+		mval := reflect.MakeMap(mapType)
+
+		// Creating a pointer to a map so that other methods can completely
+		// overwrite the map if need be (looking at you decodeMapFromMap). The
+		// indirection allows the underlying map to be settable (CanSet() == true)
+		// where as reflect.MakeMap returns an unsettable map.
+		addrVal := reflect.New(mval.Type())
+
+		reflect.Indirect(addrVal).Set(mval)
+		if err := d.decodeMapFromStruct(name, dataVal, reflect.Indirect(addrVal), mval); err != nil {
 			return err
 		}
 
-		result := d.decodeStructFromMap(name, mval, val)
+		result := d.decodeStructFromMap(name, reflect.Indirect(addrVal), val)
 		return result
 
 	default:


Test patch:
diff --git a/mapstructure_bugs_test.go b/mapstructure_bugs_test.go
index f030c2d..8c87ebd 100644
--- a/mapstructure_bugs_test.go
+++ b/mapstructure_bugs_test.go
@@ -3,6 +3,7 @@ package mapstructure
 import (
 	"reflect"
 	"testing"
+	"time"
 )
 
 // GH-1, GH-10, GH-96
@@ -475,3 +476,66 @@ func TestDecodeBadDataTypeInSlice(t *testing.T) {
 		t.Error("An error was expected, got nil")
 	}
 }
+
+// #202 Ensure that intermediate maps in the struct -> struct decode process are settable
+// and not just the elements within them.
+func TestDecodeIntermeidateMapsSettable(t *testing.T) {
+	type Timestamp struct {
+		Seconds int64
+		Nanos   int32
+	}
+
+	type TsWrapper struct {
+		Timestamp *Timestamp
+	}
+
+	type TimeWrapper struct {
+		Timestamp time.Time
+	}
+
+	input := TimeWrapper{
+		Timestamp: time.Unix(123456789, 987654),
+	}
+
+	expected := TsWrapper{
+		Timestamp: &Timestamp{
+			Seconds: 123456789,
+			Nanos:   987654,
+		},
+	}
+
+	timePtrType := reflect.TypeOf((*time.Time)(nil))
+	mapStrInfType := reflect.TypeOf((map[string]interface{})(nil))
+
+	var actual TsWrapper
+	decoder, err := NewDecoder(&DecoderConfig{
+		Result: &actual,
+		DecodeHook: func(from, to reflect.Type, data interface{}) (interface{}, error) {
+			if from == timePtrType && to == mapStrInfType {
+				ts := data.(*time.Time)
+				nanos := ts.UnixNano()
+
+				seconds := nanos / 1000000000
+				nanos = nanos % 1000000000
+
+				return &map[string]interface{}{
+					"Seconds": seconds,
+					"Nanos":   int32(nanos),
+				}, nil
+			}
+			return data, nil
+		},
+	})
+
+	if err != nil {
+		t.Fatalf("failed to create decoder: %v", err)
+	}
+
+	if err := decoder.Decode(&input); err != nil {
+		t.Fatalf("failed to decode input: %v", err)
+	}
+
+	if !reflect.DeepEqual(expected, actual) {
+		t.Fatalf("expected: %#[1]v (%[1]T), got: %#[2]v (%[2]T)", expected, actual)
+	}
+}