launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/gomaasapi-simplifyURL into lp:~maas-maintainers/maas/gomaasapi.

Commit message:
Add uri field to MAASObject. Create a utility method newjsonMAASObject to create a valid MAASObject and populate obj.uri.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~rvb/maas/gomaasapi-simplifyURL/+merge/145337
-- 
https://code.launchpad.net/~rvb/maas/gomaasapi-simplifyURL/+merge/145337
Your team MAAS Maintainers is requested to review the proposed merge of lp:~rvb/maas/gomaasapi-simplifyURL into lp:~maas-maintainers/maas/gomaasapi.

=== modified file 'example/live_example.go'
--- example/live_example.go	2013-01-28 13:17:41 +0000
+++ example/live_example.go	2013-01-29 09:52:25 +0000
@@ -46,8 +46,7 @@
 	for index, nodeObj := range listNodes {
 		node, _ := nodeObj.GetMAASObject()
 		hostname, _ := node.GetField("hostname")
-		nodeURL, _ := node.URL()
-		fmt.Printf("Node #%d is named '%v' (%v)\n", index, hostname, nodeURL)
+		fmt.Printf("Node #%d is named '%v' (%v)\n", index, hostname, node.URL())
 	}
 
 	// Create a node.
@@ -59,8 +58,7 @@
 	}
 	newNode, _ := newNodeObj.GetMAASObject()
 	newNodeName, _ := newNode.GetField("hostname")
-	newNodeURL, _ := newNode.URL()
-	fmt.Printf("New node created: %s (%s)\n", newNodeName, newNodeURL)
+	fmt.Printf("New node created: %s (%s)\n", newNodeName, newNode.URL())
 
 	// Update the new node.
 	fmt.Println("Updating the new node...")

=== modified file 'jsonobject.go'
--- jsonobject.go	2013-01-28 11:36:51 +0000
+++ jsonobject.go	2013-01-29 09:52:25 +0000
@@ -86,7 +86,7 @@
 		if _, ok := result[resource_uri]; ok {
 			// If the map contains "resource-uri", we can treat
 			// it as a MAAS object.
-			return jsonMAASObject{result, client}
+			return newjsonMAASObject(result, client)
 		}
 		return jsonMap(result)
 	case []interface{}:

=== modified file 'maas.go'
--- maas.go	2013-01-28 14:07:37 +0000
+++ maas.go	2013-01-29 09:52:25 +0000
@@ -6,5 +6,5 @@
 // NewMAAS returns an interface to the MAAS API as a MAASObject.
 func NewMAAS(client Client) (MAASObject, error) {
 	input := map[string]JSONObject{resource_uri: jsonString(client.BaseURL.String())}
-	return jsonMAASObject{jsonMap: jsonMap(input), client: client}, nil
+	return newjsonMAASObject(jsonMap(input), client), nil
 }

=== modified file 'maas_test.go'
--- maas_test.go	2013-01-28 14:07:37 +0000
+++ maas_test.go	2013-01-29 09:52:25 +0000
@@ -14,7 +14,6 @@
 	client := Client{BaseURL: baseURL}
 	maas, err := NewMAAS(client)
 	c.Check(err, IsNil)
-	URL, err := maas.URL()
-	c.Check(err, IsNil)
+	URL := maas.URL()
 	c.Check(URL, DeepEquals, baseURL)
 }

=== modified file 'maasobject.go'
--- maasobject.go	2013-01-29 08:11:40 +0000
+++ maasobject.go	2013-01-29 09:52:25 +0000
@@ -19,9 +19,9 @@
 	// Utility method to extract a string field from this MAAS object.
 	GetField(name string) (string, error)
 	// URL for this MAAS object.
-	URL() (*url.URL, error)
+	URL() *url.URL
 	// Resource URI for this MAAS object.
-	URI() (*url.URL, error)
+	URI() *url.URL
 	// Retrieve the MAAS object located at thisObject.URI()+name.
 	GetSubObject(name string) MAASObject
 	// Retrieve this MAAS object.
@@ -46,6 +46,24 @@
 type jsonMAASObject struct {
 	jsonMap
 	client Client
+	uri    *url.URL
+}
+
+// newjsonMAASObject creates a new MAAS object.  It will panic if the given map
+// does not contain a valid URL for the 'resource_uri' key.
+func newjsonMAASObject(jmap jsonMap, client Client) jsonMAASObject {
+	if uriObj, ok := jmap[resource_uri]; ok {
+		uriString, err := uriObj.GetString()
+		if err != nil {
+			panic("Cannot create jsonMAASObject object, the value of 'resource_uri' is not a string.")
+		}
+		uri, err := url.Parse(uriString)
+		if err != nil {
+			panic("Cannot create jsonMAASObject object, the value of 'resource_uri' is not a valid URL.")
+		}
+		return jsonMAASObject{jmap, client, uri}
+	}
+	panic("Cannot create jsonMAASObject object, no 'resource_uri' key present in the given jsonMap.")
 }
 
 var _ JSONObject = (*jsonMAASObject)(nil)
@@ -66,43 +84,30 @@
 	return obj.jsonMap[name].GetString()
 }
 
-func (obj jsonMAASObject) URI() (*url.URL, error) {
-	contents, err := obj.GetMap()
-	if err != nil {
-		panic("Unexpected failure converting jsonMAASObject to maasMap.")
-	}
-	urlString, err := contents[resource_uri].GetString()
-	if err != nil {
-		return &url.URL{}, err
-	}
-	return url.Parse(urlString)
+func (obj jsonMAASObject) URI() *url.URL {
+	// Duplicate the URL.
+	uri, err := url.Parse(obj.uri.String())
+	if err != nil {
+		panic(err)
+	}
+	return uri
 }
 
-func (obj jsonMAASObject) URL() (*url.URL, error) {
-	uri, err := obj.URI()
-	if err != nil {
-		return &url.URL{}, err
-	}
-	return obj.client.GetURL(uri), nil
+func (obj jsonMAASObject) URL() *url.URL {
+	return obj.client.GetURL(obj.URI())
 }
 
 func (obj jsonMAASObject) GetSubObject(name string) MAASObject {
-	uri, err := obj.URI()
-	if err != nil {
-		panic("Unexpected failure reading jsonMAASObject's URL.")
-	}
+	uri := obj.URI()
 	uri.Path = EnsureTrailingSlash(JoinURLs(uri.Path, name))
 	input := map[string]JSONObject{resource_uri: jsonString(uri.String())}
-	return jsonMAASObject{jsonMap: jsonMap(input), client: obj.client}
+	return newjsonMAASObject(jsonMap(input), obj.client)
 }
 
 var NotImplemented = errors.New("Not implemented")
 
 func (obj jsonMAASObject) Get() (MAASObject, error) {
-	uri, err := obj.URI()
-	if err != nil {
-		return nil, err
-	}
+	uri := obj.URI()
 	result, err := obj.client.Get(uri, "", url.Values{})
 	if err != nil {
 		return nil, err
@@ -115,10 +120,7 @@
 }
 
 func (obj jsonMAASObject) Post(params url.Values) (JSONObject, error) {
-	uri, err := obj.URI()
-	if err != nil {
-		return nil, err
-	}
+	uri := obj.URI()
 	result, err := obj.client.Post(uri, "", params)
 	if err != nil {
 		return nil, err
@@ -127,10 +129,7 @@
 }
 
 func (obj jsonMAASObject) Update(params url.Values) (MAASObject, error) {
-	uri, err := obj.URI()
-	if err != nil {
-		return nil, err
-	}
+	uri := obj.URI()
 	result, err := obj.client.Put(uri, params)
 	if err != nil {
 		return nil, err
@@ -143,18 +142,12 @@
 }
 
 func (obj jsonMAASObject) Delete() error {
-	uri, err := obj.URI()
-	if err != nil {
-		return err
-	}
+	uri := obj.URI()
 	return obj.client.Delete(uri)
 }
 
 func (obj jsonMAASObject) CallGet(operation string, params url.Values) (JSONObject, error) {
-	uri, err := obj.URI()
-	if err != nil {
-		return nil, err
-	}
+	uri := obj.URI()
 	result, err := obj.client.Get(uri, operation, params)
 	if err != nil {
 		return nil, err
@@ -163,10 +156,7 @@
 }
 
 func (obj jsonMAASObject) CallPost(operation string, params url.Values) (JSONObject, error) {
-	uri, err := obj.URI()
-	if err != nil {
-		return nil, err
-	}
+	uri := obj.URI()
 	result, err := obj.client.Post(uri, operation, params)
 	if err != nil {
 		return nil, err

=== modified file 'maasobject_test.go'
--- maasobject_test.go	2013-01-28 13:17:41 +0000
+++ maasobject_test.go	2013-01-29 09:52:25 +0000
@@ -45,30 +45,67 @@
 	c.Check(err, NotNil)
 }
 
+func (suite *GomaasapiTestSuite) TestnewjsonMAASObjectPanicsIfNoResourceURI(c *C) {
+	defer func() {
+		recoveredError := recover()
+		c.Check(recoveredError, NotNil)
+		c.Check(recoveredError, Matches, ".*no 'resource_uri' key.*")
+	}()
+	input := map[string]JSONObject{"test": jsonString("test")}
+	newjsonMAASObject(jsonMap(input), Client{})
+}
+
+func (suite *GomaasapiTestSuite) TestnewjsonMAASObjectPanicsIfResourceURINotString(c *C) {
+	defer func() {
+		recoveredError := recover()
+		c.Check(recoveredError, NotNil)
+		c.Check(recoveredError, Matches, ".*the value of 'resource_uri' is not a string.*")
+	}()
+	input := map[string]JSONObject{resource_uri: jsonFloat64(77.7)}
+	newjsonMAASObject(jsonMap(input), Client{})
+}
+
+func (suite *GomaasapiTestSuite) TestnewjsonMAASObjectPanicsIfResourceURINotURL(c *C) {
+	defer func() {
+		recoveredError := recover()
+		c.Check(recoveredError, NotNil)
+		c.Check(recoveredError, Matches, ".*the value of 'resource_uri' is not a valid URL.*")
+	}()
+	input := map[string]JSONObject{resource_uri: jsonString("")}
+	newjsonMAASObject(jsonMap(input), Client{})
+}
+
+func (suite *GomaasapiTestSuite) TestnewjsonMAASObjectSetsUpURI(c *C) {
+	URI, _ := url.Parse("http://example.com/a/resource";)
+	input := map[string]JSONObject{resource_uri: jsonString(URI.String())}
+	obj := newjsonMAASObject(jsonMap(input), Client{})
+	c.Check(obj.uri, DeepEquals, URI)
+}
+
 func (suite *GomaasapiTestSuite) TestURL(c *C) {
 	baseURL, _ := url.Parse("http://example.com/";)
 	uri := "http://example.com/a/resource";
 	resourceURL, _ := url.Parse(uri)
 	input := map[string]JSONObject{resource_uri: jsonString(uri)}
 	client := Client{BaseURL: baseURL}
-	obj := jsonMAASObject{jsonMap: jsonMap(input), client: client}
-
-	URL, err := obj.URL()
-
-	c.Check(err, IsNil)
+	obj := newjsonMAASObject(jsonMap(input), client)
+
+	URL := obj.URL()
+
 	c.Check(URL, DeepEquals, resourceURL)
 }
 
 func (suite *GomaasapiTestSuite) TestGetSubObject(c *C) {
+	baseURL, _ := url.Parse("http://example.com/";)
 	uri := "http://example.com/a/resource/";
 	input := map[string]JSONObject{resource_uri: jsonString(uri)}
-	obj := jsonMAASObject{jsonMap: jsonMap(input)}
+	client := Client{BaseURL: baseURL}
+	obj := newjsonMAASObject(jsonMap(input), client)
 	subName := "/test"
 
 	subObj := obj.GetSubObject(subName)
-	subURL, err := subObj.URL()
+	subURL := subObj.URL()
 
-	c.Check(err, IsNil)
 	// 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/";)