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

Proposed by Kevin McDermott on 2014-05-02
Status: Merged
Approved by: John A Meinel on 2014-05-13
Approved revision: 122
Merged at revision: 123
Proposed branch: lp:~bigkevmcd/goose/fix-for-swift-headers
Merge into: lp:goose
Diff against target: 306 lines (+114/-21)
6 files modified
http/client.go (+14/-11)
http/client_test.go (+3/-0)
swift/live_test.go (+17/-2)
swift/swift.go (+8/-8)
testservices/swiftservice/service_http.go (+22/-0)
testservices/swiftservice/service_http_test.go (+50/-0)
To merge this branch: bzr merge lp:~bigkevmcd/goose/fix-for-swift-headers
Reviewer Review Type Date Requested Status
John A Meinel 2014-05-02 Approve on 2014-05-13
Review via email: mp+218073@code.launchpad.net

Commit message

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).

Description of the change

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).

To post a comment you must log in.
John A Meinel (jameinel) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'http/client.go'
2--- http/client.go 2014-04-23 06:57:55 +0000
3+++ http/client.go 2014-05-02 13:23:44 +0000
4@@ -64,6 +64,7 @@
5 ReqReader io.Reader
6 ReqLength int
7 RespReader io.ReadCloser
8+ RespHeaders http.Header
9 }
10
11 const (
12@@ -136,13 +137,14 @@
13 }
14 }
15 headers := createHeaders(reqData.ReqHeaders, contentTypeJSON, token)
16- respBody, err := c.sendRequest(
17+ resp, err := c.sendRequest(
18 method, url, bytes.NewReader(body), len(body), headers, reqData.ExpectedStatus, logger)
19 if err != nil {
20 return
21 }
22- defer respBody.Close()
23- respData, err := ioutil.ReadAll(respBody)
24+ reqData.RespHeaders = resp.Header
25+ defer resp.Body.Close()
26+ respData, err := ioutil.ReadAll(resp.Body)
27 if err != nil {
28 err = errors.Newf(err, "failed reading the response body")
29 return
30@@ -173,15 +175,16 @@
31 url += "?" + reqData.Params.Encode()
32 }
33 headers := createHeaders(reqData.ReqHeaders, contentTypeOctetStream, token)
34- respBody, err := c.sendRequest(
35+ resp, err := c.sendRequest(
36 method, url, reqData.ReqReader, reqData.ReqLength, headers, reqData.ExpectedStatus, logger)
37 if err != nil {
38 return
39 }
40+ reqData.RespHeaders = resp.Header
41 if reqData.RespReader != nil {
42- reqData.RespReader = respBody
43+ reqData.RespReader = resp.Body
44 } else {
45- respBody.Close()
46+ resp.Body.Close()
47 }
48 return
49 }
50@@ -192,18 +195,18 @@
51 // headers: HTTP headers to include with the request.
52 // expectedStatus: a slice of allowed response status codes.
53 func (c *Client) sendRequest(method, URL string, reqReader io.Reader, length int, headers http.Header,
54- expectedStatus []int, logger *log.Logger) (rc io.ReadCloser, err error) {
55+ expectedStatus []int, logger *log.Logger) (*http.Response, error) {
56 reqData := make([]byte, length)
57 if reqReader != nil {
58 nrRead, err := io.ReadFull(reqReader, reqData)
59 if err != nil {
60 err = errors.Newf(err, "failed reading the request data, read %v of %v bytes", nrRead, length)
61- return rc, err
62+ return nil, err
63 }
64 }
65 rawResp, err := c.sendRateLimitedRequest(method, URL, headers, reqData, logger)
66 if err != nil {
67- return
68+ return nil, err
69 }
70 foundStatus := false
71 if len(expectedStatus) == 0 {
72@@ -218,9 +221,9 @@
73 if !foundStatus && len(expectedStatus) > 0 {
74 err = handleError(URL, rawResp)
75 rawResp.Body.Close()
76- return
77+ return nil, err
78 }
79- return rawResp.Body, err
80+ return rawResp, err
81 }
82
83 func (c *Client) sendRateLimitedRequest(method, URL string, headers http.Header, reqData []byte,
84
85=== modified file 'http/client_test.go'
86--- http/client_test.go 2013-10-17 23:02:16 +0000
87+++ http/client_test.go 2014-05-02 13:23:44 +0000
88@@ -29,6 +29,7 @@
89 req.Body.Close()
90 bodyChan <- string(bodyBytes)
91 resp.Header().Add("Content-Length", "0")
92+ resp.Header().Add("Testing", "true")
93 resp.WriteHeader(http.StatusNoContent)
94 resp.Write([]byte{})
95 }
96@@ -91,6 +92,7 @@
97 agent := headers.Get("User-Agent")
98 c.Check(agent, Not(Equals), "")
99 c.Check(agent, Equals, gooseAgent())
100+ c.Check(req.RespHeaders.Get("Testing"), Equals, "true")
101 }
102
103 func (s *HTTPClientTestSuite) TestJSONRequestSetsUserAgent(c *C) {
104@@ -101,6 +103,7 @@
105 agent := headers.Get("User-Agent")
106 c.Check(agent, Not(Equals), "")
107 c.Check(agent, Equals, gooseAgent())
108+ c.Check(req.RespHeaders.Get("Testing"), Equals, "true")
109 }
110
111 func (s *HTTPClientTestSuite) TestBinaryRequestSetsContentLength(c *C) {
112
113=== modified file 'swift/live_test.go'
114--- swift/live_test.go 2013-10-17 23:02:16 +0000
115+++ swift/live_test.go 2014-05-02 13:23:44 +0000
116@@ -86,7 +86,7 @@
117 data := "...some data..."
118 err := s.swift.PutReader(s.containerName, object, bytes.NewReader([]byte(data)), int64(len(data)))
119 c.Check(err, IsNil)
120- r, err := s.swift.GetReader(s.containerName, object)
121+ r, headers, err := s.swift.GetReader(s.containerName, object)
122 c.Check(err, IsNil)
123 readData, err := ioutil.ReadAll(r)
124 c.Check(err, IsNil)
125@@ -94,6 +94,7 @@
126 c.Check(string(readData), Equals, data)
127 err = s.swift.DeleteObject(s.containerName, object)
128 c.Assert(err, IsNil)
129+ c.Check(headers.Get("Date"), Not(Equals), "")
130 }
131
132 func (s *LiveTests) TestList(c *C) {
133@@ -178,7 +179,7 @@
134 data := "...some data..."
135 err := s.swift.PutReader(s.containerName, object, bytes.NewReader([]byte(data)), int64(len(data)))
136 c.Check(err, IsNil)
137- r, err := s.publicSwift.GetReader(s.containerName, object)
138+ r, headers, err := s.publicSwift.GetReader(s.containerName, object)
139 c.Check(err, IsNil)
140 readData, err := ioutil.ReadAll(r)
141 c.Check(err, IsNil)
142@@ -186,6 +187,7 @@
143 c.Check(string(readData), Equals, data)
144 err = s.swift.DeleteObject(s.containerName, object)
145 c.Assert(err, IsNil)
146+ c.Check(headers.Get("Date"), Not(Equals), "")
147 }
148
149 func (s *LiveTestsPublicContainer) TestPublicList(c *C) {
150@@ -229,3 +231,16 @@
151 err = s.swift.DeleteObject(s.containerName, object)
152 c.Assert(err, IsNil)
153 }
154+
155+func (s *LiveTests) TestHeadObject(c *C) {
156+ object := "test_obj2"
157+ data := "...some data..."
158+ err := s.swift.PutReader(s.containerName, object, bytes.NewReader([]byte(data)), int64(len(data)))
159+ c.Check(err, IsNil)
160+ headers, err := s.swift.HeadObject(s.containerName, object)
161+ c.Check(err, IsNil)
162+ err = s.swift.DeleteObject(s.containerName, object)
163+ c.Assert(err, IsNil)
164+ c.Check(headers.Get("Date"), Not(Equals), "")
165+}
166+
167
168=== modified file 'swift/swift.go'
169--- swift/swift.go 2013-11-01 03:02:01 +0000
170+++ swift/swift.go 2014-05-02 13:23:44 +0000
171@@ -77,15 +77,15 @@
172 }
173
174 // HeadObject retrieves object metadata and other standard HTTP headers.
175-func (c *Client) HeadObject(containerName, objectName string) (headers http.Header, err error) {
176- requestData := goosehttp.RequestData{ReqHeaders: headers}
177- err = c.touchObject(&requestData, client.HEAD, containerName, objectName)
178- return headers, err
179+func (c *Client) HeadObject(containerName, objectName string) (http.Header, error) {
180+ requestData := goosehttp.RequestData{}
181+ err := c.touchObject(&requestData, client.HEAD, containerName, objectName)
182+ return requestData.RespHeaders, err
183 }
184
185 // GetObject retrieves the specified object's data.
186 func (c *Client) GetObject(containerName, objectName string) (obj []byte, err error) {
187- rc, err := c.GetReader(containerName, objectName)
188+ rc, _, err := c.GetReader(containerName, objectName)
189 if err != nil {
190 return
191 }
192@@ -102,10 +102,10 @@
193 }
194
195 // GetObject retrieves the specified object's data.
196-func (c *Client) GetReader(containerName, objectName string) (rc io.ReadCloser, err error) {
197+func (c *Client) GetReader(containerName, objectName string) (io.ReadCloser, http.Header, error) {
198 requestData := goosehttp.RequestData{RespReader: &emptyReadCloser}
199- err = c.touchObject(&requestData, client.GET, containerName, objectName)
200- return requestData.RespReader, err
201+ err := c.touchObject(&requestData, client.GET, containerName, objectName)
202+ return requestData.RespReader, requestData.RespHeaders, err
203 }
204
205 // DeleteObject removes an object from the storage system permanently.
206
207=== modified file 'testservices/swiftservice/service_http.go'
208--- testservices/swiftservice/service_http.go 2013-11-01 03:11:30 +0000
209+++ testservices/swiftservice/service_http.go 2014-05-02 13:23:44 +0000
210@@ -76,6 +76,25 @@
211 w.Header().Set("Content-Length", "0")
212 w.WriteHeader(http.StatusNoContent)
213 }
214+ case "HEAD":
215+ urlParams, err := url.ParseQuery(r.URL.RawQuery)
216+ if err != nil {
217+ w.WriteHeader(http.StatusInternalServerError)
218+ w.Write([]byte(err.Error()))
219+ return
220+ }
221+ params := make(map[string]string, len(urlParams))
222+ for k := range urlParams {
223+ params[k] = urlParams.Get(k)
224+ }
225+ _, err = s.ListContainer(container, params)
226+ if err != nil {
227+ w.WriteHeader(http.StatusInternalServerError)
228+ w.Write([]byte(err.Error()))
229+ } else {
230+ w.WriteHeader(http.StatusOK)
231+ w.Header().Set("Content-Type", "application/json; charset=UF-8")
232+ }
233 case "PUT":
234 if exists {
235 w.WriteHeader(http.StatusAccepted)
236@@ -127,6 +146,9 @@
237 w.Header().Set("Content-Length", "0")
238 w.WriteHeader(http.StatusNoContent)
239 }
240+ case "HEAD":
241+ w.WriteHeader(http.StatusOK)
242+ w.Header().Set("Content-Type", "application/json; charset=UF-8")
243 case "PUT":
244 bodydata, err := ioutil.ReadAll(r.Body)
245 if err != nil {
246
247=== modified file 'testservices/swiftservice/service_http_test.go'
248--- testservices/swiftservice/service_http_test.go 2013-10-17 23:02:16 +0000
249+++ testservices/swiftservice/service_http_test.go 2014-05-02 13:23:44 +0000
250@@ -295,6 +295,56 @@
251 s.removeContainer("test", c)
252 }
253
254+func (s *SwiftHTTPSuite) TestHEADContainerExistsOK(c *C) {
255+ s.ensureContainer("test", c)
256+ data := []byte("test data")
257+ s.ensureObject("test", "obj", data, c)
258+
259+ resp := s.sendRequest(c, "HEAD", "test", nil, http.StatusOK)
260+ c.Assert(resp.Header.Get("Date"), Not(Equals), "")
261+
262+ defer resp.Body.Close()
263+ body, err := ioutil.ReadAll(resp.Body)
264+ c.Assert(err, IsNil)
265+ c.Assert(body, DeepEquals, []byte{})
266+ s.removeContainer("test", c)
267+}
268+
269+func (s *SwiftHTTPSuite) TestHEADContainerMissingNotFound(c *C) {
270+ s.ensureNotContainer("test", c)
271+
272+ s.sendRequest(c, "HEAD", "test", nil, http.StatusNotFound)
273+
274+ s.ensureNotContainer("test", c)
275+}
276+
277+func (s *SwiftHTTPSuite) TestHEADObjectExistsOK(c *C) {
278+ data := []byte("test data")
279+ s.ensureContainer("test", c)
280+ s.ensureObject("test", "obj", data, c)
281+
282+ resp := s.sendRequest(c, "HEAD", "test/obj", nil, http.StatusOK)
283+
284+ s.ensureObjectData("test", "obj", data, c)
285+ c.Assert(resp.Header.Get("Date"), Not(Equals), "")
286+
287+ defer resp.Body.Close()
288+ body, err := ioutil.ReadAll(resp.Body)
289+ c.Assert(err, IsNil)
290+ c.Assert(body, DeepEquals, []byte{})
291+
292+ s.removeContainer("test", c)
293+}
294+
295+func (s *SwiftHTTPSuite) TestHEADObjectMissingNotFound(c *C) {
296+ s.ensureContainer("test", c)
297+ s.ensureNotObject("test", "obj", c)
298+
299+ s.sendRequest(c, "HEAD", "test/obj", nil, http.StatusNotFound)
300+
301+ s.removeContainer("test", c)
302+}
303+
304 func (s *SwiftHTTPSuite) TestUnauthorizedFails(c *C) {
305 oldtoken := s.token
306 defer func() {

Subscribers

People subscribed via source and target branches