Skip to content
This repository

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

Closed
wants to merge 1 commit into from

4 participants

Joshua Krall Peter Gumeson Josef Reidinger Carlos Antonio da Silva
Joshua Krall
jkrall commented July 14, 2011

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.

Joshua Krall 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
Peter Gumeson
sporkd commented July 15, 2011

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.

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

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
Josef Reidinger

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.

Carlos Antonio da Silva

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!

Carlos Antonio da Silva carlosantoniodasilva closed this April 28, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Jul 15, 2011
Joshua Krall 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
This page is out of date. Refresh to see the latest.
6  activeresource/lib/active_resource/base.rb
@@ -1357,7 +1357,11 @@ def create
1357 1357
       end
1358 1358
 
1359 1359
       def load_attributes_from_response(response)
1360  
-        if !response['Content-Length'].blank? && response['Content-Length'] != "0" && !response.body.nil? && response.body.strip.size > 0
  1360
+        has_content_length = !response['Content-Length'].blank? && response['Content-Length'] != "0"
  1361
+        has_body = !response.body.nil? && response.body.strip.size > 0
  1362
+        is_chunked = response["Transfer-Encoding"] == "chunked"
  1363
+
  1364
+        if has_body && (has_content_length || is_chunked)
1361 1365
           load(self.class.format.decode(response.body), true)
1362 1366
           @persisted = true
1363 1367
         end
38  activeresource/test/cases/base_test.rb
@@ -711,6 +711,44 @@ def test_create_without_location
711 711
     assert_nil person.id
712 712
   end
713 713
 
  714
+  def test_create_with_response
  715
+    ActiveResource::HttpMock.respond_to do |mock|
  716
+      mock.post   "/people.json", {}, Person.new(:name=>'Rick', :other=>'stuff').to_json
  717
+    end
  718
+    person = Person.create(:name => 'Rick')
  719
+    assert_equal 'Rick', person.name
  720
+    assert_equal 'stuff', person.other
  721
+  end
  722
+
  723
+  def test_create_with_204_response
  724
+    person_json = Person.new(:name=>'Rick', :other=>'stuff').to_json
  725
+    create_request  = ActiveResource::Request.new(:post, '/people.json', person_json, {})
  726
+    create_response = ActiveResource::Response.new('', 204, {})
  727
+    create_response.headers.delete('Content-Length')
  728
+    ActiveResource::HttpMock.respond_to({create_request => create_response})
  729
+
  730
+    person = Person.create(:name => 'Rick')
  731
+    assert_equal 'Rick', person.name
  732
+    assert_equal false, person.persisted?
  733
+  end
  734
+
  735
+  def test_create_with_chunked_response
  736
+    person_json = Person.new(:name=>'Rick', :other=>'stuff').to_json
  737
+    create_request  = ActiveResource::Request.new(:post, '/people.json', person_json, {})
  738
+
  739
+    # Simulate chunked transfer
  740
+    create_response = ActiveResource::Response.new(person_json, 200, {})
  741
+    create_response["Transfer-Encoding"] = "chunked"
  742
+    create_response.headers.delete('Content-Length')
  743
+
  744
+    ActiveResource::HttpMock.respond_to({create_request => create_response})
  745
+
  746
+    person = Person.create(:name => 'Rick')
  747
+    assert_equal 'Rick', person.name
  748
+    assert_equal 'stuff', person.other
  749
+  end
  750
+
  751
+
714 752
   def test_clone
715 753
     matz = Person.find(1)
716 754
     matz_c = matz.clone
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.