[http issue 115] NetHttpTransport drops Content-Length header when posting 0-length data

[rmistry, 2012-08-22 12:22:16]

http://codereview.appspot.com/6463064/diff/1/google-http-client/src/main/java...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java
(right):

http://codereview.appspot.com/6463064/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java:76:
// TODO(yanivi): deprecate HttpRequest.setAllowEmptyContent()?
Lets remove the TODO because we have a CL out for it.

http://codereview.appspot.com/6463064/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java:93:
// for HEAD, OPTIONS, DELETE, or TRACE it would throw an exceptions
I am missing something, setDoOutput(true) is not called here, shouldn't it be
called?

http://codereview.appspot.com/6463064/diff/1/google-http-client/src/test/java...
File
google-http-client/src/test/java/com/google/api/client/http/javanet/MockHttpURLConnection.java
(right):

http://codereview.appspot.com/6463064/diff/1/google-http-client/src/test/java...
google-http-client/src/test/java/com/google/api/client/http/javanet/MockHttpURLConnection.java:15:
package com.google.api.client.http.javanet;
Shouldn't this class be in com.google.api.client.testing.http.javanet ?

[rmistry, 2012-08-22 14:36:44]

http://codereview.appspot.com/6463064/diff/1/google-http-client/src/main/java...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java
(right):

http://codereview.appspot.com/6463064/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java:93:
// for HEAD, OPTIONS, DELETE, or TRACE it would throw an exceptions
On 2012/08/22 12:22:16, rmistry wrote:
> I am missing something, setDoOutput(true) is not called here, shouldn't it be
> called?

Lets add an upgrade warning since the behavior has changed for GET + content.

[yanivi, 2012-08-22 18:55:07]

http://codereview.appspot.com/6463064/diff/1/google-http-client/src/main/java...
File
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java
(right):

http://codereview.appspot.com/6463064/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java:76:
// TODO(yanivi): deprecate HttpRequest.setAllowEmptyContent()?
On 2012/08/22 12:22:16, rmistry wrote:
> Lets remove the TODO because we have a CL out for it.

Done.

http://codereview.appspot.com/6463064/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java:93:
// for HEAD, OPTIONS, DELETE, or TRACE it would throw an exceptions
On 2012/08/22 12:22:16, rmistry wrote:
> I am missing something, setDoOutput(true) is not called here, shouldn't it be
> called?

No. I changed the wording of the comment.  Hopefully it is more clear now.

http://codereview.appspot.com/6463064/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java:93:
// for HEAD, OPTIONS, DELETE, or TRACE it would throw an exceptions
On 2012/08/22 14:36:44, rmistry wrote:
> On 2012/08/22 12:22:16, rmistry wrote:
> > I am missing something, setDoOutput(true) is not called here, shouldn't it
be
> > called?
> 
> Lets add an upgrade warning since the behavior has changed for GET + content.

Done.

[rmistry, 2012-08-22 19:00:08]

Also, I am assuming http://codereview.appspot.com/6458156/ will be changed to
set the empty content?

http://codereview.appspot.com/6463064/diff/7001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/testing/http/javanet/package-info.java
(right):

http://codereview.appspot.com/6463064/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/testing/http/javanet/package-info.java:2:
* Copyright (c) 2011 Google Inc.
2012

http://codereview.appspot.com/6463064/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/testing/http/javanet/package-info.java:16:
* Testing utilities used for writing tests based the {@code java.net} package.
based on the

[yanivi, 2012-08-22 19:08:58]

http://codereview.appspot.com/6463064/diff/7001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/testing/http/javanet/package-info.java
(right):

http://codereview.appspot.com/6463064/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/testing/http/javanet/package-info.java:2:
* Copyright (c) 2011 Google Inc.
On 2012/08/22 19:00:09, rmistry wrote:
> 2012

Done.

http://codereview.appspot.com/6463064/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/testing/http/javanet/package-info.java:16:
* Testing utilities used for writing tests based the {@code java.net} package.
On 2012/08/22 19:00:09, rmistry wrote:
> based on the

Done.

[rmistry, 2012-08-22 20:09:55]

LGTM

[mharris, 2012-08-27 21:59:12]

FYI

https://codereview.appspot.com/6463064/diff/2003/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

https://codereview.appspot.com/6463064/diff/2003/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:604:
+ " use setEmptyContent instead (if needed)");
"setEmptyContent" is confusing, as there is no such method. It would be very
helpful if this message said setContent(new EmptyContent()).