Code review comment for lp:~jimmiebtlr/juju-core/fix-no-peers-charm

Reviewers: mp+218195_code.launchpad.net,

Message:
Please take a look.

Description:
Allow charm with empty peers, provides, requires.

In the case that a null was read in for the maps, use
an empty map instead of throwing error. Added test case
to show error. Bug #1313793

https://code.launchpad.net/~jimmiebtlr/juju-core/fix-no-peers-charm/+merge/218195

(do not edit description out of merge proposal)

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

Affected files (+42, -8 lines):
   A [revision details]
   M charm/meta_test.go
   M schema/schema.go
   M schema/schema_test.go
   A testing/repo/quantal/empty-fields/metadata.yaml

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140520024339-qids5ewddm0js4yq
+New revision: <email address hidden>

Index: charm/meta_test.go
=== modified file 'charm/meta_test.go'
--- charm/meta_test.go 2014-04-09 16:36:12 +0000
+++ charm/meta_test.go 2014-05-20 03:20:37 +0000
@@ -161,6 +161,12 @@
    Scope: charm.ScopeGlobal,
   })
   c.Assert(meta.Peers, gc.IsNil)
+
+ meta, err = charm.ReadMeta(repoMeta("empty-fields"))
+ c.Assert(meta.Provides, gc.DeepEquals, map[string]charm.Relation{})
+ c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
+ c.Assert(meta.Peers, gc.DeepEquals, map[string]charm.Relation{})
+ c.Assert(err, gc.IsNil)
  }

  var relationsConstraintsTests = []struct {

Index: schema/schema.go
=== modified file 'schema/schema.go'
--- schema/schema.go 2014-01-15 10:50:11 +0000
+++ schema/schema.go 2014-05-20 03:20:37 +0000
@@ -293,6 +293,11 @@

  func (c mapC) Coerce(v interface{}, path []string) (interface{}, error) {
   rv := reflect.ValueOf(v)
+
+ // handles case where map is empty
+ if rv.Kind() == reflect.Invalid {
+ return make(map[interface{}]interface{}), nil
+ }
   if rv.Kind() != reflect.Map {
    return nil, error_{"map", v, path}
   }
@@ -334,6 +339,11 @@

  func (c stringMapC) Coerce(v interface{}, path []string) (interface{},
error) {
   rv := reflect.ValueOf(v)
+
+ // treat nil value as empty map
+ if rv.Kind() == reflect.Invalid {
+ return make(map[string]interface{}), nil
+ }
   if rv.Kind() != reflect.Map {
    return nil, error_{"map", v, path}
   }
@@ -399,6 +409,11 @@

  func (c fieldMapC) Coerce(v interface{}, path []string) (interface{},
error) {
   rv := reflect.ValueOf(v)
+
+ // treat nil value as empty map
+ if rv.Kind() == reflect.Invalid {
+ return make(map[string]interface{}), nil
+ }
   if rv.Kind() != reflect.Map {
    return nil, error_{"map", v, path}
   }
@@ -488,6 +503,11 @@

  func (c mapSetC) Coerce(v interface{}, path []string) (interface{}, error)
{
   rv := reflect.ValueOf(v)
+
+ // treat nil value as empty map
+ if rv.Kind() == reflect.Invalid {
+ return make(map[string]interface{}), nil
+ }
   if rv.Kind() != reflect.Map {
    return nil, error_{"map", v, path}
   }

Index: schema/schema_test.go
=== modified file 'schema/schema_test.go'
--- schema/schema_test.go 2014-01-14 13:10:26 +0000
+++ schema/schema_test.go 2014-05-20 03:20:37 +0000
@@ -250,8 +250,8 @@
   c.Assert(err, gc.ErrorMatches, "<path>: expected map, got int\\(42\\)")

   out, err = sch.Coerce(nil, aPath)
- c.Assert(out, gc.IsNil)
- c.Assert(err, gc.ErrorMatches, "<path>: expected map, got nothing")
+ c.Assert(out, gc.DeepEquals, map[interface{}]interface{}{})
+ c.Assert(err, gc.IsNil)

   out, err = sch.Coerce(map[int]int{1: 1}, aPath)
   c.Assert(out, gc.IsNil)
@@ -278,8 +278,8 @@
   c.Assert(err, gc.ErrorMatches, "<path>: expected map, got int\\(42\\)")

   out, err = sch.Coerce(nil, aPath)
- c.Assert(out, gc.IsNil)
- c.Assert(err, gc.ErrorMatches, "<path>: expected map, got nothing")
+ c.Assert(out, gc.DeepEquals, map[string]interface{}{})
+ c.Assert(err, gc.IsNil)

   out, err = sch.Coerce(map[int]int{1: 1}, aPath)
   c.Assert(out, gc.IsNil)
@@ -306,8 +306,8 @@
   c.Assert(err, gc.ErrorMatches, "<path>: expected map, got int\\(42\\)")

   out, err = sch.Coerce(nil, aPath)
- c.Assert(out, gc.IsNil)
- c.Assert(err, gc.ErrorMatches, "<path>: expected map, got nothing")
+ c.Assert(out, gc.DeepEquals, map[string]interface{}{})
+ c.Assert(err, gc.IsNil)

   out, err = sch.Coerce(map[string]interface{}{"a": "A", "b": "C"}, aPath)
   c.Assert(out, gc.IsNil)
@@ -416,8 +416,8 @@
   c.Assert(err, gc.ErrorMatches, `<path>: expected map, got int\(42\)`)

   out, err = sch.Coerce(nil, aPath)
- c.Assert(out, gc.IsNil)
- c.Assert(err, gc.ErrorMatches, `<path>: expected map, got nothing`)
+ c.Assert(out, gc.DeepEquals, map[string]interface{}{})
+ c.Assert(err, gc.IsNil)

   // First path entry shouldn't have dots in an error message.
   out, err = sch.Coerce(map[string]int{"a": 1}, nil)

Index: testing/repo/quantal/empty-fields/metadata.yaml
=== added file 'testing/repo/quantal/empty-fields/metadata.yaml'
--- testing/repo/quantal/empty-fields/metadata.yaml 1970-01-01 00:00:00
+0000
+++ testing/repo/quantal/empty-fields/metadata.yaml 2014-05-03 17:48:01
+0000
@@ -0,0 +1,6 @@
+name: empty-fields
+summary: "Kerosene powered blender"
+description: "Description here"
+provides:
+peers:
+requires: