Merge lp:~bigkevmcd/goose/fix-for-swift-headers into lp:goose

[Kevin McDermott]

This is a fix for this, basically, this code...

http/client.go

 func (c *Client) sendRequest(method, URL string, reqReader io.Reader, length int, headers http.Header, expectedStatus []int, logger *log.Logger) (*http.Response, error) {

originally, only returned the body of the response, which meant there was no scope for getting the headers.

Coupled with this code from swift/swift.go

-func (c *Client) HeadObject(containerName, objectName string) (headers http.Header, err error)

the fact that it's creating headers here, masks the fact that they're not originating in the request.

I implemented HEAD in the fake server, mainly because it's the quickest way to test the fix, we don't necessarily need HEAD, but we wanted to proxy the headers returned by the Swift request to downstream clients (specifically ETag / cache-control headers).

This is a fix for this, basically, this code...

http/client.go

 func (c *Client) sendRequest(method, URL string, reqReader io.Reader, length int, headers http.Header, expectedStatus []int, logger *log.Logger) (*http.Response, error) {

originally, only returned the body of the response, which meant there was no scope for getting the headers.

Coupled with this code from swift/swift.go

-func (c *Client) HeadObject(containerName, objectName string) (headers http.Header, err error)

the fact that it's creating headers here, masks the fact that they're not originating in the request.

I implemented HEAD in the fake server, mainly because it's the quickest way to test the fix, we don't necessarily need HEAD, but we wanted to proxy the headers returned by the Swift request to downstream clients (specifically ETag / cache-control headers).

[John A Meinel (jameinel)]

LGTM. Thanks for doing this. This does end up changing the public API, but I can understand that you need some way to get the headers , and we don't currently expose it.

We could return the raw http.Response object, but ATM I think this provides the best balance of preserving some flexibility in the response and giving enough information for users.

review: Approve

=== modified file 'http/client.go'
--- http/client.go	2014-04-23 06:57:55 +0000
+++ http/client.go	2014-05-02 13:23:44 +0000
@@ -64,6 +64,7 @@
 	ReqReader      io.Reader
 	ReqLength      int
 	RespReader     io.ReadCloser
+	RespHeaders    http.Header
 }
 
 const (
@@ -136,13 +137,14 @@
 		}
 	}
 	headers := createHeaders(reqData.ReqHeaders, contentTypeJSON, token)
-	respBody, err := c.sendRequest(
+	resp, err := c.sendRequest(
 		method, url, bytes.NewReader(body), len(body), headers, reqData.ExpectedStatus, logger)
 	if err != nil {
 		return
 	}
-	defer respBody.Close()
-	respData, err := ioutil.ReadAll(respBody)
+	reqData.RespHeaders = resp.Header
+	defer resp.Body.Close()
+	respData, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
 		err = errors.Newf(err, "failed reading the response body")
 		return
@@ -173,15 +175,16 @@
 		url += "?" + reqData.Params.Encode()
 	}
 	headers := createHeaders(reqData.ReqHeaders, contentTypeOctetStream, token)
-	respBody, err := c.sendRequest(
+	resp, err := c.sendRequest(
 		method, url, reqData.ReqReader, reqData.ReqLength, headers, reqData.ExpectedStatus, logger)
 	if err != nil {
 		return
 	}
+	reqData.RespHeaders = resp.Header
 	if reqData.RespReader != nil {
-		reqData.RespReader = respBody
+		reqData.RespReader = resp.Body
 	} else {
-		respBody.Close()
+		resp.Body.Close()
 	}
 	return
 }
@@ -192,18 +195,18 @@
 // headers: HTTP headers to include with the request.
 // expectedStatus: a slice of allowed response status codes.
 func (c *Client) sendRequest(method, URL string, reqReader io.Reader, length int, headers http.Header,
-	expectedStatus []int, logger *log.Logger) (rc io.ReadCloser, err error) {
+	expectedStatus []int, logger *log.Logger) (*http.Response, error) {
 	reqData := make([]byte, length)
 	if reqReader != nil {
 		nrRead, err := io.ReadFull(reqReader, reqData)
 		if err != nil {
 			err = errors.Newf(err, "failed reading the request data, read %v of %v bytes", nrRead, length)
-			return rc, err
+			return nil, err
 		}
 	}
 	rawResp, err := c.sendRateLimitedRequest(method, URL, headers, reqData, logger)
 	if err != nil {
-		return
+		return nil, err
 	}
 	foundStatus := false
 	if len(expectedStatus) == 0 {
@@ -218,9 +221,9 @@
 	if !foundStatus && len(expectedStatus) > 0 {
 		err = handleError(URL, rawResp)
 		rawResp.Body.Close()
-		return
+		return nil, err
 	}
-	return rawResp.Body, err
+	return rawResp, err
 }
 
 func (c *Client) sendRateLimitedRequest(method, URL string, headers http.Header, reqData []byte,

=== modified file 'http/client_test.go'
--- http/client_test.go	2013-10-17 23:02:16 +0000
+++ http/client_test.go	2014-05-02 13:23:44 +0000
@@ -29,6 +29,7 @@
 		req.Body.Close()
 		bodyChan <- string(bodyBytes)
 		resp.Header().Add("Content-Length", "0")
+		resp.Header().Add("Testing", "true")
 		resp.WriteHeader(http.StatusNoContent)
 		resp.Write([]byte{})
 	}
@@ -91,6 +92,7 @@
 	agent := headers.Get("User-Agent")
 	c.Check(agent, Not(Equals), "")
 	c.Check(agent, Equals, gooseAgent())
+	c.Check(req.RespHeaders.Get("Testing"), Equals, "true")
 }
 
 func (s *HTTPClientTestSuite) TestJSONRequestSetsUserAgent(c *C) {
@@ -101,6 +103,7 @@
 	agent := headers.Get("User-Agent")
 	c.Check(agent, Not(Equals), "")
 	c.Check(agent, Equals, gooseAgent())
+	c.Check(req.RespHeaders.Get("Testing"), Equals, "true")
 }
 
 func (s *HTTPClientTestSuite) TestBinaryRequestSetsContentLength(c *C) {

=== modified file 'swift/live_test.go'
--- swift/live_test.go	2013-10-17 23:02:16 +0000
+++ swift/live_test.go	2014-05-02 13:23:44 +0000
@@ -86,7 +86,7 @@
 	data := "...some data..."
 	err := s.swift.PutReader(s.containerName, object, bytes.NewReader([]byte(data)), int64(len(data)))
 	c.Check(err, IsNil)
-	r, err := s.swift.GetReader(s.containerName, object)
+	r, headers, err := s.swift.GetReader(s.containerName, object)
 	c.Check(err, IsNil)
 	readData, err := ioutil.ReadAll(r)
 	c.Check(err, IsNil)
@@ -94,6 +94,7 @@
 	c.Check(string(readData), Equals, data)
 	err = s.swift.DeleteObject(s.containerName, object)
 	c.Assert(err, IsNil)
+	c.Check(headers.Get("Date"), Not(Equals), "")
 }
 
 func (s *LiveTests) TestList(c *C) {
@@ -178,7 +179,7 @@
 	data := "...some data..."
 	err := s.swift.PutReader(s.containerName, object, bytes.NewReader([]byte(data)), int64(len(data)))
 	c.Check(err, IsNil)
-	r, err := s.publicSwift.GetReader(s.containerName, object)
+	r, headers, err := s.publicSwift.GetReader(s.containerName, object)
 	c.Check(err, IsNil)
 	readData, err := ioutil.ReadAll(r)
 	c.Check(err, IsNil)
@@ -186,6 +187,7 @@
 	c.Check(string(readData), Equals, data)
 	err = s.swift.DeleteObject(s.containerName, object)
 	c.Assert(err, IsNil)
+	c.Check(headers.Get("Date"), Not(Equals), "")
 }
 
 func (s *LiveTestsPublicContainer) TestPublicList(c *C) {
@@ -229,3 +231,16 @@
 	err = s.swift.DeleteObject(s.containerName, object)
 	c.Assert(err, IsNil)
 }
+
+func (s *LiveTests) TestHeadObject(c *C) {
+	object := "test_obj2"
+	data := "...some data..."
+	err := s.swift.PutReader(s.containerName, object, bytes.NewReader([]byte(data)), int64(len(data)))
+	c.Check(err, IsNil)
+	headers, err := s.swift.HeadObject(s.containerName, object)
+	c.Check(err, IsNil)
+	err = s.swift.DeleteObject(s.containerName, object)
+	c.Assert(err, IsNil)
+	c.Check(headers.Get("Date"), Not(Equals), "")
+}
+

=== modified file 'swift/swift.go'
--- swift/swift.go	2013-11-01 03:02:01 +0000
+++ swift/swift.go	2014-05-02 13:23:44 +0000
@@ -77,15 +77,15 @@
 }
 
 // HeadObject retrieves object metadata and other standard HTTP headers.
-func (c *Client) HeadObject(containerName, objectName string) (headers http.Header, err error) {
-	requestData := goosehttp.RequestData{ReqHeaders: headers}
-	err = c.touchObject(&requestData, client.HEAD, containerName, objectName)
-	return headers, err
+func (c *Client) HeadObject(containerName, objectName string) (http.Header, error) {
+	requestData := goosehttp.RequestData{}
+    err := c.touchObject(&requestData, client.HEAD, containerName, objectName)
+	return requestData.RespHeaders, err
 }
 
 // GetObject retrieves the specified object's data.
 func (c *Client) GetObject(containerName, objectName string) (obj []byte, err error) {
-	rc, err := c.GetReader(containerName, objectName)
+	rc, _, err := c.GetReader(containerName, objectName)
 	if err != nil {
 		return
 	}
@@ -102,10 +102,10 @@
 }
 
 // GetObject retrieves the specified object's data.
-func (c *Client) GetReader(containerName, objectName string) (rc io.ReadCloser, err error) {
+func (c *Client) GetReader(containerName, objectName string) (io.ReadCloser, http.Header, error) {
 	requestData := goosehttp.RequestData{RespReader: &emptyReadCloser}
-	err = c.touchObject(&requestData, client.GET, containerName, objectName)
-	return requestData.RespReader, err
+	err := c.touchObject(&requestData, client.GET, containerName, objectName)
+	return requestData.RespReader, requestData.RespHeaders, err
 }
 
 // DeleteObject removes an object from the storage system permanently.

=== modified file 'testservices/swiftservice/service_http.go'
--- testservices/swiftservice/service_http.go	2013-11-01 03:11:30 +0000
+++ testservices/swiftservice/service_http.go	2014-05-02 13:23:44 +0000
@@ -76,6 +76,25 @@
 			w.Header().Set("Content-Length", "0")
 			w.WriteHeader(http.StatusNoContent)
 		}
+	case "HEAD":
+		urlParams, err := url.ParseQuery(r.URL.RawQuery)
+		if err != nil {
+			w.WriteHeader(http.StatusInternalServerError)
+			w.Write([]byte(err.Error()))
+			return
+		}
+		params := make(map[string]string, len(urlParams))
+		for k := range urlParams {
+			params[k] = urlParams.Get(k)
+		}
+		_, err = s.ListContainer(container, params)
+		if err != nil {
+			w.WriteHeader(http.StatusInternalServerError)
+			w.Write([]byte(err.Error()))
+		} else {
+			w.WriteHeader(http.StatusOK)
+			w.Header().Set("Content-Type", "application/json; charset=UF-8")
+		}
 	case "PUT":
 		if exists {
 			w.WriteHeader(http.StatusAccepted)
@@ -127,6 +146,9 @@
 			w.Header().Set("Content-Length", "0")
 			w.WriteHeader(http.StatusNoContent)
 		}
+	case "HEAD":
+		w.WriteHeader(http.StatusOK)
+		w.Header().Set("Content-Type", "application/json; charset=UF-8")
 	case "PUT":
 		bodydata, err := ioutil.ReadAll(r.Body)
 		if err != nil {

=== modified file 'testservices/swiftservice/service_http_test.go'
--- testservices/swiftservice/service_http_test.go	2013-10-17 23:02:16 +0000
+++ testservices/swiftservice/service_http_test.go	2014-05-02 13:23:44 +0000
@@ -295,6 +295,56 @@
 	s.removeContainer("test", c)
 }
 
+func (s *SwiftHTTPSuite) TestHEADContainerExistsOK(c *C) {
+	s.ensureContainer("test", c)
+	data := []byte("test data")
+	s.ensureObject("test", "obj", data, c)
+
+	resp := s.sendRequest(c, "HEAD", "test", nil, http.StatusOK)
+	c.Assert(resp.Header.Get("Date"), Not(Equals), "")
+
+	defer resp.Body.Close()
+	body, err := ioutil.ReadAll(resp.Body)
+	c.Assert(err, IsNil)
+	c.Assert(body, DeepEquals, []byte{})
+	s.removeContainer("test", c)
+}
+
+func (s *SwiftHTTPSuite) TestHEADContainerMissingNotFound(c *C) {
+	s.ensureNotContainer("test", c)
+
+	s.sendRequest(c, "HEAD", "test", nil, http.StatusNotFound)
+
+	s.ensureNotContainer("test", c)
+}
+
+func (s *SwiftHTTPSuite) TestHEADObjectExistsOK(c *C) {
+	data := []byte("test data")
+	s.ensureContainer("test", c)
+	s.ensureObject("test", "obj", data, c)
+
+	resp := s.sendRequest(c, "HEAD", "test/obj", nil, http.StatusOK)
+
+	s.ensureObjectData("test", "obj", data, c)
+	c.Assert(resp.Header.Get("Date"), Not(Equals), "")
+
+	defer resp.Body.Close()
+	body, err := ioutil.ReadAll(resp.Body)
+	c.Assert(err, IsNil)
+	c.Assert(body, DeepEquals, []byte{})
+
+	s.removeContainer("test", c)
+}
+
+func (s *SwiftHTTPSuite) TestHEADObjectMissingNotFound(c *C) {
+	s.ensureContainer("test", c)
+	s.ensureNotObject("test", "obj", c)
+
+	s.sendRequest(c, "HEAD", "test/obj", nil, http.StatusNotFound)
+
+	s.removeContainer("test", c)
+}
+
 func (s *SwiftHTTPSuite) TestUnauthorizedFails(c *C) {
 	oldtoken := s.token
 	defer func() {