webmock
webmock copied to clipboard
FEATURE: implement body streaming for Net::HTTP
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.
@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: https://github.com/bblimke/webmock/blob/fc6a2ab897a069d861adbc1c88e51b2cf8aa88ac/spec/acceptance/curb/curb_spec.rb#L101-L111).
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.
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:
- If adapter supports chunked encoding (curb / net http) then yield elements of the array
- If adapter does not support chunked encoding then raise "sorry, this pattern is not supported by this adapter yet"
- 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.
@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.
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.