launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/gomaasapi/pre-getsubresource into lp:gomaasapi.

Commit message:
Preparations for GetSubResource branch: clean up some test duplication (before I make it worse), add some missing error checking, cover a neglected aspect of absolute paths in testing.  Also, make the error from a failed HTTP request a bit more accessible and recognizable.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/gomaasapi/pre-getsubresource/+merge/148976

I'm working on an additional MAASObject method: GetSubResource.  It looks as if its testing will need the new helper.


Jeroen
-- 
https://code.launchpad.net/~jtv/gomaasapi/pre-getsubresource/+merge/148976
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/gomaasapi/pre-getsubresource into lp:gomaasapi.

=== modified file 'client.go'
--- client.go	2013-02-13 11:16:55 +0000
+++ client.go	2013-02-18 04:22:21 +0000
@@ -22,11 +22,19 @@
 	Signer  OAuthSigner
 }
 
+// ServerError is an http error (or at least, a non-2xx result) received from
+// the server.  It contains the numerical HTTP status code as well as an error
+// string.
+type ServerError struct {
+	error
+	StatusCode int
+}
+
 // dispatchRequest sends a request to the server, and interprets the response.
-// Client-side error will return an empty response and a non-nil error but
-// for server-side errors (i.e. responses with a non 2XX status code), the
-// returned error will contain a warning and the body of the response will
-// still be returned.
+// Client-side errors will return an empty response and a non-nil error.  For
+// server-side errors however (i.e. responses with a non 2XX status code), the
+// returned error will be ServerError and the returned body will reflect the
+// server's response.
 func (client Client) dispatchRequest(request *http.Request) ([]byte, error) {
 	client.Signer.OAuthSign(request)
 	httpClient := http.Client{}
@@ -39,7 +47,8 @@
 		return nil, err
 	}
 	if response.StatusCode < 200 || response.StatusCode > 299 {
-		return body, fmt.Errorf("gomaasapi: got error back from server: %v", response.Status)
+		msg := fmt.Errorf("gomaasapi: got error back from server: %v", response.Status)
+		return body, ServerError{error: msg, StatusCode: response.StatusCode}
 	}
 	return body, nil
 }

=== modified file 'client_test.go'
--- client_test.go	2013-02-13 11:16:55 +0000
+++ client_test.go	2013-02-18 04:22:21 +0000
@@ -16,7 +16,7 @@
 
 var _ = Suite(&ClientSuite{})
 
