Request Parameters

177 views
Skip to first unread message

Daniel N

unread,
Jul 13, 2009, 9:34:59 PM7/13/09
to rack-...@googlegroups.com
Hi All,

I've been thinking about request parameters a bit lately and there are currently some issues that I've identified with the way that they're implemented, parsing schemes aside.

In Rails + Merb + ?? there are custom request objects.  That is, they don't use Rack::Request.new to setup their requets objects.  Most portable middleware I've seen however does.

1. Each request object parses the parameters resulting in a doubling of effort
2. A middleware author must know which type of request object is being used in order to manipulate the params hash.

For middleware like http://github.com/mutle/rack-uploads which rely on manipulating the request params hash, this could present issues when writing the middleware to be portable.  Checks for request object type would potentially fracture effort, and add unnecessary complexity to middlware developers who need to manipulate the params hash.

What I'd like to propose is that we setup the params hash of Rack::Request to be in env['rack.params'] which could then be shared across request object inplementation.  Rails, Merb, <insert framework here> can then make use of custom requirements, and prevent double parsing while allowing middleware to inpsect / inject keys and values.

The params hash should be hash like, and support accessing all keys via a string of the key name.  (ie HashWithIndifferentAccess or Mash would work too)

Any thoughts on this?

Cheers
Daniel

Luke Matthew Sutton

unread,
Jul 13, 2009, 9:47:02 PM7/13/09
to Rack Development
+1 for this idea.

I can foresee many cases where it would be useful to have middleware
inject or modify values in the params -- for example, injecting locale
as a param -- but as you point out, the differing implementations
across frameworks makes it difficult to build something generic.

On Jul 14, 10:34 am, Daniel N <has....@gmail.com> wrote:
> Hi All,
>
> I've been thinking about request parameters a bit lately and there are
> currently some issues that I've identified with the way that they're
> implemented, parsing schemes aside.
>
> In Rails + Merb + ?? there are custom request objects.  That is, they don't
> use Rack::Request.new to setup their requets objects.  Most portable
> middleware I've seen however does.
>
> 1. Each request object parses the parameters resulting in a doubling of
> effort
> 2. A middleware author must know which type of request object is being used
> in order to manipulate the params hash.
>
> For middleware likehttp://github.com/mutle/rack-uploadswhich rely on

Joshua Peek

unread,
Jul 13, 2009, 9:48:43 PM7/13/09
to rack-...@googlegroups.com
On Mon, Jul 13, 2009 at 8:34 PM, Daniel N<has...@gmail.com> wrote:
> In Rails + Merb + ?? there are custom request objects.  That is, they don't
> use Rack::Request.new to setup their requets objects.  Most portable
> middleware I've seen however does.

Yes Rails does. Calling request.params in Rails offloads Rack::Request
to parse the params and Rack::Request sticks its cache in
rack.request.form_hash.

> 1. Each request object parses the parameters resulting in a doubling of
> effort
> 2. A middleware author must know which type of request object is being used
> in order to manipulate the params hash.

These are not problems if your custom request object inherits from
Rack::Request.

> What I'd like to propose is that we setup the params hash of Rack::Request
> to be in env['rack.params'] which could then be shared across request object
> inplementation.  Rails, Merb, <insert framework here> can then make use of
> custom requirements, and prevent double parsing while allowing middleware to
> inpsect / inject keys and values.

The convention atm is to stuff your params in the rack request cache
"rack.request.form_hash". I agree, we should convert this from a
generic cache to a "spec" similar to rack.session. "rack.params"
sounds good too.

References:

http://github.com/rack/rack-contrib/blob/cfee6eefe8291d0aa3c70a12b3b2cc350ec09beb/lib/rack/contrib/nested_params.rb
http://github.com/rack/rack-contrib/blob/cfee6eefe8291d0aa3c70a12b3b2cc350ec09beb/lib/rack/contrib/post_body_content_type_parser.rb
http://github.com/rack/rack-contrib/blob/cfee6eefe8291d0aa3c70a12b3b2cc350ec09beb/lib/rack/contrib/post_body_content_type_parser.rb
http://github.com/rails/rails/blob/01d92021e69f54def1ec8103b2b99f907dd88ec4/actionpack/lib/action_dispatch/middleware/params_parser.rb

> The params hash should be hash like, and support accessing all keys via a
> string of the key name.  (ie HashWithIndifferentAccess or Mash would work
> too)

I don't think we should require any special type of data structure.
Even better, just require that whatever object is in
env['rack.params'] respond to [] and []=.

--
Joshua Peek

Yehuda Katz

unread,
Jul 13, 2009, 10:00:02 PM7/13/09
to rack-...@googlegroups.com
+1 on your proposal. Question: why not use the same structure we used for session? I think the only added things were key? and delete (not sure though).

-- Yehuda
--
Yehuda Katz
Developer | Engine Yard
(ph) 718.877.1325

Daniel N

unread,
Jul 13, 2009, 10:03:52 PM7/13/09
to rack-...@googlegroups.com

Hi Josh,

Thanks for correcting my understanding.  I've actually been bitten by this with rails when trying to set the action parameter in rails_warden after an authentication failure. 

http://github.com/hassox/rails_warden/blob/90d5ce583a238ff3b4364a4a4835cb17a86d9a7e/lib/rails_warden.rb#L10

When I used the standard request object I could not manipulate the params hash and have rails see it, and after looking at what you have it makes sense why.  Rails transfers the params hash over to
env["action_dispatch.request.request_parameters"]
Which means that on the way back up the stack the params hash is inaccessible to normal Rack::Request objects. 

The reason I suggest that we have a standard interface to them is so that items are all available as strings.  This would prevent the guessing of should I use a symbol or a string to get hold of this? type questions.  By always allowing for access to keys via string there is potentially more portablitiy I think.

Cheers
Daniel





 

Daniel N

unread,
Jul 13, 2009, 10:08:26 PM7/13/09
to rack-...@googlegroups.com
On Tue, Jul 14, 2009 at 12:00 PM, Yehuda Katz <wyc...@gmail.com> wrote:
+1 on your proposal. Question: why not use the same structure we used for session? I think the only added things were key? and delete (not sure though).

-- Yehuda

My thinking is that in middleware as a least surprise idea, we should _always_ be able to access keys via the string of the name.

Daniel
 

Christian Neukirchen

unread,
Jul 14, 2009, 3:13:18 PM7/14/09
to rack-...@googlegroups.com
On Tue, Jul 14, 2009 at 4:00 AM, Yehuda Katz<wyc...@gmail.com> wrote:
> +1 on your proposal. Question: why not use the same structure we used for
> session? I think the only added things were key? and delete (not sure
> though).
> -- Yehuda

Sounds good to me.

--
Christian Neukirchen <chneuk...@gmail.com> http://chneukirchen.org

Joshua Peek

unread,
Jul 14, 2009, 3:31:09 PM7/14/09
to rack-...@googlegroups.com
On Tue, Jul 14, 2009 at 2:13 PM, Christian
Neukirchen<chneuk...@gmail.com> wrote:
> Sounds good to me.

Great.

Daniel, do you want to write up the patch? We'll need to update Lint
to enforce the SPEC then update any other middleware appropriately.

I'll take it from there.

--
Joshua Peek

Daniel N

unread,
Jul 15, 2009, 2:40:01 AM7/15/09
to rack-...@googlegroups.com

Sure thing.. I'll get a patch sorted shortly.  Is the best way to attach it here or do it as a pull request on github?

Daniel

Daniel N

unread,
Jul 15, 2009, 5:24:22 AM7/15/09
to rack-...@googlegroups.com

I just need to check with you guys on something.  At the moment the two hashes, get and post parameters, are stored in the env seperately.  Is this something that should keep happening?  I don't think it would be robust if we store the params into rack.params if they're also going to be stored seperately unless there's a dual setup where rack.params is merged with any new params.

For example, http://github.com/rack/rack-contrib/blob/cfee6eefe8291d0aa3c70a12b3b2cc350ec09beb/lib/rack/contrib/post_body_content_type_parser.rb

If there were a merged params hash, the post_body_content_type_parser would need to maintain the  rack.request.form_hash key and also the rack.params hash. 

So is it safe to boil them both up together and just have rack.params or rack.request.params? Or should we maintain the get and post params seperately, and combine them on first access in rack.request.params.  Then for cases like the post_body_content_type_parser, it could update the rack.request.form_hash and then merge those results into the params hash.

Cheers
Daniel

Joshua Peek

unread,
Jul 15, 2009, 12:56:52 PM7/15/09
to rack-...@googlegroups.com
On Wed, Jul 15, 2009 at 4:24 AM, Daniel N<has...@gmail.com> wrote:
> So is it safe to boil them both up together and just have rack.params or
> rack.request.params? Or should we maintain the get and post params
> seperately, and combine them on first access in rack.request.params.  Then
> for cases like the post_body_content_type_parser, it could update the
> rack.request.form_hash and then merge those results into the params hash.

I'd say merge them. But I'm curious if there is any opposition.


--
Joshua Peek

Luke Matthew Sutton

unread,
Jul 15, 2009, 7:31:56 PM7/15/09
to Rack Development
I can't think of a situation where you would want to modify params
from a particular source. Most apps don't care, they just want the
params, so I think merge em.

On Jul 16, 1:56 am, Joshua Peek <j...@joshpeek.com> wrote:

Daniel N

unread,
Jul 15, 2009, 7:50:20 PM7/15/09
to rack-...@googlegroups.com
Cool,  I'll create the patch for both GET and POST parameters to be stored in 'rack.request.params'. 

Cheers
Daniel

deepak kannan

unread,
Jul 16, 2009, 2:28:38 AM7/16/09
to rack-...@googlegroups.com
We should merge the get and post params.
But, can we get the raw body anywhere and a public api to parse it.  My point being, it should be easy to extend.

Deepak Kannan

Joshua Peek

unread,
Aug 3, 2009, 12:25:12 PM8/3/09
to Rack Development
Daniel, I created a lighthouse ticket for this. Please post your patch
there and we'll get it in.

http://rack.lighthouseapp.com/projects/22435-rack/tickets/62-request-parameters-spec

Daniel N

unread,
Aug 4, 2009, 6:54:11 PM8/4/09
to rack-...@googlegroups.com
Hi Josh,

Thanks.  I have been a little busy with a new job but things are starting to settle a little.  I should have the patch shortly.

Cheers
Daniel
Reply all
Reply to author
Forward
0 new messages