Merge lp:~jameinel/juju-core/typed-registry into lp:~go-bot/juju-core/trunk

[John A Meinel]

utils/registry: TypedNameVersion registry

As part of getting API versioning in place, I was putting together some
Registry's in a couple of places. I realized I wanted 2 things that were
essentially identical code, just tracking slightly different object
types. So I put together an abstraction that still enforces the Type,
but is otherwise treats the input and output as a generic interface{}.

https://codereview.appspot.com/99180044/

utils/registry: TypedNameVersion registry

As part of getting API versioning in place, I was putting together some
Registry's in a couple of places. I realized I wanted 2 things that were
essentially identical code, just tracking slightly different object
types. So I put together an abstraction that still enforces the Type,
but is otherwise treats the input and output as a generic interface{}.

I'm not stuck on the naming, though I went with
registry.TypedNameVersion as the handle for this thing. I could just as
easily calling simply registry.Registry and the fact that it enforces
Types and uses Name+Version as the key can just be in the docs.

I'm not 100% sure that we'll strictly need this, as the code is evolving
to where I may only have 1 registry. However, it still seemed like a
useful bit of functionality, and it was easily split out from the other
work as a self-contained and well tested bit of work.

If we prefer, I can wait to land this until it is clearly necessary
(rather than just having a single Registry that has a fixed object
type). But if we ever have >=2 registries with different types, I think
we'll want something like this.

https://codereview.appspot.com/99180044/

[John A Meinel (jameinel)]

Reviewers: mp+219161_code.launchpad.net,

Message:
Please take a look.

Description:
utils/registry: TypedNameVersion registry

As part of getting API versioning in place, I was putting together some
Registry's in a couple of places. I realized I wanted 2 things that were
essentially identical code, just tracking slightly different object
types. So I put together an abstraction that still enforces the Type,
but is otherwise treats the input and output as a generic interface{}.

I'm not stuck on the naming, though I went with
registry.TypedNameVersion as the handle for this thing. I could just as
easily calling simply registry.Registry and the fact that it enforces
Types and uses Name+Version as the key can just be in the docs.

I'm not 100% sure that we'll strictly need this, as the code is evolving
to where I may only have 1 registry. However, it still seemed like a
useful bit of functionality, and it was easily split out from the other
work as a self-contained and well tested bit of work.

If we prefer, I can wait to land this until it is clearly necessary
(rather than just having a single Registry that has a fixed object
type). But if we ever have >=2 registries with different types, I think
we'll want something like this.

https://code.launchpad.net/~jameinel/juju-core/typed-registry/+merge/219161

(do not edit description out of merge proposal)

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

Affected files (+280, -0 lines):
   A [revision details]
   A utils/registry/export_test.go
   A utils/registry/package_test.go
   A utils/registry/registry.go
   A utils/registry/registry_test.go

[William Reade (fwereade)]

I'm also not sure that we'll end up using exactly this, but LGTM all the
same; just please remember to drop it if it turns out that we don't.

https://codereview.appspot.com/99180044/

[John A Meinel (jameinel)]

Please take a look.

https://codereview.appspot.com/99180044/

[Go Bot (go-bot)]

The attempt to merge lp:~jameinel/juju-core/typed-registry into lp:juju-core failed. Below is the output from the failed tests.

utils/registry/registry.go:11:2: cannot find package "launchpad.net/juju-core/errors" in any of:
 /usr/lib/go/src/pkg/launchpad.net/juju-core/errors (from $GOROOT)
 /home/tarmac/trees/src/launchpad.net/juju-core/errors (from $GOPATH)
mongod: no process found

[Roger Peppe (rogpeppe)]

Saw this in passing - it looks reasonable, although with all the talk of
"versions" and "facades" it seems as if it it might fit better as a
subpackage of or within the rpc package.

https://codereview.appspot.com/99180044/diff/20001/utils/registry/registry.go
File utils/registry/registry.go (right):

https://codereview.appspot.com/99180044/diff/20001/utils/registry/registry.go#newcode16
utils/registry/registry.go:16: // defined when the registry was created.
It will be cast during Register so
I'm not sure I see the use case for the conversion logic. Wouldn't it be
simpler just to stipulate that the same type must always be registered
for the same name?

https://codereview.appspot.com/99180044/diff/20001/utils/registry/registry.go#newcode21
utils/registry/registry.go:21: versions map[string]Versions
Another possibility might be to hold a single map with a struct{name,
version} key. Then Get is only a single map lookup and List could be
simpler too.

https://codereview.appspot.com/99180044/diff/20001/utils/registry/registry.go#newcode38
utils/registry/registry.go:38: // Versions maps concrete versions of the
objects.
a) this doc comment doesn't say what it maps from.
b) it looks as if the Versions type doesn't actually need to be
exported.

https://codereview.appspot.com/99180044/diff/20001/utils/registry/registry.go#newcode41
utils/registry/registry.go:41: // Register records the factory that can
be used to produce an instance of the
This comment doesn't seem to make sense in terms of the functionality of
this package. There's nothing factory-like going on here, although it's
certainly possible to store functions in the registry; similarly
"facade" doesn't really have any meaning here.

// Register records the object associated with the given name and
version.
// The object must be of the same type passed to NewTypedNameVersion.

?

https://codereview.appspot.com/99180044/diff/20001/utils/registry/registry.go#newcode52
utils/registry/registry.go:52: if r.versions == nil {
How can this happen, assuming NewTypedNameVersion has been used?

https://codereview.appspot.com/99180044/diff/20001/utils/registry/registry.go#newcode97
utils/registry/registry.go:97: // facade is not found, it returns
error.NotFound
I'd prefer to see a bool return here to make it clear that
there's only one kind of error (and it's closer to the map
interface too, which is essentially what this type is)

https://codereview.appspot.com/99180044/

=== added directory 'utils/registry'
=== added file 'utils/registry/export_test.go'
--- utils/registry/export_test.go	1970-01-01 00:00:00 +0000
+++ utils/registry/export_test.go	2014-05-16 11:49:09 +0000
@@ -0,0 +1,8 @@
+// Copyright 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package registry
+
+var (
+	DescriptionFromVersions = descriptionFromVersions
+)

=== added file 'utils/registry/package_test.go'
--- utils/registry/package_test.go	1970-01-01 00:00:00 +0000
+++ utils/registry/package_test.go	2014-05-16 11:49:09 +0000
@@ -0,0 +1,14 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package registry_test
+
+import (
+	stdtesting "testing"
+
+	gc "launchpad.net/gocheck"
+)
+
+func TestAll(t *stdtesting.T) {
+	gc.TestingT(t)
+}

