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

Validation Errors in 0.5.0 #428

Closed
mbleigh opened this issue Jun 15, 2013 · 6 comments
Closed

Validation Errors in 0.5.0 #428

mbleigh opened this issue Jun 15, 2013 · 6 comments
Labels

Comments

@mbleigh
Copy link
Contributor

mbleigh commented Jun 15, 2013

I'm just posting this to see if anyone else has run into this...I tried to upgrade Divshot to Grape 0.5.0 and started immediately seeing unexpected 400 errors for "missing" required parameters that were present in the request. Here's what I've determined so far:

  1. It only occurs when posting a request body that needs to be parsed (in my case, json)
  2. It seems to be caused by the validator params request taking place before the Formatter middleware has had a chance to parse the request body.
  3. I haven't been able to create a spec that repros it.

Is anyone else experiencing this issue? I'm still investigating.

@mbleigh
Copy link
Contributor Author

mbleigh commented Jun 15, 2013

Update: This can be solved by removing memoization in Grape::Request#params. What appears to be happening is params is called before the Formatter has run, then it's memoized so the value is frozen without the results of the parsed input.

@mbleigh
Copy link
Contributor Author

mbleigh commented Jun 15, 2013

Update: Tracked it down. By moving memoization to the request level, any Grape middleware that is added to the stack before the formatter has the potential to freeze the value. Commit coming momentarily.

@dblock
Copy link
Member

dblock commented Jun 18, 2013

Re: spec. What middleware are you inserting before the formatter? I tried to insert a dummy middleware before the formatter that would do something to the request, but that didn't produce the desired error. Is it possible that that middleware needs to be fixed instead of removing ||= here?

@mbleigh
Copy link
Contributor Author

mbleigh commented Jun 18, 2013

It was the Grape OAuth2 middleware (it has to be something that inherits from Grape::Middleware::Base). I'll try to contribute a minimal spec that breaks under the old code.

@dblock
Copy link
Member

dblock commented Nov 21, 2013

@mbleigh, I almost re-added this ||= again. I still don't understand why 3b1ca79 was necessary.

Maybe another way to solve this is to assign @env['grape.request.params'] and @env['grape.request.headers'] elsewhere, in a global middleware?

require 'grape/middleware/base'

module Grape
  module Middleware
    class Globals < Base
      def before
        @env['grape.request'] = Grape::Request.new(@env)
        @env['grape.request.headers'] = request.headers
        @env['grape.request.params'] = request.params if @env['rack.input']
      end
    end
  end
end

We could then try to stop creating Grape::Request in other places and rely on these to exist at all times?

@dblock
Copy link
Member

dblock commented Nov 24, 2013

@mbleigh I am pretty sure this is fixed via #512, because formatter will now use its own Rack::Request and therefore work independently. This was a chicken-and-egg problem in the OAuth middleware. I am going to put memoization back in at request level, see if your issue is back.

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

No branches or pull requests

2 participants