state/apiserver/common: Registry tracks types

state/apiserver/common: Registry tracks types

This changes the Facades registry so that instead of just tracking the
FacadeFactory functions, it also explicitly tracks the Facade type
itself. The main benefit from doing this is that we have a proper (and
passing) TestDiscardedAPIMethods that allows us to introspect everything
we are exposing over the API without having to actually instantiate any
of those objects.

While the only specific win today is the test case, the wins for the
longer term are:

1) The ability to walk the whole API and determine all Facade versions
along with all methods that are available.

2) If we so choose, we can restore the "introspect a Root object"
(rpc.Serve vs the new rpc.ServeCaller) and have it go back to doing
exactly what it did with hardcoded types per exposed facade. That was
more code right now, which meant delaying getting API versioning out
there.

3) It helps enforce the norm that (Facade, version) describes exactly 1
type, so we don't accidently do weird things and return variable APIs
based on the "id" of requests.

I don't yet have code that ensures the objects being returned by the
factory exactly match the Type that they've been registered with. It
didn't seem to be a big win given that most places use
RegisterStandardFacade that explicitly does the registration by
inferring the type from the function that is handed in.

However, "RegisterFacade(Factory, Type)" does define Factory as
returning interface{} and Type as whatever you want. So if we want a bit
more rigidity, we could add that here.

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

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

(do not edit description out of merge proposal)

[jameinel, 2014-05-22 09:07:19]

Please take a look.

[dimitern, 2014-05-22 10:24:00]

LGTM with a few minor formatting suggestions, otherwise the code looks fine.

https://codereview.appspot.com/97630045/diff/1/state/apiserver/common/registr...
File state/apiserver/common/registry_test.go (left):

https://codereview.appspot.com/97630045/diff/1/state/apiserver/common/registr...
state/apiserver/common/registry_test.go:196: // We use a FakeAuthorizer that
says it is allowed to do everything, so
How come we don't need the authorizer anymore?

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

https://codereview.appspot.com/97630045/diff/1/state/apiserver/common/registr...
state/apiserver/common/registry_test.go:42: common.RegisterFacade("myfacade", 0,
testFacade,
The indentation here looks a bit weird, how about: 

common.RegisterFacade(
    "myfacade", 0, testFacade,
    reflect.TypeOf(&v).Elem(),
)

It's more of a personal preference (esp. the comma before the last closing
parenthesis). This applies to all similar cases below, if you decide to do it.

https://codereview.appspot.com/97630045/diff/1/state/apiserver/upgrader/upgra...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/97630045/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:24: common.RegisterFacade("Upgrader", 0,
upgraderFacade,
Indentation here, as well (see previous comment).

https://codereview.appspot.com/97630045/diff/1/state/apiserver/watcher.go
File state/apiserver/watcher.go (right):

https://codereview.appspot.com/97630045/diff/1/state/apiserver/watcher.go#new...
state/apiserver/watcher.go:16: common.RegisterFacade(
See - it looks better like it is here.

[jameinel, 2014-05-22 11:07:37]

Please take a look.

https://codereview.appspot.com/97630045/diff/1/state/apiserver/common/registr...
File state/apiserver/common/registry_test.go (left):

https://codereview.appspot.com/97630045/diff/1/state/apiserver/common/registr...
state/apiserver/common/registry_test.go:196: // We use a FakeAuthorizer that
says it is allowed to do everything, so
On 2014/05/22 10:24:00, dimitern wrote:
> How come we don't need the authorizer anymore?

We are now inspecting the registered Types rather than actually instantiating an
object and inspecting it.
You need an Authorizor for objects to actually be created (because most of the
NewKeyManager style functions actually check the Authorizer before they return
anything.)

This also had the bug that the inspection *just didn't work* for the Watcher
APIs because those have to have a concrete watcher already registered with a
given Id.

So this gets around the problem by tracking the types in the registry, so that
we can inspect what APIs they make available without having to actually
instantiate those objects.

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

https://codereview.appspot.com/97630045/diff/1/state/apiserver/common/registr...
state/apiserver/common/registry_test.go:42: common.RegisterFacade("myfacade", 0,
testFacade,
On 2014/05/22 10:24:00, dimitern wrote:
> The indentation here looks a bit weird, how about: 
> 
> common.RegisterFacade(
>     "myfacade", 0, testFacade,
>     reflect.TypeOf(&v).Elem(),
> )
> 
> It's more of a personal preference (esp. the comma before the last closing
> parenthesis). This applies to all similar cases below, if you decide to do it.

So it turns out the line length wasn't very long, so I just wrapped all of them
back up into one line. But I'll keep it in mind in the future.

https://codereview.appspot.com/97630045/diff/1/state/apiserver/upgrader/upgra...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/97630045/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:24: common.RegisterFacade("Upgrader", 0,
upgraderFacade,
On 2014/05/22 10:24:00, dimitern wrote:
> Indentation here, as well (see previous comment).

Done.

https://codereview.appspot.com/97630045/diff/1/state/apiserver/watcher.go
File state/apiserver/watcher.go (right):

https://codereview.appspot.com/97630045/diff/1/state/apiserver/watcher.go#new...
state/apiserver/watcher.go:16: common.RegisterFacade(
On 2014/05/22 10:24:00, dimitern wrote:
> See - it looks better like it is here.

Done.