=== added file 'utils/registry/registry.go'
--- utils/registry/registry.go	1970-01-01 00:00:00 +0000
+++ utils/registry/registry.go	2014-05-16 11:49:09 +0000
@@ -0,0 +1,105 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package registry
+
+import (
+	"fmt"
+	"reflect"
+	"sort"
+
+	"github.com/juju/errors"
+)
+
+// TypedNameVersion is a registry that will allow you to register objects based
+// on a name and version pair. The objects must be convertible to the Type
+// defined when the registry was created. It will be cast during Register so
+// you can be sure all objects returned from Get() are safe to TypeAssert to
+// that type.
+type TypedNameVersion struct {
+	requiredType reflect.Type
+	versions     map[string]Versions
+}
+
+// NewTypedNameVersion creates a place to register your objects
+func NewTypedNameVersion(requiredType reflect.Type) *TypedNameVersion {
+	return &TypedNameVersion{
+		requiredType: requiredType,
+		versions:     make(map[string]Versions),
+	}
+}
+
+// Description gives the name and available versions in a registry.
+type Description struct {
+	Name     string
+	Versions []int
+}
+
+// Versions maps concrete versions of the objects.
+type Versions map[int]interface{}
+
+// Register records the factory that can be used to produce an instance of the
+// facade at the supplied version.
+// If the object being registered doesn't Implement the required Type, then an
+// error is returned.
+// An error is also returned if an object is already registered with the given
+// name and version.
+func (r *TypedNameVersion) Register(name string, version int, obj interface{}) error {
+	if !reflect.TypeOf(obj).ConvertibleTo(r.requiredType) {
+		return fmt.Errorf("object of type %T cannot be converted to type %s.%s", obj, r.requiredType.PkgPath(), r.requiredType.Name())
+	}
+	obj = reflect.ValueOf(obj).Convert(r.requiredType).Interface()
+	if r.versions == nil {
+		r.versions = make(map[string]Versions, 1)
+	}
+	if versions, ok := r.versions[name]; ok {
+		if _, ok := versions[version]; ok {
+			fullname := fmt.Sprintf("%s(%d)", name, version)
+			return fmt.Errorf("object %q already registered", fullname)
+		}
+		versions[version] = obj
+	} else {
+		r.versions[name] = Versions{version: obj}
+	}
+	return nil
+}
+
+// descriptionFromVersions aggregates the information in a Versions map into a
+// more friendly form for List()
+func descriptionFromVersions(name string, versions Versions) Description {
+	intVersions := make([]int, 0, len(versions))
+	for version := range versions {
+		intVersions = append(intVersions, version)
+	}
+	sort.Ints(intVersions)
+	return Description{
+		Name:     name,
+		Versions: intVersions,
+	}
+}
+
+// List returns a slice describing each of the registered Facades.
+func (r *TypedNameVersion) List() []Description {
+	names := make([]string, 0, len(r.versions))
+	for name := range r.versions {
+		names = append(names, name)
+	}
+	sort.Strings(names)
+	descriptions := make([]Description, len(r.versions))
+	for i, name := range names {
+		versions := r.versions[name]
+		descriptions[i] = descriptionFromVersions(name, versions)
+	}
+	return descriptions
+}
+
+// Get returns the object for a single name and version. If the requested
+// facade is not found, it returns error.NotFound
+func (r *TypedNameVersion) Get(name string, version int) (interface{}, error) {
+	if versions, ok := r.versions[name]; ok {
+		if factory, ok := versions[version]; ok {
+			return factory, nil
+		}
+	}
+	return nil, errors.NotFoundf("%s(%d)", name, version)
+}

