Merge lp:~dimitern/juju-core/103-relation-entity-tags into lp:~go-bot/juju-core/trunk

[Dimiter Naydenov]

state: Make Relation an Entity

This adds relation tags to the names package. The
format is "relation-<id>", where <id> is the relation's
numeric id.
It also adds a Tag() method on the relation, and a few
other necessary modifications to make state.Relation
objects behave like other entities. This is needed for
the uniter API facade, so relations can be used as
params and results.

https://codereview.appspot.com/12748044/

R=gz, rogpeppe

state: Make Relation an Entity

This adds relation tags to the names package. The
format is "relation-<id>", where <id> is the relation's
numeric id.
It also adds a Tag() method on the relation, and a few
other necessary modifications to make state.Relation
objects behave like other entities. This is needed for
the uniter API facade, so relations can be used as
params and results.

https://codereview.appspot.com/12748044/

[Dimiter Naydenov (dimitern)]

Reviewers: mp+180157_code.launchpad.net,

Message:
Please take a look.

Description:
state: Make Relation an Entity

This adds relation tags to the names package. The
format is "relation-<id>", where <id> is the relation's
numeric id.
It also adds a Tag() method on the relation, and a few
other necessary modifications to make state.Relation
objects behave like other entities. This is needed for
the uniter API facade, so relations can be used as
params and results.

https://code.launchpad.net/~dimitern/juju-core/103-relation-entity-tags/+merge/180157

Requires:
https://code.launchpad.net/~dimitern/juju-core/102-state-endpoint-refactoring/+merge/180141

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   A names/relation.go
   A names/relation_test.go
   M names/tag.go
   M names/tag_test.go
   M state/relation.go
   M state/state.go
   M state/state_test.go

[Roger Peppe (rogpeppe)]

LGTM with some extra tests.

https://codereview.appspot.com/12748044/diff/1/names/relation.go
File names/relation.go (right):

