Merge lp:~jameinel/juju-core/api-rpc-reflect-version into lp:~go-bot/juju-core/trunk

[John A Meinel]

rpc: support a Version field

This adds support for a Version field in RPC requests, and changes the
MethodCaller interface for how and when it looks up objects.
Most of the MethodCaller changes were already reviewed in:
 https://code.launchpad.net/~jameinel/juju-core/reflect-only-facades/+merge/218764
There are only a couple of tweaks because the lookup is now done with a
real Version instead of overriding the "id" field.

This also uses the registry.TypedNameVersion code in a sample registry that
shows one possible way that we can do a Registry with versioned lookups.
Note that the next steps don't actually make use of rpc/registry/* so
I'm willing to pull it out, or put it in as a test-suite only thing.
Alternatively it might be something tha could fit into
documentation/example sort of code.

This does disable one test that assumed you could statically iterate
over all the types and exposed methods. I introduce a similar function
later on once we have the factories registered.

One thing that was brought up before about always unfolding Interface(),
I didn't do that yet, but I still could if we feel it would be more
straightforward.

https://codereview.appspot.com/92410043/

[John A Meinel (jameinel)]

Reviewers: mp+219653_code.launchpad.net,

Message:
Please take a look.

Description:
rpc: support a Version field

This adds support for a Version field in RPC requests, and changes the
MethodCaller interface for how and when it looks up objects.
Most of the MethodCaller changes were already reviewed in:

https://code.launchpad.net/~jameinel/juju-core/reflect-only-facades/+merge/218764
There are only a couple of tweaks because the lookup is now done with a
real Version instead of overriding the "id" field.

This also uses the registry.TypedNameVersion code in a sample registry
that
shows one possible way that we can do a Registry with versioned lookups.
Note that the next steps don't actually make use of rpc/registry/* so
I'm willing to pull it out, or put it in as a test-suite only thing.
Alternatively it might be something tha could fit into
documentation/example sort of code.

This does disable one test that assumed you could statically iterate
over all the types and exposed methods. I introduce a similar function
later on once we have the factories registered.

One thing that was brought up before about always unfolding Interface(),
I didn't do that yet, but I still could if we feel it would be more
straightforward.

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

Requires:
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/92410043/

Affected files (+636, -124 lines):
   A [revision details]
   M rpc/jsoncodec/codec.go
   M rpc/reflect_test.go
   A rpc/registry/package_test.go
   A rpc/registry/registry.go
   A rpc/registry/registry_test.go
   M rpc/rpc_test.go
   M rpc/rpcreflect/type.go
   M rpc/rpcreflect/value.go
   M rpc/server.go
   M state/apiserver/root_test.go

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/92410043/diff/1/rpc/reflect_test.go
File rpc/reflect_test.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/reflect_test.go#newcode58
rpc/reflect_test.go:58: // Is this worth specifying?
I still think it is :).

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go
File rpc/registry/registry.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go#newcode30
rpc/registry/registry.go:30: // could just pass Factory instead of this
pointer Elem trick
Tell me about it.

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go#newcode50
rpc/registry/registry.go:50: ) {
/me feels quietly smug

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go#newcode62
rpc/registry/registry.go:62: // There was a problem, do we translate it
into CallNotImplementedError ?
I *think* this is good as it is.

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry_test.go
File rpc/registry/registry_test.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry_test.go#newcode47
rpc/registry/registry_test.go:47: // c.Check(err, gc.ErrorMatches,
`unknown object type "facade"`)
d

https://codereview.appspot.com/92410043/diff/1/rpc/rpc_test.go
File rpc/rpc_test.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/rpc_test.go#newcode789
rpc/rpc_test.go:789: // and beyond
I'm really appreciating the comments, but I'd love to see trailing full
stops.

https://codereview.appspot.com/92410043/

[lp:~jameinel/juju-core/api-rpc-reflect-version]

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

address review comments.

[John A Meinel (jameinel)]

Please take a look.

https://codereview.appspot.com/92410043/diff/1/rpc/reflect_test.go
File rpc/reflect_test.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/reflect_test.go#newcode58
rpc/reflect_test.go:58: // Is this worth specifying?
On 2014/05/16 09:05:27, fwereade wrote:
> I still think it is :).

I wasn't sure because the test used to assert that rtype.ObjType matched
rpcreflect.ObjTypeOf() (so that the object type being exposed to the
world matched the object type you get by looking it up directly.)
Since we just asserted that the actual reflect.Type is identical, it
seemed a little strange to assert that ObjTypeOf(T), gc.Equals,
ObjTypeOf(T).

But I'm willing to leave it in.

Comment removed.

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go
File rpc/registry/registry.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go#newcode50
rpc/registry/registry.go:50: ) {
On 2014/05/16 09:05:28, fwereade wrote:
> /me feels quietly smug

Having seen quite a bit of code with long params jumping around we do
have quite a few places that also do it Nate's way (possibly written by
him):

func (r *RootFromRegistry) MethodCaller(
 rootMethodName string, version int, objId, objMethodName string,
) (rcreflect.MethodCaller, error) {

I think I still prefer the consistency between args in and args out.

The only case I might switch is when the arg list is still too long
because of package names, etc. I haven't quite decided between:

func (r *RootFromRegistry) MethodCaller(
 rootMethodName string, version int,
         objId string, objMethodName string,
) (
 rpcreflect.MethodCaller, error,
) {

and

func (r *RootFromRegistry) MethodCaller(
 rootMethodName string,
         version int,
         objId string,
         objMethodName string,
) (
 rpcreflect.MethodCaller, error,
) {

(If you *have* to wrap, should you go down to one-per-line).

Anyway, I did like your suggestion, and certainly I asked looking for a
nice way to go. :)

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry.go#newcode62
rpc/registry/registry.go:62: // There was a problem, do we translate it
into CallNotImplementedError ?
On 2014/05/16 09:05:28, fwereade wrote:
> I *think* this is good as it is.

comment removed. I could see a case for mapping obvious types, like
potentially a NotFound into a CallNotImplemented.

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry_test.go
File rpc/registry/registry_test.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/registry/registry_test.go#newcode47
rpc/registry/registry_test.go:47: // c.Check(err, gc.ErrorMatches,
`unknown object type "facade"`)
On 2014/05/16 09:05:28, fwereade wrote:
> d

Done.

https://codereview.appspot.com/92410043/diff/1/rpc/rpc_test.go
File rpc/rpc_test.go (right):

https://codereview.appspot.com/92410043/diff/1/rpc/rpc_test.go#newcode789
rpc/rpc_test.go:789: // and beyond
On 2014/05/16 09:05:28, fwereade wrote:
> I'm really appreciating the comments, but I'd love to see trailing
full stops.

I went through and audited the change so far, adding full stops where
appropriate (hopefully).

https://coderev...

[lp:~jameinel/juju-core/api-rpc-reflect-version]

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

add some fullstops for comments in another branch

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

merge typed-registry, bringing it trunk and switch to github.com/juju/errors

[Dimiter Naydenov (dimitern)]

Few comments for clarification, otherwise looks great!

https://codereview.appspot.com/92410043/diff/20001/rpc/registry/registry.go
File rpc/registry/registry.go (right):

https://codereview.appspot.com/92410043/diff/20001/rpc/registry/registry.go#newcode20
rpc/registry/registry.go:20: func (r *RootFromRegistry) Register(name
string, version int, f Factory) {
Doc comment? Also, is panicking here a good idea?

https://codereview.appspot.com/92410043/diff/20001/rpc/registry/registry.go#newcode45
rpc/registry/registry.go:45: // objId: facadeVersion (will be translated
from a string into an int)
This comment is a bit confusing - we have version int and objId string -
is only the latter used for versioning?

https://codereview.appspot.com/92410043/diff/20001/rpc/server.go
File rpc/server.go (right):

https://codereview.appspot.com/92410043/diff/20001/rpc/server.go#newcode344
rpc/server.go:344: // version, id, methodName) triplet.
It's a quadruplet now actually :)

https://codereview.appspot.com/92410043/

[lp:~jameinel/juju-core/api-rpc-reflect-version]

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

apply feedback from Dimiter's review.

[John A Meinel (jameinel)]

Please take a look.

https://codereview.appspot.com/92410043/diff/20001/rpc/registry/registry.go
File rpc/registry/registry.go (right):

https://codereview.appspot.com/92410043/diff/20001/rpc/registry/registry.go#newcode20
rpc/registry/registry.go:20: func (r *RootFromRegistry) Register(name
string, version int, f Factory) {
On 2014/05/19 09:33:06, dimitern wrote:
> Doc comment? Also, is panicking here a good idea?

So this is example code, but panic() in stuff that happens during init()
is fine with me. Because then it panics early and not just end up with
things that aren't actually registered.

https://codereview.appspot.com/92410043/diff/20001/rpc/registry/registry.go#newcode45
rpc/registry/registry.go:45: // objId: facadeVersion (will be translated
from a string into an int)
On 2014/05/19 09:33:06, dimitern wrote:
> This comment is a bit confusing - we have version int and objId string
- is only
> the latter used for versioning?

Just out of date docstrings. It was added when we were using objId for
versioning, but now we have the actual Version field. I updated the
comments.

https://codereview.appspot.com/92410043/diff/20001/rpc/server.go
File rpc/server.go (right):

https://codereview.appspot.com/92410043/diff/20001/rpc/server.go#newcode344
rpc/server.go:344: // version, id, methodName) triplet.
On 2014/05/19 09:33:06, dimitern wrote:
> It's a quadruplet now actually :)

Done.

https://codereview.appspot.com/92410043/

Unmerged revisions

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

apply feedback from Dimiter's review.

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

merge typed-registry, bringing it trunk and switch to github.com/juju/errors

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

add some fullstops for comments in another branch

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

address review comments.

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.

=== modified file 'rpc/jsoncodec/codec.go'
--- rpc/jsoncodec/codec.go	2014-03-05 19:41:34 +0000
+++ rpc/jsoncodec/codec.go	2014-05-22 11:41:28 +0000
@@ -64,6 +64,7 @@
 type inMsg struct {
 	RequestId uint64
 	Type      string
+	Version   int `json:",omitempty"`
 	Id        string
 	Request   string
 	Params    json.RawMessage
@@ -76,6 +77,7 @@
 type outMsg struct {
 	RequestId uint64
 	Type      string      `json:",omitempty"`
+	Version   int         `json:",omitempty"`
 	Id        string      `json:",omitempty"`
 	Request   string      `json:",omitempty"`
 	Params    interface{} `json:",omitempty"`
@@ -122,9 +124,10 @@
 	}
 	hdr.RequestId = c.msg.RequestId
 	hdr.Request = rpc.Request{
-		Type:   c.msg.Type,
-		Id:     c.msg.Id,
-		Action: c.msg.Request,
+		Type:    c.msg.Type,
+		Version: c.msg.Version,
+		Id:      c.msg.Id,
+		Action:  c.msg.Request,
 	}
 	hdr.Error = c.msg.Error
 	hdr.ErrorCode = c.msg.ErrorCode
@@ -184,6 +187,7 @@
 	m.RequestId = hdr.RequestId
 	m.Type = hdr.Request.Type
 	m.Id = hdr.Request.Id
+	m.Version = hdr.Request.Version
 	m.Request = hdr.Request.Action
 	m.Error = hdr.Error
 	m.ErrorCode = hdr.ErrorCode

=== modified file 'rpc/reflect_test.go'
--- rpc/reflect_test.go	2014-05-20 04:27:02 +0000
+++ rpc/reflect_test.go	2014-05-22 11:41:28 +0000
@@ -11,6 +11,7 @@
 
 	"launchpad.net/juju-core/rpc/rpcreflect"
 	"launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/utils/set"
 )
 
 // We test rpcreflect in this package, so that the
@@ -23,28 +24,38 @@
 var _ = gc.Suite(&reflectSuite{})
 
 func (*reflectSuite) TestTypeOf(c *gc.C) {
-	rtype := rpcreflect.TypeOf(reflect.TypeOf(&Root{}))
+	root := SimpleRoot()
+	rtype := rpcreflect.TypeOf(reflect.TypeOf(root))
 	c.Assert(rtype.DiscardedMethods(), gc.DeepEquals, []string{
 		"Discard1",
 		"Discard2",
 		"Discard3",
 	})
-	expect := map[string]reflect.Type{
-		"CallbackMethods":  reflect.TypeOf(&CallbackMethods{}),
-		"ChangeAPIMethods": reflect.TypeOf(&ChangeAPIMethods{}),
-		"DelayedMethods":   reflect.TypeOf(&DelayedMethods{}),
-		"ErrorMethods":     reflect.TypeOf(&ErrorMethods{}),
-		"InterfaceMethods": reflect.TypeOf((*InterfaceMethods)(nil)).Elem(),
-		"SimpleMethods":    reflect.TypeOf(&SimpleMethods{}),
-	}
-	c.Assert(rtype.MethodNames(), gc.HasLen, len(expect))
-	for name, expectGoType := range expect {
-		m, err := rtype.Method(name)
+	expect := []struct {
+		Name   string
+		Id     string
+		GoType reflect.Type
+	}{
+		{"CallbackMethods", "", reflect.TypeOf(&CallbackMethods{})},
+		{"ChangeAPIMethods", "", reflect.TypeOf(&ChangeAPIMethods{})},
+		{"DelayedMethods", "", reflect.TypeOf(&DelayedMethods{})},
+		{"ErrorMethods", "", reflect.TypeOf(&ErrorMethods{})},
+		{"InterfaceMethods", "a99", reflect.TypeOf((*InterfaceMethods)(nil)).Elem()},
+		{"SimpleMethods", "a99", reflect.TypeOf(&SimpleMethods{})},
+	}
+	var uniqueNames set.Strings
+	for _, exp := range expect {
+		uniqueNames.Add(exp.Name)
+	}
+	c.Assert(rtype.MethodNames(), gc.DeepEquals, uniqueNames.SortedValues())
+	for _, exp := range expect {
+		m, err := rtype.Method(exp.Name)
 		c.Assert(err, gc.IsNil)
 		c.Assert(m, gc.NotNil)
 		c.Assert(m.Call, gc.NotNil)
-		c.Assert(m.ObjType, gc.Equals, rpcreflect.ObjTypeOf(expectGoType))
-		c.Assert(m.ObjType.GoType(), gc.Equals, expectGoType)
+		obj, err := m.Call(reflect.ValueOf(root), exp.Id)
+		c.Check(obj.Type(), gc.Equals, exp.GoType)
+		c.Check(rpcreflect.ObjTypeOf(obj.Type()), gc.Equals, rpcreflect.ObjTypeOf(exp.GoType))
 	}
 	m, err := rtype.Method("not found")
 	c.Assert(err, gc.Equals, rpcreflect.ErrMethodNotFound)
@@ -97,7 +108,7 @@
 func (*reflectSuite) TestValueOf(c *gc.C) {
 	v := rpcreflect.ValueOf(reflect.ValueOf(nil))
 	c.Check(v.IsValid(), jc.IsFalse)
-	c.Check(func() { v.MethodCaller("foo", "bar") }, gc.PanicMatches, "MethodCaller called on invalid Value")
+	c.Check(func() { v.MethodCaller("foo", 0, "bar", "") }, gc.PanicMatches, "MethodCaller called on invalid Value")
 
 	root := &Root{}
 	v = rpcreflect.ValueOf(reflect.ValueOf(root))
@@ -115,22 +126,39 @@
 	root.simple["a99"] = &SimpleMethods{root: root, id: "a99"}
 	v := rpcreflect.ValueOf(reflect.ValueOf(root))
 
-	m, err := v.MethodCaller("foo", "bar")
+	m, err := v.MethodCaller("foo", 0, "a99", "bar")
 	c.Assert(err, gc.ErrorMatches, `unknown object type "foo"`)
 	c.Assert(err, gc.FitsTypeOf, (*rpcreflect.CallNotImplementedError)(nil))
 	c.Assert(m, gc.DeepEquals, rpcreflect.MethodCaller{})
 
-	m, err = v.MethodCaller("SimpleMethods", "bar")
+	m, err = v.MethodCaller("SimpleMethods", 0, "a99", "bar")
 	c.Assert(err, gc.ErrorMatches, "no such request - method SimpleMethods.bar is not implemented")
 	c.Assert(err, gc.FitsTypeOf, (*rpcreflect.CallNotImplementedError)(nil))
 	c.Assert(m, gc.DeepEquals, rpcreflect.MethodCaller{})
 
-	m, err = v.MethodCaller("SimpleMethods", "Call1r1e")
+	m, err = v.MethodCaller("SimpleMethods", 0, "a99", "Call1r1e")
 	c.Assert(err, gc.IsNil)
 	c.Assert(m.ParamsType, gc.Equals, reflect.TypeOf(stringVal{}))
 	c.Assert(m.ResultType, gc.Equals, reflect.TypeOf(stringVal{}))
 
-	ret, err := m.Call("a99", reflect.ValueOf(stringVal{"foo"}))
+	ret, err := m.Call(reflect.ValueOf(stringVal{"foo"}))
 	c.Assert(err, gc.IsNil)
 	c.Assert(ret.Interface(), gc.Equals, stringVal{"Call1r1e ret"})
 }
+
+func (*reflectSuite) TestMethodCallerRefusesVersionsNot0(c *gc.C) {
+	root := &Root{
+		simple: make(map[string]*SimpleMethods),
+	}
+	root.simple["a99"] = &SimpleMethods{root: root, id: "a99"}
+	v := rpcreflect.ValueOf(reflect.ValueOf(root))
+
+	m, err := v.MethodCaller("SimpleMethods", 0, "a99", "Call1r1e")
+	c.Assert(err, gc.IsNil)
+	c.Assert(m.ParamsType, gc.Equals, reflect.TypeOf(stringVal{}))
+	c.Assert(m.ResultType, gc.Equals, reflect.TypeOf(stringVal{}))
+
+	m, err = v.MethodCaller("SimpleMethods", 1, "a99", "Call1r1e")
+	c.Assert(err, gc.FitsTypeOf, (*rpcreflect.CallNotImplementedError)(nil))
+	c.Assert(err, gc.ErrorMatches, `unknown version \(1\) of interface "SimpleMethods"`)
+}

=== added directory 'rpc/registry'
=== added file 'rpc/registry/package_test.go'
--- rpc/registry/package_test.go	1970-01-01 00:00:00 +0000
+++ rpc/registry/package_test.go	2014-05-22 11:41:28 +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 'rpc/registry/registry.go'
--- rpc/registry/registry.go	1970-01-01 00:00:00 +0000
+++ rpc/registry/registry.go	2014-05-22 11:41:28 +0000
@@ -0,0 +1,71 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package registry
+
+import (
+	"reflect"
+
+	"github.com/juju/errors"
+
+	"launchpad.net/juju-core/rpc/rpcreflect"
+	"launchpad.net/juju-core/utils/registry"
+)
+
+// RootFromRegistry is a map from (facade name, version) to a factory function that
+// creates an object manifesting that facade.
+type RootFromRegistry struct {
+	registry *registry.TypedNameVersion
+}
+
+// Register records a factory that will be returned from Get() when passed the
+// specific (name, version) pair.
+func (r *RootFromRegistry) Register(name string, version int, f Factory) {
+	err := r.registry.Register(name, version, f)
+	if err != nil {
+		panic(err)
+	}
+}
+
+// NewRegistry creates a place to register facades.
+func NewRegistry() *RootFromRegistry {
+	// Note: 'type' is not a first class object like functions are, so we
+	// can't just pass Factory. Instead we create an pointer to a (nil)
+	// instance of it, and then use Elem to get the correct reflect.Type.
+	factoryType := reflect.TypeOf((*Factory)(nil)).Elem()
+	return &RootFromRegistry{
+		registry: registry.NewTypedNameVersion(factoryType),
+	}
+}
+
+type Factory func() (interface{}, error)
+
+// MethodCaller is used by rpc.Server to place a call to a given Method on a
+// Facade at the specific version.
+// The parameter names match that of the rpcreflect.Value implementation, but
+// might be better named:
+//	rootMethodName: facadeName
+//	version: facadeVersion (will be translated from a string into an int)
+//	objId: Id for the a concrete version of the facade (used for watchers)
+//	objMethodName: methodName
+func (r *RootFromRegistry) MethodCaller(
+	rootMethodName string, version int, objId, objMethodName string,
+) (
+	rpcreflect.MethodCaller, error,
+) {
+	factory, err := r.registry.Get(rootMethodName, version)
+	if errors.IsNotFound(err) {
+		return rpcreflect.MethodCaller{}, &rpcreflect.CallNotImplementedError{
+			RootMethod: rootMethodName,
+			Version:    version,
+		}
+	} else if err != nil {
+		return rpcreflect.MethodCaller{}, err
+	}
+	facade, err := factory.(Factory)()
+	if err != nil {
+		return rpcreflect.MethodCaller{}, err
+	}
+	return rpcreflect.MethodCallerFromValue(
+		reflect.ValueOf(facade), rootMethodName, version, objMethodName)
+}

=== added file 'rpc/registry/registry_test.go'
--- rpc/registry/registry_test.go	1970-01-01 00:00:00 +0000
+++ rpc/registry/registry_test.go	2014-05-22 11:41:28 +0000
@@ -0,0 +1,96 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package registry_test
+
+import (
+	"fmt"
+	"reflect"
+
+	jc "github.com/juju/testing/checkers"
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/rpc/registry"
+	"launchpad.net/juju-core/rpc/rpcreflect"
+	"launchpad.net/juju-core/testing/testbase"
+)
+
+type registrySuite struct {
+	testbase.LoggingSuite
+}
+
+var _ = gc.Suite(&registrySuite{})
+
+func nilFactory() (interface{}, error) {
+	return nil, nil
+}
+
+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) TestMethodCallerNoEntries(c *gc.C) {
+	r := registry.NewRegistry()
+	_, err := r.MethodCaller("facade", 1, "", "method")
+	c.Check(err, gc.FitsTypeOf, (*rpcreflect.CallNotImplementedError)(nil))
+	c.Check(err, gc.ErrorMatches, `unknown version \(1\) of interface "facade"`)
+}
+
+func (s *registrySuite) TestMethodCallerUnknownVersion(c *gc.C) {
+	r := registry.NewRegistry()
+	r.Register("name", 0, nilFactory)
+	_, err := r.MethodCaller("name", 1, "", "method")
+	c.Check(err, gc.FitsTypeOf, (*rpcreflect.CallNotImplementedError)(nil))
+	c.Check(err, gc.ErrorMatches, `unknown version \(1\) of interface "name"`)
+}
+
+func (s *registrySuite) TestMethodCaller(c *gc.C) {
+	r := registry.NewRegistry()
+	testF := &testFacade{version: "v0"}
+	singletonFactory := func() (interface{}, error) { return testF, nil }
+	r.Register("test", 0, singletonFactory)
+	mc, err := r.MethodCaller("test", 0, "", "TestMethod")
+	c.Assert(err, gc.IsNil)
+	result, err := mc.Call(reflect.ValueOf(stringVal{""}))
+	c.Assert(err, gc.IsNil)
+	c.Check(result.Interface().(stringVal).value, gc.Equals, "called v0")
+	c.Check(testF.called, jc.IsTrue)
+}
+
+func (s *registrySuite) TestMethodCallerFactoryError(c *gc.C) {
+	r := registry.NewRegistry()
+	r.Register("test", 0, func() (interface{}, error) {
+		return nil, fmt.Errorf("internal factory failure")
+	})
+	_, err := r.MethodCaller("test", 0, "", "TestMethod")
+	c.Check(err, gc.ErrorMatches, `internal factory failure`)
+}
+
+func (s *registrySuite) TestMethodCallerMultipleVersions(c *gc.C) {
+	r := registry.NewRegistry()
+	testV0 := &testFacade{version: "v0"}
+	testV1 := &testFacade{version: "v1"}
+	r.Register("test", 0, func() (interface{}, error) {
+		return testV0, nil
+	})
+	r.Register("test", 1, func() (interface{}, error) {
+		return testV1, nil
+	})
+	mc, err := r.MethodCaller("test", 1, "", "TestMethod")
+	c.Assert(err, gc.IsNil)
+	result, err := mc.Call(reflect.ValueOf(stringVal{""}))
+	c.Assert(err, gc.IsNil)
+	c.Check(result.Interface().(stringVal).value, gc.Equals, "called v1")
+	c.Check(testV0.called, jc.IsFalse)
+	c.Check(testV1.called, jc.IsTrue)
+}

=== modified file 'rpc/rpc_test.go'
--- rpc/rpc_test.go	2014-05-20 04:27:02 +0000
+++ rpc/rpc_test.go	2014-05-22 11:41:28 +0000
@@ -18,6 +18,7 @@
 
 	"launchpad.net/juju-core/rpc"
 	"launchpad.net/juju-core/rpc/jsoncodec"
+	"launchpad.net/juju-core/rpc/rpcreflect"
 	"launchpad.net/juju-core/testing"
 )
 
@@ -230,7 +231,7 @@
 		return int64val{1}, nil
 	}
 	var r int64val
-	err := a.root.conn.Call(rpc.Request{"CallbackMethods", "", "Factorial"}, int64val{x.I - 1}, &r)
+	err := a.root.conn.Call(rpc.Request{"CallbackMethods", 0, "", "Factorial"}, int64val{x.I - 1}, &r)
 	if err != nil {
 		return int64val{}, err
 	}
@@ -257,11 +258,80 @@
 	return stringVal{"new method result"}
 }
 
+type VariableMethods1 struct {
+	sm *SimpleMethods
+}
+
+func (vm *VariableMethods1) Call0r1() stringVal {
+	return vm.sm.Call0r1()
+}
+
+type VariableMethods2 struct {
+	sm *SimpleMethods
+}
+
+func (vm *VariableMethods2) Call1r1(s stringVal) stringVal {
+	return vm.sm.Call1r1(s)
+}
+
+type RestrictedMethods struct {
+	InterfaceMethods
+}
+
+type CustomCaller struct {
+	root *Root
+}
+
+func (cc *CustomCaller) MethodCaller(
+	rootMethodName string, version int, objId, objMethodName string,
+) (
+	rpcreflect.MethodCaller, error,
+) {
+	logger.Infof("got to MethodCaller: %q %d %q %q\n",
+		rootMethodName, version, objId, objMethodName)
+	if rootMethodName != "MultiVersion" {
+		return rpcreflect.MethodCaller{}, &rpcreflect.CallNotImplementedError{
+			RootMethod: rootMethodName,
+		}
+	}
+	sm, err := cc.root.SimpleMethods(objId)
+	if err != nil {
+		return rpcreflect.MethodCaller{}, err
+	}
+	var facade interface{}
+	switch version {
+	case 0:
+		facade = &VariableMethods1{sm}
+	case 1:
+		facade = &VariableMethods2{sm}
+	case 2:
+		var interfaced InterfaceMethods
+		interfaced = sm
+		facade = interfaced
+	case 3:
+		var restricted RestrictedMethods
+		restricted.InterfaceMethods = sm
+		facade = restricted
+	default:
+		return rpcreflect.MethodCaller{}, &rpcreflect.CallNotImplementedError{
+			RootMethod: rootMethodName,
+			Version:    version,
+		}
+	}
+	return rpcreflect.MethodCallerFromValue(
+		reflect.ValueOf(facade), rootMethodName, version, objMethodName)
+}
+
+func SimpleRoot() *Root {
+	root := &Root{
+		simple: make(map[string]*SimpleMethods),
+	}
+	root.simple["a99"] = &SimpleMethods{root: root, id: "a99"}
+	return root
+}
+
 func (*rpcSuite) TestRPC(c *gc.C) {
-	root := &Root{
-		simple: make(map[string]*SimpleMethods),
-	}
-	root.simple["a99"] = &SimpleMethods{root: root, id: "a99"}
+	root := SimpleRoot()
 	client, srvDone, clientNotifier, serverNotifier := newRPCClientServer(c, root, nil, false)
 	defer closeClient(c, client, srvDone)
 	for narg := 0; narg < 2; narg++ {
@@ -308,11 +378,11 @@
 	serverNotifier *notifier
 
 	// entry holds the top-level type that will be invoked
-	// (e.g. "SimpleMethods")
+	// (e.g. "SimpleMethods").
 	entry string
 
 	// narg holds the number of arguments accepted by the
-	// call (0 or 1)
+	// call (0 or 1).
 	narg int
 
 	// nret holds the number of values returned by the
@@ -324,14 +394,18 @@
 
 	// testErr specifies whether the call should be made to return an error.
 	testErr bool
+
+	// version specifies what version of the interface to call, defaults to 0.
+	version int
 }
 
 // request returns the RPC request for the test call.
 func (p testCallParams) request() rpc.Request {
 	return rpc.Request{
-		Type:   p.entry,
-		Id:     "a99",
-		Action: callName(p.narg, p.nret, p.retErr),
+		Type:    p.entry,
+		Version: p.version,
+		Id:      "a99",
+		Action:  callName(p.narg, p.nret, p.retErr),
 	}
 }
 
@@ -356,7 +430,10 @@
 		})
 		c.Assert(r, gc.Equals, stringVal{})
 	case p.nret > 0:
