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)
}
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