https://codereview.appspot.com/12748044/diff/1/names/relation.go#newcode18
names/relation.go:18: func RelationTag(relationId string) string {
if !IsRelation(relationId) {
    panic(fmt.Sprintf("%q is not a valid relation id", relationId))
}

https://codereview.appspot.com/12748044/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/12748044/diff/1/state/relation.go#newcode66
state/relation.go:66: func (r *Relation) Tag() string {
This should have a test.

https://codereview.appspot.com/12748044/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/12748044/diff/1/state/state_test.go#newcode1438
state/state_test.go:1438: tag: "service-ser-vice2",
We need a relation tag test here.

https://codereview.appspot.com/12748044/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/12748044/diff/1/names/relation.go
File names/relation.go (right):

https://codereview.appspot.com/12748044/diff/1/names/relation.go#newcode18
names/relation.go:18: func RelationTag(relationId string) string {
On 2013/08/14 15:02:49, rog wrote:
> if !IsRelation(relationId) {
> panic(fmt.Sprintf("%q is not a valid relation id", relationId))
> }

Done.

https://codereview.appspot.com/12748044/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/12748044/diff/1/state/relation.go#newcode66
state/relation.go:66: func (r *Relation) Tag() string {
On 2013/08/14 15:02:49, rog wrote:
> This should have a test.

Done.

https://codereview.appspot.com/12748044/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/12748044/diff/1/state/state_test.go#newcode1438
state/state_test.go:1438: tag: "service-ser-vice2",
On 2013/08/14 15:02:49, rog wrote:
> We need a relation tag test here.

Done.

https://codereview.appspot.com/12748044/

[Martin Packman (gz)]

LGTM.

https://codereview.appspot.com/12748044/diff/5002/names/relation_test.go
File names/relation_test.go (right):

https://codereview.appspot.com/12748044/diff/5002/names/relation_test.go#newcode16
names/relation_test.go:16: var relationIdTests = []struct {
Okay, so this is a case where I like the table test style.

https://codereview.appspot.com/12748044/diff/5002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/12748044/diff/5002/state/state.go#newcode498
state/state.go:498: return nil, errors.NotFoundf("relation %s", id)
If the conversion fails, is that really a not found rather than an
invalid situation?

Is there a test exercising this branch?

https://codereview.appspot.com/12748044/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/12748044/diff/5002/names/relation_test.go
File names/relation_test.go (right):

https://codereview.appspot.com/12748044/diff/5002/names/relation_test.go#newcode16
names/relation_test.go:16: var relationIdTests = []struct {
On 2013/08/14 15:54:56, gz wrote:
> Okay, so this is a case where I like the table test style.

Yeah!

https://codereview.appspot.com/12748044/diff/5002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/12748044/diff/5002/state/state.go#newcode498
state/state.go:498: return nil, errors.NotFoundf("relation %s", id)
On 2013/08/14 15:54:56, gz wrote:
> If the conversion fails, is that really a not found rather than an
invalid
> situation?

> Is there a test exercising this branch?

I'll add a test, thanks. And the point is: there won't be a relation
with an invalid id anyway, so it's NotFoundError. It will also help
later in the API when we convert NotFoundErrors to ErrPerm.

https://codereview.appspot.com/12748044/

=== added file 'names/relation.go'
--- names/relation.go	1970-01-01 00:00:00 +0000
+++ names/relation.go	2013-08-14 16:01:40 +0000
@@ -0,0 +1,24 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package names
+
+import (
+	"fmt"
+	"regexp"
+)
+
+var validRelation = regexp.MustCompile("^" + NumberSnippet + "$")
+
+// IsRelation returns whether id is a valid relation id.
+func IsRelation(id string) bool {
+	return validRelation.MatchString(id)
+}
+
+// RelationTag returns the tag for the relation with the given id.
+func RelationTag(relationId string) string {
+	if !IsRelation(relationId) {
+		panic(fmt.Sprintf("%q is not a valid relation id", relationId))
+	}
+	return makeTag(RelationTagKind, relationId)
+}

=== added file 'names/relation_test.go'
--- names/relation_test.go	1970-01-01 00:00:00 +0000
+++ names/relation_test.go	2013-08-14 16:01:40 +0000
@@ -0,0 +1,36 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package names_test
+
+import (
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/names"
+)
+
+type relationSuite struct{}
+
+var _ = gc.Suite(&relationSuite{})
+
+var relationIdTests = []struct {
+	pattern string
+	valid   bool
+}{
+	{pattern: "", valid: false},
+	{pattern: "foo", valid: false},
+	{pattern: "foo42", valid: false},
+	{pattern: "42", valid: true},
+	{pattern: "0", valid: true},
+	{pattern: "%not", valid: false},
+	{pattern: "42also-not", valid: false},
+	{pattern: "042", valid: false},
+	{pattern: "0x42", valid: false},
+}
+
+func (s *relationSuite) TestRelationIdFormats(c *gc.C) {
+	for i, test := range relationIdTests {
+		c.Logf("test %d: %q", i, test.pattern)
+		c.Assert(names.IsRelation(test.pattern), gc.Equals, test.valid)
+	}
+}

=== modified file 'names/tag.go'
--- names/tag.go	2013-08-07 14:21:04 +0000
+++ names/tag.go	2013-08-14 16:01:40 +0000
@@ -9,19 +9,21 @@
 )
 
 const (
-	UnitTagKind    = "unit"
-	MachineTagKind = "machine"
-	ServiceTagKind = "service"
-	EnvironTagKind = "environment"
-	UserTagKind    = "user"
+	UnitTagKind     = "unit"
+	MachineTagKind  = "machine"
+	ServiceTagKind  = "service"
+	EnvironTagKind  = "environment"
+	UserTagKind     = "user"
+	RelationTagKind = "relation"
 )
 
 var validKinds = map[string]bool{
-	UnitTagKind:    true,
-	MachineTagKind: true,
-	ServiceTagKind: true,
-	EnvironTagKind: true,
-	UserTagKind:    true,
+	UnitTagKind:     true,
+	MachineTagKind:  true,
+	ServiceTagKind:  true,
+	EnvironTagKind:  true,
+	UserTagKind:     true,
+	RelationTagKind: true,
 }
 
 var tagSuffixToId = map[string]func(string) string{
@@ -30,11 +32,12 @@
 }
 
 var verifyId = map[string]func(string) bool{
-	UnitTagKind:    IsUnit,
-	MachineTagKind: IsMachine,
-	ServiceTagKind: IsService,
-	UserTagKind:    IsUser,
-	EnvironTagKind: IsEnvironment,
+	UnitTagKind:     IsUnit,
+	MachineTagKind:  IsMachine,
+	ServiceTagKind:  IsService,
+	UserTagKind:     IsUser,
+	EnvironTagKind:  IsEnvironment,
+	RelationTagKind: IsRelation,
 }
 
 // TagKind returns one of the *TagKind constants for the given tag, or

=== modified file 'names/tag_test.go'
--- names/tag_test.go	2013-08-07 14:21:04 +0000
+++ names/tag_test.go	2013-08-14 16:01:40 +0000
@@ -23,6 +23,7 @@
 	{tag: "service-foo", kind: names.ServiceTagKind},
 	{tag: "environment-42", kind: names.EnvironTagKind},
 	{tag: "user-admin", kind: names.UserTagKind},
+	{tag: "relation-42", kind: names.RelationTagKind},
 	{tag: "foo", err: `"foo" is not a valid tag`},
 	{tag: "unit", err: `"unit" is not a valid tag`},
 }
@@ -95,6 +96,10 @@
 	expectKind: names.EnvironTagKind,
 	resultId:   "foo",
 }, {
+	tag:        "relation-42",
+	expectKind: names.RelationTagKind,
+	resultId:   "42",
+}, {
 	tag:        "environment-/",
 	expectKind: names.EnvironTagKind,
 	resultErr:  `"environment-/" is not a valid environment tag`,
@@ -113,9 +118,10 @@
 }}
 
 var makeTag = map[string]func(id string) string{
-	names.MachineTagKind: names.MachineTag,
-	names.UnitTagKind:    names.UnitTag,
-	names.ServiceTagKind: names.ServiceTag,
+	names.MachineTagKind:  names.MachineTag,
+	names.UnitTagKind:     names.UnitTag,
+	names.ServiceTagKind:  names.ServiceTag,
+	names.RelationTagKind: names.RelationTag,
 	// TODO(rog) environment and user, when they have Tag functions.
 }
 

=== modified file 'state/relation.go'
--- state/relation.go	2013-07-09 10:32:23 +0000
+++ state/relation.go	2013-08-14 16:01:40 +0000
@@ -15,6 +15,7 @@
 
 	"launchpad.net/juju-core/charm"
 	errors "launchpad.net/juju-core/errors"
+	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/utils"
 )
 
@@ -60,6 +61,12 @@
 	return r.doc.Key
 }
 
