Merge lp:~jameinel/goose/transfer-content-length-1124561 into lp:goose

[John A Meinel]

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://codereview.appspot.com/10465043/

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://codereview.appspot.com/10465043/

[John A Meinel (jameinel)]

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

[Dave Cheney (dave-cheney)]

LGTM. I can't believe that bug in nginx took SO LONG to get fixed.

On Sun, Jun 23, 2013 at 7:17 PM, John A Meinel <email address hidden>wrote:

> The proposal to merge lp:~jameinel/goose/transfer-content-length-1124561
> into lp:goose has been updated.
>
> Description changed to:
>
> 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://codereview.appspot.com/10465043/
>
>
> For more details, see:
>
> https://code.launchpad.net/~jameinel/goose/transfer-content-length-1124561/+merge/170976
> --
>
> https://code.launchpad.net/~jameinel/goose/transfer-content-length-1124561/+merge/170976
> Your team juju hackers is requested to review the proposed merge of
> lp:~jameinel/goose/transfer-content-length-1124561 into lp:goose.
>

[Martin Packman (gz)]

LGTM, let's land this so it's in the next juju-core release.

https://codereview.appspot.com/10465043/diff/1/http/client_test.go
File http/client_test.go (right):

https://codereview.appspot.com/10465043/diff/1/http/client_test.go#newcode60
http/client_test.go:60: emptyBody := "<no-response-yet>"
Why not just the empty string here?

https://codereview.appspot.com/10465043/

[lp:~jameinel/goose/transfer-content-length-1124561]

97. By John A Meinel on 2013-06-27

feedback from dfc.

[John A Meinel (jameinel)]

Please take a look.

https://codereview.appspot.com/10465043/diff/1/http/client_test.go
File http/client_test.go (right):

https://codereview.appspot.com/10465043/diff/1/http/client_test.go#newcode59
http/client_test.go:59: headers := http.Header{}
On 2013/06/26 23:48:37, dfc wrote:
> var headers http.Header

Care to explain why one would be preferred to the other?

https://codereview.appspot.com/10465043/diff/1/http/client_test.go#newcode60
http/client_test.go:60: emptyBody := "<no-response-yet>"
On 2013/06/26 14:59:43, gz wrote:
> Why not just the empty string here?

I was distinguishing between "the body read from remote was empty" and
"I didn't get to the point of reading the content".

I can probably switch back, though.

https://codereview.appspot.com/10465043/diff/1/http/client_test.go#newcode110
http/client_test.go:110: c.Assert(body, NotNil)
On 2013/06/26 23:48:37, dfc wrote:
> this is weird, i cannot see how body will ever not be nil

body is a pointer to a string, and when we get the response from the
server, we point that to the contents of the request.

Why would it be nil?

https://codereview.appspot.com/10465043/

[John A Meinel (jameinel)]

Please take a look.

https://codereview.appspot.com/10465043/

[lp:~jameinel/goose/transfer-content-length-1124561]

98. By John A Meinel on 2013-06-27

Use a channel, since dfc didn't like the string pointer

[John A Meinel (jameinel)]

Please take a look.

https://codereview.appspot.com/10465043/

[Dimiter Naydenov (dimitern)]

LGTM

https://codereview.appspot.com/10465043/

=== modified file 'http/client.go'
--- http/client.go	2013-04-19 09:19:53 +0000
+++ http/client.go	2013-06-27 06:51:25 +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)

=== modified file 'http/client_test.go'
--- http/client_test.go	2013-02-22 00:36:43 +0000
+++ http/client_test.go	2013-06-27 06:51:25 +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,25 @@
 		http.Header{"Foo": []string{"Bar"}, "Content-Type": contentTypes, "Accept": contentTypes, "User-Agent": []string{gooseAgent()}})
 }
 
-func (s *HTTPClientTestSuite) setupLoopbackRequest() (*http.Header, *Client) {
-	headers := http.Header{}
+func (s *HTTPClientTestSuite) setupLoopbackRequest() (*http.Header, chan string, *Client) {
+	var headers http.Header
+	bodyChan := make(chan string, 1)
 	handler := func(resp http.ResponseWriter, req *http.Request) {
 		headers = req.Header
+		bodyBytes, _ := ioutil.ReadAll(req.Body)
+		req.Body.Close()
+		bodyChan <- string(bodyBytes)
 		resp.Header().Add("Content-Length", "0")
 		resp.WriteHeader(http.StatusNoContent)
 		resp.Write([]byte{})
 	}
 	s.Mux.HandleFunc("/", handler)
 	client := New()
-	return &headers, client
+	return &headers, bodyChan, 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 +83,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 +92,45 @@
 	c.Check(agent, Equals, gooseAgent())
 }
 
+func (s *HTTPClientTestSuite) TestBinaryRequestSetsContentLength(c *C) {
+	headers, bodyChan, 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.Check(length, Equals, fmt.Sprintf("%d", len(content)))
+	body, ok := <-bodyChan
+	c.Assert(ok, Equals, true)
+	c.Check(body, Equals, content)
+}
+
+func (s *HTTPClientTestSuite) TestJSONRequestSetsContentLength(c *C) {
+	headers, bodyChan, 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")
+	body, ok := <-bodyChan
+	c.Assert(ok, Equals, true)
+	c.Check(body, Not(Equals), "")
+	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 +139,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)