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

Do not shadow helpers with same method but more params #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mdelaossa
Copy link

@mdelaossa mdelaossa commented Dec 20, 2017

When two helper methods are the same, but have different arguments (for example: api_v1_cats_owners_path(id: 1) vs api_v1_cats_owners_path(id: 1, owner_id: 2)) if the helper method with the least number of arguments is defined first (because the route was defined first) then it will shadow the longer route.

Example:
Consider the following routes:

      resource :cats do
        get '/' do
          %w(cats cats cats)
        end

        route_param :id do
          get do
            'cat'
          end
        end

        get ':id/owners' do
          %w(owner1 owner2)
        end

        get ':id/owners/:owner_id' do
          'owner'
        end
      end

The helper methods return:

api_v1_cats_owners_path(id: 1) #=> '/api/v1/cats/1/owners.json'
api_v1_cats_owners_path(id: 1, owner_id: 2) #=> '/api/v1/cats/1/owners.json'

which is wrong. api_v1_cats_owners_path(id: 1, owner_id: 2) should return '/api/v1/cats/1/owners/2.json'

This PR fixes this issue in the simplest possible way: when GrapeRouteHelpers::AllRoutes is generating @decorated_routes, it sorts the routes in descending order by the count of dynamic_path_segments it has.

Fixes #22

@mdelaossa
Copy link
Author

BTW your travis build is failing on jruby-19 due to

Gem::Exception: can't find executable rake for gem rake. rake is not currently included in the bundle, perhaps you meant to add it to your Gemfile?

markglenfletcher pushed a commit to gitlabhq/gitlabhq that referenced this pull request Feb 7, 2018
Bringing in reprah/grape-route-helpers#21 as a
monkey patch since the grape-route-helpers project seems to be abandoned
mayra-cabrera pushed a commit to gitlabhq/gitlabhq that referenced this pull request May 30, 2018
This gem (https://gitlab.com/gitlab-org/grape-path-helpers) makes a number of changes:

1. Brings in @mdelaossa's changes in reprah/grape-route-helpers#21
2. Fixes some broken specs and code for Grape 1.0+
3. Optimizes the generation of paths by bringing in @dblessing's
   HashWithIndifferentAccess changes in https://gitlab.com/gitlab-org/gitlab-ce/issues/45718#note_70123793

Closes #45718
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.

Searching for helper stops with first possible match
1 participant