Added support for a maximum content length property. The property rep… #12

eschlenz · 2017-02-10T22:16:12Z

…resents a threshold for content that should be saved verbatim, or not.

WHAT: This PR introduces a "max content length" property to the ChuckInterceptor. A request or response body that exceeds the new property's value will not be recorded. That is, there will still be a record of the transaction, but the content itself is not kept.

WHY: Chuck was crashing on me when I would try to go to the Chuck UI. I determined that the saved response body for a few of my requests were simply too large for SQLite to handle. In such scenarios, I still want to see the transaction in the log. I don't need to see the content however. It made sense to apply this new max content length property to requests as well.

The max content length property is essentially disabled by default (set to Integer.MAX_VALUE). The user can optionally set a max length that works for them.

MISC: I tried to stick with the existing code conventions. Thanks for putting this tool together. It's very handy.

…resents a threshold for content that should be saved verbatim, or not.

joaocsousa · 2017-02-13T09:24:00Z

library/src/main/java/com/readystatesoftware/chuck/ChuckInterceptor.java

@@ -52,6 +52,7 @@
    private Context context;
    private NotificationHelper notificationHelper;
    private boolean showNotification;
+    private int maxContentLength = Integer.MAX_VALUE;


If this field will be compared against a Long (transaction.getRequestContentLength), shouldn't this field also be a long with default value Long.MAX_VALUE?

Good catch! I'll fix that right now.

joaocsousa · 2017-02-13T09:34:14Z

library/src/main/java/com/readystatesoftware/chuck/ChuckInterceptor.java

+                    }
+                }
+                if (isPlaintext(buffer)) {
+                    transaction.setResponseBody(buffer.clone().readString(charset));


I'm not sure if this would work, but instead of ignoring the response all together (due to it being too big), couldn't you try to truncate it instead to the bytes specified by maxContentLength, via buffer.clone().readString(maxContentLength, charset)?

I thought about doing that, but that would essentially create malformed JSON. So when the FormatUtils.formatJson() runs to try to "pretty up" the JSON for display, it's probably going to barf.

I agree with @joaocsousa that truncating the response is a better approach. The formatting methods shouldn't barf, they'll just return the original text if there are parsing errors, and it would greatly simplify this change.

If we created a method like this:

private String readFromBuffer(Buffer buffer, Charset charset) { long bufferSize = buffer.size(); long maxBytes = Math.min(bufferSize, maxContentLength); String body = buffer.readString(maxBytes, charset); if (bufferSize > maxContentLength) { body += "\n\n--- Content truncated ---"; } return body; }

then it could simply be called where we set the request and response body on the transaction, and there's no need to store these requestBodyIsTooBig and responseBodyIsTooBig fields in the database. Everything else would work as is.

jgilfelt · 2017-02-13T16:28:11Z

Thanks! I've finally got my mock web server setup to send responses large enough to break things, so I will look at this as soon as possible.

I looks like it is Android's CursorWindow that breaks first, whose hard limit is 2048 KB.

eschlenz · 2017-02-13T16:58:00Z

@joaocsousa @jgilfelt Thanks for the review. I've made the switch from Integer to Long per joaocsuousa's recommendation. I've responded to the other callout (whether or not to keep the partial result).

eschlenz · 2017-02-13T18:26:10Z

Fixed the merge conflicts as well.

…ata size - refs #12

jgilfelt · 2017-02-13T23:26:35Z

library/src/main/java/com/readystatesoftware/chuck/ChuckInterceptor.java

@@ -74,6 +74,7 @@
    private NotificationHelper notificationHelper;
    private RetentionManager retentionManager;
    private boolean showNotification;
+    private long maxContentLength = Long.MAX_VALUE;


I think we should set a conservative default value that will ensure we don't exceed the platform limits, and let users increase it if they need to. We know the CursorWindow limit is 2 MB, but even at that size, I found that a response may still cause ANR issues when displayed with TextView, and potentially consume the application's Java heap space.

I think 250000 is a good default that will be suitable for most apps, and ensures things wont break.

jgilfelt · 2017-02-13T23:27:20Z

library/src/main/java/com/readystatesoftware/chuck/ChuckInterceptor.java

+     * @param max the max length for request/response content.
+     * @return The {@link ChuckInterceptor} instance.
+     */
+    public ChuckInterceptor maxContentLength(long max) {


You'll need to shadow any methods that are public API in the corresponding class within the library-no-op module.

jgilfelt · 2017-02-13T23:29:30Z

library/src/main/java/com/readystatesoftware/chuck/ChuckInterceptor.java

+                    }
+                }
+                if (isPlaintext(buffer)) {
+                    transaction.setResponseBody(buffer.clone().readString(charset));


I agree with @joaocsousa that truncating the response is a better approach. The formatting methods shouldn't barf, they'll just return the original text if there are parsing errors, and it would greatly simplify this change.

If we created a method like this:

private String readFromBuffer(Buffer buffer, Charset charset) { long bufferSize = buffer.size(); long maxBytes = Math.min(bufferSize, maxContentLength); String body = buffer.readString(maxBytes, charset); if (bufferSize > maxContentLength) { body += "\n\n--- Content truncated ---"; } return body; }

then it could simply be called where we set the request and response body on the transaction, and there's no need to store these requestBodyIsTooBig and responseBodyIsTooBig fields in the database. Everything else would work as is.

eschlenz · 2017-02-13T23:49:48Z

@jgilfelt Saw your change requests. I'll knock that out tomorrow and hit you back.

…esponse body)

