launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/gomaasapi/keep-file-content into lp:gomaasapi.

Commit message:
Fix small bug in test service, where the "list" operation accidentally deleted files' contents.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/gomaasapi/keep-file-content/+merge/150747

Map objects have pass-by-reference semantics internally.  So where the test service delete()d the "content" entry from a file's attributes, it did so on the original file.  And so this branch creates a copy first.

I also extracted some code because Go's "sweet spot" for function length seems to be very low, at least in terms of functionality provided.  In line count it's probably not particularly high.


Jeroen
-- 
https://code.launchpad.net/~jtv/gomaasapi/keep-file-content/+merge/150747
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/gomaasapi/keep-file-content into lp:gomaasapi.

=== modified file 'testservice.go'
--- testservice.go	2013-02-20 08:35:19 +0000
+++ testservice.go	2013-02-27 09:33:28 +0000
@@ -288,28 +288,48 @@
 
 }
 
+// listFilenames returns the names of those uploaded files whose names start
+// with the given prefix, sorted lexicographically.
+func listFilenames(server *TestServer, prefix string) []string {
+	var filenames = make([]string, 0)
+	for filename, _ := range server.files {
+		if strings.HasPrefix(filename, prefix) {
+			filenames = append(filenames, filename)
+		}
+	}
+	sort.Strings(filenames)
+	return filenames
+}
+
+// stripFileContent copies a map of attributes representing an uploaded file,
+// but with the "content" attribute removed.
+func stripContent(original map[string]JSONObject) map[string]JSONObject {
+	newMap := make(map[string]JSONObject, len(original)-1)
+	for key, value := range original {
+		if key != "content" {
+			newMap[key] = value
+		}
+	}
+	return newMap
+}
+
 // fileListingHandler handles requests for '/api/<version>/files/?op=list'.
 func fileListingHandler(server *TestServer, w http.ResponseWriter, r *http.Request) {
 	values, _ := url.ParseQuery(r.URL.RawQuery)
 	prefix := values.Get("prefix")
-	var convertedFiles = []map[string]JSONObject{}
-	// Create slice of selected filenames.
-	var filenames = []string{}
-	for filename, _ := range server.files {
-		if strings.Index(filename, prefix) == 0 {
-			filenames = append(filenames, filename)
-		}
-	}
-	// Sort filenames.
-	sort.Strings(filenames)
+	filenames := listFilenames(server, prefix)
+
 	// Build a sorted list of the files as map[string]JSONObject objects. 
+	convertedFiles := make([]map[string]JSONObject, 0)
 	for _, filename := range filenames {
-		file, _ := server.files[filename]
-		fileMap := file.GetMap()
-		delete(fileMap, "content")
+		// The "content" attribute is not in the listing.
+		fileMap := stripContent(server.files[filename].GetMap())
 		convertedFiles = append(convertedFiles, fileMap)
 	}
-	res, _ := json.Marshal(convertedFiles)
+	res, err := json.Marshal(convertedFiles)
+	if err != nil {
+		panic(err)
+	}
 	w.WriteHeader(http.StatusOK)
 	fmt.Fprint(w, string(res))
 }

=== modified file 'testservice_test.go'
--- testservice_test.go	2013-02-14 16:40:47 +0000
+++ testservice_test.go	2013-02-27 09:33:28 +0000
@@ -207,7 +207,7 @@
 	c.Check(content, DeepEquals, fileContent)
 }
 
-func (suite *TestServerSuite) TestHandlesListReturnedSortedFilenames(c *C) {
+func (suite *TestServerSuite) TestHandlesListReturnsSortedFilenames(c *C) {
 	fileName1 := "filename1"
 	suite.server.NewFile(fileName1, []byte("test file content"))
 	fileName2 := "filename2"
@@ -218,15 +218,16 @@
 	c.Check(err, IsNil)
 	c.Check(resp.StatusCode, Equals, http.StatusOK)
 	content, err := ioutil.ReadAll(resp.Body)
-	c.Check(err, IsNil)
+	c.Assert(err, IsNil)
 	var files []map[string]string
 	err = json.Unmarshal(content, &files)
+	c.Assert(err, IsNil)
 	c.Check(len(files), Equals, 2)
 	c.Check(files[0]["filename"], Equals, fileName1)
 	c.Check(files[1]["filename"], Equals, fileName2)
 }
 
-func (suite *TestServerSuite) TestHandlesListReturnedFilteredFiles(c *C) {
+func (suite *TestServerSuite) TestHandlesListFiltersFiles(c *C) {
 	fileName1 := "filename1"
 	suite.server.NewFile(fileName1, []byte("test file content"))
 	fileName2 := "prefixFilename"
@@ -238,13 +239,41 @@
 	c.Check(err, IsNil)
 	c.Check(resp.StatusCode, Equals, http.StatusOK)
 	content, err := ioutil.ReadAll(resp.Body)
-	c.Check(err, IsNil)
+	c.Assert(err, IsNil)
 	var files []map[string]string
 	err = json.Unmarshal(content, &files)
+	c.Assert(err, IsNil)
 	c.Check(len(files), Equals, 1)
 	c.Check(files[0]["filename"], Equals, fileName2)
 }
 
+func (suite *TestServerSuite) TestHandlesListOmitsContent(c *C) {
+	const filename = "myfile"
+	fileContent := []byte("test file content")
+	suite.server.NewFile(filename, fileContent)
+	getURI := fmt.Sprintf("/api/%s/files/?op=list", suite.server.version)
+
+	resp, err := http.Get(suite.server.Server.URL + getURI)
+	c.Assert(err, IsNil)
+
+	content, err := ioutil.ReadAll(resp.Body)
+	c.Assert(err, IsNil)
+	var files []map[string]string
+	err = json.Unmarshal(content, &files)
+
+	// The resulting dict does not have a "content" entry.
+	file := files[0]
+	_, ok := file["content"]
+	c.Check(ok, Equals, false)
+
+	// But the original as stored in the test service still has it.
+	contentAfter, err := suite.server.files[filename].GetField("content")
+	c.Assert(err, IsNil)
+	bytes, err := base64.StdEncoding.DecodeString(contentAfter)
+	c.Assert(err, IsNil)
+	c.Check(string(bytes), Equals, string(fileContent))
+}
+
 func (suite *TestServerSuite) TestDeleteFile(c *C) {
 	fileName1 := "filename1"
 	suite.server.NewFile(fileName1, []byte("test file content"))