-func (suite *ClientSuite) TestClientdispatchRequestReturnsError(c *C) {
+func (suite *ClientSuite) TestClientdispatchRequestReturnsServerError(c *C) {
 	URI := "/some/url/?param1=test"
 	expectedResult := "expected:result"
 	server := newSingleServingServer(URI, expectedResult, http.StatusBadRequest)
@@ -27,9 +27,26 @@
 	result, err := client.dispatchRequest(request)
 
 	c.Check(err, ErrorMatches, "gomaasapi: got error back from server: 400 Bad Request.*")
+	c.Check(err.(ServerError).StatusCode, Equals, 400)
 	c.Check(string(result), Equals, expectedResult)
 }
 
+func (suite *ClientSuite) TestClientDispatchRequestReturnsNonServerError(c *C) {
+	client, err := NewAnonymousClient("/foo")
+	c.Assert(err, IsNil)
+	// Create a bad request that will fail to dispatch.
+	request, err := http.NewRequest("GET", "/", nil)
+	c.Assert(err, IsNil)
+
+	result, err := client.dispatchRequest(request)
+
+	// This type of failure is an error, but not a ServerError.
+	c.Check(err, NotNil)
+	c.Check(err, Not(FitsTypeOf), ServerError{})
+	// For this kind of error, result is guaranteed to be nil.
+	c.Check(result, IsNil)
+}
+
 func (suite *ClientSuite) TestClientdispatchRequestSignsRequest(c *C) {
 	URI := "/some/url/?param1=test"
 	expectedResult := "expected:result"

=== modified file 'maasobject_test.go'
--- maasobject_test.go	2013-02-12 09:12:12 +0000
+++ maasobject_test.go	2013-02-18 04:22:21 +0000
@@ -82,16 +82,19 @@
 }
 
 func (suite *MAASObjectSuite) TestNewJSONMAASObjectSetsUpURI(c *C) {
-	URI, _ := url.Parse("http://example.com/a/resource";)
+	URI, err := url.Parse("http://example.com/a/resource";)
+	c.Assert(err, IsNil)
 	attrs := map[string]interface{}{resourceURI: URI.String()}
 	obj := newJSONMAASObject(attrs, Client{})
 	c.Check(obj.uri, DeepEquals, URI)
 }
 
 func (suite *MAASObjectSuite) TestURL(c *C) {
-	baseURL, _ := url.Parse("http://example.com/";)
+	baseURL, err := url.Parse("http://example.com/";)
+	c.Assert(err, IsNil)
 	uri := "http://example.com/a/resource";
-	resourceURL, _ := url.Parse(uri)
+	resourceURL, err := url.Parse(uri)
+	c.Assert(err, IsNil)
 	input := map[string]interface{}{resourceURI: uri}
 	client := Client{BaseURL: baseURL}
 	obj := newJSONMAASObject(input, client)
@@ -101,35 +104,62 @@
 	c.Check(URL, DeepEquals, resourceURL)
 }
 
+// makeFakeMAASObject creates a MAASObject for some imaginary resource.
+// There is no actual HTTP service or resource attached.
+// serviceURL is the base URL of the service, and resourceURI is the path for
+// the object, relative to serviceURL.
+func makeFakeMAASObject(serviceURL, resourcePath string) MAASObject {
+	baseURL, err := url.Parse(serviceURL)
+	if err != nil {
+		panic(fmt.Errorf("creation of fake object failed: %v", err))
+	}
+	uri := serviceURL + resourcePath
+	input := map[string]interface{}{resourceURI: uri}
+	client := Client{BaseURL: baseURL}
+	return newJSONMAASObject(input, client)
+}
+
+// Passing GetSubObject a relative path effectively concatenates that path to
+// the original object's resource URI.
 func (suite *MAASObjectSuite) TestGetSubObjectRelative(c *C) {
-	baseURL, _ := url.Parse("http://example.com/";)
-	uri := "http://example.com/a/resource/";
-	input := map[string]interface{}{resourceURI: uri}
-	client := Client{BaseURL: baseURL}
-	obj := newJSONMAASObject(input, client)
-	subName := "test"
+	obj := makeFakeMAASObject("http://example.com/";, "a/resource/")
 
-	subObj := obj.GetSubObject(subName)
+	subObj := obj.GetSubObject("test")
 	subURL := subObj.URL()
 
 	// uri ends with a slash and subName starts with one, but the two paths
 	// should be concatenated as "http://example.com/a/resource/test/";.
-	expectedSubURL, _ := url.Parse("http://example.com/a/resource/test/";)
+	expectedSubURL, err := url.Parse("http://example.com/a/resource/test/";)
+	c.Assert(err, IsNil)
 	c.Check(subURL, DeepEquals, expectedSubURL)
 }
 
+// Passing GetSubObject an absolute path effectively substitutes that path for
+// the path component in the original object's resource URI.
 func (suite *MAASObjectSuite) TestGetSubObjectAbsolute(c *C) {
-	baseURL, _ := url.Parse("http://example.com/";)
-	uri := "http://example.com/a/resource/";
-	input := map[string]interface{}{resourceURI: uri}
-	client := Client{BaseURL: baseURL}
-	obj := newJSONMAASObject(input, client)
-	subName := "/b/test"
-
-	subObj := obj.GetSubObject(subName)
-	subURL := subObj.URL()
-
-	expectedSubURL, _ := url.Parse("http://example.com/b/test/";)
+	obj := makeFakeMAASObject("http://example.com/";, "a/resource/")
+
+	subObj := obj.GetSubObject("/b/test")
+	subURL := subObj.URL()
+
+	expectedSubURL, err := url.Parse("http://example.com/b/test/";)
+	c.Assert(err, IsNil)
+	c.Check(subURL, DeepEquals, expectedSubURL)
+}
+
+// An absolute path passed to GetSubObject is rooted at the server root, not
+// at the service root.  So every absolute resource URI must repeat the part
+// of the path that leads to the service root.  This does not double that part
+// of the URI.
+func (suite *MAASObjectSuite) TestGetSubObjectAbsoluteDoesNotIncludeServiceRoot(c *C) {
+	obj := makeFakeMAASObject("http://example.com/service";, "a/resource/")
+
+	subObj := obj.GetSubObject("/service/test")
+	subURL := subObj.URL()
+
+	// The "/service" part is not repeated; it must be included.
+	expectedSubURL, err := url.Parse("http://example.com/service/test/";)
+	c.Assert(err, IsNil)
 	c.Check(subURL, DeepEquals, expectedSubURL)
 }