Do not rely on "Content-Length" for payload size. by MiSikora · Pull Request #391 · ChuckerTeam/chucker

MiSikora · 2020-07-19T05:06:40Z

📷 Screenshots

Before	After

📄 Context

There is an open issue - #209. Also, recently the community asked for it.

📝 Changes

Two main changes. I redesigned a little bit DB entities to reflect that they have payload size and not content length. This required a destructive migration. Since this is changed, the field is set after the response read is done by ChuckerInterceptor and it does not rely on the Content-Length header.

🚫 Breaking

No.

🛠️ How to test

Run the sample app. Responses without the Content-Length headers should display now payload size correctly. Also, some tests were written.

⏱️ Next steps

Closes #209.

cortinico

Looks good to me, just a couple of small comments 👌

cortinico · 2020-07-25T12:01:19Z

+         * that the client downloaded even if the [file] is corrupted.
         */
-        fun onClosed(file: File)
+        fun onClosed(file: File, totalBytesRead: Long)


Let's invert the parameter order either here or in the onFailure:

fun onClosed(file: File, totalBytesRead: Long) fun onFailure(file: File, exception: IOException)

File should always be either the first or the last param.

vbuberen

LGTM. Thanks there. Just a minor comment update and we are good to merge.

vbuberen · 2020-07-25T14:30:06Z

         * Called when the upstream was closed. All read bytes are copied to the [file].
-         * This does not mean that the content of a [file] is valid. Only that the user
-         * is done with the reading process.
+         * This does not mean that the content of the [file] is valid. Only that the user


Minor observation - I guess user on this line and client on 95 mean the same thing. Suggest to pick one word for naming. client seems more suitable for this case.

vbuberen · 2020-07-25T14:32:03Z

-    fileFactory: FileFactory,
-    maxContentLength: Long = 250000L,
-    headersToRedact: Set<String> = emptySet()
+internal data class ChuckerInterceptorDelegate(


Not sure I understood what this change gives us. Could explain, please?

It comes from some tests for maxContentLength.

chucker/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt

Lines 269 to 282 in e3e259a

fun payloadSize_isNotLimitedByInterceptorMaxContentLength(factory: ClientFactory) {

val body = Buffer().apply {

repeat(10_000) { writeUtf8("!") }

}

server.enqueue(MockResponse().setBody(body))

val request = Request.Builder().url(serverUrl).build()

val chuckerInterceptor = chuckerInterceptor.copy(maxContentLength = 1_000)

val client = factory.create(chuckerInterceptor)

client.newCall(request).execute().readByteStringBody()

val transaction = chuckerInterceptor.expectTransaction()

assertThat(transaction.responsePayloadSize).isEqualTo(body.size())

}

Since the interceptor is created in setUp there is no way of changing its input parameters. I wanted to have more granular control over that and an explicit test.

Now that I'm thinking copy is not needed. I could just create a new interceptor. What do you think?

It might be a question of preference. I think creation of a new interceptor would be sort of more understandable for a new reader of the code.

Create it with a constructor and explicit arguments for better readability.

Michał Sikora added 2 commits July 19, 2020 07:55

Do not rely on "Content-Length" for payload size.

b441200

Add tests for max content length.

8168ddc

cortinico approved these changes Jul 25, 2020

View reviewed changes

Align order of parameters in callback methods.

e3e259a

vbuberen approved these changes Jul 25, 2020

View reviewed changes

Michał Sikora added 3 commits July 25, 2020 17:00

Align documentation wording

fa59640

Do not copy interceptor in tests

f4755c9

Create it with a constructor and explicit arguments for better readability.

Set lower bound for payload size test

5029636

vbuberen approved these changes Jul 25, 2020

View reviewed changes

vbuberen merged commit ba2c21b into ChuckerTeam:develop Jul 25, 2020

MiSikora deleted the payload-size branch August 22, 2020 10:31

vbuberen added this to the 3.3.0 milestone Sep 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Do not rely on "Content-Length" for payload size.#391

Do not rely on "Content-Length" for payload size.#391
vbuberen merged 6 commits into
ChuckerTeam:developfrom
MiSikora:payload-size

MiSikora commented Jul 19, 2020 •

edited

Loading

Uh oh!

cortinico left a comment

Uh oh!

cortinico Jul 25, 2020

Uh oh!

Uh oh!

vbuberen left a comment

Uh oh!

vbuberen Jul 25, 2020

Uh oh!

vbuberen Jul 25, 2020

Uh oh!

MiSikora Jul 25, 2020 •

edited

Loading

Uh oh!

MiSikora Jul 25, 2020

Uh oh!

vbuberen Jul 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	fun payloadSize_isNotLimitedByInterceptorMaxContentLength(factory: ClientFactory) {
	val body = Buffer().apply {
	repeat(10_000) { writeUtf8("!") }
	}
	server.enqueue(MockResponse().setBody(body))
	val request = Request.Builder().url(serverUrl).build()

	val chuckerInterceptor = chuckerInterceptor.copy(maxContentLength = 1_000)
	val client = factory.create(chuckerInterceptor)
	client.newCall(request).execute().readByteStringBody()

	val transaction = chuckerInterceptor.expectTransaction()
	assertThat(transaction.responsePayloadSize).isEqualTo(body.size())
	}

Uh oh!

Conversation

MiSikora commented Jul 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📷 Screenshots

📄 Context

📝 Changes

🚫 Breaking

🛠️ How to test

⏱️ Next steps

Uh oh!

cortinico left a comment

Choose a reason for hiding this comment

Uh oh!

cortinico Jul 25, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vbuberen left a comment

Choose a reason for hiding this comment

Uh oh!

vbuberen Jul 25, 2020

Choose a reason for hiding this comment

Uh oh!

vbuberen Jul 25, 2020

Choose a reason for hiding this comment

Uh oh!

MiSikora Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiSikora Jul 25, 2020

Choose a reason for hiding this comment

Uh oh!

vbuberen Jul 25, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MiSikora commented Jul 19, 2020 •

edited

Loading

MiSikora Jul 25, 2020 •

edited

Loading