Commit 990af4fb authored by Stan Hu's avatar Stan Hu

Replace grape-route-helpers with our own grape-path-helpers

This gem (https://gitlab.com/gitlab-org/grape-path-helpers) makes a number of changes:

1. Brings in @mdelaossa's changes in https://github.com/reprah/grape-route-helpers/pull/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
parent 5b1416aa
...@@ -28,7 +28,7 @@ gem 'mysql2', '~> 0.4.10', group: :mysql ...@@ -28,7 +28,7 @@ gem 'mysql2', '~> 0.4.10', group: :mysql
gem 'pg', '~> 0.18.2', group: :postgres gem 'pg', '~> 0.18.2', group: :postgres
gem 'rugged', '~> 0.27' gem 'rugged', '~> 0.27'
gem 'grape-route-helpers', '~> 2.1.0' gem 'grape-path-helpers', '~> 1.0'
gem 'faraday', '~> 0.12' gem 'faraday', '~> 0.12'
......
...@@ -348,7 +348,7 @@ GEM ...@@ -348,7 +348,7 @@ GEM
signet (~> 0.7) signet (~> 0.7)
gpgme (2.0.13) gpgme (2.0.13)
mini_portile2 (~> 2.1) mini_portile2 (~> 2.1)
grape (1.0.2) grape (1.0.3)
activesupport activesupport
builder builder
mustermann-grape (~> 1.0.0) mustermann-grape (~> 1.0.0)
...@@ -358,10 +358,10 @@ GEM ...@@ -358,10 +358,10 @@ GEM
grape-entity (0.7.1) grape-entity (0.7.1)
activesupport (>= 4.0) activesupport (>= 4.0)
multi_json (>= 1.3.2) multi_json (>= 1.3.2)
grape-route-helpers (2.1.0) grape-path-helpers (1.0.1)
activesupport activesupport (~> 4)
grape (>= 0.16.0) grape (~> 1.0)
rake rake (~> 12)
grape_logging (1.7.0) grape_logging (1.7.0)
grape grape
grpc (1.11.0) grpc (1.11.0)
...@@ -1049,7 +1049,7 @@ DEPENDENCIES ...@@ -1049,7 +1049,7 @@ DEPENDENCIES
gpgme gpgme
grape (~> 1.0) grape (~> 1.0)
grape-entity (~> 0.7.1) grape-entity (~> 0.7.1)
grape-route-helpers (~> 2.1.0) grape-path-helpers (~> 1.0)
grape_logging (~> 1.7) grape_logging (~> 1.7)
grpc (~> 1.11.0) grpc (~> 1.11.0)
haml_lint (~> 0.26.0) haml_lint (~> 0.26.0)
......
---
title: Replace grape-route-helpers with our own grape-path-helpers
merge_request:
author:
type: performance
if defined?(GrapeRouteHelpers)
module GrapeRouteHelpers
module AllRoutes
# Bringing in PR https://github.com/reprah/grape-route-helpers/pull/21 due to abandonment.
#
# Without the following fix, 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.
#
# The fix is to sort descending by amount of arguments
def decorated_routes
@decorated_routes ||= all_routes
.map { |r| DecoratedRoute.new(r) }
.sort_by { |r| -r.dynamic_path_segments.count }
end
end
class DecoratedRoute
# GrapeRouteHelpers gem tries to parse the versions
# from a string, not supporting Grape `version` array definition.
#
# Without the following fix, we get this on route helpers generation:
#
# => undefined method `scan' for ["v3", "v4"]
#
# 2.0.0 implementation of this method:
#
# ```
# def route_versions
# version_pattern = /[^\[",\]\s]+/
# if route_version
# route_version.scan(version_pattern)
# else
# [nil]
# end
# end
# ```
def route_versions
return [nil] if route_version.nil? || route_version.empty?
if route_version.is_a?(String)
version_pattern = /[^\[",\]\s]+/
route_version.scan(version_pattern)
else
route_version
end
end
end
end
end
module API module API
module Helpers module Helpers
module RelatedResourcesHelpers module RelatedResourcesHelpers
include GrapeRouteHelpers::NamedRouteMatcher include GrapePathHelpers::NamedRouteMatcher
def issues_available?(project, options) def issues_available?(project, options)
available?(:issues, project, options[:current_user]) available?(:issues, project, options[:current_user])
......
require 'spec_helper'
require_relative '../../config/initializers/grape_route_helpers_fix'
describe 'route shadowing' do
include GrapeRouteHelpers::NamedRouteMatcher
it 'does not occur' do
path = api_v4_projects_merge_requests_path(id: 1)
expect(path).to eq('/api/v4/projects/1/merge_requests')
path = api_v4_projects_merge_requests_path(id: 1, merge_request_iid: 3)
expect(path).to eq('/api/v4/projects/1/merge_requests/3')
end
end
...@@ -29,6 +29,18 @@ describe API::MergeRequests do ...@@ -29,6 +29,18 @@ describe API::MergeRequests do
project.add_reporter(user) project.add_reporter(user)
end end
describe 'route shadowing' do
include GrapePathHelpers::NamedRouteMatcher
it 'does not occur' do
path = api_v4_projects_merge_requests_path(id: 1)
expect(path).to eq('/api/v4/projects/1/merge_requests')
path = api_v4_projects_merge_requests_path(id: 1, merge_request_iid: 3)
expect(path).to eq('/api/v4/projects/1/merge_requests/3')
end
end
describe 'GET /merge_requests' do describe 'GET /merge_requests' do
context 'when unauthenticated' do context 'when unauthenticated' do
it 'returns an array of all merge requests' do it 'returns an array of all merge requests' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment