Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

ActiveResource::Base#create data is not loaded if the transfer-encoding is chunked #2079

Closed
wants to merge 1 commit into from

4 participants

@jkrall

Fixing bug in ActiveResource::Base#create where returned data is not loaded if the transfer-encoding is chunked. Includes tests & all ActiveResource tests pass.

Commit 154081f created this bug, with the goal of fixing 204 responses. There were no tests with this patch, so I added new tests to cover this & my own issue. Refactored the load_attributes_from_response method to make the logic around when to load attributes more obvious.

@jkrall jkrall Fixing bug in AR::Base#create where returned data is not loaded if th…
…e transfer-encoding is chunked. Includes tests & all ActiveResource tests pass.
00920bb
@sporkd

Thank you! Thank you! I just ran into this sam issue today as well. Hard to debug too because it was only breaking against our production API.

@jreidinger jreidinger commented on the diff
activeresource/lib/active_resource/base.rb
@@ -1357,7 +1357,11 @@ module ActiveResource
end
def load_attributes_from_response(response)
- if !response['Content-Length'].blank? && response['Content-Length'] != "0" && !response.body.nil? && response.body.strip.size > 0
+ has_content_length = !response['Content-Length'].blank? && response['Content-Length'] != "0"
+ has_body = !response.body.nil? && response.body.strip.size > 0
+ is_chunked = response["Transfer-Encoding"] == "chunked"
+
+ if has_body && (has_content_length || is_chunked)

is_chunked is not neccessary as net response object has method chunked?
so it should look like
if has_body && (has_content_length || response.chunked?)

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

Please include this fix as it is really annoying that one rails server which produce chunked output cannot serve to another one which read it.

@carlosantoniodasilva

ActiveResource has been moved out from Rails to its own repo. If this is still considered an issue, please reopen the pull request on ARes repo with updated code. I'm closing it here, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 15, 2011
  1. @jkrall

    Fixing bug in AR::Base#create where returned data is not loaded if th…

    jkrall authored
    …e transfer-encoding is chunked. Includes tests & all ActiveResource tests pass.
This page is out of date. Refresh to see the latest.
View
6 activeresource/lib/active_resource/base.rb
@@ -1357,7 +1357,11 @@ def create
end
def load_attributes_from_response(response)
- if !response['Content-Length'].blank? && response['Content-Length'] != "0" && !response.body.nil? && response.body.strip.size > 0
+ has_content_length = !response['Content-Length'].blank? && response['Content-Length'] != "0"
+ has_body = !response.body.nil? && response.body.strip.size > 0
+ is_chunked = response["Transfer-Encoding"] == "chunked"
+
+ if has_body && (has_content_length || is_chunked)

is_chunked is not neccessary as net response object has method chunked?
so it should look like
if has_body && (has_content_length || response.chunked?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
load(self.class.format.decode(response.body), true)
@persisted = true
end
View
38 activeresource/test/cases/base_test.rb
@@ -711,6 +711,44 @@ def test_create_without_location
assert_nil person.id
end
+ def test_create_with_response
+ ActiveResource::HttpMock.respond_to do |mock|
+ mock.post "/people.json", {}, Person.new(:name=>'Rick', :other=>'stuff').to_json
+ end
+ person = Person.create(:name => 'Rick')
+ assert_equal 'Rick', person.name
+ assert_equal 'stuff', person.other
+ end
+
+ def test_create_with_204_response
+ person_json = Person.new(:name=>'Rick', :other=>'stuff').to_json
+ create_request = ActiveResource::Request.new(:post, '/people.json', person_json, {})
+ create_response = ActiveResource::Response.new('', 204, {})
+ create_response.headers.delete('Content-Length')
+ ActiveResource::HttpMock.respond_to({create_request => create_response})
+
+ person = Person.create(:name => 'Rick')
+ assert_equal 'Rick', person.name
+ assert_equal false, person.persisted?
+ end
+
+ def test_create_with_chunked_response
+ person_json = Person.new(:name=>'Rick', :other=>'stuff').to_json
+ create_request = ActiveResource::Request.new(:post, '/people.json', person_json, {})
+
+ # Simulate chunked transfer
+ create_response = ActiveResource::Response.new(person_json, 200, {})
+ create_response["Transfer-Encoding"] = "chunked"
+ create_response.headers.delete('Content-Length')
+
+ ActiveResource::HttpMock.respond_to({create_request => create_response})
+
+ person = Person.create(:name => 'Rick')
+ assert_equal 'Rick', person.name
+ assert_equal 'stuff', person.other
+ end
+
+
def test_clone
matz = Person.find(1)
matz_c = matz.clone
Something went wrong with that request. Please try again.