Merge lp:~jameinel/juju-core/api-register-standard-facade into lp:~go-bot/juju-core/trunk

[John A Meinel]

state/apiserver/common: RegisterStandardFacade

So my previous patch introduced a Registry that can be used for
registering and then looking up Facades. However, almost all of our
existing code has methods like:
  func NewKeyManagerAPI(
      st *state.State,
      resources *common.Resources,
      authorizer common.Authorizer,
  ) (*KeyManagerAPI, error) {

And that has the property that it is != o
  type FacadeFactory func(
      st *state.State, resources *Resources, authorizer Authorizer, id string,
  ) (interface{}, error)

Because the return type for the latter is interface{} and the former is
a concrete type (that implements that interface, of course).

I could think of 3 ways to work around this, and this seemed the most
tasteful.

1) Rewrite all of the New*API() functions to return interface{}, and
then rewrite all of their tests to cast it back to the concrete type. I
like that New* returns a concrete type, though, so I'd rather keep that.

2) Wrap all of the calls to RegisterFacade with a bespoke func() that
just does the type currying, then all of the init() functions looked
like:

  func init() {
    common.RegisterFacade("KeyManager", 0, func(
          st *state.State, resources *Resources, authorizer Authorizer, id string,
   ) (interface{}, error) {
     if id != "" {
       return nil, common.ErrBadId
     }
     return NewKeyManagerAPI(st, resources, authorizer), nil
   })
  }

That isn't terrible, but it is a lot of boilerplate to write.

3) Use reflect to create the boilerplate functions. Then all of the
actual init lines just become:

  func init() {
      common.RegisterStandardFacade("KeyManager", 0, NewKeyManagerAPI)
  }

So at the cost of having RegisterStandardFacade have to do reflect
introspection, it gives us really nice syntax when we want to add more.
(And note that when we have multiple versions of Facades, (2) would have
to have a bespoke func() for each one.)

I could go back to (2) if people don't like the reflect magic, but I
think it does make things nicer overall.

https://codereview.appspot.com/99290043/

[John A Meinel (jameinel)]

Reviewers: mp+219660_code.launchpad.net,

Message:
Please take a look.

Description:
state/apiserver/common: RegisterStandardFacade

So my previous patch introduced a Registry that can be used for
registering and then looking up Facades. However, almost all of our
existing code has methods like:
   func NewKeyManagerAPI(
       st *state.State,
       resources *common.Resources,
       authorizer common.Authorizer,
   ) (*KeyManagerAPI, error) {

And that has the property that it is != o
   type FacadeFactory func(
       st *state.State, resources *Resources, authorizer Authorizer, id
string,
   ) (interface{}, error)

Because the return type for the latter is interface{} and the former is
a concrete type (that implements that interface, of course).

I could think of 3 ways to work around this, and this seemed the most
tasteful.

1) Rewrite all of the New*API() functions to return interface{}, and
then rewrite all of their tests to cast it back to the concrete type. I
like that New* returns a concrete type, though, so I'd rather keep that.

2) Wrap all of the calls to RegisterFacade with a bespoke func() that
just does the type currying, then all of the init() functions looked
like:

   func init() {
     common.RegisterFacade("KeyManager", 0, func(
           st *state.State, resources *Resources, authorizer Authorizer,
id string,
    ) (interface{}, error) {
     if id != "" {
       return nil, common.ErrBadId
     }
     return NewKeyManagerAPI(st, resources, authorizer), nil
    })
   }

That isn't terrible, but it is a lot of boilerplate to write.

3) Use reflect to create the boilerplate functions. Then all of the
actual init lines just become:

   func init() {
       common.RegisterStandardFacade("KeyManager", 0, NewKeyManagerAPI)
   }

So at the cost of having RegisterStandardFacade have to do reflect
introspection, it gives us really nice syntax when we want to add more.
(And note that when we have multiple versions of Facades, (2) would have
to have a bespoke func() for each one.)

I could go back to (2) if people don't like the reflect magic, but I
think it does make things nicer overall.

https://code.launchpad.net/~jameinel/juju-core/api-register-standard-facade/+merge/219660

Requires:
https://code.launchpad.net/~jameinel/juju-core/api-facade-registry/+merge/219654

(do not edit description out of merge proposal)

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

Affected files (+211, -0 lines):
   A [revision details]
   A state/apiserver/common/export_test.go
   M state/apiserver/common/registry.go
   M state/apiserver/common/registry_test.go

[William Reade (fwereade)]

LGTM. The yuck of the reflection is more than balanced by the squee of
the external interface.

https://codereview.appspot.com/99290043/

[lp:~jameinel/juju-core/api-register-standard-facade]

2726. By John A Meinel on 2014-05-16

Merged api-facade-registry into api-register-standard-facade.

2727. By John A Meinel on 2014-05-16

Merged api-facade-registry into api-register-standard-facade.

[John A Meinel (jameinel)]

Please take a look.

https://codereview.appspot.com/99290043/

[lp:~jameinel/juju-core/api-register-standard-facade]

2728. By John A Meinel on 2014-05-16

merge api-facade-registry to get updated trunk and github.com/juju/errors

2729. By John A Meinel on 2014-05-20

Merged api-facade-registry into api-register-standard-facade.

2730. By John A Meinel on 2014-05-22

Merged api-facade-registry into api-register-standard-facade.

Unmerged revisions

2730. By John A Meinel on 2014-05-22

Merged api-facade-registry into api-register-standard-facade.

2729. By John A Meinel on 2014-05-20

Merged api-facade-registry into api-register-standard-facade.

2728. By John A Meinel on 2014-05-16

merge api-facade-registry to get updated trunk and github.com/juju/errors

2727. By John A Meinel on 2014-05-16

Merged api-facade-registry into api-register-standard-facade.

2726. By John A Meinel on 2014-05-16

Merged api-facade-registry into api-register-standard-facade.

2725. By John A Meinel on 2014-05-15

merge deletion of ServeCaller

2724. By John A Meinel on 2014-05-14

Merge up the changes and resolve the conflicts

2723. By John A Meinel on 2014-05-14

merge in just the RegisterStandardFacade helpers.

2722. By John A Meinel on 2014-05-14

merge describe-api-versions but strip things down to just having the registry.

2721. By John A Meinel on 2014-05-14

merge in rpc-version.

This gives us all of the functionality for passing Version in the RPC,
and changes the MethodCaller signature so that it can take the version
and the objId.

=== added file 'state/apiserver/common/export_test.go'
--- state/apiserver/common/export_test.go	1970-01-01 00:00:00 +0000
+++ state/apiserver/common/export_test.go	2014-05-22 12:03:12 +0000
@@ -0,0 +1,9 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package common
+
+var (
+	ValidateNewFacade = validateNewFacade
+	WrapNewFacade     = wrapNewFacade
+)
 
=== modified file 'state/apiserver/common/registry.go'
--- state/apiserver/common/registry.go	2014-05-22 12:03:11 +0000
+++ state/apiserver/common/registry.go	2014-05-22 12:03:12 +0000
@@ -4,7 +4,9 @@
 package common
 
 import (
+	"fmt"
 	"reflect"
+	"runtime"
 
 	"github.com/juju/errors"
 
@@ -31,6 +33,89 @@
 	}
 }
 
+// validateNewFacade ensures that the facade factory we have has the right
+// input and output parameters for being used as a NewFoo function.
+func validateNewFacade(funcValue reflect.Value) error {
+	if !funcValue.IsValid() {
+		return fmt.Errorf("cannot wrap nil")
+	}
+	if funcValue.Kind() != reflect.Func {
+		return fmt.Errorf("wrong type %q is not a function", funcValue.Kind())
+	}
+	funcType := funcValue.Type()
+	funcName := runtime.FuncForPC(funcValue.Pointer()).Name()
+	if funcType.NumIn() != 3 || funcType.NumOut() != 2 {
+		return fmt.Errorf("function %q does not take 3 parameters and return 2",
+			funcName)
+	}
+	facadeType := reflect.TypeOf((*FacadeFactory)(nil)).Elem()
+	isSame := true
+	for i := 0; i < 3; i++ {
+		if funcType.In(i) != facadeType.In(i) {
+			isSame = false
+			break
+		}
+	}
+	if funcType.Out(1) != facadeType.Out(1) {
+		isSame = false
+	}
+	if !isSame {
+		return fmt.Errorf("function %q does not have the signature func (*state.State, *common.Resources, common.Authorizer) (*Type, error)",
+			funcName)
+	}
+	return nil
+}
+
+// wrapNewFacade turns a given NewFoo(st, resources, authorizer) (*Instance, error)
+// function and wraps it into a proper FacadeFactory function.
+func wrapNewFacade(newFunc interface{}) (FacadeFactory, error) {
+	funcValue := reflect.ValueOf(newFunc)
+	err := validateNewFacade(funcValue)
+	if err != nil {
+		return nil, err
+	}
+	// So we know newFunc is a func with the right args in and out, so
+	// wrap it into a helper function that matches the FacadeFactory.
+	wrapped := func(
+		st *state.State, resources *Resources, auth Authorizer, id string,
+	) (
+		interface{}, error,
+	) {
+		if id != "" {
+			return nil, ErrBadId
+		}
+		// st, resources, or auth is nil, then reflect.Call dies
+		// because reflect.ValueOf(anynil) is the Zero Value.
+		// So we use &obj.Elem() which gives us a concrete Value object
+		// that can refer to nil.
+		in := []reflect.Value{
+			reflect.ValueOf(&st).Elem(),
+			reflect.ValueOf(&resources).Elem(),
+			reflect.ValueOf(&auth).Elem(),
+		}
+		out := funcValue.Call(in)
+		if out[1].Interface() != nil {
+			err := out[1].Interface().(error)
+			return nil, err
+		}
+		return out[0].Interface(), nil
+	}
+	return wrapped, nil
+}
+
+// RegisterStandardFacade registers a factory function for a normal New* style
+// function. This requires that the function has the form:
+// NewFoo(*state.State, *common.Resources, common.Authorizer) (*Type, error)
+// With that syntax, we will create a helper function that wraps calling NewFoo
+// with the right parameters, and returns the *Type correctly.
+func RegisterStandardFacade(name string, version int, newFunc interface{}) {
+	wrapped, err := wrapNewFacade(newFunc)
+	if err != nil {
+		panic(err)
+	}
+	RegisterFacade(name, version, wrapped)
+}
+
 func GetFacade(name string, version int) (FacadeFactory, error) {
 	f, err := Facades.Get(name, version)
 	if err != nil {
 
=== modified file 'state/apiserver/common/registry_test.go'
--- state/apiserver/common/registry_test.go	2014-05-22 12:03:11 +0000
+++ state/apiserver/common/registry_test.go	2014-05-22 12:03:12 +0000
@@ -11,6 +11,7 @@
 	"launchpad.net/juju-core/rpc/rpcreflect"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/apiserver/common"
+	apiservertesting "launchpad.net/juju-core/state/apiserver/testing"
 	"launchpad.net/juju-core/testing/testbase"
 	"launchpad.net/juju-core/utils/registry"
 )
@@ -75,6 +76,120 @@
 		gc.PanicMatches, `object "myfacade\(0\)" already registered`)
 }
 
+func checkValidateNewFacadeFailsWith(c *gc.C, obj interface{}, errMsg string) {
+	err := common.ValidateNewFacade(reflect.ValueOf(obj))
+	c.Check(err, gc.NotNil)
+	c.Check(err, gc.ErrorMatches, errMsg)
+}
+
+func noArgs() {
+}
+
+func badCountIn(a string) (*int, error) {
+	return nil, nil
+}
+
+func badCountOut(a, b, c string) error {
+	return nil
+}
+
+func wrongIn(a, b, c string) (*int, error) {
+	return nil, nil
+}
+
+func wrongOut(*state.State, *common.Resources, common.Authorizer) (error, *int) {
+	return nil, nil
+}
+
+func validFactory(*state.State, *common.Resources, common.Authorizer) (*int, error) {
+	var i = 100
+	return &i, nil
+}
+
+func (*facadeRegistrySuite) TestValidateNewFacade(c *gc.C) {
+	checkValidateNewFacadeFailsWith(c, nil,
+		`cannot wrap nil`)
+	checkValidateNewFacadeFailsWith(c, "notafunc",
+		`wrong type "string" is not a function`)
+	checkValidateNewFacadeFailsWith(c, noArgs,
+		`function ".*noArgs" does not take 3 parameters and return 2`)
+	checkValidateNewFacadeFailsWith(c, badCountIn,
+		`function ".*badCountIn" does not take 3 parameters and return 2`)
+	checkValidateNewFacadeFailsWith(c, badCountOut,
+		`function ".*badCountOut" does not take 3 parameters and return 2`)
+	checkValidateNewFacadeFailsWith(c, wrongIn,
+		`function ".*wrongIn" does not have the signature func \(\*state.State, \*common.Resources, common.Authorizer\) \(\*Type, error\)`)
+	checkValidateNewFacadeFailsWith(c, wrongOut,
+		`function ".*wrongOut" does not have the signature func \(\*state.State, \*common.Resources, common.Authorizer\) \(\*Type, error\)`)
+	err := common.ValidateNewFacade(reflect.ValueOf(validFactory))
+	c.Assert(err, gc.IsNil)
+}
+
+func (*facadeRegistrySuite) TestWrapNewFacadeFailure(c *gc.C) {
+	_, err := common.WrapNewFacade("notafunc")
+	c.Check(err, gc.ErrorMatches, `wrong type "string" is not a function`)
+}
+
+func (*facadeRegistrySuite) TestWrapNewFacadeHandlesId(c *gc.C) {
+	wrapped, err := common.WrapNewFacade(validFactory)
+	c.Assert(err, gc.IsNil)
+	val, err := wrapped(nil, nil, nil, "badId")
+	c.Check(err, gc.ErrorMatches, "id not found")
+	c.Check(val, gc.Equals, nil)
+}
+
+func (*facadeRegistrySuite) TestWrapNewFacadeCallsFunc(c *gc.C) {
+	wrapped, err := common.WrapNewFacade(validFactory)
+	c.Assert(err, gc.IsNil)
+	val, err := wrapped(nil, nil, nil, "")
+	c.Assert(err, gc.IsNil)
+	c.Check(*(val.(*int)), gc.Equals, 100)
+}
+
+type myResult struct {
+	st        *state.State
+	resources *common.Resources
+	auth      common.Authorizer
+}
+
+func (*facadeRegistrySuite) TestWrapNewFacadeCallsWithRightParams(c *gc.C) {
+	authorizer := apiservertesting.FakeAuthorizer{}
+	resources := common.NewResources()
+	testFunc := func(
+		st *state.State, resources *common.Resources,
+		authorizer common.Authorizer,
+	) (*myResult, error) {
+		return &myResult{st, resources, authorizer}, nil
+	}
+	wrapped, err := common.WrapNewFacade(testFunc)
+	val, err := wrapped(nil, resources, authorizer, "")
+	c.Assert(err, gc.IsNil)
+	asResult := val.(*myResult)
+	c.Check(asResult.st, gc.IsNil)
+	c.Check(asResult.resources, gc.Equals, resources)
+	c.Check(asResult.auth, gc.Equals, authorizer)
+}
+
+func (r *facadeRegistrySuite) TestRegisterStandardFacade(c *gc.C) {
+	r.sanitizeFacades()
+	common.RegisterStandardFacade("testing", 0, validFactory)
+	wrapped, err := common.GetFacade("testing", 0)
+	c.Assert(err, gc.IsNil)
+	val, err := wrapped(nil, nil, nil, "")
+	c.Assert(err, gc.IsNil)
+	c.Check(*(val.(*int)), gc.Equals, 100)
+}
+
+func (r *facadeRegistrySuite) TestRegisterStandardFacadePanic(c *gc.C) {
+	r.sanitizeFacades()
+	c.Assert(
+		func() { common.RegisterStandardFacade("badtest", 0, noArgs) },
+		gc.PanicMatches,
+		`function ".*noArgs" does not take 3 parameters and return 2`)
+	_, err := common.GetFacade("badtest", 0)
+	c.Assert(err, gc.FitsTypeOf, (*rpcreflect.CallNotImplementedError)(nil))
+	c.Assert(err, gc.ErrorMatches, `unknown object type "badtest"`)
+}
 func (*facadeRegistrySuite) TestDiscardedAPIMethods(c *gc.C) {
 	allFacades := common.Facades.List()
 	c.Assert(allFacades, gc.HasLen, 0)