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

Proposed by John A Meinel on 2014-05-15
Status: Work in progress
Proposed branch: lp:~jameinel/juju-core/api-register-standard-facade
Merge into: lp:~go-bot/juju-core/trunk
Prerequisite: lp:~jameinel/juju-core/api-facade-registry
Diff against target: 250 lines (+209/-0)
3 files modified
state/apiserver/common/export_test.go (+9/-0)
state/apiserver/common/registry.go (+85/-0)
state/apiserver/common/registry_test.go (+115/-0)
To merge this branch: bzr merge lp:~jameinel/juju-core/api-register-standard-facade
Reviewer Review Type Date Requested Status
Juju Engineering 2014-05-15 Pending
Review via email: mp+219660@code.launchpad.net

Description of the change

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/

To post a comment you must log in.
John A Meinel (jameinel) wrote :

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) wrote :

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

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

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.

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'state/apiserver/common/export_test.go'
2--- state/apiserver/common/export_test.go 1970-01-01 00:00:00 +0000
3+++ state/apiserver/common/export_test.go 2014-05-22 12:03:12 +0000
4@@ -0,0 +1,9 @@
5+// Copyright 2014 Canonical Ltd.
6+// Licensed under the AGPLv3, see LICENCE file for details.
7+
8+package common
9+
10+var (
11+ ValidateNewFacade = validateNewFacade
12+ WrapNewFacade = wrapNewFacade
13+)
14
15=== modified file 'state/apiserver/common/registry.go'
16--- state/apiserver/common/registry.go 2014-05-22 12:03:11 +0000
17+++ state/apiserver/common/registry.go 2014-05-22 12:03:12 +0000
18@@ -4,7 +4,9 @@
19 package common
20
21 import (
22+ "fmt"
23 "reflect"
24+ "runtime"
25
26 "github.com/juju/errors"
27
28@@ -31,6 +33,89 @@
29 }
30 }
31
32+// validateNewFacade ensures that the facade factory we have has the right
33+// input and output parameters for being used as a NewFoo function.
34+func validateNewFacade(funcValue reflect.Value) error {
35+ if !funcValue.IsValid() {
36+ return fmt.Errorf("cannot wrap nil")
37+ }
38+ if funcValue.Kind() != reflect.Func {
39+ return fmt.Errorf("wrong type %q is not a function", funcValue.Kind())
40+ }
41+ funcType := funcValue.Type()
42+ funcName := runtime.FuncForPC(funcValue.Pointer()).Name()
43+ if funcType.NumIn() != 3 || funcType.NumOut() != 2 {
44+ return fmt.Errorf("function %q does not take 3 parameters and return 2",
45+ funcName)
46+ }
47+ facadeType := reflect.TypeOf((*FacadeFactory)(nil)).Elem()
48+ isSame := true
49+ for i := 0; i < 3; i++ {
50+ if funcType.In(i) != facadeType.In(i) {
51+ isSame = false
52+ break
53+ }
54+ }
55+ if funcType.Out(1) != facadeType.Out(1) {
56+ isSame = false
57+ }
58+ if !isSame {
59+ return fmt.Errorf("function %q does not have the signature func (*state.State, *common.Resources, common.Authorizer) (*Type, error)",
60+ funcName)
61+ }
62+ return nil
63+}
64+
65+// wrapNewFacade turns a given NewFoo(st, resources, authorizer) (*Instance, error)
66+// function and wraps it into a proper FacadeFactory function.
67+func wrapNewFacade(newFunc interface{}) (FacadeFactory, error) {
68+ funcValue := reflect.ValueOf(newFunc)
69+ err := validateNewFacade(funcValue)
70+ if err != nil {
71+ return nil, err
72+ }
73+ // So we know newFunc is a func with the right args in and out, so
74+ // wrap it into a helper function that matches the FacadeFactory.
75+ wrapped := func(
76+ st *state.State, resources *Resources, auth Authorizer, id string,
77+ ) (
78+ interface{}, error,
79+ ) {
80+ if id != "" {
81+ return nil, ErrBadId
82+ }
83+ // st, resources, or auth is nil, then reflect.Call dies
84+ // because reflect.ValueOf(anynil) is the Zero Value.
85+ // So we use &obj.Elem() which gives us a concrete Value object
86+ // that can refer to nil.
87+ in := []reflect.Value{
88+ reflect.ValueOf(&st).Elem(),
89+ reflect.ValueOf(&resources).Elem(),
90+ reflect.ValueOf(&auth).Elem(),
91+ }
92+ out := funcValue.Call(in)
93+ if out[1].Interface() != nil {
94+ err := out[1].Interface().(error)
95+ return nil, err
96+ }
97+ return out[0].Interface(), nil
98+ }
99+ return wrapped, nil
100+}
101+
102+// RegisterStandardFacade registers a factory function for a normal New* style
103+// function. This requires that the function has the form:
104+// NewFoo(*state.State, *common.Resources, common.Authorizer) (*Type, error)
105+// With that syntax, we will create a helper function that wraps calling NewFoo
106+// with the right parameters, and returns the *Type correctly.
107+func RegisterStandardFacade(name string, version int, newFunc interface{}) {
108+ wrapped, err := wrapNewFacade(newFunc)
109+ if err != nil {
110+ panic(err)
111+ }
112+ RegisterFacade(name, version, wrapped)
113+}
114+
115 func GetFacade(name string, version int) (FacadeFactory, error) {
116 f, err := Facades.Get(name, version)
117 if err != nil {
118
119=== modified file 'state/apiserver/common/registry_test.go'
120--- state/apiserver/common/registry_test.go 2014-05-22 12:03:11 +0000
121+++ state/apiserver/common/registry_test.go 2014-05-22 12:03:12 +0000
122@@ -11,6 +11,7 @@
123 "launchpad.net/juju-core/rpc/rpcreflect"
124 "launchpad.net/juju-core/state"
125 "launchpad.net/juju-core/state/apiserver/common"
126+ apiservertesting "launchpad.net/juju-core/state/apiserver/testing"
127 "launchpad.net/juju-core/testing/testbase"
128 "launchpad.net/juju-core/utils/registry"
129 )
130@@ -75,6 +76,120 @@
131 gc.PanicMatches, `object "myfacade\(0\)" already registered`)
132 }
133
134+func checkValidateNewFacadeFailsWith(c *gc.C, obj interface{}, errMsg string) {
135+ err := common.ValidateNewFacade(reflect.ValueOf(obj))
136+ c.Check(err, gc.NotNil)
137+ c.Check(err, gc.ErrorMatches, errMsg)
138+}
139+
140+func noArgs() {
141+}
142+
143+func badCountIn(a string) (*int, error) {
144+ return nil, nil
145+}
146+
147+func badCountOut(a, b, c string) error {
148+ return nil
149+}
150+
151+func wrongIn(a, b, c string) (*int, error) {
152+ return nil, nil
153+}
154+
155+func wrongOut(*state.State, *common.Resources, common.Authorizer) (error, *int) {
156+ return nil, nil
157+}
158+
159+func validFactory(*state.State, *common.Resources, common.Authorizer) (*int, error) {
160+ var i = 100
161+ return &i, nil
162+}
163+
164+func (*facadeRegistrySuite) TestValidateNewFacade(c *gc.C) {
165+ checkValidateNewFacadeFailsWith(c, nil,
166+ `cannot wrap nil`)
167+ checkValidateNewFacadeFailsWith(c, "notafunc",
168+ `wrong type "string" is not a function`)
169+ checkValidateNewFacadeFailsWith(c, noArgs,
170+ `function ".*noArgs" does not take 3 parameters and return 2`)
171+ checkValidateNewFacadeFailsWith(c, badCountIn,
172+ `function ".*badCountIn" does not take 3 parameters and return 2`)
173+ checkValidateNewFacadeFailsWith(c, badCountOut,
174+ `function ".*badCountOut" does not take 3 parameters and return 2`)
175+ checkValidateNewFacadeFailsWith(c, wrongIn,
176+ `function ".*wrongIn" does not have the signature func \(\*state.State, \*common.Resources, common.Authorizer\) \(\*Type, error\)`)
177+ checkValidateNewFacadeFailsWith(c, wrongOut,
178+ `function ".*wrongOut" does not have the signature func \(\*state.State, \*common.Resources, common.Authorizer\) \(\*Type, error\)`)
179+ err := common.ValidateNewFacade(reflect.ValueOf(validFactory))
180+ c.Assert(err, gc.IsNil)
181+}
182+
183+func (*facadeRegistrySuite) TestWrapNewFacadeFailure(c *gc.C) {
184+ _, err := common.WrapNewFacade("notafunc")
185+ c.Check(err, gc.ErrorMatches, `wrong type "string" is not a function`)
186+}
187+
188+func (*facadeRegistrySuite) TestWrapNewFacadeHandlesId(c *gc.C) {
189+ wrapped, err := common.WrapNewFacade(validFactory)
190+ c.Assert(err, gc.IsNil)
191+ val, err := wrapped(nil, nil, nil, "badId")
192+ c.Check(err, gc.ErrorMatches, "id not found")
193+ c.Check(val, gc.Equals, nil)
194+}
195+
196+func (*facadeRegistrySuite) TestWrapNewFacadeCallsFunc(c *gc.C) {
197+ wrapped, err := common.WrapNewFacade(validFactory)
198+ c.Assert(err, gc.IsNil)
199+ val, err := wrapped(nil, nil, nil, "")
200+ c.Assert(err, gc.IsNil)
201+ c.Check(*(val.(*int)), gc.Equals, 100)
202+}
203+
204+type myResult struct {
205+ st *state.State
206+ resources *common.Resources
207+ auth common.Authorizer
208+}
209+
210+func (*facadeRegistrySuite) TestWrapNewFacadeCallsWithRightParams(c *gc.C) {
211+ authorizer := apiservertesting.FakeAuthorizer{}
212+ resources := common.NewResources()
213+ testFunc := func(
214+ st *state.State, resources *common.Resources,
215+ authorizer common.Authorizer,
216+ ) (*myResult, error) {
217+ return &myResult{st, resources, authorizer}, nil
218+ }
219+ wrapped, err := common.WrapNewFacade(testFunc)
220+ val, err := wrapped(nil, resources, authorizer, "")
221+ c.Assert(err, gc.IsNil)
222+ asResult := val.(*myResult)
223+ c.Check(asResult.st, gc.IsNil)
224+ c.Check(asResult.resources, gc.Equals, resources)
225+ c.Check(asResult.auth, gc.Equals, authorizer)
226+}
227+
228+func (r *facadeRegistrySuite) TestRegisterStandardFacade(c *gc.C) {
229+ r.sanitizeFacades()
230+ common.RegisterStandardFacade("testing", 0, validFactory)
231+ wrapped, err := common.GetFacade("testing", 0)
232+ c.Assert(err, gc.IsNil)
233+ val, err := wrapped(nil, nil, nil, "")
234+ c.Assert(err, gc.IsNil)
235+ c.Check(*(val.(*int)), gc.Equals, 100)
236+}
237+
238+func (r *facadeRegistrySuite) TestRegisterStandardFacadePanic(c *gc.C) {
239+ r.sanitizeFacades()
240+ c.Assert(
241+ func() { common.RegisterStandardFacade("badtest", 0, noArgs) },
242+ gc.PanicMatches,
243+ `function ".*noArgs" does not take 3 parameters and return 2`)
244+ _, err := common.GetFacade("badtest", 0)
245+ c.Assert(err, gc.FitsTypeOf, (*rpcreflect.CallNotImplementedError)(nil))
246+ c.Assert(err, gc.ErrorMatches, `unknown object type "badtest"`)
247+}
248 func (*facadeRegistrySuite) TestDiscardedAPIMethods(c *gc.C) {
249 allFacades := common.Facades.List()
250 c.Assert(allFacades, gc.HasLen, 0)

Subscribers

People subscribed via source and target branches

to status/vote changes: