launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/gomaasapi/check-for-errors into lp:gomaasapi.

Commit message:
Fix some neglected error handling in the test service.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/gomaasapi/check-for-errors/+merge/150771

It's possible that some of these checks are actually not needed.  But I don't want anybody to have to waste the time to figure that out whenever they came across one of these in debugging.  Or forget to add checks when future changes do make them necessary.

Errors really need to be checked, or debugging becomes a nightmare of misleading failures.  And we can't assume without a clear promise that we always know exactly what errors can or can't happen.  To do so is reckless arrogance.  Even when we do have a clear promise we should not rely on it routinely, only in rare and well-documented cases where there is no other choice.


Jeroen
-- 
https://code.launchpad.net/~jtv/gomaasapi/check-for-errors/+merge/150771
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/gomaasapi/check-for-errors into lp:gomaasapi.

=== modified file 'testservice.go'
--- testservice.go	2013-02-27 09:21:07 +0000
+++ testservice.go	2013-02-27 12:08:20 +0000
@@ -24,12 +24,20 @@
 	TestServer *TestServer
 }
 
+// checkError is a shorthand helper that panics if err is not nil.
+func checkError(err error) {
+	if err != nil {
+		panic(err)
+	}
+}
+
 // NewTestMAAS returns a TestMAASObject that implements the MAASObject
 // interface and thus can be used as a test object instead of the one returned
 // by gomaasapi.NewMAAS().
 func NewTestMAAS(version string) *TestMAASObject {
 	server := NewTestServer(version)
-	authClient, _ := NewAnonymousClient(server.URL + fmt.Sprintf("/api/%s/", version))
+	authClient, err := NewAnonymousClient(server.URL + fmt.Sprintf("/api/%s/", version))
+	checkError(err)
 	maas := NewMAAS(*authClient)
 	return &TestMAASObject{*maas, server}
 }
@@ -92,9 +100,7 @@
 func (server *TestServer) NewNode(jsonText string) MAASObject {
 	var attrs map[string]interface{}
 	err := json.Unmarshal([]byte(jsonText), &attrs)
-	if err != nil {
-		panic(err)
-	}
+	checkError(err)
 	systemIdEntry, hasSystemId := attrs["system_id"]
 	if !hasSystemId {
 		panic("The given map json string does not contain a 'system_id' value.")
@@ -172,7 +178,8 @@
 	})
 
 	newServer := httptest.NewServer(serveMux)
-	client, _ := NewAnonymousClient(newServer.URL)
+	client, err := NewAnonymousClient(newServer.URL)
+	checkError(err)
 	server.Server = newServer
 	server.serveMux = serveMux
 	server.client = *client
@@ -182,7 +189,8 @@
 
 // nodesHandler handles requests for '/api/<version>/nodes/*'.
 func nodesHandler(server *TestServer, w http.ResponseWriter, r *http.Request) {
-	values, _ := url.ParseQuery(r.URL.RawQuery)
+	values, err := url.ParseQuery(r.URL.RawQuery)
+	checkError(err)
 	op := values.Get("op")
 	nodeURLRE := getNodeURLRE(server.version)
 	nodeURLMatch := nodeURLRE.FindStringSubmatch(r.URL.Path)
@@ -250,7 +258,8 @@
 
 // nodeListingHandler handles requests for '/nodes/'.
 func nodeListingHandler(server *TestServer, w http.ResponseWriter, r *http.Request) {
-	values, _ := url.ParseQuery(r.URL.RawQuery)
+	values, err := url.ParseQuery(r.URL.RawQuery)
+	checkError(err)
 	ids, hasId := values["id"]
 	var convertedNodes = []map[string]JSONObject{}
 	for systemId, node := range server.nodes {
@@ -258,14 +267,16 @@
 			convertedNodes = append(convertedNodes, node.GetMap())
 		}
 	}
-	res, _ := json.Marshal(convertedNodes)
+	res, err := json.Marshal(convertedNodes)
+	checkError(err)
 	w.WriteHeader(http.StatusOK)
 	fmt.Fprint(w, string(res))
 }
 
 // filesHandler handles requests for '/api/<version>/files/*'.
 func filesHandler(server *TestServer, w http.ResponseWriter, r *http.Request) {
-	values, _ := url.ParseQuery(r.URL.RawQuery)
+	values, err := url.ParseQuery(r.URL.RawQuery)
+	checkError(err)
 	op := values.Get("op")
 	fileURLRE := getFileURLRE(server.version)
 	fileURLMatch := fileURLRE.FindStringSubmatch(r.URL.Path)
@@ -315,7 +326,8 @@
 
 // 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)
+	values, err := url.ParseQuery(r.URL.RawQuery)
+	checkError(err)
 	prefix := values.Get("prefix")
 	filenames := listFilenames(server, prefix)
 
@@ -327,9 +339,7 @@
 		convertedFiles = append(convertedFiles, fileMap)
 	}
 	res, err := json.Marshal(convertedFiles)
-	if err != nil {
-		panic(err)
-	}
+	checkError(err)
 	w.WriteHeader(http.StatusOK)
 	fmt.Fprint(w, string(res))
 }
@@ -355,7 +365,8 @@
 // getFileHandler handles requests for
 // '/api/<version>/files/?op=get&filename=filename'.
 func getFileHandler(server *TestServer, w http.ResponseWriter, r *http.Request) {
-	values, _ := url.ParseQuery(r.URL.RawQuery)
+	values, err := url.ParseQuery(r.URL.RawQuery)
+	checkError(err)
 	filename := values.Get("filename")
 	file, found := server.files[filename]
 	if !found {
@@ -388,11 +399,10 @@
 // filesHandler handles requests for '/api/<version>/files/?op=add&filename=filename'.
 func addFileHandler(server *TestServer, w http.ResponseWriter, r *http.Request) {
 	err := r.ParseMultipartForm(10000000)
-	if err != nil {
-		panic(err)
-	}
+	checkError(err)
 
-	values, _ := url.ParseQuery(r.URL.RawQuery)
+	values, err := url.ParseQuery(r.URL.RawQuery)
+	checkError(err)
 	filename := values.Get("filename")
 
 	uploads := r.MultipartForm.File
@@ -404,9 +414,7 @@
 		upload = uploadContent[0]
 	}
 	content, err := readMultipart(upload)
-	if err != nil {
-		panic(err)
-	}
+	checkError(err)
 	server.NewFile(filename, content)
 	w.WriteHeader(http.StatusOK)
 }