juju-core

Merge lp:~jameinel/juju-core/api-srvRoot-ensures-type into lp:~go-bot/juju-core/trunk

  1. api-srvRoot-ensures-type
  2. Merge into trunk
Proposed by John A Meinel
Status: Work in progress
Proposed branch: lp:~jameinel/juju-core/api-srvRoot-ensures-type
Merge into: lp:~go-bot/juju-core/trunk
Prerequisite: lp:~jameinel/juju-core/api-registry-tracks-type
Diff against target: 228 lines (+130/-7)
5 files modified
state/apiserver/export_test.go (+18/-0)
state/apiserver/root.go (+16/-6)
state/apiserver/root_test.go (+53/-1)
utils/registry/registry.go (+11/-0)
utils/registry/registry_test.go (+32/-0)
To merge this branch: bzr merge lp:~jameinel/juju-core/api-srvRoot-ensures-type
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+220615@code.launchpad.net

Description of the change

state/apiserver: ensure Facades match their type

FacadeFactory has to be a type defined to return interface{} because of
go type limitations. However, when we Register a Facade we define what
Type is meant to be exposed in that API. This change makes sure that we
enforce that Type at runtime.

This change also cleans up srvRoot slightly to reduce its coupling with
the rest of the system and make it possible to create one for testing.

https://codereview.appspot.com/97570051/

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

Reviewers: mp+220615_code.launchpad.net,

Message:
Please take a look.

Description:
state/apiserver: ensure Facades match their type

FacadeFactory has to be a type defined to return interface{} because of
go type limitations. However, when we Register a Facade we define what
Type is meant to be exposed in that API. This change makes sure that we
enforce that Type at runtime.

This change also cleans up srvRoot slightly to reduce its coupling with
the rest of the system and make it possible to create one for testing.

https://code.launchpad.net/~jameinel/juju-core/api-srvRoot-ensures-type/+merge/220615

Requires:
https://code.launchpad.net/~jameinel/juju-core/api-registry-tracks-type/+merge/220588

(do not edit description out of merge proposal)

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

Affected files (+133, -8 lines):
   A [revision details]
   M state/apiserver/export_test.go
   M state/apiserver/root.go
   M state/apiserver/root_test.go
   M utils/registry/registry.go
   M utils/registry/registry_test.go

Reply
lp:~jameinel/juju-core/api-srvRoot-ensures-type updated
2756. By John A Meinel

Merge up the removal of all of the compat functions.

2757. By John A Meinel

Merged api-registry-tracks-type into api-srvRoot-ensures-type.

2758. By John A Meinel

Merged api-registry-tracks-type into api-srvRoot-ensures-type.

2759. By John A Meinel

Merged api-registry-tracks-type into api-srvRoot-ensures-type.

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

LGTM

https://codereview.appspot.com/97570051/

Reply
lp:~jameinel/juju-core/api-srvRoot-ensures-type updated
2760. By John A Meinel

Merged api-registry-tracks-type into api-srvRoot-ensures-type.

Unmerged revisions

2760. By John A Meinel

Merged api-registry-tracks-type into api-srvRoot-ensures-type.

2759. By John A Meinel

Merged api-registry-tracks-type into api-srvRoot-ensures-type.

2758. By John A Meinel

Merged api-registry-tracks-type into api-srvRoot-ensures-type.

2757. By John A Meinel

Merged api-registry-tracks-type into api-srvRoot-ensures-type.

2756. By John A Meinel

Merge up the removal of all of the compat functions.

2755. By John A Meinel

Merged api-registry-tracks-type into api-srvRoot-ensures-type.

2754. By John A Meinel

change srvRoot.MethodCaller to actually ensure objects returned match their type.

RegisterStandardFacade makes sure the right type gets registered,
but RegisterFacade doesn't because the Facade interface has to
return interface{}. But we can test it at runtime, so lets do that
to avoid ever having accidental skew.

2753. By John A Meinel

Add registry.TypeNameVersion.Discard()

This allows us to register things for a test and then cleanup
when we are done.

2752. By John A Meinel

fix a changed function name

2751. By John A Meinel

