juju-core

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

  1. api-facade-registry
  2. Merge into trunk
Proposed by John A Meinel
Status: Work in progress
Proposed branch: lp:~jameinel/juju-core/api-facade-registry
Merge into: lp:~go-bot/juju-core/trunk
Prerequisite: lp:~jameinel/juju-core/api-rpc-reflect-version
Diff against target: 142 lines (+133/-0)
2 files modified
state/apiserver/common/registry.go (+52/-0)
state/apiserver/common/registry_test.go (+81/-0)
To merge this branch: bzr merge lp:~jameinel/juju-core/api-facade-registry
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+219654@code.launchpad.net

Description of the change

state/apiserver/common: Add Facades registry

This adds a registry.TypedNameVersion registry that the API Facades can
register themselves with so that srvRoot can return them from a custom
MethodCaller implementation.

This sets up what the actual interface for Facades will be:

  type FacadeFactory func(
   st *state.State, resources *Resources, authorizer Authorizer, id string,
  ) (
   interface{}, error,
  )

It doesn't start actually registering the Facades. It also doesn't
implement srvRoot.MethodCaller in this patch.

https://codereview.appspot.com/97490044/

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

Reviewers: mp+219654_code.launchpad.net,

Message:
Please take a look.

Description:
state/apiserver/common: Add Facades registry

This adds a registry.TypedNameVersion registry that the API Facades can
register themselves with so that srvRoot can return them from a custom
MethodCaller implementation.

This sets up what the actual interface for Facades will be:

   type FacadeFactory func(
    st *state.State, resources *Resources, authorizer Authorizer, id
string,
   ) (
    interface{}, error,
   )

It doesn't start actually registering the Facades. It also doesn't
implement srvRoot.MethodCaller in this patch.

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

Requires:
https://code.launchpad.net/~jameinel/juju-core/api-rpc-reflect-version/+merge/219653

(do not edit description out of merge proposal)

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

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

Reply
Revision history for this message
William Reade (fwereade) wrote :

LGTM with trivials

https://codereview.appspot.com/97490044/diff/1/state/apiserver/common/registry.go
File state/apiserver/common/registry.go (right):

https://codereview.appspot.com/97490044/diff/1/state/apiserver/common/registry.go#newcode17
state/apiserver/common/registry.go:17: // connection to the State
Full stops please :).

https://codereview.appspot.com/97490044/diff/1/state/apiserver/common/registry_test.go
File state/apiserver/common/registry_test.go (right):

https://codereview.appspot.com/97490044/diff/1/state/apiserver/common/registry_test.go#newcode34
state/apiserver/common/registry_test.go:34: s.AddCleanup(func(c *gc.C) {
common.Facades = oldFacades })
s.PatchValue?

https://codereview.appspot.com/97490044/diff/1/state/apiserver/common/registry_test.go#newcode69
state/apiserver/common/registry_test.go:69: // proper CodeNotImplemented
agreed :)

also, induce an error and check it panics?

https://codereview.appspot.com/97490044/

Reply
lp:~jameinel/juju-core/api-facade-registry updated
2725. By John A Meinel

Merged api-rpc-reflect-version into api-facade-registry.

2726. By John A Meinel

respond to review feedback

use PatchValue and put full stops in comments.

Revision history for this message
John A Meinel (jameinel) wrote :

Please take a look.

https://codereview.appspot.com/97490044/diff/1/state/apiserver/common/registry.go
File state/apiserver/common/registry.go (right):

https://codereview.appspot.com/97490044/diff/1/state/apiserver/common/registry.go#newcode17
state/apiserver/common/registry.go:17: // connection to the State
On 2014/05/16 09:30:48, fwereade wrote:
> Full stops please :).

Done.

https://codereview.appspot.com/97490044/diff/1/state/apiserver/common/registry_test.go
File state/apiserver/common/registry_test.go (right):

https://codereview.appspot.com/97490044/diff/1/state/apiserver/common/registry_test.go#newcode34
state/apiserver/common/registry_test.go:34: s.AddCleanup(func(c *gc.C) {
common.Facades = oldFacades })
On 2014/05/16 09:30:48, fwereade wrote:
> s.PatchValue?