-		c.Assert(r, gc.Equals, stringVal{p.request().Action + " ret"})
+		c.Check(r, gc.Equals, stringVal{p.request().Action + " ret"})
+	}
+	if !p.testErr {
+		c.Check(err, gc.IsNil)
 	}
 
 	// Check that the call was actually made, the right
@@ -466,10 +543,7 @@
 }
 
 func (*rpcSuite) TestInterfaceMethods(c *gc.C) {
-	root := &Root{
-		simple: make(map[string]*SimpleMethods),
-	}
-	root.simple["a99"] = &SimpleMethods{root: root, id: "a99"}
+	root := SimpleRoot()
 	client, srvDone, clientNotifier, serverNotifier := newRPCClientServer(c, root, nil, false)
 	defer closeClient(c, client, srvDone)
 	p := testCallParams{
@@ -486,6 +560,138 @@
 	root.testCall(c, p)
 	p.testErr = true
 	root.testCall(c, p)
+	// Call0r0 is defined on the underlying SimpleMethods, but is not
+	// exposed at the InterfaceMethods level, so this call should fail with
+	// CodeNotImplemented.
+	var r stringVal
+	err := client.Call(rpc.Request{"InterfaceMethods", 0, "a99", "Call0r0"}, stringVal{"arg"}, &r)
+	c.Assert(err, gc.DeepEquals, &rpc.RequestError{
+		Message: "no such request - method InterfaceMethods.Call0r0 is not implemented",
+		Code:    rpc.CodeNotImplemented,
+	})
+}
+
+func (*rpcSuite) TestCustomCallerV0(c *gc.C) {
+	root := &CustomCaller{SimpleRoot()}
+	client, srvDone, clientNotifier, serverNotifier := newRPCClientServer(c, root, nil, false)
+	defer closeClient(c, client, srvDone)
+	// V0 of MultiVersion implements only VariableMethods1.Call0r1.
+	p := testCallParams{
+		client:         client,
+		clientNotifier: clientNotifier,
+		serverNotifier: serverNotifier,
+		entry:          "MultiVersion",
+		version:        0,
+		narg:           0,
+		nret:           1,
+		retErr:         false,
+		testErr:        false,
+	}
+
+	root.root.testCall(c, p)
+	// Call1r1 is exposed in version 1, but not in version 0.
+	var r stringVal
+	err := client.Call(rpc.Request{"MultiVersion", 0, "a99", "Call1r1"}, stringVal{"arg"}, &r)
+	c.Assert(err, gc.DeepEquals, &rpc.RequestError{
+		Message: "no such request - method MultiVersion.Call1r1 is not implemented",
+		Code:    rpc.CodeNotImplemented,
+	})
+}
+
+func (*rpcSuite) TestCustomCallerV1(c *gc.C) {
+	root := &CustomCaller{SimpleRoot()}
+	client, srvDone, clientNotifier, serverNotifier := newRPCClientServer(c, root, nil, false)
+	defer closeClient(c, client, srvDone)
+	// V1 of MultiVersion implements only VariableMethods2.Call1r1.
+	p := testCallParams{
+		client:         client,
+		clientNotifier: clientNotifier,
+		serverNotifier: serverNotifier,
+		entry:          "MultiVersion",
+		version:        1,
+		narg:           1,
+		nret:           1,
+		retErr:         false,
+		testErr:        false,
+	}
+
+	root.root.testCall(c, p)
+	// Call0r1 is exposed in version 0, but not in version 1.
+	var r stringVal
+	err := client.Call(rpc.Request{"MultiVersion", 1, "a99", "Call0r1"}, nil, &r)
+	c.Assert(err, gc.DeepEquals, &rpc.RequestError{
+		Message: "no such request - method MultiVersion(1).Call0r1 is not implemented",
+		Code:    rpc.CodeNotImplemented,
+	})
+}
+
+func (*rpcSuite) TestCustomCallerV2(c *gc.C) {
+	root := &CustomCaller{SimpleRoot()}
+	client, srvDone, clientNotifier, serverNotifier := newRPCClientServer(c, root, nil, false)
+	defer closeClient(c, client, srvDone)
+	p := testCallParams{
+		client:         client,
+		clientNotifier: clientNotifier,
+		serverNotifier: serverNotifier,
+		entry:          "MultiVersion",
+		version:        2,
+		narg:           1,
+		nret:           1,
+		retErr:         true,
+		testErr:        false,
+	}
+
+	root.root.testCall(c, p)
+	// Because of interface unwrapping, someone might have thought
+	// "interface" would only expose the methods on InterfaceMethods
+	// (Call1r1e), but it actually exposes all of them.
+	p.narg = 0
+	root.root.testCall(c, p)
+	var r stringVal
+	err := client.Call(rpc.Request{"MultiVersion", 2, "a99", "Call0r1e"}, nil, &r)
+	c.Assert(err, gc.IsNil)
+	c.Check(r.Val, gc.Equals, "Call0r1e ret")
+}
+
+func (*rpcSuite) TestCustomCallerV3(c *gc.C) {
+	root := &CustomCaller{SimpleRoot()}
+	client, srvDone, clientNotifier, serverNotifier := newRPCClientServer(c, root, nil, false)
+	defer closeClient(c, client, srvDone)
+	p := testCallParams{
+		client:         client,
+		clientNotifier: clientNotifier,
+		serverNotifier: serverNotifier,
+		entry:          "MultiVersion",
+		version:        3,
+		narg:           1,
+		nret:           1,
+		retErr:         true,
+		testErr:        false,
+	}
+
+	root.root.testCall(c, p)
+	// By embedding the InterfaceMethods inside a concrete
+	// RestrictedMethods type, we actually only expose the methods defined
+	// in InterfaceMethods.
+	var r stringVal
+	err := client.Call(rpc.Request{"MultiVersion", 3, "a99", "Call0r1e"}, nil, &r)
+	c.Assert(err, gc.DeepEquals, &rpc.RequestError{
+		Message: `no such request - method MultiVersion(3).Call0r1e is not implemented`,
+		Code:    rpc.CodeNotImplemented,
+	})
+}
+
+func (*rpcSuite) TestCustomCallerUnknownVersion(c *gc.C) {
+	root := &CustomCaller{SimpleRoot()}
+	client, srvDone, _, _ := newRPCClientServer(c, root, nil, false)
+	defer closeClient(c, client, srvDone)
+	var r stringVal
+	// Unknown version 5
+	err := client.Call(rpc.Request{"MultiVersion", 5, "a99", "Call0r1"}, nil, &r)
+	c.Assert(err, gc.DeepEquals, &rpc.RequestError{
+		Message: `unknown version (5) of interface "MultiVersion"`,
+		Code:    rpc.CodeNotImplemented,
+	})
 }
 
 func (*rpcSuite) TestConcurrentCalls(c *gc.C) {
@@ -505,7 +711,7 @@
 	defer closeClient(c, client, srvDone)
 	call := func(id string, done chan<- struct{}) {
 		var r stringVal
-		err := client.Call(rpc.Request{"DelayedMethods", id, "Delay"}, nil, &r)
+		err := client.Call(rpc.Request{"DelayedMethods", 0, id, "Delay"}, nil, &r)
 		c.Check(err, gc.IsNil)
 		c.Check(r.Val, gc.Equals, "return "+id)
 		done <- struct{}{}
@@ -545,7 +751,7 @@
 	}
 	client, srvDone, _, _ := newRPCClientServer(c, root, nil, false)
 	defer closeClient(c, client, srvDone)
-	err := client.Call(rpc.Request{"ErrorMethods", "", "Call"}, nil, nil)
+	err := client.Call(rpc.Request{"ErrorMethods", 0, "", "Call"}, nil, nil)
 	c.Assert(err, gc.ErrorMatches, `request error: message \(code\)`)
 	c.Assert(err.(rpc.ErrorCoder).ErrorCode(), gc.Equals, "code")
 }
@@ -566,18 +772,33 @@
 	}
 	client, srvDone, _, _ := newRPCClientServer(c, root, tfErr, false)
 	defer closeClient(c, client, srvDone)
