Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

params 'missing' due to rack bug #559

Closed
myitcv opened this issue Jan 27, 2014 · 7 comments
Closed

params 'missing' due to rack bug #559

myitcv opened this issue Jan 27, 2014 · 7 comments

Comments

@myitcv
Copy link
Contributor

myitcv commented Jan 27, 2014

This commit in rack fixes an issue whereby all requests which use a temp file (all requests larger 128 kb?) fail to be properly parsed.

The issue manifests itself in grape in the following way:

params do
  optional :picture, type: String
end
patch '/' do
  picture_from_params = params[:picture] # nil - but should not be
  picture_from_env = env['rack.request.form_hash']['picture'] # not nil - as expected

  env["rack.request.form_input"].eql? env["rack.input"] # false - THIS IS THE BUG fixed by the commit in rack
  env["rack.request.form_input"].equal? env["rack.input"] # true
end

Indeed this could have been the root cause of a few issues raised in the Google Group.

Not so much an issue from grape's perspective apart from it being the place where this error manifests itself. Definitely something to be aware of when running Ruby 1.9+ (commit comment suggests this doesn't affect Ruby 1.8.x)

So more of an FYI in case anyone else gets bitten by this.

The issue is further complicated by the fact that running rack/test doesn't cross this codepath. Presumably because temp files are not used in tests??

This issue is present in rack == 1.5.2, fixed in master as of the time of this post, but not yet released (expected in 1.5.4 I believe)

@dblock
Copy link
Member

dblock commented Jan 27, 2014

I think it would be good to have a test in Grape that breaks right now, then we can lock Rack >= 1.5.4 and merge the test.

@myitcv
Copy link
Contributor Author

myitcv commented Jan 29, 2014

Please see #563 for a first cut at such a test

@myitcv myitcv closed this as completed Jan 29, 2014
@myitcv myitcv reopened this Jan 29, 2014
@myitcv
Copy link
Contributor Author

myitcv commented Jan 29, 2014

Sorry, I'll leave you to close this if you think #563 covers this.

@zuk
Copy link

zuk commented Feb 24, 2014

I just spent half a day trying to track this bug down... finally figured it out, came here to report, and found this already here.

Using the github#master version of rack (i.e. rack 1.6.0.alpha) seems to fix the problem by the way.

@olleolleolle
Copy link
Contributor

@zuk rack 1.6.0.beta got released back in August. Your tip helped me a lot.

@dblock dblock closed this as completed in c72181d Jan 2, 2015
@dblock
Copy link
Member

dblock commented Jan 2, 2015

Grape doesn't lock to a particular Rack version, so now that Rack 1.6.0 has shipped, this works. I committed the test with c72181d.

@dblock
Copy link
Member

dblock commented May 7, 2015

Note that rack/rack#756 back-ported the fix into Rack 1.5.3.

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

No branches or pull requests

4 participants