+// Tag returns a name identifying the relation that is safe to use
+// as a file name.
+func (r *Relation) Tag() string {
+	return names.RelationTag(strconv.Itoa(r.doc.Id))
+}
+
 // Refresh refreshes the contents of the relation from the underlying
 // state. It returns an error that satisfies IsNotFound if the relation has been
 // removed.

=== modified file 'state/state.go'
--- state/state.go	2013-08-07 14:23:50 +0000
+++ state/state.go	2013-08-14 16:01:40 +0000
@@ -492,6 +492,12 @@
 			return nil, errors.NotFoundf("environment %q", id)
 		}
 		return st.Environment()
+	case names.RelationTagKind:
+		relId, err := strconv.Atoi(id)
+		if err != nil {
+			return nil, errors.NotFoundf("relation %s", id)
+		}
+		return st.Relation(relId)
 	}
 	return nil, err
 }
@@ -512,6 +518,8 @@
 		coll = st.units.Name
 	case names.UserTagKind:
 		coll = st.users.Name
+	case names.RelationTagKind:
+		coll = st.relations.Name
 	default:
 		return "", "", fmt.Errorf("%q is not a valid collection tag", tag)
 	}
@@ -1181,6 +1189,7 @@
 	's': names.ServiceTagKind + "-",
 	'u': names.UnitTagKind + "-",
 	'e': names.EnvironTagKind + "-",
+	'r': names.RelationTagKind + "-",
 }
 
 func tagForGlobalKey(key string) (string, bool) {

=== modified file 'state/state_test.go'
--- state/state_test.go	2013-08-08 17:56:28 +0000
+++ state/state_test.go	2013-08-14 16:01:40 +0000
@@ -1412,6 +1412,12 @@
 	tag: "unit-123",
 	err: `"unit-123" is not a valid unit tag`,
 }, {
+	tag: "relation-blah",
+	err: `"relation-blah" is not a valid relation tag`,
+}, {
+	tag: "relation-42",
+	err: "relation 42 not found",
+}, {
 	tag: "unit-foo",
 	err: `"unit-foo" is not a valid unit tag`,
 }, {
@@ -1437,6 +1443,8 @@
 }, {
 	tag: "service-ser-vice2",
 }, {
+	tag: "relation-0",
+}, {
 	tag: "unit-ser-vice2-0",
 }, {
 	tag: "user-arble",
@@ -1445,11 +1453,12 @@
 }}
 
 var entityTypes = map[string]interface{}{
-	names.UserTagKind:    (*state.User)(nil),
-	names.EnvironTagKind: (*state.Environment)(nil),
-	names.ServiceTagKind: (*state.Service)(nil),
-	names.UnitTagKind:    (*state.Unit)(nil),
-	names.MachineTagKind: (*state.Machine)(nil),
+	names.UserTagKind:     (*state.User)(nil),
+	names.EnvironTagKind:  (*state.Environment)(nil),
+	names.ServiceTagKind:  (*state.Service)(nil),
+	names.UnitTagKind:     (*state.Unit)(nil),
+	names.MachineTagKind:  (*state.Machine)(nil),
+	names.RelationTagKind: (*state.Relation)(nil),
 }
 
 func (s *StateSuite) TestFindEntity(c *gc.C) {
@@ -1461,6 +1470,13 @@
 	c.Assert(err, gc.IsNil)
 	_, err = s.State.AddUser("arble", "pass")
 	c.Assert(err, gc.IsNil)
+	_, err = s.State.AddService("wordpress", s.AddTestingCharm(c, "wordpress"))
+	c.Assert(err, gc.IsNil)
+	eps, err := s.State.InferEndpoints([]string{"wordpress", "ser-vice2"})
+	c.Assert(err, gc.IsNil)
+	rel, err := s.State.AddRelation(eps...)
+	c.Assert(err, gc.IsNil)
+	c.Assert(rel.Id(), gc.Equals, 0)
 
 	for i, test := range findEntityTests {
 		c.Logf("test %d: %q", i, test.tag)