Merge lp:~niemeyer/juju-core/schema-field-map-defaults into lp:~juju/juju-core/trunk

[Gustavo Niemeyer]

schema: support defaults in FieldMap

https://codereview.appspot.com/6419056/

[Gustavo Niemeyer (niemeyer)]

Reviewers: mp+115863_code.launchpad.net,

Message:
Please take a look.

Description:
schema: support defaults in FieldMap

https://code.launchpad.net/~niemeyer/juju-core/schema-field-map-defaults/+merge/115863

Requires:
https://code.launchpad.net/~niemeyer/juju-core/schema-string-map/+merge/115849

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6419056/

Affected files:
   A [revision details]
   M charm/config.go
   M charm/meta.go
   M environs/config/config.go
   M environs/dummy/environs.go
   M environs/ec2/config.go
   M schema/schema.go
   M schema/schema_test.go

[Gustavo Niemeyer (niemeyer)]

Please take a look.

https://codereview.appspot.com/6419056/

[Roger Peppe (rogpeppe)]

On 2012/07/20 00:32:02, niemeyer wrote:
> Please take a look.

excellent stuff; make schema work for its living!

LGTM

https://codereview.appspot.com/6419056/

[Roger Peppe (rogpeppe)]

https://codereview.appspot.com/6419056/diff/3001/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/6419056/diff/3001/schema/schema.go#newcode311
schema/schema.go:311: var Omit omit
nice.

https://codereview.appspot.com/6419056/

[William Reade (fwereade)]

LGTM, very nice.

https://codereview.appspot.com/6419056/

[Gustavo Niemeyer (niemeyer)]

*** Submitted:

schema: support defaults in FieldMap

R=rog, fwereade
CC=
https://codereview.appspot.com/6419056

https://codereview.appspot.com/6419056/

=== modified file 'charm/config.go'
--- charm/config.go	2012-07-19 22:31:15 +0000
+++ charm/config.go	2012-07-20 00:33:23 +0000
@@ -130,7 +130,10 @@
 		"default":     schema.OneOf(schema.String(), schema.Int(), schema.Float(), schema.Bool()),
 		"description": schema.String(),
 	},
-	schema.Optional{"default", "description"},
+	schema.Defaults{
+		"default": schema.Omit,
+		"description": schema.Omit,
+	},
 )
 
 var configSchema = schema.FieldMap(
 
=== modified file 'charm/meta.go'
--- charm/meta.go	2012-07-19 22:31:15 +0000
+++ charm/meta.go	2012-07-20 00:33:23 +0000
@@ -155,10 +155,6 @@
 		return
 	}
 
-	// Optional values are context-sensitive and/or have
-	// defaults, which is different than what FieldMap can
-	// readily support. So just do it here first, then
-	// coerce to the real schema.
 	v, err = mapC.Coerce(v, path)
 	if err != nil {
 		return
@@ -167,12 +163,6 @@
 	if _, ok := m["limit"]; !ok {
 		m["limit"] = c.limit
 	}
-	if _, ok := m["optional"]; !ok {
-		m["optional"] = false
-	}
-	if _, ok := m["scope"]; !ok {
-		m["scope"] = ScopeGlobal
-	}
 	return ifaceSchema.Coerce(m, path)
 }
 
@@ -183,7 +173,10 @@
 		"scope":     schema.OneOf(schema.Const(ScopeGlobal), schema.Const(ScopeContainer)),
 		"optional":  schema.Bool(),
 	},
-	schema.Optional{"scope"},
+	schema.Defaults{
+		"scope":    ScopeGlobal,
+		"optional": false,
+	},
 )
 
 var charmSchema = schema.FieldMap(
@@ -197,5 +190,11 @@
 		"revision":    schema.Int(), // Obsolete
 		"subordinate": schema.Bool(),
 	},
-	schema.Optional{"provides", "requires", "peers", "revision", "subordinate"},
+	schema.Defaults{
+		"provides":    schema.Omit,
+		"requires":    schema.Omit,
+		"peers":       schema.Omit,
+		"revision":    schema.Omit,
+		"subordinate": schema.Omit,
+	},
 )
 
=== modified file 'environs/config/config.go'
--- environs/config/config.go	2012-07-19 22:31:15 +0000
+++ environs/config/config.go	2012-07-20 00:33:23 +0000
@@ -23,20 +23,21 @@
 		m: m.(map[string]interface{}),
 		t: make(map[string]interface{}),
 	}
-	if s, _ := c.m["default-series"].(string); s == "" {
+
+	if c.m["default-series"].(string) == "" {
 		c.m["default-series"] = CurrentSeries
 	}
 
 	// Load authorized-keys-path onto authorized-keys, if necessary.
-	path, _ := c.m["authorized-keys-path"].(string)
-	keys, _ := c.m["authorized-keys"].(string)
+	path := c.m["authorized-keys-path"].(string)
+	keys := c.m["authorized-keys"].(string)
 	if path != "" || keys == "" {
 		c.m["authorized-keys"], err = authorizedKeys(path)
 		if err != nil {
 			return nil, err
 		}
-		delete(c.m, "authorized-keys-path")
 	}
+	delete(c.m, "authorized-keys-path")
 
 	// Check if there are any required fields that are empty.
 	for _, attr := range []string{"name", "type", "default-series", "authorized-keys"} {
@@ -103,11 +104,10 @@
 	"authorized-keys-path": schema.String(),
 }
 
-var checker = schema.FieldMap(
-	fields,
-	[]string{
-		"default-series",
-		"authorized-keys",
-		"authorized-keys-path",
-	},
-)
+var defaults = schema.Defaults{
+	"default-series": CurrentSeries,
+	"authorized-keys": "",
+	"authorized-keys-path": "",
+}
+
+var checker = schema.FieldMap(fields, defaults)
 
=== modified file 'environs/dummy/environs.go'
--- environs/dummy/environs.go	2012-07-19 22:31:15 +0000
+++ environs/dummy/environs.go	2012-07-20 00:33:23 +0000
@@ -218,8 +218,8 @@
 		"zookeeper": schema.Bool(),
 		"broken":    schema.Bool(),
 	},
-	[]string{
-		"broken",
+	schema.Defaults{
+		"broken": false,
 	},
 )
 
@@ -233,7 +233,7 @@
 		Config:    cfg,
 		zookeeper: m1["zookeeper"].(bool),
 	}
-	ecfg.broken, _ = m1["broken"].(bool)
+	ecfg.broken = m1["broken"].(bool)
 	return ecfg, nil
 }
 
 
=== modified file 'environs/ec2/config.go'
--- environs/ec2/config.go	2012-07-19 22:31:15 +0000
+++ environs/ec2/config.go	2012-07-20 00:33:23 +0000
@@ -25,11 +25,12 @@
 		"region":         schema.String(),
 		"control-bucket": schema.String(),
 		"public-bucket":  schema.String(),
-	}, []string{
-		"access-key",
-		"secret-key",
-		"region",
-		"public-bucket",
+	},
+	schema.Defaults{
+		"access-key":    "",
+		"secret-key":    "",
+		"region":        "us-east-1",
+		"public-bucket": "",
 	},
 )
 
@@ -41,9 +42,9 @@
 	m := v.(map[string]interface{})
 	c := &providerConfig{Config: config}
 	c.bucket = m["control-bucket"].(string)
-	c.publicBucket = maybeString(m["public-bucket"], "")
-	c.auth.AccessKey = maybeString(m["access-key"], "")
-	c.auth.SecretKey = maybeString(m["secret-key"], "")
+	c.publicBucket = m["public-bucket"].(string)
+	c.auth.AccessKey = m["access-key"].(string)
+	c.auth.SecretKey = m["secret-key"].(string)
 	if c.auth.AccessKey == "" || c.auth.SecretKey == "" {
 		if c.auth.AccessKey != "" {
 			return nil, fmt.Errorf("environment has access-key but no secret-key")
@@ -57,17 +58,9 @@
 		}
 	}
 
-	regionName := maybeString(m["region"], "us-east-1")
-	if _, ok := aws.Regions[regionName]; !ok {
-		return nil, fmt.Errorf("invalid region name %q", regionName)
+	c.region = m["region"].(string)
+	if _, ok := aws.Regions[c.region]; !ok {
+		return nil, fmt.Errorf("invalid region name %q", c.region)
 	}
-	c.region = regionName
 	return c, nil
 }
-
-func maybeString(x interface{}, dflt string) string {
-	if x == nil {
-		return dflt
-	}
-	return x.(string)
-}
 
=== modified file 'schema/schema.go'
--- schema/schema.go	2012-07-19 22:31:15 +0000
+++ schema/schema.go	2012-07-20 00:33:23 +0000
@@ -305,8 +305,14 @@
 	return out, nil
 }
 
+// Omit is a marker for FieldMap and StructFieldMap defaults parameter.
+// If a field is not present in the map and defaults to Omit, the missing
+// field will be ommitted from the coerced map as well.
+var Omit omit
+type omit struct{}
+
 type Fields map[string]Checker
-type Optional []string
+type Defaults map[string]interface{}
 
 // FieldMap returns a Checker that accepts a map value with defined
 // string keys. Every key has an independent checker associated,
@@ -314,32 +320,27 @@
 // individually. If a field fails to be processed, processing stops
 // and returns with the underlying error.
 //
+// Fields in defaults will be set to the provided value if not present
+// in the coerced map. If the default value is schema.Omit, the field
+// missing field will be ommitted from the coerced map as well.
+//
 // The coerced output value has type map[string]interface{}.
