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

Net::HTTPResponse nil checking #71

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Conversation

BrianHawley
Copy link
Contributor

Fix nil handling in read_body and stream_check.

Fixes: #70

Fix nil handling in read_body and stream_check.

Fixes: ruby#70
@BrianHawley
Copy link
Contributor Author

@nobu or @jeremyevans what do you think? The tests pass locally. Just improving error handling.

@jeremyevans
Copy link
Contributor

The stream_check change looks fine. It's questionable whether body should return nil or an empty string in such an error case. That's probably a decision that @nurse should make.

@BrianHawley
Copy link
Contributor Author

BrianHawley commented Sep 26, 2022

@jeremyevans if body should return an empty string, then we should assign the empty string a few lines above instead of assigning nil. At that point, the line I added can be removed and the test can be adjusted accordingly.

It's notable that if you do a head request now, body currently returns nil, so that's the behavior I was emulating. Same goes for 204 responses, like the test, or any other response in lib/net/http/responses.rb with HAS_BODY = false. I had to explicitly set the encoding to even get the test to fail at all, as that attempt to set the encoding on nil is the bug, not the nil itself. It looks like the existing code returns nil if there is no body, which helps to distinguish from when it returns an empty string for a zero-length body.

There is a lot of Ruby built-in support for dealing with nils (blank? being an ActiveSupport add-on), but there is also a lot of code that seems to not realize that body could have returned nil the whole time, and just assumes that it returned a string or raised an exception. I like being able to tell the difference between no body and zero-length content, especially since I can just do response.body.to_s if that distinction doesn't matter to me.

If it's to return an empty string, it'd be good to make sure that the string is not frozen, just to be consistent with the string returned if the body exists. You never know who depends on that value being modifiable.

@BrianHawley
Copy link
Contributor Author

@jeremyevans and @nurse what do I need to do to get this moving forward? Should I split the read_body and stream_check changes into separate PRs? I made this PR in September, so I'm interested in getting this in.

@jeremyevans
Copy link
Contributor

@BrianHawley I don't think there is more you can do. @hsbt @nobu Can either of you review/merge this?

@BrianHawley
Copy link
Contributor Author

BrianHawley commented Oct 5, 2023

Can this get merged in time to get it into Ruby 3.3? I made sure to make the PR in time for it to get into Ruby 3.2.

Attn: @hsbt, @nobu, @nurse, @jeremyevans.

@nurse nurse merged commit 46ca992 into ruby:master Oct 5, 2023
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.

Net::HTTPResponse has bad nil handling in read_body and stream_check
3 participants