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

Path based versioning not recognizing '/' route #86

Closed
jch opened this issue Dec 1, 2011 · 6 comments
Closed

Path based versioning not recognizing '/' route #86

jch opened this issue Dec 1, 2011 · 6 comments

Comments

@jch
Copy link
Contributor

jch commented Dec 1, 2011

Path based versioning works for all routes whose paths are not the root path '/'

require 'grape'
require 'rack-test'

class API < Grape::API
  version 'v1', using: :path
  # this works fine if you change it to /hello
  get '/' do
    "I am version 1"
  end
end

include Rack::Test::Methods
def app
  API
end

get '/v1/'
# prints not found
puts "/ with path version v1: " + last_response.body
puts

get '/v1'
# also prints not found
puts "/ with path version v1: " + last_response.body
puts
jch pushed a commit that referenced this issue Dec 1, 2011
@ghost ghost assigned jch Dec 3, 2011
@gerhard
Copy link

gerhard commented May 23, 2012

Is anyone else bothered about this? Worth a patch?

@jch
Copy link
Contributor Author

jch commented May 23, 2012

@gerhard definitely worth a patch, just haven't found time to fix it.

@gerhard
Copy link

gerhard commented May 23, 2012

@jch will see if I can make some time for this. This is what I'm currently using (as a workaround):

    # This should have been the default route, but grape requires a fix for this:
    # https://github.com/intridea/grape/issues/86
    desc 'The current page, showing details about all available routes'
    get '/routes' do
      API.routes.map { |route| route.instance_variable_get(:@options) }
    end

@jch
Copy link
Contributor Author

jch commented May 23, 2012

@gerhard thanks, feel free to ping me if you need any explanation of the code. My fix in the project was to not use a top level route and always scope things underneath a resource name:

class API < Grape::API
  resources :things do
    # routes /things fine
    get do
    end
  end
end

@walski
Copy link
Contributor

walski commented Nov 14, 2012

Hey guys, I'm running into the same issue. After some investigating it seems all to boil down to a mechanism in rack-mount which normalizes the path of each request before matching it against the route set. Anyway, if you do that for a path versioned call you end up with a call to /v1/ actually being a call to /v1 while the grape route matches on:

/\A\/(?<version>v1)\/(?:\.(?<format>[^\/.?]+))?\Z/

which then does not match anymore. I suggest just changing the matcher to be like

/\A\/(?<version>v1)(\/?:\.(?<format>[^\/.?]+))?\Z/

(notice that the middle \/ part is now inside of the brackets)

This way it will match on the normalized path. Any obligations? If not I will just do so and send you guys a pull request once that is done.

Cheers

@jch
Copy link
Contributor Author

jch commented Nov 14, 2012

@walski great sleuthing! I'd love to see a PR for this, especially if you could have tests for both cases (trailing and non-trailing slash).

@walski walski mentioned this issue Nov 14, 2012
walski added a commit to walski/grape that referenced this issue Nov 14, 2012
@dblock dblock closed this as completed in f1856a4 Nov 14, 2012
dblock added a commit that referenced this issue Nov 14, 2012
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