=== added file 'utils/registry/registry_test.go'
--- utils/registry/registry_test.go	1970-01-01 00:00:00 +0000
+++ utils/registry/registry_test.go	2014-05-16 11:49:09 +0000
@@ -0,0 +1,151 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package registry_test
+
+import (
+	"reflect"
+
+	jc "github.com/juju/testing/checkers"
+	gc "launchpad.net/gocheck"
+
+	"github.com/juju/errors"
+	"launchpad.net/juju-core/testing/testbase"
+	"launchpad.net/juju-core/utils/registry"
+)
+
+type registrySuite struct {
+	testbase.LoggingSuite
+}
+
+var _ = gc.Suite(&registrySuite{})
+
+type Factory func() (interface{}, error)
+
+func nilFactory() (interface{}, error) {
+	return nil, nil
+}
+
+var factoryType = reflect.TypeOf((*Factory)(nil)).Elem()
+
+type testFacade struct {
+	version string
+	called  bool
+}
+
+type stringVal struct {
+	value string
+}
+
+func (t *testFacade) TestMethod() stringVal {
+	t.called = true
+	return stringVal{"called " + t.version}
+}
+
+func (s *registrySuite) TestDescriptionFromVersions(c *gc.C) {
+	versions := registry.Versions{0: nilFactory}
+	c.Check(registry.DescriptionFromVersions("name", versions),
+		gc.DeepEquals,
+		registry.Description{
+			Name:     "name",
+			Versions: []int{0},
+		})
+	versions[2] = nilFactory
+	c.Check(registry.DescriptionFromVersions("name", versions),
+		gc.DeepEquals,
+		registry.Description{
+			Name:     "name",
+			Versions: []int{0, 2},
+		})
+}
+
+func (s *registrySuite) TestDescriptionFromVersionsAreSorted(c *gc.C) {
+	versions := registry.Versions{
+		10: nilFactory,
+		5:  nilFactory,
+		0:  nilFactory,
+		18: nilFactory,
+		6:  nilFactory,
+		4:  nilFactory,
+	}
+	c.Check(registry.DescriptionFromVersions("name", versions),
+		gc.DeepEquals,
+		registry.Description{
+			Name:     "name",
+			Versions: []int{0, 4, 5, 6, 10, 18},
+		})
+}
+
+func (s *registrySuite) TestRegisterAndList(c *gc.C) {
+	r := registry.NewTypedNameVersion(factoryType)
+	c.Assert(r.Register("name", 0, nilFactory), gc.IsNil)
+	c.Check(r.List(), gc.DeepEquals, []registry.Description{
+		{Name: "name", Versions: []int{0}},
+	})
+}
+
+func (s *registrySuite) TestRegisterAndListMultiple(c *gc.C) {
+	r := registry.NewTypedNameVersion(factoryType)
+	c.Assert(r.Register("other", 0, nilFactory), gc.IsNil)
+	c.Assert(r.Register("name", 0, nilFactory), gc.IsNil)
+	c.Assert(r.Register("third", 2, nilFactory), gc.IsNil)
+	c.Check(r.List(), gc.DeepEquals, []registry.Description{
+		{Name: "name", Versions: []int{0}},
+		{Name: "other", Versions: []int{0}},
+		{Name: "third", Versions: []int{2}},
+	})
+}
+
+func (s *registrySuite) TestRegisterWrongType(c *gc.C) {
+	r := registry.NewTypedNameVersion(factoryType)
+	err := r.Register("other", 0, "notAFactory")
+	c.Check(err, gc.ErrorMatches, `object of type string cannot be converted to type .*registry_test.Factory`)
+}
+
+func (s *registrySuite) TestRegisterAlreadyPresent(c *gc.C) {
+	r := registry.NewTypedNameVersion(factoryType)
+	err := r.Register("name", 0, func() (interface{}, error) {
+		return "orig", nil
+	})
+	c.Assert(err, gc.IsNil)
+	err = r.Register("name", 0, func() (interface{}, error) {
+		return "broken", nil
+	})
+	c.Check(err, gc.ErrorMatches, `object "name\(0\)" already registered`)
+	f, err := r.Get("name", 0)
+	c.Assert(err, gc.IsNil)
+	val, err := f.(Factory)()
+	c.Assert(err, gc.IsNil)
+	c.Check(val, gc.Equals, "orig")
+}
+
+func (s *registrySuite) TestGet(c *gc.C) {
+	r := registry.NewTypedNameVersion(factoryType)
+	customFactory := func() (interface{}, error) {
+		return 10, nil
+	}
+	c.Assert(r.Register("name", 0, customFactory), gc.IsNil)
+	f, err := r.Get("name", 0)
+	c.Assert(err, gc.IsNil)
+	c.Assert(f, gc.NotNil)
+	res, err := f.(Factory)()
+	c.Assert(err, gc.IsNil)
+	c.Check(res, gc.Equals, 10)
+}
+
+func (s *registrySuite) TestGetUnknown(c *gc.C) {
+	r := registry.NewTypedNameVersion(factoryType)
+	f, err := r.Get("name", 0)
+	c.Check(err, jc.Satisfies, errors.IsNotFound)
+	c.Check(err, gc.ErrorMatches, `name\(0\) not found`)
+	c.Check(f, gc.IsNil)
+}
+
+func (s *registrySuite) TestGetUnknownVersion(c *gc.C) {
+	r := registry.NewTypedNameVersion(factoryType)
+	c.Assert(r.Register("name", 0, nilFactory), gc.IsNil)
+	f, err := r.Get("name", 1)
+	c.Check(err, jc.Satisfies, errors.IsNotFound)
+	c.Check(err, gc.ErrorMatches, `name\(1\) not found`)
+	c.Check(f, gc.IsNil)
+}