Skip to content

Expose ForwardingRequestBody in the main module - #9808

Open
Amir-Golmoradi wants to merge 2 commits into
lysine-dev:mainfrom
Amir-Golmoradi:add-forwarding-request-body
Open

Amir-Golmoradi wants to merge 2 commits into
lysine-dev:mainfrom
Amir-Golmoradi:add-forwarding-request-body

Conversation

@Amir-Golmoradi

Copy link
Copy Markdown
Contributor

Fixes #9800

Wrapping a RequestBody to change contentType() also means forwarding isOneShot() and isDuplex(). Missing either loses the delegate's behavior, and each wrapper has to keep up when new methods are added.

I moved the existing ForwardingRequestBody from okhttp-testing-support into the main module and added the missing isOneShot() delegation. Subclasses can override contentType() while inheriting the rest of the forwarding behavior.

Tests cover null and non-null content types, one-shot delegation, and overriding the content type while preserving the content, content length, and all four combinations of one-shot and duplex flags. The JVM and Android API snapshots are updated.

The ForwardingRequestBodyTest tests, apiCheck, and spotlessCheck pass.

Full check failed on nine JVM tests and one Android host test. All nine JVM tests also fail on the unchanged base commit e71e32849, though connectTimeout fails with a different assertion. The Android host failure hasn't been compared with the base.

@JakeWharton

Copy link
Copy Markdown
Collaborator

Are you using an LLM to write these? Why are you counting test failures?

@Amir-Golmoradi

Copy link
Copy Markdown
Contributor Author

After encountering unexpected failures in IPv6-related tests, I wanted some context on how to investigate them. I even used an LLM to research the issue, but I haven't established the underlying cause.

I reported the failures to make the local validation results clear. Two IPv6 tests expected hello from IPv6 but received unexpected call to IPv4. Both also failed with the same assertions on the unchanged base commit e71e32849.

I couldn't identify the root cause locally, so I opened the PR with these failures documented to ask for help investigating them.

I wrote all the KDocs, tests, and comments, prepared the PRs, and made all the changes myself.

@JakeWharton JakeWharton left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a test that uses reflection to ensure all declared methods on the ResponseBody are forwarded. Otherwise we could add a new function with a default implementation without overriding on this new type.

delegate.writeTo(sink)
}

// Duplex allows the request body to be written while the response body is read.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Duplex allows the request body to be written while the response body is read.

This comment provides no value on top of the KDoc of the base method. It's also not visible to anyone since it's not KDoc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

import okio.BufferedSink

open class ForwardingRequestBody(
delegate: RequestBody?,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not accept nulls

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


open class ForwardingRequestBodyTest {
@Test
fun forwardsIsOneShot() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is redundant given the one that follows it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Amir-Golmoradi

Copy link
Copy Markdown
Contributor Author

We should have a test that uses reflection to ensure all declared methods on the ResponseBody are forwarded. Otherwise we could add a new function with a default implementation without overriding on this new type.

@JakeWharton
Did you mean RequestBody rather than ResponseBody here, since this PR adds ForwardingRequestBody, which directly extends RequestBody?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Consider ForwardingRequestBody

3 participants