-	err := client.Call(rpc.Request{"ErrorMethods", "", "Call"}, nil, nil)
+	// First, we don't transform methods we can't find.
+	err := client.Call(rpc.Request{"foo", 0, "", "bar"}, nil, nil)
+	c.Assert(err, gc.DeepEquals, &rpc.RequestError{
+		Message: `unknown object type "foo"`,
+		Code:    rpc.CodeNotImplemented,
+	})
+
+	err = client.Call(rpc.Request{"ErrorMethods", 0, "", "NoMethod"}, nil, nil)
+	c.Assert(err, gc.DeepEquals, &rpc.RequestError{
+		Message: "no such request - method ErrorMethods.NoMethod is not implemented",
+		Code:    rpc.CodeNotImplemented,
+	})
+
+	// We do transform any errors that happen from calling the RootMethod
+	// and beyond.
+	err = client.Call(rpc.Request{"ErrorMethods", 0, "", "Call"}, nil, nil)
 	c.Assert(err, gc.DeepEquals, &rpc.RequestError{
 		Message: "transformed: message",
 		Code:    "transformed: code",
 	})
 
 	root.errorInst.err = nil
-	err = client.Call(rpc.Request{"ErrorMethods", "", "Call"}, nil, nil)
+	err = client.Call(rpc.Request{"ErrorMethods", 0, "", "Call"}, nil, nil)
 	c.Assert(err, gc.IsNil)
 
 	root.errorInst = nil
-	err = client.Call(rpc.Request{"ErrorMethods", "", "Call"}, nil, nil)
+	err = client.Call(rpc.Request{"ErrorMethods", 0, "", "Call"}, nil, nil)
 	c.Assert(err, gc.DeepEquals, &rpc.RequestError{
 		Message: "transformed: no error methods",
 	})
@@ -600,7 +821,7 @@
 	done := make(chan struct{})
 	go func() {
 		var r stringVal
-		err := client.Call(rpc.Request{"DelayedMethods", "1", "Delay"}, nil, &r)
+		err := client.Call(rpc.Request{"DelayedMethods", 0, "1", "Delay"}, nil, &r)
 		c.Check(err, gc.Equals, rpc.ErrShutdown)
 		done <- struct{}{}
 	}()
