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

Middleware to parse Rails style url parameters #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Middleware to parse Rails style url parameters #5

wants to merge 2 commits into from

Conversation

rsslldnphy
Copy link

Hi, I've created a piece of middleware to parse Rails style URL params. It accepts a URL pattern in the same format as a rails routes file and parses out the params, adding them to the env['params'] hash.

So for example with this in your API:

use Goliath::Contrib::PathParams, '/kittens/:name/:favourite_colour'

a request path of '/kittens/jeremy/mauve' would lead to a params hash containing:

{ 'name' => 'jeremy', 'favourite_colour' => 'mauve' }


def matched_params(env)
request_path(env).match(regexp).tap do |matched|
raise Goliath::Validation::BadRequestError, "Request path does not match expected pattern: #{request_path(env)}" if matched.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why this has to be exclusive? As in, currently if you include this middleware then you can't use any other type of route.

@rsslldnphy
Copy link
Author

Hi Ilya, I'm not 100% sure what you mean by type of route - do you mean if you are serving multiple routes from a single goliath instance? We are only serving a single route per Goliath instance (as suggested in the google group thread on removing routing for 1.0). I guess it would be possible to make this middleware work for a number of different routes but I think if you are serving multiple routes from a single instance this functionality would belong better in the code that's doing the routing, unless I've misunderstood you?

@igrigorik
Copy link
Member

Well, the general recommendation is to have logically separated services, instead of a kitchen-sink controller. Having said that, a single web service can respond to multiple URL patterns. Ex:

/api/param1/param2
/api/private/param1/param2

There is no reason why the above shouldn't be served by the same API. We shouldn't have code that forces a single URL, as that will also break support for optional parameters amongst other things.

It seems like the simplest approach is to remove the raise: if pattern matches, great, if not, just pass it through. Perhaps, as a config flag, you can make it raise.

@rsslldnphy
Copy link
Author

OK, that makes sense, ta. I'll remove the raise.

I'd be reluctant to make it a config option as it would only work if you were using one, not multiple instances of the middleware, and it seems a bit off to have a config option that only works in some circumstances. The only other option I can think of is if a single instance of the middleware takes all possible paths as a parameter and then raises if none of them match, perhaps in a block something like this:

use Goliath::Contrib::PathParams do
  path '/api/:param1/:param2'
  path '/api/private/:param1/:param2'
end

where path is just a method that adds its argument to an array of url patterns to try.

But then it feels like I'm starting to implement more and more of the functionality of a router so maybe that's a bad idea?

@igrigorik
Copy link
Member

Yeah, it's a slippery slope. Personally, I like the multiple path idea. As long as we don't make any promises about dispatch of these requests.. only parsing of the parameters, I think we're in good shape.

@karlfreeman
Copy link

Just stumbled on this, would love to see this get merged in as this would be a perfect contrib library for those migrating from 0.9.4 to 1.0.0 ( eg routerless ) 👍

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

Successfully merging this pull request may close these issues.

None yet

3 participants