Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE: implement body streaming for Net::HTTP #1051

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SamSaffron
Copy link

Per #629, this adds support for body stream mocking.

It is only implemented on Net::HTTP for now as this is the most
surgical change we can make without impacting the entire framework.

Per bblimke#629, this adds support for body stream mocking.

It is only implemented on Net::HTTP for now as this is the most
surgical change we can make without impacting the entire framework.
CHANGELOG.md Outdated Show resolved Hide resolved
@bblimke
Copy link
Owner

bblimke commented Apr 6, 2024

@SamSaffron Thank you for the PR and the delightful AI conversation transcript! 😄 It's the first PR I've received with an attached AI discussion, and I appreciate the attempt!

Apologies for the delay in addressing this PR. It requires some investigation into WebMock usage cases, and I haven't been able to allocate sufficient time for that yet.

I find the proposed change intuitive and appreciate its alignment with the existing behavior of the Curb adapter when Transfer-Encoding is set to 'chunked' (as demonstrated in the WebMock specs:

it "should call on_body for each chunk with chunked response" do
stub_request(:any, "example.com").
to_return(body: ["first_chunk", "second_chunk"],
headers: {"Transfer-Encoding" => "chunked"})
test = []
@curl.on_body do |data|
test << data
end
@curl.http_get
expect(test).to eq(["first_chunk", "second_chunk"])
).

However, I have concerns regarding backward compatibility. A search on GitHub reveals instances where the response body is declared as an Array object (https://github.com/search?q=%22to_return%28body%3A+%5B%22+stub_request+NOT+repo%3Abblimke%2Fwebmock+NOT+%22%5D.to_json%22&type=code). While it's surprising that these examples work, it's possible that private projects also rely on this behavior.

One edge case is using an empty array (to_return(body: [])), where Ruby converts the array to the string "[]", which happens to be the same as the JSON representation. Another potentially confusing case is .to_return_json(body: ['a','b']), where the expectation might be to return the JSON representation of an array.

The issue likely due to WebMock's Readme not clearly defining valid cases for using an Array as the response body, yet permitting it. It's possible that for some HTTP clients, it accidentally works as if the JSON representation of an array was declared.

Given the potential for breaking changes, I believe a change like this requires a new major version to avoid backward incompatibility issues.

In the new major version, WebMock API should be deterministic and only accept an Array response for certain cases, rejecting it when the Array type is not supported as a response. This would help prevent confusion and inconsistent behaviour.

To make the WebMock API more deterministic, we could consider introducing a new keyword, :body_chunks, accepted by to_return. This would allow to_return to accept either :body or :body_chunks, but not both, making the intention clear and avoiding confusion in cases like .to_return_json(body: ['a','b']) with chunked Transfer-Encoding.

I'd love to hear your thoughts on this.

@SamSaffron
Copy link
Author

I hear you, the [] edge case is quite a curveball, even though it is technically correct (imo) to break people here, breaking on a minor is no fun.

I would prefer not to introduce "body_chunks" cause it is harder to learn about and instead:

  1. If adapter supports chunked encoding (curb / net http) then yield elements of the array
  2. If adapter does not support chunked encoding then raise "sorry, this pattern is not supported by this adapter yet"
  3. Explicitly raise on [] or any array containing things that are not strings. That way we teach people how to use the API correctly during usage.

That also kind of opens the door to PRs adding this support to other adapters and keeps the simple API

Downside is ... got to wait for a major.

@bblimke
Copy link
Owner

bblimke commented Apr 8, 2024

@SamSaffron thank you for your prompt feedback.

I do agree that adding :body_chunks will make the API more complicated.

I'm concerned about people declaring arrays of strings as the response body, thinking they are declaring a JSON array.

WebMock will accept that and no "sorry, this pattern is not supported by this adapter yet" error will be raised, but instead of a JSON array, they will receive a chunked string.

@SamSaffron
Copy link
Author

Maybe a middle ground here is to make an "option" on webmock, then if you explicitly opt-in you get the chunked behavior and we don't need to unravel history here and can release this on a minor?

Eg:

Webmock.enable_chunked_encoding_mocks!

Of course curb is already a snowflake but we can make this a no-op for curb for now.

@bblimke
Copy link
Owner

bblimke commented May 24, 2024

@SamSaffron thank you! I appreciate your suggestion.

After giving myself more time and spending more time considering the implications, I still have hesitations about an implicit API where the developer is expected to know that declaring body as an Array will lead to a chunked response. If it's not explicit, my initial assumption when seeing body: [] would be that it's declaring a JSON response with an Array, not a chunked response.

While the Webmock.enable_chunked_encoding_mocks! config option you proposed is a reasonable approach, I worry it could make supporting both JSON and chunked responses in the same test suite more challenging. It also introduces another configuration detail for people to keep in mind.

Ideally, I believe the API should aim to be explicit and intuitive to minimize confusion. With that in mind, my current leaning is to have something like body_chunks: [...] or chunked_body: [...], and have to_return raise an argument error if body is also provided. This makes the code intent clear.

Regarding curb, I think we should deprecate declaring the body option as an array, and have WebMock display a helpful deprecation notice guiding users on how to migrate their code. Then in a new major version, we can fully prevent declaring body as an Array.

I'm open to other ideas though! Let me know what you think about this approach or if you have any other suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants