Code review comment for lp:~jameinel/goose/transfer-content-length-1124561

Reviewers: mp+170976_code.launchpad.net,

Message:
Please take a look.

Description:
http/client: Set Content-Length

The net/http.Request object takes a Reader
so you have to set ContentLength as an attribute
or it will not set in in the POST request.

Fixes bug #1124561.

Oddly enough, we always buffer everything anyway,
(probably to handle retries?). I would have thought
for at least BinaryRequest we would have not
touched the io.Reader that we got.

https://code.launchpad.net/~jameinel/goose/transfer-content-length-1124561/+merge/170976

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/10465043/

Affected files:
   A [revision details]
   M http/client.go
   M http/client_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: http/client.go
=== modified file 'http/client.go'
--- http/client.go 2013-04-19 09:19:53 +0000
+++ http/client.go 2013-06-23 07:53:30 +0000
@@ -209,6 +209,7 @@
      req.Header.Add(header, value)
     }
    }
+ req.ContentLength = int64(len(reqData))
    resp, err = c.Do(req)
    if err != nil {
     return nil, errors.Newf(err, "failed executing the request %s", URL)

Index: http/client_test.go
=== modified file 'http/client_test.go'
--- http/client_test.go 2013-02-22 00:36:43 +0000
+++ http/client_test.go 2013-06-23 07:53:30 +0000
@@ -1,6 +1,9 @@
  package http

  import (
+ "bytes"
+ "fmt"
+ "io/ioutil"
   . "launchpad.net/gocheck"
   "launchpad.net/goose/testing/httpsuite"
   "net/http"
@@ -52,21 +55,27 @@
    http.Header{"Foo": []string{"Bar"}, "Content-Type":
contentTypes, "Accept": contentTypes, "User-Agent": []string{gooseAgent()}})
  }

-func (s *HTTPClientTestSuite) setupLoopbackRequest() (*http.Header,
*Client) {
+func (s *HTTPClientTestSuite) setupLoopbackRequest() (*http.Header,
*string, *Client) {
   headers := http.Header{}
+ emptyBody := "<no-response-yet>"
+ body := &emptyBody
   handler := func(resp http.ResponseWriter, req *http.Request) {
    headers = req.Header
+ bodyBytes, _ := ioutil.ReadAll(req.Body)
+ content := string(bodyBytes)
+ *body = content
+ req.Body.Close()
    resp.Header().Add("Content-Length", "0")
    resp.WriteHeader(http.StatusNoContent)
    resp.Write([]byte{})
   }
   s.Mux.HandleFunc("/", handler)
   client := New()
- return &headers, client
+ return &headers, body, client
  }

  func (s *HTTPClientTestSuite) TestBinaryRequestSetsUserAgent(c *C) {
- headers, client := s.setupLoopbackRequest()
+ headers, _, client := s.setupLoopbackRequest()
   req := &RequestData{ExpectedStatus: []int{http.StatusNoContent}}
   err := client.BinaryRequest("POST", s.Server.URL, "", req, nil)
   c.Assert(err, IsNil)
@@ -76,7 +85,7 @@
  }

  func (s *HTTPClientTestSuite) TestJSONRequestSetsUserAgent(c *C) {
- headers, client := s.setupLoopbackRequest()
+ headers, _, client := s.setupLoopbackRequest()
   req := &RequestData{ExpectedStatus: []int{http.StatusNoContent}}
   err := client.JsonRequest("POST", s.Server.URL, "", req, nil)
   c.Assert(err, IsNil)
@@ -85,8 +94,42 @@
   c.Check(agent, Equals, gooseAgent())
  }

+func (s *HTTPClientTestSuite) TestBinaryRequestSetsContentLength(c *C) {
+ headers, body, client := s.setupLoopbackRequest()
+ content := "binary\ncontent\n"
+ req := &RequestData{
+ ExpectedStatus: []int{http.StatusNoContent},
+ ReqReader: bytes.NewBufferString(content),
+ ReqLength: len(content),
+ }
+ err := client.BinaryRequest("POST", s.Server.URL, "", req, nil)
+ c.Assert(err, IsNil)
+ encoding := headers.Get("Transfer-Encoding")
+ c.Check(encoding, Equals, "")
+ length := headers.Get("Content-Length")
+ c.Assert(body, NotNil)
+ c.Check(length, Equals, fmt.Sprintf("%d", len(content)))
+ c.Check(*body, Equals, content)
+}
+
+func (s *HTTPClientTestSuite) TestJSONRequestSetsContentLength(c *C) {
+ headers, body, client := s.setupLoopbackRequest()
+ reqMap := map[string]string{"key": "value"}
+ req := &RequestData{
+ ExpectedStatus: []int{http.StatusNoContent},
+ ReqValue: reqMap,
+ }
+ err := client.JsonRequest("POST", s.Server.URL, "", req, nil)
+ c.Assert(err, IsNil)
+ encoding := headers.Get("Transfer-Encoding")
+ c.Check(encoding, Equals, "")
+ length := headers.Get("Content-Length")
+ c.Assert(body, NotNil)
+ c.Check(length, Equals, fmt.Sprintf("%d", len(*body)))
+}
+
  func (s *HTTPClientTestSuite) TestBinaryRequestSetsToken(c *C) {
- headers, client := s.setupLoopbackRequest()
+ headers, _, client := s.setupLoopbackRequest()
   req := &RequestData{ExpectedStatus: []int{http.StatusNoContent}}
   err := client.BinaryRequest("POST", s.Server.URL, "token", req, nil)
   c.Assert(err, IsNil)
@@ -95,7 +138,7 @@
  }

  func (s *HTTPClientTestSuite) TestJSONRequestSetsToken(c *C) {
- headers, client := s.setupLoopbackRequest()
+ headers, _, client := s.setupLoopbackRequest()
   req := &RequestData{ExpectedStatus: []int{http.StatusNoContent}}
   err := client.JsonRequest("POST", s.Server.URL, "token", req, nil)
   c.Assert(err, IsNil)