@@ -635,7 +856,7 @@
 	defer closeClient(c, client, srvDone)
 	call := func(method string, arg, ret interface{}) (passedArg interface{}) {
 		root.calls = nil
-		err := client.Call(rpc.Request{"SimpleMethods", "a0", method}, arg, ret)
+		err := client.Call(rpc.Request{"SimpleMethods", 0, "a0", method}, arg, ret)
 		c.Assert(err, gc.IsNil)
 		c.Assert(root.calls, gc.HasLen, 1)
 		info := root.calls[0]
@@ -678,22 +899,19 @@
 	defer closeClient(c, client, srvDone)
 
 	testBadCall(c, client, clientNotifier, serverNotifier,
-		rpc.Request{"BadSomething", "a0", "No"},
+		rpc.Request{"BadSomething", 0, "a0", "No"},
 		`unknown object type "BadSomething"`,
 		rpc.CodeNotImplemented,
-		false,
 	)
 	testBadCall(c, client, clientNotifier, serverNotifier,
-		rpc.Request{"SimpleMethods", "xx", "No"},
+		rpc.Request{"SimpleMethods", 0, "a0", "No"},
 		"no such request - method SimpleMethods.No is not implemented",
 		rpc.CodeNotImplemented,
-		false,
 	)
 	testBadCall(c, client, clientNotifier, serverNotifier,
-		rpc.Request{"SimpleMethods", "xx", "Call0r0"},
+		rpc.Request{"SimpleMethods", 0, "xx", "Call0r0"},
 		`unknown SimpleMethods id`,
 		"",
-		true,
 	)
 }
 
@@ -704,7 +922,6 @@
 	req rpc.Request,
 	expectedErr string,
 	expectedErrCode string,
-	requestKnown bool,
 ) {
 	clientNotifier.reset()
 	serverNotifier.reset()
@@ -746,9 +963,6 @@
 	// 	If the request was not recognized or there was
 	//	an error reading the body, body will be nil.
 	var expectBody interface{}
-	if requestKnown {
-		expectBody = struct{}{}
-	}
 	c.Assert(serverReq, gc.DeepEquals, requestEvent{
 		hdr: rpc.Header{
 			RequestId: requestId,
@@ -786,10 +1000,10 @@
 	}{
 		X: map[string]int{"hello": 65},
 	}
-	err := client.Call(rpc.Request{"SimpleMethods", "a0", "SliceArg"}, arg0, &ret)
+	err := client.Call(rpc.Request{"SimpleMethods", 0, "a0", "SliceArg"}, arg0, &ret)
 	c.Assert(err, gc.ErrorMatches, `request error: json: cannot unmarshal object into Go value of type \[\]string`)
 
-	err = client.Call(rpc.Request{"SimpleMethods", "a0", "SliceArg"}, arg0, &ret)
+	err = client.Call(rpc.Request{"SimpleMethods", 0, "a0", "SliceArg"}, arg0, &ret)
 	c.Assert(err, gc.ErrorMatches, `request error: json: cannot unmarshal object into Go value of type \[\]string`)
 
 	arg1 := struct {
@@ -797,7 +1011,7 @@
 	}{
 		X: []string{"one"},
 	}
-	err = client.Call(rpc.Request{"SimpleMethods", "a0", "SliceArg"}, arg1, &ret)
+	err = client.Call(rpc.Request{"SimpleMethods", 0, "a0", "SliceArg"}, arg1, &ret)
 	c.Assert(err, gc.IsNil)
 	c.Assert(ret.Val, gc.Equals, "SliceArg ret")
 }
@@ -806,7 +1020,7 @@
 	client, srvDone, _, _ := newRPCClientServer(c, &Root{}, nil, false)
 	err := client.Close()
 	c.Assert(err, gc.IsNil)
-	err = client.Call(rpc.Request{"Foo", "", "Bar"}, nil, nil)
+	err = client.Call(rpc.Request{"Foo", 0, "", "Bar"}, nil, nil)
 	c.Assert(err, gc.Equals, rpc.ErrShutdown)
 	err = chanReadError(c, srvDone, "server done")
 	c.Assert(err, gc.IsNil)
@@ -848,7 +1062,7 @@
 	clientRoot := &Root{conn: client}
 	client.Serve(clientRoot, nil)
 	var r int64val
-	err := client.Call(rpc.Request{"CallbackMethods", "", "Factorial"}, int64val{12}, &r)
+	err := client.Call(rpc.Request{"CallbackMethods", 0, "", "Factorial"}, int64val{12}, &r)
 	c.Assert(err, gc.IsNil)
 	c.Assert(r.I, gc.Equals, int64(479001600))
 }
@@ -858,7 +1072,7 @@
 	client, srvDone, _, _ := newRPCClientServer(c, srvRoot, nil, true)
 	defer closeClient(c, client, srvDone)
 	var r int64val
-	err := client.Call(rpc.Request{"CallbackMethods", "", "Factorial"}, int64val{12}, &r)
+	err := client.Call(rpc.Request{"CallbackMethods", 0, "", "Factorial"}, int64val{12}, &r)
 	c.Assert(err, gc.ErrorMatches, "request error: request error: no service")
 }
 
@@ -867,13 +1081,13 @@
 	client, srvDone, _, _ := newRPCClientServer(c, srvRoot, nil, true)
 	defer closeClient(c, client, srvDone)
 	var s stringVal
-	err := client.Call(rpc.Request{"NewlyAvailable", "", "NewMethod"}, nil, &s)
+	err := client.Call(rpc.Request{"NewlyAvailable", 0, "", "NewMethod"}, nil, &s)
 	c.Assert(err, gc.ErrorMatches, `request error: unknown object type "NewlyAvailable" \(not implemented\)`)
-	err = client.Call(rpc.Request{"ChangeAPIMethods", "", "ChangeAPI"}, nil, nil)
+	err = client.Call(rpc.Request{"ChangeAPIMethods", 0, "", "ChangeAPI"}, nil, nil)
 	c.Assert(err, gc.IsNil)
-	err = client.Call(rpc.Request{"ChangeAPIMethods", "", "ChangeAPI"}, nil, nil)
+	err = client.Call(rpc.Request{"ChangeAPIMethods", 0, "", "ChangeAPI"}, nil, nil)
 	c.Assert(err, gc.ErrorMatches, `request error: unknown object type "ChangeAPIMethods" \(not implemented\)`)
-	err = client.Call(rpc.Request{"NewlyAvailable", "", "NewMethod"}, nil, &s)
+	err = client.Call(rpc.Request{"NewlyAvailable", 0, "", "NewMethod"}, nil, &s)
 	c.Assert(err, gc.IsNil)
 	c.Assert(s, gc.Equals, stringVal{"new method result"})
 }
@@ -883,10 +1097,10 @@
 	client, srvDone, _, _ := newRPCClientServer(c, srvRoot, nil, true)
 	defer closeClient(c, client, srvDone)
 
-	err := client.Call(rpc.Request{"ChangeAPIMethods", "", "RemoveAPI"}, nil, nil)
+	err := client.Call(rpc.Request{"ChangeAPIMethods", 0, "", "RemoveAPI"}, nil, nil)
 	c.Assert(err, gc.IsNil)
 
-	err = client.Call(rpc.Request{"ChangeAPIMethods", "", "RemoveAPI"}, nil, nil)
+	err = client.Call(rpc.Request{"ChangeAPIMethods", 0, "", "RemoveAPI"}, nil, nil)
 	c.Assert(err, gc.ErrorMatches, "request error: no service")
 }
 
@@ -906,11 +1120,11 @@
 
 	result := make(chan error)
 	go func() {
-		result <- client.Call(rpc.Request{"DelayedMethods", "1", "Delay"}, nil, nil)
+		result <- client.Call(rpc.Request{"DelayedMethods", 0, "1", "Delay"}, nil, nil)
 	}()
 	chanRead(c, ready, "method ready")
 
-	err := client.Call(rpc.Request{"ChangeAPIMethods", "", "ChangeAPI"}, nil, nil)
+	err := client.Call(rpc.Request{"ChangeAPIMethods", 0, "", "ChangeAPI"}, nil, nil)
 	c.Assert(err, gc.IsNil)
 
 	// Ensure that not only does the request in progress complete,
@@ -957,7 +1171,12 @@
 			role = roleBoth
 		}
 		rpcConn := rpc.NewConn(NewJSONCodec(conn, role), serverNotifier)