eschlenz · 2017-02-14T16:30:37Z

@jgilfelt @joaocsousa

I have addressed the feedback you guys gave me:

I have shadowed the maxContentLength function in the no-op library.
I have removed the "content too big" properties that I had previously added to the HttpTransaction model.
The content is now retained, but truncated if it is too long.
I have set the default maxContentLength to a more reasonable (and logical) value of 250000L.

With maxContentLength left at its default value:

With maxContentLength set to 5 (for testing):

jgilfelt · 2017-02-14T17:23:26Z

@eschlenz Nice work! I've just updated the method comment to reflect the new truncation behaviour. Thanks for putting this together so quickly.

eschlenz · 2017-02-14T17:27:50Z

@jgilfelt No problem! Happy to help.

Added support for a maximum content length property. The property rep…

f3cc1a7

…resents a threshold for content that should be saved verbatim, or not.

joaocsousa reviewed Feb 13, 2017

View reviewed changes

Changed maxContentLength property to be a Long instead of an Integer

db38d5e

eschlenz and others added 2 commits February 13, 2017 11:24

Merge branch 'master' into hotfix/max_content_length

7a0d540

Fixing merge conflict compilation issue

bc77078

jgilfelt added a commit that referenced this pull request Feb 13, 2017

exclude body fields from the list cursor to mitigate crashes due to d…

8ee9204

…ata size - refs #12

jgilfelt requested changes Feb 13, 2017

View reviewed changes

eschlenz added 5 commits February 14, 2017 09:16

Truncating request/response body content if it exceeds maxContentLength

a18a14b

Removing white space. Removed test code for maxContentLength

2262bb6

Moving setResponseContentLength back to where it was (after setting r…

eb9125e

…esponse body)

Removing unused String resource

05f6666

Moving unexpected eof and content truncated string to string resources

7cb8191

jgilfelt approved these changes Feb 14, 2017

View reviewed changes

jgilfelt merged commit 7cb8191 into jgilfelt:master Feb 14, 2017

eschlenz deleted the hotfix/max_content_length branch February 14, 2017 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for a maximum content length property. The property rep… #12

Added support for a maximum content length property. The property rep… #12

eschlenz commented Feb 10, 2017 •

edited

joaocsousa Feb 13, 2017

eschlenz Feb 13, 2017

joaocsousa Feb 13, 2017 •

edited

eschlenz Feb 13, 2017

jgilfelt Feb 13, 2017

jgilfelt commented Feb 13, 2017

eschlenz commented Feb 13, 2017

eschlenz commented Feb 13, 2017

jgilfelt Feb 13, 2017

eschlenz Feb 13, 2017

jgilfelt Feb 13, 2017

eschlenz Feb 13, 2017

jgilfelt Feb 13, 2017

eschlenz commented Feb 13, 2017

eschlenz commented Feb 14, 2017

jgilfelt commented Feb 14, 2017

eschlenz commented Feb 14, 2017

Added support for a maximum content length property. The property rep… #12

Added support for a maximum content length property. The property rep… #12

Conversation

eschlenz commented Feb 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaocsousa Feb 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgilfelt commented Feb 13, 2017

eschlenz commented Feb 13, 2017

eschlenz commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschlenz commented Feb 13, 2017

eschlenz commented Feb 14, 2017

jgilfelt commented Feb 14, 2017

eschlenz commented Feb 14, 2017

eschlenz commented Feb 10, 2017 •

edited

joaocsousa Feb 13, 2017 •

edited