Merge lp:~jtv/gomaasapi/test-upload-file into lp:gomaasapi

[Jeroen T. Vermeulen]

Test the test service's handling of file uploads, and fix handling of the "filename" parameter.

This was a silly one, but it took me a few days to figure out because I was hitting the problem in another project, when really the gomaasapi test suite should have caught it first.

The handler in the test service that receives a file upload was not receiving the required "filename" parameter. It always used the empty string instead. We did not notice this because originally the handler did not check for the case that the parameter might be missing (and obviously the test suite did not cover this case), and file uploads were not covered by the test suite.

Once I had a test, it was easy to see what went wrong: instead of looking in the http request's Form field (which holds both POST-style and GET-style parameters), the code re-parsed the query part of the raw, unparsed version of the request's URL and looked for a GET-style parameter there. The requesting code actually submits it as a POST parameter. It might have been nice for debugging to pass this particular parameter as part of the URL, but we don't currently have a way to tell the client which parameters to pass as GET and which to pass as POST (and we probably don't want all parameters as GET). There is a test for parameter-passing, but the parameter-passing code that ran here was hidden from the tests by what I call the "Swiss Army Knife" antipattern.

There are a few lessons we can learn from this:

 * Go does not have exceptions. Check your error conditions, or debugging turns into a nightmare.

 * Code changes. You may know that something doesn't fail now, but you still need to check in case somebody breaks it in the future.

 * Distrust third-party input. Check for obvious mistakes.

 * Avoid unnecessary cyclomatic complexity. It hides problems from both tests and humans.

 * Particularly avoid "if" clauses that are there just to reconstruct the intent of the calling code, if the intent was already known and constant at the call site. This is the Swiss Army Knife function: it's both a weak knife and a wobbly screwdriver. Better to accept a bit more code at the call site, and look for other ways to avoid repetition.

 * Unit-testing and integration-testing are both necessary. Integration-testing should cover a basic happy path and error handling, unit-testing can go through the nooks and crannies. But that doesn't work well with the Swiss Army Knife, because it needs to integrate multiple code paths at the call level with multiple code paths at the callee level.

 * If it's not tested, it's broken. Wherever possible, test before implementing so that you can see the test go from failure to success.

 * Fixing bugs as you come across them is disruptive and makes for unpredictable costs. Slower development earlier on could have saved us time in the current stage.

Jeroen

[Gavin Panella (allenap)]

Looks good.

[1]

+ filename := r.Form.Get("filename")
  if filename == "" {
   panic("upload has no filename")

Can filename ever be nil? If so, better check for that condition too.

[2]

> Once I had a test, it was easy to see what went wrong: instead of
> looking in the http request's Form field (which holds both
> POST-style and GET-style parameters), the code re-parsed the query

It isn't about the code, but I think it's worthwhile deprecated these
descriptions of parameters.

GET-style parameters are just query string parameters, and can used
with any HTTP method.

POST-style parameters are typically those that are encoded into the
request body in the same way as query string parameters are encoded,
but can be multipart/form-data, in which case the values of these
parameters are actually structured (e.g. they have a content-type).

Many web frameworks call them GET and POST parameters, erroneously I
feel.

Much like Django, hence why it's hard to take seriously. However,
Django then steps up its game another notch, and steals the whole
packet of biscuits by copying PHP without blushing:

> HttpRequest.REQUEST
>
> For convenience, a dictionary-like object that searches POST first,
> then GET. Inspired by PHP’s $_REQUEST.

From https://docs.djangoproject.com/en/dev/ref/request-response/

(Sorry, it's Django and I couldn't resist the chance to twist the
knife.)

I know that you know all this... If you've read this far, what I'm
trying to say is that *we* should stop referring to them as GET and
POST parameters.

review: Approve

[Jeroen T. Vermeulen (jtv)]

> [1]
>
> + filename := r.Form.Get("filename")
> if filename == "" {
> panic("upload has no filename")
>
> Can filename ever be nil? If so, better check for that condition too.

No, but I like your thinking! If Python is a bit like SQL in that any type of variable can be None, Go is more like Oracle (or Django) in that strings can't be.

> [2]
>
> > Once I had a test, it was easy to see what went wrong: instead of
> > looking in the http request's Form field (which holds both
> > POST-style and GET-style parameters), the code re-parsed the query
>
> It isn't about the code, but I think it's worthwhile deprecated these
> descriptions of parameters.

Thanks for explaining this. I've long wondered. This is also why I say "GET-style parameters" but not "GET parameters." Nor do I call them "PUT parameters" just because I happen to be using a different http method. :-)

> I know that you know all this... If you've read this far, what I'm
> trying to say is that *we* should stop referring to them as GET and
> POST parameters.

I'm all for it! What would be the right terms?

Jeroen

[Gavin Panella (allenap)]

>> I know that you know all this... If you've read this far, what I'm
>> trying to say is that *we* should stop referring to them as GET and
>> POST parameters.
>
> I'm all for it! What would be the right terms?

Ah, good question :)

"Query string parameters" is the least ambiguous thing I can think of
for GET(-style) parameters. The term POST-style isn't very misleading,
though it does hide the different body types that are conventionally
accepted as parameters, and also makes us blind to using other content
types (which is especially pertinent when trying to do ReST). "Request
body parameters" is more accurate I think. Though, again, it is more
of a mouthful.

=== modified file 'testservice.go'
--- testservice.go	2013-03-04 02:50:16 +0000
+++ testservice.go	2013-03-04 09:02:22 +0000
@@ -420,9 +420,7 @@
 	err := r.ParseMultipartForm(10000000)
 	checkError(err)
 
-	values, err := url.ParseQuery(r.URL.RawQuery)
-	checkError(err)
-	filename := values.Get("filename")
+	filename := r.Form.Get("filename")
 	if filename == "" {
 		panic("upload has no filename")
 	}

=== modified file 'testservice_test.go'
--- testservice_test.go	2013-03-04 02:50:16 +0000
+++ testservice_test.go	2013-03-04 09:02:22 +0000
@@ -420,3 +420,22 @@
 	operations := nodeOperations["mysystemid"]
 	c.Check(operations, DeepEquals, []string{"start"})
 }
+
+func (suite *TestMAASObjectSuite) TestUploadFile(c *C) {
+	const filename = "myfile.txt"
+	const fileContent = "uploaded contents"
+	files := suite.TestMAASObject.GetSubObject("files")
+	params := url.Values{"filename": {filename}}
+	filesMap := map[string][]byte{"file": []byte(fileContent)}
+
+	// Upload a file.
+	_, err := files.CallPostFiles("add", params, filesMap)
+	c.Assert(err, IsNil)
+
+	// The file can now be downloaded.
+	downloadedFile, err := files.CallGet("get", params)
+	c.Assert(err, IsNil)
+	bytes, err := downloadedFile.GetBytes()
+	c.Assert(err, IsNil)
+	c.Check(string(bytes), Equals, fileContent)
+}