Merge lp:~jimmiebtlr/juju-core/fix-no-peers-charm into lp:~go-bot/juju-core/trunk

[Jimmie Butler]

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://codereview.appspot.com/98410043/

[John A Meinel (jameinel)]

31 @@ -353,6 +359,7 @@
32 vpath[len(vpath)-1] = fmt.Sprint(k.Interface())
33 newv, err := c.value.Coerce(rv.MapIndex(k).Interface(), vpath)
34 if err != nil {
35 +
36 return nil, err
37 }
38 out[newk.(string)] = newv

I think this is a spurious change.

I'm a bit surprised we don't have a direct test of schema/* that would fit this change. I believe you have it covered, but only at the charm level.
Is there a test in schema that you could add that would be a bit more of a direct test?

Otherwise, LGTM.

[Jimmie Butler (jimmiebtlr)]

I'll take a look at the schema/* tests, and see about removing the
extraneous space. Thanks for the feedback.

On Mon, May 5, 2014 at 9:23 PM, John A Meinel <email address hidden>wrote:

> 31 @@ -353,6 +359,7 @@
> 32 vpath[len(vpath)-1] = fmt.Sprint(k.Interface())
> 33 newv, err := c.value.Coerce(rv.MapIndex(k).Interface(), vpath)
> 34 if err != nil {
> 35 +
> 36 return nil, err
> 37 }
> 38 out[newk.(string)] = newv
>
> I think this is a spurious change.
>
> I'm a bit surprised we don't have a direct test of schema/* that would fit
> this change. I believe you have it covered, but only at the charm level.
> Is there a test in schema that you could add that would be a bit more of a
> direct test?
>
> Otherwise, LGTM.
> --
>
> https://code.launchpad.net/~jimmiebtlr/juju-core/fix-no-peers-charm/+merge/218195
> You are the owner of lp:~jimmiebtlr/juju-core/fix-no-peers-charm.
>

--
Thanks,
Jimmie Butler

[William Reade (fwereade)]

FWIW, if you're planning to fix schema stringMap to assume nil->empty-map (+1 to that approach) please make sure the other map types in schema are consistent.

[William Reade (fwereade)]

Thanks, this was definitely the right place to make the change. A few more comments:

10 + c.Assert(err, gc.IsNil)

Please check the content of the returned metadata, on general principles. If we end up with inconsistent sometimes-nil-sometimes-empty behaviour, please fix that too; a casual reading indicates that we won't but some tests might start failing because we now have empty maps where we expected nils.

24 + return make(map[string]interface{}), nil

mapC should be returning a map[interface{}]interface{}

35 + // handles case where map is empty
48 + // handles case where map is empty
61 + // handles case where map is empty

"treat nil value as empty map", perhaps? And, FWIW, I'd skip the blank lines around the stanzas you inserted, but YMMV legitimately.

[Jimmie Butler (jimmiebtlr)]

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:3...

[Dave Cheney (dave-cheney)]

On 2014/05/20 03:33:38, jimmiebtlr wrote:
> Please take a look.

This code looks fine but I don't feel I can +1 this change without some
discussion as this changes the behaviour of _all_ map, mapSet, etc
fields.

https://codereview.appspot.com/98410043/

[William Reade (fwereade)]

LGTM, I'd accept this as-is, but please take a little look at the
nil/map{} issue. If it's too complex, let me know, and I'll approve it.

Dave, thanks for the caution; but schema isn't as heavily used as one
might think (especially the map bits), and I think this is a good
change.

https://codereview.appspot.com/98410043/diff/1/charm/meta_test.go
File charm/meta_test.go (left):

https://codereview.appspot.com/98410043/diff/1/charm/meta_test.go#oldcode163
charm/meta_test.go:163: c.Assert(meta.Peers, gc.IsNil)
It's this bit that bugs me, that I alluded to yesterday -- having a Meta
type that sometimes has nil and sometimes has {} feels weirdly
inconsistent. It's not really a very big deal, but I'd appreciate a fix
if it's not excessively complex to change.

https://codereview.appspot.com/98410043/diff/1/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/98410043/diff/1/schema/schema.go#newcode297
schema/schema.go:297: // handles case where map is empty
treat nil value as empty map :)

https://codereview.appspot.com/98410043/

[Jimmie Butler (jimmiebtlr)]

Ok, misunderstood that little bit. Basically it should return an empty map
rather than nil even if peers, requires, and provides are not mentioned in
the charm file. That will require a change in a different location,
meaning we may not need to change the map behavior at all.

On Tue, May 20, 2014 at 12:48 AM, William Reade <<email address hidden>
> wrote:

> LGTM, I'd accept this as-is, but please take a little look at the
> nil/map{} issue. If it's too complex, let me know, and I'll approve it.
>
> Dave, thanks for the caution; but schema isn't as heavily used as one
> might think (especially the map bits), and I think this is a good
> change.
>
>
> https://codereview.appspot.com/98410043/diff/1/charm/meta_test.go
> File charm/meta_test.go (left):
>
>
> https://codereview.appspot.com/98410043/diff/1/charm/meta_test.go#oldcode163
> charm/meta_test.go:163: c.Assert(meta.Peers, gc.IsNil)
> It's this bit that bugs me, that I alluded to yesterday -- having a Meta
> type that sometimes has nil and sometimes has {} feels weirdly
> inconsistent. It's not really a very big deal, but I'd appreciate a fix
> if it's not excessively complex to change.
>
> https://codereview.appspot.com/98410043/diff/1/schema/schema.go
> File schema/schema.go (right):
>
> https://codereview.appspot.com/98410043/diff/1/schema/schema.go#newcode297
> schema/schema.go:297: // handles case where map is empty
> treat nil value as empty map :)
>
> https://codereview.appspot.com/98410043/
>
> --
>
> https://code.launchpad.net/~jimmiebtlr/juju-core/fix-no-peers-charm/+merge/218195
> You are the owner of lp:~jimmiebtlr/juju-core/fix-no-peers-charm.
>

--
Thanks,
Jimmie Butler

[Jimmie Butler (jimmiebtlr)]

Please take a look.

https://codereview.appspot.com/98410043/

[William Reade (fwereade)]

> Ok, misunderstood that little bit. Basically it should return an empty map
> rather than nil even if peers, requires, and provides are not mentioned in
> the charm file. That will require a change in a different location,
> meaning we may not need to change the map behavior at all.

An easy alternative would be to just return nil maps (of the right type) from the schema package. I'm not bothered about which we pick so much as I am about consistency.

[William Reade (fwereade)]

Rejecting this; appears to have been supplanted by https://code.launchpad.net/~jimmiebtlr/juju-core/fix-no-peers-charms/+merge/221156

[Jimmie Butler (jimmiebtlr)]

Correct, thanks
On May 28, 2014 3:57 AM, "William Reade" <email address hidden>
wrote:

> The proposal to merge lp:~jimmiebtlr/juju-core/fix-no-peers-charm into
> lp:juju-core has been updated.
>
> Status: Needs review => Rejected
>
> For more details, see:
>
> https://code.launchpad.net/~jimmiebtlr/juju-core/fix-no-peers-charm/+merge/218195
> --
>
> https://code.launchpad.net/~jimmiebtlr/juju-core/fix-no-peers-charm/+merge/218195
> You are the owner of lp:~jimmiebtlr/juju-core/fix-no-peers-charm.
>

Unmerged revisions

2713. By Jimmie Butler on 2014-05-28

Go fmt.

2712. By Jimmie Butler on 2014-05-28

Change test order, put error check first.

2711. By Jimmie Butler on 2014-05-28

Change test order to check error first.

2710. By Jimmie Butler on 2014-05-28

Merge with most recent master.

2709. By Jimmie Butler on 2014-05-26

Merge in most recent master

2708. By Jimmie Butler on 2014-05-21

Remove unneeded statement in parseRelation.

2707. By Jimmie Butler on 2014-05-21

Merge master chanegs

2706. By Jimmie Butler on 2014-05-21

Add back commented empty charm fields test.

2705. By Jimmie Butler on 2014-05-21

Go fmt.

2704. By Jimmie Butler on 2014-05-21

Merge in most recent master

=== modified file 'charm/meta.go'
--- charm/meta.go	2014-04-09 16:36:12 +0000
+++ charm/meta.go	2014-05-28 05:03:23 +0000
@@ -96,13 +96,13 @@
 	Summary     string
 	Description string
 	Subordinate bool
-	Provides    map[string]Relation `bson:",omitempty"`
-	Requires    map[string]Relation `bson:",omitempty"`
-	Peers       map[string]Relation `bson:",omitempty"`
-	Format      int                 `bson:",omitempty"`
-	OldRevision int                 `bson:",omitempty"` // Obsolete
-	Categories  []string            `bson:",omitempty"`
-	Series      string              `bson:",omitempty"`
+	Provides    map[string]Relation
+	Requires    map[string]Relation
+	Peers       map[string]Relation
+	Format      int      `bson:",omitempty"`
+	OldRevision int      `bson:",omitempty"` // Obsolete
+	Categories  []string `bson:",omitempty"`
+	Series      string   `bson:",omitempty"`
 }
 
 func generateRelationHooks(relName string, allHooks map[string]bool) {
@@ -262,10 +262,10 @@
 }
 
 func parseRelations(relations interface{}, role RelationRole) map[string]Relation {
+	result := make(map[string]Relation)
 	if relations == nil {
-		return nil
+		return result
 	}
-	result := make(map[string]Relation)
 	for name, rel := range relations.(map[string]interface{}) {
 		relMap := rel.(map[string]interface{})
 		relation := Relation{
 
=== modified file 'charm/meta_test.go'
--- charm/meta_test.go	2014-04-09 16:36:12 +0000
+++ charm/meta_test.go	2014-05-28 05:03:23 +0000
@@ -93,8 +93,8 @@
 		Interface: "mysql",
 		Scope:     charm.ScopeGlobal,
 	})
-	c.Assert(meta.Requires, gc.IsNil)
-	c.Assert(meta.Peers, gc.IsNil)
+	c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
+	c.Assert(meta.Peers, gc.DeepEquals, map[string]charm.Relation{})
 
 	meta, err = charm.ReadMeta(repoMeta("riak"))
 	c.Assert(err, gc.IsNil)
@@ -117,7 +117,7 @@
 		Limit:     1,
 		Scope:     charm.ScopeGlobal,
 	})
-	c.Assert(meta.Requires, gc.IsNil)
+	c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
 
 	meta, err = charm.ReadMeta(repoMeta("terracotta"))
 	c.Assert(err, gc.IsNil)
@@ -135,7 +135,7 @@
 		Limit:     1,
 		Scope:     charm.ScopeGlobal,
 	})
-	c.Assert(meta.Requires, gc.IsNil)
+	c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
 
 	meta, err = charm.ReadMeta(repoMeta("wordpress"))
 	c.Assert(err, gc.IsNil)
@@ -160,7 +160,13 @@
 		Optional:  true,
 		Scope:     charm.ScopeGlobal,
 	})
-	c.Assert(meta.Peers, gc.IsNil)
+	c.Assert(meta.Peers, gc.DeepEquals, map[string]charm.Relation{})
+
+	meta, err = charm.ReadMeta(repoMeta("empty-fields"))
+	c.Assert(err, gc.IsNil)
+	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{})
 }
 
 var relationsConstraintsTests = []struct {
@@ -396,12 +402,15 @@
 	for i, codec := range codecs {
 		c.Logf("codec %d", i)
 		empty_input := charm.Meta{}
+		empty_input.Provides = make(map[string]charm.Relation)
+		empty_input.Peers = make(map[string]charm.Relation)
+		empty_input.Requires = make(map[string]charm.Relation)
 		data, err := codec.Marshal(empty_input)
 		c.Assert(err, gc.IsNil)
 		var empty_output charm.Meta
 		err = codec.Unmarshal(data, &empty_output)
 		c.Assert(err, gc.IsNil)
-		c.Assert(empty_input, gc.DeepEquals, empty_output)
+		c.Assert(empty_output, gc.DeepEquals, empty_input)
 	}
 }
 
 
=== modified file 'schema/schema.go'
--- schema/schema.go	2014-01-15 10:50:11 +0000
+++ schema/schema.go	2014-05-28 05:03:23 +0000
@@ -293,6 +293,11 @@
 
 func (c mapC) 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[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}
 	}
 
=== modified file 'schema/schema_test.go'
--- schema/schema_test.go	2014-01-14 13:10:26 +0000
+++ schema/schema_test.go	2014-05-28 05:03:23 +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(err, gc.IsNil)
+	c.Assert(out, gc.DeepEquals, map[interface{}]interface{}{})
 
 	out, err = sch.Coerce(map[int]int{1: 1}, aPath)
 	c.Assert(out, gc.IsNil)
@@ -274,12 +274,12 @@
 	c.Assert(out, gc.DeepEquals, map[string]interface{}{"a": int64(1), "b": int64(2)})
 
 	out, err = sch.Coerce(42, aPath)
-	c.Assert(out, gc.IsNil)
 	c.Assert(err, gc.ErrorMatches, "<path>: expected map, got int\\(42\\)")
+	c.Assert(out, gc.IsNil)
 
 	out, err = sch.Coerce(nil, aPath)
-	c.Assert(out, gc.IsNil)
-	c.Assert(err, gc.ErrorMatches, "<path>: expected map, got nothing")
+	c.Assert(err, gc.IsNil)
+	c.Assert(out, gc.DeepEquals, map[string]interface{}{})
 
 	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(err, gc.IsNil)
+	c.Assert(out, gc.DeepEquals, map[string]interface{}{})
 
 	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(err, gc.IsNil)
+	c.Assert(out, gc.DeepEquals, map[string]interface{}{})
 
 	// First path entry shouldn't have dots in an error message.
 	out, err = sch.Coerce(map[string]int{"a": 1}, nil)
 
=== added directory 'testing/repo/quantal/empty-fields'
=== 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-28 05:03:23 +0000
@@ -0,0 +1,6 @@
+name: empty-fields
+summary: "Kerosene powered blender"
+description: "Description here"
+provides:
+peers:
+requires: