Merge pull request #13878 from arnodel/juju-819-remove-connection-cli…

…ent-method

#13878

The `api.Connection` interface has a `Client` method returning an `*api.Client` for the connection. There is a comment above it indicating that this should not be part of the `Connection` interface.

This PR replaces the method with a function `api.NewClient(Connection) *api.Client`. A consequence of this is that the `api.Client` type is decoupled from `api.state` as it is only interacts with the `Connection` interface.

Some tests do reach into the underlying `state` data structure of a Client's Connection instance. In this case the Connection is coerced to a `*state` to make it work. This already happens elsewhere in the testing code (not a very good excuse I know).

api/apiclient_test.go

@@ -321,7 +321,7 @@ func (s *apiclientSuite) TestOpenHonorsModelTag(c *gc.C) {
 }
 
 func (s *apiclientSuite) TestServerRoot(c *gc.C) {
-	url := api.ServerRoot(s.APIState.Client())
+	url := api.ServerRoot(api.NewClient(s.APIState))
 	c.Assert(url, gc.Matches, "https://localhost:[0-9]+")
 }
 

api/client.go

@@ -46,7 +46,14 @@ const websocketTimeout = 30 * time.Second
 type Client struct {
 	base.ClientFacade
 	facade base.FacadeCaller
-	st     *state
+	conn   Connection
+}
+
+// NewClient returns an object that can be used to access client-specific
+// functionality.
+func NewClient(c Connection) *Client {
+	frontend, backend := base.NewClientFacade(c, "Client")
+	return &Client{ClientFacade: frontend, facade: backend, conn: c}
 }
 
 // Status returns the status of the juju model.
@@ -236,7 +243,7 @@ func (c *Client) SetModelConstraints(constraints constraints.Value) error {
 // ModelUUID returns the model UUID from the client connection
 // and reports whether it is valued.
 func (c *Client) ModelUUID() (string, bool) {
-	tag, ok := c.st.ModelTag()
+	tag, ok := c.conn.ModelTag()
 	if !ok {
 		return "", false
 	}
@@ -268,15 +275,15 @@ func (c *Client) WatchAll() (*AllWatcher, error) {
 	if err := c.facade.FacadeCall("WatchAll", nil, &info); err != nil {
 		return nil, err
 	}
-	return NewAllWatcher(c.st, &info.AllWatcherId), nil
+	return NewAllWatcher(c.conn, &info.AllWatcherId), nil
 }
 
 // Close closes the Client's underlying State connection
 // Client is unique among the api.State facades in closing its own State
 // connection, but it is conventional to use a Client object without any access
 // to its underlying state connection.
 func (c *Client) Close() error {
-	return c.st.Close()
+	return c.conn.Close()
 }
 
 // SetModelAgentVersion sets the model agent-version setting
@@ -541,7 +548,7 @@ func openCharmArgs(curl *charm.URL) (string, url.Values) {
 
 // OpenURI performs a GET on a Juju HTTP endpoint returning the
 func (c *Client) OpenURI(uri string, query url.Values) (io.ReadCloser, error) {
-	return openURI(c.st, uri, query)
+	return openURI(c.conn, uri, query)
 }
 
 func openURI(apiCaller base.APICaller, uri string, query url.Values) (io.ReadCloser, error) {
@@ -595,7 +602,7 @@ func (c *Client) httpPost(content io.ReadSeeker, endpoint, contentType string, r
 	req.Header.Set("Content-Type", contentType)
 
 	// The returned httpClient sets the base url to /model/<uuid> if it can.
-	httpClient, err := c.st.HTTPClient()
+	httpClient, err := c.conn.HTTPClient()
 	if err != nil {
 		return errors.Trace(err)
 	}
@@ -698,7 +705,7 @@ func (s *DeadlineStream) WriteJSON(v interface{}) error {
 // WatchDebugLog returns a channel of structured Log Messages. Only log entries
 // that match the filtering specified in the DebugLogParams are returned.
 func (c *Client) WatchDebugLog(args common.DebugLogParams) (<-chan common.LogMessage, error) {
-	return common.StreamDebugLog(context.TODO(), c.st, args)
+	return common.StreamDebugLog(context.TODO(), c.conn, args)
 }
 
 // lxdCharmProfiler massages a charm.Charm into a LXDProfiler inside of the

api/client_macaroon_test.go

@@ -32,7 +32,7 @@ func (s *clientMacaroonSuite) createTestClient(c *gc.C) *api.Client {
 	s.AddControllerUser(c, username, permission.LoginAccess)
 	cookieJar := apitesting.NewClearableCookieJar()
 	s.DischargerLogin = func() string { return username }
-	client := s.OpenAPI(c, nil, cookieJar).Client()
+	client := api.NewClient(s.OpenAPI(c, nil, cookieJar))
 
 	// Even though we've logged into the API, we want
 	// the tests below to exercise the discharging logic

api/client_test.go

@@ -57,7 +57,7 @@ func (s *clientSuite) SetUpTest(c *gc.C) {
 }
 
 func (s *clientSuite) TestCloseMultipleOk(c *gc.C) {
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 	c.Assert(client.Close(), gc.IsNil)
 	c.Assert(client.Close(), gc.IsNil)
 	c.Assert(client.Close(), gc.IsNil)
@@ -67,7 +67,7 @@ func (s *clientSuite) TestUploadToolsOtherModel(c *gc.C) {
 	otherSt, otherAPISt := s.otherModel(c)
 	defer otherSt.Close()
 	defer otherAPISt.Close()
-	client := otherAPISt.Client()
+	client := api.NewClient(otherAPISt)
 	newVersion := version.MustParseBinary("5.4.3-ubuntu-amd64")
 	var called bool
 
@@ -145,7 +145,7 @@ func (s *clientSuite) TestAddLocalCharm(c *gc.C) {
 	curl := charm.MustParseURL(
 		fmt.Sprintf("local:quantal/%s-%d", charmArchive.Meta().Name, charmArchive.Revision()),
 	)
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 
 	// Test the sanity checks first.
 	_, err := client.AddLocalCharm(charm.MustParseURL("cs:quantal/wordpress-1"), nil, false)
@@ -190,7 +190,7 @@ func (s *clientSuite) TestAddLocalCharmWithLXDProfile(c *gc.C) {
 	curl := charm.MustParseURL(
 		fmt.Sprintf("local:quantal/%s-%d", charmArchive.Meta().Name, charmArchive.Revision()),
 	)
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 
 	// Upload an archive with its original revision.
 	savedURL, err := client.AddLocalCharm(curl, charmArchive, false)
@@ -215,7 +215,7 @@ func (s *clientSuite) TestAddLocalCharmWithInvalidLXDProfile(c *gc.C) {
 	curl := charm.MustParseURL(
 		fmt.Sprintf("local:quantal/%s-%d", charmArchive.Meta().Name, charmArchive.Revision()),
 	)
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 
 	// Upload an archive with its original revision.
 	_, err := client.AddLocalCharm(curl, charmArchive, false)
@@ -235,7 +235,7 @@ func (s *clientSuite) testAddLocalCharmWithWithForceSucceeds(name string, c *gc.
 	curl := charm.MustParseURL(
 		fmt.Sprintf("local:quantal/%s-%d", charmArchive.Meta().Name, charmArchive.Revision()),
 	)
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 
 	// Upload an archive with its original revision.
 	savedURL, err := client.AddLocalCharm(curl, charmArchive, true)
@@ -258,7 +258,7 @@ func (s *clientSuite) testAddLocalCharmWithWithForceSucceeds(name string, c *gc.
 func (s *clientSuite) assertAddLocalCharmFailed(c *gc.C, f func(string) (bool, error), msg string) {
 	curl, ch := s.testCharm(c)
 	s.PatchValue(api.HasHooksOrDispatch, f)
-	_, err := s.APIState.Client().AddLocalCharm(curl, ch, false)
+	_, err := api.NewClient(s.APIState).AddLocalCharm(curl, ch, false)
 	c.Assert(err, gc.ErrorMatches, msg)
 }
 
@@ -267,7 +267,7 @@ func (s *clientSuite) TestAddLocalCharmDefinetelyWithHooks(c *gc.C) {
 	s.PatchValue(api.HasHooksOrDispatch, func(string) (bool, error) {
 		return true, nil
 	})
-	savedCURL, err := s.APIState.Client().AddLocalCharm(curl, ch, false)
+	savedCURL, err := api.NewClient(s.APIState).AddLocalCharm(curl, ch, false)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(savedCURL.String(), gc.Equals, curl.String())
 }
@@ -289,7 +289,7 @@ func (s *clientSuite) TestAddLocalCharmOtherModel(c *gc.C) {
 	otherSt, otherAPISt := s.otherModel(c)
 	defer otherSt.Close()
 	defer otherAPISt.Close()
-	client := otherAPISt.Client()
+	client := api.NewClient(otherAPISt)
 
 	// Upload an archive
 	savedURL, err := client.AddLocalCharm(curl, charmArchive, false)
@@ -313,7 +313,7 @@ func (s *clientSuite) otherModel(c *gc.C) (*state.State, api.Connection) {
 }
 
 func (s *clientSuite) TestAddLocalCharmError(c *gc.C) {
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 
 	// AddLocalCharm does not use the facades, so instead of patching the
 	// facade call, we set up a fake endpoint to test.
@@ -356,7 +356,7 @@ func (s *clientSuite) TestMinVersionLocalCharm(c *gc.C) {
 		{"1.25.0", "1.25-alpha1", true, true},
 		{"1.25-alpha1", "1.25.0", true, true},
 	}
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 	for _, t := range tests {
 		testMinVer(client, t, c)
 	}
@@ -413,7 +413,7 @@ func (s *clientSuite) TestOpenURIFound(c *gc.C) {
 	const toolsVersion = "2.0.0-ubuntu-ppc64"
 	s.AddToolsToState(c, version.MustParseBinary(toolsVersion))
 
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 	reader, err := client.OpenURI("/tools/"+toolsVersion, nil)
 	c.Assert(err, jc.ErrorIsNil)
 	defer reader.Close()
@@ -425,13 +425,13 @@ func (s *clientSuite) TestOpenURIFound(c *gc.C) {
 }
 
 func (s *clientSuite) TestOpenURIError(c *gc.C) {
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 	_, err := client.OpenURI("/tools/foobar", nil)
 	c.Assert(err, gc.ErrorMatches, ".*error parsing version.+")
 }
 
 func (s *clientSuite) TestOpenCharmFound(c *gc.C) {
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 	curl, ch, repoPath := addLocalCharm(c, client, "dummy", false)
 	defer os.Remove(repoPath)
 	c.Logf("added local charm as %v", curl)
@@ -448,7 +448,7 @@ func (s *clientSuite) TestOpenCharmFound(c *gc.C) {
 }
 
 func (s *clientSuite) TestOpenCharmFoundWithForceStillSucceeds(c *gc.C) {
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 	curl, ch, repoPath := addLocalCharm(c, client, "dummy", true)
 	defer os.Remove(repoPath)
 	expected, err := ioutil.ReadFile(ch.Path)
@@ -466,7 +466,7 @@ func (s *clientSuite) TestOpenCharmFoundWithForceStillSucceeds(c *gc.C) {
 
 func (s *clientSuite) TestOpenCharmMissing(c *gc.C) {
 	curl := charm.MustParseURL("cs:quantal/spam-3")
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 
 	_, err := client.OpenCharm(curl)
 
@@ -509,14 +509,14 @@ func (s *clientSuite) TestClientModelUUID(c *gc.C) {
 	model, err := s.State.Model()
 	c.Assert(err, jc.ErrorIsNil)
 
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 	uuid, ok := client.ModelUUID()
 	c.Assert(ok, jc.IsTrue)
 	c.Assert(uuid, gc.Equals, model.Tag().Id())
 }
 
 func (s *clientSuite) TestClientModelUsers(c *gc.C) {
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 	cleanup := api.PatchClientFacadeCall(client,
 		func(request string, paramsIn interface{}, response interface{}) error {
 			c.Assert(paramsIn, gc.IsNil)
@@ -546,7 +546,7 @@ func (s *clientSuite) TestClientModelUsers(c *gc.C) {
 }
 
 func (s *clientSuite) TestWatchDebugLogConnected(c *gc.C) {
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 	// Use the no tail option so we don't try to start a tailing cursor
 	// on the oplog when there is no oplog configured in mongo as the tests
 	// don't set up mongo in replicaset mode.
@@ -643,7 +643,7 @@ func (s *clientSuite) TestWatchDebugLogParamsEncoded(c *gc.C) {
 		StartTime:     time.Date(2016, 11, 30, 11, 48, 0, 100, time.UTC),
 	}
 
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 	_, err := client.WatchDebugLog(params)
 	c.Assert(err, jc.ErrorIsNil)
 
@@ -707,7 +707,7 @@ func (s *clientSuite) TestOpenUsesModelUUIDPaths(c *gc.C) {
 }
 
 func (s *clientSuite) TestAbortCurrentUpgrade(c *gc.C) {
-	client := s.APIState.Client()
+	client := api.NewClient(s.APIState)
 	someErr := errors.New("random")
 	cleanup := api.PatchClientFacadeCall(client,
 		func(request string, args interface{}, response interface{}) error {

api/export_test.go

@@ -48,13 +48,13 @@ type RPCConnection rpcConnection
 // SetServerAddress allows changing the URL to the internal API server
 // that AddLocalCharm uses in order to test NotImplementedError.
 func SetServerAddress(c *Client, scheme, addr string) {
-	c.st.serverScheme = scheme
-	c.st.addr = addr
+	c.conn.(*state).serverScheme = scheme
+	c.conn.(*state).addr = addr
 }
 
 // ServerRoot is exported so that we can test the built URL.
 func ServerRoot(c *Client) string {
-	return c.st.serverRoot()
+	return c.conn.(*state).serverRoot()
 }
 
 // UnderlyingConn returns the underlying transport connection.
@@ -114,7 +114,7 @@ func NewTestingState(params TestingStateParams) Connection {
 // an error state (anything else is likely to panic.)
 func APIClient(apiCaller base.APICallCloser) *Client {
 	frontend, backend := base.NewClientFacade(apiCaller, "Client")
-	return &Client{ClientFacade: frontend, facade: backend, st: &state{}}
+	return &Client{ClientFacade: frontend, facade: backend, conn: &state{}}
 }
 
 // PatchClientFacadeCall changes the internal FacadeCaller to one that lets

api/interface.go

@@ -322,11 +322,7 @@ type Connection interface {
 	// associated with.
 	CookieURL() *url.URL
 
-	// These methods expose a bunch of worker-specific facades, and basically
-	// just should not exist; but removing them is too noisy for a single CL.
-	// Client in particular is intimately coupled with State -- and the others
-	// will be easy to remove, but until we're using them via manifolds it's
-	// prohibitively ugly to do so.
-	Client() *Client
+	// These method exposes aworker-specific facade, and basically just should
+	// not exist; but removing it is too noisy for a single CL.
 	Reboot() (reboot.State, error)
 }

api/state.go

@@ -21,7 +21,6 @@ import (
 
 	"github.com/juju/juju/api/agent/keyupdater"
 	"github.com/juju/juju/api/agent/reboot"
-	"github.com/juju/juju/api/base"
 	"github.com/juju/juju/core/network"
 	"github.com/juju/juju/feature"
 	"github.com/juju/juju/rpc"
@@ -303,13 +302,6 @@ func addAddress(servers []network.MachineHostPorts, addr string) ([]network.Mach
 	return result, nil
 }
 
-// Client returns an object that can be used
-// to access client-specific functionality.
-func (st *state) Client() *Client {
-	frontend, backend := base.NewClientFacade(st, "Client")
-	return &Client{ClientFacade: frontend, facade: backend, st: st}
-}
-
 // Reboot returns access to the Reboot API
 func (st *state) Reboot() (reboot.State, error) {
 	switch tag := st.authTag.(type) {