Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Use equal? to compare form_input to rack.input #588

Merged
merged 1 commit into from Aug 22, 2013

Conversation

Projects
None yet
9 participants
Contributor

statianzo commented Jul 18, 2013

I was having issues with rack-contrib's PostBodyContentTypeParser when input was large enough to cause Puma and Thin to use Tempfiles as input. POST was not returning env["rack.request_form_hash"] as expected

I tracked the issue down to Rack::Request#POST's usage of eql?. It doesn't work for comparing tempfiles in 1.9 or 2.0. Changing it to equal? worked consistently on 1.8 and 1.9+ .

In 1.8, eql? compares Tempfiles correctly
In 1.9+, t.eql?(t) always returns false

In 1.8, == will change the position of the Tempfile.
In 1.9+, == compares Tempfiles correctly.

Using equal? provides consistent results of equality between 1.8, 1.9,
and 2.0 when comparing Tempfile objects.

@statianzo statianzo Use equal? to compare form_input to rack.input
Using equal? provides consistent results of equality between 1.8, 1.9,
and 2.0 when comparing Tempfile objects.

In 1.8, == will change the position of the Tempfile.
In 1.9+, == compares Tempfiles correctly.

In 1.8, eql? compares Tempfiles correctly
In 1.9+, t.eql?(t) always returns false
b059307
Contributor

statianzo commented Aug 22, 2013

Is there anything needed to fix or help clarify this pull request?

@spastorino spastorino added a commit that referenced this pull request Aug 22, 2013

@spastorino spastorino Merge pull request #588 from statianzo/tempfile-form-input
Use equal? to compare form_input to rack.input
bbae0ce

@spastorino spastorino merged commit bbae0ce into rack:master Aug 22, 2013

1 check passed

default The Travis CI build passed
Details

gucki commented on b059307 Jan 21, 2014

Please release 1.5.4 with this fixed. Withou this fix, the request body for all requests which use a temp file (all requests larger 128 kb?) fails to be properly parsed. One project hit by this bug was our gitlab ci installation for example. Pinning rack to master in the Gemfile does not work, as rails depends on 1.5.x but master has version 1.6.0.alpha.

+1 on an updated release to fix this

+1 on an updated release to fix this

Member

spastorino replied Feb 23, 2014

I think we should backport this fix to rack-1.5 /cc @raggi

@spastorino please just merge the backport #756

Thanks for this patch! Took me a while to figure out why I lose all my large POST's payload. I see that the backport was merged. Is it possible to get a new 1.5.x version that includes this patch?

@spastorino any word on when and if this fix will make it to rack-1.5?

Member

spastorino commented May 30, 2014

@raggi is the one releasing, will let him know ...

Owner

raggi commented May 30, 2014

sorry guys, i've done a ~200 hour sprint in the last 3 weeks, I need a break. I know a release is loooooong overdue, and I will get to that as soon as I possibly can do so without breaking myself. At this point there's about 10-15 hours of PRs, prep and backports to do.

Contributor

statianzo commented Aug 4, 2014

@raggi Any updates on a release date? Anything I can do to help you out?

👍

@dblock dblock added a commit to ruby-grape/grape that referenced this pull request May 7, 2015

@dblock dblock Rack 1.5.3 back-ported rack/rack#588 via rack/rack#756. 7f79d36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment