launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/gomaasapi/getsubobject-takes-name-not-url into lp:gomaasapi.

Commit message:
Really make MAASObject.GetSubObject treat its argument as a path, not a URL.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/gomaasapi/getsubobject-takes-name-not-url/+merge/151476

This is something we never really bothered with because it didn't matter much.  But with the juju file storage API, where we may have user-supplied file paths in URLs, it does.  If we pass GetSubObject a string with a "?" in it, that should _not_ mean that the following text is a query.  It's just a weird character in a path.

I looked into changing the function's documentation, but actually it's already right.  It's just a choice we never fully committed to because it wasn't entirely clear that we would really need it to be this way.


Jeroen
-- 
https://code.launchpad.net/~jtv/gomaasapi/getsubobject-takes-name-not-url/+merge/151476
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/gomaasapi/getsubobject-takes-name-not-url into lp:gomaasapi.

=== modified file 'maasobject.go'
--- maasobject.go	2013-02-12 13:44:38 +0000
+++ maasobject.go	2013-03-04 12:19:23 +0000
@@ -114,11 +114,8 @@
 // at a given sub-path of the current object's resource URI.
 func (obj MAASObject) GetSubObject(name string) MAASObject {
 	uri := obj.URI()
-	newUrl, err := url.Parse(name)
-	if err != nil {
-		panic(err)
-	}
-	resUrl := uri.ResolveReference(newUrl)
+	newURL := url.URL{Path: name}
+	resUrl := uri.ResolveReference(&newURL)
 	resUrl.Path = EnsureTrailingSlash(resUrl.Path)
 	input := map[string]interface{}{resourceURI: resUrl.String()}
 	return newJSONMAASObject(input, obj.client)

=== modified file 'maasobject_test.go'
--- maasobject_test.go	2013-02-18 04:25:25 +0000
+++ maasobject_test.go	2013-03-04 12:19:23 +0000
@@ -163,6 +163,17 @@
 	c.Check(subURL, DeepEquals, expectedSubURL)
 }
 
+// The argument to GetSubObject is a relative path, not a URL.  So it won't
+// take a query part.  The special characters that mark a query are escaped
+// so they are recognized as parts of the path.
+func (suite *MAASObjectSuite) TestGetSubObjectTakesPathNotURL(c *C) {
+	obj := makeFakeMAASObject("http://example.com/";, "x/")
+
+	subObj := obj.GetSubObject("/y?z")
+
+	c.Check(subObj.URL().String(), Equals, "http://example.com/y%3Fz/";)
+}
+
 func (suite *MAASObjectSuite) TestGetField(c *C) {
 	uri := "http://example.com/a/resource";
 	fieldName := "field name"