Code review comment for lp:~rvb/gwacl/handle-307

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 review: approve

Thanks for this, nice change. I must admit, I'm surprised that curl
doesn't do this by default, wget does.

Some minor points below.

On 01/07/13 23:08, Raphaël Badin wrote:
> === modified file 'x509dispatcher.go' --- x509dispatcher.go
> 2013-06-27 10:55:35 +0000 +++ x509dispatcher.go 2013-07-01 13:05:38
> +0000 @@ -178,6 +178,9 @@ return response, nil }
>
> +// The maximum number of redirection-followings allowed. +var
> _CURL_MAX_REDIRECTS = 10

Why did you choose 10? Document that here in the comment.

> + // makeCurlRequest produces a curl.CURL representing the
> request. // Clean up the request by calling its Cleanup() method
> after you're done // with it. @@ -188,6 +191,8 @@
> ch.Setopt(curl.OPT_KEYPASSWD, "") ch.Setopt(curl.OPT_URL,
> request.URL) //ch.Setopt(curl.OPT_VERBOSE, true) +
> ch.Setopt(curl.OPT_FOLLOWLOCATION, true) +
> ch.Setopt(curl.OPT_MAXREDIRS, _CURL_MAX_REDIRECTS)
> ch.Setopt(curl.OPT_SSLVERSION, 3) versionHeader := "x-ms-version: "
> + request.APIVersion headers := []string{versionHeader}
>
> === modified file 'x509dispatcher_test.go' ---
> x509dispatcher_test.go 2013-03-26 10:23:51 +0000 +++
> x509dispatcher_test.go 2013-07-01 13:05:38 +0000 @@ -14,9 +14,12
> @@ // that serves at the given base URL, copies incoming requests
> into the given // channel, and finally returns the given status
> code. If body is not nil, it // will be returned as the request
> body. -func makeRecordingHTTPServer(requests chan *http.Request,
> status int, body []byte) *httptest.Server { +func
> makeRecordingHTTPServer(requests chan *http.Request, status int,
> body []byte, headers map[string]string

Consider using http.Header instead of map[string]string here (like
performRequest() in storage_base.go does), it is the same thing
underneath but conveys intent better.

) *httptest.Server {
> returnRequest := func(w http.ResponseWriter, r *http.Request) {
> requests <- r + for key, value := range headers { +
> w.Header().Set(key, value) + } w.WriteHeader(status) if body
> != nil { w.Write(body) @@ -55,7 +58,7 @@
>
> func (*x509DispatcherSuite) TestGetRequestDoesHTTPGET(c *C) {
> httpRequests := make(chan *http.Request, 1) - server :=
> makeRecordingHTTPServer(httpRequests, http.StatusOK, nil) +
> server := makeRecordingHTTPServer(httpRequests, http.StatusOK, nil,
> nil) defer server.Close() // No real certificate needed since we're
> testing on http, not https. session, err :=
> newX509Session("subscriptionid", "cert.pem") @@ -72,12 +75,49 @@
> c.Check(httpRequest.URL.String(), Equals, path) }
>
> +func (*x509DispatcherSuite) TestGetFollowsRedirects(c *C) { +
> redirectPath := "/redirect/path" + httpRequests := make(chan
> *http.Request, _CURL_MAX_REDIRECTS+1) + locationHeaders :=
> map[string]string{"Location": redirectPath} + server :=
> makeRecordingHTTPServer(httpRequests, http.StatusTemporaryRedirect,
> nil, locationHeaders) + defer server.Close() + // No real
> certificate needed since we're testing on http, not https. +
> session, err := newX509Session("subscriptionid", "cert.pem") +
> c.Assert(err, IsNil) + path := "/foo/bar" + request :=
> newX509RequestGET(server.URL + path) + + _, err =
> performX509CurlRequest(session, request) + c.Check(err,
> ErrorMatches, ".*Number of redirects hit maximum amount.*")

Consider making this a separate test, it muddies the focus of the rest
of the test below. Short, focused are much easier to read and less flaky.

> + + var httpRequest *http.Request + // The original GET
> request has been performed. + select { + case httpRequest =
> <-httpRequests: + default: + c.Error("The original
> request has not been performed.") + } +
> c.Check(httpRequest.Method, Equals, "GET") +
> c.Check(httpRequest.URL.String(), Equals, path) + + //
> _CURL_MAX_REDIRECTS redirected requests have been performed. +
> for i := 0; i < _CURL_MAX_REDIRECTS; i++ { + select { +
> case httpRequest := <-httpRequests: +
> c.Check(httpRequest.Method, Equals, "GET") +
> c.Check(httpRequest.URL.String(), Equals, redirectPath) +
> default: + c.Error("No redirection has happened.") +
> } + } +} + func (*x509DispatcherSuite)
> TestPostRequestDoesHTTPPOST(c *C) { httpRequests := make(chan
> *http.Request, 1) requestBody := []byte{1, 2, 3} responseBody :=
> []byte{4, 5, 6} requestContentType := "bogusContentType" -
> server := makeRecordingHTTPServer(httpRequests, http.StatusOK,
> responseBody) + server := makeRecordingHTTPServer(httpRequests,
> http.StatusOK, responseBody, nil) defer server.Close() // No real
> certificate needed since we're testing on http, not https. session,
> err := newX509Session("subscriptionid", "cert.pem") @@ -99,7 +139,7
> @@
>
> func (*x509DispatcherSuite) TestDeleteRequestDoesHTTPDELETE(c *C)
> { httpRequests := make(chan *http.Request, 1) - server :=
> makeRecordingHTTPServer(httpRequests, http.StatusOK, []byte{}) +
> server := makeRecordingHTTPServer(httpRequests, http.StatusOK, nil,
> nil) defer server.Close() // No real certificate needed since we're
> testing on http, not https. session, err :=
> newX509Session("subscriptionid", "cert.pem") @@ -120,7 +160,7 @@
> httpRequests := make(chan *http.Request, 1) requestBody :=
> []byte{1, 2, 3} responseBody := []byte{4, 5, 6} - server :=
> makeRecordingHTTPServer(httpRequests, http.StatusOK, responseBody)
> + server := makeRecordingHTTPServer(httpRequests, http.StatusOK,
> responseBody, nil) defer server.Close() // No real certificate
> needed since we're testing on http, not https. session, err :=
> newX509Session("subscriptionid", "cert.pem")

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlHSIwkACgkQWhGlTF8G/HfKYACfcOPXde6Q2CRYKFnpJW7xj4xc
KwIAoI/9MJAvQ7jF5BDyY0sVMqymI/9K
=xozN
-----END PGP SIGNATURE-----