-func FieldMap(fields Fields, optional Optional) Checker {
-	return fieldMapC{fields, optional, false}
+func FieldMap(fields Fields, defaults Defaults) Checker {
+	return fieldMapC{fields, defaults, false}
 }
 
 // StrictFieldMap returns a Checker that acts as the one returned by FieldMap,
 // but the Checker returns an error if it encounters an unknown key.
-func StrictFieldMap(fields Fields, optional Optional) Checker {
-	return fieldMapC{fields, optional, true}
+func StrictFieldMap(fields Fields, defaults Defaults) Checker {
+	return fieldMapC{fields, defaults, true}
 }
 
 type fieldMapC struct {
 	fields   Fields
-	optional []string
+	defaults Defaults
 	strict   bool
 }
 
-func (c fieldMapC) isOptional(key string) bool {
-	for _, k := range c.optional {
-		if k == key {
-			return true
-		}
-	}
-	return false
-}
-
 func (c fieldMapC) Coerce(v interface{}, path []string) (interface{}, error) {
 	rv := reflect.ValueOf(v)
 	if rv.Kind() != reflect.Map {
@@ -362,20 +363,40 @@
 	l := rv.Len()
 	out := make(map[string]interface{}, l)
 	for k, checker := range c.fields {
-		vpath[len(vpath)-1] = k
 		var value interface{}
 		valuev := rv.MapIndex(reflect.ValueOf(k))
 		if valuev.IsValid() {
 			value = valuev.Interface()
-		} else if c.isOptional(k) {
-			continue
+		} else if dflt, ok := c.defaults[k]; ok {
+			if dflt == Omit {
+				continue
+			}
+			value = dflt
 		}
+		vpath[len(vpath)-1] = k
 		newv, err := checker.Coerce(value, vpath)
 		if err != nil {
 			return nil, err
 		}
 		out[k] = newv
 	}
+	for k, v := range c.defaults {
+		if v == Omit {
+			continue
+		}
+		if _, ok := out[k]; !ok {
+			checker, ok := c.fields[k]
+			if !ok {
+				return nil, fmt.Errorf("got default value for unknown field %q", k)
+			}
+			vpath[len(vpath)-1] = k
+			newv, err := checker.Coerce(v, vpath)
+			if err != nil {
+				return nil, err
+			}
+			out[k] = newv
+		}
+	}
 	return out, nil
 }
 
 
=== modified file 'schema/schema_test.go'
--- schema/schema_test.go	2012-07-19 22:31:15 +0000
+++ schema/schema_test.go	2012-07-20 00:33:23 +0000
@@ -240,7 +240,7 @@
 	out, err := sch.Coerce(map[string]interface{}{"a": "A", "b": "B"}, aPath)
 
 	c.Assert(err, IsNil)
-	c.Assert(out, DeepEquals, map[string]interface{}{"a": "A", "b": "B"})
+	c.Assert(out, DeepEquals, map[string]interface{}{"a": "A", "b": "B", "c": "C"})
 
 	out, err = sch.Coerce(42, aPath)
 	c.Assert(out, IsNil)
@@ -261,7 +261,7 @@
 	// b is optional
 	out, err = sch.Coerce(map[string]interface{}{"a": "A"}, aPath)
 	c.Assert(err, IsNil)
-	c.Assert(out, DeepEquals, map[string]interface{}{"a": "A"})
+	c.Assert(out, DeepEquals, map[string]interface{}{"a": "A", "c": "C"})
 
 	// First path entry shouldn't have dots in an error message.
 	out, err = sch.Coerce(map[string]bool{"a": true}, nil)
@@ -273,26 +273,48 @@
 	fields := schema.Fields{
 		"a": schema.Const("A"),
 		"b": schema.Const("B"),
-	}
-	sch := schema.FieldMap(fields, schema.Optional{"b"})
+		"c": schema.Const("C"),
+	}
+	defaults := schema.Defaults{
+		"b": schema.Omit,
+		"c": "C",
+	}
+	sch := schema.FieldMap(fields, defaults)
 	assertFieldMap(c, sch)
 
-	out, err := sch.Coerce(map[string]interface{}{"a": "A", "b": "B", "c": "C"}, aPath)
+	out, err := sch.Coerce(map[string]interface{}{"a": "A", "b": "B", "d": "D"}, aPath)
 	c.Assert(err, IsNil)
-	c.Assert(out, DeepEquals, map[string]interface{}{"a": "A", "b": "B"})
+	c.Assert(out, DeepEquals, map[string]interface{}{"a": "A", "b": "B", "c": "C"})
+}
+
+func (s *S) TestFieldMapDefaultInvalid(c *C) {
+	fields := schema.Fields{
+		"a": schema.Const("A"),
+	}
+	defaults := schema.Defaults{
+		"a": "B",
+	}
+	sch := schema.FieldMap(fields, defaults)
+	_, err := sch.Coerce(map[string]interface{}{}, aPath)
+	c.Assert(err, ErrorMatches, `<path>.a: expected "A", got "B"`)
 }
 
 func (s *S) TestStrictFieldMap(c *C) {
 	fields := schema.Fields{
 		"a": schema.Const("A"),
 		"b": schema.Const("B"),
-	}
-	sch := schema.StrictFieldMap(fields, schema.Optional{"b"})
+		"c": schema.Const("C"),
+	}
+	defaults := schema.Defaults{
+		"b": schema.Omit,
+		"c": "C",
+	}
+	sch := schema.StrictFieldMap(fields, defaults)
 	assertFieldMap(c, sch)
 
-	out, err := sch.Coerce(map[string]interface{}{"a": "A", "b": "B", "c": "C"}, aPath)
+	out, err := sch.Coerce(map[string]interface{}{"a": "A", "b": "B", "d": "D"}, aPath)
 	c.Assert(out, IsNil)
-	c.Assert(err, ErrorMatches, `<path>.c: expected nothing, got "C"`)
+	c.Assert(err, ErrorMatches, `<path>.d: expected nothing, got "D"`)
 }
 
 func (s *S) TestSchemaMap(c *C) {