-		rpcConn.Serve(root, tfErr)
+		if custroot, ok := root.(*CustomCaller); ok {
+			rpcConn.ServeCaller(custroot, tfErr)
+			custroot.root.conn = rpcConn
+		} else {
+			rpcConn.Serve(root, tfErr)
+		}
 		if root, ok := root.(*Root); ok {
 			root.conn = rpcConn
 		}

=== modified file 'rpc/rpcreflect/type.go'
--- rpc/rpcreflect/type.go	2014-04-02 04:59:44 +0000
+++ rpc/rpcreflect/type.go	2014-05-22 11:41:28 +0000
@@ -73,10 +73,6 @@
 	// the same type as the root object. The given id is passed
 	// as an argument to the method.
 	Call func(rcvr reflect.Value, id string) (reflect.Value, error)
-
-	// Methods holds RPC object-method information about
-	// objects returned from the above call.
-	ObjType *ObjType
 }
 
 // TypeOf returns information on all root-level RPC methods
@@ -153,11 +149,22 @@
 			err, _ = out[1].Interface().(error)
 		}
 		r = out[0]
+		if r.Kind() == reflect.Interface && r.NumMethod() == 0 {
+			// The original function returned (interface{}, error).
+			// We want to unwrap the interface{} into its more
+			// concrete type, so that we can get the proper methods
+			// exposed.
+			// TODO: jam 2014-05-08 We could change this to always
+			// defer down to the concrete type, at the cost that to
+			// expose a subset of a type you must always expose a
+			// new struct type. (Though you can expose a struct
+			// that only embeds the interface you want.)
+			r = reflect.ValueOf(r.Interface())
+		}
 		return
 	}
 	return &RootMethod{
-		Call:    f,
-		ObjType: ObjTypeOf(t.Out(0)),
+		Call: f,
 	}
 }
 

=== modified file 'rpc/rpcreflect/value.go'
--- rpc/rpcreflect/value.go	2013-12-17 20:09:27 +0000
+++ rpc/rpcreflect/value.go	2014-05-22 11:41:28 +0000
@@ -11,15 +11,23 @@
 // CallNotImplementedError is an error, returned an attempt to call to
 // an unknown API method is made.
 type CallNotImplementedError struct {
+	RootMethod string
+	Version    int
 	Method     string
-	RootMethod string
 }
 
 func (e *CallNotImplementedError) Error() string {
 	if e.Method == "" {
+		if e.Version != 0 {
+			return fmt.Sprintf("unknown version (%d) of interface %q", e.Version, e.RootMethod)
+		}
 		return fmt.Sprintf("unknown object type %q", e.RootMethod)
 	}
-	return fmt.Sprintf("no such request - method %s.%s is not implemented", e.RootMethod, e.Method)
+	methodVersion := e.RootMethod
+	if e.Version != 0 {
+		methodVersion = fmt.Sprintf("%s(%d)", e.RootMethod, e.Version)
+	}
+	return fmt.Sprintf("no such request - method %s.%s is not implemented", methodVersion, e.Method)
 }
 
 // MethodCaller knows how to call a particular RPC method.
@@ -29,9 +37,8 @@
 	// ResultType holds the result type of the result of caling the object method.
 	ResultType reflect.Type
 
-	rootValue  reflect.Value
-	rootMethod RootMethod
-	objMethod  ObjMethod
+	objMethod ObjMethod
+	obj       reflect.Value
 }
 
 // MethodCaller holds the value of the root of an RPC server that
@@ -69,36 +76,54 @@
 // It returns an error if either the root method or the object
 // method were not found.
 // It panics if called on the zero Value.
-func (v Value) MethodCaller(rootMethodName, objMethodName string) (MethodCaller, error) {
+func (v Value) MethodCaller(rootMethodName string, rootVersion int, objId, objMethodName string) (MethodCaller, error) {
 	if !v.IsValid() {
 		panic("MethodCaller called on invalid Value")
 	}
-	caller := MethodCaller{
-		rootValue: v.rootValue,
+	if rootVersion != 0 {
+		return MethodCaller{}, &CallNotImplementedError{
+			RootMethod: rootMethodName,
+			Version:    rootVersion,
+		}
 	}
 	var err error
-	caller.rootMethod, err = v.rootType.Method(rootMethodName)
+	rootMethod, err := v.rootType.Method(rootMethodName)
 	if err != nil {
 		return MethodCaller{}, &CallNotImplementedError{
 			RootMethod: rootMethodName,
 		}
 	}
-	caller.objMethod, err = caller.rootMethod.ObjType.Method(objMethodName)
+	obj, err := rootMethod.Call(v.rootValue, objId)
+	if err != nil {
+		return MethodCaller{}, err
+	}
+	return MethodCallerFromValue(obj, rootMethodName, rootVersion, objMethodName)
+}
+
+// MethodCallerFromValue will reflect for ObjType and probe for the actual
+// method, wrapping it into a MethodCaller.
+func MethodCallerFromValue(
+	obj reflect.Value, rootMethodName string, version int, objMethodName string,
+) (
+	MethodCaller, error,
+) {
+	objType := ObjTypeOf(obj.Type())
+	objMethod, err := objType.Method(objMethodName)
 	if err != nil {
 		return MethodCaller{}, &CallNotImplementedError{
 			RootMethod: rootMethodName,
+			Version:    version,
 			Method:     objMethodName,
 		}
 	}
-	caller.ParamsType = caller.objMethod.Params
-	caller.ResultType = caller.objMethod.Result
-	return caller, nil
+	return MethodCaller{
+		ParamsType: objMethod.Params,
+		ResultType: objMethod.Result,
+		objMethod:  objMethod,
+		obj:        obj,
+	}, nil
 }
 
-func (caller MethodCaller) Call(objId string, arg reflect.Value) (reflect.Value, error) {
-	obj, err := caller.rootMethod.Call(caller.rootValue, objId)
-	if err != nil {
-		return reflect.Value{}, err
-	}
-	return caller.objMethod.Call(obj, arg)
+func (caller MethodCaller) Call(arg reflect.Value) (reflect.Value, error) {
+	return caller.objMethod.Call(caller.obj, arg)
 }

=== modified file 'rpc/server.go'
--- rpc/server.go	2014-03-05 19:41:34 +0000
+++ rpc/server.go	2014-05-22 11:41:28 +0000
@@ -69,6 +69,9 @@
 	// Type holds the type of object to act on.
 	Type string
 
+	// Version holds the version of the interface to the Object to act on
+	Version int
+
 	// Id holds the id of the object to act on.
 	Id string
 
@@ -107,8 +110,12 @@
 	// mutex guards the following values.
 	mutex sync.Mutex
 
-	// rootValue holds the value to use to serve RPC requests, if any.
-	rootValue rpcreflect.Value
+	// rootCaller holds the value to use to serve RPC requests, if any.
+	rootCaller Caller
+
+	// killer is the original interface{} that was passed to
+	// Serve/ServeCaller if it implemented the Killer interface
+	killer Killer
 
 	// transformErrors is used to transform returned errors.
 	transformErrors func(error) error
@@ -238,14 +245,44 @@
 // set of methods being served by the connection. This will have
 // no effect on calls that are currently being services.
 // If root is nil, the connection will serve no methods.
+//
+// root can optionally implement the Killer method. If implemented, when
+// the connection is closed, root.Kill() will be called.
 func (conn *Conn) Serve(root interface{}, transformErrors func(error) error) {
+	var rootCaller Caller
 	rootValue := rpcreflect.ValueOf(reflect.ValueOf(root))
-	if rootValue.IsValid() && transformErrors == nil {
+	if rootValue.IsValid() {
+		rootCaller = rootValue
+	}
+	conn.serve(rootCaller, root, transformErrors)
+}
+
+// ServeCaller serves RPC requests on the connection by invoking methods retrieved
+// from rootCaller. Note that it does not start the connection running, though
+// it may be called once the connection is already started.
+//
+// The server executes each client request by calling MethodCaller to obtain a
+// method to invoke. It invokes that method with the request parameters,
+// possibly returning some result.
+//
+// rootCaller can optionally implement the Killer method. If implemented, when
+// the connection is closed, rootCaller.Kill() will be called.
+func (conn *Conn) ServeCaller(rootCaller Caller, transformErrors func(error) error) {
+	conn.serve(rootCaller, rootCaller, transformErrors)
+}
+
+func (conn *Conn) serve(rootCaller Caller, maybeKiller interface{}, transformErrors func(error) error) {
+	if rootCaller != nil && transformErrors == nil {
 		transformErrors = func(err error) error { return err }
 	}
 	conn.mutex.Lock()
 	defer conn.mutex.Unlock()
-	conn.rootValue = rootValue
+	conn.rootCaller = rootCaller
+	if killer, ok := maybeKiller.(Killer); ok {
+		conn.killer = killer
+	} else {
+		conn.killer = nil
+	}
 	conn.transformErrors = transformErrors
 }
 
@@ -279,10 +316,8 @@
 	conn.closing = true
 	// Kill server requests if appropriate.  Client requests will be
 	// terminated when the input loop finishes.
-	if conn.rootValue.IsValid() {
-		if killer, ok := conn.rootValue.GoValue().Interface().(Killer); ok {
-			killer.Kill()
-		}
+	if conn.killer != nil {
+		conn.killer.Kill()
 	}
 	conn.mutex.Unlock()
 
@@ -305,6 +340,14 @@
 	ErrorCode() string
 }
 
+// Caller is a way to dynamically lookup methods from a given (rootName,
+// version, id, methodName) quadruplet.
+type Caller interface {
+	// MethodCaller takes a description of a method to call, and returns an
+	// rpcreflect.MethodCaller that can be used to call the concrete method.
+	MethodCaller(rootMethodName string, version int, objId, objMethodName string) (rpcreflect.MethodCaller, error)
+}
+
 // Killer represents a type that can be asked to abort any outstanding
 // requests.  The Kill method should return immediately.
 type Killer interface {
@@ -373,8 +416,8 @@
 		if err := conn.readBody(nil, true); err != nil {
 			return err
 		}
-		// We don't transform the error because there
-		// may be no transformErrors function available.
+		// We don't transform the error here. bindRequest will have
+		// already transformed it and returned a zero req.
 		return conn.writeErrorResponse(hdr, err, startTime)
 	}
 	var argp interface{}
@@ -455,20 +498,23 @@
 // a boundRequest that can call those methods.
 func (conn *Conn) bindRequest(hdr *Header) (boundRequest, error) {
 	conn.mutex.Lock()
-	rootValue := conn.rootValue
+	rootCaller := conn.rootCaller
 	transformErrors := conn.transformErrors
 	conn.mutex.Unlock()
 
-	if !rootValue.IsValid() {
+	if conn.rootCaller == nil {
 		return boundRequest{}, fmt.Errorf("no service")
 	}
-	caller, err := rootValue.MethodCaller(hdr.Request.Type, hdr.Request.Action)
+	caller, err := rootCaller.MethodCaller(
+		hdr.Request.Type, hdr.Request.Version, hdr.Request.Id, hdr.Request.Action)
 	if err != nil {
 		if _, ok := err.(*rpcreflect.CallNotImplementedError); ok {
 			err = &serverError{
 				Message: err.Error(),
 				Code:    CodeNotImplemented,
 			}
+		} else {
+			err = transformErrors(err)
 		}
 		return boundRequest{}, err
 	}
@@ -482,7 +528,7 @@
 // runRequest runs the given request and sends the reply.
 func (conn *Conn) runRequest(req boundRequest, arg reflect.Value, startTime time.Time) {
 	defer conn.srvPending.Done()
-	rv, err := req.Call(req.hdr.Request.Id, arg)
+	rv, err := req.Call(arg)
 	if err != nil {
 		err = conn.writeErrorResponse(&req.hdr, req.transformErrors(err), startTime)
 	} else {

=== modified file 'state/apiserver/root_test.go'
--- state/apiserver/root_test.go	2014-05-20 08:02:01 +0000
+++ state/apiserver/root_test.go	2014-05-22 11:41:28 +0000
@@ -8,7 +8,6 @@
 
 	gc "launchpad.net/gocheck"
 
-	"launchpad.net/juju-core/rpc/rpcreflect"
 	"launchpad.net/juju-core/state/apiserver"
 	"launchpad.net/juju-core/testing"
 )
@@ -27,23 +26,26 @@
 	"GetAuthTag",
 }
 
-func (*rootSuite) TestDiscardedAPIMethods(c *gc.C) {
-	t := rpcreflect.TypeOf(apiserver.RootType)
-	// We must have some root-level methods.
-	c.Assert(t.MethodNames(), gc.Not(gc.HasLen), 0)
-	c.Assert(t.DiscardedMethods(), gc.DeepEquals, allowedDiscardedMethods)
-
-	for _, name := range t.MethodNames() {
-		m, err := t.Method(name)
-		c.Assert(err, gc.IsNil)
-		// We must have some methods on every object returned
-		// by a root-level method.
-		c.Assert(m.ObjType.MethodNames(), gc.Not(gc.HasLen), 0)
-		// We don't allow any methods that don't implement
-		// an RPC entry point.
-		c.Assert(m.ObjType.DiscardedMethods(), gc.HasLen, 0)
-	}
-}
+// TODO: jam 2014-05-11 reintroduce this test once we properly have types
+// registered that can be reflected again. This was only disabled because we
+// got rid of ObjType as an exposed attribute of RootType
+// func (*rootSuite) TestDiscardedAPIMethods(c *gc.C) {
+// 	t := rpcreflect.TypeOf(apiserver.RootType)
+// 	// We must have some root-level methods.
+// 	c.Assert(t.MethodNames(), gc.Not(gc.HasLen), 0)
+// 	c.Assert(t.DiscardedMethods(), gc.DeepEquals, allowedDiscardedMethods)
+//
+// 	for _, name := range t.MethodNames() {
+// 		m, err := t.Method(name)
+// 		c.Assert(err, gc.IsNil)
+// 		// We must have some methods on every object returned
+// 		// by a root-level method.
+// 		c.Assert(m.ObjType.MethodNames(), gc.Not(gc.HasLen), 0)
+// 		// We don't allow any methods that don't implement
+// 		// an RPC entry point.
+// 		c.Assert(m.ObjType.DiscardedMethods(), gc.HasLen, 0)
+// 	}
+// }
 
 func (r *rootSuite) TestPingTimeout(c *gc.C) {
 	closedc := make(chan time.Time, 1)

=== modified file 'utils/registry/registry.go'
--- utils/registry/registry.go	2014-05-16 11:48:18 +0000
+++ utils/registry/registry.go	2014-05-22 11:41:28 +0000
@@ -21,7 +21,7 @@
 	versions     map[string]Versions
 }
 
-// NewTypedNameVersion creates a place to register your objects
+// NewTypedNameVersion creates a place to register your objects.
 func NewTypedNameVersion(requiredType reflect.Type) *TypedNameVersion {
 	return &TypedNameVersion{
 		requiredType: requiredType,
@@ -39,11 +39,9 @@
 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.
+// 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())
@@ -65,7 +63,7 @@
 }
 
 // descriptionFromVersions aggregates the information in a Versions map into a
-// more friendly form for List()
+// more friendly form for List().
 func descriptionFromVersions(name string, versions Versions) Description {
 	intVersions := make([]int, 0, len(versions))
 	for version := range versions {
@@ -94,7 +92,7 @@
 }
 
 // Get returns the object for a single name and version. If the requested
-// facade is not found, it returns error.NotFound
+// 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 {

=== modified file 'utils/registry/registry_test.go'
--- utils/registry/registry_test.go	2014-05-20 04:27:02 +0000
+++ utils/registry/registry_test.go	2014-05-22 11:41:28 +0000
@@ -6,10 +6,10 @@
 import (
 	"reflect"
 
+	"github.com/juju/errors"
 	jc "github.com/juju/testing/checkers"
 	gc "launchpad.net/gocheck"
 
-	"github.com/juju/errors"
 	"launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/utils/registry"
 )