Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # 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