Grape recognize_path incorrectly recognizes path

[glebsonik]

Problem

I faced an issue with path recognizing using recognize_path from Grape::API ancestor, here is an example of the API structure I have

class Books < Grape::API
  resource :books do
    route_param :id, type: Integer do
      # GET /books/:id
      get do
        #...
      end
    end

    resource :share do
      # POST /books/share
      post do
      # ....
      end
    end
  end
end

If I try to use Books.recognize_path('/books/share') Then it will recognize it as GET /books/:id request instead of the POST /books/share

I guess this happens because the recognize_path does not rely on the request method. In this case Grape recognizer treats share as :id param instead of path

[dblock]

Do you think you can you turn this into a failing test?

[glebsonik]

If there is a way to predefine two routes or somehow simulate this then yes. Where should I set up this test?

UPD: I've taken a look at the .recognize_path specs in the repo and was able to quickly reproduce an error. Where should I implement this test?

Anyway, I'll just place it here for now until I understand what is the better way to share it

# frozen_string_literal: true

require 'rails_helper'

describe Grape::API do
  describe '.recognize_path' do
    subject { Class.new(described_class) }

    context 'given parametrized route and static route' do
      it 'recognizes it as static' do
        subject.get('/foo/:id') {}
        subject.post('/foo/share') {}

        actual = subject.recognize_path('/foo/share').routes[0].origin
        expect(actual).to eq('/foo/share')
      end
    end
  end
end

[dblock]

You can make a pull request on top of the existing specs, so

grape/spec/grape/api/recognize_path_spec.rb

Line 4 in d390315

describe '.recognize_path' do

.

Looking at the problem though, I wonder if this is a bug at all. AFAIK routes in Grape are matched in order of declaration, not like Ruby methods where the last one wins - it's the first one that matches. Amazingly, I actually couldn't find anything on this in our README, and I think we need to state it unambiguously in it. In fact, there's #1858 that asks for the same. Want to take it on? I would add recognize_path specs for all combinations (your example, nested routes, inherited APIs, mounted APIs) and add a section to README about the order of matching.

[dblock]

@glebsonik Btw, in your specific example you have a type: Integer in the first route, and so it does make sense that it shouldn't match in theory, so could be a bug anyway.

[glebsonik]

@dblock I've done a commit but I can neither create a PR to the grape repo nor push my changes. I think, I just don't have access to do this. Is there any other way I should address my PR?

I cloned the repo and checked out a new branch from the master

[glebsonik]

Okay nvm, I've just created PR in the fork #2352

[jcagarcia]

Hi 👋

I was taking a look at this issue, and as the first step I can add a section in the README talking about the order of matching, as you suggested in your comment above @dblock. Agree?

On the other hand, I find this issue pretty interesting, as it's not the first time I need to change the order of my endpoints to make them work. So I was trying to debug the issue following the tests added by @glebsonik in #2352.

Seems that the problem is that the regular expression that Mustermann generates is not taking into account the type of the parameter. This is the current regular expression generated by Mustermann::Grape for the /books/:id pattern:

/(?-mix:\A(?-mix:\/(?:b|%62)(?:o|%6f|%6F)(?:o|%6f|%6F)(?:k|%6b|%6B)(?:s|%73)\/(?<id>(?:(?!(?:\.|%2e|%2E)(?:(?!(?:\.|%2e|%2E))[^\/\?#.])+?$)[^\/\?#.])+)(?:(?:\.|%2e|%2E)(?<format>[^\/\?#.]+))?)\Z)/

This regular expression should only match if the content of the :id group are digits, but is not the case as you can see in this quick irb check:

3.2.2 :002 > "/books/1".match?(regex)
 => true
3.2.2 :003 > "/books/share".match?(regex)
 => true
3.2.2 :004 >

If I manually change the regular expression generated for the /books/:id endpoint for matching only if the id group are digits:

/(?-mix:\A(?-mix:\/(?:b|%62)(?:o|%6f|%6F)(?:o|%6f|%6F)(?:k|%6b|%6B)(?:s|%73)\/(?<id>\d?+)(?:(?:\.|%2e|%2E)(?<format>[^\/\?#.]+))?)\Z)/

You can see that the /books/share pattern does not match with it:

3.2.2 :002 > "/books/1".match?(regex)
 => true
3.2.2 :003 > "/books/share".match?(regex)
 => false
3.2.2 :004 >

So I was trying to see the options that we are sending to instantiate the Mustermann::Grape object, and these are:

Mustermann::Grape.new("/books/:id(.:format)", {:uri_decode=>true})

However, the options received when initialising the pattern class contains much more interesting information that can be really useful for generating a much more precise regular expression.

{:params=>{"id"=>{:required=>true, :type=>"Integer"}}, :namespace=>"/books/:id", :version=>nil, :requirements=>{}, :prefix=>nil, :anchor=>true, :settings=>{}, :forward_match=>nil, :suffix=>"(.:format)"}

I can see that Grape is using its own Mustermann pattern , so don't know if adding extra information to the pattern options we can generate much more precise regular expression, or if we need to update the Mustermann::Grape implementation for allowing to receive those parameters. Also, maybe is not a problem from Mustermann::Grape and we need to check if Mustermann can generate this more precise regex by itself.

What do you think @dblock ? Any suggestion for start digging on it?

Thanks!

[dblock]

I was taking a look at this issue, and as the first step I can add a section in the README talking about the order of matching, as you suggested in your comment above @dblock. Agree?

Yes.

I can see that Grape is using its own Mustermann pattern , so don't know if adding extra information to the pattern options we can generate much more precise regular expression, or if we need to update the Mustermann::Grape implementation for allowing to receive those parameters. Also, maybe is not a problem from Mustermann::Grape and we need to check if Mustermann can generate this more precise regex by itself.

What do you think @dblock ? Any suggestion for start digging on it?

This is a bit over my head, by now you understand way more than me of what's happening here ;) @namusyaka though might have an opinion.

I do see that musterman-grape hasn't been updated for a while, generally a good place to contribute!

[jcagarcia]

Hey @dblock @namusyaka ,

I've added a PR with the proper changes in the mustermann-grape project for being able to provide the params information to the Mustermann::Grape instance and to obtain a more precise regular expression that can solve this problem.

Can you take a look? ruby-grape/mustermann-grape#21

After that, and if you consider this change is appropiated, I would continue passing the params when initialising the Mustermann::Grape class, so we will be able to pass the red suite provided by @glebsoni

EDIT: I've just ran the tests provided by @glebsonik using my local gem mustermann-grape that includes the last changes and the tests are passing! 🥳 I'm also including extra test to verify that everything continue working as expected.

Comments, suggestions and feedback are totally welcome.

[jcagarcia]

Solved in #2379