Merge api-use-register-standard-facade.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'state/apiserver/export_test.go'
2--- state/apiserver/export_test.go 2014-04-04 15:22:39 +0000
3+++ state/apiserver/export_test.go 2014-05-26 10:02:25 +0000
4@@ -8,6 +8,7 @@
5
6 "launchpad.net/juju-core/state"
7 "launchpad.net/juju-core/state/api/params"
8+ "launchpad.net/juju-core/state/apiserver/common"
9 )
10
11 var (
12@@ -36,3 +37,20 @@
13 doCheckCreds = delayedCheckCreds
14 return
15 }
16+
17+type bogusTaggedAuthenticator struct {
18+ state.Entity
19+ state.Authenticator
20+}
21+
22+// TestingSrvRoot gives you an srvRoot that is *barely* connected to anything.
23+// Just enough to let you probe some of the interfaces of srvRoot, but not
24+// enough to actually do any RPC calls
25+func TestingSrvRoot(st *state.State) *srvRoot {
26+ return &srvRoot{
27+ state: st,
28+ rpcConn: nil,
29+ resources: common.NewResources(),
30+ entity: nil,
31+ }
32+}
33
34=== modified file 'state/apiserver/root.go'
35--- state/apiserver/root.go 2014-05-26 10:02:24 +0000
36+++ state/apiserver/root.go 2014-05-26 10:02:25 +0000
37@@ -4,6 +4,7 @@
38 package apiserver
39
40 import (
41+ "fmt"
42 "reflect"
43 "time"
44
45@@ -35,7 +36,7 @@
46 // srvRoot represents a single client's connection to the state
47 // after it has logged in.
48 type srvRoot struct {
49- srv *Server
50+ state *state.State
51 rpcConn *rpc.Conn
52 resources *common.Resources
53 entity taggedAuthenticator
54@@ -46,14 +47,14 @@
55 // connection.
56 func newSrvRoot(root *initialRoot, entity taggedAuthenticator) *srvRoot {
57 r := &srvRoot{
58- srv: root.srv,
59+ state: root.srv.state,
60 rpcConn: root.rpcConn,
61 resources: common.NewResources(),
62 entity: entity,
63 }
64 // Note: Only the Client API is part of srvRoot rather than all
65 // the other ones being created on-demand. Why is that?
66- r.resources.RegisterNamed("dataDir", common.StringResource(r.srv.dataDir))
67+ r.resources.RegisterNamed("dataDir", common.StringResource(root.srv.dataDir))
68 return r
69 }
70
71@@ -72,9 +73,18 @@
72 if err != nil {
73 return rpcreflect.MethodCaller{}, err
74 }
75- facade, err := factory(r.srv.state, r.resources, r, objId)
76- if err != nil {
77- return rpcreflect.MethodCaller{}, err
78+ expectedType, err := common.GetFacadeType(rootMethodName, version)
79+ if err != nil {
80+ return rpcreflect.MethodCaller{}, err
81+ }
82+ facade, err := factory(r.state, r.resources, r, objId)
83+ if err != nil {
84+ return rpcreflect.MethodCaller{}, err
85+ }
86+ if !reflect.TypeOf(facade).AssignableTo(expectedType) {
87+ return rpcreflect.MethodCaller{}, fmt.Errorf(
88+ "internal error, %s(%d) claimed to return %s but returned %T",
89+ rootMethodName, version, expectedType, facade)
90 }
91 return rpcreflect.MethodCallerFromValue(
92 reflect.ValueOf(facade), rootMethodName, version, objMethodName)
93
94=== modified file 'state/apiserver/root_test.go'
95--- state/apiserver/root_test.go 2014-05-26 10:02:24 +0000
96+++ state/apiserver/root_test.go 2014-05-26 10:02:25 +0000
97@@ -4,15 +4,21 @@
98 package apiserver_test
99
100 import (
101+ "fmt"
102+ "reflect"
103 "time"
104
105 gc "launchpad.net/gocheck"
106
107+ "launchpad.net/juju-core/state"
108 "launchpad.net/juju-core/state/apiserver"
109+ "launchpad.net/juju-core/state/apiserver/common"
110 "launchpad.net/juju-core/testing"
111 )
112
113-type rootSuite struct{}
114+type rootSuite struct {
115+ testing.BaseSuite
116+}
117
118 var _ = gc.Suite(&rootSuite{})
119
120@@ -53,3 +59,49 @@
121 case <-time.After(testing.ShortWait):
122 }
123 }
124+
125+type testingType struct{}
126+
127+func (testingType) Exposed() error {
128+ return fmt.Errorf("Exposed was bogus")
129+}
130+
131+type badType struct{}
132+
133+func (badType) Exposed() error {
134+ return fmt.Errorf("badType.Exposed was not to be exposed")
135+}
136+
137+func (r *rootSuite) TestMethodCallerEnsuresTypeMatch(c *gc.C) {
138+ srvRoot := apiserver.TestingSrvRoot(nil)
139+ defer common.Facades.Discard("my-testing-facade", 0)
140+ defer common.Facades.Discard("my-testing-facade", 1)
141+ myBadFacade := func(
142+ *state.State, *common.Resources, common.Authorizer, string,
143+ ) (
144+ interface{}, error,
145+ ) {
146+ return &badType{}, nil
147+ }
148+ myGoodFacade := func(
149+ *state.State, *common.Resources, common.Authorizer, string,
150+ ) (
151+ interface{}, error,
152+ ) {
153+ return &testingType{}, nil
154+ }
155+ expectedType := reflect.TypeOf((*testingType)(nil))
156+ common.RegisterFacade("my-testing-facade", 0, myBadFacade, expectedType)
157+ common.RegisterFacade("my-testing-facade", 1, myGoodFacade, expectedType)
158+ // Now, myGoodFacade returns the right type, so calling it should work
159+ // fine
160+ caller, err := srvRoot.MethodCaller("my-testing-facade", 1, "", "Exposed")
161+ c.Assert(err, gc.IsNil)
162+ _, err = caller.Call(reflect.Value{})
163+ c.Check(err, gc.ErrorMatches, "Exposed was bogus")
164+ // However, myBadFacade returns the wrong type, so trying to access it
165+ // should create an error
166+ caller, err = srvRoot.MethodCaller("my-testing-facade", 0, "", "Exposed")
167+ c.Check(err, gc.ErrorMatches,
168+ `internal error, my-testing-facade\(0\) claimed to return \*apiserver_test.testingType but returned \*apiserver_test.badType`)
169+}
170
171=== modified file 'utils/registry/registry.go'
172--- utils/registry/registry.go 2014-05-26 10:02:24 +0000
173+++ utils/registry/registry.go 2014-05-26 10:02:25 +0000
174@@ -101,3 +101,14 @@
175 }
176 return nil, errors.NotFoundf("%s(%d)", name, version)
177 }
178+
179+// Discard gets rid of a registration that has already been done. Calling
180+// discard on an entry that is not present is not considered an error.
181+func (r *TypedNameVersion) Discard(name string, version int) {
182+ if versions, ok := r.versions[name]; ok {
183+ delete(versions, version)
184+ if len(versions) == 0 {
185+ delete(r.versions, name)
186+ }
187+ }
188+}
189
190=== modified file 'utils/registry/registry_test.go'
191--- utils/registry/registry_test.go 2014-05-23 11:24:54 +0000
192+++ utils/registry/registry_test.go 2014-05-26 10:02:25 +0000
193@@ -149,3 +149,35 @@
194 c.Check(err, gc.ErrorMatches, `name\(1\) not found`)
195 c.Check(f, gc.IsNil)
196 }
197+
198+func (s *registrySuite) TestDiscardHandlesNotPresent(c *gc.C) {
199+ r := registry.NewTypedNameVersion(factoryType)
200+ r.Discard("name", 1)
201+}
202+
203+func (s *registrySuite) TestDiscardRemovesEntry(c *gc.C) {
204+ r := registry.NewTypedNameVersion(factoryType)
205+ c.Assert(r.Register("name", 0, nilFactory), gc.IsNil)
206+ _, err := r.Get("name", 0)
207+ c.Assert(err, gc.IsNil)
208+ r.Discard("name", 0)
209+ f, err := r.Get("name", 0)
210+ c.Check(err, jc.Satisfies, errors.IsNotFound)
211+ c.Check(err, gc.ErrorMatches, `name\(0\) not found`)
212+ c.Check(f, gc.IsNil)
213+ c.Check(r.List(), gc.DeepEquals, []registry.Description{})
214+}
215+
216+func (s *registrySuite) TestDiscardLeavesOtherVersions(c *gc.C) {
217+ r := registry.NewTypedNameVersion(factoryType)
218+ c.Assert(r.Register("name", 0, nilFactory), gc.IsNil)
219+ c.Assert(r.Register("name", 1, nilFactory), gc.IsNil)
220+ r.Discard("name", 0)
221+ _, err := r.Get("name", 0)
222+ c.Check(err, jc.Satisfies, errors.IsNotFound)
223+ _, err = r.Get("name", 1)
224+ c.Check(err, gc.IsNil)
225+ c.Check(r.List(), gc.DeepEquals, []registry.Description{
226+ {Name: "name", Versions: []int{1}},
227+ })
228+}

Subscribers

People subscribed via source and target branches

to status/vote changes: