HTTP::Client: don't turn body_io into nil if content-length is zero #8503
Conversation
src/http/common.cr
Outdated
| @@ -38,7 +38,7 @@ module HTTP | |||
| if body_type.prohibited? | |||
| body = nil | |||
| elsif content_length = content_length(headers) | |||
| if content_length != 0 | |||
| if content_length >= 0 | |||
There was a problem hiding this comment.
The type is UInt64, so this condition will always be true.
There was a problem hiding this comment.
Regardless of the type, what would it even mean to have a negative length?
There was a problem hiding this comment.
the content_length method seems to be calling to_u64 on the string, so a negative length will raise overflow, I think.
I guess negative content length doesn't make sense, I was just checking because I don't know how to handle negative values. But since I now know it's handled (kind of badly) in the content_length method, it's probably good now.
src/http/common.cr
Outdated
| @@ -38,7 +38,7 @@ module HTTP | |||
| if body_type.prohibited? | |||
| body = nil | |||
| elsif content_length = content_length(headers) | |||
| if content_length != 0 | |||
| if content_length >= 0 | |||
There was a problem hiding this comment.
Regardless of the type, what would it even mean to have a negative length?
src/http/common.cr
Outdated
| @@ -38,7 +38,7 @@ module HTTP | |||
| if body_type.prohibited? | |||
| body = nil | |||
| elsif content_length = content_length(headers) | |||
| if content_length != 0 | |||
| if content_length >= 0 | |||
| # Don't create IO for Content-Length == 0 | |||
There was a problem hiding this comment.
This comment needs touchup too.
There was a problem hiding this comment.
This is a valid improvement, but we need to figure out if this is really the intended solution for #8461. See #8461 (comment)
There was a problem hiding this comment.
I think this is a valid fix. And is better to return an empty body_io rather than a nil.
I stumble on this while trying to log the responses from an api that sometimes return a response with content-length 0. In that scenario is hard to find it up front.
I suggest merging as is to keep the current api and improve the handling of content-length 0 responses.
|
Cool! I can't merge because of the requested changes, but if someone else can, please do. |
…rystal-lang#8503) * HTTP::Client: don't turn body_io into nil if content-length is zero * fixup! HTTP::Client: don't turn body_io into nil if content-length is zero
Fixes #8461