ActiveResource shouldn't rely on the presence of Content-Length #2678

Merged
merged 2 commits into from Sep 12, 2011

Conversation

Projects
None yet
3 participants
Contributor

jmileham commented Aug 24, 2011

Hi,

I recently ran into issues using ActiveResource against another Rails app running out of Pow. Rails doesn't use the ContentLength middleware by default when started from config.ru, so if the rack server (in Pow's case nack) doesn't add a Content-Length header on its own (which nack doesn't, but many servers do), ActiveResource ignores the response body of a save, preventing me from reading the server state (which in the case of creates means you lose track of the new object's ID).

The relevant part of the HTTP 1.1 spec says that the Content-Length header "SHOULD be sent whenever the message's length can be determined prior to being transferred," but SHOULD isn't MUST, and even if it were, it's conceivable that a custom JSON API might start streaming before it has finished counting bytes, so requiring a Content-Length header as evidence that a response body exists seems incorrect.

The current behavior came about in this commit from @malyk, associated with lighthouse ticket #5038. The test case from that thread provided by @neerajdotname still passes, so this patch doesn't alter any tested behavior, and presumably satisfies #5038.

This patch also provides better resilience by ignoring the body of an HTTP response that isn't allowed to have one as another way of preventing parse errors if the server sends a garbage body along with a 204 response.

#5038 didn't make it into 3.0, so this bug only manifests on 3-1-stable and onward. I'd be happy to provide a pull request for 3-1-stable as well.

Contributor

jmileham commented Sep 9, 2011

@kaiwren @jnicklas - this patch fixes the issue reported on rails-core this AM. I replied on rails-core but the post hasn't been approved yet.

Member

jonleighton commented Sep 9, 2011

Thanks for the pull request.

I realise you're just copying format of the existing tests, but calling a private method via send is quite brittle. Could you look into stubbing the call to connection.post or connection.put instead?

Contributor

jmileham commented Sep 9, 2011

@jonleighton will do. IIRC I took a crack at this and things looked a bit nasty (and thought that since I was just copying existing style, as you say, I could get away with it), but I'll give it another go.

Owner

jmileham commented on f776661 Sep 9, 2011

@jonleighton - is this more of what you were looking for? If so should I force push this version to #2678 or open a new pull request? If not, any other notes?

Contributor

jmileham commented Sep 10, 2011

force pushed revised tests to keep the conversation together.

+ def test_not_persisted_with_body_and_zero_content_length
+ resp = ActiveResource::Response.new(@rick)
+ resp['Content-Length'] = "0"
+ Person.connection.expects(:post).returns(resp)
@jonleighton

jonleighton Sep 10, 2011

Member

great, yeah this was what i'm after, but use stubs(:post) rather than expects because we are not testing whether Post.connection.post gets called in this test, just stubbing out the response. (also, force pushing is good, keeps the conversation in one place)

@jmileham

jmileham Sep 11, 2011

Contributor

I had originally written it to use stubs() but I was concerned that since we're only asserting a boolean in each test, if somebody changes the implementation of ARes.create not to ultimatelly call Ares.connection.post, these tests will no longer be exercising the expected logic, and only some of them will fail, leaving effectively no-op tests behind that would no longer protect against a regression. Using expects should ensure that the tests get refactored if/when they need to, but shouldn't be more brittle than necessary. What do you think?

@jonleighton

jonleighton Sep 12, 2011

Member

But if ARes.create was changed to not call Ares.connection.post, at least one of these tests would end up failing, surely?

@jmileham

jmileham Sep 12, 2011

Contributor

At least one, yup. Just not all. I just didn't want to put the onus on somebody working on something else to find adjacent passing tests and fix them as well.

@jonleighton

jonleighton Sep 12, 2011

Member

I guess. Alright then, I'll merge.

+ else
+ true
+ end
+ end
@dasch

dasch Sep 10, 2011

Contributor

How about response['Content-Length'].nil? || response['Content-Length'] != "0"?

@jmileham

jmileham Sep 11, 2011

Contributor

Good point -- not the tidiest code ever. I'll probably throw this back into the calling method actually.

- if !response['Content-Length'].blank? && response['Content-Length'] != "0" && !response.body.nil? && response.body.strip.size > 0
+ if (response_code_allows_body?(response.code) &&
+ (response['Content-Length'].nil? || response['Content-Length'] != "0") &&
+ !response.body.nil? && response.body.strip.size > 0)
@jonleighton

jonleighton Sep 12, 2011

Member

Hey, one more thing... how about having a body_present? method, like this:

def body_present?(response)
  !(100..199).include?(response.code) && ![204,304].include?(response.code) &&
  (response['Content-Length'].nil? || response['Content-Length'] != "0") &&
  response.body &&
  response.body.strip.size > 0
end

I am going to commit now anyway so we can get this fix in before the 3.1.1 RC, but if you fancy refactoring in master in a new PR you are welcome to...

@jmileham

jmileham Sep 12, 2011

Contributor

Yeah, this does seem like the right way to go. I was edgy about how long that line was but didn't want to throw away the lazy evaluation win that &&'ing it all together brought. I'll send you a pull request this week. Thanks a lot!

@jonleighton

jonleighton Sep 12, 2011

Member

In fact, you could look at putting the body_present? method inside the ActiveResource::Response and then just call if response.body_present?.

@jmileham

jmileham Sep 12, 2011

Contributor

Yeah, sounds great.

jonleighton added a commit that referenced this pull request Sep 12, 2011

Merge pull request #2678 from jmileham/ares_content_length_bug
ActiveResource shouldn't rely on the presence of Content-Length

@jonleighton jonleighton merged commit 8397a56 into rails:master Sep 12, 2011

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