Return nil when the body is stubbed as '' or nil. #32

Merged
merged 1 commit into from May 15, 2013

Conversation

2 participants
@myronmarston
Contributor

myronmarston commented Jun 12, 2012

When a real Net::HTTP request is made and receives an
empty-body response (such as a 204 No Content), then the
response body is nil.

This gist demonstrates the behavior:
https://gist.github.com/2918173

Return nil when the body is stubbed as '' or nil.
When a real Net::HTTP request is made and receives an
empty-body response (such as a 204 No Content), then the
response body is nil.

This gist demonstrates the behavior:
https://gist.github.com/2918173

chrisk added a commit that referenced this pull request May 15, 2013

Merge branch myronmarston/nil_response_body, closing #32
* myronmarston/nil_response_body:
  Return nil when the body is stubbed as '' or nil.

Conflicts:
	test/test_fake_web.rb

@chrisk chrisk merged commit 6b2075f into chrisk:master May 15, 2013

@chrisk

This comment has been minimized.

Show comment Hide comment
@chrisk

chrisk Dec 5, 2013

Owner

@myronmarston You know, this behavior actually isn't quite right. #body sometimes returns nil and sometimes returns ""—it depends on Net::HTTPResponse::HAS_BODY, which is different for the various subclasses. All the responses in your gist were Net::HTTPNoContent, which has HAS_BODY = false. Here's the relevant part of Net::HTTPResponse where the body is set to whatever comes out of the socket or nil:

def read_body(dest = nil, &block)
  if @read
    raise IOError, "#{self.class}\#read_body called twice" if dest or block
    return @body
  end
  to = procdest(dest, block)
  stream_check
  if @body_exist
    read_body_0 to
    @body = to
  else
    @body = nil
  end
  @read = true

  @body
end

(above that, @body_exist gets set to reqmethodallowbody && self.class.body_permitted? — so it also depends on the request method. That is, when you issue a HEAD request and get an empty 200 response, Net::HTTPOK#body will be nil even though Net::HTTPOK::HAS_BODY = true...)

I'll see if I can put together a patch to do the same thing for fake responses.

Owner

chrisk commented Dec 5, 2013

@myronmarston You know, this behavior actually isn't quite right. #body sometimes returns nil and sometimes returns ""—it depends on Net::HTTPResponse::HAS_BODY, which is different for the various subclasses. All the responses in your gist were Net::HTTPNoContent, which has HAS_BODY = false. Here's the relevant part of Net::HTTPResponse where the body is set to whatever comes out of the socket or nil:

def read_body(dest = nil, &block)
  if @read
    raise IOError, "#{self.class}\#read_body called twice" if dest or block
    return @body
  end
  to = procdest(dest, block)
  stream_check
  if @body_exist
    read_body_0 to
    @body = to
  else
    @body = nil
  end
  @read = true

  @body
end

(above that, @body_exist gets set to reqmethodallowbody && self.class.body_permitted? — so it also depends on the request method. That is, when you issue a HEAD request and get an empty 200 response, Net::HTTPOK#body will be nil even though Net::HTTPOK::HAS_BODY = true...)

I'll see if I can put together a patch to do the same thing for fake responses.

@chrisk

This comment has been minimized.

Show comment Hide comment
@chrisk

chrisk Dec 5, 2013

Owner

Hrm, yeah, fake responses are really too permissive re: allowing a body compared to Net::HTTP's behavior... for example,

http = Net::HTTP.new("example.com", 80)

http.request(Net::HTTP::Head.new("/")).body  # => nil

FakeWeb.register_uri(:head, "http://example.com", :body => "example")
http.request(Net::HTTP::Head.new("/")).body  # => "example"

At the least, we should probably issue a warning when you register a response body for a HEAD request, or provide both a body and e.g. :status => [204, "No Content"] in the response options...

Owner

chrisk commented Dec 5, 2013

Hrm, yeah, fake responses are really too permissive re: allowing a body compared to Net::HTTP's behavior... for example,

http = Net::HTTP.new("example.com", 80)

http.request(Net::HTTP::Head.new("/")).body  # => nil

FakeWeb.register_uri(:head, "http://example.com", :body => "example")
http.request(Net::HTTP::Head.new("/")).body  # => "example"

At the least, we should probably issue a warning when you register a response body for a HEAD request, or provide both a body and e.g. :status => [204, "No Content"] in the response options...

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Dec 5, 2013

Contributor

@chrisk -- interesting. I didn't realize that Net::HTTP would return nil sometimes, '', sometimes. I agree it would be good to make FakeWeb match that behavior.

Contributor

myronmarston commented Dec 5, 2013

@chrisk -- interesting. I didn't realize that Net::HTTP would return nil sometimes, '', sometimes. I agree it would be good to make FakeWeb match that behavior.

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