Should have a limit to the amount of content to log

Fix for Bug http://code.google.com/p/google-http-java-client/issues/detail?id=43.

[yanivi, 2011-11-16 16:01:14]

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

http://codereview.appspot.com/5373102/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:338:
"The content logging limit must be non negative.");
non-negative

http://codereview.appspot.com/5373102/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:680:
if (contentLength <= contentLoggingLimit && contentLoggingLimit != 0) {
What about the case of contentLength == -1, i.e. unknown content length? 
Usually it is not -1, but we should try to fully support the case where it is
-1.

I recommend we try to be more robust in LogContent.  We should pass
contentLoggingLimit to LogContent.  In its writeTo() method, we should check
debugContent.length before logging.

http://codereview.appspot.com/5373102/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:683:
logbuf.append("Content will not be logged because the content length " +
contentLength
design question: should we instead show the first contentLoggingLimit bytes?

similarly for HttpResponse

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

http://codereview.appspot.com/5373102/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:280:
"The content logging limit must be non negative.");
"non-negative"

http://codereview.appspot.com/5373102/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:406:
if (debugContentByteArray.length <= contentLoggingLimit && contentLoggingLimit
!= 0) {
don't need contentLoggingLimit since we already know
debugContentByteArray.length != 0

http://codereview.appspot.com/5373102/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:406:
if (debugContentByteArray.length <= contentLoggingLimit && contentLoggingLimit
!= 0) {
missing unit test

[rmistry, 2011-11-16 20:38:14]

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

http://codereview.appspot.com/5373102/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:338:
"The content logging limit must be non negative.");
On 2011/11/16 16:01:14, yanivi wrote:
> non-negative

Done.

http://codereview.appspot.com/5373102/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:680:
if (contentLength <= contentLoggingLimit && contentLoggingLimit != 0) {
On 2011/11/16 16:01:14, yanivi wrote:
> What about the case of contentLength == -1, i.e. unknown content length? 
> Usually it is not -1, but we should try to fully support the case where it is
> -1.
> 
> I recommend we try to be more robust in LogContent.  We should pass
> contentLoggingLimit to LogContent.  In its writeTo() method, we should check
> debugContent.length before logging.

Done. Let me know if this is what you had in mind.

http://codereview.appspot.com/5373102/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:683:
logbuf.append("Content will not be logged because the content length " +
contentLength
On 2011/11/16 16:01:14, yanivi wrote:
> design question: should we instead show the first contentLoggingLimit bytes?
> 
> similarly for HttpResponse

I think it might be confusing for users if we show the first contentLoggingLimit
bytes, they may see it and think that is it for their content, not showing
anything because we went over the limit might be better.

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

http://codereview.appspot.com/5373102/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:280:
"The content logging limit must be non negative.");
On 2011/11/16 16:01:14, yanivi wrote:
> "non-negative"

Done.

http://codereview.appspot.com/5373102/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:406:
if (debugContentByteArray.length <= contentLoggingLimit && contentLoggingLimit
!= 0) {
On 2011/11/16 16:01:14, yanivi wrote:
> don't need contentLoggingLimit since we already know
> debugContentByteArray.length != 0

Done.

http://codereview.appspot.com/5373102/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java:406:
if (debugContentByteArray.length <= contentLoggingLimit && contentLoggingLimit
!= 0) {
On 2011/11/16 16:01:14, yanivi wrote:
> missing unit test

Yes I could not figure out a good way to test this. But after looking at
LogContentTest I now know. Done.

[yanivi, 2011-11-16 22:12:28]

only one important issue left

http://codereview.appspot.com/5373102/diff/3002/google-http-client/src/main/j...
File google-http-client/src/main/java/com/google/api/client/http/LogContent.java
(right):

http://codereview.appspot.com/5373102/diff/3002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/LogContent.java:63:
if (contentLength != -1 && contentLoggingLimit != 0 && contentLength <=
contentLoggingLimit) {
it would be a shame to not log content if contentLength == -1

for a long time we didn't even compute content length for any of our HttpContent
implementations.  I suggest instead checking debugContent.length if
contentLength == -1.  the downside is the risk of OutOfMemoryError, but it feels
like that's the right trade-off.

http://codereview.appspot.com/5373102/diff/3002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/LogContent.java:84:
public int getContentLoggingLimit() {
package-private

[rmistry, 2011-11-16 23:41:06]

FYI, I will wait for this CL to be LGTM'ed and submitted before doing the sync.

http://codereview.appspot.com/5373102/diff/3002/google-http-client/src/main/j...
File google-http-client/src/main/java/com/google/api/client/http/LogContent.java
(right):

http://codereview.appspot.com/5373102/diff/3002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/LogContent.java:63:
if (contentLength != -1 && contentLoggingLimit != 0 && contentLength <=
contentLoggingLimit) {
On 2011/11/16 22:12:28, yanivi wrote:
> it would be a shame to not log content if contentLength == -1
> 
> for a long time we didn't even compute content length for any of our
HttpContent
> implementations.  I suggest instead checking debugContent.length if
> contentLength == -1.  the downside is the risk of OutOfMemoryError, but it
feels
> like that's the right trade-off.

Ok. Thank you for the context. Done.

http://codereview.appspot.com/5373102/diff/3002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/LogContent.java:84:
public int getContentLoggingLimit() {
On 2011/11/16 22:12:28, yanivi wrote:
> package-private

Done.

[yanivi, 2011-11-16 23:52:10]

http://codereview.appspot.com/5373102/diff/8002/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/5373102/diff/8002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:679:
&& (loggable && !disableContentLogging || logger.isLoggable(Level.ALL))) {
we should not use LogContent if contentLength > contentLoggingLimit or if
contentLoggingLimit == 0

http://codereview.appspot.com/5373102/diff/8002/google-http-client/src/main/j...
File google-http-client/src/main/java/com/google/api/client/http/LogContent.java
(right):

http://codereview.appspot.com/5373102/diff/8002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/LogContent.java:70:
if (contentLoggingLimit != 0 && length <= contentLoggingLimit) {
replace lines 65-70 with just:

if (debugContent.length <= contentLoggingLimit) {

[rmistry, 2011-11-17 00:13:59]

http://codereview.appspot.com/5373102/diff/8002/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/5373102/diff/8002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:679:
&& (loggable && !disableContentLogging || logger.isLoggable(Level.ALL))) {
On 2011/11/16 23:52:10, yanivi wrote:
> we should not use LogContent if contentLength > contentLoggingLimit or if
> contentLoggingLimit == 0

Done. Put back the tests as well.

http://codereview.appspot.com/5373102/diff/8002/google-http-client/src/main/j...
File google-http-client/src/main/java/com/google/api/client/http/LogContent.java
(right):

http://codereview.appspot.com/5373102/diff/8002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/LogContent.java:70:
if (contentLoggingLimit != 0 && length <= contentLoggingLimit) {
On 2011/11/16 23:52:10, yanivi wrote:
> replace lines 65-70 with just:
> 
> if (debugContent.length <= contentLoggingLimit) {

Ah makes sense. Done.

[yanivi, 2011-11-17 00:47:33]

LGTM

http://codereview.appspot.com/5373102/diff/6013/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/5373102/diff/6013/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:680:
if (contentLength <= contentLoggingLimit && contentLoggingLimit != 0) {
[optional] merge with enclosing if-statement

http://codereview.appspot.com/5373102/diff/6013/google-http-client/src/main/j...
File google-http-client/src/main/java/com/google/api/client/http/LogContent.java
(right):

http://codereview.appspot.com/5373102/diff/6013/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/LogContent.java:56:
Preconditions.checkArgument(contentLoggingLimit >= 0,
[optional] not needed since this is already checked elsewhere

[rmistry, 2011-11-17 01:13:24]

http://codereview.appspot.com/5373102/diff/6013/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/5373102/diff/6013/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:680:
if (contentLength <= contentLoggingLimit && contentLoggingLimit != 0) {
On 2011/11/17 00:47:33, yanivi wrote:
> [optional] merge with enclosing if-statement

Leaving it as if for now because the enclosing if-statement is already big. I
will merge after disableContentLogging is removed.

http://codereview.appspot.com/5373102/diff/6013/google-http-client/src/main/j...
File google-http-client/src/main/java/com/google/api/client/http/LogContent.java
(right):

http://codereview.appspot.com/5373102/diff/6013/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/LogContent.java:56:
Preconditions.checkArgument(contentLoggingLimit >= 0,
On 2011/11/17 00:47:33, yanivi wrote:
> [optional] not needed since this is already checked elsewhere

Leaving this because in-case LogContent is used from other classes in the future
then they know that -1 is not allowed.