Done.

https://codereview.appspot.com/97490044/diff/1/state/apiserver/common/registry_test.go#newcode69
state/apiserver/common/registry_test.go:69: // proper CodeNotImplemented
On 2014/05/16 09:30:48, fwereade wrote:
> agreed :)

> also, induce an error and check it panics?

right. At this level I do have the tests that we are returning
CodeNotImplementedError (only because I got lucky in breaking one of the
calls and noticing that it was returning ErrNotFound instead).

And I believe we have decent coverage at the RPC layer, but I think
we're missing something probably in root_test.go about trying to call a
Facade that isn't there, and eventually trying to call a Facade+Version
that isn't there.

I did add the test for panic(). I have one in the next layer up for
RegisterStandardFacade. But it is good to have this layer as well,
thanks.

https://codereview.appspot.com/97490044/

Reply
lp:~jameinel/juju-core/api-facade-registry updated
2727. By John A Meinel

merge api-rpc-reflect-version including trunk and update to github.com/juju/errors

2728. By John A Meinel

Merged api-rpc-reflect-version into api-facade-registry.

2729. By John A Meinel

Merge codereview cleanups from api-rpc-reflect-version

Unmerged revisions

2729. By John A Meinel

Merge codereview cleanups from api-rpc-reflect-version

2728. By John A Meinel

Merged api-rpc-reflect-version into api-facade-registry.

2727. By John A Meinel

merge api-rpc-reflect-version including trunk and update to github.com/juju/errors

2726. By John A Meinel

respond to review feedback

use PatchValue and put full stops in comments.

2725. By John A Meinel

Merged api-rpc-reflect-version into api-facade-registry.

2724. By John A Meinel

we don't have MethodCaller, thus we don't need to pretend it is there

2723. By John A Meinel

the common Registry type returns errors.NotFound, but we want them to be CodeNotImplemented.

2722. By John A Meinel

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

2721. By John A Meinel

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/registry.go'
2--- state/apiserver/common/registry.go 1970-01-01 00:00:00 +0000
3+++ state/apiserver/common/registry.go 2014-05-22 11:43:36 +0000
4@@ -0,0 +1,52 @@
5+// Copyright 2014 Canonical Ltd.
6+// Licensed under the AGPLv3, see LICENCE file for details.
7+
8+package common
9+
10+import (
11+ "reflect"
12+
13+ "github.com/juju/errors"
14+
15+ "launchpad.net/juju-core/rpc/rpcreflect"
16+ "launchpad.net/juju-core/state"
17+ "launchpad.net/juju-core/utils/registry"
18+)
19+
20+// FacadeFactory represent a way of creating a Facade from the current
21+// connection to the State.
22+type FacadeFactory func(
23+ st *state.State, resources *Resources, authorizer Authorizer, id string,
24+) (
25+ interface{}, error,
26+)
27+
28+// RegisterFacade updates the global facade registry with a new version of a new type.
29+func RegisterFacade(name string, version int, factory FacadeFactory) {
30+ err := Facades.Register(name, version, factory)
31+ if err != nil {
32+ // This is meant to be called during init() so errors should be
33+ // considered fatal.
34+ panic(err)
35+ }
36+}
37+
38+func GetFacade(name string, version int) (FacadeFactory, error) {
39+ f, err := Facades.Get(name, version)
40+ if err != nil {
41+ if errors.IsNotFound(err) {
42+ return nil, &rpcreflect.CallNotImplementedError{
43+ RootMethod: name,
44+ Version: version,
45+ }
46+ }
47+ return nil, err
48+ }
49+ // We don't need to check the type because registry.TypedNameVersion
50+ // should ensure we have a proper FacadeFactory.
51+ return f.(FacadeFactory), nil
52+}
53+
54+var facadeFactoryType = reflect.TypeOf((*FacadeFactory)(nil)).Elem()
55+
56+var Facades = registry.NewTypedNameVersion(facadeFactoryType)
57
58=== added file 'state/apiserver/common/registry_test.go'
59--- state/apiserver/common/registry_test.go 1970-01-01 00:00:00 +0000
60+++ state/apiserver/common/registry_test.go 2014-05-22 11:43:36 +0000
61@@ -0,0 +1,81 @@
62+// Copyright 2014 Canonical Ltd.
63+// Licensed under the AGPLv3, see LICENCE file for details.
64+
65+package common_test
66+
67+import (
68+ "reflect"
69+
70+ gc "launchpad.net/gocheck"
71+
72+ "launchpad.net/juju-core/rpc/rpcreflect"
73+ "launchpad.net/juju-core/state"
74+ "launchpad.net/juju-core/state/apiserver/common"
75+ "launchpad.net/juju-core/testing/testbase"
76+ "launchpad.net/juju-core/utils/registry"
77+)
78+
79+type facadeRegistrySuite struct {
80+ testbase.LoggingSuite
81+}
82+
83+var _ = gc.Suite(&facadeRegistrySuite{})
84+
85+func testFacade(
86+ st *state.State, resources *common.Resources,
87+ authorizer common.Authorizer, id string,
88+) (interface{}, error) {
89+ return "myobject", nil
90+}
91+
92+func (s *facadeRegistrySuite) sanitizeFacades() {
93+ // reset common.Facades so that we don't mutate the officially
94+ // registered Facades.
95+
96+ // a raw type isn't an expression, so we have to play tricks.
97+ factoryType := reflect.TypeOf((*common.FacadeFactory)(nil)).Elem()
98+ emptyFacades := registry.NewTypedNameVersion(factoryType)
99+ s.PatchValue(&common.Facades, emptyFacades)
100+}
101+
102+func (s *facadeRegistrySuite) TestRegister(c *gc.C) {
103+ s.sanitizeFacades()
104+ common.RegisterFacade("myfacade", 0, testFacade)
105+ f, err := common.GetFacade("myfacade", 0)
106+ c.Assert(err, gc.IsNil)
107+ val, err := f(nil, nil, nil, "")
108+ c.Assert(err, gc.IsNil)
109+ c.Check(val, gc.Equals, "myobject")
110+}
111+
112+func (s *facadeRegistrySuite) TestGetFacadeBadName(c *gc.C) {
113+ s.sanitizeFacades()
114+ common.RegisterFacade("myfacade", 0, testFacade)
115+ _, err := common.GetFacade("notmyfacade", 0)
116+ c.Assert(err, gc.NotNil)
117+ c.Assert(err, gc.FitsTypeOf, (*rpcreflect.CallNotImplementedError)(nil))
118+ c.Assert(err, gc.ErrorMatches, `unknown object type "notmyfacade"`)
119+}
120+
121+func (s *facadeRegistrySuite) TestGetFacadeBadVersion(c *gc.C) {
122+ s.sanitizeFacades()
123+ common.RegisterFacade("myfacade", 0, testFacade)
124+ _, err := common.GetFacade("myfacade", 1)
125+ c.Assert(err, gc.NotNil)
126+ c.Assert(err, gc.FitsTypeOf, (*rpcreflect.CallNotImplementedError)(nil))
127+ c.Assert(err, gc.ErrorMatches, `unknown version \(1\) of interface "myfacade"`)
128+}
129+
130+// TODO: We need a test that calling API versions that aren't there return the
131+// proper CodeNotImplemented.
132+
133+func (s *facadeRegistrySuite) TestRegisterFacadePanicsOnDoubleRegistry(c *gc.C) {
134+ common.RegisterFacade("myfacade", 0, testFacade)
135+ c.Assert(func() { common.RegisterFacade("myfacade", 0, testFacade) },
136+ gc.PanicMatches, `object "myfacade\(0\)" already registered`)
137+}
138+
139+func (*facadeRegistrySuite) TestDiscardedAPIMethods(c *gc.C) {
140+ allFacades := common.Facades.List()
141+ c.Assert(allFacades, gc.HasLen, 0)
142+}

Subscribers

People subscribed via source and target branches

to status/vote changes: