helper/schema: apply schema defaults at the field level when reading from config

We were waiting until the higher-level (m schemaMap) diffString method
to apply defaults, which was messing with set hashcode evaluation for
cases when a field with a default is included in the hash function.

fixes #824

helper/schema/field_reader_config.go

@@ -10,8 +10,10 @@ import (
 	"github.com/mitchellh/mapstructure"
 )
 
-// ConfigFieldReader reads fields out of an untyped map[string]string to
+// ConfigFieldReader reads fields out of an untyped map[string]string to the
-// the best of its ability.
+// best of its ability. It also applies defaults from the Schema. (The other
+// field readers do not need default handling because they source fully
+// populated data structures.)
 type ConfigFieldReader struct {
 	Config *terraform.ResourceConfig
 	Schema map[string]*Schema
@@ -138,7 +140,16 @@ func (r *ConfigFieldReader) readPrimitive(
 	k string, schema *Schema) (FieldReadResult, error) {
 	raw, ok := r.Config.Get(k)
 	if !ok {
-		return FieldReadResult{}, nil
+		// Nothing in config, but we might still have a default from the schema
+		var err error
+		raw, err = schema.DefaultValue()
+		if err != nil {
+			return FieldReadResult{}, fmt.Errorf("%s, error loading default: %s", k, err)
+		}
+
+		if raw == nil {
+			return FieldReadResult{}, nil
+		}
 	}
 
 	var result string

helper/schema/field_reader_config_test.go

@@ -1,6 +1,7 @@
 package schema
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/hashicorp/terraform/config"
@@ -56,3 +57,88 @@ func testConfig(
 
 	return terraform.NewResourceConfig(rc)
 }
+
+func TestConfigFieldReader_DefaultHandling(t *testing.T) {
+	schema := map[string]*Schema{
+		"strWithDefault": &Schema{
+			Type:    TypeString,
+			Default: "ImADefault",
+		},
+		"strWithDefaultFunc": &Schema{
+			Type: TypeString,
+			DefaultFunc: func() (interface{}, error) {
+				return "FuncDefault", nil
+			},
+		},
+	}
+
+	cases := map[string]struct {
+		Addr   []string
+		Result FieldReadResult
+		Config *terraform.ResourceConfig
+		Err    bool
+	}{
+		"gets default value when no config set": {
+			[]string{"strWithDefault"},
+			FieldReadResult{
+				Value:    "ImADefault",
+				Exists:   true,
+				Computed: false,
+			},
+			testConfig(t, map[string]interface{}{}),
+			false,
+		},
+		"config overrides default value": {
+			[]string{"strWithDefault"},
+			FieldReadResult{
+				Value:    "fromConfig",
+				Exists:   true,
+				Computed: false,
+			},
+			testConfig(t, map[string]interface{}{
+				"strWithDefault": "fromConfig",
+			}),
+			false,
+		},
+		"gets default from function when no config set": {
+			[]string{"strWithDefaultFunc"},
+			FieldReadResult{
+				Value:    "FuncDefault",
+				Exists:   true,
+				Computed: false,
+			},
+			testConfig(t, map[string]interface{}{}),
+			false,
+		},
+		"config overrides default function": {
+			[]string{"strWithDefaultFunc"},
+			FieldReadResult{
+				Value:    "fromConfig",
+				Exists:   true,
+				Computed: false,
+			},
+			testConfig(t, map[string]interface{}{
+				"strWithDefaultFunc": "fromConfig",
+			}),
+			false,
+		},
+	}
+
+	for name, tc := range cases {
+		r := &ConfigFieldReader{
+			Schema: schema,
+			Config: tc.Config,
+		}
+		out, err := r.ReadField(tc.Addr)
+		if (err != nil) != tc.Err {
+			t.Fatalf("%s: err: %s", name, err)
+		}
+		if s, ok := out.Value.(*Set); ok {
+			// If it is a set, convert to a list so its more easily checked.
+			out.Value = s.List()
+		}
+		if !reflect.DeepEqual(tc.Result, out) {
+			t.Fatalf("%s: bad: %#v", name, out)
+		}
+	}
+}

helper/schema/schema.go

@@ -201,6 +201,24 @@ func (s *Schema) GoString() string {
 	return fmt.Sprintf("*%#v", *s)
 }
 
+// Returns a default value for this schema by either reading Default or
+// evaluating DefaultFunc. If neither of these are defined, returns nil.
+func (s *Schema) DefaultValue() (interface{}, error) {
+	if s.Default != nil {
+		return s.Default, nil
+	}
+
+	if s.DefaultFunc != nil {
+		defaultValue, err := s.DefaultFunc()
+		if err != nil {
+			return nil, fmt.Errorf("error loading default: %s", err)
+		}
+		return defaultValue, nil
+	}
+
+	return nil, nil
+}
+
 func (s *Schema) finalizeDiff(
 	d *terraform.ResourceAttrDiff) *terraform.ResourceAttrDiff {
 	if d == nil {
@@ -373,23 +391,16 @@ func (m schemaMap) Input(
 			continue
 		}
 
-		// Skip if it has a default
+		// Skip if it has a default value
-		if v.Default != nil {
+		defaultValue, err := v.DefaultValue()
-			continue
+		if err != nil {
+			return nil, fmt.Errorf("%s: error loading default: %s", k, err)
 		}
-		if f := v.DefaultFunc; f != nil {
+		if defaultValue != nil {
-			value, err := f()
+			continue
-			if err != nil {
-				return nil, fmt.Errorf(
-					"%s: error loading default: %s", k, err)
-			}
-			if value != nil {
-				continue
-			}
 		}
 
 		var value interface{}
-		var err error
 		switch v.Type {
 		case TypeBool:
 			fallthrough
@@ -835,16 +846,6 @@ func (m schemaMap) diffString(
 	var originalN interface{}
 	var os, ns string
 	o, n, _, _ := d.diffChange(k)
-	if n == nil {
-		n = schema.Default
-		if schema.DefaultFunc != nil {
-			var err error
-			n, err = schema.DefaultFunc()
-			if err != nil {
-				return fmt.Errorf("%s, error loading default: %s", k, err)
-			}
-		}
-	}
 	if schema.StateFunc != nil {
 		originalN = n
 		n = schema.StateFunc(n)

helper/schema/schema_test.go

@@ -1,12 +1,15 @@
 package schema
 
 import (
+	"bytes"
+	"fmt"
 	"os"
 	"reflect"
 	"testing"
 
 	"github.com/hashicorp/terraform/config"
 	"github.com/hashicorp/terraform/config/lang/ast"
+	"github.com/hashicorp/terraform/helper/hashcode"
 	"github.com/hashicorp/terraform/terraform"
 )
 
@@ -1886,6 +1889,60 @@ func TestSchemaMap_Diff(t *testing.T) {
 
 			Err: false,
 		},
+
+		// #47 - https://github.com/hashicorp/terraform/issues/824
+		{
+			Schema: map[string]*Schema{
+				"block_device": &Schema{
+					Type:     TypeSet,
+					Optional: true,
+					Computed: true,
+					Elem: &Resource{
+						Schema: map[string]*Schema{
+							"device_name": &Schema{
+								Type:     TypeString,
+								Required: true,
+							},
+							"delete_on_termination": &Schema{
+								Type:     TypeBool,
+								Optional: true,
+								Default:  true,
+							},
+						},
+					},
+					Set: func(v interface{}) int {
+						var buf bytes.Buffer
+						m := v.(map[string]interface{})
+						buf.WriteString(fmt.Sprintf("%s-", m["device_name"].(string)))
+						buf.WriteString(fmt.Sprintf("%t-", m["delete_on_termination"].(bool)))
+						return hashcode.String(buf.String())
+					},
+				},
+			},
+
+			State: &terraform.InstanceState{
+				Attributes: map[string]string{
+					"block_device.#":                                "2",
+					"block_device.616397234.delete_on_termination":  "true",
+					"block_device.616397234.device_name":            "/dev/sda1",
+					"block_device.2801811477.delete_on_termination": "true",
+					"block_device.2801811477.device_name":           "/dev/sdx",
+				},
+			},
+
+			Config: map[string]interface{}{
+				"block_device": []map[string]interface{}{
+					map[string]interface{}{
+						"device_name": "/dev/sda1",
+					},
+					map[string]interface{}{
+						"device_name": "/dev/sdx",
+					},
+				},
+			},
+			Diff: nil,
+			Err:  false,
+		},
 	}
 
 	for i, tc := range cases {
@@ -1918,7 +1975,7 @@ func TestSchemaMap_Diff(t *testing.T) {
 }
 
 func TestSchemaMap_Input(t *testing.T) {
-	cases := []struct {
+	cases := map[string]struct {
 		Schema map[string]*Schema
 		Config map[string]interface{}
 		Input  map[string]string
@@ -1929,7 +1986,7 @@ func TestSchemaMap_Input(t *testing.T) {
 		 * String decode
 		 */
 
-		{
+		"uses input on optional field with no config": {
 			Schema: map[string]*Schema{
 				"availability_zone": &Schema{
 					Type:     TypeString,
@@ -1948,7 +2005,7 @@ func TestSchemaMap_Input(t *testing.T) {
 			Err: false,
 		},
 
-		{
+		"input ignored when config has a value": {
 			Schema: map[string]*Schema{
 				"availability_zone": &Schema{
 					Type:     TypeString,
@@ -1969,7 +2026,7 @@ func TestSchemaMap_Input(t *testing.T) {
 			Err: false,
 		},
 
-		{
+		"input ignored when schema has a default": {
 			Schema: map[string]*Schema{
 				"availability_zone": &Schema{
 					Type:     TypeString,
@@ -1987,7 +2044,7 @@ func TestSchemaMap_Input(t *testing.T) {
 			Err: false,
 		},
 
-		{
+		"input ignored when default function returns a value": {
 			Schema: map[string]*Schema{
 				"availability_zone": &Schema{
 					Type: TypeString,
@@ -2007,7 +2064,7 @@ func TestSchemaMap_Input(t *testing.T) {
 			Err: false,
 		},
 
-		{
+		"input used when default function returns nil": {
 			Schema: map[string]*Schema{
 				"availability_zone": &Schema{
 					Type: TypeString,
@@ -2048,11 +2105,11 @@ func TestSchemaMap_Input(t *testing.T) {
 
 		actual, err := schemaMap(tc.Schema).Input(input, rc)
 		if (err != nil) != tc.Err {
-			t.Fatalf("#%d err: %s", i, err)
+			t.Fatalf("#%v err: %s", i, err)
 		}
 
 		if !reflect.DeepEqual(tc.Result, actual.Config) {
-			t.Fatalf("#%d: bad:\n\n%#v", i, actual.Config)
+			t.Fatalf("#%v: bad:\n\ngot: %#v\nexpected: %#v", i, actual.Config, tc.Result)
 		}
 	}
 }